Re: GIT, libcurl and GSS-Negotiate
On Fri, May 16, 2014 at 10:34:10PM +, brian m. carlson wrote:
> > The tricky part is figuring out when to return HTTP_NOAUTH ("do not try
> > again, we failed") versus HTTP_REAUTH ("get credentials and try again")
> > in handle_curl_result. Right now the decision is based on "did we have a
> > username and password for this request?" I'm not clear on what extra
> > bits would be needed to decide to continue in the case you guys are
> > discussing.
>
> I'm honestly not sure, either. That's why I said, "how to do that is
> unknown".
>
> However, if you base64-decode the two Negotiate replies in the
> successful attempt with WinHTTP and pass it through od -tc, you'll see
> that the second reply contains some form of user ID that the first one
> does not. The curl binary sends an identical reply for the first pass,
> but then gives up and does not try a second pass. I don't know if
> libcurl is able to provide the data required in the second pass.
>
> All of this is way outside my knowledge, since my Kerberos/GSSAPI
> Negotiate requests look very different than the NTLM ones.
Mine too. I think a good place to start would be somebody who has a
setup to replicate the problem dumping the curl data (either from
GIT_CURL_VERBOSE, or instrumenting git with some curl_easy_getinfo
calls), and seeing what is interesting.
> > Yeah, we just set CURLAUTH_ANY now, but it would be fairly trivial to
> > add "http.authtype" and "http.proxyauthtype" to map to CURLOPT_HTTPAUTH
> > and CURLOPT_PROXYAUTH.
>
> This might be the easiest option.
I can help with working up a patch, but I don't have any meaningful way
to test it.
The patch below might help somebody get started. I don't know if
CURLAUTH_ONLY would be useful to be able to set, too.
diff --git a/http.c b/http.c
index 94e1afd..ba56f7e 100644
--- a/http.c
+++ b/http.c
@@ -51,6 +51,9 @@ struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
static const char *user_agent;
+static long curl_http_authtype;
+static long curl_proxy_authtype;
+
#if LIBCURL_VERSION_NUM >= 0x071700
/* Use CURLOPT_KEYPASSWD as is */
#elif LIBCURL_VERSION_NUM >= 0x070903
@@ -143,6 +146,37 @@ static void process_curl_messages(void)
}
#endif
+static int parse_auth_type(const char *var, const char *value, long *type)
+{
+ static struct {
+ const char *name;
+ long value;
+ } types[] = {
+{ "basic", CURLAUTH_BASIC },
+{ "digest", CURLAUTH_DIGEST },
+#if CURL_VERSION >= 0x071303
+{ "digest-ie", CURLAUTH_DIGEST_IE },
+#endif
+{ "negotiate", CURLAUTH_GSSNEGOTIATE },
+#if CURL_VERSION >= 0x071600
+{ "ntlm-wb", CURLAUTH_NTLM_WB },
+#endif
+{ "ntlm", CURLAUTH_NTLM }
+ };
+ int i;
+
+ if (!value)
+ return config_error_nonbool(var);
+
+ for (i = 0; i < ARRAY_SIZE(types); i++)
+ if (!strcmp(value, types[i].name))
+ *type |= types[i].value;
+
+ if (i == ARRAY_SIZE(types))
+ return error("unknown auth type for '%s': %s", var, value);
+ return 0;
+}
+
static int http_options(const char *var, const char *value, void *cb)
{
if (!strcmp("http.sslverify", var)) {
@@ -216,6 +250,11 @@ static int http_options(const char *var, const char
*value, void *cb)
if (!strcmp("http.useragent", var))
return git_config_string(&user_agent, var, value);
+ if (!strcmp("http.authtype", var))
+ return parse_auth_type(var, value, &curl_http_authtype);
+ if (!strcmp("http.proxyauthtype", var))
+ return parse_auth_type(var, value, &curl_proxy_authtype);
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
}
@@ -296,6 +335,17 @@ static void set_curl_keepalive(CURL *c)
}
#endif
+static void set_curl_authtype(CURL *c, CURLoption option, long value)
+{
+ if (value)
+ curl_easy_setopt(c, option, value);
+ else {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+ curl_easy_setopt(c, option, CURLAUTH_ANY);
+#endif
+ }
+}
+
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
@@ -313,8 +363,13 @@ static CURL *get_curl_handle(void)
#if LIBCURL_VERSION_NUM >= 0x070907
curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
#endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
- curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
+
+#if LIBCURL_VERSION_NUM >= 0x070a06
+ set_curl_authtype(result, CURLOPT_HTTPAUTH, curl_http_authtype);
+#endif
+
+#if LIBCURL_VERSION_NUM >= 0x070a07
+ set_curl_authtype(result, CURLOPT_PROXYAUTH, curl_proxy_authtype);
#endif
if (http_proactive_auth)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/major
[PATCH v2 09/10] replace: add --edit to usage string
Signed-off-by: Christian Couder
---
builtin/replace.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/replace.c b/builtin/replace.c
index 4ee3d92..1bb491d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -16,6 +16,7 @@
static const char * const git_replace_usage[] = {
N_("git replace [-f] "),
+ N_("git replace [-f] --edit "),
N_("git replace -d ..."),
N_("git replace [--format=] [-l []]"),
NULL
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/10] Documentation: replace: describe new --edit option
Signed-off-by: Christian Couder --- Documentation/git-replace.txt | 15 ++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt index 0a02f70..37d872d 100644 --- a/Documentation/git-replace.txt +++ b/Documentation/git-replace.txt @@ -9,6 +9,7 @@ SYNOPSIS [verse] 'git replace' [-f] +'git replace' [-f] --edit 'git replace' -d ... 'git replace' [--format=] [-l []] @@ -63,6 +64,14 @@ OPTIONS --delete:: Delete existing replace refs for the given objects. +--edit :: + Launch an editor to let the user change the content of + , then create a new object of the same type with the + changed content, and create a replace ref to replace + with the new object. See linkgit:git-commit[1] and + linkgit:git-var[1] for details about how the editor will be + chosen. + -l :: --list :: List replace refs for objects that match the given pattern (or @@ -92,7 +101,9 @@ CREATING REPLACEMENT OBJECTS linkgit:git-filter-branch[1], linkgit:git-hash-object[1] and linkgit:git-rebase[1], among other git commands, can be used to create -replacement objects from existing objects. +replacement objects from existing objects. The `--edit` option can +also be used with 'git replace' to create a replacement object by +editing an existing object. If you want to replace many blobs, trees or commits that are part of a string of commits, you may just want to create a replacement string of @@ -117,6 +128,8 @@ linkgit:git-filter-branch[1] linkgit:git-rebase[1] linkgit:git-tag[1] linkgit:git-branch[1] +linkgit:git-commit[1] +linkgit:git-var[1] linkgit:git[1] GIT -- 1.9.rc0.17.g651113e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/10] replace: use OPT_CMDMODE to handle modes
From: Jeff King
By using OPT_CMDMODE, the mutual exclusion between modes is
taken care of for us. It also makes it easy for us to
maintain a single variable with the mode, which makes its
intent more clear. We can use a single switch() to make sure
we have covered all of the modes.
This ends up breaking even in code size, but the win will be
much bigger when we start adding more modes.
Signed-off-by: Jeff King
Signed-off-by: Christian Couder
---
builtin/replace.c | 49 +
1 file changed, 25 insertions(+), 24 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 28db96f..29cf699 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -168,11 +168,17 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
int cmd_replace(int argc, const char **argv, const char *prefix)
{
- int list = 0, delete = 0, force = 0;
+ int force = 0;
const char *format = NULL;
+ enum {
+ MODE_UNSPECIFIED = 0,
+ MODE_LIST,
+ MODE_DELETE,
+ MODE_REPLACE
+ } cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
- OPT_BOOL('l', "list", &list, N_("list replace refs")),
- OPT_BOOL('d', "delete", &delete, N_("delete replace refs")),
+ OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"),
MODE_LIST),
+ OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"),
MODE_DELETE),
OPT_BOOL('f', "force", &force, N_("replace the ref if it
exists")),
OPT_STRING(0, "format", &format, N_("format"), N_("use this
format")),
OPT_END()
@@ -182,42 +188,37 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
- if (!list && !delete)
- if (!argc)
- list = 1;
+ if (!cmdmode)
+ cmdmode = argc ? MODE_REPLACE : MODE_LIST;
- if (list && delete)
- usage_msg_opt("-l and -d cannot be used together",
- git_replace_usage, options);
-
- if (format && !list)
+ if (format && cmdmode != MODE_LIST)
usage_msg_opt("--format cannot be used when not listing",
git_replace_usage, options);
- if (force && (list || delete))
- usage_msg_opt("-f cannot be used with -d or -l",
+ if (force && cmdmode != MODE_REPLACE)
+ usage_msg_opt("-f only makes sense when writing a replacement",
git_replace_usage, options);
- /* Delete refs */
- if (delete) {
+ switch (cmdmode) {
+ case MODE_DELETE:
if (argc < 1)
usage_msg_opt("-d needs at least one argument",
git_replace_usage, options);
return for_each_replace_name(argv, delete_replace_ref);
- }
- /* Replace object */
- if (!list && argc) {
+ case MODE_REPLACE:
if (argc != 2)
usage_msg_opt("bad number of arguments",
git_replace_usage, options);
return replace_object(argv[0], argv[1], force);
- }
- /* List refs, even if "list" is not set */
- if (argc > 1)
- usage_msg_opt("only one pattern can be given with -l",
- git_replace_usage, options);
+ case MODE_LIST:
+ if (argc > 1)
+ usage_msg_opt("only one pattern can be given with -l",
+ git_replace_usage, options);
+ return list_replace_refs(argv[0], format);
- return list_replace_refs(argv[0], format);
+ default:
+ die("BUG: invalid cmdmode %d", (int)cmdmode);
+ }
}
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/10] replace: refactor command-mode determination
From: Jeff King
The git-replace command has three modes: listing, deleting,
and replacing. The first two are selected explicitly. If
none is selected, we fallback to listing when there are no
arguments, and replacing otherwise.
Let's figure out up front which operation we are going to
do, before getting into the application logic. That lets us
simplify our option checks (e.g., we currently have to check
whether a useless "--force" is given both along with an
explicit list, as well as with an implicit one).
This saves some lines, makes the logic easier to follow, and
will facilitate further cleanups.
Signed-off-by: Jeff King
Signed-off-by: Christian Couder
---
builtin/replace.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..28db96f 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -182,12 +182,16 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
+ if (!list && !delete)
+ if (!argc)
+ list = 1;
+
if (list && delete)
usage_msg_opt("-l and -d cannot be used together",
git_replace_usage, options);
- if (format && delete)
- usage_msg_opt("--format and -d cannot be used together",
+ if (format && !list)
+ usage_msg_opt("--format cannot be used when not listing",
git_replace_usage, options);
if (force && (list || delete))
@@ -207,9 +211,6 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
if (argc != 2)
usage_msg_opt("bad number of arguments",
git_replace_usage, options);
- if (format)
- usage_msg_opt("--format cannot be used when not
listing",
- git_replace_usage, options);
return replace_object(argv[0], argv[1], force);
}
@@ -217,9 +218,6 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
if (argc > 1)
usage_msg_opt("only one pattern can be given with -l",
git_replace_usage, options);
- if (force)
- usage_msg_opt("-f needs some arguments",
- git_replace_usage, options);
return list_replace_refs(argv[0], format);
}
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 08/10] replace: add tests for --edit
Signed-off-by: Christian Couder --- t/t6050-replace.sh | 29 + 1 file changed, 29 insertions(+) diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index 719a116..7609174 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -318,6 +318,35 @@ test_expect_success 'test --format long' ' test_cmp expected actual ' +test_expect_success 'setup a fake editor' ' + cat >fakeeditor <<-\EOF && + #!/bin/sh + sed -e "s/A U Thor/A fake Thor/" "$1" >"$1.new" + mv "$1.new" "$1" + EOF + chmod +x fakeeditor +' + +test_expect_success '--edit with and without already replaced object' ' + GIT_EDITOR=./fakeeditor test_must_fail git replace --edit "$PARA3" && + GIT_EDITOR=./fakeeditor git replace --force --edit "$PARA3" && + git replace -l | grep "$PARA3" && + git cat-file commit "$PARA3" | grep "A fake Thor" && + git replace -d "$PARA3" && + GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" && + git replace -l | grep "$PARA3" && + git cat-file commit "$PARA3" | grep "A fake Thor" +' + +test_expect_success '--edit and change nothing or command failed' ' + git replace -d "$PARA3" && + GIT_EDITOR=true test_must_fail git replace --edit "$PARA3" && + GIT_EDITOR="./fakeeditor;false" test_must_fail git replace --edit "$PARA3" && + GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" && + git replace -l | grep "$PARA3" && + git cat-file commit "$PARA3" | grep "A fake Thor" +' + test_expect_success 'replace ref cleanup' ' test -n "$(git replace)" && git replace -d $(git replace) && -- 1.9.rc0.17.g651113e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/10] replace: add option to edit a Git object
This patch series comes from what Peff sent in the following thread: http://thread.gmane.org/gmane.comp.version-control.git/243361/focus=243528 The first 4 patches (1/4, 2/4, 3/4 and 4/4) didn't change since v1. I added 6 more small patches to add tests, documentation and a few small improvements. Christian Couder (6): replace: make sure --edit results in a different object replace: refactor checking ref validity replace: die early if replace ref already exists replace: add tests for --edit replace: add --edit to usage string Documentation: replace: describe new --edit option Jeff King (4): replace: refactor command-mode determination replace: use OPT_CMDMODE to handle modes replace: factor object resolution out of replace_object replace: add --edit option Documentation/git-replace.txt | 15 ++- builtin/replace.c | 225 +- t/t6050-replace.sh| 29 ++ 3 files changed, 223 insertions(+), 46 deletions(-) -- 1.9.rc0.17.g651113e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/10] replace: make sure --edit results in a different object
It's a bad idea to create a replace ref for an object
that points to the original object itself.
That's why we have to check if the result from editing
the original object is a different object and error out
if it isn't.
Signed-off-by: Christian Couder
---
builtin/replace.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..0751804 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -275,6 +275,9 @@ static int edit_and_replace(const char *object_ref, int
force)
free(tmpfile);
+ if (!hashcmp(old, new))
+ return error("new object is the same as the old one: '%s'",
sha1_to_hex(old));
+
return replace_object_sha1(object_ref, old, "replacement", new, force);
}
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/10] replace: factor object resolution out of replace_object
From: Jeff King
As we add new options that operate on objects before
replacing them, we'll want to be able to feed raw sha1s
straight into replace_object. Split replace_object into the
object-resolution part and the actual replacement.
Signed-off-by: Jeff King
Signed-off-by: Christian Couder
---
builtin/replace.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 29cf699..af40129 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -123,19 +123,17 @@ static int delete_replace_ref(const char *name, const
char *ref,
return 0;
}
-static int replace_object(const char *object_ref, const char *replace_ref,
- int force)
+static int replace_object_sha1(const char *object_ref,
+ unsigned char object[20],
+ const char *replace_ref,
+ unsigned char repl[20],
+ int force)
{
- unsigned char object[20], prev[20], repl[20];
+ unsigned char prev[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
struct ref_lock *lock;
- if (get_sha1(object_ref, object))
- die("Failed to resolve '%s' as a valid ref.", object_ref);
- if (get_sha1(replace_ref, repl))
- die("Failed to resolve '%s' as a valid ref.", replace_ref);
-
if (snprintf(ref, sizeof(ref),
"refs/replace/%s",
sha1_to_hex(object)) > sizeof(ref) - 1)
@@ -166,6 +164,18 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
return 0;
}
+static int replace_object(const char *object_ref, const char *replace_ref, int
force)
+{
+ unsigned char object[20], repl[20];
+
+ if (get_sha1(object_ref, object))
+ die("Failed to resolve '%s' as a valid ref.", object_ref);
+ if (get_sha1(replace_ref, repl))
+ die("Failed to resolve '%s' as a valid ref.", replace_ref);
+
+ return replace_object_sha1(object_ref, object, replace_ref, repl,
force);
+}
+
int cmd_replace(int argc, const char **argv, const char *prefix)
{
int force = 0;
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/10] replace: add --edit option
From: Jeff King
This allows you to run:
git replace --edit SHA1
to get dumped in an editor with the contents of the object
for SHA1. The result is then read back in and used as a
"replace" object for SHA1. The writing/reading is
type-aware, so you get to edit "ls-tree" output rather than
the binary tree format.
Missing documentation and tests.
Signed-off-by: Jeff King
Signed-off-by: Christian Couder
---
builtin/replace.c | 112 +-
1 file changed, 111 insertions(+), 1 deletion(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index af40129..3da1bae 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -12,6 +12,7 @@
#include "builtin.h"
#include "refs.h"
#include "parse-options.h"
+#include "run-command.h"
static const char * const git_replace_usage[] = {
N_("git replace [-f] "),
@@ -176,6 +177,107 @@ static int replace_object(const char *object_ref, const
char *replace_ref, int f
return replace_object_sha1(object_ref, object, replace_ref, repl,
force);
}
+/*
+ * Write the contents of the object named by "sha1" to the file "filename",
+ * pretty-printed for human editing based on its type.
+ */
+static void export_object(const unsigned char *sha1, const char *filename)
+{
+ const char *argv[] = { "--no-replace-objects", "cat-file", "-p", NULL,
NULL };
+ struct child_process cmd = { argv };
+ int fd;
+
+ fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+ if (fd < 0)
+ die_errno("unable to open %s for writing", filename);
+
+ argv[3] = sha1_to_hex(sha1);
+ cmd.git_cmd = 1;
+ cmd.out = fd;
+
+ if (run_command(&cmd))
+ die("cat-file reported failure");
+
+ close(fd);
+}
+
+/*
+ * Read a previously-exported (and possibly edited) object back from
"filename",
+ * interpreting it as "type", and writing the result to the object database.
+ * The sha1 of the written object is returned via sha1.
+ */
+static void import_object(unsigned char *sha1, enum object_type type,
+ const char *filename)
+{
+ int fd;
+
+ fd = open(filename, O_RDONLY);
+ if (fd < 0)
+ die_errno("unable to open %s for reading", filename);
+
+ if (type == OBJ_TREE) {
+ const char *argv[] = { "mktree", NULL };
+ struct child_process cmd = { argv };
+ struct strbuf result = STRBUF_INIT;
+
+ cmd.argv = argv;
+ cmd.git_cmd = 1;
+ cmd.in = fd;
+ cmd.out = -1;
+
+ if (start_command(&cmd))
+ die("unable to spawn mktree");
+
+ if (strbuf_read(&result, cmd.out, 41) < 0)
+ die_errno("unable to read from mktree");
+ close(cmd.out);
+
+ if (finish_command(&cmd))
+ die("mktree reported failure");
+ if (get_sha1_hex(result.buf, sha1) < 0)
+ die("mktree did not return an object name");
+
+ strbuf_release(&result);
+ } else {
+ struct stat st;
+ int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;
+
+ if (fstat(fd, &st) < 0)
+ die_errno("unable to fstat %s", filename);
+ if (index_fd(sha1, fd, &st, type, NULL, flags) < 0)
+ die("unable to write object to database");
+ /* index_fd close()s fd for us */
+ }
+
+ /*
+* No need to close(fd) here; both run-command and index-fd
+* will have done it for us.
+*/
+}
+
+static int edit_and_replace(const char *object_ref, int force)
+{
+ char *tmpfile = git_pathdup("REPLACE_EDITOBJ");
+ enum object_type type;
+ unsigned char old[20], new[20];
+
+ if (get_sha1(object_ref, old) < 0)
+ die("Not a valid object name: '%s'", object_ref);
+
+ type = sha1_object_info(old, NULL);
+ if (type < 0)
+ die("unable to get object type for %s", sha1_to_hex(old));
+
+ export_object(old, tmpfile);
+ if (launch_editor(tmpfile, NULL, NULL) < 0)
+ die("editing object file failed");
+ import_object(new, type, tmpfile);
+
+ free(tmpfile);
+
+ return replace_object_sha1(object_ref, old, "replacement", new, force);
+}
+
int cmd_replace(int argc, const char **argv, const char *prefix)
{
int force = 0;
@@ -184,11 +286,13 @@ int cmd_replace(int argc, const char **argv, const char
*prefix)
MODE_UNSPECIFIED = 0,
MODE_LIST,
MODE_DELETE,
+ MODE_EDIT,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"),
MODE_LIST),
OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replac
[PATCH v2 06/10] replace: refactor checking ref validity
This will be useful in a following commit when we will
want to check if the ref already exists before we let the
user edit an object.
Signed-off-by: Christian Couder
---
builtin/replace.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 0751804..3d6edaf 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -124,6 +124,25 @@ static int delete_replace_ref(const char *name, const char
*ref,
return 0;
}
+static void check_ref_valid(unsigned char object[20],
+ unsigned char prev[20],
+ char *ref,
+ int ref_size,
+ int force)
+{
+ if (snprintf(ref, ref_size,
+"refs/replace/%s",
+sha1_to_hex(object)) > ref_size - 1)
+ die("replace ref name too long: %.*s...", 50, ref);
+ if (check_refname_format(ref, 0))
+ die("'%s' is not a valid ref name.", ref);
+
+ if (read_ref(ref, prev))
+ hashclr(prev);
+ else if (!force)
+ die("replace ref '%s' already exists", ref);
+}
+
static int replace_object_sha1(const char *object_ref,
unsigned char object[20],
const char *replace_ref,
@@ -135,13 +154,6 @@ static int replace_object_sha1(const char *object_ref,
char ref[PATH_MAX];
struct ref_lock *lock;
- if (snprintf(ref, sizeof(ref),
-"refs/replace/%s",
-sha1_to_hex(object)) > sizeof(ref) - 1)
- die("replace ref name too long: %.*s...", 50, ref);
- if (check_refname_format(ref, 0))
- die("'%s' is not a valid ref name.", ref);
-
obj_type = sha1_object_info(object, NULL);
repl_type = sha1_object_info(repl, NULL);
if (!force && obj_type != repl_type)
@@ -151,10 +163,7 @@ static int replace_object_sha1(const char *object_ref,
object_ref, typename(obj_type),
replace_ref, typename(repl_type));
- if (read_ref(ref, prev))
- hashclr(prev);
- else if (!force)
- die("replace ref '%s' already exists", ref);
+ check_ref_valid(object, prev, ref, sizeof(ref), force);
lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/10] replace: die early if replace ref already exists
If a replace ref already exists for an object, it is
much better for the user if we error out before we
let the user edit the object, rather than after.
Signed-off-by: Christian Couder
---
builtin/replace.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 3d6edaf..4ee3d92 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -268,7 +268,8 @@ static int edit_and_replace(const char *object_ref, int
force)
{
char *tmpfile = git_pathdup("REPLACE_EDITOBJ");
enum object_type type;
- unsigned char old[20], new[20];
+ unsigned char old[20], new[20], prev[20];
+ char ref[PATH_MAX];
if (get_sha1(object_ref, old) < 0)
die("Not a valid object name: '%s'", object_ref);
@@ -277,6 +278,8 @@ static int edit_and_replace(const char *object_ref, int
force)
if (type < 0)
die("unable to get object type for %s", sha1_to_hex(old));
+ check_ref_valid(old, prev, ref, sizeof(ref), force);
+
export_object(old, tmpfile);
if (launch_editor(tmpfile, NULL, NULL) < 0)
die("editing object file failed");
--
1.9.rc0.17.g651113e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
On Sat, May 17, 2014 at 12:25:30AM -0500, Felipe Contreras wrote: > > I agree with the line of reasoning you laid out in your email, > > especially: > > What a shock. Please stop with these unproductive and rude comments. > > I hadn't thought of the rename idea, and it would address the concerns I > > brought up. I do think "obsolete" is the wrong word, as it sends the > > wrong message. The helpers are not obsolete; it is our _copy_ of them > > that is. > > Originally you said you would respect if I wanted the code out > for v2.0, I said I would like it out at some point, not necessarily in > v2.0. Junio said he was fine with that, but the proposals above don't do > that. > > Now it seems you are changing your mind and you are OK with the code > remaining in. My concerns were with people not noticing the README. Removing the code entirely is the way I thought of to address that. Junio suggested another way, which I would also be fine with. And it seems like a friendlier way than removal to handle it for v2.0, if we are going to remove the code entirely post-v2.0. As before, if your desire is to have the code out for v2.0, then say so. I think we should respect such a request. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
Jeff King wrote: > I agree with the line of reasoning you laid out in your email, > especially: What a shock. > > I would say that the options I see are these three, and I would rank > > the "warn every time" as less helpful to end-users: > > > > - rename contrib/remote-helpers to contrib/obsolete-remote-helpers > >and add README to point at the upstream. > > > > - remove contrib/remote-helpers scripts and add README. > > > > - warn every time the user runs the scripts. > > I hadn't thought of the rename idea, and it would address the concerns I > brought up. I do think "obsolete" is the wrong word, as it sends the > wrong message. The helpers are not obsolete; it is our _copy_ of them > that is. Originally you said you would respect if I wanted the code out for v2.0, I said I would like it out at some point, not necessarily in v2.0. Junio said he was fine with that, but the proposals above don't do that. Now it seems you are changing your mind and you are OK with the code remaining in. Do what you will, but I already told you what I will do in response. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
James Denholm wrote: > On Fri, May 16, 2014 at 05:39:42PM -0500, Felipe Contreras wrote: > > (...) I would venture to say you have never made a package in your > > life. > > And you have, Felipe? Let us see the years of experience you surely have > in the field. As a matter of fact, yes I've written many packages, for Debian, Fedora, ArchLinux, and others. Even Windows installers. But that's a red herring. Even if was the worst packager in history, that doesn't make Junio's decision any more correct. > > The fact that you think packagers of git would simply package > > git-remote-hg/bzr as well is pretty appalling. > > It's not an outlandish thought, in fact, I'd suggest it as probable - > provided that they find the projects to be stable and of high quality. Do you want to bet? > You, or someone else, might have to tap them on the shoulder and play > nice to _ensure_ they know about them (after all, we all know that > packagers _never_ read READMEs, do they), but you're capable of that, > I'm sure. In my experience packagers scratch their own itches, and if git-remote-hg/bzr are not their itch, I don't see why any amount of nice poking would make them package them. Some other packager would have to do it, not the Git packagers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] RelNotes/2.0.0.txt: Fix several grammar issues, notably a lack of hyphens, double quotes, or articles
Signed-off-by: Jason St. John
---
Documentation/RelNotes/2.0.0.txt | 75
1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/Documentation/RelNotes/2.0.0.txt b/Documentation/RelNotes/2.0.0.txt
index 6e628d4..e6bf9d6 100644
--- a/Documentation/RelNotes/2.0.0.txt
+++ b/Documentation/RelNotes/2.0.0.txt
@@ -44,7 +44,7 @@ with "git diff-files --diff-filter=d").
The default prefix for "git svn" has changed in Git 2.0. For a long
time, "git svn" created its remote-tracking branches directly under
refs/remotes, but it now places them under refs/remotes/origin/ unless
-it is told otherwise with its --prefix option.
+it is told otherwise with its "--prefix" option.
Updates since v1.9 series
@@ -53,7 +53,7 @@ Updates since v1.9 series
UI, Workflows & Features
* The "multi-mail" post-receive hook (in contrib/) has been updated
- to a more recent version from the upstream.
+ to a more recent version from upstream.
* "git gc --aggressive" learned "--depth" option and
"gc.aggressiveDepth" configuration variable to allow use of a less
@@ -63,12 +63,13 @@ UI, Workflows & Features
single strand-of-pearls is broken in its output.
* The "rev-parse --parseopt" mechanism used by scripted Porcelains to
- parse command line options and to give help text learned to take
+ parse command-line options and to give help text learned to take
the argv-help (the placeholder string for an option parameter,
e.g. "key-id" in "--gpg-sign=").
* The pattern to find where the function begins in C/C++ used in
- "diff" and "grep -p" has been updated to help C++ source better.
+ "diff" and "grep -p" has been updated to improve viewing C++
+ sources.
* "git rebase" learned to interpret a lone "-" as "@{-1}", the
branch that we were previously on.
@@ -79,7 +80,7 @@ UI, Workflows & Features
"--sort=version:refname".
* Discard the accumulated "heuristics" to guess from which branch the
- result wants to be pulled from and make sure what the end user
+ result wants to be pulled from and make sure that what the end user
specified is not second-guessed by "git request-pull", to avoid
mistakes. When you pushed out your 'master' branch to your public
repository as 'for-linus', use the new "master:for-linus" syntax to
@@ -88,7 +89,7 @@ UI, Workflows & Features
* "git grep" learned to behave in a way similar to native grep when
"-h" (no header) and "-c" (count) options are given.
- * "git push" via transport-helper interface (e.g. remote-hg) has
+ * "git push" via transport-helper interfaces (e.g. remote-hg) has
been updated to allow forced ref updates in a way similar to the
natively supported transports.
@@ -114,28 +115,28 @@ UI, Workflows & Features
* The progress indicators from various time-consuming commands have
been marked for i18n/l10n.
- * "git notes -C " diagnoses an attempt to use an object that
- is not a blob as an error.
+ * "git notes -C " diagnoses as an error an attempt to use an
+ object that is not a blob.
* "git config" learned to read from the standard input when "-" is
given as the value to its "--file" parameter (attempting an
- operation to update the configuration in the standard input of
- course is rejected).
+ operation to update the configuration in the standard input is
+ rejected, of course).
* Trailing whitespaces in .gitignore files, unless they are quoted
for fnmatch(3), e.g. "path\ ", are warned and ignored. Strictly
- speaking, this is a backward incompatible change, but very unlikely
+ speaking, this is a backward-incompatible change, but very unlikely
to bite any sane user and adjusting should be obvious and easy.
- * Many commands that create commits, e.g. "pull", "rebase",
- learned to take the --gpg-sign option on the command line.
+ * Many commands that create commits, e.g. "pull" and "rebase",
+ learned to take the "--gpg-sign" option on the command line.
* "git commit" can be told to always GPG sign the resulting commit
- by setting "commit.gpgsign" configuration variable to true (the
- command line option --no-gpg-sign should override it).
+ by setting the "commit.gpgsign" configuration variable to "true"
+ (the command-line option "--no-gpg-sign" should override it).
* "git pull" can be told to only accept fast-forward by setting the
- new "pull.ff" configuration.
+ new "pull.ff" configuration variable.
* "git reset" learned the "-N" option, which does not reset the index
fully for paths the index knows about but the tree-ish the command
@@ -152,7 +153,7 @@ Performance, Internal Implementation, etc.
* Uses of curl's "multi" interface and "easy" interface do not mix
well when we attempt to reuse outgoing connections. Teach the RPC
- over http code, used in the smart HTTP transport, not to use the
+ over HTTP code, used in the smart HTTP transport, not to use the
Re: [PATCH] remote-helpers: point at their upstream repositories
On Fri, May 16, 2014 at 05:39:42PM -0500, Felipe Contreras wrote: > (...) I would venture to say you have never made a package in your > life. And you have, Felipe? Let us see the years of experience you surely have in the field. > The fact that you think packagers of git would simply package > git-remote-hg/bzr as well is pretty appalling. It's not an outlandish thought, in fact, I'd suggest it as probable - provided that they find the projects to be stable and of high quality. You, or someone else, might have to tap them on the shoulder and play nice to _ensure_ they know about them (after all, we all know that packagers _never_ read READMEs, do they), but you're capable of that, I'm sure. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git-users] worlds slowest git repo- what to do?
On Sat, May 17, 2014 at 4:22 AM, John Fisher wrote: >> Probably known issues. But some elaboration would be nice (e.g. what >> operation is slow, how slow, some more detail >> characteristics of the repo..) in case new problems pop up. > > so far I have done add, commit, status, clone - commit and status are slow; > add seems to depend on the files involved, > clone seems to run at network speed. > I can provide metrics later, see above. email me offline with what you want. OK "commit -a" should be just as slow as "add", but as-is commit and status should be fast unless there are lots of files (how many in your worktree?) or we hit something that makes us look into (large) file content anyway. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] commit: allow core.commentChar=auto for character auto selection
When core.commentChar is "auto", the comment char starts with '#' as
in default but if it's already in the prepared message, find another
char in a small subset. This should stop surprises because git strips
some lines unexpectedly.
Note that git is not smart enough to recognize '#' as the comment char
in custom templates and convert it if the final comment char is
different. It thinks '#' lines in custom templates as part of the
commit message. So don't use this with custom templates.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/config.txt | 3 +++
builtin/commit.c | 32
cache.h | 1 +
config.c | 3 +++
environment.c| 1 +
t/t7502-commit.sh| 26 ++
6 files changed, 66 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..9f3ce06 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -544,6 +544,9 @@ core.commentchar::
messages consider a line that begins with this character
commented, and removes them after the editor returns
(default '#').
++
+If set to "auto", `git-commit` would select a character that is not
+the beginning character of any line in existing commit messages.
sequence.editor::
Text editor used by `git rebase -i` for editing the rebase instruction
file.
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..515c4c4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -594,6 +594,36 @@ static char *cut_ident_timestamp_part(char *string)
return ket;
}
+static void adjust_comment_line_char(const struct strbuf *sb)
+{
+ char candidates[] = "#;@!$%^&|:";
+ char *candidate;
+ const char *p;
+
+ comment_line_char = candidates[0];
+ if (!memchr(sb->buf, comment_line_char, sb->len))
+ return;
+
+ p = sb->buf;
+ candidate = strchr(candidates, *p);
+ if (candidate)
+ *candidate = ' ';
+ for (p = sb->buf; *p; p++) {
+ if ((p[0] == '\n' || p[0] == '\r') && p[1]) {
+ candidate = strchr(candidates, p[1]);
+ if (candidate)
+ *candidate = ' ';
+ }
+ }
+
+ for (p = candidates; *p == ' '; p++)
+ ;
+ if (!*p)
+ die(_("unable to select a comment character that is not used\n"
+ "in the current commit message"));
+ comment_line_char = *p;
+}
+
static int prepare_to_commit(const char *index_file, const char *prefix,
struct commit *current_head,
struct wt_status *s,
@@ -748,6 +778,8 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
die_errno(_("could not write commit template"));
+ if (auto_comment_line_char)
+ adjust_comment_line_char(&sb);
strbuf_release(&sb);
/* This checks if committer ident is explicitly given */
diff --git a/cache.h b/cache.h
index 107ac61..646fb81 100644
--- a/cache.h
+++ b/cache.h
@@ -602,6 +602,7 @@ extern int precomposed_unicode;
* that is subject to stripspace.
*/
extern char comment_line_char;
+extern int auto_comment_line_char;
enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,
diff --git a/config.c b/config.c
index 491a905..d29c733 100644
--- a/config.c
+++ b/config.c
@@ -828,8 +828,11 @@ static int git_default_core_config(const char *var, const
char *value)
int ret = git_config_string(&comment, var, value);
if (ret)
return ret;
+ else if (!strcasecmp(comment, "auto"))
+ auto_comment_line_char = 1;
else if (comment[0] && !comment[1]) {
comment_line_char = comment[0];
+ auto_comment_line_char = 0;
} else
return error("core.commentChar should only be one
character");
return 0;
diff --git a/environment.c b/environment.c
index 5c4815d..f2de1ee 100644
--- a/environment.c
+++ b/environment.c
@@ -69,6 +69,7 @@ unsigned long pack_size_limit_cfg;
* that is subject to stripspace.
*/
char comment_line_char = '#';
+int auto_comment_line_char;
/* Parallel index stat data preload? */
int core_preload_index = 0;
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 9a3f3a1..30e4688 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -563,4 +563,30 @@ test_expect_success 'commit --status with custom comment
character' '
test_i18ngrep "^; Changes to be committed:" .git/COMMIT_EDITMSG
'
+test_expect_success 'switch core.commentchar' '
+ test_commit "#foo" foo &&
+ GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend
&&
+
[PATCH v2 1/2] config: be strict on core.commentChar
We don't support comment _strings_ (at least not yet). And multi-byte
character encoding could also be misinterpreted.
The test with two commas is updated because it violates this. It's
added with the patch that introduces core.commentChar in eff80a9
(Allow custom "comment char" - 2013-01-16). It's not clear to me _why_
that behavior is wanted.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
config.c | 8 ++--
t/t7508-status.sh | 3 +--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index a30cb5c..491a905 100644
--- a/config.c
+++ b/config.c
@@ -826,9 +826,13 @@ static int git_default_core_config(const char *var, const
char *value)
if (!strcmp(var, "core.commentchar")) {
const char *comment;
int ret = git_config_string(&comment, var, value);
- if (!ret)
+ if (ret)
+ return ret;
+ else if (comment[0] && !comment[1]) {
comment_line_char = comment[0];
- return ret;
+ } else
+ return error("core.commentChar should only be one
character");
+ return 0;
}
if (!strcmp(var, "core.askpass"))
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..148ab9e 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1350,8 +1350,7 @@ test_expect_success "status (core.commentchar with
submodule summary)" '
test_expect_success "status (core.commentchar with two chars with submodule
summary)" '
test_config core.commentchar ";;" &&
- git -c status.displayCommentPrefix=true status >output &&
- test_i18ncmp expect output
+ test_must_fail git -c status.displayCommentPrefix=true status
'
test_expect_success "--ignore-submodules=all suppresses submodule summary" '
--
1.9.1.346.ga2b5940
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
On Fri, May 16, 2014 at 11:40 PM, Jonathan Nieder wrote:
> Nguyễn Thái Ngọc Duy wrote:
>
>> core.commentChar starts with '#' as in default but if it's already in
>> the prepared message, find another one among a small subset. This
>> should stop surprises because git strips some lines unexpectedly.
>
> Probably worth mentioning this only kicks in if someone explicitly
> configures [core] commentchar = auto.
>
> Would it be a goal to make 'auto' the default eventually if people
> turn out to like it?
No. I started this with a patch that does this automatically without a
config knob. It broke git-commit with custom templates. It broke
--cleanup=strip -e -F.. So people may want to put this in ~/.gitconfig
but it's their decision.
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
>> return ket;
>> }
>>
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> + char candidates[] = " @!#$%^&|:;~";
>
> This prefers '@' over '#'. Intended?
It used to be the order of keys 1, 2, 3... on qwerty keyboard, but I
was afraid ! may become history expansion in bash so I made @
preferred over !. Will make # and ; higher priority.
>> + char *candidate;
>> + const char *p;
>> +
>> + if (!sb->len)
>> + return;
>> +
>> + if (!strchr(candidates, comment_line_char))
>> + candidates[0] = comment_line_char;
>
> Could do
>
> if (!memchr(sb->buf, comment_line_char, sb->len))
> return;
>
> to solve the precedence problem. The comment_line_char not appearing
> in the message is the usual case and handling it separately means it
> gets handled faster.
Now that we use "auto" to turn this on, the placeholder candidates[0]
could probably be removed, we know comment_line_char is '#' at this
point.
>> --- a/config.c
>> +++ b/config.c
>> @@ -829,6 +829,8 @@ static int git_default_core_config(const char *var,
>> const char *value)
>> if (!ret) {
>> if (comment[0] && !comment[1])
>> comment_line_char = comment[0];
>> + else if (!strcasecmp(comment, "auto"))
>> + auto_comment_line_char = 1;
>
> Is there a way to disable 'auto' if 'auto' is already set?
>
> comment_line_char still can be set and matters when 'auto' is set.
> Should they be separate settings?
I think the next core.commentChar should override the old one, so
auto_comment_line_char should be clear when we set new value to
comment_line_char.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
On Fri, May 16, 2014 at 11:33:48AM -0700, Jonathan Nieder wrote:
> (cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
> from long ago)
No, but I have very good tools for searching the list archive. ;)
> > In s_update_ref there are two calls that when they fail we return an error
> > based on the errno value. In particular we want to return a specific error
> > if ENOTDIR happened. Both these functions do have failure modes where they
> > may return an error without updating errno
>
> That's a bug. Any function for which errno is supposed to be
> meaningful when it returns an error should always set errno.
> Otherwise errno may be set to ENOTDIR within the function by an
> unrelated system call.
I'd agree. It's OK for a function not to set errno if it is
_successful_. But if setting errno on error is part of the interface for
lock_any_ref_for_update, it should do so consistently.
We have not been very good about documenting which functions use errno,
or using it consistently (e.g., in my original patch, we are explicitly
converting errno into a numeric return value!), so I can definitely see
how it is confusing.
> That should cover the cases affecting the code path in fetch.c you
> mentioned (I haven't checked the others).
>
> How about something like this?
> [...]
Yeah, I think this is a better direction.
> diff --git i/refs.c w/refs.c
> index 82a8d4e..f98acf0 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
> unsigned char *sha1, int rea
> if (flag)
> *flag = 0;
>
> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> + errno = EINVAL;
> return NULL;
> + }
check_refname_format comes up in a few places. I wonder if it should set
EINVAL itself.
Other than that, I just skimmed through your list. All looked
reasonable to me, though I was not thorough enough to be sure we got
call cases (OTOH, we can add them later on top).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Returning error message from custom smart http server
On Tue, May 13, 2014 at 09:39:59AM +0200, "Ákos, Tajti" wrote:
> Dear List,
>
> we implemented our own git smart http server to be able to check permissions
> and other thing before pushes. It works fine, however, the error messages we
> generate on the server side are not displayed by the command line client. On
> the server we generate error messages like this:
>
> response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
> response.getWriter().write(msg);
>
> On the command line we get this:
>
> Total 0 (delta 0), reused 0 (delta 0)
> POST git-receive-pack (290 bytes)
> efrror: RPC failed; result=22, HTTP code = 401
> atal: The remote end hung up unexpectedly
> fatal: The remote end hung up unexpectedly
>
> The server message is completely missing. Is there a solution for this?
It does look that way. Does the following patch work for you?
-- >8 --
Subject: [PATCH] http: provide server's error message on RPC failure
The server might provide a custom error message that is useful to the user.
Provide this message to the user if HTTP RPC fails.
Signed-off-by: brian m. carlson
---
remote-curl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 52c2d96..5984d35 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -426,8 +426,8 @@ static int run_slot(struct active_request_slot *slot,
err = run_one_slot(slot, results);
if (err != HTTP_OK && err != HTTP_REAUTH) {
- error("RPC failed; result=%d, HTTP code = %ld",
- results->curl_result, results->http_code);
+ error("RPC failed; result=%d, HTTP code = %ld (%s)",
+ results->curl_result, results->http_code, curl_errorstr);
}
return err;
-- >8 --
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
signature.asc
Description: Digital signature
Re: [PATCH v8 23/44] fetch.c: change s_update_ref to use a ref transaction
Ronnie Sahlberg wrote:
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
[...]
> @@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
> snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>
> errno = 0;
> - lock = lock_any_ref_for_update(ref->name,
> -check_old ? ref->old_sha1 : NULL,
> -0, NULL);
> - if (!lock)
> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> - STORE_REF_ERROR_OTHER;
> - if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
> + transaction = ref_transaction_begin();
> + if (!transaction ||
> + ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +ref->old_sha1, 0, check_old) ||
> + ref_transaction_commit(transaction, msg, NULL)) {
Since 'err' is NULL, does that mean there's no message shown to the
user on error?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 24/44] fetch.c: use a single ref transaction for all ref updates
Ronnie Sahlberg wrote:
> Change store_updated_refs to use a single ref transaction for all refs that
> are updated during the fetch. This makes the fetch more atomic when update
> failures occur.
Fun.
[...]
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
[...]
> @@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
> struct ref *ref,
> int check_old)
> {
> - char msg[1024];
> - char *rla = getenv("GIT_REFLOG_ACTION");
> - struct ref_transaction *transaction;
Previously fetch respected GIT_REFLOG_ACTION, and this is dropping
that support. Intended?
[...]
> + if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +ref->old_sha1, 0, check_old))
> + return STORE_REF_ERROR_OTHER;
Should pass a strbuf in to get a message back.
[...]
> +
> return 0;
> }
>
> @@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const
> char *remote_name,
> goto abort;
> }
>
> + errno = 0;
> + transaction = ref_transaction_begin();
> + if (!transaction) {
> + rc = error(_("cannot start ref transaction\n"));
> + goto abort;
error() appends a newline on its own, so the message shouldn't
end with newline.
More importantly, this message isn't helpful without more detail about
why _transaction_begin() failed. The user doesn't know what
error: cannot start ref transaction
means.
error: cannot connect to mysqld for ref storage: [etc]
would tell what they need to know. That is:
transaction = ref_transaction_begin(err);
if (!transaction) {
rc = error("%s", err.buf);
goto abort;
}
errno is not used here, so no need to set errno = 0.
[...]
> @@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const
> char *remote_name,
> }
> }
> }
> + if ((ret = ref_transaction_commit(transaction, NULL)))
> + rc |= ret == -2 ? STORE_REF_ERROR_DF_CONFLICT :
> + STORE_REF_ERROR_OTHER;
(I cheated and stole the newer code from your repo.)
Yay! Style nit: git avoids assignments in "if" conditionals.
ret = ref_tr...
if (ret == -2)
rc |= ...
else if (ret)
rc |= ...
Does _update need the too as futureproofing for backends that can
detect the name conflict sooner?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
On Fri, May 16, 2014 at 09:52:15AM -0700, Junio C Hamano wrote: > Or am I reacting to a typo and you meant to say "I would prefer not > to instrument"? Your "shipping the warnings to end users who did > not package the software will not help" was unclear if you meant the > README that has warning or warning message they have to see every > time from the instrumented code. Argh, yes, it is a typo. I had written "I would prefer _not_ to instrument the code with warnings...". While reading it back to myself, I thought "using underlining there is too argumentative", but somehow managed to delete the whole word rather than simply the "_" characters. I'm very sorry to have wasted people's time by accidentally making the opposite point. I agree with the line of reasoning you laid out in your email, especially: > I would say that the options I see are these three, and I would rank > the "warn every time" as less helpful to end-users: > > - rename contrib/remote-helpers to contrib/obsolete-remote-helpers >and add README to point at the upstream. > > - remove contrib/remote-helpers scripts and add README. > > - warn every time the user runs the scripts. I hadn't thought of the rename idea, and it would address the concerns I brought up. I do think "obsolete" is the wrong word, as it sends the wrong message. The helpers are not obsolete; it is our _copy_ of them that is. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
Junio C Hamano wrote: > Jeff King writes: > > > On Thu, May 15, 2014 at 03:56:29PM -0700, Junio C Hamano wrote: > > > >> Two announcements for their version 0.2 on the list archive are not > >> quite enough to advertise them to their users. > > > > I do not think this README nor a mention in the release notes will get > > their attention either, and many people (and packagers) will continue to > > use the stale versions forever until those versions go away. > > > > I would much rather _replace_ them with a README in the long run, and > > people will notice that they are gone, and then use the README to update > > their install procedure. > > > > For 2.0, I am hesitant to do that, though I do not have a problem with a > > README like this as a heads-up to prepare packagers for the future. I > > say hesitant because people may have been test-packaging 2.0.0-rc3 in > > preparation for release, and it will be annoying to them to suddenly > > switch. > > I share the latter two of the above three. I was giving distro > packagers a bit more credit for their diligence in knowing what they > are packaging than you seem to be in your first point, but I admit > that it is a blind faith. I don't see why anybody would think otherwise. There are dozens of tools in contrib/ many without documentation, or even a description of that whey do. I bet most Git developers don't know what half of them do (even a quarter), how are package maintainers supposed to know? All the packages I've seen stash everything into /usr/share/git, with a few exceptions, like shell completions. > > But that being said, this is Felipe's code. While we have a legal right > > to distribute it in v2.0, if he would really prefer it out for v2.0, I > > would respect that. > > I am fine with that. Are you? Because in two of the three options you list below you wouldn't be doing that. > > I would prefer to instrument the code with warnings, as that is the sort > > of thing a packager moving from -rc3 to -final might not notice, and > > shipping the warnings to end users who did not package the software in > > the first place will not help them. It is the attention of the packagers > > (and source-builders) you want to get. > > I do agree that a new warning every time it is run will be a more > likely to grab users' attention. The conclusion I draw from that > shared observation is that the user will be annoyed all the time, That's exactly what we do when anything gets deprecated. Try running Git with an empty configuration for a day and you'll see tons of warnings, often huge ones every time the user does common operations, like `git push`. Why is it OK to warn about minor behavior changes, but not when the whole code the user is exercising might be broken? At least the warning I proposed for the remote-helpers is small and to the point, unlike the warning of `git push` without refspec. > without having a power to do anything about the annoyance, until the > report s/he (or other users) Throw at the packager, even when the > version that was packaged hasn't diverged that much yet, which does > not help end users. What do you expect the packagers are going to do? They will simply ignore the report as it's not related to their package (git). I suspect this will continue for quite some time, and very likely these will not be packaged as part of the official distribution, but some user-based repository. In some distributions they might end up being unpackaged completely. Take git-imerge as an example, a very popular tool you yourself are very fond of. I don't see it packaged either in ArchLinux, Debian, or Fedora, and I don't think it will ever be packaged. So, yes, this will hurt our users, that's what every user of an out-of-tree tool must suffer, and that's what you unilaterally decided users of git-remote-hg/bzr should suffer. > I guess the ideal we want is to make sure their build break. We cannot make 'cp -r contrib /usr/share/git' fail, that's all packagers do. I already pointed to the fact that most of the tools in contrib shouldn't even be there, and that most of them should have tests. If there was a standardized way to tests contrib, we could make the build break. But you ignored my call of attention. > What if we do the README in addition to rename contrib/remote-helpers > to contrib/obsolete-remote-helpers (or s/obsolete/graduated/)? It > will give the packagers three choices and I think it hurts people much > less. > > * The packagers that dump the entirety of contrib/ to somewhere >without doing anything to expose the scripts to user's PATH do >not have to do anything. The users who chose to pick them up >from there notice the missing contrib/remote-helpers, notice >"obsolete" (or "graduated"), and read README. This is wrong. Users will not notice the missing contrib/remote-helpers. At least not initially. If they setup links to /usr/share, they'll just see: fatal: Unable to find remote helper fo
Delaying 2.0 final
As we seem to have a few regressions we may want to fix, I will not
be cutting the 2.0 final today (https://tinyurl.com/gitCal).
I queued the following near the bottom of 'pu' (these are also
merged to 'next' to keep pu^{/match.next} in sync with next), and
plan to cut 2.0.0-rc4 early next week.
* git-gui has a show-stopper bug that wants to see if it is used
with a version of git that "vsatisfies" 1.7.0; because 2.x is
considered significantly different, it incorrectly decides to use
the codepath for ancient versions that compares older than 1.7.0
when working inside a submodule working tree. A fix by Jens
Lehmann is queued as an emergency patch to use vcompare instead.
* There was a vague admission of having broken git-remote-hg/bzr by
its author that hinted one topic branch is the culprit, without
much detail. As a precautionary measure, the merge of the topic
has been reverted.
* git-remote-hg/bzr are maintained outside our tree, our copy will
become stale and will eventually go away. A warning message to
its standard error output has been added to each of them to tell
the users where to find the latest of these tools.
* git-request-pull has a minor regression and lost a useful DWIM
that was designed to be friendly to users of Git older than
1.7.10. "git request-pull $base $URL for-linus" used to say
"Please pull tags/for-linus" if that is what the user meant, but
the version in 2.0.0-rc3 requires the command line to be "git
request-pull $base $URL tags/for-linus". It is simple enough to
special case this use case, and a patch is included to fix it
with a band-aid.
If you fetch tonight's 'pu' branch, you will find all of the above
at f608b83. Hopefully 2.0.0-rc4 and 2.0.0-final will be the same
tree as that commit, with updated release notes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GIT, libcurl and GSS-Negotiate
On Mon, May 12, 2014 at 04:21:53PM -0400, Jeff King wrote:
> On Sat, May 10, 2014 at 09:01:32PM +, brian m. carlson wrote:
> > * Make git understand that it really needs to try again with different
> > credentials in this case (how to do that is unknown).
>
> It should be pretty straightforward to loop again; http_request_reauth
> just needs to turn into a for-loop on getting HTTP_REAUTH, rather than a
> static two-tries (I even had a patch for this a while ago, but the
> function has changed a bit in the interim).
>
> The tricky part is figuring out when to return HTTP_NOAUTH ("do not try
> again, we failed") versus HTTP_REAUTH ("get credentials and try again")
> in handle_curl_result. Right now the decision is based on "did we have a
> username and password for this request?" I'm not clear on what extra
> bits would be needed to decide to continue in the case you guys are
> discussing.
I'm honestly not sure, either. That's why I said, "how to do that is
unknown".
However, if you base64-decode the two Negotiate replies in the
successful attempt with WinHTTP and pass it through od -tc, you'll see
that the second reply contains some form of user ID that the first one
does not. The curl binary sends an identical reply for the first pass,
but then gives up and does not try a second pass. I don't know if
libcurl is able to provide the data required in the second pass.
All of this is way outside my knowledge, since my Kerberos/GSSAPI
Negotiate requests look very different than the NTLM ones.
> > * Provide some way of forcing git to use a particular authentication
> > protocol.
>
> Yeah, we just set CURLAUTH_ANY now, but it would be fairly trivial to
> add "http.authtype" and "http.proxyauthtype" to map to CURLOPT_HTTPAUTH
> and CURLOPT_PROXYAUTH.
This might be the easiest option.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
signature.asc
Description: Digital signature
Re: [PATCH v8 23/44] fetch.c: change s_update_ref to use a ref transaction
On Fri, May 16, 2014 at 12:12 PM, Jonathan Nieder wrote:
> (+cc: peff for STORE_REF_ERROR_DF_CONFLICT expertise)
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
> [...]
>> + transaction = ref_transaction_begin();
>> + if (!transaction ||
>> + ref_transaction_update(transaction, ref->name, ref->new_sha1,
>> +ref->old_sha1, 0, check_old) ||
>> + ref_transaction_commit(transaction, msg, NULL)) {
>> + ref_transaction_rollback(transaction);
>> return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
>> STORE_REF_ERROR_OTHER;
>> + }
>
> I'd rather not rely on errno here (see the previous patch for why).
> Is there some other way to distinguish the case where a ref couldn't
> be created because there was a prefix of that ref in the way?
>
> For example, maybe ref_transaction_commit could return a different
> negative integer in this case.
I have changed it to make transaction_commit will return a special
error if there is a name conflict
and fetch.c now uses that instead of looking at errno to decide if to
print the "try pruning" message.
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/31] refs.c: allow multiple reflog updates during a single transaction
On Fri, May 16, 2014 at 5:35 PM, Junio C Hamano wrote:
> Ronnie Sahlberg writes:
>
>> Allow to make multiple reflog updates to the same ref during a transaction.
>> This means we only need to lock the reflog once, during the first update that
>> touches the reflog, and that all further updates can just write the reflog
>> entry since the reflog is already locked.
>>
>> This allows us to write code such as:
>>
>> t = transaction_begin()
>> transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
>> loop-over-somehting...
>>transaction_reflog_update(t, "foo", 0, );
>> transaction_commit(t)
>
> OK, so you are now doing not just "refs" but also "reflogs", you
> felt that "ref_transaction()" does not cover the latter. Is that
> the reason for the rename in the earlier step?
>
> I am sort-of on the fence.
>
> Calling the begin "ref_transaction_begin" and then calling the new
> function "ref_transaction_log_update" would still allow us to
> differentiate transactions on refs and reflogs, while allowing other
> kinds of transactions that are not related to refs at all to employ
> a mechanism that is different from the one that is used to implement
> the transactions on refs and reflogs you are building here.
>
> But I think I am OK with the generic "transaction-begin" now.
> Having one mechanism for refs and reflogs, and then having another
> completely different mechanism for other things, will not let us
> coordinate between the two easily, so "allow transactions that are
> not related to refs at all to be built on a different mechanism" may
> not be a worthwhile goal to pursue in the first place. Please
> consider the question on the naming in the earlier one dropped.
>
>>
>> where we first truncate the reflog and then build the new content one line
>> at a
>> time.
>>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> refs.c | 58 +-
>> 1 file changed, 49 insertions(+), 9 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index a3f60ad..e7ede03 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -37,6 +37,10 @@ static inline int bad_ref_char(int ch)
>> * need to lock the loose ref during the transaction.
>> */
>> #define REF_ISPACKONLY 0x0200
>> +/** Only the first reflog update needs to lock the reflog file. Further
>> updates
>> + * just use the lock taken by the first update.
>> + */
>
> Style.
>
>> @@ -3349,8 +3355,23 @@ int transaction_update_reflog(struct ref_transaction
>> *transaction,
>> int flags)
>> {
>> struct ref_update *update;
>> + int i;
>>
>> update = add_update(transaction, refname, UPDATE_LOG);
>> + update->flags = flags;
>> + for (i = 0; i < transaction->nr - 1; i++) {
>> + if (transaction->updates[i]->update_type != UPDATE_LOG)
>> + continue;
>> + if (!strcmp(transaction->updates[i]->refname,
>> + update->refname)) {
>> + update->flags |= UPDATE_REFLOG_NOLOCK;
>> + update->orig_update = transaction->updates[i];
>> + break;
>> + }
>> + }
>> + if (!(update->flags & UPDATE_REFLOG_NOLOCK))
>> + update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
>
> So with two calls to transaction-update-reflog, we make two calls to
> add-update, and each holds a separate lock? If we write two entries
> to record two updates of the same ref, would we still want to do so?
Also, indent with tabs rather than spaces (the line following the 'if').
>> + /* Rollback any reflog files that are still open */
>> + for (i = 0; i < n; i++) {
>> + struct ref_update *update = updates[i];
>> +
>> + if (update->update_type != UPDATE_LOG)
>> + continue;
>> + if (update->flags & UPDATE_REFLOG_NOLOCK)
>> + continue;
>> + if (update->reflog_fd == -1)
>> + continue;
>> + rollback_lock_file(update->reflog_lock);
>> + }
>> transaction->status = ret ? REF_TRANSACTION_ERROR
>> : REF_TRANSACTION_CLOSED;
Indent with tabs.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/31] refs.c: allow multiple reflog updates during a single transaction
Ronnie Sahlberg writes:
> Allow to make multiple reflog updates to the same ref during a transaction.
> This means we only need to lock the reflog once, during the first update that
> touches the reflog, and that all further updates can just write the reflog
> entry since the reflog is already locked.
>
> This allows us to write code such as:
>
> t = transaction_begin()
> transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
> loop-over-somehting...
>transaction_reflog_update(t, "foo", 0, );
> transaction_commit(t)
OK, so you are now doing not just "refs" but also "reflogs", you
felt that "ref_transaction()" does not cover the latter. Is that
the reason for the rename in the earlier step?
I am sort-of on the fence.
Calling the begin "ref_transaction_begin" and then calling the new
function "ref_transaction_log_update" would still allow us to
differentiate transactions on refs and reflogs, while allowing other
kinds of transactions that are not related to refs at all to employ
a mechanism that is different from the one that is used to implement
the transactions on refs and reflogs you are building here.
But I think I am OK with the generic "transaction-begin" now.
Having one mechanism for refs and reflogs, and then having another
completely different mechanism for other things, will not let us
coordinate between the two easily, so "allow transactions that are
not related to refs at all to be built on a different mechanism" may
not be a worthwhile goal to pursue in the first place. Please
consider the question on the naming in the earlier one dropped.
>
> where we first truncate the reflog and then build the new content one line at
> a
> time.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 58 +-
> 1 file changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index a3f60ad..e7ede03 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -37,6 +37,10 @@ static inline int bad_ref_char(int ch)
> * need to lock the loose ref during the transaction.
> */
> #define REF_ISPACKONLY 0x0200
> +/** Only the first reflog update needs to lock the reflog file. Further
> updates
> + * just use the lock taken by the first update.
> + */
Style.
> @@ -3349,8 +3355,23 @@ int transaction_update_reflog(struct ref_transaction
> *transaction,
> int flags)
> {
> struct ref_update *update;
> + int i;
>
> update = add_update(transaction, refname, UPDATE_LOG);
> + update->flags = flags;
> + for (i = 0; i < transaction->nr - 1; i++) {
> + if (transaction->updates[i]->update_type != UPDATE_LOG)
> + continue;
> + if (!strcmp(transaction->updates[i]->refname,
> + update->refname)) {
> + update->flags |= UPDATE_REFLOG_NOLOCK;
> + update->orig_update = transaction->updates[i];
> + break;
> + }
> + }
> + if (!(update->flags & UPDATE_REFLOG_NOLOCK))
> + update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
So with two calls to transaction-update-reflog, we make two calls to
add-update, and each holds a separate lock? If we write two entries
to record two updates of the same ref, would we still want to do so?
> + /* Rollback any reflog files that are still open */
> + for (i = 0; i < n; i++) {
> + struct ref_update *update = updates[i];
> +
> + if (update->update_type != UPDATE_LOG)
> + continue;
> + if (update->flags & UPDATE_REFLOG_NOLOCK)
> + continue;
> + if (update->reflog_fd == -1)
> + continue;
> + rollback_lock_file(update->reflog_lock);
> + }
> transaction->status = ret ? REF_TRANSACTION_ERROR
> : REF_TRANSACTION_CLOSED;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/31] refs.c: only write reflog update if msg is non-NULL
Ronnie Sahlberg writes:
> When performing a reflog transaction update, only write to the reflog iff
> msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
> an update that only truncates but does not write.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 8 +---
> refs.h | 1 +
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index a8b583a..a3f60ad 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3625,9 +3625,11 @@ int transaction_commit(struct ref_transaction
> *transaction,
> ftruncate(update->reflog_fd, 0)) {
> error("Could not truncate reflog");
> }
> - if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
> - update->new_sha1,
> - update->committer, update->msg)) {
> + if (update->msg && log_ref_write_fd(update->reflog_fd,
> + update->old_sha1,
> + update->new_sha1,
> + update->committer,
> + update->msg)) {
Wouldn't it make it easier to read if you chopped immediately after
the &&, i.e. chopping at a gap at a higher-level in the parse tree?
if (update->msg &&
log_ref_write_fd(update->reflog_fd,
update->old_sha1, update->old_sha1,
update->committer, update->msg)) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git-users] worlds slowest git repo- what to do?
On 05/16/2014 03:13 AM, Duy Nguyen wrote: > On Fri, May 16, 2014 at 2:06 AM, Philip Oakley wrote: >> From: "John Fisher" >>> I assert based on one piece of evidence ( a post from a facebook dev) that >>> I now have the worlds biggest and slowest git >>> repository, and I am not a happy guy. I used to have the worlds biggest >>> CVS repository, but CVS can't handle multi-G >>> sized files. So I moved the repo to git, because we are using that for our >>> new projects. >>> >>> goal: >>> keep 150 G of files (mostly binary) from tiny sized to over 8G in a >>> version-control system. > I think your best bet so far is git-annex good, I am looking at that > (or maybe bup) for dealing > with huge files. I plan on resurrecting Junio's split-blob series to > make core git handle huge files better, but there's no eta on that. > The problem here is about file size, not the number of files, or > history depth, right? When things here calm down, I could easily test the repo without the giant files, leaving 99% of files in the repo. There is hardly any history depth because these are releases, version controlled by directory name. As has been suggested I could be forced to abandon the version-control, even to the point of just using rsync. But I've been doing this with CVS for 10 years now and I hate to change or in any way move away fron KISS. Moving it to Git may not have been one of my better ideas... > Probably known issues. But some elaboration would be nice (e.g. what > operation is slow, how slow, some more detail > characteristics of the repo..) in case new problems pop up. so far I have done add, commit, status, clone - commit and status are slow; add seems to depend on the files involved, clone seems to run at network speed. I can provide metrics later, see above. email me offline with what you want. John -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/31] refs.c: add a flag to allow reflog updates to truncate the log
Ronnie Sahlberg writes:
> Add a flag that allows us to truncate the reflog before we write the update.
>
> Signed-off-by: Ronnie Sahlberg
> ---
Until we read the callers it is hard to see how such a feature is
useful, though.
(style) The two multi-line comments are formatted differently.
> refs.c | 12 ++--
> refs.h | 4 +++-
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d8a1568..a8b583a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3608,7 +3608,11 @@ int transaction_commit(struct ref_transaction
> *transaction,
> }
> }
>
> - /* Update all reflog files */
> + /* Update all reflog files
> +We have already done all ref updates and deletes.
> +There is not much we can do here if there are any reflog
> +update errors, other than complain.
> + */
> for (i = 0; i < n; i++) {
> struct ref_update *update = updates[i];
>
> @@ -3616,7 +3620,11 @@ int transaction_commit(struct ref_transaction
> *transaction,
> continue;
> if (update->reflog_fd == -1)
> continue;
> -
> + if (update->flags & REFLOG_TRUNCATE)
> + if (lseek(update->reflog_fd, 0, SEEK_SET) < 0 ||
> + ftruncate(update->reflog_fd, 0)) {
> + error("Could not truncate reflog");
> + }
The {} looks somewhat curious here. If you seeked to the beginning,
and failed to truncate, do you still want to go on without "return"
in front of the error()? Would that overwrite existing entries?
> if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
>update->new_sha1,
>update->committer, update->msg)) {
> diff --git a/refs.h b/refs.h
> index 2e22a26..b1b97e7 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -269,8 +269,10 @@ int transaction_delete_sha1(struct ref_transaction
> *transaction,
> const unsigned char *old_sha1,
> int flags, int have_old, const char *msg);
>
> +#define REFLOG_TRUNCATE 0x01
> /*
> - * Append a reflog entry for refname.
> + * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
> + * this update will first truncate the reflog before writing the entry.
> */
> int transaction_update_reflog(struct ref_transaction *transaction,
> const char *refname,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/31] refs.c: rename the transaction functions
Ronnie Sahlberg writes: > Rename the transaction functions. Remove the leading ref_ from the names > and append _sha1 to the names for functions that create/delete/update sha1 > refs. > ... > - transaction = ref_transaction_begin(); > + transaction = transaction_begin(); Why? Do we forsee that there will never be other kinds of transaction, and whenever we say a "transaction" that will be always about updating the refs and nothing else? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
errno is hairy :-(
You probably also want something like this :
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4603cb6..55e7dd8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -426,7 +426,7 @@ static int update_local_ref(struct ref *ref,
_("! %-*s %-*s -> %s (can't fetch in current branch
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref);
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
if (!is_null_sha1(ref->old_sha1) &&
@@ -513,7 +513,7 @@ static int update_local_ref(struct ref *ref,
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref,
_("(non-fast-forward)"));
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
}
To make it more clear that this function returns a specific error and
not just a generic not-zero.
On Fri, May 16, 2014 at 11:33 AM, Jonathan Nieder wrote:
> (cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
> from long ago)
> Ronnie Sahlberg wrote:
>
>> In s_update_ref there are two calls that when they fail we return an error
>> based on the errno value. In particular we want to return a specific error
>> if ENOTDIR happened. Both these functions do have failure modes where they
>> may return an error without updating errno
>
> That's a bug. Any function for which errno is supposed to be
> meaningful when it returns an error should always set errno.
> Otherwise errno may be set to ENOTDIR within the function by an
> unrelated system call.
>
> Functions that fail and lead callers to check errno, based on a quick
> search with 'git grep -e 'errno *[!=]=':
>
> fopen
> lstat
> builtin/apply.c::try_create_file (= mkdir, symlink, or open)
> rmdir
> open
> mkdir
> unlink
> lock_any_ref_for_update
> write_ref_sha1
> strtol
> kill
> odb_pack_keep
> opendir
> fgets
> read
> poll
> pread
> strtoimax
> strtoumax
> git_parse_int
> git_parse_int64
> git_parse_ulong
> write_in_full
> credential-cache.c::send_request
> fstat
> run_command_v_opt
> git.c::run_argv
> readlink
> resolve_ref_unsafe
> hold_lock_file_for_update
> unlink_or_warn
> rename
> execvp
> waitpid
> execv_git_cmd
> execv_shell_cmd
> sane_execvp
> stat
> read_object
> git_mkstemp_mode
> create_tmpfile
> start_command
> xwrite
> iconv
> fast_export_ls
> fast_export_ls_rev
> delete_ref
>
> lock_any_ref_for_update has failure paths
> * check_ref_format [!]
> * lock_ref_sha1_basic
>
> lock_ref_sha1_basic has failure paths
> * remove_empty_directories
> * resolve_ref_unsafe
> * safe_create_leading_directories
> * verify_lock
>
> remove_empty_directories has one failure path
> * remove_dir_recursively
> but could be more explicit about the need to preserve errno.
>
> errno from remove_dir_recursively is meaningful.
>
> resolve_ref_unsafe has failure paths
> * check_refname_format [!]
> * too much recursion [!]
> * lstat, readlink, open
> * handle_missing_loose_ref
> * read_in_full, but errno gets clobbered
> * invalid ref [!]
> * invalid symref [!]
>
> verify_lock has failure paths
> * read_ref_full, but errno gets clobbered
> * old_sha1 didn't match [!]
>
> write_ref_sha1 has failure paths
> * !lock [!]
> * missing object [!]
> * non-commit object [!]
> * write_in_full, close_ref, but errno gets clobbered
> * log_ref_write
> * commit_ref
>
> log_ref_write has failure paths
> * log_ref_setup
> * write_in_full, close. errno gets clobbered
>
> commit_ref just calls commit_lock_file.
>
> log_ref_setup has failure paths
> * safe_create_leading_directories, but errno gets clobbered
> * remove_empty_directories, but errno gets clobbered
> * open, but errno gets clobbered
>
> safe_create_leading_directories doesn't set errno for SCLD_EXISTS
> but otherwise its errno is useful.
>
> That should cover the cases affecting the code path in fetch.c you
> mentioned (I haven't checked the others).
>
> How about something like this?
>
> It's also tempting to teach vreportf and vwritef to save errno, which
> would handle some of these cases automatically.
>
> diff --git i/refs.c w/refs.c
> index 82a8d4e..f98acf0 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
> unsigned char *sha1, int rea
> if (flag)
> *flag = 0;
>
> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> + errno = EINVAL;
> return NULL;
> + }
>
> for (;;) {
> char path[PATH_MAX];
> @@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname,
> unsigned char *sha1, int rea
> char *buf;
> int fd;
>
> - if (--depth < 0)
>
Re: [PATCH v8 23/44] fetch.c: change s_update_ref to use a ref transaction
(+cc: peff for STORE_REF_ERROR_DF_CONFLICT expertise)
Ronnie Sahlberg wrote:
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
[...]
> + transaction = ref_transaction_begin();
> + if (!transaction ||
> + ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +ref->old_sha1, 0, check_old) ||
> + ref_transaction_commit(transaction, msg, NULL)) {
> + ref_transaction_rollback(transaction);
> return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> STORE_REF_ERROR_OTHER;
> + }
I'd rather not rely on errno here (see the previous patch for why).
Is there some other way to distinguish the case where a ref couldn't
be created because there was a prefix of that ref in the way?
For example, maybe ref_transaction_commit could return a different
negative integer in this case.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command
Jonathan Nieder writes:
> Perhaps something like the following would work?
>
> tree-wide: convert test -a/-o to && and ||
>
> The interaction with unary operators and operator precedence
> for && and || are better known than -a and -o, and for that
> reason we prefer them. Replace all existing instances in git
> of -a and -o to save readers from the burden of thinking
> about such things.
>
> Add a check-non-portable-shell.pl to avoid more instances of
> test -a and -o arising in the future.
Yeah, the title is certainly better than "-a or -b option" I see
above ;-) and a single tree-wide fix may be OK while the tree is
quiescent.
I however do think "better known" is much less of an issue than that
"-a/-o" is more error prone e.g. 'test -n "$x" -a a = b' is buggy
because it does not consider that $x could be "=".
> [...]
>> -test $status = D -o $status = T && echo "$sm_path" &&
>> continue
>> + ( test $status = D || test $status = T ) && echo
>> "$sm_path" && continue
>
> There's no need for a subshell for this. Better to use a block:
>
> {
> test "$status" = D ||
> test "$status" = T
> } &&
> echo "$sm_path" &&
> continue
Yes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: switch core.commentChar if it's found in existing commit
Junio C Hamano writes:
> Nguyễn Thái Ngọc Duy writes:
>
>> If we need to use core.commentChar and it's already in the prepared
>> message, find another char among a small subset. This should stop
>> surprises because git strips some lines unexpectedly. Of course if
>> candicate characters happen to be all out, this change does not help.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy
>> ---
>> On Fri, May 16, 2014 at 5:28 PM, Duy Nguyen wrote:
>> > But maybe git should detect that the
>> > current commit message has leading '#' and automatically switch to
>> > another character..
>>
>> Something like this. Lightly tested.. I know there's a small bug..
>>
>> builtin/commit.c | 29 +
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 6ab4605..70ceb61 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -593,6 +593,32 @@ static char *cut_ident_timestamp_part(char *string)
>> return ket;
>> }
>>
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> +char candidates[] = " !@#$%^&|:;~";
>
> Did you really mean to add a SP to the candidates?
Please ignore this noise. I realized that the SP is there only to
be overwritten in the meantime, and this is an old message sent out
before that realization, just emerging from my mail provider's
outbox.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] t4205, t6006: Add failing tests for the case when i18n.logOutputEncoding is set
Alexey Shumkin writes: > Pretty format string %<(N,[ml]trunc)>%s truncates subject to a given > length with an appropriate padding. This works for non-ASCII texts when > i18n.logOutputEncoding is UTF-8 only (independently of a printed commit > message encoding) but does not work when i18n.logOutputEncoding is NOT > UTF-8. > > There were no breakages as far as were no tests for the case > when both a commit message and logOutputEncoding are not UTF-8. > > Add failing tests for that which will be fixed in the next patch. > > Signed-off-by: Alexey Shumkin > Reviewed-by: Nguyễn Thái Ngọc Duy > --- > t/t4205-log-pretty-formats.sh | 169 > ++ > t/t6006-rev-list-format.sh| 75 ++- > 2 files changed, 242 insertions(+), 2 deletions(-) > > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index 2a6278b..6791e0d 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -153,6 +153,19 @@ EOF > test_cmp expected actual > ' > > +test_expect_success 'left alignment formatting. i18n.logOutputEncoding' ' > + git -c i18n.logOutputEncoding=iso8859-1 log --pretty="format:%<(40)%s" > >actual && > + # complete the incomplete line at the end > + echo >>actual && Would it change the meaning of the test if you used tformat: instead of format: (or --format="%<(40)%s")? If it doesn't, it would make it unnecessary to append an extra LF and explain why you do so. > + qz_to_tab_space+ git -c i18n.logOutputEncoding=iso8859-1 log --pretty="format:%<(1)%s" > >actual && > + # complete the incomplete line at the end > + echo >>actual && Likewise for all the other "--pretty=format:" followed by an echo. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* regression: request-pull with signed tag lacks tags/ in master
Junio C Hamano writes: > "Michael S. Tsirkin" writes: > >>> My reading of the earlier parts of the series is that Linus wanted >>> us never dwim "for-upstream" to "tags/for-upstream" or any other ref >>> that happens to point at the same commit as for-upstream you have. >>> The changes done for that purpose covered various cases a bit too >>> widely, and "request-pull ... tags/for_upstream" were incorrectly >>> stripped to a request to pull "for_upstream", which was fixed by >>> 5aae66bd (request-pull: resurrect "pretty refname" feature, >>> 2014-02-25). >>> >>> But that fix does not resurrect the dwimming forbid by the earlier >>> parts of the series to turn "for_upstream" into "tags/for_upstream". >>> >>> What would you get if you do this? >>> >>> $ git request-pull origin/master \ >>> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git \ >>> tags/for_upstream | grep git.kernel.org >> >> >> I get >> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > > Thanks for double-checking. Let's close this as working as > intended, then. > > I personally feel that the "intention" tightened the logic a bit too > much [*1*], and with the updates mentioned in [*2*], existing users > may find it still too tight, but that is what we have today. > > > [References] > > *1* http://thread.gmane.org/gmane.comp.version-control.git/240860 > *2* http://thread.gmane.org/gmane.comp.version-control.git/240886 An update. In the ideal world, I think it would be nice to make $ git request-pull $mine $URL for_upstream explicitly say "Please pull tags/for_upstream" rather than without "tags/" prefix to accomodate older Git, where $ git pull $URL for_upstream did not DWIM to fetch and merge tags/for_upstream and the user had to say "tags/for_upstream" instead. That "older Git" refers to those without 47d84b6a (fetch: allow "git fetch $there v1.0" to fetch a tag, 2011-11-04). v1.7.10 (tagged on April 6th, 2012) and later versions of Git will notice that the name refers to the tags/for_upstream just fine, so I think it is fair to label this as a minor cosmetic regression whose fix can wait for a future maintenance release, rather than a blocker for the upcoming release. I _think_ the fix, without breaking the spirit of Linus's "I do not want the thing DWIM based on what the remote end has" original, would be as simple as this patch. We can queue it as a regression fix and do another round of -rc4 if those who depend on request-pull heavily feel strongly about it. Comments? git-request-pull.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/git-request-pull.sh b/git-request-pull.sh index 5c15997..d5500fd 100755 --- a/git-request-pull.sh +++ b/git-request-pull.sh @@ -119,6 +119,12 @@ then status=1 fi +# Special case: turn "for_linus" to "tags/for_linus" when it is correct +if test "$ref" = "refs/tags/$pretty_remote" +then + pretty_remote=tags/$pretty_remote +fi + url=$(git ls-remote --get-url "$url") git show -s --format='The following changes since commit %H: -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: switch core.commentChar if it's found in existing commit
Nguyễn Thái Ngọc Duy writes:
> If we need to use core.commentChar and it's already in the prepared
> message, find another char among a small subset. This should stop
> surprises because git strips some lines unexpectedly. Of course if
> candicate characters happen to be all out, this change does not help.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> On Fri, May 16, 2014 at 5:28 PM, Duy Nguyen wrote:
> > But maybe git should detect that the
> > current commit message has leading '#' and automatically switch to
> > another character..
>
> Something like this. Lightly tested.. I know there's a small bug..
>
> builtin/commit.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6ab4605..70ceb61 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -593,6 +593,32 @@ static char *cut_ident_timestamp_part(char *string)
> return ket;
> }
>
> +static void adjust_comment_line_char(const struct strbuf *sb)
> +{
> + char candidates[] = " !@#$%^&|:;~";
Did you really mean to add a SP to the candidates?
> + char *candidate;
> + const char *p;
> + if (!sb->len)
> + return;
> + candidates[0] = comment_line_char;
> + p = sb->buf;
> + do {
> + candidate = strchr(candidates, *p);
> + if (candidate)
> + *candidate = ' ';
> + p = strchrnul(p, '\n');
> + if (*p)
> + p++;
> + } while (*p);
> + if (strchr(candidates, comment_line_char)) {
> + p = candidates;
> + while (*p && *p == ' ')
> + p++;
> + if (*p)
> + comment_line_char = *p;
> + }
> +}
> +
> static int prepare_to_commit(const char *index_file, const char *prefix,
>struct commit *current_head,
>struct wt_status *s,
> @@ -748,6 +774,9 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
> die_errno(_("could not write commit template"));
>
> + if (use_editor && include_status)
> + adjust_comment_line_char(&sb);
> +
> strbuf_release(&sb);
>
> /* This checks if committer ident is explicitly given */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
Jakub Narębski writes: >> Correct, but is "where does it appear" the question we are >> primarily interested in, wrt this breakage and its fix? > > That of course depends on how we want to test gitweb output. > The simplest solution, comparing with known output with perhaps > fragile / variable elements masked out could be done quickly... > but changes in output (even if they don't change functionality, > or don't change visible output) require regenerating test cases > (expected output) to test against - which might be source of > errors in test suite. I agree with your "to test it fully, we need extra dependencies", but my point is that it does not have to be a full "HTML-validating, picking the expected attribute via XPATH matching" kind of test if what we want is only to add a new test to protect this particular fix from future breakages. For example, I think it is sufficient to grep for 'href="...%xx%xx"' in the output after preparing a sample tree with one entry to show. The expected substring either exists (in which case we got it right), or it doesn't (in which case we are showing garbage). Of course that depends on the assumption that its output is not too heavily contaminated with volatile parts outside our control, as I already mentioned in the message you are responding to. But it all depends on "if" we wanted to add a new test ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 00/44] Use ref transactions for all ref updates
Ronnie Sahlberg wrote: > This patch series can also be found at > https://github.com/rsahlberg/git/tree/ref-transactions I think the rerolls are coming too fast. I've been using your repository on github to check if I should not send any particular comment because you've already fixed it, so the extra mail is not needed for that purpose, at least. In general, I think it's best to stick with one version of a series, gather review comments on it, comment inline to solicit feedback about approaches to particular problems, point reviewers to an up-to-date branch which is more in flux, etc, and then only resend when reviewers have had a chance to get through it to make it easier to keep track of the review without getting lost. Thanks for the quick work so far, by the way. It's been fun watching the API evolve. My two cents, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 22/44] fetch.c: clear errno before calling functions that might set it
(cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
from long ago)
Ronnie Sahlberg wrote:
> In s_update_ref there are two calls that when they fail we return an error
> based on the errno value. In particular we want to return a specific error
> if ENOTDIR happened. Both these functions do have failure modes where they
> may return an error without updating errno
That's a bug. Any function for which errno is supposed to be
meaningful when it returns an error should always set errno.
Otherwise errno may be set to ENOTDIR within the function by an
unrelated system call.
Functions that fail and lead callers to check errno, based on a quick
search with 'git grep -e 'errno *[!=]=':
fopen
lstat
builtin/apply.c::try_create_file (= mkdir, symlink, or open)
rmdir
open
mkdir
unlink
lock_any_ref_for_update
write_ref_sha1
strtol
kill
odb_pack_keep
opendir
fgets
read
poll
pread
strtoimax
strtoumax
git_parse_int
git_parse_int64
git_parse_ulong
write_in_full
credential-cache.c::send_request
fstat
run_command_v_opt
git.c::run_argv
readlink
resolve_ref_unsafe
hold_lock_file_for_update
unlink_or_warn
rename
execvp
waitpid
execv_git_cmd
execv_shell_cmd
sane_execvp
stat
read_object
git_mkstemp_mode
create_tmpfile
start_command
xwrite
iconv
fast_export_ls
fast_export_ls_rev
delete_ref
lock_any_ref_for_update has failure paths
* check_ref_format [!]
* lock_ref_sha1_basic
lock_ref_sha1_basic has failure paths
* remove_empty_directories
* resolve_ref_unsafe
* safe_create_leading_directories
* verify_lock
remove_empty_directories has one failure path
* remove_dir_recursively
but could be more explicit about the need to preserve errno.
errno from remove_dir_recursively is meaningful.
resolve_ref_unsafe has failure paths
* check_refname_format [!]
* too much recursion [!]
* lstat, readlink, open
* handle_missing_loose_ref
* read_in_full, but errno gets clobbered
* invalid ref [!]
* invalid symref [!]
verify_lock has failure paths
* read_ref_full, but errno gets clobbered
* old_sha1 didn't match [!]
write_ref_sha1 has failure paths
* !lock [!]
* missing object [!]
* non-commit object [!]
* write_in_full, close_ref, but errno gets clobbered
* log_ref_write
* commit_ref
log_ref_write has failure paths
* log_ref_setup
* write_in_full, close. errno gets clobbered
commit_ref just calls commit_lock_file.
log_ref_setup has failure paths
* safe_create_leading_directories, but errno gets clobbered
* remove_empty_directories, but errno gets clobbered
* open, but errno gets clobbered
safe_create_leading_directories doesn't set errno for SCLD_EXISTS
but otherwise its errno is useful.
That should cover the cases affecting the code path in fetch.c you
mentioned (I haven't checked the others).
How about something like this?
It's also tempting to teach vreportf and vwritef to save errno, which
would handle some of these cases automatically.
diff --git i/refs.c w/refs.c
index 82a8d4e..f98acf0 100644
--- i/refs.c
+++ w/refs.c
@@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (flag)
*flag = 0;
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
return NULL;
+ }
for (;;) {
char path[PATH_MAX];
@@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
char *buf;
int fd;
- if (--depth < 0)
+ if (--depth < 0) {
+ errno = ELOOP;
return NULL;
+ }
git_snpath(path, sizeof(path), "%s", refname);
@@ -1405,9 +1409,13 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
return NULL;
}
len = read_in_full(fd, buffer, sizeof(buffer)-1);
- close(fd);
- if (len < 0)
+ if (len < 0) {
+ int save_errno = errno;
+ close(fd);
+ errno = save_errno;
return NULL;
+ }
+ close(fd);
while (len && isspace(buffer[len-1]))
len--;
buffer[len] = '\0';
@@ -1424,6 +1432,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
(buffer[40] != '\0' && !isspace(buffer[40]))) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
return refname;
@@ -1436,6 +1445,7 @@ const char *resolve_ref_unsaf
Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
Jonathan Nieder writes:
> Nguyễn Thái Ngọc Duy wrote:
>
>> core.commentChar starts with '#' as in default but if it's already in
>> the prepared message, find another one among a small subset. This
>> should stop surprises because git strips some lines unexpectedly.
>
> Probably worth mentioning this only kicks in if someone explicitly
> configures [core] commentchar = auto.
>
> Would it be a goal to make 'auto' the default eventually if people
> turn out to like it?
>
> [...]
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
>> return ket;
>> }
>>
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> +char candidates[] = " @!#$%^&|:;~";
>
> This prefers '@' over '#'. Intended?
I think the candidates[0] will almost always be overridden with "#"
so it probably does not matter in practice, but I tend to agree that
"#" (and probably ";") should come before all others.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] config: be strict on core.commentChar
Jonathan Nieder writes:
> Nguyễn Thái Ngọc Duy wrote:
>
>> --- a/config.c
>> +++ b/config.c
>> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
>> const char *value)
>> if (!strcmp(var, "core.commentchar")) {
>> const char *comment;
>> int ret = git_config_string(&comment, var, value);
>> -if (!ret)
>> -comment_line_char = comment[0];
>> +if (!ret) {
>> +if (comment[0] && !comment[1])
>> +comment_line_char = comment[0];
>> +else
>> +return error("core.commentChar should only be
>> one character");
>> +}
>
> Perhaps, to decrease indentation a little:
>
> if (ret)
> return ret;
> if (comment[0] && !comment[1])
> comment_line_char = comment[0];
> else
> return error(...);
> return 0;
>
> [...]
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with
>> submodule summary)" '
>> test_i18ncmp expect output
>> '
>>
>> -test_expect_success "status (core.commentchar with two chars with submodule
>> summary)" '
>> -test_config core.commentchar ";;" &&
>> -git -c status.displayCommentPrefix=true status >output &&
>> -test_i18ncmp expect output
>
> Could keep the test to avoid regressions:
>
> test_config core.commentchar ";;" &&
> test_must_fail git -c status.displayCommentPrefix=true status
All good points, including your other review message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] request-pull: resurrect for-linus -> tags/for-linus DWIM
Older versions of Git before v1.7.10 did not DWIM $ git pull $URL for-linus to the tag "tags/for-linus" and the users were required to say $ git pull $URL tags/for-linus instead. Because newer versions of Git works either way, request-pull used to show tags/for-linus when asked $ git request-pull origin/master $URL for-linus The recent updates broke this and in the output we see "for-linus" without the "tags/" prefix. As v1.7.10 is more than 2 years old, this should matter very little in practice, but resurrecting it is very simple. Signed-off-by: Junio C Hamano --- * I _think_ the fix, without breaking the spirit of Linus's "I do not want the thing DWIM based on what the remote end has" original, would be as simple as this patch. We can queue it as a regression fix and do another round of -rc4 if those who depend on request-pull heavily feel strongly about it. git-request-pull.sh | 6 ++ t/t5150-request-pull.sh | 8 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/git-request-pull.sh b/git-request-pull.sh index b67513a..d6648b2 100755 --- a/git-request-pull.sh +++ b/git-request-pull.sh @@ -118,6 +118,12 @@ then status=1 fi +# Special case: turn "for_linus" to "tags/for_linus" when it is correct +if test "$ref" = "refs/tags/$pretty_remote" +then + pretty_remote=tags/$pretty_remote +fi + url=$(git ls-remote --get-url "$url") git show -s --format='The following changes since commit %H: diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh index 75d6b38..93e2c65 100755 --- a/t/t5150-request-pull.sh +++ b/t/t5150-request-pull.sh @@ -223,7 +223,13 @@ test_expect_success 'pull request format' ' git request-pull initial "$downstream_url" tags/full:refs/tags/full ) >request && sed -nf fuzz.sed request.fuzzy && - test_i18ncmp expect request.fuzzy + test_i18ncmp expect request.fuzzy && + + ( + cd local && + git request-pull initial "$downstream_url" full + ) >request && + grep ' tags/full$' ' test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' ' -- 2.0.0-rc3-434-g1ba2fe8 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] Gitweb: Convert UTF-8 encoded file names
(sorry if you receive a dup; pobox.com seems to be constipated right now) Jakub Narębski writes: >> Correct, but is "where does it appear" the question we are >> primarily interested in, wrt this breakage and its fix? > > That of course depends on how we want to test gitweb output. > The simplest solution, comparing with known output with perhaps > fragile / variable elements masked out could be done quickly... > but changes in output (even if they don't change functionality, > or don't change visible output) require regenerating test cases > (expected output) to test against - which might be source of > errors in test suite. I agree with your "to test it fully, we need extra dependencies", but my point is that it does not have to be a full "HTML-validating, picking the expected attribute via XPATH matching" kind of test if what we want is only to add a new test to protect this particular fix from future breakages. For example, I think it is sufficient to grep for 'href="...%xx%xx"' in the output after preparing a sample tree with one entry to show. The expected substring either exists (in which case we got it right), or it doesn't (in which case we are showing garbage). Of course that depends on the assumption that its output is not too heavily contaminated with volatile parts outside our control, as I already mentioned in the message you are responding to. But it all depends on "if" we wanted to add a new test ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
Jeff King writes: > On Thu, May 15, 2014 at 03:56:29PM -0700, Junio C Hamano wrote: > >> Two announcements for their version 0.2 on the list archive are not >> quite enough to advertise them to their users. > > I do not think this README nor a mention in the release notes will get > their attention either, and many people (and packagers) will continue to > use the stale versions forever until those versions go away. > > I would much rather _replace_ them with a README in the long run, and > people will notice that they are gone, and then use the README to update > their install procedure. > > For 2.0, I am hesitant to do that, though I do not have a problem with a > README like this as a heads-up to prepare packagers for the future. I > say hesitant because people may have been test-packaging 2.0.0-rc3 in > preparation for release, and it will be annoying to them to suddenly > switch. I share the latter two of the above three. I was giving distro packagers a bit more credit for their diligence in knowing what they are packaging than you seem to be in your first point, but I admit that it is a blind faith. > But that being said, this is Felipe's code. While we have a legal right > to distribute it in v2.0, if he would really prefer it out for v2.0, I > would respect that. I am fine with that. > I would prefer to instrument the code with warnings, as that is the sort > of thing a packager moving from -rc3 to -final might not notice, and > shipping the warnings to end users who did not package the software in > the first place will not help them. It is the attention of the packagers > (and source-builders) you want to get. I do agree that a new warning every time it is run will be a more likely to grab users' attention. The conclusion I draw from that shared observation is that the user will be annoyed all the time, without having a power to do anything about the annoyance, until the report s/he (or other users) Throw at the packager, even when the version that was packaged hasn't diverged that much yet, which does not help end users. I guess the ideal we want is to make sure their build break. What if we do the README in addition to rename contrib/remote-helpers to contrib/obsolete-remote-helpers (or s/obsolete/graduated/)? It will give the packagers three choices and I think it hurts people much less. * The packagers that dump the entirety of contrib/ to somewhere without doing anything to expose the scripts to user's PATH do not have to do anything. The users who chose to pick them up from there notice the missing contrib/remote-helpers, notice "obsolete" (or "graduated"), and read README. * The packagers that pick up from contrib/remote-helpers and arrange the scripts to be on user's PATH will find their build broken, because they are no longer where they expect them to be. They will notice "obsolete"(or "graduated"), and read README. - They can choose to pick them up from "obsolete", perhaps for expediency, perhaps for their change aversion, but that will not last once we grabbed their attention, and they will switch their upstream in some later release once such a choice makes them feel dirty enough. - They can choose to switch their upstream right now in response to the breakage. I would say that the options I see are these three, and I would rank the "warn every time" as less helpful to end-users: - rename contrib/remote-helpers to contrib/obsolete-remote-helpers and add README to point at the upstream. - remove contrib/remote-helpers scripts and add README. - warn every time the user runs the scripts. Or am I reacting to a typo and you meant to say "I would prefer not to instrument"? Your "shipping the warnings to end users who did not package the software will not help" was unclear if you meant the README that has warning or warning message they have to see every time from the instrumented code. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-helpers: point at their upstream repositories
(Sorry if you receive a dup; pobox.com seems to be constipated right now). Jeff King writes: > On Thu, May 15, 2014 at 03:56:29PM -0700, Junio C Hamano wrote: > >> Two announcements for their version 0.2 on the list archive are not >> quite enough to advertise them to their users. > > I do not think this README nor a mention in the release notes will get > their attention either, and many people (and packagers) will continue to > use the stale versions forever until those versions go away. > > I would much rather _replace_ them with a README in the long run, and > people will notice that they are gone, and then use the README to update > their install procedure. > > For 2.0, I am hesitant to do that, though I do not have a problem with a > README like this as a heads-up to prepare packagers for the future. I > say hesitant because people may have been test-packaging 2.0.0-rc3 in > preparation for release, and it will be annoying to them to suddenly > switch. I share the latter two of the above three. I was giving distro packagers a bit more credit for their diligence in knowing what they are packaging than you seem to be in your first point, but I admit that it is a blind faith. > But that being said, this is Felipe's code. While we have a legal right > to distribute it in v2.0, if he would really prefer it out for v2.0, I > would respect that. I am fine with that. > I would prefer to instrument the code with warnings, as that is the sort > of thing a packager moving from -rc3 to -final might not notice, and > shipping the warnings to end users who did not package the software in > the first place will not help them. It is the attention of the packagers > (and source-builders) you want to get. I do agree that a new warning every time it is run will be a more likely to grab users' attention. The conclusion I draw from that shared observation is that the user will be annoyed all the time, without having a power to do anything about the annoyance, until the report s/he (or other users) Throw at the packager, even when the version that was packaged hasn't diverged that much yet, which does not help end users. I guess the ideal we want is to make sure their build break. What if we do the README in addition to rename contrib/remote-helpers to contrib/obsolete-remote-helpers (or s/obsolete/graduated/)? It will give the packagers three choices and I think it hurts people much less. * The packagers that dump the entirety of contrib/ to somewhere without doing anything to expose the scripts to user's PATH do not have to do anything. The users who chose to pick them up from there notice the missing contrib/remote-helpers, notice "obsolete" (or "graduated"), and read README. * The packagers that pick up from contrib/remote-helpers and arrange the scripts to be on user's PATH will find their build broken, because they are no longer where they expect them to be. They will notice "obsolete"(or "graduated"), and read README. - They can choose to pick them up from "obsolete", perhaps for expediency, perhaps for their change aversion, but that will not last once we grabbed their attention, and they will switch their upstream in some later release once such a choice makes them feel dirty enough. - They can choose to switch their upstream right now in response to the breakage. I would say that the options I see are these three, and I would rank the "warn every time" as less helpful to end-users: - rename contrib/remote-helpers to contrib/obsolete-remote-helpers and add README to point at the upstream. - remove contrib/remote-helpers scripts and add README. - warn every time the user runs the scripts. Or am I reacting to a typo and you meant to say "I would prefer not to instrument"? Your "shipping the warnings to end users who did not package the software will not help" was unclear if you meant the README that has warning or warning message they have to see every time from the instrumented code. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 01/44] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 48573e3..3b7e604 100644
--- a/refs.c
+++ b/refs.c
@@ -,7 +,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3347,7 +3348,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3361,7 +3362,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index a07a5d0..50ca06a 100644
--- a/refs.h
+++ b/refs.h
@@ -238,7 +238,8 @@ struct ref_transaction *ref_transaction_begin(void);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -249,7 +250,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -259,7 +260,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 11/44] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 12
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9f328b2..e9c216e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, NULL))
+ die("cannot create ref '%s'", refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index a4fdfc7..06a4fed 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,18 +3397,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die("BUG: create ref with null new_sha1");
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update->new_sha1, new_sha1);
hashclr(update->old_sha1);
update->flags = flags;
update->have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 454871a..bcae348 100644
--- a/refs.h
+++ b/refs.h
@@ -251,11 +251,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err);
/*
* Add a reference deletion to transaction. If have_old is true, then
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 37/44] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 98f3c85..1210345 100644
--- a/refs.c
+++ b/refs.c
@@ -2640,7 +2640,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+ if (!read_ref_full(newrefname, sha1, 1, NULL) &&
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s",
newrefname))) {
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg
---
refs.c | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 2d9789f..4f7ae92 100644
--- a/refs.c
+++ b/refs.c
@@ -3336,6 +3336,12 @@ struct ref_update {
const char refname[FLEX_ARRAY];
};
+enum ref_transaction_status {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
@@ -3345,6 +3351,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_status status;
};
struct ref_transaction *ref_transaction_begin(void)
@@ -3390,6 +3397,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: update on transaction that is not open");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
@@ -3410,6 +3420,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
if (!new_sha1 || is_null_sha1(new_sha1))
die("BUG: create ref with null new_sha1");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: create on transaction that is not open");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
@@ -3430,6 +3443,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: delete on transaction that is not open");
+
update = add_update(transaction, refname);
update->flags = flags;
update->have_old = have_old;
@@ -3500,8 +3516,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
- if (!n)
+ if (transaction->status != REF_TRANSACTION_OPEN)
+ die("BUG: commit on transaction that is not open");
+
+ if (!n) {
+ transaction->status = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3564,6 +3585,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(&ref_cache);
cleanup:
+ transaction->status = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 14/44] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg
---
builtin/replace.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..e8932cd 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -133,7 +133,8 @@ static int replace_object_sha1(const char *object_ref,
unsigned char prev[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (snprintf(ref, sizeof(ref),
"refs/replace/%s",
@@ -156,11 +157,12 @@ static int replace_object_sha1(const char *object_ref,
else if (!force)
die("replace ref '%s' already exists", ref);
- lock = lock_any_ref_for_update(ref, prev, 0, NULL);
- if (!lock)
- die("%s: cannot lock the ref", ref);
- if (write_ref_sha1(lock, repl, NULL) < 0)
- die("%s: cannot update the ref", ref);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
return 0;
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it
In s_update_ref there are two calls that when they fail we return an error based on the errno value. In particular we want to return a specific error if ENOTDIR happened. Both these functions do have failure modes where they may return an error without updating errno, in which case a previous and unrelated ENOTDIR may cause us to return the wrong error. Clear errno before calling any functions if we check errno afterwards. Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0. Signed-off-by: Ronnie Sahlberg --- builtin/fetch.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c..a93c893 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -44,7 +44,7 @@ static struct transport *gtransport; static struct transport *gsecondary; static const char *submodule_prefix = ""; static const char *recurse_submodules_default; -static int shown_url = 0; +static int shown_url; static int option_parse_recurse_submodules(const struct option *opt, const char *arg, int unset) @@ -382,6 +382,8 @@ static int s_update_ref(const char *action, if (!rla) rla = default_rla.buf; snprintf(msg, sizeof(msg), "%s: %s", rla, action); + + errno = 0; lock = lock_any_ref_for_update(ref->name, check_old ? ref->old_sha1 : NULL, 0, NULL); -- 2.0.0.rc3.510.g20c254b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 03/44] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 6357089..686b802 100644
--- a/refs.c
+++ b/refs.c
@@ -3418,7 +3418,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3447,6 +3448,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update->flags,
&update->type, onerr);
if (!update->lock) {
+ if (err)
+ strbuf_addf(err, "Cannot lock the ref '%s'.",
+ update->refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 50ca06a..8669fc9 100644
--- a/refs.h
+++ b/refs.h
@@ -267,9 +267,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/*
* Free an existing transaction and all associated data.
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 02/44] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) and hence ref_transaction_rollback(NULL)
as no-ops. This makes ref_transaction_rollback easier to use and more similar
to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
This allows us to write code like
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 3b7e604..6357089 100644
--- a/refs.c
+++ b/refs.c
@@ -3312,6 +3312,9 @@ void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i < transaction->nr; i++)
free(transaction->updates[i]);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 34/44] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 69623e4..3a5f1e5 100644
--- a/refs.c
+++ b/refs.c
@@ -29,6 +29,11 @@ static inline int bad_ref_char(int ch)
return 0;
}
+/** Used as a flag to ref_transaction_delete when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+
/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
@@ -2330,17 +2335,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r->name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path("%s", r->name));
- unlock_ref(lock);
- try_remove_empty_parents(r->name);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, r->name, r->sha1,
+ REF_ISPRUNING, 1, &err) ||
+ ref_transaction_commit(transaction, NULL, &err)) {
+ ref_transaction_free(transaction);
+ warning("prune_ref: %s", err.buf);
+ strbuf_release(&err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r->name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3533,9 +3545,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- delnames[delnum++] = update->lock->ref_name;
ret |= delete_ref_loose(update->lock, update->type,
err);
+ if (!(update->flags & REF_ISPRUNING))
+ delnames[delnum++] = update->lock->ref_name;
}
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 20/44] refs.c: free the transaction before returning when number of updates is 0
We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
t = ref_transaction_begin()
ref_transaction_commit(t)
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 78312b5..2c3f070 100644
--- a/refs.c
+++ b/refs.c
@@ -3497,8 +3497,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
- if (!n)
+ if (!n) {
+ ref_transaction_free(transaction);
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 05/44] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index a470e51..57ec72a 100644
--- a/refs.c
+++ b/refs.c
@@ -3410,6 +3410,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3417,6 +3418,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
+ if (err)
+ strbuf_addf(err, str, updates[i]->refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]->refname); break;
@@ -3447,7 +3451,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 28/44] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
refs.h | 3 ---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 266a792..93e2cd2 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
@@ -2833,7 +2835,7 @@ static int is_branch(const char *refname)
return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
}
-int write_ref_sha1(struct ref_lock *lock,
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index e0f5f67..0c4d043 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/** Setup reflog before using. **/
int log_ref_setup(const char *refname, char *logfile, int bufsize);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 12/44] refs.c: ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++-
refs.h | 12
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e9c216e..cdb71a8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("delete %s: extra input: %s", refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old, NULL))
+ die("failed transaction delete for %s", refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 06a4fed..a588194 100644
--- a/refs.c
+++ b/refs.c
@@ -3417,19 +3417,25 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+
+ update = add_update(transaction, refname);
update->flags = flags;
update->have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update->old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index bcae348..9b978da 100644
--- a/refs.h
+++ b/refs.h
@@ -265,11 +265,15 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* Add a reference deletion to transaction. If have_old is true, then
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 10/44] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..9f328b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("update %s: failed: %s", refname, err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("verify %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("verify %s: %s", refname, err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
diff --git a/refs.c b/refs.c
index 29cfe78..a4fdfc7 100644
--- a/refs.c
+++ b/refs.c
@@ -3376,19 +3376,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+ update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
update->have_old = have_old;
if (have_old)
hashcpy(update->old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 05e65fa..454871a 100644
--- a/refs.h
+++ b/refs.h
@@ -235,12 +235,16 @@ struct ref_transaction *ref_transaction_begin(void);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+
[PATCH v10 24/44] fetch.c: use a single ref transaction for all ref updates
Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.
Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old cod
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.
This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.
For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
if (current_branch &&
!strcmp(ref->name, current_branch->name) &&
!(update_head_ok || is_bare_repository()) &&
!is_null_sha1(ref->old_sha1)) {
/*
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
In the old code since we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the who
fetch with STORE_REF_ERROR_DF_CONFLICT
In the new code, since we defer committing the transaction until all refs
have been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update t
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.
Signed-off-by: Ronnie Sahlberg
---
builtin/fetch.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8cf70cd..5b0cc31 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
static int shown_url;
+static struct ref_transaction *transaction;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
- char msg[1024];
- char *rla = getenv("GIT_REFLOG_ACTION");
- struct ref_transaction *transaction;
-
if (dry_run)
return 0;
- if (!rla)
- rla = default_rla.buf;
- snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- errno = 0;
- transaction = ref_transaction_begin();
- if (!transaction ||
- ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, NULL) ||
- ref_transaction_commit(transaction, msg, NULL)) {
- ref_transaction_free(transaction);
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- }
- ref_transaction_free(transaction);
+ if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, NULL))
+ return STORE_REF_ERROR_OTHER;
+
return 0;
}
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
goto abort;
}
+ errno = 0;
+ transaction = ref_transaction_begin();
+ if (!transaction) {
+ rc = error(_("cannot start ref transaction\n"));
+ goto abort;
+ }
+
/*
* We do a pass for each fetch_head_status type in their enum order, so
* merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
+ if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+ rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ ST
[PATCH v10 08/44] update-ref.c: log transaction error from the update_ref
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die("Refusing to perform update with empty message.");
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
- return ret;
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
+ return 0;
}
if (end_null)
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 07/44] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index bc444c7..582c74b 100644
--- a/refs.c
+++ b/refs.c
@@ -3301,10 +3301,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) < 0) {
const char *str = "Cannot update the ref '%s'.";
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3425,7 +3428,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3507,7 +3510,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, onerr);
+ update->lock, err, onerr);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 15/44] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/commit.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index d28505a..c429216 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1581,11 +1581,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = &parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1707,16 +1708,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(&author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update("HEAD",
- !current_head
- ? NULL
- : current_head->object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_("cannot lock HEAD ref"));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1725,9 +1716,15 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", sha1,
+ current_head ?
+ current_head->object.sha1 : NULL,
+ 0, !!current_head, &err) ||
+ ref_transaction_commit(transaction, sb.buf, &err)) {
rollback_index_files();
- die(_("cannot update HEAD ref"));
+ die("%s", err.buf);
}
unlink(git_path("CHERRY_PICK_HEAD"));
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 36/44] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Signed-off-by: Ronnie Sahlberg
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 10 --
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 31 +--
refs.h | 5 -
sequencer.c| 4 ++--
walker.c | 6 +++---
12 files changed, 60 insertions(+), 38 deletions(-)
diff --git a/branch.c b/branch.c
index f78a28b..6dfdc2e 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, &err) ||
- ref_transaction_commit(transaction, msg, &err))
+ null_sha1, 0, !forcing, msg, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b888f2..b361a13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, "HEAD", sha1,
current_head ?
current_head->object.sha1 : NULL,
- 0, !!current_head, &err) ||
- ref_transaction_commit(transaction, sb.buf, &err)) {
+ 0, !!current_head, sb.buf, &err) ||
+ ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5b0cc31..4603cb6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
+ char msg[1024];
+ char *rla = getenv("GIT_REFLOG_ACTION");
+
if (dry_run)
return 0;
+ if (!rla)
+ rla = default_rla.buf;
+ snprintf(msg, sizeof(msg), "%s: %s", rla, action);
if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, NULL))
+ ref->old_sha1, 0, check_old, msg, NULL))
return STORE_REF_ERROR_OTHER;
return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
- if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+ if (ref_transaction_commit(transaction, NULL))
rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5534138..324a220 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,8 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "shallow error";
if (ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, &err))
+ new_sha1, old_sha1, 0, 1, "push",
+ &err))
return "failed to update";
return NULL; /* good */
}
@@ -823,7 +824,7 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
- if (ref_transaction_commit(transaction, "push", &err))
+ if (ref_transaction_commit(transaction, &err))
error("%s", err.buf);
ref_transaction_free(transaction);
strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index af7f72d..4fa74c1 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), &err) ||
- ref_transaction_commit(transaction, NULL, &err))
+
[PATCH v10 29/44] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 93e2cd2..2d9789f 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,7 +2126,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 0c4d043..90c7fb4 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 09/44] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, &err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
return 0;
}
diff --git a/refs.c b/refs.c
index 582c74b..29cfe78 100644
--- a/refs.c
+++ b/refs.c
@@ -3439,8 +3439,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i < n; i++)
@@ -3450,22 +3449,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]->refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]->refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]->refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3480,7 +3470,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3492,7 +3482,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update->have_old ?
update->old_sha1 : NULL),
update->flags,
- &update->type, onerr);
+ &update->type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
@@ -3510,7 +3501,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, err, onerr);
+ update->lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 8669fc9..05e65fa 100644
--- a/refs.h
+++ b/refs.h
@@ -271,8 +271,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/*
* Free an existing transaction and all associated data.
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 04/44] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.
Signed-off-by: Ronnie Sahlberg
---
cache.h| 2 ++
lockfile.c | 21 -
refs.c | 25 +++--
3 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/cache.h b/cache.h
index 8c6cdc2..48548d6 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_strbuf(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..9e04b43 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk->fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
if (err == EEXIST) {
- strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
+ strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
"If no other git process is currently running, this
probably means a\n"
"git process crashed in this repository earlier. Make sure
no other git\n"
"process is running and remove the file manually to
continue.",
absolute_path(path), strerror(err));
} else
- strbuf_addf(&buf, "Unable to create '%s.lock': %s",
+ strbuf_addf(buf, "Unable to create '%s.lock': %s",
absolute_path(path), strerror(err));
- return strbuf_detach(&buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error("%s", msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, &buf);
+ error("%s", buf.buf);
+ strbuf_release(&buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die("%s", unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_strbuf(path, err, &buf);
+ die("%s", buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
diff --git a/refs.c b/refs.c
index 686b802..a470e51 100644
--- a/refs.c
+++ b/refs.c
@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(&ref_cache);
int error = 0;
+ int save_errno;
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
@@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
&packed_ref_cache->lock->fd);
- if (commit_lock_file(packed_ref_cache->lock))
+ if (commit_lock_file(packed_ref_cache->lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache->lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
return error;
}
@@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-static int repack_without_refs(const char **refnames, int n)
+static int repack_without_refs(const char **refnames, int n, struct strbuf
*err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i < n; i++)
@@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames,
int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_strbuf(git_path("packed-refs"), errno,
+ err);
+ return 1;
+ }
unable_to_lock_error(git_path("packed-refs"), errno);
return error("cannot delete '%s' from packed refs",
refnames[i]);
}
@@ -2470,12 +2479,16 @@ static
[PATCH v10 25/44] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg
---
builtin/receive-pack.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..5534138 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct strbuf err = STRBUF_INIT;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd->old_sha1;
unsigned char *new_sha1 = cmd->new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
update_shallow_ref(cmd, si))
return "shallow error";
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error("failed to lock %s", name);
- return "failed to lock";
- }
- if (write_ref_sha1(lock, new_sha1, "push")) {
- return "failed to write"; /* error() already called */
- }
+ if (ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1, &err))
+ return "failed to update";
return NULL; /* good */
}
}
@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
checked_connectivity = 1;
+ transaction = ref_transaction_begin();
for (cmd = commands; cmd; cmd = cmd->next) {
if (cmd->error_string)
continue;
@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
+ if (ref_transaction_commit(transaction, "push", &err))
+ error("%s", err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
if (shallow_update && !checked_connectivity)
error("BUG: run 'git fsck' for safety.\n"
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 42/44] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 43 +--
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index c5d41bb..3967333 100644
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+ int i;
+ for(i = 0; i < data->skipnum; i++) {
+ if (!strcmp(entry->name, data->skip[i])) {
+ return 0;
+ }
+ }
if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
return 0;
if (names_conflict(data->refname, entry->name)) {
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
*/
static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2037,7 +2051,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2084,7 +2099,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing &&
-!is_refname_available(refname, NULL, get_packed_refs(&ref_cache)))
{
+!is_refname_available(refname, NULL,
+ get_packed_refs(&ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2142,7 +2159,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2620,6 +2637,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
const char *symref = NULL;
+ if (!strcmp(oldrefname, newrefname))
+ return 0;
+
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2630,10 +2650,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error("refname %s not found", oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(&ref_cache)))
+ if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(&ref_cache), NULL, 0))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_re
[PATCH v10 32/44] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 04de777..b1acf51 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,25 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) < 0) {
- const char *str = "Cannot update the ref '%s'.";
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3539,14 +3520,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update->new_sha1)) {
- ret = update_ref_write(msg,
- update->refname,
- update->new_sha1,
- update->lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update->lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update->lock, update->new_sha1,
+msg);
+ update->lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = "Cannot update the ref '%s'.";
+
+ if (err)
+ strbuf_addf(err, str, update->refname);
goto cleanup;
+ }
}
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 43/44] refs.c: make rename_ref use a transaction
Change rename_ref to use a single transaction to perform the ref rename.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 73 ++
1 file changed, 20 insertions(+), 53 deletions(-)
diff --git a/refs.c b/refs.c
index 3967333..71e3059 100644
--- a/refs.c
+++ b/refs.c
@@ -2630,9 +2630,10 @@ static int rename_tmp_log(const char *newrefname)
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
+ unsigned char sha1[20];
+ int flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
const char *symref = NULL;
@@ -2643,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
- symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
+ symref = resolve_ref_unsafe(oldrefname, sha1, 1, &flag);
if (flag & REF_ISSYMREF)
return error("refname %s is a symbolic ref, renaming it is not
supported",
oldrefname);
@@ -2665,62 +2666,28 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
return error("unable to pack refs");
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
- error("unable to delete old %s", oldrefname);
- goto rollback;
- }
-
- if (!read_ref_full(newrefname, sha1, 1, NULL) &&
- delete_ref(newrefname, sha1, REF_NODEREF)) {
- if (errno==EISDIR) {
- if (remove_empty_directories(git_path("%s",
newrefname))) {
- error("Directory not empty: %s", newrefname);
- goto rollback;
- }
- } else {
- error("unable to delete existing %s", newrefname);
- goto rollback;
- }
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, oldrefname, sha1,
+ REF_NODEREF | REF_ISPACKONLY,
+ 1, NULL, &err) ||
+ ref_transaction_update(transaction, newrefname, sha1,
+ NULL, 0, 0, logmsg, &err) ||
+ ref_transaction_commit(transaction, &err)) {
+ ref_transaction_free(transaction);
+ error("rename_ref failed: %s", err.buf);
+ strbuf_release(&err);
+ goto rollbacklog;
}
+ ref_transaction_free(transaction);
if (log && rename_tmp_log(newrefname))
- goto rollback;
-
- logmoved = log;
-
- lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error("unable to lock %s for update", newrefname);
- goto rollback;
- }
- lock->force_write = 1;
- hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
- error("unable to write current sha1 into %s", newrefname);
- goto rollback;
- }
-
- return 0;
-
- rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error("unable to lock %s for rollback", oldrefname);
goto rollbacklog;
- }
- lock->force_write = 1;
- flag = log_all_ref_updates;
- log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
- error("unable to write current sha1 into %s", oldrefname);
- log_all_ref_updates = flag;
+ return 0;
rollbacklog:
- if (logmoved && rename(git_path("logs/%s", newrefname),
git_path("logs/%s", oldrefname)))
- error("unable to restore logfile %s from %s: %s",
- oldrefname, newrefname, strerror(errno));
- if (!logmoved && log &&
+ if (log &&
rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 38/44] refs.c: pack all refs before we start to rename a ref
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.
Signed-off-by: Ronnie Sahlberg
---
refs.c| 3 +++
t/t3200-branch.sh | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 1210345..f4899e0 100644
--- a/refs.c
+++ b/refs.c
@@ -2635,6 +2635,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return error("unable to move logfile logs/%s to
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
+ if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+ return error("unable to pack refs");
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master &&
test_must_fail git branch -m master2 master3 &&
git symbolic-ref refs/heads/master2 &&
- test_path_is_file .git/refs/heads/master &&
+ test_path_is_missing .git/refs/heads/master &&
test_path_is_missing .git/refs/heads/master3
'
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 33/44] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial "refs/"
part of the ref path name, the initial "refs/" that this caller had already
stripped off before calling lock_ref_sha1.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index b1acf51..69623e4 100644
--- a/refs.c
+++ b/refs.c
@@ -2126,15 +2126,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath("refs/%s", refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2339,8 +2330,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r->name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path("%s", r->name));
unlock_ref(lock);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 39/44] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index f4899e0..f63a356 100644
--- a/refs.c
+++ b/refs.c
@@ -2044,6 +2044,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ return NULL;
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
@@ -2135,8 +2138,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 31/44] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 4f7ae92..04de777 100644
--- a/refs.c
+++ b/refs.c
@@ -3283,24 +3283,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = "Cannot lock the ref '%s'.";
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3537,12 +3519,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = update_ref_lock(update->refname,
- (update->have_old ?
- update->old_sha1 : NULL),
- update->flags,
- &update->type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update->lock = lock_any_ref_for_update(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 40/44] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index f63a356..564feb6 100644
--- a/refs.c
+++ b/refs.c
@@ -3511,12 +3511,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = lock_any_ref_for_update(update->refname,
- (update->have_old ?
- update->old_sha1 :
- NULL),
- update->flags,
- &update->type);
+ update->lock = lock_ref_sha1_basic(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only
Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.
At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/refs.c b/refs.c
index 564feb6..c5d41bb 100644
--- a/refs.c
+++ b/refs.c
@@ -33,6 +33,10 @@ static inline int bad_ref_char(int ch)
* pruned.
*/
#define REF_ISPRUNING 0x0100
+/** Deletion of a ref that only exists as a packed ref in which case we do not
+ * need to lock the loose ref during the transaction.
+ */
+#define REF_ISPACKONLY 0x0200
/*
* Try to read one refname component from the front of refname. Return
@@ -3366,6 +3370,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (transaction->status != REF_TRANSACTION_OPEN)
die("BUG: update on transaction that is not open");
+ if (flags & REF_ISPACKONLY)
+ die("BUG: REF_ISPACKONLY can not be used with updates");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
@@ -3392,6 +3399,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
if (transaction->status != REF_TRANSACTION_OPEN)
die("BUG: create on transaction that is not open");
+ if (flags & REF_ISPACKONLY)
+ die("BUG: REF_ISPACKONLY can not be used with creates");
+
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
@@ -3507,10 +3517,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (ret)
goto cleanup;
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->flags & REF_ISPACKONLY)
+ delnames[delnum++] = update->refname;
+ }
+
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->flags & REF_ISPACKONLY)
+ continue;
+
update->lock = lock_ref_sha1_basic(update->refname,
(update->have_old ?
update->old_sha1 :
@@ -3548,6 +3568,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->flags & REF_ISPACKONLY)
+ continue;
+
if (update->lock) {
ret |= delete_ref_loose(update->lock, update->type,
err);
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 23/44] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg
---
builtin/fetch.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..8cf70cd 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv("GIT_REFLOG_ACTION");
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
if (dry_run)
return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
errno = 0;
- lock = lock_any_ref_for_update(ref->name,
- check_old ? ref->old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, NULL) ||
+ ref_transaction_commit(transaction, msg, NULL)) {
+ ref_transaction_free(transaction);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 26/44] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index cb3f5af..9c76c73 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,22 @@ static void dump_tags(void)
{
static const char *msg = "fast-import";
struct tag *t;
- struct ref_lock *lock;
char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin();
for (t = first_tag; t; t = t->next_tag) {
- sprintf(ref_name, "tags/%s", t->name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
- failure |= error("Unable to update %s", ref_name);
+ sprintf(ref_name, "refs/tags/%s", t->name);
+
+ if (ref_transaction_update(transaction, ref_name, t->sha1,
+ NULL, 0, 0, &err))
+ failure |= error("%s", err.buf);
}
+ if (failure || ref_transaction_commit(transaction, msg, &err))
+ failure |= error("%s", err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
}
static void dump_marks_helper(FILE *f,
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 27/44] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg
---
walker.c | 51 ++-
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..cbe5686 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i < targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error("Can't lock ref %s", write_ref[i]);
- goto unlock_and_fail;
- }
- }
+ transaction = ref_transaction_begin();
+ if (!transaction)
+ return -1;
if (!walker->get_recover)
for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (interpret_target(walker, target[i], &sha1[20 * i])) {
error("Could not interpret response from server '%s' as
something to pull", target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
(unknown)");
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ sprintf(ref_name, "refs/%s", write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name,
+ &sha1[20 * i], NULL,
+ 0, 0, &err))
+ goto rollback_and_fail;
+ }
+
+ if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i < targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ free(msg);
+ strbuf_release(&err);
+ ref_transaction_free(transaction);
return -1;
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info
[PATCH v10 44/44] refs.c: remove forward declaration of write_ref_sha1
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/refs.c b/refs.c
index 71e3059..bf84306 100644
--- a/refs.c
+++ b/refs.c
@@ -260,8 +260,6 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
- const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 21/44] refs.c: ref_transaction_commit should not free the transaction
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by calling
ref_transaction_free.
By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_free
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval)) ||
(ref_transaction_commit(t, action, &err) && !(t = NULL))) {
ref_transaction_free(t);
we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval) ||
ref_transaction_commit(&t, action, &err)) {
ref_transaction_free(t);
... die/return ...
ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg
---
branch.c | 1 +
builtin/commit.c | 1 +
builtin/replace.c| 1 +
builtin/tag.c| 1 +
builtin/update-ref.c | 1 +
fast-import.c| 4 ++--
refs.c | 12 +---
refs.h | 8 +++-
sequencer.c | 4 ++--
9 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/branch.c b/branch.c
index 8bf7588..f78a28b 100644
--- a/branch.c
+++ b/branch.c
@@ -304,6 +304,7 @@ void create_branch(const char *head,
null_sha1, 0, !forcing, &err) ||
ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
}
if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c429216..6b888f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1726,6 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
rollback_index_files();
die("%s", err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/replace.c b/builtin/replace.c
index e8932cd..af7f72d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -164,6 +164,7 @@ static int replace_object_sha1(const char *object_ref,
ref_transaction_commit(transaction, NULL, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index b05f9a5..30b471c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
0, !is_null_sha1(prev), &err) ||
ref_transaction_commit(transaction, NULL, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index cdb71a8..4c2145e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index 60d4538..cb3f5af 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1710,13 +1710,13 @@ static int update_branch(struct branch *b)
if (!transaction ||
ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
0, 1, &err) ||
- (ref_transaction_commit(transaction, msg, &err) &&
-!(transaction = NULL))) {
+ ref_transaction_commit(transaction, msg, &err)) {
ref_transaction_free(transaction);
error("%s", err.buf);
strbuf_release(&err);
return -1;
}
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/refs.c b/refs.c
index 2c3f070..266a792 100644
--- a/refs.c
+++ b/refs.c
@@ -3446,10 +3446,10 @@ int update_ref(const char *action, const char *refname,
struct strbuf err = STRBUF_INIT;
t = ref_transaction_begin();
- if ((!t ||
+ if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
- !!oldval, &err)) ||
- (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+ !!oldval, &err) ||
+ ref_transaction_commit(t, action, &err)) {
c
[PATCH v10 19/44] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index a588194..78312b5 100644
--- a/refs.c
+++ b/refs.c
@@ -3442,11 +3442,28 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin();
+ if ((!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, &err)) ||
+ (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+ const char *str = "update_ref failed for ref '%s': %s";
+
+ ref_transaction_free(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf); break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf); break;
+ case UPDATE_REFS_QUIET_ON_ERR: break;
+ }
+ strbuf_release(&err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 17/44] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..60d4538 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,44 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = "fast-import";
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b->name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b->sha1)) {
if (b->delete)
delete_ref(b->name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
- if (!lock)
- return error("Unable to lock %s", b->name);
if (!force_update && !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b->sha1, 0);
- if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
+ if (!old_cmit || !new_cmit)
return error("Branch %s is missing commits.", b->name);
- }
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning("Not updating %s"
" (new tip %s does not contain %s)",
b->name, sha1_to_hex(b->sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b->sha1, msg) < 0)
- return error("Unable to update %s", b->name);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+ 0, 1, &err) ||
+ (ref_transaction_commit(transaction, msg, &err) &&
+!(transaction = NULL))) {
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return -1;
+ }
return 0;
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 35/44] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 3a5f1e5..b16cf4c 100644
--- a/refs.c
+++ b/refs.c
@@ -2495,11 +2495,6 @@ static int repack_without_refs(const char **refnames,
int n, struct strbuf *err)
return ret;
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(&refname, 1, NULL);
-}
-
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *e, int rc)
{
@@ -2543,24 +2538,18 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
- if (!lock)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 && !is_null_sha1(sha1), NULL) ||
+ ref_transaction_commit(transaction, NULL, NULL)) {
+ ref_transaction_free(transaction);
return 1;
- ret |= delete_ref_loose(lock, flag, NULL);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock->ref_name);
-
- unlink_or_warn(git_path("logs/%s", lock->ref_name));
- clear_loose_ref_cache(&ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 13/44] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Signed-off-by: Ronnie Sahlberg
---
builtin/tag.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..b05f9a5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, &buf, &opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_("%s: cannot lock the ref"), ref.buf);
- if (write_ref_sha1(lock, object, NULL) < 0)
- die(_("%s: cannot update the ref"), ref.buf);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 16/44] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..b047fb8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_("Failed to lock HEAD during fast_forward_to"));
strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", to, from,
+ 0, !unborn, &err) ||
+ (ref_transaction_commit(transaction, sb.buf, &err) &&
+!(transaction = NULL))) {
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&sb);
+ strbuf_release(&err);
+ return -1;
+ }
strbuf_release(&sb);
- return ret;
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 18/44] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.
If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
branch.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..8bf7588 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_("Not a valid branch point: '%s'."), start_name);
hashcpy(sha1, commit->object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_("Failed to lock ref for update"));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, "branch: Reset to %s",
start_name);
@@ -301,13 +291,24 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, "branch: Created from %s",
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, &err) ||
+ ref_transaction_commit(transaction, msg, &err))
+ die("%s", err.buf);
+ }
+
if (real_ref && track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) < 0)
- die_errno(_("Failed to write ref"));
-
strbuf_release(&ref);
free(real_ref);
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 06/44] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Signed-off-by: Ronnie Sahlberg
---
refs.c | 39 +--
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 57ec72a..bc444c7 100644
--- a/refs.c
+++ b/refs.c
@@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(&refname, 1, NULL);
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc < 0 && err != ENOENT) {
+ strbuf_addf(e, "unable to %s %s: %s",
+ op, file, strerror(errno));
+ errno = err;
+ }
+ return rc;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable("unlink", file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+ res = unlink_or_err(lock->lk->filename, err);
lock->lk->filename[i] = '.';
- if (err && errno != ENOENT)
+ if (res && errno != ENOENT) {
+ if (err)
+ strbuf_addf(err,
+ "failed to delete loose ref '%s'",
+ lock->lk->filename);
return 1;
+ }
}
return 0;
}
@@ -2514,7 +2540,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
if (!lock)
return 1;
- ret |= delete_ref_loose(lock, flag);
+ ret |= delete_ref_loose(lock, flag, NULL);
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -3494,7 +3520,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
- ret |= delete_ref_loose(update->lock, update->type);
+ ret |= delete_ref_loose(update->lock, update->type,
+ err);
}
}
--
2.0.0.rc3.510.g20c254b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 00/44] Use ref transactions for all ref updates
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 10: - Add err argument to _update/_delete/_create - Remove the ref_transaction_rollback function - Other changes based on JN's reviews. Version 9: - Lots of updates to error messages based on JN's review. Version 8: - Updates after review by JN - Improve commit messages - Add a patch that adds an err argument to repack_without_refs - Add a patch that adds an err argument to delete_loose_ref - Better document that a _update/_delete/_create failure means the whole transaction has failed and needs rollback. Version 7: - Updated commit messages per JNs review comments. - Changed REF_ISPRUNING and REF_ISPACKONLY to be private flags and not exposed through refs.h Version 6: - Convert all updates in refs.c to use transactions too. Version 5: - Reword commit messages for having _create/_delete/_update returning success/failure. There are no conditions yet that return an error from these failures but there will be in the future. So we still check the return from these functions in the callers in preparation for this. - Don't leak memory by just passing a strbuf_detach() pointer to functions. Use .buf and explicitely strbuf_release the data afterwards. - Remove the function update_ref_lock. - Remove the function update_ref_write. - Track transaction status and die(BUG:) if we call _create/_delete/_update/ _commit for a transaction that is not OPEN. Version 4: - Rename patch series from "Use ref transactions from most callers" to "Use ref transactions for all ref updates". - Convert all external ref writes to use transactions and make write_ref_sha1 and lock_ref_sha1 static functions. - Change the ref commit and free handling so we no longer pass pointer to pointer to _commit. _commit no longer frees the transaction. The caller MUST call _free itself. - Change _commit to take a strbuf pointer instead of a char* for error reporting back to the caller. - Re-add the walker patch after fixing it. Version 3: - Remove the walker patch for now. Walker needs more complex solution so defer it until the basics are done. - Remove the onerr argument to ref_transaction_commit(). All callers that need to die() on error now have to do this explicitely. - Pass an error string from ref_transaction_commit() back to the callers so that they can craft a nice error message upon failures. - Make ref_transaction_rollback() accept NULL as argument. - Change ref_transaction_commit() to take a pointer to pointer argument for the transaction and have it clear the callers pointer to NULL when invoked. This allows for much nicer handling of transaction rollback on failure. Version 2: - Add a patch to ref_transaction_commit to make it honor onerr even if the error triggered in ref_Transaction_commit itself rather than in a call to other functions (that already honor onerr). - Add a patch to make the update_ref() helper function use transactions internally. - Change ref_transaction_update to die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change ref_transaction_create to die() instead of error() if new_sha1 is false but we pass it a null_sha1. - Change ref_transaction_delete die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change several places to do if(!transaction || ref_transaction_update() || ref_Transaction_commit()) die(generic-message) instead of checking each step separately and having a different message for each failure. Most users are likely not interested in what step of the transaction failed and only whether it failed or not. - Change commit.c to only pass a pointer to ref_transaction_update iff current_head is non-NULL. The previous patch used to compute a garbage pointer for current_head->object.sha1 and relied on the fact that ref_transaction_update would not try to dereference this pointer if !!current_head was 0. - Updated commit message for the walker_fetch change to try to justify why the change in locking semantics should not be harmful. Ronnie Sahlberg (44): refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error log
Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
Nguyễn Thái Ngọc Duy wrote:
> core.commentChar starts with '#' as in default but if it's already in
> the prepared message, find another one among a small subset. This
> should stop surprises because git strips some lines unexpectedly.
Probably worth mentioning this only kicks in if someone explicitly
configures [core] commentchar = auto.
Would it be a goal to make 'auto' the default eventually if people
turn out to like it?
[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
> return ket;
> }
>
> +static void adjust_comment_line_char(const struct strbuf *sb)
> +{
> + char candidates[] = " @!#$%^&|:;~";
This prefers '@' over '#'. Intended?
[...]
> + char *candidate;
> + const char *p;
> +
> + if (!sb->len)
> + return;
> +
> + if (!strchr(candidates, comment_line_char))
> + candidates[0] = comment_line_char;
Could do
if (!memchr(sb->buf, comment_line_char, sb->len))
return;
to solve the precedence problem. The comment_line_char not appearing
in the message is the usual case and handling it separately means it
gets handled faster.
[...]
> --- a/config.c
> +++ b/config.c
> @@ -829,6 +829,8 @@ static int git_default_core_config(const char *var, const
> char *value)
> if (!ret) {
> if (comment[0] && !comment[1])
> comment_line_char = comment[0];
> + else if (!strcasecmp(comment, "auto"))
> + auto_comment_line_char = 1;
Is there a way to disable 'auto' if 'auto' is already set?
comment_line_char still can be set and matters when 'auto' is set.
Should they be separate settings?
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -563,4 +563,29 @@ test_expect_success 'commit --status with custom comment
> character' '
[...]
> + GIT_EDITOR=.git/FAKE_EDITOR test_must_fail \
Shells make it obnoxiously hard to set a one-shot envvar while
calling a function without the setting leaking into later commands.
(
test_set_editor .git/FAKE_EDITOR &&
test_must_fail ...
)
or
test_must_fail env GIT_EDITOR=.git/FAKE_EDITOR ...
should do the trick.
Thanks and hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] config: be strict on core.commentChar
Nguyễn Thái Ngọc Duy wrote:
> --- a/config.c
> +++ b/config.c
> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var,
> const char *value)
> if (!strcmp(var, "core.commentchar")) {
> const char *comment;
> int ret = git_config_string(&comment, var, value);
> - if (!ret)
> - comment_line_char = comment[0];
> + if (!ret) {
> + if (comment[0] && !comment[1])
> + comment_line_char = comment[0];
> + else
> + return error("core.commentChar should only be
> one character");
> + }
Perhaps, to decrease indentation a little:
if (ret)
return ret;
if (comment[0] && !comment[1])
comment_line_char = comment[0];
else
return error(...);
return 0;
[...]
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with
> submodule summary)" '
> test_i18ncmp expect output
> '
>
> -test_expect_success "status (core.commentchar with two chars with submodule
> summary)" '
> - test_config core.commentchar ";;" &&
> - git -c status.displayCommentPrefix=true status >output &&
> - test_i18ncmp expect output
Could keep the test to avoid regressions:
test_config core.commentchar ";;" &&
test_must_fail git -c status.displayCommentPrefix=true status
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v3 09/31] Test suite: properly check the exit status of commands.
On Fri, May 16, 2014 at 04:45:56PM +0200, Per Cederqvist wrote:
> The "cmd" and "shouldfail" functions checked the exit status of the
> replace_path function instead of the actual command that was running.
> (The $? construct checks the exit status of the last command in a
> pipeline, not the first command.)
>
> Updated t-032.sh, which used "shouldfail" instead of "cmd" in one
> place. (The comment in the script makes it clear that the command is
> expected to succeed.)
>
> Signed-off-by: Per Cederqvist
> ---
> regression/scaffold | 17 +++--
> regression/t-032.sh | 2 +-
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/regression/scaffold b/regression/scaffold
> index 5c8b73e..e4d7487 100644
> --- a/regression/scaffold
> +++ b/regression/scaffold
> @@ -51,18 +51,23 @@ function filter_dd
> function cmd
> {
> echo "% $@"
> - "$@" 2>&1 | replace_path && return 0
> - return 1
> + (
> + exec 3>&1
> + rv=`(("$@" 2>&1; echo $? >&4) | replace_path >&3 ) 4>&1`
Wow. This took a while to decipher :)
Signed-off-by: Josef 'Jeff' Sipek
> + exit $rv
> + )
> + return $?
> }
>
> # usage: shouldfail ..
> function shouldfail
> {
> echo "% $@"
> - (
> - "$@" 2>&1 || return 0
> - return 1
> - ) | replace_path
> + ! (
> + exec 3>&1
> + rv=`(("$@" 2>&1; echo $? >&4) | replace_path >&3 ) 4>&1`
> + exit $rv
> + )
> return $?
> }
>
> diff --git a/regression/t-032.sh b/regression/t-032.sh
> index b1d5f19..bba401e 100755
> --- a/regression/t-032.sh
> +++ b/regression/t-032.sh
> @@ -28,7 +28,7 @@ shouldfail guilt import -P foo3 foo
> cmd guilt import -P foo2 foo
>
> # ok
> -shouldfail guilt import foo
> +cmd guilt import foo
>
> # duplicate patch name (implicit)
> shouldfail guilt import foo
> --
> 1.8.3.1
>
--
Fact: 28.1% of all statistics are generated randomly.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
