Re: [PATCH v3 1/2] fsck.c: modify fsck_ident() and fsck_commit()

[Eric Sunshine]

On Mon, Mar 24, 2014 at 8:08 AM, Ashwin Jha  wrote:
> Subject: fsck.c: modify fsck_ident() and fsck_commit()

The subject should summarize the change while being concise and
expressive. The above is a bit lacking. A better subject might be:

Subject: fsck: add missing 'const's

> In fsck_ident(): Replace argument char **ident with const char **ident
> In fsck_commit(): Replace char *buffer with const char *buffer

No need to repeat in prose what the patch itself already states more
concisely and precisely. You can drop this part.

> In both the cases, referenced memory addresses are not modified. So, it
> will be a good practice, to declare them as const.

The subject suggested above ("add missing 'const's") implies that the
referenced memory is never modified, so it's not really necessary to
explain it here too. Stating that it's good practice may be a
reasonable way to justify the change, although is also rather obvious.
I'd say that the suggested subject alone is a sufficient commit
message for this patch, though you and/or Junio might feel otherwise.

> Signed-off-by: Ashwin Jha 
> ---
>
> Change in fsck_commit() and fsck_ident() as per the discussion with
> Eric (follow at [1]).
> [1]: 
> http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html

Providing a link to the previous attempt is indeed courteous to
reviewers. Since reviewer time is precious (due to this being a
high-volume mailing list), it is possible to be even more helpful by
providing more detail in your summary of changes. For instance,
instead of saying "Change in ... as per discussion", you might say:

Changes since v2 [1]:

Expanded to 2-patch series.

patch 1/1: add missing 'const's to fsck_ident() argument and
fsck_commit() 'buffer' variable so that 2/2 doesn't have to
cast-away constness from the result of skip_prefix().

patch 2/2: dropped the now unnecessary casts

The patch itself looks fine, thanks.

>  fsck.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 64bf279..b5f017f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, 
> fsck_error error_func)
> return retval;
>  }
>
> -static int fsck_ident(char **ident, struct object *obj, fsck_error 
> error_func)
> +static int fsck_ident(const char **ident, struct object *obj, fsck_error 
> error_func)
>  {
> char *end;
>
> @@ -284,7 +284,7 @@ static int fsck_ident(char **ident, struct object *obj, 
> fsck_error error_func)
>
>  static int fsck_commit(struct commit *commit, fsck_error error_func)
>  {
> -   char *buffer = commit->buffer;
> +   const char *buffer = commit->buffer;
> unsigned char tree_sha1[20], sha1[20];
> struct commit_graft *graft;
> int parents = 0;
> --
> 1.9.1
--
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/3] test-lib: Document short options in t/README

[Eric Sunshine]

On Mon, Mar 24, 2014 at 4:49 AM, Ilya Bobyr  wrote:
> Most arguments that could be provided to a test have short forms.
> Unless documented the only way to learn then is to read the code.

s/then/them/

(Also, add a comma after "documented".)

> Signed-off-by: Ilya Bobyr 
> ---
>  t/README |   10 +-
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/README b/t/README
> index caeeb9d..ccb5989 100644
> --- a/t/README
> +++ b/t/README
> @@ -71,7 +71,7 @@ You can pass --verbose (or -v), --debug (or -d), and 
> --immediate
>  (or -i) command line argument to the test, or by setting GIT_TEST_OPTS
>  appropriately before running "make".
>
> ---verbose::
> +-v,--verbose::
> This makes the test more verbose.  Specifically, the
> command being run and their output if any are also
> output.
> @@ -81,7 +81,7 @@ appropriately before running "make".
> numbers matching .  The number matched against is
> simply the running count of the test within the file.
>
> ---debug::
> +-d,--debug::
> This may help the person who is developing a new test.
> It causes the command defined with test_debug to run.
> The "trash" directory (used to store all temporary data
> @@ -89,18 +89,18 @@ appropriately before running "make".
> failed tests so that you can inspect its contents after
> the test finished.
>
> ---immediate::
> +-i,--immediate::
> This causes the test to immediately exit upon the first
> failed test. Cleanup commands requested with
> test_when_finished are not executed if the test failed,
> in order to keep the state for inspection by the tester
> to diagnose the bug.
>
> ---long-tests::
> +-l,--long-tests::
> This causes additional long-running tests to be run (where
> available), for more exhaustive testing.
>
> ---valgrind=::
> +-v,--valgrind=::
> Execute all Git binaries under valgrind tool  and exit
> with status 126 on errors (just like regular tests, this will
> only stop the test script when running under -i).
> --
> 1.7.9
>
--
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] Allow --pretty to be passed to git-describe.

[Eric Sunshine]

On Mon, Mar 24, 2014 at 9:04 PM, Cyril Roelandt  wrote:
> In some cases, ony may want to find the the most recent tag that is reachable

s/ony/one/

> from a commit and have it pretty printed, using the formatting options 
> available
> in git-log and git-show.
>
> Signed-off-by: Cyril Roelandt 
> ---
>  Documentation/git-describe.txt |  4 
>  builtin/describe.c | 39 ++-
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index d20ca40..fae4713 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -93,6 +93,10 @@ OPTIONS
> This is useful when you wish to not match tags on branches merged
> in the history of the target commit.
>
> +include::pretty-options.txt[]
> +
> +include::pretty-formats.txt[]
> +
>  EXAMPLES
>  
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 24d740c..4c0ebae 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -8,8 +8,8 @@
>  #include "diff.h"
>  #include "hashmap.h"
>  #include "argv-array.h"
> +#include "revision.h"
>
> -#define SEEN   (1u << 0)
>  #define MAX_TAGS   (FLAG_BITS - 1)
>
>  static const char * const describe_usage[] = {
> @@ -30,6 +30,8 @@ static int have_util;
>  static const char *pattern;
>  static int always;
>  static const char *dirty;
> +static const char *fmt_pretty;
> +static enum cmit_fmt commit_format;
>
>  /* diff-index command arguments to check if working tree is dirty. */
>  static const char *diff_index_args[] = {
> @@ -266,8 +268,14 @@ static void describe(const char *arg, int last_one)
>  * Exact match to an existing ref.
>  */
> display_name(n);
> -   if (longformat)
> +   if (longformat) {
> show_suffix(0, n->tag ? n->tag->tagged->sha1 : sha1);
> +   } else if (fmt_pretty) {
> +   struct strbuf buf = STRBUF_INIT;
> +   pp_commit_easy(commit_format, cmit, &buf);
> +   printf("%s", buf.buf);
> +   strbuf_release(&buf);
> +   }
> if (dirty)
> printf("%s", dirty);
> printf("\n");
> @@ -386,9 +394,16 @@ static void describe(const char *arg, int last_one)
> }
> }
>
> -   display_name(all_matches[0].name);
> -   if (abbrev)
> -   show_suffix(all_matches[0].depth, cmit->object.sha1);
> +   if (fmt_pretty) {
> +   struct strbuf buf = STRBUF_INIT;
> +   pp_commit_easy(commit_format, cmit, &buf);
> +   printf("%s", buf.buf);
> +   strbuf_release(&buf);
> +   } else {
> +   display_name(all_matches[0].name);
> +   if (abbrev)
> +   show_suffix(all_matches[0].depth, cmit->object.sha1);
> +   }
> if (dirty)
> printf("%s", dirty);
> printf("\n");
> @@ -419,6 +434,10 @@ int cmd_describe(int argc, const char **argv, const char 
> *prefix)
> {OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
> N_("append  on dirty working tree (default: 
> \"-dirty\")"),
> PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
> +   OPT_STRING(0, "pretty",  &fmt_pretty, N_("pattern"),
> +  N_("pretty print")),
> +   OPT_STRING(0, "format",  &fmt_pretty, N_("pattern"),
> +  N_("pretty print")),
> OPT_END(),
> };
>
> @@ -437,6 +456,9 @@ int cmd_describe(int argc, const char **argv, const char 
> *prefix)
> if (longformat && abbrev == 0)
> die(_("--long is incompatible with --abbrev=0"));
>
> +   if (longformat && fmt_pretty)
> +   die(_("--long is incompatible with --pretty"));
> +
> if (contains) {
> struct argv_array args;
>
> @@ -458,6 +480,13 @@ int cmd_describe(int argc, const char **argv, const char 
> *prefix)
> return cmd_name_rev(args.argc, args.argv, prefix);
> }
>
> +   if (fmt_pretty) {
> +   struct rev_info rev;
> +   init_revisions(&rev, prefix);
> +   get_commit_format(fmt_pretty, &rev);
> +   commit_format = rev.commit_format;
> +   }
> +
> hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, 0);
> for_each_rawref(get_name, NULL);
> if (!names.size && !always)
> --
> 1.9.1
>
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More

Re: [PATCH] Allow --pretty to be passed to git-describe.

[Junio C Hamano]

Cyril Roelandt  writes:

> In some cases, ony may want to find the the most recent tag that is reachable
> from a commit and have it pretty printed, using the formatting options 
> available
> in git-log and git-show.

Sorry, but I do not understand the motivation I can read from these
three lines well enough to say that such a change makes any sense.

It somewhat sounds similar to adding a "--long" option to "git show"
so that "git show --long v1.9.0" will act as if it were running a
"git log v1.9.0".  Yes, you can add any option to tell a command to
do something that is usually considered to be a task for some other
command, but does it even make sense to do so in the first place?
--
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: [RFC/PATCH] Better control of the tests run by a test suite

[Junio C Hamano]

Jeff King  writes:

> On Mon, Mar 24, 2014 at 01:49:44AM -0700, Ilya Bobyr wrote:
>
>> Here are some examples of how functionality added by the patch
>> could be used.  In order to run setup tests and then only a
>> specific test (use case 1) one can do:
>> 
>> $ ./t-init.sh --run='1 2 25'
>> 
>> or:
>> 
>> $ ./t-init.sh --run='<3 25'
>> 
>> ('<=' is also supported, as well as '>' and '>=').
>
> I don't have anything against this in principle, but I suspect it will
> end up being a big pain to figure out which of the early tests are
> required to set up the state, and which are not. Having "<" makes
> specifying it easier, but you still have to read the test script to
> figure out which tests need to be run.

Likewise.

> I wonder if it would make sense to "auto-select" tests that match a
> regex like "set.?up|create"? A while ago, Jonathan made a claim that
> this would cover most tests that are dependencies of other tests. I did
> not believe him, but looking into it, I recall that we did seem to have
> quite a few matching that pattern. If there were a good feature like
> this that gave us a reason to follow that pattern, I think people might
> fix the remainder

This may be worth experimenting with, I would think.
--
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/12] t: drop useless sane_unset GIT_* calls

[Junio C Hamano]

Jeff King  writes:

>> Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
>> applying this patch, and it does look consistently done with
>> GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
>> cursory read it is done consistently for tests on non-bare
>> repositories).
>
> I don't understand why we stop bothering with the unsets starting with
> "init with --template". Are those variables not important to the outcome
> of that and later tests, or did the author simply not bother because
> they are noops?

If I had to guess (without running "blame", so it may well turn out
that "the author" may turn out to be me ;-), it is simply the author
not being careful.

--
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] builtin/apply.c: use iswspace() to detect line-ending-like chars

[Junio C Hamano]

Michael Haggerty  writes:

>> -while ((*last1 == '\r') || (*last1 == '\n'))
>> +while (iswspace(*last1))
>>  last1--;
>> -while ((*last2 == '\r') || (*last2 == '\n'))
>> +while (iswspace(*last2))
>>  last2--;
>>  
>>  /* skip leading whitespace */
>> 
>
> In addition to Eric's comments...
>
> What happens if the string consists *only* of whitespace?

Also, why would casting char to wchar_t without any conversion be
safe and/or sane?

I would sort-of understand if the change were to use isspace(), but
I do not think that is a correct conversion, either.  Isn't a pair
of strings "a bc" and "a bc " supposed not to match?

My understanding is that two strings that differ only at places
where they have runs of whitespaces whose length differ are to
compare the same, e.g. "a_bc__" and "a__bc_" (SP replaced with _ to
make them stand out).  Ignoring whitespace change is very different
from ignoring all whitespaces (the latter of which would make "a b"
and "ab" match).

As a tangent, I have a suspicion that the current implementation may
be wrong at the beginning of the string.  Wouldn't it match " abc"
and "abc", even though these two strings shouldn't match?
--
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] Allow --pretty to be passed to git-describe.

[Cyril Roelandt]

In some cases, ony may want to find the the most recent tag that is reachable
from a commit and have it pretty printed, using the formatting options available
in git-log and git-show.

Signed-off-by: Cyril Roelandt 
---
 Documentation/git-describe.txt |  4 
 builtin/describe.c | 39 ++-
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index d20ca40..fae4713 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -93,6 +93,10 @@ OPTIONS
This is useful when you wish to not match tags on branches merged
in the history of the target commit.
 
+include::pretty-options.txt[]
+
+include::pretty-formats.txt[]
+
 EXAMPLES
 
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 24d740c..4c0ebae 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -8,8 +8,8 @@
 #include "diff.h"
 #include "hashmap.h"
 #include "argv-array.h"
+#include "revision.h"
 
-#define SEEN   (1u << 0)
 #define MAX_TAGS   (FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {
@@ -30,6 +30,8 @@ static int have_util;
 static const char *pattern;
 static int always;
 static const char *dirty;
+static const char *fmt_pretty;
+static enum cmit_fmt commit_format;
 
 /* diff-index command arguments to check if working tree is dirty. */
 static const char *diff_index_args[] = {
@@ -266,8 +268,14 @@ static void describe(const char *arg, int last_one)
 * Exact match to an existing ref.
 */
display_name(n);
-   if (longformat)
+   if (longformat) {
show_suffix(0, n->tag ? n->tag->tagged->sha1 : sha1);
+   } else if (fmt_pretty) {
+   struct strbuf buf = STRBUF_INIT;
+   pp_commit_easy(commit_format, cmit, &buf);
+   printf("%s", buf.buf);
+   strbuf_release(&buf);
+   }
if (dirty)
printf("%s", dirty);
printf("\n");
@@ -386,9 +394,16 @@ static void describe(const char *arg, int last_one)
}
}
 
-   display_name(all_matches[0].name);
-   if (abbrev)
-   show_suffix(all_matches[0].depth, cmit->object.sha1);
+   if (fmt_pretty) {
+   struct strbuf buf = STRBUF_INIT;
+   pp_commit_easy(commit_format, cmit, &buf);
+   printf("%s", buf.buf);
+   strbuf_release(&buf);
+   } else {
+   display_name(all_matches[0].name);
+   if (abbrev)
+   show_suffix(all_matches[0].depth, cmit->object.sha1);
+   }
if (dirty)
printf("%s", dirty);
printf("\n");
@@ -419,6 +434,10 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
N_("append  on dirty working tree (default: 
\"-dirty\")"),
PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
+   OPT_STRING(0, "pretty",  &fmt_pretty, N_("pattern"),
+  N_("pretty print")),
+   OPT_STRING(0, "format",  &fmt_pretty, N_("pattern"),
+  N_("pretty print")),
OPT_END(),
};
 
@@ -437,6 +456,9 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
if (longformat && abbrev == 0)
die(_("--long is incompatible with --abbrev=0"));
 
+   if (longformat && fmt_pretty)
+   die(_("--long is incompatible with --pretty"));
+
if (contains) {
struct argv_array args;
 
@@ -458,6 +480,13 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
return cmd_name_rev(args.argc, args.argv, prefix);
}
 
+   if (fmt_pretty) {
+   struct rev_info rev;
+   init_revisions(&rev, prefix);
+   get_commit_format(fmt_pretty, &rev);
+   commit_format = rev.commit_format;
+   }
+
hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, 0);
for_each_rawref(get_name, NULL);
if (!names.size && !always)
-- 
1.9.1

--
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 v3] Clarify pre-push hook documentation

[David Cowden]

The documentation as-is does not mention that the pre-push hook is
executed even when there is nothing to push.  This can lead a new
reader to believe there will always be lines fed to the script's
standard input and cause minor confusion as to what is happening
when there are no lines provided to the pre-push script.

Signed-off-by: David Cowden 
---

Notes:
I'm not sure if I've covered every case here.  If there are more cases to
consider, please let me know and I can update to include them.

c.f. 
http://stackoverflow.com/questions/22585091/git-hooks-pre-push-script-does-not-receive-input-via-stdin

 Documentation/githooks.txt | 9 +
 1 file changed, 9 insertions(+)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d954bf6..1fd6da9 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -203,6 +203,15 @@ SHA-1>` will be 40 `0`.  If the local commit was specified 
by something other
 than a name which could be expanded (such as `HEAD~`, or a SHA-1) it will be
 supplied as it was originally given.
 
+The hook is executed regardless of whether changes will actually be pushed or
+not.  This may happen when 'git push' is called and:
+
+ - the remote ref is already up to date, or
+ - pushing to the remote ref cannot be handled by a simple fast-forward
+
+In other words, the script is called for every push.  In the event that nothing
+is to be pushed, no data will be provided on the script's standard input.
+
 If this hook exits with a non-zero status, 'git push' will abort without
 pushing anything.  Information about why the push is rejected may be sent
 to the user by writing to standard error.
-- 
1.9.1

--
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] Clarify pre-push hook documentation

[David Cowden]

The documentation as-is does not mention that the pre-push hook is
executed even when there is nothing to push.  This can lead a new
reader to believe there will always be lines fed to the script's
standard input and cause minor confusion as to what is happening
when there are no lines provided to the pre-push script.

Signed-off-by: David Cowden 
---

Notes:
c.f. 
http://stackoverflow.com/questions/22585091/git-hooks-pre-push-script-does-not-receive-input-via-stdin

I'm not sure if I've covered every case here.  If there are more cases to
consider, please let me know and I can update to include them.

 Documentation/githooks.txt | 9 +
 1 file changed, 9 insertions(+)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d954bf6..d05b3c5 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -203,6 +203,15 @@ SHA-1>` will be 40 `0`.  If the local commit was specified 
by something other
 than a name which could be expanded (such as `HEAD~`, or a SHA-1) it will be
 supplied as it was originally given.
 
+The hook is executed regardless of whether changes will actually be pushed or
+not.  This may happen are when 'git push' is called and:
+
+ - the remote ref is already up to date, or
+ - pushing to the remote ref cannot be handled by a simple fast-forward
+
+In other words, the script is called for every push.  In the event that nothing
+is to be pushed, no data will be provided on the script's standard input.
+
 If this hook exits with a non-zero status, 'git push' will abort without
 pushing anything.  Information about why the push is rejected may be sent
 to the user by writing to standard error.
-- 
1.9.1

--
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: [RFC/PATCH] Better control of the tests run by a test suite

[Jeff King]

On Mon, Mar 24, 2014 at 01:49:44AM -0700, Ilya Bobyr wrote:

> Here are some examples of how functionality added by the patch
> could be used.  In order to run setup tests and then only a
> specific test (use case 1) one can do:
> 
> $ ./t-init.sh --run='1 2 25'
> 
> or:
> 
> $ ./t-init.sh --run='<3 25'
> 
> ('<=' is also supported, as well as '>' and '>=').

I don't have anything against this in principle, but I suspect it will
end up being a big pain to figure out which of the early tests are
required to set up the state, and which are not. Having "<" makes
specifying it easier, but you still have to read the test script to
figure out which tests need to be run.

I wonder if it would make sense to "auto-select" tests that match a
regex like "set.?up|create"? A while ago, Jonathan made a claim that
this would cover most tests that are dependencies of other tests. I did
not believe him, but looking into it, I recall that we did seem to have
quite a few matching that pattern. If there were a good feature like
this that gave us a reason to follow that pattern, I think people might
fix the remainder

-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: Git push race condition?

[Nasser Grainawi]

On Mar 24, 2014, at 4:54 PM, Jeff King  wrote:

> On Mon, Mar 24, 2014 at 03:18:14PM -0400, Scott Sandler wrote:
> 
>> I've noticed that a few times in the past several weeks, we've had
>> events where pushes have been lost when two people pushed at just
>> about the same time. The scenario is that two users both have commits
>> based on commit A, call them B and B'. The user with commit B pushes
>> at about the same time as the user who pushes B'. Both pushes are
>> determined to be fast-forwards and both succeed, but B' overwrites B
>> and B is no longer on origin/master. The server does have B in its
>> .git directory but the commit isn't on any branch.
> 
> What version of git are you running on the server? Is it possible that
> there is a simultaneous process running `git pack-refs` (e.g., a `git
> gc` run by a cron job or similar)?

`git gc --auto` could be getting triggered as well, so if you suspect
that you could set gc.auto=0 on the server side.

> 
> There were some race conditions fixed last year wherein git could see
> stale values of refs, but I do not think they could impact writing to a
> ref like this.  When we take the lock on the ref, we always go straight
> to the filesystem, so the value we see is up-to-date.
> 
> -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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora 
Forum, hosted by The Linux Foundation

--
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 push race condition?

[Jeff King]

On Mon, Mar 24, 2014 at 03:18:14PM -0400, Scott Sandler wrote:

> I've noticed that a few times in the past several weeks, we've had
> events where pushes have been lost when two people pushed at just
> about the same time. The scenario is that two users both have commits
> based on commit A, call them B and B'. The user with commit B pushes
> at about the same time as the user who pushes B'. Both pushes are
> determined to be fast-forwards and both succeed, but B' overwrites B
> and B is no longer on origin/master. The server does have B in its
> .git directory but the commit isn't on any branch.

What version of git are you running on the server? Is it possible that
there is a simultaneous process running `git pack-refs` (e.g., a `git
gc` run by a cron job or similar)?

There were some race conditions fixed last year wherein git could see
stale values of refs, but I do not think they could impact writing to a
ref like this.  When we take the lock on the ref, we always go straight
to the filesystem, so the value we see is up-to-date.

-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: Git push race condition?

[Jeff King]

On Mon, Mar 24, 2014 at 10:16:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > error: Ref refs/heads/master is at
> > 4584c1f34e07cea2df6abc8e0d407fe016017130 but expected
> > 61b79b6d35b066d054fb3deab550f1c51598cf5f
> > remote: error: failed to lock refs/heads/master
> 
> I also see this error once in a while. I read the code a while back
> and it's basically because there's two levels of locks that
> receive-pack tries to get, and it's possible for two pushers to get
> the first lock due to a race condition.
> 
> I've never seen data loss due to this though, because the inner lock is 
> atomic.

The reason is that there are not 2 locks. Each side remembers the "old"
value when it started the operation, and only takes a lock when it comes
time to write the ref (and then checks that the old value is still
current). Two pushes happening simultaneously do not have any idea that
the other is occurring.

-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: Git push race condition?

[Junio C Hamano]

Matthieu Moy  writes:

> What you describe really looks like a force-push, or a hook doing a ref
> update (e.g. a hook on a dev branch that updates master if the code
> passes tests or so).

... or a filesystem that is broken.  But I thought this is just a
plain-vanilla ext4, nothing exotic, so  Puzzled.

--
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 5/5] log: do not segfault on gmtime errors

[Jeff King]

On Mon, Mar 24, 2014 at 11:03:42PM +0100, René Scharfe wrote:

> >If the result is all-zeroes, can we check for that case instead? I
> >suppose that will eventually create a "trap" at midnight on January 1st
> >of the year 0 (though I am not sure such a date is even meaningful,
> >given the history of our calendars).
> 
> Makes sense. That would be "Sun Jan 0 00:00:00 1900", however -- days are
> 1-based and years 1900-based in struct tm.

Oh right, I was stupidly forgetting about being 1900-based.

> Since a zeroth day is invalid, would this suffice:
>
>   if (!tm || !tm->tm_mday) {
> 
> (Yes, I'm lazy. :)

That looks perfect (and I like that it is quick to check, too, since
this case should be extremely rare).

-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 03/10] t4209: factor out helper function test_log_icase()

[Junio C Hamano]

René Scharfe  writes:

> Am 24.03.2014 22:10, schrieb Jeff King:
>> On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:
>>
 +test_log_icase() {
 +  test_log $@ --regexp-ignore-case
 +  test_log $@ -i
>>>
>>> &&-cascade broken?  Will squash in an obvious fix.
>>
>> I don't think so. This is happening outside of test_expect_success,
>> which is run by test_log. So adding a && means that if the first test
>> fails, we do not bother to run the second one at all, which is not what
>> we want.
>
> Right; this function runs two independent tests and && is left out
> intentionally.

Ahh, sorry and 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: Git push race condition?

[Matthieu Moy]

Scott Sandler  writes:

> It's a bare repo and I didn't realize server-side reflogs were a
> thing. Just ran "git config core.logallrefupdates true" in the repo on
> the server which seems to be what I should do to enable that.

That should be it, yes.

> The server does know about B, it shows up when you do "git show B".
> However "git branch --contains B" returns nothing.

That is "normal": git push sends objects to the object store in a
lockless manner, and then updates the reference corresponding to the
branch you're pushing to. So in case of concurrent access, the objects
may be sent, but the reference update will fail. Objects would be
garbage collected by a further "git gc [--prune]".

The "not normal" part is that the race condition on the ref update does
actually break for you.

> Gitlab's update hook maintains an event log when any push event
> happens, who pushed and which commits. The most recent time this
> happened, the first push which was lost occured at 2014-03-24 19:04:51
> and the one that overwrote it happened at 2014-03-24 19:05:04. That's
> when the update hook ran, not necessarily when the user hit "git
> push", but it is notable that it's 13 seconds apart which is a pretty
> long time. We do run several hooks for checking coding syntax and
> various other things so it's believable to me that the hooks would
> take more than 13 seconds on occasion, but based on the testing I did
> with the sleep hook it didn't seem like the hooks were actually the
> problem.

Are you really, really, really sure that there's no force-push involved?
(either "push --force" or "push remotename +branchname")

What you describe really looks like a force-push, or a hook doing a ref
update (e.g. a hook on a dev branch that updates master if the code
passes tests or so).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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/12] t: drop useless sane_unset GIT_* calls

[Junio C Hamano]

Jeff King  writes:

> I do not have a problem with that, as it implicitly covers all of the
> tests following it. I do not think it is particularly necessary, though.
> Assuming we start with a known test environment and avoiding polluting
> it for further tests are basic principles of _all_ test scripts.

They should be, but I suspect majority of tests, especially the
older ones, do have dependencies on earlier test pieces X-<.

--
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 5/5] log: do not segfault on gmtime errors

[René Scharfe]

Am 24.03.2014 22:33, schrieb Jeff King:

On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:

@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum 
date_mode mode)
tz = local_tzoffset(time);

tm = time_to_tm(time, tz);
-   if (!tm)
-   return NULL;
+   if (!tm) {


Would it make sense to work around the FreeBSD issue by adding a check like
this?

if (!tm || tm->tm_year < 70) {


That feels like a bit of a maintenance headache.  Right now we do not
internally represent times prior to the epoch, since we mostly use
"unsigned long" through the code. So anything < 70 has to be an error.
But it would be nice one day to consistently use a 64-bit signed time_t
throughout git, and represent even ancient timestamps (e.g., for people
doing historical projects, like importing laws into git). This would set
quite a trap for people working later on the code.

If the result is all-zeroes, can we check for that case instead? I
suppose that will eventually create a "trap" at midnight on January 1st
of the year 0 (though I am not sure such a date is even meaningful,
given the history of our calendars).


Makes sense. That would be "Sun Jan 0 00:00:00 1900", however -- days 
are 1-based and years 1900-based in struct tm.  Since a zeroth day is 
invalid, would this suffice:


if (!tm || !tm->tm_mday) {

(Yes, I'm lazy. :)

René
--
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

[RFC][PATCH] send-email: add --[no-]xmailer option

[Luis Henriques]

Add --[no-]xmailer that allows a user to disable adding the 'X-Mailer:'
header to the email being sent.

Signed-off-by: Luis Henriques 
---
 Documentation/config.txt |  1 +
 Documentation/git-send-email.txt |  3 +++
 git-send-email.perl  | 12 ++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73c8973..c33d5a1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -,6 +,7 @@ sendemail.smtpserveroption::
 sendemail.smtpuser::
 sendemail.thread::
 sendemail.validate::
+sendemail.xmailer::
See linkgit:git-send-email[1] for description.
 
 sendemail.signedoffcc::
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f0e57a5..fab6264 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -131,6 +131,9 @@ Note that no attempts whatsoever are made to validate the 
encoding.
Specify encoding of compose message. Default is the value of the
'sendemail.composeencoding'; if that is unspecified, UTF-8 is assumed.
 
+--xmailer::
+   Prevent adding the "X-Mailer:" header.  Default value is
+   'sendemail.xmailer'.
 
 Sending
 ~~~
diff --git a/git-send-email.perl b/git-send-email.perl
index fdb0029..8789124 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -54,6 +54,7 @@ git send-email [options] 
 --[no-]bcc* Email Bcc:
 --subject * Email "Subject:"
 --in-reply-to * Email "In-Reply-To:"
+--[no-]xmailer * Don't add "X-Mailer:" header.  Default on.
 --[no-]annotate* Review each patch that will be sent in an 
editor.
 --compose  * Open an editor for introduction.
 --compose-encoding* Encoding to assume for introduction.
@@ -174,6 +175,9 @@ my $force = 0;
 my $multiedit;
 my $editor;
 
+# Usage of X-Mailer email header
+my $xmailer;
+
 sub do_edit {
if (!defined($editor)) {
$editor = Git::command_oneline('var', 'GIT_EDITOR');
@@ -214,7 +218,8 @@ my %config_bool_settings = (
 "signedoffcc" => [\$signed_off_by_cc, undef],  # Deprecated
 "validate" => [\$validate, 1],
 "multiedit" => [\$multiedit, undef],
-"annotate" => [\$annotate, undef]
+"annotate" => [\$annotate, undef],
+"xmailer" => [\$xmailer, 1]
 );
 
 my %config_settings = (
@@ -311,6 +316,7 @@ my $rc = GetOptions("h" => \$help,
"8bit-encoding=s" => \$auto_8bit_encoding,
"compose-encoding=s" => \$compose_encoding,
"force" => \$force,
+   "xmailer!" => \$xmailer,
 );
 
 usage() if $help;
@@ -1144,8 +1150,10 @@ To: $to${ccline}
 Subject: $subject
 Date: $date
 Message-Id: $message_id
-X-Mailer: git-send-email $gitversion
 ";
+   if ($xmailer) {
+   $header .= "X-Mailer: git-send-email $gitversion\n";
+   }
if ($reply_to) {
 
$header .= "In-Reply-To: $reply_to\n";
-- 
1.9.1
--
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 04/12] t: stop using GIT_CONFIG to cross repo boundaries

[Jeff King]

On Fri, Mar 21, 2014 at 02:26:02PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Some tests want to check or set config in another
> > repository. E.g., t1000 creates repositories and makes sure
> > that their core.bare and core.worktree settings are what we
> > expect. We can do this with:
> >
> >   GIT_CONFIG=$repo/.git/config git config ...
> >
> > but it better shows the intent to just enter the repository
> > and let "git config" do the normal lookups:
> >
> >   (cd $repo && git config ...)
> >
> > In theory, this would cause us to use an extra subshell, but
> > in all such cases, we are actually already in a subshell.
> 
> Sure; alternatively we could use "git -C $there", but this rewrite
> is fine by me.

The existing callers all pass actual $GIT_DIRs, so I initially wrote it
as "git --git-dir=$repo config ...". Doing it as "-C" is perhaps nicer,
as callers could potentially pass a shorter string to the repo root,
and not bother with adding "/.git". However, t0001 needs the actual
$GIT_DIR (because it looks for things like the refs/ directory in the
same function), and the other callers are just passing bare repos.

So I'm fine with any of them. Feel free to mark it up if you have a
preference.

-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 03/12] t: drop useless sane_unset GIT_* calls

[Jeff King]

On Fri, Mar 21, 2014 at 02:24:31PM -0700, Junio C Hamano wrote:

> > Unsetting these is not only useless, but can be confusing to
> > a reader, who may wonder why some tests in a script unset
> > them and others do not (t0001 is particularly guilty of this
> > inconsistency, probably because many of its tests predate
> > the test-lib.sh environment-cleansing).
> [...]
> > I suppose one could make an argument that test-lib.sh may later change
> > the set of variables it clears, and these unsets are documenting an
> > explicit need of each test. I'd find that more compelling if it were
> > actually applied consistently.
> 
> Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
> applying this patch, and it does look consistently done with
> GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
> cursory read it is done consistently for tests on non-bare
> repositories).

I don't understand why we stop bothering with the unsets starting with
"init with --template". Are those variables not important to the outcome
of that and later tests, or did the author simply not bother because
they are noops?

> So I would actually agree with your alternative interpretation
> "Unsetting these is useless, but it does serve documentation
> purpose---without having to see what the state of the environment
> when the subprocess is started, the reader can understand what is
> being tested", rather than the one in the log message.

I'd agree with that if I were convinced that the presence of them there
versus the absence of them later was meaningful.

> Having said that, I am perfectly OK with the change to t0001 in this
> patch, if we added at the very beginning of the test sequence a
> comment that says:
> 
> Below, creation and use of repositories are tested with various
> combinations of environment settings and command line flags.
> They are done inside subshells to avoid leaking temporary
> environment settings to later tests *and* assumes that the
> initial environment does not have have GIT_DIR, GIT_CONFIG, and
> GIT_WORK_TREE defined.
> 
> or something.

I do not have a problem with that, as it implicitly covers all of the
tests following it. I do not think it is particularly necessary, though.
Assuming we start with a known test environment and avoiding polluting
it for further tests are basic principles of _all_ test scripts.

-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 00/10] userdiff: cpp pattern simplification and test framework

[Jeff King]

On Fri, Mar 21, 2014 at 10:07:12PM +0100, Johannes Sixt wrote:

> Here is a series that makes the hunk header pattern for C and C++ even
> simpler than suggested by Peff in [1] to catch a lot more C++ functions
> and two more C patterns.
> 
> As a preparatory work, the test cases are totally rewritten to make it
> a lot simpler to drop in new tests. There was an earlier attempt to
> change the infrastructure [2], and it is the reason for the widened Cc
> list.

Thanks. This looks sane overall, and I am especially happy about the
increased test coverage. I do not know if your 10/10 misses some cases,
but at least now we can collect and reason about a set of examples going
forward.

-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 17/19] Portable alloca for Git

[Junio C Hamano]

Kirill Smelkov  writes:

> On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote:
>> On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov  wrote:
>> ...
>> > In fact that would be maybe preferred, for maintainers to enable alloca
>> > with knowledge and testing, as one person can't have them all at hand.
>> 
>> Yeah, you're probably right.
>
> Erik, the patch has been merged into pu today. Would you please
> follow-up with tested MINGW change?

Sooo I lost track but this discussion seems to have petered out
around here.  I think the copy we have had for a while on 'pu' is
basically sound, and can easily built on by platform folks by adding
or removing the -DHAVE_ALLOCA_H from the Makefile.

--
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/10] t4209: factor out helper function test_log_icase()

[René Scharfe]

Am 24.03.2014 22:10, schrieb Jeff King:

On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:


+test_log_icase() {
+   test_log $@ --regexp-ignore-case
+   test_log $@ -i


&&-cascade broken?  Will squash in an obvious fix.


I don't think so. This is happening outside of test_expect_success,
which is run by test_log. So adding a && means that if the first test
fails, we do not bother to run the second one at all, which is not what
we want.


Right; this function runs two independent tests and && is left out 
intentionally.


René
--
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 04/10] t4209: use helper functions to test --grep

[René Scharfe]

Am 24.03.2014 22:14, schrieb Jeff King:
> On Mon, Mar 24, 2014 at 11:22:58AM -0700, Junio C Hamano wrote:
> 
>> René Scharfe  writes:
>>
>>> -test_expect_success 'log --grep -i' '
>>> -   git log -i --grep=InItial --format=%H >actual &&
>>> -   test_cmp expect_initial actual
>>> -'
>>> +test_log   expect_initial  --grep initial
>>> +test_log   expect_nomatch  --grep InItial
>>
>> This, and the next --author one, assumes that we will never break
>> "--grep=foo" without breaking "--grep foo".  That should be OK, but
>> we might want to add separate tests e.g.
>>
>>  test_log expect_initial --grep=initial
>>
>> perhaps?  I dunno.
> 
> Yeah, I I'd prefer "--grep=" here (and in all scripts).  In general, I
> think our attitude should be that "--foo=bar" is guaranteed to work
> forever, but "--foo bar" is not. The latter only works if the argument
> is non-optional, so that leaves us room to "break" compatibility to make
> an argument optional in a future version.
> 
> Now, whether the rest of the world and its script-writers are aware of
> this fact, I don't know. So it may be too late already (but it does look
> like we mention it in gitcli(7)).

OK, then the following should be squashed into patch 2 (t4209: factor out
helper function test_log()):

diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index 9f3bb40..f47231a 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -10,6 +10,14 @@ test_log() {
shift 3
rest=$@
 
+   case $kind in
+   --*)
+   opt=$kind=$needle
+   ;;
+   *)
+   opt=$kind$needle
+   ;;
+   esac
case $expect in
expect_nomatch)
match=nomatch
@@ -20,7 +28,7 @@ test_log() {
esac
 
test_expect_success "log $kind${rest:+ $rest} ($match)" "
-   git log $rest $kind $needle --format=%H >actual &&
+   git log $rest $opt --format=%H >actual &&
test_cmp $expect actual
"
 }
--
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 16/19] tree-diff: reuse base str(buf) memory on sub-tree recursion

[Junio C Hamano]

Kirill Smelkov  writes:

> instead of allocating it all the time for every subtree in
> __diff_tree_sha1, let's allocate it once in diff_tree_sha1, and then all
> callee just use it in stacking style, without memory allocations.
>
> This should be faster, and for me this change gives the following
> slight speedups for
>
> git log --raw --no-abbrev --no-renames --format='%H'
>
> navy.gitlinux.git v3.10..v3.11
>
> before  0.618s  1.903s
> after   0.611s  1.889s
> speedup 1.1%0.7%
>
> Signed-off-by: Kirill Smelkov 
> ---
>
> Changes since v1:
>
>  - don't need to touch diff.h, as the function we are changing became static.
>
>  tree-diff.c | 36 ++--
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/tree-diff.c b/tree-diff.c
> index aea0297..c76821d 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -115,7 +115,7 @@ static void show_path(struct strbuf *base, struct 
> diff_options *opt,
>   if (recurse) {
>   strbuf_addch(base, '/');
>   __diff_tree_sha1(t1 ? t1->entry.sha1 : NULL,
> -  t2 ? t2->entry.sha1 : NULL, base->buf, opt);
> +  t2 ? t2->entry.sha1 : NULL, base, opt);
>   }
>  
>   strbuf_setlen(base, old_baselen);

I was scratching my head for a while, after seeing that there does
not seem to be any *new* code added by this patch in order to
store-away the original length and restore the singleton base buffer
to the original length after using addch/addstr to extend it.

But I see that the code has already been prepared to do this
conversion.  I wonder why we didn't do this earlier ;-)

Looks good.  Thanks.

> @@ -138,12 +138,10 @@ static void skip_uninteresting(struct tree_desc *t, 
> struct strbuf *base,
>  }
>  
>  static int __diff_tree_sha1(const unsigned char *old, const unsigned char 
> *new,
> - const char *base_str, struct diff_options *opt)
> + struct strbuf *base, struct diff_options *opt)
>  {
>   struct tree_desc t1, t2;
>   void *t1tree, *t2tree;
> - struct strbuf base;
> - int baselen = strlen(base_str);
>  
>   t1tree = fill_tree_descriptor(&t1, old);
>   t2tree = fill_tree_descriptor(&t2, new);
> @@ -151,17 +149,14 @@ static int __diff_tree_sha1(const unsigned char *old, 
> const unsigned char *new,
>   /* Enable recursion indefinitely */
>   opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
>  
> - strbuf_init(&base, PATH_MAX);
> - strbuf_add(&base, base_str, baselen);
> -
>   for (;;) {
>   int cmp;
>  
>   if (diff_can_quit_early(opt))
>   break;
>   if (opt->pathspec.nr) {
> - skip_uninteresting(&t1, &base, opt);
> - skip_uninteresting(&t2, &base, opt);
> + skip_uninteresting(&t1, base, opt);
> + skip_uninteresting(&t2, base, opt);
>   }
>   if (!t1.size && !t2.size)
>   break;
> @@ -173,7 +168,7 @@ static int __diff_tree_sha1(const unsigned char *old, 
> const unsigned char *new,
>   if (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||
>   hashcmp(t1.entry.sha1, t2.entry.sha1) ||
>   (t1.entry.mode != t2.entry.mode))
> - show_path(&base, opt, &t1, &t2);
> + show_path(base, opt, &t1, &t2);
>  
>   update_tree_entry(&t1);
>   update_tree_entry(&t2);
> @@ -181,18 +176,17 @@ static int __diff_tree_sha1(const unsigned char *old, 
> const unsigned char *new,
>  
>   /* t1 < t2 */
>   else if (cmp < 0) {
> - show_path(&base, opt, &t1, /*t2=*/NULL);
> + show_path(base, opt, &t1, /*t2=*/NULL);
>   update_tree_entry(&t1);
>   }
>  
>   /* t1 > t2 */
>   else {
> - show_path(&base, opt, /*t1=*/NULL, &t2);
> + show_path(base, opt, /*t1=*/NULL, &t2);
>   update_tree_entry(&t2);
>   }
>   }
>  
> - strbuf_release(&base);
>   free(t2tree);
>   free(t1tree);
>   return 0;
> @@ -209,7 +203,7 @@ static inline int diff_might_be_rename(void)
>   !DIFF_FILE_VALID(diff_queued_diff.queue[0]->one);
>  }
>  
> -static void try_to_follow_renames(const unsigned char *old, const unsigned 
> char *new, const char *base, struct diff_options *opt)
> +static void try_to_follow_renames(const unsigned char *old, const unsigned 
> char *new, struct strbuf *base, struct diff_options *opt)
>  {
>   struct diff_options diff_opts;
>   struct diff_queue_struct *q = &diff_queued_diff;
> @@ -306,13 +300,19 @@ static void try_to_follow_renames(const unsigned char 

Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers

[Jeff King]

On Mon, Mar 24, 2014 at 05:36:59PM -0400, Jeff King wrote:

> > +How to write RIGHT test cases
> > +=
> > +
> > +Insert the word "ChangeMe" (exactly this form) at a distance of
> > +at least two lines from the line that must appear in the hunk header.
> 
> The existing tests use -U1 to make writing cases simpler. Is there a
> reason not to continue that (or if you found that porting the existing
> cases was not a chore with -U3, I can buy that argument, too)?

I take it back. You did keep "-U1" in the result. Is this "two lines"
rule necessary, then?

-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 v2 14/19] tree-diff: rework diff_tree interface to be sha1 based

[Junio C Hamano]

Kirill Smelkov  writes:

> The downside is that try_to_follow_renames(), if active, we cause
> re-reading of 2 initial trees, which was negligible based on my timings,

That would depend on how often the codepath triggered in your test
case, but is totally understandable.  It fires only when the path we
have been following disappears from the parent, and the processing
of try-to-follow itself is very compute-intensive (it needs to run
find-copies-harder logic) that will end up reading many subtrees of
the two initial trees; two more reading of tree objects will be
dwarfed by the actual processing.

> and which is outweighed cogently by the upsides.

> Changes since v1:
>
>  - don't need to touch diff.h, as diff_tree() became static.

Nice.  I wonder if it is an option to let the function keep its name
diff_tree() without renaming it to __diff_tree_whatever(), though.

>  tree-diff.c | 60 
>  1 file changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/tree-diff.c b/tree-diff.c
> index b99622c..f90acf5 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -137,12 +137,17 @@ static void skip_uninteresting(struct tree_desc *t, 
> struct strbuf *base,
>   }
>  }
>  
> -static int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
> -  const char *base_str, struct diff_options *opt)
> +static int __diff_tree_sha1(const unsigned char *old, const unsigned char 
> *new,
> + const char *base_str, struct diff_options *opt)
>  {
> + struct tree_desc t1, t2;
> + void *t1tree, *t2tree;
>   struct strbuf base;
>   int baselen = strlen(base_str);
>  
> + t1tree = fill_tree_descriptor(&t1, old);
> + t2tree = fill_tree_descriptor(&t2, new);
> +
>   /* Enable recursion indefinitely */
>   opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
>  
> @@ -155,39 +160,41 @@ static int diff_tree(struct tree_desc *t1, struct 
> tree_desc *t2,
>   if (diff_can_quit_early(opt))
>   break;
>   if (opt->pathspec.nr) {
> - skip_uninteresting(t1, &base, opt);
> - skip_uninteresting(t2, &base, opt);
> + skip_uninteresting(&t1, &base, opt);
> + skip_uninteresting(&t2, &base, opt);
>   }
> - if (!t1->size && !t2->size)
> + if (!t1.size && !t2.size)
>   break;
>  
> - cmp = tree_entry_pathcmp(t1, t2);
> + cmp = tree_entry_pathcmp(&t1, &t2);
>  
>   /* t1 = t2 */
>   if (cmp == 0) {
>   if (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||
> - hashcmp(t1->entry.sha1, t2->entry.sha1) ||
> - (t1->entry.mode != t2->entry.mode))
> - show_path(&base, opt, t1, t2);
> + hashcmp(t1.entry.sha1, t2.entry.sha1) ||
> + (t1.entry.mode != t2.entry.mode))
> + show_path(&base, opt, &t1, &t2);
>  
> - update_tree_entry(t1);
> - update_tree_entry(t2);
> + update_tree_entry(&t1);
> + update_tree_entry(&t2);
>   }
>  
>   /* t1 < t2 */
>   else if (cmp < 0) {
> - show_path(&base, opt, t1, /*t2=*/NULL);
> - update_tree_entry(t1);
> + show_path(&base, opt, &t1, /*t2=*/NULL);
> + update_tree_entry(&t1);
>   }
>  
>   /* t1 > t2 */
>   else {
> - show_path(&base, opt, /*t1=*/NULL, t2);
> - update_tree_entry(t2);
> + show_path(&base, opt, /*t1=*/NULL, &t2);
> + update_tree_entry(&t2);
>   }
>   }
>  
>   strbuf_release(&base);
> + free(t2tree);
> + free(t1tree);
>   return 0;
>  }
>  
> @@ -202,7 +209,7 @@ static inline int diff_might_be_rename(void)
>   !DIFF_FILE_VALID(diff_queued_diff.queue[0]->one);
>  }
>  
> -static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc 
> *t2, const char *base, struct diff_options *opt)
> +static void try_to_follow_renames(const unsigned char *old, const unsigned 
> char *new, const char *base, struct diff_options *opt)
>  {
>   struct diff_options diff_opts;
>   struct diff_queue_struct *q = &diff_queued_diff;
> @@ -240,7 +247,7 @@ static void try_to_follow_renames(struct tree_desc *t1, 
> struct tree_desc *t2, co
>   diff_opts.break_opt = opt->break_opt;
>   diff_opts.rename_score = opt->rename_score;
>   diff_setup_done(&diff_opts);
> - diff_tree(t1, t2, base, &diff_opts);
> + __diff_tree_sha1(old, new, base, &diff_opts);
>   diffcore_std(&diff_opts);
>   free_pathspec(&diff_opts.pathspec);
>  
> @@ -3

Re: [PATCH 03/10] t4018: an infrastructure to test hunk headers

[Jeff King]

On Fri, Mar 21, 2014 at 10:07:15PM +0100, Johannes Sixt wrote:

> Add an infrastructure that simplifies adding new tests of the hunk
> header regular expressions.
> 
> To add new tests, a file with the syntax to test can be dropped in the
> directory t4018. The README file explains how a test file must contain;
> the README itself tests the default behavior.

I really like the cleanups you've done in t4018. I noticed how messy it
was when I modified it recently, but I didn't take the time to clean it.

> diff --git a/t/t4018/README b/t/t4018/README
> new file mode 100644
> index 000..283e01cc
> --- /dev/null
> +++ b/t/t4018/README
> @@ -0,0 +1,18 @@
> +How to write RIGHT test cases
> +=
> +
> +Insert the word "ChangeMe" (exactly this form) at a distance of
> +at least two lines from the line that must appear in the hunk header.

The existing tests use -U1 to make writing cases simpler. Is there a
reason not to continue that (or if you found that porting the existing
cases was not a chore with -U3, I can buy that argument, too)?

> +The text that must appear in the hunk header must contain the word
> +"right", but in all upper-case, like in the title above.
> +
> +To mark a test case that highlights a malfunction, insert the word
> +BROKEN in all lower-case somewhere in the file.

I wondered why you wouldn't write them in the case you are indicating,
when...

> +This text is a bit twisted and out of order, but it is itself a
> +test case for the default hunk header pattern. Know what you are doing
> +if you change it.

Ah. Clever. :)

-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 5/5] log: do not segfault on gmtime errors

[Jeff King]

On Sat, Mar 22, 2014 at 10:32:37AM +0100, René Scharfe wrote:

> >This test is of questionable portability, since we are depending on
> >gmtime's arbitrary point to decide that our input is crazy and return
> >NULL. The value is sufficiently large that I'd expect most to do so,
> >though, so it may be safe.
> 
> Just came around to testing on FreeBSD 10 amd64; the new test in t4212 fails
> there:

Thanks for the report. I don't think the problem is in the test here,
but rather that we should do a more thorough job of detecting gmtime's
"I don't know what to do with this" response.

> >@@ -184,8 +184,10 @@ const char *show_date(unsigned long time, int tz, enum 
> >date_mode mode)
> > tz = local_tzoffset(time);
> >
> > tm = time_to_tm(time, tz);
> >-if (!tm)
> >-return NULL;
> >+if (!tm) {
> 
> Would it make sense to work around the FreeBSD issue by adding a check like
> this?
> 
>   if (!tm || tm->tm_year < 70) {

That feels like a bit of a maintenance headache.  Right now we do not
internally represent times prior to the epoch, since we mostly use
"unsigned long" through the code. So anything < 70 has to be an error.
But it would be nice one day to consistently use a 64-bit signed time_t
throughout git, and represent even ancient timestamps (e.g., for people
doing historical projects, like importing laws into git). This would set
quite a trap for people working later on the code.

If the result is all-zeroes, can we check for that case instead? I
suppose that will eventually create a "trap" at midnight on January 1st
of the year 0 (though I am not sure such a date is even meaningful,
given the history of our calendars).

-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: Git push race condition?

[Scott Sandler]

Right. Receiving that error is what happens during my testing with a
hook that sleeps for 60s, and that outcome makes sense. But whatever
is occurring in production must be different, since both users see
successful pushes with the first one just being overwritten.

On Mon, Mar 24, 2014 at 5:16 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Mon, Mar 24, 2014 at 8:18 PM, Scott Sandler
>  wrote:
>> I run a private Git repository (using Gitlab) with about 200 users
>> doing about 100 pushes per day.
>
> Ditto but about 2x those numbers.
>
>> error: Ref refs/heads/master is at
>> 4584c1f34e07cea2df6abc8e0d407fe016017130 but expected
>> 61b79b6d35b066d054fb3deab550f1c51598cf5f
>> remote: error: failed to lock refs/heads/master
>
> I also see this error once in a while. I read the code a while back
> and it's basically because there's two levels of locks that
> receive-pack tries to get, and it's possible for two pushers to get
> the first lock due to a race condition.
>
> I've never seen data loss due to this though, because the inner lock is 
> atomic.
--
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 11/19] tree-diff: simplify tree_entry_pathcmp

[Junio C Hamano]

Kirill Smelkov  writes:

> Since an earlier "Finally switch over tree descriptors to contain a
> pre-parsed entry", we can safely access all tree_desc->entry fields
> directly instead of first "extracting" them through
> tree_entry_extract.
>
> Use it. The code generated stays the same - only it now visually looks
> cleaner.
>
> Signed-off-by: Kirill Smelkov 
> Signed-off-by: Junio C Hamano 
> ---
>
> ( re-posting without change )

Thanks.

Hopefully I'll be merging the series up to this point to 'next'
soonish.

>
>  tree-diff.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/tree-diff.c b/tree-diff.c
> index 20a4fda..cf96ad7 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -15,18 +15,13 @@
>   */
>  static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
>  {
> - unsigned mode1, mode2;
> - const char *path1, *path2;
> - const unsigned char *sha1, *sha2;
> - int cmp, pathlen1, pathlen2;
> + struct name_entry *e1, *e2;
> + int cmp;
>  
> - sha1 = tree_entry_extract(t1, &path1, &mode1);
> - sha2 = tree_entry_extract(t2, &path2, &mode2);
> -
> - pathlen1 = tree_entry_len(&t1->entry);
> - pathlen2 = tree_entry_len(&t2->entry);
> -
> - cmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);
> + e1 = &t1->entry;
> + e2 = &t2->entry;
> + cmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode,
> + e2->path, tree_entry_len(e2), e2->mode);
>   return cmp;
>  }
--
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 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[Jeff King]

On Sat, Mar 22, 2014 at 06:15:50PM +0100, René Scharfe wrote:

> This series allows the options -i/--regexp-ignore-case, --pickaxe-regex,
> and -S to be used together and work as expected to perform a pickaxe
> search using case-insensitive regular expression matching.  Its first
> half refactors the test script and extends test coverage a bit while
> we're at it.  The actual change is in the sixth patch.  It enables the
> two following cleanups.  The last two patches are independent simple
> cleanups.

This all looks very nice.

Upon reading your cover letter, I wondered if somebody would ever want
case-sensitive "--grep", but case-insensitive pickaxe (or vice versa).
However, you are not adding case-insensitivity to -G/-S, but rather
applying it more consistently. So I think that ship has already sailed
anyway (and even if it did not, I think applying "-i" everywhere makes
the interface simpler for the common cases, so the loss of flexibility
may be worth it).

-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 08/10] pickaxe: move pickaxe() after pickaxe_match()

[Jeff King]

On Sat, Mar 22, 2014 at 06:15:58PM +0100, René Scharfe wrote:

> pickaxe() calls pickaxe_match(); moving the definition of the former
> those after the latter allows us to do without an explicit function
> declaration.

s/those //

-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: Borrowing objects from nearby repositories

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 12, 2014 at 4:37 AM, Andrew Keller  wrote:
> Hi all,
>
> I am considering developing a new feature, and I'd like to poll the group for 
> opinions.
>
> Background: A couple years ago, I wrote a set of scripts that speed up 
> cloning of frequently used repositories.  The scripts utilize a bare Git 
> repository located at a known location, and automate providing a --reference 
> parameter to `git clone` and `git submodule update`.  Recently, some 
> coworkers of mine expressed an interest in using the scripts, so I published 
> the current version of my scripts, called `git repocache`, described at the 
> bottom of .
>
> Slowly, it has occurred to me that this feature, or something similar to it, 
> may be worth adding to Git, so I've been thinking about the best approach.  
> Here's my best idea so far:
>
> 1)  Introduce '--borrow' to `git-fetch`.  This would behave similarly to 
> '--reference', except that it operates on a temporary basis, and does not 
> assume that the reference repository will exist after the operation 
> completes, so any used objects are copied into the local objects database.  
> In theory, this mechanism would be distinct from '--reference', so if both 
> are used, some objects would be copied, and some objects would be accessible 
> via a reference repository referenced by the alternates file.

Isn't this the same as git clone --reference  --no-hardlinks  ?

Also without --no-hardlinks we're not assuming that the other repo
doesn't go away (you could rm-rf it), just that the files won't be
*modified*, which Git won't do, but you could manually do with other
tools, so the default is to hardlink.

> 2)  Teach `git fetch` to read 'repocache.path' (or a better-named 
> configuration), and use it to automatically activate borrowing.

So a default path for --reference  --no-hardlinks ?

> 3)  For consistency, `git clone`, `git pull`, and `git submodule update` 
> should probably all learn '--borrow', and forward it to `git fetch`.
>
> 4)  In some scenarios, it may be necessary to temporarily not automatically 
> borrow, so `git fetch`, and everything that calls it may need an argument to 
> do that.
>
> Intended outcome: With 'repocache.path' set, and the cached repository 
> properly updated, one could run `git clone `, and the operation would 
> complete much faster than it does now due to less load on the network.
>
> Things I haven't figured out yet:
>
> *  What's the best approach to copying the needed objects?  It's probably 
> inefficient to copy individual objects out of pack files one at a time, but 
> it could be wasteful to copy entire pack files just because you need one 
> object.  Hard-linking could help, but that won't always be available.  One of 
> my previous ideas was to add a '--auto-repack' option to `git-clone`, which 
> solves this problem better, but introduces some other front-end usability 
> problems.
> *  To maintain optimal effectiveness, users would have to regularly run a 
> fetch in the cache repository.  Not all users know how to set up a scheduled 
> task on their computer, so this might become a maintenance problem for the 
> user.  This kind of problem I think brings into question the viability of the 
> underlying design here, assuming that the ultimate goal is to clone faster, 
> with very little or no change in the use of git.
>
>
> Thoughts?
>
> Thanks,
> Andrew Keller
>
> --
> 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
--
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 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases

[Junio C Hamano]

Kirill Smelkov  writes:

> via teaching tree_entry_pathcmp() how to compare empty tree descriptors:

Drop this line, as you explain the "pretend empty compares bigger
than anything else" idea later anyway?  This early part of the
proposed log message made me hiccup while reading it.

> While walking trees, we iterate their entries from lowest to highest in
> sort order, so empty tree means all entries were already went over.
>
> If we artificially assign +infinity value to such tree "entry", it will
> go after all usual entries, and through the usual driver loop we will be
> taking the same actions, which were hand-coded for special cases, i.e.
>
> t1 empty, t2 non-empty
> pathcmp(+∞, t2) -> +1
> show_path(/*t1=*/NULL, t2); /* = t1 > t2 case in main loop */
>
> t1 non-empty, t2-empty
> pathcmp(t1, +∞) -> -1
> show_path(t1, /*t2=*/NULL); /* = t1 < t2 case in main loop */

Sounds good.  I would have phrased a bit differently, though:

When we have T1 and T2, we return a sign that tells the caller
to indicate the "earlier" one to be emitted, and by returning
the sign that causes the non-empty side to be emitted, we will
automatically cause the entries from the remaining side to be
emitted, without attempting to touch the empty side at all.  We
can teach tree_entry_pathcmp() to pretend that an empty tree has
an element that sorts after anything else to achieve this.

without saying "infinity".

> Right now we never go to when compared tree descriptors are infinity,...

Sorry, but I cannot parse this.

> as
> this condition is checked in the loop beginning as finishing criteria,

What condition and which loop?  The loop that immediately surrounds
the callsite of tree_entry_pathcmp() is the infinite "for (;;) {" loop,
and after it prepares t1 and t2 by skipping paths outside pathspec,
we check if both are empty (i.e. we ran out).  Is that the condition
you are referring to?

> but will do in the future, when there will be several parents iterated
> simultaneously, and some pair of them would run to the end.
>
> Signed-off-by: Kirill Smelkov 
> Signed-off-by: Junio C Hamano 
> ---
>
> ( re-posting without change )
>
>  tree-diff.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/tree-diff.c b/tree-diff.c
> index cf96ad7..2fd6d0e 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -12,12 +12,19 @@
>   *
>   * NOTE files and directories *always* compare differently, even when having
>   *  the same name - thanks to base_name_compare().
> + *
> + * NOTE empty (=invalid) descriptor(s) take part in comparison as +infty.

The basic idea is very sane.  It is a nice (and obvious---once you
are told about the trick) and clean restructuring of the code.

>   */
>  static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
>  {
>   struct name_entry *e1, *e2;
>   int cmp;
>  
> + if (!t1->size)
> + return t2->size ? +1 /* +∞ > c */  : 0 /* +∞ = +∞ */;
> + else if (!t2->size)
> + return -1;  /* c < +∞ */

Where do these "c" come from?  I somehow feel that these comments
are making it harder to understand what is going on.

>   e1 = &t1->entry;
>   e2 = &t2->entry;
>   cmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode,
> @@ -151,18 +158,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
>   skip_uninteresting(t1, &base, opt);
>   skip_uninteresting(t2, &base, opt);
>   }
> - if (!t1->size) {
> - if (!t2->size)
> - break;
> - show_path(&base, opt, /*t1=*/NULL, t2);
> - update_tree_entry(t2);
> - continue;
> - }
> - if (!t2->size) {
> - show_path(&base, opt, t1, /*t2=*/NULL);
> - update_tree_entry(t1);
> - continue;
> - }
> + if (!t1->size && !t2->size)
> + break;
>  
>   cmp = tree_entry_pathcmp(t1, t2);
--
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 push race condition?

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 24, 2014 at 8:18 PM, Scott Sandler
 wrote:
> I run a private Git repository (using Gitlab) with about 200 users
> doing about 100 pushes per day.

Ditto but about 2x those numbers.

> error: Ref refs/heads/master is at
> 4584c1f34e07cea2df6abc8e0d407fe016017130 but expected
> 61b79b6d35b066d054fb3deab550f1c51598cf5f
> remote: error: failed to lock refs/heads/master

I also see this error once in a while. I read the code a while back
and it's basically because there's two levels of locks that
receive-pack tries to get, and it's possible for two pushers to get
the first lock due to a race condition.

I've never seen data loss due to this though, because the inner lock is atomic.
--
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 04/10] t4209: use helper functions to test --grep

[Jeff King]

On Mon, Mar 24, 2014 at 11:22:58AM -0700, Junio C Hamano wrote:

> René Scharfe  writes:
> 
> > -test_expect_success 'log --grep -i' '
> > -   git log -i --grep=InItial --format=%H >actual &&
> > -   test_cmp expect_initial actual
> > -'
> > +test_log   expect_initial  --grep initial
> > +test_log   expect_nomatch  --grep InItial
> 
> This, and the next --author one, assumes that we will never break
> "--grep=foo" without breaking "--grep foo".  That should be OK, but
> we might want to add separate tests e.g.
> 
>   test_log expect_initial --grep=initial
> 
> perhaps?  I dunno.

Yeah, I I'd prefer "--grep=" here (and in all scripts).  In general, I
think our attitude should be that "--foo=bar" is guaranteed to work
forever, but "--foo bar" is not. The latter only works if the argument
is non-optional, so that leaves us room to "break" compatibility to make
an argument optional in a future version.

Now, whether the rest of the world and its script-writers are aware of
this fact, I don't know. So it may be too late already (but it does look
like we mention it in gitcli(7)).

-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 03/10] t4209: factor out helper function test_log_icase()

[Jeff King]

On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:

> > +test_log_icase() {
> > +   test_log $@ --regexp-ignore-case
> > +   test_log $@ -i
> 
> &&-cascade broken?  Will squash in an obvious fix.

I don't think so. This is happening outside of test_expect_success,
which is run by test_log. So adding a && means that if the first test
fails, we do not bother to run the second one at all, which is not what
we want.

-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: with reuse-delta patches, fetching with bitmaps segfaults due to possibly incomplete bitmap traverse

[Junio C Hamano]

Jeff King  writes:

> On Fri, Mar 21, 2014 at 07:58:55PM -0700, Siddharth Agarwal wrote:
>
>> At Facebook we've found that fetch speed is a bottleneck for our Git repos,
>> so we've been looking to deploy bitmaps to speed up fetches. We've been
>> trying out git-next with the top two patches from
>> https://github.com/peff/git/commits/jk/bitmap-reuse-delta, but the following
>> is reproducible with tip of that branch, currently 81cdec2.
>
> Is it also reproducible just with the tip of "next"? Note that the
> patches in jk/bitmap-reuse-delta have not been widely deployed (in
> particular, we are not yet using them at GitHub, and we track segfaults
> on our servers closely and have not seen any related to this).

Nice to hear.  I was worried for a short while if I merged what was
not cooked well, before I realized that Siddharth is on a codebase
that is more bleeding edge than I use myself ;-)
--
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

What's cooking in git.git (Mar 2014, #05; Mon, 24)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

More topics merged to 'master', some of which have been cooking
before the v1.9.0 final release, many of them fallouts from GSoC
microprojects.  Many topics that have been marked to be discarded
are finally discarded.

There seems to be a crasher somewhere in the new pack bitmap
codepath that was introduced recently. I am hoping that the root
cause is found and fixed soonish.  Other than that, things look more
or less calm on the 'next' and up.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* dk/skip-prefix-scan-only-once (2014-03-03) 1 commit
  (merged to 'next' on 2014-03-14 at ff375fc)
 + skip_prefix(): scan prefix only once

 Update implementation of skip_prefix() to scan only once; given
 that most "prefix" arguments to the inline function are constant
 strings whose strlen() can be determined at the compile time, this
 might actually make things worse with a compiler with sufficient
 intelligence.


* es/sh-i18n-envsubst (2014-03-12) 1 commit
  (merged to 'next' on 2014-03-14 at e4d5603)
 + sh-i18n--envsubst: retire unused string_list_member()


* jc/stash-pop-not-popped (2014-02-26) 1 commit
  (merged to 'next' on 2014-03-14 at 9ba1de8)
 + stash pop: mention we did not drop the stash upon failing to apply

 "stash pop", upon failing to apply the stash, refrains from
 discarding the stash to avoid information loss.  Be more explicit
 in the error message.

 The wording may want to get a bit more bikeshedding.


* jk/shallow-update-fix (2014-03-17) 3 commits
  (merged to 'next' on 2014-03-17 at 011942e)
 + shallow: verify shallow file after taking lock
  (merged to 'next' on 2014-03-12 at ce5abbf)
 + shallow: automatically clean up shallow tempfiles
 + shallow: use stat_validity to check for up-to-date file

 Serving objects from a shallow repository needs to write a new file
 to hold the temporary shallow boundaries but it was not cleaned
 when we exit due to die() or a signal.


* jn/wt-status (2014-03-12) 4 commits
  (merged to 'next' on 2014-03-14 at 8ac862c)
 + wt-status: lift the artificual "at least 20 columns" floor
 + wt-status: i18n of section labels
 + wt-status: extract the code to compute width for labels
 + wt-status: make full label string to be subject to l10n

 Unify the codepaths that format new/modified/changed sections and
 conflicted paths in the "git status" output and make it possible to
 properly internationalize their output.


* lt/request-pull (2014-03-13) 6 commits
  (merged to 'next' on 2014-03-17 at 21a598d)
 + request-pull: documentation updates
 + request-pull: resurrect "pretty refname" feature
 + request-pull: test updates
 + request-pull: pick up tag message as before
 + request-pull: allow "local:remote" to specify names on both ends
 + request-pull: more strictly match local/remote branches

 Discard the accumulated "heuristics" to guess from which branch the
 result wants to be pulled from and make sure what the end user
 specified is not second-guessed by "git request-pull", to avoid
 mistakes.


* nd/tag-version-sort (2014-02-27) 1 commit
  (merged to 'next' on 2014-03-14 at 4e7f714)
 + tag: support --sort=

 Allow v1.9.0 sorted before v1.10.0 in "git tag --list" output.


* nd/upload-pack-shallow (2014-03-11) 1 commit
  (merged to 'next' on 2014-03-14 at d40b8c3)
 + upload-pack: send shallow info over stdin to pack-objects

 Serving objects from a shallow repository needs to write a
 temporary file to be used, but the serving upload-pack may not have
 write access to the repository which is meant to be read-only.
 Instead feed these temporary shallow bounds from the standard input
 of pack-objects so that we do not have to use a temporary file.


* tc/commit-dry-run-exit-status-tests (2014-02-24) 1 commit
  (merged to 'next' on 2014-03-12 at b839886)
 + demonstrate git-commit --dry-run exit code behaviour

--
[New Topics]

* ca/doc-config-third-party (2014-03-21) 1 commit
 - config.txt: third-party tools may and do use their own variables

 Will merge to 'next'.


* dw/doc-status-no-longer-shows-pound-prefix (2014-03-21) 1 commit
 - doc: status, remove leftover statement about '#' prefix

 Will merge to 'next'.


* js/userdiff-cc (2014-03-21) 10 commits
 - userdiff: have 'cpp' hunk header pattern catch more C++ anchor points
 - t4018: test cases showing that the cpp pattern misses many anchor points
 - t4018: test cases for the built-in cpp pattern
 - t4018: reduce test files for pattern compilation tests
 - t4018: convert custom pattern test to the new infrastructure
 - t4018: convert java pattern test to the new infrastructure
 - t4018: convert perl pattern tests to th

Re: Git push race condition?

[Scott Sandler]

It's a bare repo and I didn't realize server-side reflogs were a
thing. Just ran "git config core.logallrefupdates true" in the repo on
the server which seems to be what I should do to enable that.

The server does know about B, it shows up when you do "git show B".
However "git branch --contains B" returns nothing.

The filesystem is ext4 on linux. It's on a virtual machine in our own
datacenter. It's not an NFS share or anything like that, there is
definitely only one server accessing the filesystem at a time.

Gitlab's update hook maintains an event log when any push event
happens, who pushed and which commits. The most recent time this
happened, the first push which was lost occured at 2014-03-24 19:04:51
and the one that overwrote it happened at 2014-03-24 19:05:04. That's
when the update hook ran, not necessarily when the user hit "git
push", but it is notable that it's 13 seconds apart which is a pretty
long time. We do run several hooks for checking coding syntax and
various other things so it's believable to me that the hooks would
take more than 13 seconds on occasion, but based on the testing I did
with the sleep hook it didn't seem like the hooks were actually the
problem.

On Mon, Mar 24, 2014 at 3:44 PM, Matthieu Moy
 wrote:
> Scott Sandler  writes:
>
>> Both pushes are
>> determined to be fast-forwards and both succeed, but B' overwrites B
>> and B is no longer on origin/master. The server does have B in its
>> .git directory but the commit isn't on any branch.
>
> Is the reflog enabled on the server? If so, does it say anything about B
> and B'?
>
> What filesystem do you use on the server? Is there any kind of NFS, and
> if so are you sure that there's only one machine accessing the
> filesystem at the same time?
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
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 push race condition?

[Matthieu Moy]

Scott Sandler  writes:

> Both pushes are
> determined to be fast-forwards and both succeed, but B' overwrites B
> and B is no longer on origin/master. The server does have B in its
> .git directory but the commit isn't on any branch.

Is the reflog enabled on the server? If so, does it say anything about B
and B'?

What filesystem do you use on the server? Is there any kind of NFS, and
if so are you sure that there's only one machine accessing the
filesystem at the same time?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] fetch: handle overlaping refspecs on --prune

[Junio C Hamano]

Carlos Martín Nieto  writes:

> From: Carlos Martín Nieto 
>
> We need to consider that a remote-tracking branch may match more than
> one rhs of a fetch refspec. In such a case, it is not enough to stop at
> the first match but look at all of the matches in order to determine
> whether a head is stale.
>
> To this goal, introduce a variant of query_refspecs which returns all of
> the matching refspecs and loop over those answers to check for
> staleness.
>
> Signed-off-by: Carlos Martín Nieto 
> ---
>
> There is an unfortunate duplication of code here, as
> query_refspecs_multiple is mostly query_refspecs but we only care
> about the other side of matching refspecs and disregard the 'force'
> information which query_refspecs does want.
>
> I thought about putting both together via callbacks and having
> query_refspecs stop at the first one, but I'm not sure that it would
> make it easier to read or manage.

Sorry for a belated review.

I agree with your analysis of the root-cause of the symptom exposed
by new tests in [1/2] and the proposed solution.

> @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
>   const unsigned char *sha1, int flags, void *cb_data)
>  {
>   struct stale_heads_info *info = cb_data;
> + struct string_list matches = STRING_LIST_INIT_DUP;
>   struct refspec query;
> + int i, stale = 1;
>   memset(&query, 0, sizeof(struct refspec));
>   query.dst = (char *)refname;
>  
> - if (query_refspecs(info->refs, info->ref_count, &query))
> + query_refspecs_multiple(info->refs, info->ref_count, &query, &matches);
> + if (matches.nr == 0)
>   return 0; /* No matches */
>  
>   /*
>* If we did find a suitable refspec and it's not a symref and
>* it's not in the list of refs that currently exist in that
> -  * remote we consider it to be stale.
> +  * remote we consider it to be stale. In order to deal with
> +  * overlapping refspecs, we need to go over all of the
> +  * matching refs.
>*/
> - if (!((flags & REF_ISSYMREF) ||
> -   string_list_has_string(info->ref_names, query.src))) {
> + if (flags & REF_ISSYMREF)
> + return 0;

Who frees "matches"?  At this point matches.nr != 0 so there must be
something we need to free, no?

> + for (i = 0; i < matches.nr; i++) {
> + if (string_list_has_string(info->ref_names, 
> matches.items[i].string)) {
> + stale = 0;
> + break;
> + }
> + }
> +
> + string_list_clear(&matches, 0);
> +
> + if (stale) {
>   struct ref *ref = make_linked_ref(refname, 
> &info->stale_refs_tail);
>   hashcpy(ref->new_sha1, sha1);
>   }
>  
> - free(query.src);

In the new code, query_refspecs_multiple() uses the result allocated
by match_name_with_pattern() to the results list, taking it out of
query.src without copying, so losing this free() is the right thing
to do---"matches" must be cleared.

And "string_list matches" is initialized as INIT_DUP, so we can rely
on string_list_clear() to free these strings.

>   return 0;
>  }

Regarding the seemingly duplicated logic in the new function, I
wonder if the callers of non-duplicated variant may benefit if they
notice there are multiple hits, even if they cannot use more than
one in their context.  That is, what would happen if we changed
these callers to instead of calling query-refspecs call the "multi"
variant, and if that call finds multiple matches, do something about
it (e.g. warn if they use "the first hit" because they are not
acting on later hits, possibly losing information)?

Here is a minor clean-ups, both to fix style and plug leaks, that
can be squashed to this patch.  How does it look?

Thanks.

 remote.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index 26140c7..fde7b52 100644
--- a/remote.c
+++ b/remote.c
@@ -839,9 +839,8 @@ static void query_refspecs_multiple(struct refspec *refs, 
int ref_count, struct
if (!refspec->dst)
continue;
if (refspec->pattern) {
-   if (match_name_with_pattern(key, needle, value, 
result)) {
+   if (match_name_with_pattern(key, needle, value, result))
string_list_append_nodup(results, *result);
-   }
} else if (!strcmp(needle, key)) {
string_list_append(results, value);
}
@@ -1989,32 +1988,29 @@ static int get_stale_heads_cb(const char *refname,
 
query_refspecs_multiple(info->refs, info->ref_count, &query, &matches);
if (matches.nr == 0)
-   return 0; /* No matches */
+   goto clean_exit; /* No matches */
 
/*
 * If we did find a suitable refspec and it's not a symref and
 * it's not in the list o

Git push race condition?

[Scott Sandler]

Hi folks,

I run a private Git repository (using Gitlab) with about 200 users
doing about 100 pushes per day.

I've noticed that a few times in the past several weeks, we've had
events where pushes have been lost when two people pushed at just
about the same time. The scenario is that two users both have commits
based on commit A, call them B and B'. The user with commit B pushes
at about the same time as the user who pushes B'. Both pushes are
determined to be fast-forwards and both succeed, but B' overwrites B
and B is no longer on origin/master. The server does have B in its
.git directory but the commit isn't on any branch.

I'm confident nobody is force pushing (we have a hook to disallow it
on master branches and I've seen screenshots of both user's clients
after they pushed). Both git clients say "successfully pushed A..B
master -> master" (or A..B') in the output of their push commands.
However, when the user that had B does a fetch, it shows master as
having been force updated.

We have a few pre-receive hooks and post-receive hooks that run on
pushes, and Gitlab has an update hook as well. My original theory was
that this was happening because Git checks if it's a fast-forward
before running hooks, and that the hooks taking a few seconds creates
more opportunity for a race condition to occur.

However, after reading
http://git.661346.n2.nabble.com/push-race-td7569254.html and doing
some of my own testing (creating a hook that runs for 60 seconds and
pushing from two locations to a test repo) this theory seems to be
wrong. With the 60 second sleep hook (tried as an update hook and a
pre-receive hook), I wasn't able to reproduce the problem. The second
pusher always got an error like this:

error: Ref refs/heads/master is at
4584c1f34e07cea2df6abc8e0d407fe016017130 but expected
61b79b6d35b066d054fb3deab550f1c51598cf5f
remote: error: failed to lock refs/heads/master

Which looks like exactly what I'd want Git to be doing in this
scenario, and supports what that archived thread says about how this
should work.

So the question is, how might this be happening and what can I do about it?

Thanks,
Scott
--
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 00/10] pickaxe: honor -i when used with -S and --pickaxe-regex; cleanups

[Junio C Hamano]

René Scharfe  writes:

> This series allows the options -i/--regexp-ignore-case, --pickaxe-regex,
> and -S to be used together and work as expected to perform a pickaxe
> search using case-insensitive regular expression matching.  Its first
> half refactors the test script and extends test coverage a bit while
> we're at it.  The actual change is in the sixth patch.  It enables the
> two following cleanups.  The last two patches are independent simple
> cleanups.

Very nice.  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: [PATCH 04/10] t4209: use helper functions to test --grep

[Junio C Hamano]

René Scharfe  writes:

> -test_expect_success 'log --grep -i' '
> - git log -i --grep=InItial --format=%H >actual &&
> - test_cmp expect_initial actual
> -'
> +test_log expect_initial  --grep initial
> +test_log expect_nomatch  --grep InItial

This, and the next --author one, assumes that we will never break
"--grep=foo" without breaking "--grep foo".  That should be OK, but
we might want to add separate tests e.g.

test_log expect_initial --grep=initial

perhaps?  I dunno.

Queued without any tweaks for now.  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: [PATCH 03/10] t4209: factor out helper function test_log_icase()

[Junio C Hamano]

René Scharfe  writes:

> Reduce code duplication by introducing test_log_icase() that runs the
> same test with both --regexp-ignore-case and -i.  The specification of
> the four basic test scenarios (matching/nomatching combined with case
> sensitive/insensitive) becomes easier to read and write.
>
> Signed-off-by: Rene Scharfe 
> ---
>  t/t4209-log-pickaxe.sh | 25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 9f3bb40..dd911c2 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -25,6 +25,11 @@ test_log() {
>   "
>  }
>  
> +test_log_icase() {
> + test_log $@ --regexp-ignore-case
> + test_log $@ -i

&&-cascade broken?  Will squash in an obvious fix.

> +}
--
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 14/27] update-ref.c: Extract a new function, parse_next_sha1()

[Michael Haggerty]

Replace three functions, update_store_new_sha1(),
update_store_old_sha1(), and parse_next_arg(), with a single function,
parse_next_sha1().  The new function takes care of a whole argument,
including checking whether it is there, converting it to an SHA-1, and
emitting errors on EOF or for invalid values.  The return value
indicates whether the argument was present or absent, which requires
a bit of intelligence because absent values are represented
differently depending on whether "-z" was used.

The new interface means that the calling functions, parse_cmd_*(),
don't have to interpret the result differently based on the
line_termination mode that is in effect.  It also means that
parse_cmd_create() can distinguish unambiguously between an empty new
value and a zeros new value, which fixes a failure in t1400.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  | 160 +++---
 t/t1400-update-ref.sh |   2 +-
 2 files changed, 99 insertions(+), 63 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a9eb5fe..6462b2f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,27 +35,6 @@ static struct ref_update *update_alloc(void)
return update;
 }
 
-static void update_store_new_sha1(const char *command,
- struct ref_update *update,
- const char *newvalue)
-{
-   if (*newvalue && get_sha1(newvalue, update->new_sha1))
-   die("%s %s: invalid : %s",
-   command, update->ref_name, newvalue);
-}
-
-static void update_store_old_sha1(const char *command,
- struct ref_update *update,
- const char *oldvalue)
-{
-   if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-   die("%s %s: invalid : %s",
-   command, update->ref_name, oldvalue);
-
-   /* We have an old value if non-empty, or if empty without -z */
-   update->have_old = *oldvalue || line_termination;
-}
-
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
  * and append the result to arg.  Return a pointer to the terminator.
@@ -112,35 +91,94 @@ static char *parse_refname(struct strbuf *input, const 
char **next)
 }
 
 /*
- * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
- * argument, if any.  If there is an argument, write it to arg, set
- * *next to point at the character terminating the argument, and
+ * The value being parsed is  (as opposed to ; the
+ * difference affects which error messages are generated):
+ */
+#define PARSE_SHA1_OLD 0x01
+
+/*
+ * For backwards compatibility, accept an empty string for create's
+ *  in binary mode to be equivalent to specifying zeros.
+ */
+#define PARSE_SHA1_ALLOW_EMPTY 0x02
+
+/*
+ * Parse an argument separator followed by the next argument, if any.
+ * If there is an argument, convert it to a SHA-1, write it to sha1,
+ * set *next to point at the character terminating the argument, and
  * return 0.  If there is no argument at all (not even the empty
- * string), return a non-zero result and leave *next unchanged.
+ * string), return 1 and leave *next unchanged.  If the value is
+ * provided but cannot be converted to a SHA-1, die.  flags can
+ * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_arg(struct strbuf *input, const char **next,
- struct strbuf *arg)
+static int parse_next_sha1(struct strbuf *input, const char **next,
+  unsigned char *sha1,
+  const char *command, const char *refname,
+  int flags)
 {
-   strbuf_reset(arg);
+   struct strbuf arg = STRBUF_INIT;
+   int ret = 0;
+
+   if (*next == input->buf + input->len)
+   goto eof;
+
if (line_termination) {
/* Without -z, consume SP and use next argument */
if (!**next || **next == line_termination)
-   return -1;
+   return 1;
if (**next != ' ')
-   die("expected SP but got: %s", *next);
+   die("%s %s: expected SP but got: %s",
+   command, refname, *next);
(*next)++;
-   *next = parse_arg(*next, arg);
+   *next = parse_arg(*next, &arg);
+   if (arg.len) {
+   if (get_sha1(arg.buf, sha1))
+   goto invalid;
+   } else {
+   /* Without -z, an empty value means all zeros: */
+   hashclr(sha1);
+   }
} else {
/* With -z, read the next NUL-terminated line */
if (**next)
-   die("expected NUL but got: %s", *next);
+   die("%s %s: expected NUL but got: %s",
+  

[PATCH v2 17/27] update-ref --stdin: Improve the error message for unexpected EOF

[Michael Haggerty]

Distinguish this error from the error that an argument is missing for
another reason.  Update the tests accordingly.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  |  4 ++--
 t/t1400-update-ref.sh | 12 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index eef7537..b49a5b0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -178,8 +178,8 @@ static int parse_next_sha1(struct strbuf *input, const char 
**next,
 
  eof:
die(flags & PARSE_SHA1_OLD ?
-   "%s %s missing " :
-   "%s %s missing ",
+   "%s %s: unexpected end of input when reading " :
+   "%s %s: unexpected end of input when reading ",
command, refname);
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6b21e45..1db0689 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -709,7 +709,7 @@ test_expect_success 'stdin -z fails create with bad ref 
name' '
 test_expect_success 'stdin -z fails create with no new value' '
printf $F "create $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: create $a missing " err
+   grep "fatal: create $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails create with too many arguments' '
@@ -727,7 +727,7 @@ test_expect_success 'stdin -z fails update with no ref' '
 test_expect_success 'stdin -z fails update with too few args' '
printf $F "update $a" "$m" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: update $a missing " err
+   grep "fatal: update $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails update with bad ref name' '
@@ -747,13 +747,13 @@ test_expect_success 'stdin -z emits warning with empty 
new value' '
 test_expect_success 'stdin -z fails update with no new value' '
printf $F "update $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: update $a missing " err
+   grep "fatal: update $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails update with no old value' '
printf $F "update $a" "$m" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: update $a missing " err
+   grep "fatal: update $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails update with too many arguments' '
@@ -777,7 +777,7 @@ test_expect_success 'stdin -z fails delete with bad ref 
name' '
 test_expect_success 'stdin -z fails delete with no old value' '
printf $F "delete $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: delete $a missing " err
+   grep "fatal: delete $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails delete with too many arguments' '
@@ -795,7 +795,7 @@ test_expect_success 'stdin -z fails verify with too many 
arguments' '
 test_expect_success 'stdin -z fails verify with no old value' '
printf $F "verify $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: verify $a missing " err
+   grep "fatal: verify $a: unexpected end of input when reading 
" err
 '
 
 test_expect_success 'stdin -z fails option with unknown name' '
-- 
1.9.0

--
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 22/27] struct ref_update: Rename field "ref_name" to "refname"

[Michael Haggerty]

This is consistent with the usual nomenclature.

Signed-off-by: Michael Haggerty 
---
 refs.c | 18 +-
 refs.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index dfff117..d72d0ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const 
char *refname,
  * value or to zero to ensure the ref does not exist before update.
  */
 struct ref_update {
-   const char *ref_name;
+   const char *refname;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
@@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction 
*transaction)
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
 
-   free((char *)update->ref_name);
+   free((char *)update->refname);
free(update);
}
 
@@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct 
ref_transaction *transaction,
 {
struct ref_update *update = xcalloc(1, sizeof(*update));
 
-   update->ref_name = xstrdup(refname);
+   update->refname = xstrdup(refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1, 
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
@@ -3383,7 +3383,7 @@ static int ref_update_compare(const void *r1, const void 
*r2)
 {
const struct ref_update * const *u1 = r1;
const struct ref_update * const *u2 = r2;
-   return strcmp((*u1)->ref_name, (*u2)->ref_name);
+   return strcmp((*u1)->refname, (*u2)->refname);
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
@@ -3391,14 +3391,14 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
 {
int i;
for (i = 1; i < n; i++)
-   if (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {
+   if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
-   error(str, updates[i]->ref_name); break;
+   error(str, updates[i]->refname); break;
case UPDATE_REFS_DIE_ON_ERR:
-   die(str, updates[i]->ref_name); break;
+   die(str, updates[i]->refname); break;
case UPDATE_REFS_QUIET_ON_ERR:
break;
}
@@ -3435,7 +3435,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
-   locks[i] = update_ref_lock(updates[i]->ref_name,
+   locks[i] = update_ref_lock(updates[i]->refname,
   (updates[i]->have_old ?
updates[i]->old_sha1 : NULL),
   updates[i]->flags,
@@ -3450,7 +3450,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
for (i = 0; i < n; i++)
if (!is_null_sha1(updates[i]->new_sha1)) {
ret = update_ref_write(msg,
-  updates[i]->ref_name,
+  updates[i]->refname,
   updates[i]->new_sha1,
   locks[i], onerr);
locks[i] = NULL; /* freed by update_ref_write */
diff --git a/refs.h b/refs.h
index 99c194b..30ee721 100644
--- a/refs.h
+++ b/refs.h
@@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *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 *ref_name, char *logfile, int bufsize);
+int log_ref_setup(const char *refname, char *logfile, int bufsize);
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
-- 
1.9.0

--
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 20/27] update-ref --stdin: Reimplement using reference transactions

[Michael Haggerty]

This change is mostly clerical: the parse_cmd_*() functions need to
use local variables rather than a struct ref_update to collect the
arguments needed for each update, and then call ref_transaction_*() to
queue the change rather than building up the list of changes at the
caller side.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c | 142 +++
 1 file changed, 75 insertions(+), 67 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index bbc04b2..2c8678b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
NULL
 };
 
-static int updates_alloc;
-static int updates_count;
-static struct ref_update **updates;
+static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
 
-static struct ref_update *update_alloc(void)
-{
-   struct ref_update *update;
-
-   /* Allocate and zero-init a struct ref_update */
-   update = xcalloc(1, sizeof(*update));
-   ALLOC_GROW(updates, updates_count + 1, updates_alloc);
-   updates[updates_count++] = update;
-
-   /* Store and reset accumulated options */
-   update->flags = update_flags;
-   update_flags = 0;
-
-   return update;
-}
-
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
  * and append the result to arg.  Return a pointer to the terminator.
@@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const 
char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
-
-   update = update_alloc();
+   char *refname;
+   unsigned char new_sha1[20];
+   unsigned char old_sha1[20];
+   int have_old;
 
-   update->ref_name = parse_refname(input, &next);
-   if (!update->ref_name)
+   refname = parse_refname(input, &next);
+   if (!refname)
die("update: missing ");
 
-   if (parse_next_sha1(input, &next, update->new_sha1,
-   "update", update->ref_name,
+   if (parse_next_sha1(input, &next, new_sha1, "update", refname,
PARSE_SHA1_ALLOW_EMPTY))
-   die("update %s: missing ", update->ref_name);
+   die("update %s: missing ", refname);
 
-   update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
-   "update", update->ref_name,
-   PARSE_SHA1_OLD);
+   have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname,
+   PARSE_SHA1_OLD);
 
if (*next != line_termination)
-   die("update %s: extra input: %s", update->ref_name, next);
+   die("update %s: extra input: %s", refname, next);
+
+   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old);
+
+   update_flags = 0;
+   free(refname);
 
return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
-
-   update = update_alloc();
+   char *refname;
+   unsigned char new_sha1[20];
 
-   update->ref_name = parse_refname(input, &next);
-   if (!update->ref_name)
+   refname = parse_refname(input, &next);
+   if (!refname)
die("create: missing ");
 
-   if (parse_next_sha1(input, &next, update->new_sha1,
-   "create", update->ref_name, 0))
-   die("create %s: missing ", update->ref_name);
+   if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0))
+   die("create %s: missing ", refname);
 
-   if (is_null_sha1(update->new_sha1))
-   die("create %s: zero ", update->ref_name);
+   if (is_null_sha1(new_sha1))
+   die("create %s: zero ", refname);
 
if (*next != line_termination)
-   die("create %s: extra input: %s", update->ref_name, next);
+   die("create %s: extra input: %s", refname, next);
+
+   ref_transaction_create(transaction, refname, new_sha1, update_flags);
+
+   update_flags = 0;
+   free(refname);
 
return next;
 }
 
 static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
+   char *refname;
+   unsigned char old_sha1[20];
+   int have_old;
 
-   update = update_alloc();
-
-   update->ref_name = parse_refname(input, &next);
-   if (!update->ref_name)
+   refname = parse_refname(input, &next);
+   if (!refname)
die("delete: missing ");
 
-   if (parse_next_sha1(input, &next, update->old_sha1,
-   "delete", update->ref_name, PARSE_SHA1_OLD)) {
-   update->have_old = 0;
+   

[PATCH v2 16/27] t1400: Test one mistake at a time

[Michael Haggerty]

This case wants to test passing a bad refname to the "update" command.
But it also passes too few arguments to "update", which muddles the
situation: which error should be diagnosed?  So split this test into
two:

* One that passes too few arguments to update

* One that passes all three arguments to "update", but with a bad
  refname.

Signed-off-by: Michael Haggerty 

t1400: Add a test of "update" with too few arguments

Signed-off-by: Michael Haggerty 
---
 t/t1400-update-ref.sh | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 2d61cce..6b21e45 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' '
grep "fatal: update line missing " err
 '
 
+test_expect_success 'stdin -z fails update with too few args' '
+   printf $F "update $a" "$m" >stdin &&
+   test_must_fail git update-ref -z --stdin err &&
+   grep "fatal: update $a missing " err
+'
+
 test_expect_success 'stdin -z fails update with bad ref name' '
-   printf $F "update ~a" "$m" >stdin &&
+   printf $F "update ~a" "$m" "" >stdin &&
test_must_fail git update-ref -z --stdin err &&
grep "fatal: invalid ref format: ~a" err
 '
-- 
1.9.0

--
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 21/27] refs: Remove API function update_refs()

[Michael Haggerty]

It has been superseded by reference transactions.  This also means
that struct ref_update can become private.

Signed-off-by: Michael Haggerty 
---
 refs.c | 33 -
 refs.h | 20 
 2 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/refs.c b/refs.c
index e788c27..dfff117 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,20 @@ static int update_ref_write(const char *action, const 
char *refname,
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
+ * while locking the ref, set have_old to 1 and set old_sha1 to the
+ * value or to zero to ensure the ref does not exist before update.
+ */
+struct ref_update {
+   const char *ref_name;
+   unsigned char new_sha1[20];
+   unsigned char old_sha1[20];
+   int flags; /* REF_NODEREF? */
+   int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -3393,16 +3407,17 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
return 0;
 }
 
-int update_refs(const char *action, struct ref_update * const *updates_orig,
-   int n, enum action_on_err onerr)
+int ref_transaction_commit(struct ref_transaction *transaction,
+  const char *msg, enum action_on_err onerr)
 {
int ret = 0, delnum = 0, i;
struct ref_update **updates;
int *types;
struct ref_lock **locks;
const char **delnames;
+   int n = transaction->nr;
 
-   if (!updates_orig || !n)
+   if (!n)
return 0;
 
/* Allocate work space */
@@ -3412,7 +3427,7 @@ int update_refs(const char *action, struct ref_update * 
const *updates_orig,
delnames = xmalloc(sizeof(*delnames) * n);
 
/* Copy, sort, and reject duplicate refs */
-   memcpy(updates, updates_orig, sizeof(*updates) * n);
+   memcpy(updates, transaction->updates, sizeof(*updates) * n);
qsort(updates, n, sizeof(*updates), ref_update_compare);
ret = ref_update_reject_duplicates(updates, n, onerr);
if (ret)
@@ -3434,7 +3449,7 @@ int update_refs(const char *action, struct ref_update * 
const *updates_orig,
/* Perform updates first so live commits remain referenced */
for (i = 0; i < n; i++)
if (!is_null_sha1(updates[i]->new_sha1)) {
-   ret = update_ref_write(action,
+   ret = update_ref_write(msg,
   updates[i]->ref_name,
   updates[i]->new_sha1,
   locks[i], onerr);
@@ -3462,14 +3477,6 @@ cleanup:
free(types);
free(locks);
free(delnames);
-   return ret;
-}
-
-int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, enum action_on_err onerr)
-{
-   int ret = update_refs(msg, transaction->updates, transaction->nr,
- onerr);
ref_transaction_free(transaction);
return ret;
 }
diff --git a/refs.h b/refs.h
index 476a923..99c194b 100644
--- a/refs.h
+++ b/refs.h
@@ -10,20 +10,6 @@ struct ref_lock {
int force_write;
 };
 
-/**
- * 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
- * while locking the ref, set have_old to 1 and set old_sha1 to the
- * value or to zero to ensure the ref does not exist before update.
- */
-struct ref_update {
-   const char *ref_name;
-   unsigned char new_sha1[20];
-   unsigned char old_sha1[20];
-   int flags; /* REF_NODEREF? */
-   int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
-};
-
 struct ref_transaction;
 
 /*
@@ -290,12 +276,6 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr);
 
-/**
- * Lock all refs and then perform all modifications.
- */
-int update_refs(const char *action, struct ref_update * const *updates,
-   int n, enum action_on_err onerr);
-
 extern int parse_hide_refs_config(const char *var, const char *value, const 
char *);
 extern int ref_is_hidden(const char *);
 
-- 
1.9.0

--
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 13/27] t1400: Test that stdin -z update treats empty as zeros

[Michael Haggerty]

This is the (slightly inconsistent) status quo; make sure it doesn't
change by accident.

Signed-off-by: Michael Haggerty 
---
 t/t1400-update-ref.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a2015d0..208f56e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref 
name' '
grep "fatal: invalid ref format: ~a" err
 '
 
+test_expect_success 'stdin -z treats empty new value as zeros' '
+   git update-ref $a $m &&
+   printf $F "update $a" "" "" >stdin &&
+   git update-ref -z --stdin stdin &&
test_must_fail git update-ref -z --stdin err &&
-- 
1.9.0

--
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/27] Clean up update-refs --stdin and implement ref_transaction

[Michael Haggerty]

This is v2 of this patch series.  See also [1] for more context.

Thanks to Brad, Junio, and Johan for their feedback on v1 [2].  I
think I have addressed all of your points.

Changes relative to v1:

* Rename the functions associated with ref_transactions to be more
  reminiscent of database transactions:

  * create_ref_transaction() -> ref_transaction_begin()
  * free_ref_transaction() -> ref_transaction_rollback()
  * queue_update_ref() -> ref_transaction_update()
  * queue_create_ref() -> ref_transaction_create()
  * queue_delete_ref() -> ref_transaction_delete()
  * commit_ref_transaction() -> ref_transaction_commit()

* Change ref_transaction_commit() to also free the transaction, so the
  user doesn't have to think about memory resources at all.

* Fix backwards compatibility of "git update-ref --stdin -z"'s
  handling of the "create" command: allow  to be the empty
  string, treating it the same zeros.  But deprecate this usage.

* Rebased to current master (there were no conflicts).

[1] http://article.gmane.org/gmane.comp.version-control.git/243726
[2] http://thread.gmane.org/gmane.comp.version-control.git/243731

Michael Haggerty (27):
  t1400: Fix name and expected result of one test
  t1400: Provide more usual input to the command
  parse_arg(): Really test that argument is properly terminated
  t1400: Add some more tests involving quoted arguments
  refs.h: Rename the action_on_err constants
  update_refs(): Fix constness
  update-ref --stdin: Read the whole input at once
  parse_cmd_verify(): Copy old_sha1 instead of evaluating 
twice
  update-ref.c: Extract a new function, parse_refname()
  update-ref --stdin: Improve error messages for invalid values
  update-ref --stdin: Make error messages more consistent
  update-ref --stdin: Simplify error messages for missing oldvalues
  t1400: Test that stdin -z update treats empty  as zeros
  update-ref.c: Extract a new function, parse_next_sha1()
  update-ref --stdin -z: Deprecate interpreting the empty string as
zeros
  t1400: Test one mistake at a time
  update-ref --stdin: Improve the error message for unexpected EOF
  update-ref --stdin: Harmonize error messages
  refs: Add a concept of a reference transaction
  update-ref --stdin: Reimplement using reference transactions
  refs: Remove API function update_refs()
  struct ref_update: Rename field "ref_name" to "refname"
  struct ref_update: Store refname as a FLEX_ARRAY.
  ref_transaction_commit(): Introduce temporary variables
  struct ref_update: Add a lock member
  struct ref_update: Add type field
  ref_transaction_commit(): Work with transaction->updates in place

 Documentation/git-update-ref.txt   |  18 +-
 builtin/checkout.c |   2 +-
 builtin/clone.c|   9 +-
 builtin/merge.c|   6 +-
 builtin/notes.c|   6 +-
 builtin/reset.c|   6 +-
 builtin/update-ref.c   | 425 -
 contrib/examples/builtin-fetch--tool.c |   3 +-
 notes-cache.c  |   2 +-
 notes-utils.c  |   3 +-
 refs.c | 192 +++
 refs.h |  94 ++--
 t/t1400-update-ref.sh  | 100 +---
 13 files changed, 582 insertions(+), 284 deletions(-)

-- 
1.9.0

--
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 09/27] update-ref.c: Extract a new function, parse_refname()

[Michael Haggerty]

There is no reason to obscure the fact that parse_first_arg() always
parses refnames.  Form the new function by combining parse_first_arg()
and update_store_ref_name().

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c | 90 
 1 file changed, 41 insertions(+), 49 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51adf2d..0dc2061 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,14 +35,6 @@ static struct ref_update *update_alloc(void)
return update;
 }
 
-static void update_store_ref_name(struct ref_update *update,
- const char *ref_name)
-{
-   if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))
-   die("invalid ref format: %s", ref_name);
-   update->ref_name = xstrdup(ref_name);
-}
-
 static void update_store_new_sha1(struct ref_update *update,
  const char *newvalue)
 {
@@ -86,23 +78,35 @@ static const char *parse_arg(const char *next, struct 
strbuf *arg)
 }
 
 /*
- * Parse the argument immediately after "command SP".  If not -z, then
- * handle C-quoting.  Write the argument to arg.  Set *next to point
- * at the character that terminates the argument.  Die if C-quoting is
- * malformed.
+ * Parse the reference name immediately after "command SP".  If not
+ * -z, then handle C-quoting.  Return a pointer to a newly allocated
+ * string containing the name of the reference, or NULL if there was
+ * an error.  Update *next to point at the character that terminates
+ * the argument.  Die if C-quoting is malformed or the reference name
+ * is invalid.
  */
-static void parse_first_arg(struct strbuf *input, const char **next,
-   struct strbuf *arg)
+static char *parse_refname(struct strbuf *input, const char **next)
 {
-   strbuf_reset(arg);
+   struct strbuf ref = STRBUF_INIT;
+
if (line_termination) {
/* Without -z, use the next argument */
-   *next = parse_arg(*next, arg);
+   *next = parse_arg(*next, &ref);
} else {
/* With -z, use everything up to the next NUL */
-   strbuf_addstr(arg, *next);
-   *next += arg->len;
+   strbuf_addstr(&ref, *next);
+   *next += ref.len;
+   }
+
+   if (!ref.len) {
+   strbuf_release(&ref);
+   return NULL;
}
+
+   if (check_refname_format(ref.buf, REFNAME_ALLOW_ONELEVEL))
+   die("invalid ref format: %s", ref.buf);
+
+   return strbuf_detach(&ref, NULL);
 }
 
 /*
@@ -150,111 +154,99 @@ static int parse_next_arg(struct strbuf *input, const 
char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-   struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
struct strbuf oldvalue = STRBUF_INIT;
struct ref_update *update;
 
update = update_alloc();
 
-   parse_first_arg(input, &next, &ref);
-   if (ref.buf[0])
-   update_store_ref_name(update, ref.buf);
-   else
+   update->ref_name = parse_refname(input, &next);
+   if (!update->ref_name)
die("update line missing ");
 
if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
-   die("update %s missing ", ref.buf);
+   die("update %s missing ", update->ref_name);
 
if (!parse_next_arg(input, &next, &oldvalue)) {
update_store_old_sha1(update, oldvalue.buf);
if (*next != line_termination)
-   die("update %s has extra input: %s", ref.buf, next);
+   die("update %s has extra input: %s", update->ref_name, 
next);
} else if (!line_termination)
-   die("update %s missing [] NUL", ref.buf);
+   die("update %s missing [] NUL", update->ref_name);
 
return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-   struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
struct ref_update *update;
 
update = update_alloc();
 
-   parse_first_arg(input, &next, &ref);
-   if (ref.buf[0])
-   update_store_ref_name(update, ref.buf);
-   else
+   update->ref_name = parse_refname(input, &next);
+   if (!update->ref_name)
die("create line missing ");
 
if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
-   die("create %s missing ", ref.buf);
+   die("create %s missing ", update->ref_name);
 
if (is_null_sha1(update->new_sha1))
-   die("create %s given zero new value", ref.buf);
+   die("create %s given zero new value", update->ref_

[PATCH v2 23/27] struct ref_update: Store refname as a FLEX_ARRAY.

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
---
 refs.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index d72d0ab..2b80f6d 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const 
char *refname,
  * value or to zero to ensure the ref does not exist before update.
  */
 struct ref_update {
-   const char *refname;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+   const char refname[FLEX_ARRAY];
 };
 
 /*
@@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction 
*transaction)
 {
int i;
 
-   for (i = 0; i < transaction->nr; i++) {
-   struct ref_update *update = transaction->updates[i];
-
-   free((char *)update->refname);
-   free(update);
-   }
+   for (i = 0; i < transaction->nr; i++)
+   free(transaction->updates[i]);
 
free(transaction->updates);
free(transaction);
@@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction 
*transaction)
 static struct ref_update *add_update(struct ref_transaction *transaction,
 const char *refname)
 {
-   struct ref_update *update = xcalloc(1, sizeof(*update));
+   size_t len = strlen(refname);
+   struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
 
-   update->refname = xstrdup(refname);
+   strcpy((char *)update->refname, refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1, 
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
-- 
1.9.0

--
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 24/27] ref_transaction_commit(): Introduce temporary variables

[Michael Haggerty]

Use temporary variables in the for-loop blocks to simplify expressions
in the rest of the loop.

Signed-off-by: Michael Haggerty 
---
 refs.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 2b80f6d..d51566c 100644
--- a/refs.c
+++ b/refs.c
@@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
-   locks[i] = update_ref_lock(updates[i]->refname,
-  (updates[i]->have_old ?
-   updates[i]->old_sha1 : NULL),
-  updates[i]->flags,
+   struct ref_update *update = updates[i];
+
+   locks[i] = update_ref_lock(update->refname,
+  (update->have_old ?
+   update->old_sha1 : NULL),
+  update->flags,
   &types[i], onerr);
if (!locks[i]) {
ret = 1;
@@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
}
 
/* Perform updates first so live commits remain referenced */
-   for (i = 0; i < n; i++)
-   if (!is_null_sha1(updates[i]->new_sha1)) {
+   for (i = 0; i < n; i++) {
+   struct ref_update *update = updates[i];
+
+   if (!is_null_sha1(update->new_sha1)) {
ret = update_ref_write(msg,
-  updates[i]->refname,
-  updates[i]->new_sha1,
+  update->refname,
+  update->new_sha1,
   locks[i], onerr);
locks[i] = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
}
+   }
 
/* Perform deletes now that updates are safely completed */
for (i = 0; i < n; i++)
-- 
1.9.0

--
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/27] parse_cmd_verify(): Copy old_sha1 instead of evaluating twice

[Michael Haggerty]

Aside from avoiding a tiny bit of work, this makes it transparently
obvious that old_sha1 and new_sha1 are identical.  It is arguably a
bit silly to have to set new_sha1 in order to verify old_sha1, but
that is a problem for another day.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f197fe..51adf2d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -249,7 +249,7 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
 
if (!parse_next_arg(input, &next, &value)) {
update_store_old_sha1(update, value.buf);
-   update_store_new_sha1(update, value.buf);
+   hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
die("verify %s missing [] NUL", ref.buf);
 
-- 
1.9.0

--
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 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros

[Michael Haggerty]

In the original version of this command, for the single case of the
"update" command's , the empty string was interpreted as
being equivalent to 40 "0"s.  This shorthand is unnecessary (binary
input will usually be generated programmatically anyway), and it
complicates the parser and the documentation.

So gently deprecate this usage: remove its description from the
documentation and emit a warning if it is found.  But for reasons of
backwards compatibility, continue to accept it.

Helped-by: Brad King 
Signed-off-by: Michael Haggerty 
---
 Documentation/git-update-ref.txt | 18 --
 builtin/update-ref.c |  2 ++
 t/t1400-update-ref.sh|  5 +++--
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0a0a551..c8f5ae5 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -68,7 +68,12 @@ performs all modifications together.  Specify commands of 
the form:
option SP  LF
 
 Quote fields containing whitespace as if they were strings in C source
-code.  Alternatively, use `-z` to specify commands without quoting:
+code; i.e., surrounded by double-quotes and with backslash escapes.
+Use 40 "0" characters or the empty string to specify a zero value.  To
+specify a missing value, omit the value and its preceding SP entirely.
+
+Alternatively, use `-z` to specify in NUL-terminated format, without
+quoting:
 
update SP  NUL  NUL [] NUL
create SP  NUL  NUL
@@ -76,8 +81,12 @@ code.  Alternatively, use `-z` to specify commands without 
quoting:
verify SP  NUL [] NUL
option SP  NUL
 
-Lines of any other format or a repeated  produce an error.
-Command meanings are:
+In this format, use 40 "0" to specify a zero value, and use the empty
+string to specify a missing value.
+
+In either format, values can be specified in any form that Git
+recognizes as an object name.  Commands in any other format or a
+repeated  produce an error.  Command meanings are:
 
 update::
Set  to  after verifying , if given.
@@ -102,9 +111,6 @@ option::
The only valid option is `no-deref` to avoid dereferencing
a symbolic ref.
 
-Use 40 "0" or the empty string to specify a zero value, except that
-with `-z` an empty  is considered missing.
-
 If all s can be locked with matching s
 simultaneously, all modifications are performed.  Otherwise, no
 modifications are performed.  Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6462b2f..eef7537 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char 
**next,
goto invalid;
} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
/* With -z, treat an empty value as all zeros: */
+   warning("%s %s: missing , treating as zero",
+   command, refname);
hashclr(sha1);
} else {
/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 15f5bfd..2d61cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref 
name' '
grep "fatal: invalid ref format: ~a" err
 '
 
-test_expect_success 'stdin -z treats empty new value as zeros' '
+test_expect_success 'stdin -z emits warning with empty new value' '
git update-ref $a $m &&
printf $F "update $a" "" "" >stdin &&
-   git update-ref -z --stdin err &&
+   grep "warning: update $a: missing , treating as zero" err &&
test_must_fail git rev-parse --verify -q $a
 '
 
-- 
1.9.0

--
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 25/27] struct ref_update: Add a lock member

[Michael Haggerty]

Now that we manage ref_update objects internally, we can use them to
hold some of the scratch space we need when actually carrying out the
updates.  Store the (struct ref_lock *) there.

Signed-off-by: Michael Haggerty 
---
 refs.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index d51566c..d1edd57 100644
--- a/refs.c
+++ b/refs.c
@@ -3278,6 +3278,7 @@ struct ref_update {
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+   struct ref_lock *lock;
const char refname[FLEX_ARRAY];
 };
 
@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
int ret = 0, delnum = 0, i;
struct ref_update **updates;
int *types;
-   struct ref_lock **locks;
const char **delnames;
int n = transaction->nr;
 
@@ -3420,7 +3420,6 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
/* Allocate work space */
updates = xmalloc(sizeof(*updates) * n);
types = xmalloc(sizeof(*types) * n);
-   locks = xcalloc(n, sizeof(*locks));
delnames = xmalloc(sizeof(*delnames) * n);
 
/* Copy, sort, and reject duplicate refs */
@@ -3434,12 +3433,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
 
-   locks[i] = update_ref_lock(update->refname,
-  (update->have_old ?
-   update->old_sha1 : NULL),
-  update->flags,
-  &types[i], onerr);
-   if (!locks[i]) {
+   update->lock = update_ref_lock(update->refname,
+  (update->have_old ?
+   update->old_sha1 : NULL),
+  update->flags,
+  &types[i], onerr);
+   if (!update->lock) {
ret = 1;
goto cleanup;
}
@@ -3453,19 +3452,23 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
ret = update_ref_write(msg,
   update->refname,
   update->new_sha1,
-  locks[i], onerr);
-   locks[i] = NULL; /* freed by update_ref_write */
+  update->lock, onerr);
+   update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
}
}
 
/* Perform deletes now that updates are safely completed */
-   for (i = 0; i < n; i++)
-   if (locks[i]) {
-   delnames[delnum++] = locks[i]->ref_name;
-   ret |= delete_ref_loose(locks[i], types[i]);
+   for (i = 0; i < n; i++) {
+   struct ref_update *update = updates[i];
+
+   if (update->lock) {
+   delnames[delnum++] = update->lock->ref_name;
+   ret |= delete_ref_loose(update->lock, types[i]);
}
+   }
+
ret |= repack_without_refs(delnames, delnum);
for (i = 0; i < delnum; i++)
unlink_or_warn(git_path("logs/%s", delnames[i]));
@@ -3473,11 +3476,10 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
 cleanup:
for (i = 0; i < n; i++)
-   if (locks[i])
-   unlock_ref(locks[i]);
+   if (updates[i]->lock)
+   unlock_ref(updates[i]->lock);
free(updates);
free(types);
-   free(locks);
free(delnames);
ref_transaction_free(transaction);
return ret;
-- 
1.9.0

--
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 11/27] update-ref --stdin: Make error messages more consistent

[Michael Haggerty]

The old error messages emitted for invalid input sometimes said
""/"" and sometimes said "old value"/"new value".
Convert them all to the former.  Update the tests accordingly.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  |  8 
 t/t1400-update-ref.sh | 14 +++---
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 13a884a..e4c0854 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -40,7 +40,7 @@ static void update_store_new_sha1(const char *command,
  const char *newvalue)
 {
if (*newvalue && get_sha1(newvalue, update->new_sha1))
-   die("%s %s: invalid new value: %s",
+   die("%s %s: invalid : %s",
command, update->ref_name, newvalue);
 }
 
@@ -49,7 +49,7 @@ static void update_store_old_sha1(const char *command,
  const char *oldvalue)
 {
if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-   die("%s %s: invalid old value: %s",
+   die("%s %s: invalid : %s",
command, update->ref_name, oldvalue);
 
/* We have an old value if non-empty, or if empty without -z */
@@ -198,7 +198,7 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
die("create %s missing ", update->ref_name);
 
if (is_null_sha1(update->new_sha1))
-   die("create %s given zero new value", update->ref_name);
+   die("create %s given zero ", update->ref_name);
 
if (*next != line_termination)
die("create %s has extra input: %s", update->ref_name, next);
@@ -220,7 +220,7 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
if (!parse_next_arg(input, &next, &oldvalue)) {
update_store_old_sha1("delete", update, oldvalue.buf);
if (update->have_old && is_null_sha1(update->old_sha1))
-   die("delete %s given zero old value", update->ref_name);
+   die("delete %s given zero ", 
update->ref_name);
} else if (!line_termination)
die("delete %s missing [] NUL", update->ref_name);
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f6c6e96..ef61fe3 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,21 +518,21 @@ test_expect_success 'stdin update ref fails with wrong 
old value' '
 test_expect_success 'stdin update ref fails with bad old value' '
echo "update $c $m does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: update $c: invalid old value: does-not-exist" err &&
+   grep "fatal: update $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with bad new value' '
echo "create $c does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: create $c: invalid new value: does-not-exist" err &&
+   grep "fatal: create $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with zero new value' '
echo "create $c " >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: create $c given zero new value" err &&
+   grep "fatal: create $c given zero " err &&
test_must_fail git rev-parse --verify -q $c
 '
 
@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old 
value' '
 test_expect_success 'stdin delete ref fails with zero old value' '
echo "delete $a " >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: delete $a given zero old value" err &&
+   grep "fatal: delete $a given zero " err &&
git rev-parse $m >expect &&
git rev-parse $a >actual &&
test_cmp expect actual
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong 
old value' '
 test_expect_success 'stdin -z update ref fails with bad old value' '
printf $F "update $c" "$m" "does-not-exist" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: update $c: invalid old value: does-not-exist" err &&
+   grep "fatal: update $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin -z create ref fails with bad new value' '
printf $F "create $c" "does-not-exist" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: create $c: invalid new value: does-not-exist" err &&
+   grep "fatal: create $c: invalid : does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
@@ -878,7 +878,7 @@ test_expect_success 'stdin -z delete ref fails with wrong 
old value' '
 test_expect_

[PATCH v2 26/27] struct ref_update: Add type field

[Michael Haggerty]

This is temporary space for ref_transaction_commit().

Signed-off-by: Michael Haggerty 
---
 refs.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index d1edd57..07f900a 100644
--- a/refs.c
+++ b/refs.c
@@ -3279,6 +3279,7 @@ struct ref_update {
int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
+   int type;
const char refname[FLEX_ARRAY];
 };
 
@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 {
int ret = 0, delnum = 0, i;
struct ref_update **updates;
-   int *types;
const char **delnames;
int n = transaction->nr;
 
@@ -3419,7 +3419,6 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Allocate work space */
updates = xmalloc(sizeof(*updates) * n);
-   types = xmalloc(sizeof(*types) * n);
delnames = xmalloc(sizeof(*delnames) * n);
 
/* Copy, sort, and reject duplicate refs */
@@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   (update->have_old ?
update->old_sha1 : NULL),
   update->flags,
-  &types[i], onerr);
+  &update->type, onerr);
if (!update->lock) {
ret = 1;
goto cleanup;
@@ -3465,7 +3464,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
-   ret |= delete_ref_loose(update->lock, types[i]);
+   ret |= delete_ref_loose(update->lock, update->type);
}
}
 
@@ -3479,7 +3478,6 @@ cleanup:
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
free(updates);
-   free(types);
free(delnames);
ref_transaction_free(transaction);
return ret;
-- 
1.9.0

--
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 18/27] update-ref --stdin: Harmonize error messages

[Michael Haggerty]

Make (most of) the error messages for invalid input have the same
format [1]:

$COMMAND [SP $REFNAME]: $MESSAGE

Update the tests accordingly.

[1] A few error messages are left with their old form, because
$COMMAND and $REFNAME aren't passed all the way down the call
stack.  Maybe those sites should be changed some day, too.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  | 24 
 t/t1400-update-ref.sh | 32 
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index b49a5b0..bbc04b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, 
const char *next)
 
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
-   die("update line missing ");
+   die("update: missing ");
 
if (parse_next_sha1(input, &next, update->new_sha1,
"update", update->ref_name,
PARSE_SHA1_ALLOW_EMPTY))
-   die("update %s missing ", update->ref_name);
+   die("update %s: missing ", update->ref_name);
 
update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
"update", update->ref_name,
PARSE_SHA1_OLD);
 
if (*next != line_termination)
-   die("update %s has extra input: %s", update->ref_name, next);
+   die("update %s: extra input: %s", update->ref_name, next);
 
return next;
 }
@@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
 
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
-   die("create line missing ");
+   die("create: missing ");
 
if (parse_next_sha1(input, &next, update->new_sha1,
"create", update->ref_name, 0))
-   die("create %s missing ", update->ref_name);
+   die("create %s: missing ", update->ref_name);
 
if (is_null_sha1(update->new_sha1))
-   die("create %s given zero ", update->ref_name);
+   die("create %s: zero ", update->ref_name);
 
if (*next != line_termination)
-   die("create %s has extra input: %s", update->ref_name, next);
+   die("create %s: extra input: %s", update->ref_name, next);
 
return next;
 }
@@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
 
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
-   die("delete line missing ");
+   die("delete: missing ");
 
if (parse_next_sha1(input, &next, update->old_sha1,
"delete", update->ref_name, PARSE_SHA1_OLD)) {
update->have_old = 0;
} else {
if (is_null_sha1(update->old_sha1))
-   die("delete %s given zero ", 
update->ref_name);
+   die("delete %s: zero ", update->ref_name);
update->have_old = 1;
}
 
if (*next != line_termination)
-   die("delete %s has extra input: %s", update->ref_name, next);
+   die("delete %s: extra input: %s", update->ref_name, next);
 
return next;
 }
@@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
 
update->ref_name = parse_refname(input, &next);
if (!update->ref_name)
-   die("verify line missing ");
+   die("verify: missing ");
 
if (parse_next_sha1(input, &next, update->old_sha1,
"verify", update->ref_name, PARSE_SHA1_OLD)) {
@@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
}
 
if (*next != line_termination)
-   die("verify %s has extra input: %s", update->ref_name, next);
+   die("verify %s: extra input: %s", update->ref_name, next);
 
return next;
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1db0689..48ccc4d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted 
argument' '
 test_expect_success 'stdin fails create with no ref' '
echo "create " >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: create line missing " err
+   grep "fatal: create: missing " err
 '
 
 test_expect_success 'stdin fails create with bad ref name' '
@@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref 
name' '
 test_expect_success 'stdin fails create with no new value' '
echo "create $a" >stdin &&
test_must_fail git updat

[PATCH v2 19/27] refs: Add a concept of a reference transaction

[Michael Haggerty]

Build out the API for dealing with a bunch of reference checks and
changes within a transaction.  Define an opaque ref_transaction type
that is managed entirely within refs.c.  Introduce functions for
beginning a transaction, adding updates to a transaction, and
committing/rolling back a transaction.

This API will soon replace update_refs().

Signed-off-by: Michael Haggerty 
---
 refs.c | 96 ++
 refs.h | 65 +
 2 files changed, 161 insertions(+)

diff --git a/refs.c b/refs.c
index 1305eb1..e788c27 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const 
char *refname,
return 0;
 }
 
+/*
+ * Data structure for holding a reference transaction, which can
+ * consist of checks and updates to multiple references, carried out
+ * as atomically as possible.  This structure is opaque to callers.
+ */
+struct ref_transaction {
+   struct ref_update **updates;
+   size_t alloc;
+   size_t nr;
+};
+
+struct ref_transaction *ref_transaction_begin(void)
+{
+   return xcalloc(1, sizeof(struct ref_transaction));
+}
+
+static void ref_transaction_free(struct ref_transaction *transaction)
+{
+   int i;
+
+   for (i = 0; i < transaction->nr; i++) {
+   struct ref_update *update = transaction->updates[i];
+
+   free((char *)update->ref_name);
+   free(update);
+   }
+
+   free(transaction->updates);
+   free(transaction);
+}
+
+void ref_transaction_rollback(struct ref_transaction *transaction)
+{
+   ref_transaction_free(transaction);
+}
+
+static struct ref_update *add_update(struct ref_transaction *transaction,
+const char *refname)
+{
+   struct ref_update *update = xcalloc(1, sizeof(*update));
+
+   update->ref_name = xstrdup(refname);
+   ALLOC_GROW(transaction->updates, transaction->nr + 1, 
transaction->alloc);
+   transaction->updates[transaction->nr++] = update;
+   return update;
+}
+
+void ref_transaction_update(struct ref_transaction *transaction,
+   const char *refname,
+   unsigned char *new_sha1, unsigned char *old_sha1,
+   int flags, int have_old)
+{
+   struct ref_update *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);
+}
+
+void ref_transaction_create(struct ref_transaction *transaction,
+   const char *refname,
+   unsigned char *new_sha1,
+   int flags)
+{
+   struct ref_update *update = add_update(transaction, refname);
+
+   hashcpy(update->new_sha1, new_sha1);
+   hashclr(update->old_sha1);
+   update->flags = flags;
+   update->have_old = 1;
+}
+
+void ref_transaction_delete(struct ref_transaction *transaction,
+   const char *refname,
+   unsigned char *old_sha1,
+   int flags, int have_old)
+{
+   struct ref_update *update = add_update(transaction, refname);
+
+   update->flags = flags;
+   update->have_old = have_old;
+   if (have_old)
+   hashcpy(update->old_sha1, old_sha1);
+}
+
 int update_ref(const char *action, const char *refname,
   const unsigned char *sha1, const unsigned char *oldval,
   int flags, enum action_on_err onerr)
@@ -3378,6 +3465,15 @@ cleanup:
return ret;
 }
 
+int ref_transaction_commit(struct ref_transaction *transaction,
+  const char *msg, enum action_on_err onerr)
+{
+   int ret = update_refs(msg, transaction->updates, transaction->nr,
+ onerr);
+   ref_transaction_free(transaction);
+   return ret;
+}
+
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
int i;
diff --git a/refs.h b/refs.h
index 08e60ac..476a923 100644
--- a/refs.h
+++ b/refs.h
@@ -24,6 +24,8 @@ struct ref_update {
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 };
 
+struct ref_transaction;
+
 /*
  * Bit values set in the flags argument passed to each_ref_fn():
  */
@@ -220,6 +222,69 @@ enum action_on_err {
UPDATE_REFS_QUIET_ON_ERR
 };
 
+/*
+ * Begin a reference transaction.  The reference transaction must
+ * eventually be commited using ref_transaction_commit() or rolled
+ * back using ref_transaction_rollback().
+ */
+struct ref_transaction *ref_transaction_begin(void);
+
+/*
+ * Roll back a ref_transaction and free all associated data.
+ */
+void ref_transaction_rollback(struct ref_transaction *transaction);
+
+
+/*
+ * The following functions add a reference check or update to a
+ * ref_transaction.  In all of the

[PATCH v2 07/27] update-ref --stdin: Read the whole input at once

[Michael Haggerty]

Read the whole input into a strbuf at once, and then parse it from
there.  This might also be a tad faster, but that is not the point.
The point is to decouple the parsing code from the input source (the
old parsing code had to read new data even in the middle of commands).
Add docstrings for the parsing functions.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c | 170 ---
 1 file changed, 108 insertions(+), 62 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a8a68e8..5f197fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -85,44 +85,70 @@ static const char *parse_arg(const char *next, struct 
strbuf *arg)
return next;
 }
 
-static const char *parse_first_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse the argument immediately after "command SP".  If not -z, then
+ * handle C-quoting.  Write the argument to arg.  Set *next to point
+ * at the character that terminates the argument.  Die if C-quoting is
+ * malformed.
+ */
+static void parse_first_arg(struct strbuf *input, const char **next,
+   struct strbuf *arg)
 {
-   /* Parse argument immediately after "command SP" */
strbuf_reset(arg);
if (line_termination) {
/* Without -z, use the next argument */
-   next = parse_arg(next, arg);
+   *next = parse_arg(*next, arg);
} else {
-   /* With -z, use rest of first NUL-terminated line */
-   strbuf_addstr(arg, next);
-   next = next + arg->len;
+   /* With -z, use everything up to the next NUL */
+   strbuf_addstr(arg, *next);
+   *next += arg->len;
}
-   return next;
 }
 
-static const char *parse_next_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
+ * argument, if any.  If there is an argument, write it to arg, set
+ * *next to point at the character terminating the argument, and
+ * return 0.  If there is no argument at all (not even the empty
+ * string), return a non-zero result and leave *next unchanged.
+ */
+static int parse_next_arg(struct strbuf *input, const char **next,
+ struct strbuf *arg)
 {
-   /* Parse next SP-terminated or NUL-terminated argument, if any */
strbuf_reset(arg);
if (line_termination) {
/* Without -z, consume SP and use next argument */
-   if (!*next)
-   return NULL;
-   if (*next != ' ')
-   die("expected SP but got: %s", next);
-   next = parse_arg(next + 1, arg);
+   if (!**next || **next == line_termination)
+   return -1;
+   if (**next != ' ')
+   die("expected SP but got: %s", *next);
+   (*next)++;
+   *next = parse_arg(*next, arg);
} else {
/* With -z, read the next NUL-terminated line */
-   if (*next)
-   die("expected NUL but got: %s", next);
-   if (strbuf_getline(arg, stdin, '\0') == EOF)
-   return NULL;
-   next = arg->buf + arg->len;
+   if (**next)
+   die("expected NUL but got: %s", *next);
+   (*next)++;
+   if (*next == input->buf + input->len)
+   return -1;
+   strbuf_addstr(arg, *next);
+   *next += arg->len;
}
-   return next;
+   return 0;
 }
 
-static void parse_cmd_update(const char *next)
+
+/*
+ * The following five parse_cmd_*() functions parse the corresponding
+ * command.  In each case, next points at the character following the
+ * command name and the following space.  They each return a pointer
+ * to the character terminating the command, and die with an
+ * explanatory message if there are any parsing problems.  All of
+ * these functions handle either text or binary format input,
+ * depending on how line_termination is set.
+ */
+
+static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
struct strbuf ref = STRBUF_INIT;
struct strbuf newvalue = STRBUF_INIT;
@@ -131,26 +157,28 @@ static void parse_cmd_update(const char *next)
 
update = update_alloc();
 
-   if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+   parse_first_arg(input, &next, &ref);
+   if (ref.buf[0])
update_store_ref_name(update, ref.buf);
else
die("update line missing ");
 
-   if ((next = parse_next_arg(next, &newvalue)) != NULL)
+   if (!parse_next_arg(input, &next, &newvalue))
update_store_new_sha1(update, newvalue.buf);
else
die("update %s missing ", ref.buf);
 
-   if ((next = parse_next_arg(next, &oldvalue)) != N

[PATCH v2 27/27] ref_transaction_commit(): Work with transaction->updates in place

[Michael Haggerty]

Now that we free the transaction when we are done, there is no need to
make a copy of transaction->updates before working with it.

Signed-off-by: Michael Haggerty 
---
 refs.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 07f900a..aaf75f6 100644
--- a/refs.c
+++ b/refs.c
@@ -3410,19 +3410,17 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   const char *msg, enum action_on_err onerr)
 {
int ret = 0, delnum = 0, i;
-   struct ref_update **updates;
const char **delnames;
int n = transaction->nr;
+   struct ref_update **updates = transaction->updates;
 
if (!n)
return 0;
 
/* Allocate work space */
-   updates = xmalloc(sizeof(*updates) * n);
delnames = xmalloc(sizeof(*delnames) * n);
 
/* Copy, sort, and reject duplicate refs */
-   memcpy(updates, transaction->updates, sizeof(*updates) * n);
qsort(updates, n, sizeof(*updates), ref_update_compare);
ret = ref_update_reject_duplicates(updates, n, onerr);
if (ret)
@@ -3477,7 +3475,6 @@ cleanup:
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
-   free(updates);
free(delnames);
ref_transaction_free(transaction);
return ret;
-- 
1.9.0

--
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/27] update-ref --stdin: Improve error messages for invalid values

[Michael Haggerty]

If an invalid value is passed to "update-ref --stdin" as  or
, include the command and the name of the reference at the
beginning of the error message.  Update the tests accordingly.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  | 24 +---
 t/t1400-update-ref.sh |  8 
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0dc2061..13a884a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,20 +35,22 @@ static struct ref_update *update_alloc(void)
return update;
 }
 
-static void update_store_new_sha1(struct ref_update *update,
+static void update_store_new_sha1(const char *command,
+ struct ref_update *update,
  const char *newvalue)
 {
if (*newvalue && get_sha1(newvalue, update->new_sha1))
-   die("invalid new value for ref %s: %s",
-   update->ref_name, newvalue);
+   die("%s %s: invalid new value: %s",
+   command, update->ref_name, newvalue);
 }
 
-static void update_store_old_sha1(struct ref_update *update,
+static void update_store_old_sha1(const char *command,
+ struct ref_update *update,
  const char *oldvalue)
 {
if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-   die("invalid old value for ref %s: %s",
-   update->ref_name, oldvalue);
+   die("%s %s: invalid old value: %s",
+   command, update->ref_name, oldvalue);
 
/* We have an old value if non-empty, or if empty without -z */
update->have_old = *oldvalue || line_termination;
@@ -165,12 +167,12 @@ static const char *parse_cmd_update(struct strbuf *input, 
const char *next)
die("update line missing ");
 
if (!parse_next_arg(input, &next, &newvalue))
-   update_store_new_sha1(update, newvalue.buf);
+   update_store_new_sha1("update", update, newvalue.buf);
else
die("update %s missing ", update->ref_name);
 
if (!parse_next_arg(input, &next, &oldvalue)) {
-   update_store_old_sha1(update, oldvalue.buf);
+   update_store_old_sha1("update", update, oldvalue.buf);
if (*next != line_termination)
die("update %s has extra input: %s", update->ref_name, 
next);
} else if (!line_termination)
@@ -191,7 +193,7 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
die("create line missing ");
 
if (!parse_next_arg(input, &next, &newvalue))
-   update_store_new_sha1(update, newvalue.buf);
+   update_store_new_sha1("create", update, newvalue.buf);
else
die("create %s missing ", update->ref_name);
 
@@ -216,7 +218,7 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
die("delete line missing ");
 
if (!parse_next_arg(input, &next, &oldvalue)) {
-   update_store_old_sha1(update, oldvalue.buf);
+   update_store_old_sha1("delete", update, oldvalue.buf);
if (update->have_old && is_null_sha1(update->old_sha1))
die("delete %s given zero old value", update->ref_name);
} else if (!line_termination)
@@ -240,7 +242,7 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
die("verify line missing ");
 
if (!parse_next_arg(input, &next, &value)) {
-   update_store_old_sha1(update, value.buf);
+   update_store_old_sha1("verify", update, value.buf);
hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
die("verify %s missing [] NUL", update->ref_name);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 00862bc..f6c6e96 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,14 +518,14 @@ test_expect_success 'stdin update ref fails with wrong 
old value' '
 test_expect_success 'stdin update ref fails with bad old value' '
echo "update $c $m does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: invalid old value for ref $c: does-not-exist" err &&
+   grep "fatal: update $c: invalid old value: does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with bad new value' '
echo "create $c does-not-exist" >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: invalid new value for ref $c: does-not-exist" err &&
+   grep "fatal: create $c: invalid new value: does-not-exist" err &&
test_must_fail git rev-parse --verify -q $c
 '
 
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z updat

[PATCH v2 01/27] t1400: Fix name and expected result of one test

[Michael Haggerty]

The test

stdin -z create ref fails with zero new value

actually passes an empty new value, not a zero new value.  So rename
the test s/zero/empty/, and change the expected error from

fatal: create $c given zero new value

to

fatal: create $c missing 

Of course, this makes the test fail now, so mark it
test_expect_failure.  The failure will be fixed later in this patch
series.

Signed-off-by: Michael Haggerty 
---
 t/t1400-update-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6ffd82f..fa927d2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad 
new value' '
test_must_fail git rev-parse --verify -q $c
 '
 
-test_expect_success 'stdin -z create ref fails with zero new value' '
+test_expect_failure 'stdin -z create ref fails with empty new value' '
printf $F "create $c" "" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: create $c given zero new value" err &&
+   grep "fatal: create $c missing " err &&
test_must_fail git rev-parse --verify -q $c
 '
 
-- 
1.9.0

--
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 12/27] update-ref --stdin: Simplify error messages for missing oldvalues

[Michael Haggerty]

Instead of, for example,

fatal: update refs/heads/master missing [] NUL

emit

fatal: update refs/heads/master missing 

Update the tests accordingly.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  | 6 +++---
 t/t1400-update-ref.sh | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e4c0854..a9eb5fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -176,7 +176,7 @@ static const char *parse_cmd_update(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die("update %s has extra input: %s", update->ref_name, 
next);
} else if (!line_termination)
-   die("update %s missing [] NUL", update->ref_name);
+   die("update %s missing ", update->ref_name);
 
return next;
 }
@@ -222,7 +222,7 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
if (update->have_old && is_null_sha1(update->old_sha1))
die("delete %s given zero ", 
update->ref_name);
} else if (!line_termination)
-   die("delete %s missing [] NUL", update->ref_name);
+   die("delete %s missing ", update->ref_name);
 
if (*next != line_termination)
die("delete %s has extra input: %s", update->ref_name, next);
@@ -245,7 +245,7 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
update_store_old_sha1("verify", update, value.buf);
hashcpy(update->new_sha1, update->old_sha1);
} else if (!line_termination)
-   die("verify %s missing [] NUL", update->ref_name);
+   die("verify %s missing ", update->ref_name);
 
if (*next != line_termination)
die("verify %s has extra input: %s", update->ref_name, next);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ef61fe3..a2015d0 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -739,7 +739,7 @@ test_expect_success 'stdin -z fails update with no new 
value' '
 test_expect_success 'stdin -z fails update with no old value' '
printf $F "update $a" "$m" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: update $a missing \\[\\] NUL" err
+   grep "fatal: update $a missing " err
 '
 
 test_expect_success 'stdin -z fails update with too many arguments' '
@@ -763,7 +763,7 @@ test_expect_success 'stdin -z fails delete with bad ref 
name' '
 test_expect_success 'stdin -z fails delete with no old value' '
printf $F "delete $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: delete $a missing \\[\\] NUL" err
+   grep "fatal: delete $a missing " err
 '
 
 test_expect_success 'stdin -z fails delete with too many arguments' '
@@ -781,7 +781,7 @@ test_expect_success 'stdin -z fails verify with too many 
arguments' '
 test_expect_success 'stdin -z fails verify with no old value' '
printf $F "verify $a" >stdin &&
test_must_fail git update-ref -z --stdin err &&
-   grep "fatal: verify $a missing \\[\\] NUL" err
+   grep "fatal: verify $a missing " err
 '
 
 test_expect_success 'stdin -z fails option with unknown name' '
-- 
1.9.0

--
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/27] refs.h: Rename the action_on_err constants

[Michael Haggerty]

Given that these constants are only being used when updating
references, it is inappropriate to give them such generic names as
"DIE_ON_ERR".  So prefix their names with "UPDATE_REFS_".

Signed-off-by: Michael Haggerty 
---
 builtin/checkout.c |  2 +-
 builtin/clone.c|  9 +
 builtin/merge.c|  6 +++---
 builtin/notes.c|  6 +++---
 builtin/reset.c|  6 --
 builtin/update-ref.c   |  5 +++--
 contrib/examples/builtin-fetch--tool.c |  3 ++-
 notes-cache.c  |  2 +-
 notes-utils.c  |  3 ++-
 refs.c | 18 +-
 refs.h |  9 +++--
 11 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..f79b222 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -624,7 +624,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
/* Nothing to do. */
} else if (opts->force_detach || !new->path) {  /* No longer on any 
branch. */
update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
-  REF_NODEREF, DIE_ON_ERR);
+  REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
if (!opts->quiet) {
if (old->path && advice_detached_head)
detach_advice(new->name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..af3b86f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -521,7 +521,7 @@ static void write_followtags(const struct ref *refs, const 
char *msg)
if (!has_sha1_file(ref->old_sha1))
continue;
update_ref(msg, ref->name, ref->old_sha1,
-  NULL, 0, DIE_ON_ERR);
+  NULL, 0, UPDATE_REFS_DIE_ON_ERR);
}
 }
 
@@ -589,14 +589,15 @@ static void update_head(const struct ref *our, const 
struct ref *remote,
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
const char *head = skip_prefix(our->name, 
"refs/heads/");
-   update_ref(msg, "HEAD", our->old_sha1, NULL, 0, 
DIE_ON_ERR);
+   update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
+  UPDATE_REFS_DIE_ON_ERR);
install_branch_config(0, head, option_origin, 
our->name);
}
} else if (our) {
struct commit *c = lookup_commit_reference(our->old_sha1);
/* --branch specifies a non-branch (i.e. tags), detach HEAD */
update_ref(msg, "HEAD", c->object.sha1,
-  NULL, REF_NODEREF, DIE_ON_ERR);
+  NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
} else if (remote) {
/*
 * We know remote HEAD points to a non-branch, or
@@ -604,7 +605,7 @@ static void update_head(const struct ref *our, const struct 
ref *remote,
 * Detach HEAD in all these cases.
 */
update_ref(msg, "HEAD", remote->old_sha1,
-  NULL, REF_NODEREF, DIE_ON_ERR);
+  NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
}
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..a4c3b17 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -398,7 +398,7 @@ static void finish(struct commit *head_commit,
const char *argv_gc_auto[] = { "gc", "--auto", NULL };
update_ref(reflog_message.buf, "HEAD",
new_head, head, 0,
-   DIE_ON_ERR);
+   UPDATE_REFS_DIE_ON_ERR);
/*
 * We ignore errors in 'gc --auto', since the
 * user should see them.
@@ -1222,7 +1222,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
die(_("%s - not something we can merge"), argv[0]);
read_empty(remote_head->object.sha1, 0);
update_ref("initial pull", "HEAD", remote_head->object.sha1,
-  NULL, 0, DIE_ON_ERR);
+  NULL, 0, UPDATE_REFS_DIE_ON_ERR);
goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1339,7 +1339,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
}
 
update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
-  NULL, 0, DIE_ON_ERR);
+  NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
if (remoteheads && !common)
; /* No common ancestors found. We need a real merge. */
diff --git

[PATCH v2 06/27] update_refs(): Fix constness

[Michael Haggerty]

Since full const correctness is beyond the ability of C's type system,
just put the const where it doesn't do any harm.  A (struct ref_update
**) can be passed to a (struct ref_update * const *) argument, but not
to a (const struct ref_update **) argument.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c | 2 +-
 refs.c   | 2 +-
 refs.h   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index f6345e5..a8a68e8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {
 
 static int updates_alloc;
 static int updates_count;
-static const struct ref_update **updates;
+static struct ref_update **updates;
 
 static char line_termination = '\n';
 static int update_flags;
diff --git a/refs.c b/refs.c
index 196984e..1305eb1 100644
--- a/refs.c
+++ b/refs.c
@@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update 
**updates, int n,
return 0;
 }
 
-int update_refs(const char *action, const struct ref_update **updates_orig,
+int update_refs(const char *action, struct ref_update * const *updates_orig,
int n, enum action_on_err onerr)
 {
int ret = 0, delnum = 0, i;
diff --git a/refs.h b/refs.h
index a713b34..08e60ac 100644
--- a/refs.h
+++ b/refs.h
@@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,
 /**
  * Lock all refs and then perform all modifications.
  */
-int update_refs(const char *action, const struct ref_update **updates,
+int update_refs(const char *action, struct ref_update * const *updates,
int n, enum action_on_err onerr);
 
 extern int parse_hide_refs_config(const char *var, const char *value, const 
char *);
-- 
1.9.0

--
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/27] t1400: Add some more tests involving quoted arguments

[Michael Haggerty]

Previously there were no good tests of C-quoted arguments.

Signed-off-by: Michael Haggerty 
---
 t/t1400-update-ref.sh | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 774f8c5..00862bc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -350,12 +350,18 @@ test_expect_success 'stdin fails on unknown command' '
grep "fatal: unknown command: unknown $a" err
 '
 
-test_expect_success 'stdin fails on badly quoted input' '
+test_expect_success 'stdin fails on unbalanced quotes' '
echo "create $a \"master" >stdin &&
test_must_fail git update-ref --stdin err &&
grep "fatal: badly quoted argument: \\\"master" err
 '
 
+test_expect_success 'stdin fails on invalid escape' '
+   echo "create $a \"ma\zter\"" >stdin &&
+   test_must_fail git update-ref --stdin err &&
+   grep "fatal: badly quoted argument: \\\"mazter\\\"" err
+'
+
 test_expect_success 'stdin fails on junk after quoted argument' '
echo "create \"$a\"master" >stdin &&
test_must_fail git update-ref --stdin err &&
@@ -458,6 +464,24 @@ test_expect_success 'stdin create ref works' '
test_cmp expect actual
 '
 
+test_expect_success 'stdin succeeds with quoted argument' '
+   git update-ref -d $a &&
+   echo "create $a \"$m\"" >stdin &&
+   git update-ref --stdin expect &&
+   git rev-parse $a >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'stdin succeeds with escaped character' '
+   git update-ref -d $a &&
+   echo "create $a \"ma\\163ter\"" >stdin &&
+   git update-ref --stdin expect &&
+   git rev-parse $a >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'stdin update ref creates with zero old value' '
echo "update $b $m $Z" >stdin &&
git update-ref --stdin http://vger.kernel.org/majordomo-info.html

[PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated

[Michael Haggerty]

Test that the argument is properly terminated by either whitespace or
a NUL character, even if it is quoted, to be consistent with the
non-quoted case.  Adjust the tests to expect the new error message.
Add a docstring to the function, incorporating the comments that were
formerly within the function plus some added information.

Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c  | 20 +++-
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..02b5f95 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
update->have_old = *oldvalue || line_termination;
 }
 
+/*
+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
+ * and append the result to arg.  Return a pointer to the terminator.
+ * Die if there is an error in how the argument is C-quoted.  This
+ * function is only used if not -z.
+ */
 static const char *parse_arg(const char *next, struct strbuf *arg)
 {
-   /* Parse SP-terminated, possibly C-quoted argument */
-   if (*next != '"')
+   if (*next == '"') {
+   const char *orig = next;
+
+   if (unquote_c_style(arg, next, &next))
+   die("badly quoted argument: %s", orig);
+   if (*next && !isspace(*next))
+   die("unexpected character after quoted argument: %s", 
orig);
+   } else {
while (*next && !isspace(*next))
strbuf_addch(arg, *next++);
-   else if (unquote_c_style(arg, next, &next))
-   die("badly quoted argument: %s", next);
+   }
 
-   /* Return position after the argument */
return next;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 29391c6..774f8c5 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
grep "fatal: badly quoted argument: \\\"master" err
 '
 
-test_expect_success 'stdin fails on arguments not separated by space' '
+test_expect_success 'stdin fails on junk after quoted argument' '
echo "create \"$a\"master" >stdin &&
test_must_fail git update-ref --stdin err &&
-   grep "fatal: expected SP but got: master" err
+   grep "fatal: unexpected character after quoted argument: 
\\\"$a\\\"master" err
 '
 
 test_expect_success 'stdin fails create with no ref' '
-- 
1.9.0

--
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/27] t1400: Provide more usual input to the command

[Michael Haggerty]

The old version was passing (among other things)

update SP refs/heads/c NUL NUL 0{40} NUL

to "git update-ref -z --stdin" to test whether the old-value check for
c is working.  But the  is empty, which is a bit off the
beaten track.

So, to be sure that we are testing what we want to test, provide an
actual  on the "update" line.

Signed-off-by: Michael Haggerty 
---
 t/t1400-update-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fa927d2..29391c6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with 
identity updates' '
 
 test_expect_success 'stdin -z update refs fails with wrong old value' '
git update-ref $c $m &&
-   printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "" 
"$Z" >stdin &&
+   printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" 
"$Z" >stdin &&
test_must_fail git update-ref -z --stdin err &&
grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
git rev-parse $m >expect &&
-- 
1.9.0

--
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 3/3] parse-options: make sure argh string does not have SP or _

[Junio C Hamano]

We encourage to spell an argument hint that consists of multiple
words as a single-token separated with dashes.  In order to help
catching violations added by new callers of parse-options, make sure
argh does not contain SP or _ when the code validates the option
definitions.

Signed-off-by: Junio C Hamano 
---
 parse-options.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/parse-options.c b/parse-options.c
index a5fa0b8..c81d3a0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -375,6 +375,9 @@ static void parse_options_check(const struct option *opts)
default:
; /* ok. (usually accepts an argument) */
}
+   if (opts->argh &&
+   strcspn(opts->argh, " _") != strlen(opts->argh))
+   err |= optbug(opts, "multi-word argh should use dash to 
separate words");
}
if (err)
exit(128);
-- 
1.9.1-471-gcccbd8b

--
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 0/3] Parse-options: spell multi-word placeholders with dashes

[Junio C Hamano]

This is a follow-up to Ilya's 4th round of letting scripted
porcelains to give argv-help to their users with their command line
option parser based on "rev-parse --parseopt".

While reviewing the patch, we found that a few options to the
built-in commands were described with an argv-help (the placeholder
for an option parameter, e.g. "key-id" in "--gpg-sign ")
that has multiple words to decribe a single entity, spelling these
multiple words separated in spaces.  It is more customary to spell a
multi-word parameter with dashes, and the first patch in series is
about making it so.

During the course of the development of the first patch, I needed a
mechanical way to catch existing offenders; the last patch teaches
the parse-options API implementation to find argv-help strings that
contain SP or underscore.

There is one glitch, though.  "update-index --cacheinfo" option
takes THREE parameters: mode, sha1, and path.  Because a command
line option that takes multiple options is very unusual, the second
patch introduces a new syntax to pass these three items as a single
parameter to "--cacheinfo" option, which brings our command line
argument convention more uniform and consistent.  We however cannot
deprecate or remove the traditional syntax, so it is still kept as
an alternative "backward compatibility" syntax.

Junio C Hamano (3):
  parse-options: multi-word argh should use dash to separate words
  update-index: teach --cacheinfo a new syntax "mode,sha1,path"
  parse-options: make sure argh string does not have SP or _

 Documentation/git-cherry-pick.txt  |  6 +++---
 Documentation/git-commit.txt   |  2 +-
 Documentation/git-merge.txt|  2 +-
 Documentation/git-notes.txt|  2 +-
 Documentation/git-rev-parse.txt| 16 
 Documentation/git-revert.txt   |  6 +++---
 Documentation/git-update-index.txt |  8 ++--
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 builtin/merge.c|  2 +-
 builtin/notes.c|  2 +-
 builtin/revert.c   |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-index.c | 34 +++---
 parse-options.c|  3 +++
 parse-options.h|  2 +-
 t/t2107-update-index-basic.sh  | 13 +
 17 files changed, 77 insertions(+), 29 deletions(-)

-- 
1.9.1-471-gcccbd8b

--
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 1/3] parse-options: multi-word argh should use dash to separate words

[Junio C Hamano]

"When you need to use space, use dash" is a strange way to say that
you must not use a space.  Because it is more common for the command
line descriptions to use dashed-multi-words, you do not even want to
use spaces in these places.  Rephrase the documentation to avoid
this strangeness.

Fix a few existing multi-word argument help strings, i.e.

 - GPG key-ids given to -S/--gpg-sign are "key-id";
 - Refs used for storing notes are "notes-ref"; and
 - Expiry timestamps given to --expire are "expiry-date".

and update the corresponding documentation pages.

Signed-off-by: Junio C Hamano 
---
 Documentation/git-cherry-pick.txt |  6 +++---
 Documentation/git-commit.txt  |  2 +-
 Documentation/git-merge.txt   |  2 +-
 Documentation/git-notes.txt   |  2 +-
 Documentation/git-rev-parse.txt   | 16 
 Documentation/git-revert.txt  |  6 +++---
 builtin/checkout.c|  2 +-
 builtin/commit.c  |  2 +-
 builtin/merge.c   |  2 +-
 builtin/notes.c   |  2 +-
 builtin/revert.c  |  2 +-
 builtin/tag.c |  2 +-
 parse-options.h   |  2 +-
 13 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index f1e6b2f..1c03c79 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 
 [verse]
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
- [-S[]] ...
+ [-S[]] ...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
 'git cherry-pick' --abort
@@ -101,8 +101,8 @@ effect to your index in a row.
 --signoff::
Add Signed-off-by line at the end of the commit message.
 
--S[]::
---gpg-sign[=]::
+-S[]::
+--gpg-sign[=]::
GPG-sign commits.
 
 --ff::
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 7c42e9c..d58758f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-F  | -m ] [--reset-author] [--allow-empty]
   [--allow-empty-message] [--no-verify] [-e] [--author=]
   [--date=] [--cleanup=] [--[no-]status]
-  [-i | -o] [-S[]] [--] [...]
+  [-i | -o] [-S[]] [--] [...]
 
 DESCRIPTION
 ---
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4395459..a3c1fa3 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-   [-s ] [-X ] [-S[]]
+   [-s ] [-X ] [-S[]]
[--[no-]rerere-autoupdate] [-m ] [...]
 'git merge'  HEAD ...
 'git merge' --abort
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 84bb0fe..310f0a5 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git notes' append [-F  | -m  | (-c | -C) ] []
 'git notes' edit []
 'git notes' show []
-'git notes' merge [-v | -q] [-s  ] 
+'git notes' merge [-v | -q] [-s  ] 
 'git notes' merge --commit [-v | -q]
 'git notes' merge --abort [-v | -q]
 'git notes' remove [--ignore-missing] [--stdin] [...]
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index e05e6b3..c452f33 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -290,14 +290,14 @@ The lines after the separator describe the options.
 Each line of options has this format:
 
 
-*? SP+ help LF
+*? SP+ help LF
 
 
-``::
+``::
its format is the short option character, then the long option name
separated by a comma. Both parts are not required, though at least one
is necessary. `h,help`, `dry-run` and `f` are all three correct
-   ``.
+   ``.
 
 ``::
`` are of `*`, `=`, `?` or `!`.
@@ -313,11 +313,11 @@ Each line of options has this format:
 
* Use `!` to not make the corresponding negated long option available.
 
-``::
-   ``, if specified, is used as a name of the argument in the
-   help output, for options that take arguments. `` is
-   terminated by the first whitespace. When you need to use space in the
-   argument hint use dash instead.
+``::
+   ``, if specified, is used as a name of the argument in the
+   help output, for options that take arguments. `` is
+   terminated by the first whitespace.  It is customary to use a
+   dash to separate words in a multi-word argument hint.
 
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 9eb83f0..cceb5f2 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,7 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 
 [verse]
-'git reve

[PATCH 2/3] update-index: teach --cacheinfo a new syntax "mode,sha1,path"

[Junio C Hamano]

The "--cacheinfo" option is unusual in that it takes three option
parameters.  An option with an optional parameter is bad enough.  An
option with multiple parameters is simply insane.

Introduce a new syntax that takes these three things concatenated
together with a comma, which makes the command line syntax more
uniform across subcommands, while retaining the traditional syntax
for backward compatiblity.

If we were designing the "update-index" subcommand from scratch
today, it may probably have made sense to make this option (and
possibly others) a command mode option that does not take any option
parameter (hence no need for arg-help).  But we do not live in such
an ideal world, and as far as I can tell, the command still supports
(and must support) mixed command modes in a single invocation, e.g.

$ git update-index path1 --add path2 \
--cacheinfo 100644 $(git hash-object --stdin -w 
---
 Documentation/git-update-index.txt |  8 ++--
 builtin/update-index.c | 34 +++---
 t/t2107-update-index-basic.sh  | 13 +
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index e0a8702..d6de4a0 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git update-index'
 [--add] [--remove | --force-remove] [--replace]
 [--refresh] [-q] [--unmerged] [--ignore-missing]
-[(--cacheinfo   )...]
+[(--cacheinfo ,,)...]
 [--chmod=(+|-)x]
 [--[no-]assume-unchanged]
 [--[no-]skip-worktree]
@@ -68,8 +68,12 @@ OPTIONS
 --ignore-missing::
Ignores missing files during a --refresh
 
+--cacheinfo ,,::
 --cacheinfo   ::
-   Directly insert the specified info into the index.
+   Directly insert the specified info into the index.  For
+   backward compatibility, you can also give these three
+   arguments as three separate parameters, but new users are
+   encouraged to use a single-parameter form.
 
 --index-info::
 Read index information from stdin.
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d12ad95..ba54e19 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -629,14 +629,42 @@ static int resolve_undo_clear_callback(const struct 
option *opt,
return 0;
 }
 
+static int parse_new_style_cacheinfo(const char *arg,
+unsigned int *mode,
+unsigned char sha1[],
+const char **path)
+{
+   unsigned long ul;
+   char *endp;
+
+   errno = 0;
+   ul = strtoul(arg, &endp, 8);
+   if (errno || endp == arg || *endp != ',' || (unsigned int) ul != ul)
+   return -1; /* not a new-style cacheinfo */
+   *mode = ul;
+   endp++;
+   if (get_sha1_hex(endp, sha1) || endp[40] != ',')
+   return -1;
+   *path = endp + 41;
+   return 0;
+}
+
 static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
const struct option *opt, int unset)
 {
unsigned char sha1[20];
unsigned int mode;
+   const char *path;
 
+   if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, sha1, &path)) {
+   if (add_cacheinfo(mode, sha1, path, 0))
+   die("git update-index: --cacheinfo cannot add %s", 
path);
+   ctx->argv++;
+   ctx->argc--;
+   return 0;
+   }
if (ctx->argc <= 3)
-   return error("option 'cacheinfo' expects three arguments");
+   return error("option 'cacheinfo' expects ,,");
if (strtoul_ui(*++ctx->argv, 8, &mode) ||
get_sha1_hex(*++ctx->argv, sha1) ||
add_cacheinfo(mode, sha1, *++ctx->argv, 0))
@@ -740,9 +768,9 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG | PARSE_OPT_NONEG,
really_refresh_callback},
{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
-   N_("  "),
+   N_(",,"),
N_("add the specified entry to the index"),
-   PARSE_OPT_NOARG |   /* disallow --cacheinfo= 
form */
+   PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
(parse_opt_cb *) cacheinfo_callback},
{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index a6405d3..fe2fb17 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -48,4 +48,17 @@ test_expect_success '--cacheinfo does not accept gitlink 
null sha1' '
test_cmp expe

Re: [PATCH 1/3] test-lib: Document short options in t/README

[Ilya Bobyr]

On 3/24/2014 4:39 AM, Ramsay Jones wrote:
> On 24/03/14 08:49, Ilya Bobyr wrote:
>> Most arguments that could be provided to a test have short forms.
>> Unless documented the only way to learn then is to read the code.
>>
>> Signed-off-by: Ilya Bobyr 
>> ---
>>  t/README |   10 +-
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/README b/t/README
>> index caeeb9d..ccb5989 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -71,7 +71,7 @@ You can pass --verbose (or -v), --debug (or -d), and 
>> --immediate
>>  (or -i) command line argument to the test, or by setting GIT_TEST_OPTS
>>  appropriately before running "make".
>>  
>> ---verbose::
>> +-v,--verbose::
> OK
>
>> [...]
>>  
>> ---valgrind=::
>> +-v,--valgrind=::
> The -v short option is taken, above ... :-P

Right %)
Thanks :)
This one starts only with "--va", will fix it.
--
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 000/144] Use the $( ... ) construct for command substitution instead of using the back-quotes

[Elia Pinto]

This patch series changes everywhere the back-quotes construct for command
substitution with the $( ... ).  The Git CodingGuidelines prefer 
the $( ... ) construct for command substitution instead of using the back-quotes
, or grave accents (`..`).

The backquoted form is the historical method for command substitution,
and is supported by POSIX. However,all but the simplest uses become
complicated quickly. In particular,embedded command substitutions
and/or the use of double quotes require careful escaping with the backslash
character. Because of this the POSIX shell adopted the $(…) feature from
the Korn shell. Because this construct uses distinct
opening and closing delimiters, it is much easier to follow. 
Also now the embedded double quotes no longer need escaping.

The patch is simple but involves a large number of files with different 
authors. 
Being simple I think it is wasteful to cc a large number of different people
for doing a review. 

Elia Pinto (144):
  check-builtins.sh: use the $( ... ) construct for command
substitution
  git-am.sh: use the $( ... ) construct for command substitution
  git-pull.sh: use the $( ... ) construct for command substitution
  git-rebase--merge.sh: use the $( ... ) construct for command
substitution
  git-rebase.sh: use the $( ... ) construct for command substitution
  git-stash.sh: use the $( ... ) construct for command substitution
  git-web--browse.sh: use the $( ... ) construct for command
substitution
  unimplemented.sh: use the $( ... ) construct for command substitution
  t0001-init.sh: use the $( ... ) construct for command substitution
  t0010-racy-git.sh: use the $( ... ) construct for command
substitution
  t0020-crlf.sh: use the $( ... ) construct for command substitution
  t0025-crlf-auto.sh: use the $( ... ) construct for command
substitution
  t0026-eol-config.sh: use the $( ... ) construct for command
substitution
  t0030-stripspace.sh: use the $( ... ) construct for command
substitution
  t0204-gettext-reencode-sanity.sh: use the $( ... ) construct for
command substitution
  t0300-credentials.sh: use the $( ... ) construct for command
substitution
  t1000-read-tree-m-3way.sh: use the $( ... ) construct for command
substitution
  t1001-read-tree-m-2way.sh: use the $( ... ) construct for command
substitution
  t1002-read-tree-m-u-2way.sh: use the $( ... ) construct for command
substitution
  t1003-read-tree-prefix.sh: use the $( ... ) construct for command
substitution
  t1004-read-tree-m-u-wf.sh: use the $( ... ) construct for command
substitution
  t1020-subdirectory.sh: use the $( ... ) construct for command
substitution
  t1050-large.sh: use the $( ... ) construct for command substitution
  t1100-commit-tree-options.sh: use the $( ... ) construct for command
substitution
  t1401-symbolic-ref.sh: use the $( ... ) construct for command
substitution
  t1410-reflog.sh: use the $( ... ) construct for command substitution
  t1511-rev-parse-caret.sh: use the $( ... ) construct for command
substitution
  t1512-rev-parse-disambiguation.sh: use the $( ... ) construct for
command substitution
  t2102-update-index-symlinks.sh: use the $( ... ) construct for
command substitution
  t3030-merge-recursive.sh: use the $( ... ) construct for command
substitution
  t3100-ls-tree-restrict.sh: use the $( ... ) construct for command
substitution
  t3101-ls-tree-dirname.sh: use the $( ... ) construct for command
substitution
  t3210-pack-refs.sh: use the $( ... ) construct for command
substitution
  t3403-rebase-skip.sh: use the $( ... ) construct for command
substitution
  t3511-cherry-pick-x.sh: use the $( ... ) construct for command
substitution
  t3600-rm.sh: use the $( ... ) construct for command substitution
  t3700-add.sh: use the $( ... ) construct for command substitution
  t3905-stash-include-untracked.sh: use the $( ... ) construct for
command substitution
  t3910-mac-os-precompose.sh: use the $( ... ) construct for command
substitution
  t4006-diff-mode.sh: use the $( ... ) construct for command
substitution
  t4010-diff-pathspec.sh: use the $( ... ) construct for command
substitution
  t4012-diff-binary.sh: use the $( ... ) construct for command
substitution
  t4013-diff-various.sh: use the $( ... ) construct for command
substitution
  t4014-format-patch.sh: use the $( ... ) construct for command
substitution
  t4036-format-patch-signer-mime.sh: use the $( ... ) construct for
command substitution
  t4038-diff-combined.sh: use the $( ... ) construct for command
substitution
  t4057-diff-combined-paths.sh: use the $( ... ) construct for command
substitution
  t4116-apply-reverse.sh: use the $( ... ) construct for command
substitution
  t4119-apply-config.sh: use the $( ... ) construct for command
substitution
  t4204-patch-id.sh: use the $( ... ) construct for command
substitution
  t5000-tar-tree.sh

Re: [PATCH] git-remote-hg : Enable use of, $GIT_DIR/hg/origin/clone/.hg/hgrc

[Delcypher]

ping.

On 21 February 2014 15:17, Daniel Liew  wrote:
> git-remote-hg : Enable use of, $GIT_DIR/hg/origin/clone/.hg/hgrc
>
> Use the hgrc configuration file in the internal mercurial repository in
> addition to the other system wide hgrc files. This is done by using the
> 'ui' object from the 'repository' object which will have loaded the
> repository hgrc file if it exists.
>
> Prior to this patch the mercurial repository's hgrc file was ignored
> which I consider to be a bug.
>
> Signed-off-by: Dan Liew 
> ---
>  contrib/remote-helpers/git-remote-hg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/remote-helpers/git-remote-hg
> b/contrib/remote-helpers/git-remote-hg
> index eb89ef6..451842a 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -421,7 +421,7 @@ def get_repo(url, alias):
>
>  repo = hg.repository(myui, local_path)
>  try:
> -peer = hg.peer(myui, {}, url)
> +peer = hg.peer(repo._unfilteredrepo.ui, {}, url)
>  except:
>  die('Repository error')
>  repo.pull(peer, heads=None, force=True)
> --
> 1.9.0
>
>
--
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 v3 18/25] setup.c: support multi-checkout repo setup

[Torsten Bögershausen]

On 02/18/2014 02:40 PM, Nguyễn Thái Ngọc Duy wrote:
> The repo setup procedure is updated to detect $GIT_DIR/commondir and
> set $GIT_COMMON_DIR properly.
>
> The core.worktree is ignored when $GIT_DIR/commondir presents. This is
> because "commondir" repos are intended for separate/linked checkouts
> and pointing them back to a fixed core.worktree just does not make
> sense.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/config.txt|  3 +-
>  Documentation/git-rev-parse.txt |  3 ++
>  builtin/rev-parse.c |  4 +++
>  cache.h |  1 +
>  environment.c   |  8 ++---
>  setup.c | 33 +-
>  t/t1501-worktree.sh | 76 
> +
>  t/t1510-repo-setup.sh   |  1 +
>  trace.c |  1 +
>  9 files changed, 115 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5f4d793..cbf4d97 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -381,7 +381,8 @@ false), while all other repositories are assumed to be 
> bare (bare
>  
>  core.worktree::
>   Set the path to the root of the working tree.
> - This can be overridden by the GIT_WORK_TREE environment
> + This can be overridden by the GIT_WORK_TREE
> + or GIT_COMMON_DIR environment
>   variable and the '--work-tree' command line option.
>   The value can be an absolute path or relative to the path to
>   the .git directory, which is either specified by --git-dir
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 33e4e90..8e6ad32 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -215,6 +215,9 @@ If `$GIT_DIR` is not defined and the current directory
>  is not detected to lie in a Git repository or work tree
>  print a message to stderr and exit with nonzero status.
>  
> +--git-common-dir::
> + Show `$GIT_COMMON_DIR` if defined, else `$GIT_DIR`.
> +
>  --is-inside-git-dir::
>   When the current working directory is below the repository
>   directory print "true", otherwise "false".
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index e50bc65..c7057ce 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -744,6 +744,10 @@ int cmd_rev_parse(int argc, const char **argv, const 
> char *prefix)
>   printf("%s%s.git\n", cwd, len && cwd[len-1] != 
> '/' ? "/" : "");
>   continue;
>   }
> + if (!strcmp(arg, "--git-common-dir")) {
> + puts(get_git_common_dir());
> + continue;
> + }
>   if (!strcmp(arg, "--resolve-git-dir")) {
>   const char *gitdir = resolve_gitdir(argv[i+1]);
>   if (!gitdir)
> diff --git a/cache.h b/cache.h
> index 51ade32..98b5dd3 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -407,6 +407,7 @@ extern char *get_object_directory(void);
>  extern char *get_index_file(void);
>  extern char *get_graft_file(void);
>  extern int set_git_dir(const char *path);
> +extern int get_common_dir(struct strbuf *sb, const char *gitdir);
>  extern const char *get_git_namespace(void);
>  extern const char *strip_namespace(const char *namespaced_ref);
>  extern const char *get_git_work_tree(void);
> diff --git a/environment.c b/environment.c
> index c998120..0999fc1 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -126,6 +126,7 @@ static char *expand_namespace(const char *raw_namespace)
>  
>  static void setup_git_env(void)
>  {
> + struct strbuf sb = STRBUF_INIT;
>   const char *gitfile;
>   const char *shallow_file;
>  
> @@ -134,12 +135,9 @@ static void setup_git_env(void)
>   git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
>   gitfile = read_gitfile(git_dir);
>   git_dir = xstrdup(gitfile ? gitfile : git_dir);
> - git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
> - if (git_common_dir) {
> + if (get_common_dir(&sb, git_dir))
>   git_common_dir_env = 1;
> - git_common_dir = xstrdup(git_common_dir);
> - } else
> - git_common_dir = git_dir;
> + git_common_dir = strbuf_detach(&sb, NULL);
>   git_object_dir = getenv(DB_ENVIRONMENT);
>   if (!git_object_dir) {
>   git_object_dir = xmalloc(strlen(git_common_dir) + 9);
> diff --git a/setup.c b/setup.c
> index e56ec11..d4ac878 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -170,14 +170,15 @@ void verify_non_filename(const char *prefix, const char 
> *arg)
>   "'git  [...] -- [...]'", arg);
>  }
>  
> -static void get_common_dir(struct strbuf *sb, const char *gitdir)
> +int get_common_dir(struct strbuf *sb, const char *gitdir)
>  {
>   struct strbuf data = STRBUF_INIT;
> 

Re: git log omits deleting merges

[Jeff King]

On Mon, Mar 24, 2014 at 11:25:32AM +0100, Ephrim Khong wrote:

> Thank you for the explanation, I now understand why this is happening from a
> technical point of view. From a usability perspective, it is a bit confusing
> that a flag that should intuitively increase the number of shown commits
> (--follow) removes a commit from the output. Though this is just a minor
> annoyance, so no strong opinion here.

Sorry, I focused on the --diff-filter aspect of your question. As for
--follow, I think that is less "by design" and more "what happens to
occur, because --follow is a bit of a hack".

So there may be a bug, or room for improvement in the code there, though
if the solution involves turning on diffs for all merges, that may be
prohibitively expensive.

-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][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Michael Haggerty]

On 03/24/2014 08:28 AM, Aleksey Mokhovikov wrote:
> On 03/22/2014 04:13 AM, Michael Haggerty wrote:
>> My expectation when I invented that microproject was that converting the
>> code to be table-driven would be judged *not* to be an improvement.  I
>> was hoping that a student would say "the 'if' statement is OK, but let's
>> delete this ridiculous unreachable else branch".  Possibly they would
>> convert the "if" chain into nested "if"s, which I think would allow some
>> code consolidation in one of the branches.
>>
>> But not a single student agreed with me, so I must be in a minority of
>> one (which, unfortunately, is the definition of lunacy).
>>
>> The multidimensional array lookup table is not so terrible, but I
>> personally still prefer the "if".
> 
> That was expectable. But the main goal for me was to participate in git
> development process, to become familiar with it.
> It looks hard to participate when not proposing a patch.
> I thought about make a small change in if chain, but it looked to minor
> to feel whole development process.
> I've used git features for formatting and sending a patch to mailing list.
> I've met the GNU gettext restrictions when proposed a first patch.
> Proposed another patch and tried to show Pros and Cons.
> It didn't look like applying a patch to git master branch was the main goal.
> As for me that was quite interesting and useful.

Sorry if there was a misunderstanding.  I didn't mean to criticize you
and certainly not to single you out among the many students who went for
a table-driven solution.  I was rather replying to Junio's general
comment, that maybe changing the code to be table-driven wasn't such a
good idea.  These things are always a matter a taste, and sometimes hard
to predict before one has tried writing the code a couple of different ways.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 v3 2/2] fsck.c:fsck_commit replace memcmp by skip_prefix

[Ashwin Jha]

Replace memcmp by skip_prefix as it serves the dual
purpose of checking the string for a prefix as well
as skipping that prefix.

Signed-off-by: Ashwin Jha 
---

fsck_commit(): After the first patch in this series, it is now safe to replace
memcmp() with skip_prefix().

Previous versions can be found at [v1] and [v2]:
[v1]: 
http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html
[v2]: 
http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html

 fsck.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fsck.c b/fsck.c
index b5f017f..2232ce3 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "fsck.h"
+#include "git-compat-util.h"
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -284,21 +285,23 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
 
 static int fsck_commit(struct commit *commit, fsck_error error_func)
 {
-   const char *buffer = commit->buffer;
+   const char *parent_id, *buffer = commit->buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
int err;
 
-   if (memcmp(buffer, "tree ", 5))
+   buffer = skip_prefix(buffer, "tree ");
+   if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format 
- expected 'tree' line");
-   if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
+   if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' 
line format - bad sha1");
-   buffer += 46;
-   while (!memcmp(buffer, "parent ", 7)) {
-   if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+   buffer += 41;
+   while ((parent_id = skip_prefix(buffer, "parent "))) {
+   buffer = parent_id;
+   if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid 
'parent' line format - bad sha1");
-   buffer += 48;
+   buffer += 41;
parents++;
}
graft = lookup_commit_graft(commit->object.sha1);
@@ -322,15 +325,15 @@ static int fsck_commit(struct commit *commit, fsck_error 
error_func)
if (p || parents)
return error_func(&commit->object, FSCK_ERROR, "parent 
objects missing");
}
-   if (memcmp(buffer, "author ", 7))
+   buffer = skip_prefix(buffer, "author ");
+   if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format 
- expected 'author' line");
-   buffer += 7;
err = fsck_ident(&buffer, &commit->object, error_func);
if (err)
return err;
-   if (memcmp(buffer, "committer ", strlen("committer ")))
+   buffer = skip_prefix(buffer, "committer ");
+   if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format 
- expected 'committer' line");
-   buffer += strlen("committer ");
err = fsck_ident(&buffer, &commit->object, error_func);
if (err)
return err;
-- 
1.9.1

--
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 v3 1/2] fsck.c: modify fsck_ident() and fsck_commit()

[Ashwin Jha]

In fsck_ident(): Replace argument char **ident with const char **ident
In fsck_commit(): Replace char *buffer with const char *buffer

In both the cases, referenced memory addresses are not modified. So, it
will be a good practice, to declare them as const.

Signed-off-by: Ashwin Jha 
---

Change in fsck_commit() and fsck_ident() as per the discussion with
Eric (follow at [1]).
[1]: 
http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html

 fsck.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsck.c b/fsck.c
index 64bf279..b5f017f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, 
fsck_error error_func)
return retval;
 }
 
-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
+static int fsck_ident(const char **ident, struct object *obj, fsck_error 
error_func)
 {
char *end;
 
@@ -284,7 +284,7 @@ static int fsck_ident(char **ident, struct object *obj, 
fsck_error error_func)
 
 static int fsck_commit(struct commit *commit, fsck_error error_func)
 {
-   char *buffer = commit->buffer;
+   const char *buffer = commit->buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
-- 
1.9.1

--
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/3] test-lib: Document short options in t/README

[Ramsay Jones]

On 24/03/14 08:49, Ilya Bobyr wrote:
> Most arguments that could be provided to a test have short forms.
> Unless documented the only way to learn then is to read the code.
> 
> Signed-off-by: Ilya Bobyr 
> ---
>  t/README |   10 +-
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/t/README b/t/README
> index caeeb9d..ccb5989 100644
> --- a/t/README
> +++ b/t/README
> @@ -71,7 +71,7 @@ You can pass --verbose (or -v), --debug (or -d), and 
> --immediate
>  (or -i) command line argument to the test, or by setting GIT_TEST_OPTS
>  appropriately before running "make".
>  
> ---verbose::
> +-v,--verbose::

OK

>   This makes the test more verbose.  Specifically, the
>   command being run and their output if any are also
>   output.
> @@ -81,7 +81,7 @@ appropriately before running "make".
>   numbers matching .  The number matched against is
>   simply the running count of the test within the file.
>  
> ---debug::
> +-d,--debug::
>   This may help the person who is developing a new test.
>   It causes the command defined with test_debug to run.
>   The "trash" directory (used to store all temporary data
> @@ -89,18 +89,18 @@ appropriately before running "make".
>   failed tests so that you can inspect its contents after
>   the test finished.
>  
> ---immediate::
> +-i,--immediate::
>   This causes the test to immediately exit upon the first
>   failed test. Cleanup commands requested with
>   test_when_finished are not executed if the test failed,
>   in order to keep the state for inspection by the tester
>   to diagnose the bug.
>  
> ---long-tests::
> +-l,--long-tests::
>   This causes additional long-running tests to be run (where
>   available), for more exhaustive testing.
>  
> ---valgrind=::
> +-v,--valgrind=::

The -v short option is taken, above ... :-P

>   Execute all Git binaries under valgrind tool  and exit
>   with status 126 on errors (just like regular tests, this will
>   only stop the test script when running under -i).
> 

ATB,
Ramsay Jones



--
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 log omits deleting merges

[Ephrim Khong]

Am 20.03.14 20:54, schrieb Jeff King:

On Thu, Feb 20, 2014 at 08:35:33AM +0100, Ephrim Khong wrote:


Hi, git log seems to omit merge commits that delete a file if --follow or
--diff-filter=D is given. Below is a testcase. I'm not sure if it is desired
behaviour for --diff-filter=D, but it's probably not correct that --follow
_removes_ the merge commit from the log output.


This is by design. Git-log does not calculate or show merge diffs unless
"-c" or "--cc" is specified, and thus no diff-filter can match.


Thank you for the explanation, I now understand why this is happening 
from a technical point of view. From a usability perspective, it is a 
bit confusing that a flag that should intuitively increase the number of 
shown commits (--follow) removes a commit from the output. Though this 
is just a minor annoyance, so no strong opinion here.


- Eph


--
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 3/3] test-lib: '--run' to run only specific tests

[Ilya Bobyr]

Allow better control of the set of tests that will be executed for a
single test suite.  Mostly useful while debugging or developing as it
allows to focus on a specific test.

Signed-off-by: Ilya Bobyr 
---
 t/README |   65 ++-
 t/t-basic.sh |  233 ++
 t/test-lib.sh|   85 
 3 files changed, 379 insertions(+), 4 deletions(-)

diff --git a/t/README b/t/README
index ccb5989..519f0dc 100644
--- a/t/README
+++ b/t/README
@@ -100,6 +100,10 @@ appropriately before running "make".
This causes additional long-running tests to be run (where
available), for more exhaustive testing.
 
+-r,--run=::
+   This causes only specific tests to be included or excluded.  See
+   section "Skipping Tests" below for "" syntax.
+
 -v,--valgrind=::
Execute all Git binaries under valgrind tool  and exit
with status 126 on errors (just like regular tests, this will
@@ -187,10 +191,63 @@ and either can match the "t[0-9]{4}" part to skip the 
whole
 test, or t[0-9]{4} followed by ".$number" to say which
 particular test to skip.
 
-Note that some tests in the existing test suite rely on previous
-test item, so you cannot arbitrarily disable one and expect the
-remainder of test to check what the test originally was intended
-to check.
+For an individual test suite --run could be used to specify that
+only some tests should be run or that some tests should be
+excluded from a run.
+
+--run argument is a list of patterns with optional prefixes that
+are matched against test numbers within the current test suite.
+Supported pattern:
+
+ - A number matches a test with that number.
+
+ - sh metacharacters such as '*', '?' and '[]' match as usual in
+   shell.
+
+ - A number prefixed with '<', '<=', '>', or '>=' matches all
+   tests 'before', 'before or including', 'after', or 'after or
+   including' the specified one.
+
+Optional prefixes are:
+
+ - '+' or no prefix: test(s) matching the pattern are included in
+   the run.
+
+ - '-' or '!': test(s) matching the pattern are exluded from the
+   run.
+
+If --run starts with '+' or unprefixed pattern the initial set of
+tests to run is empty. If the first pattern starts with '-' or
+'!' all the tests are added to the initial set.  After initial
+set is determined every pattern, test number or range is added or
+excluded from the set one by one, from left to right.
+
+For example, common case is to run several setup tests and then a
+specific test that relies on that setup:
+
+$ sh ./t9200-git-cvsexport-commit.sh --run='1 2 3 21'
+
+or:
+
+$ sh ./t9200-git-cvsexport-commit.sh --run='<4 21'
+
+To run only tests up to a specific test one could do this:
+
+$ sh ./t9200-git-cvsexport-commit.sh --run='!>=21'
+
+As noted above test set is build going though patterns left to
+right, so this:
+
+$ sh ./t9200-git-cvsexport-commit.sh --run='<5 !3'
+
+will run tests 1, 2, and 4.
+
+Some tests in the existing test suite rely on previous test item,
+so you cannot arbitrarily disable one and expect the remainder of
+test to check what the test originally was intended to check.
+--run is mostly useful when you want to focus on a specific test
+and know what you are doing.  Or when you want to run up to a
+certain test.
 
 
 Naming Tests
diff --git a/t/t-basic.sh b/t/t-basic.sh
index ae8874e..4da527f 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -333,6 +333,239 @@ test_expect_success 'GIT_SKIP_TESTS sh pattern' "
EOF
 "
 
+test_expect_success '--run basic' "
+   run_sub_test_lib_test run-basic \
+   '--run basic' --run='1 3 5' <<-\\EOF &&
+   for i in 1 2 3 4 5 6
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test run-basic <<-\\EOF
+   > ok 1 - passing test #1
+   > ok 2 # skip passing test #2 (--run)
+   > ok 3 - passing test #3
+   > ok 4 # skip passing test #4 (--run)
+   > ok 5 - passing test #5
+   > ok 6 # skip passing test #6 (--run)
+   > # passed all 6 test(s)
+   > 1..6
+   EOF
+"
+
+test_expect_success '--run with <' "
+   run_sub_test_lib_test run-lt \
+   '--run with <' --run='<4' <<-\\EOF &&
+   for i in 1 2 3 4 5 6
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test run-lt <<-\\EOF
+   > ok 1 - passing test #1
+   > ok 2 - passing test #2
+   > ok 3 - passing test #3
+   > ok 4 # skip passing test #4 (--run)
+   > ok 5 # skip passing test #5 (--run)
+   > ok 6 # skip passing test #6 (--run)
+   > # passed all 6 test(s)
+   > 1..6
+   EOF
+"
+
+test_expect_success '--run with <=' "
+   run_sub_test_lib_test run-le \
+   '--run with <=' --run='<=4' <<-\\EOF &&
+   for i in 1 2 3 4 5 6
+  

[RFC/PATCH] Better control of the tests run by a test suite

[Ilya Bobyr]

Hello,

This is a second attempt on a functionality I proposed in

[PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
http://www.mail-archive.com/git%40vger.kernel.org/msg44828.html

except that the implementation is quite different now.

I hope that I have accounted for the comments that were voiced so
far.  Let's see :)

The idea behind the change is that sometimes it is convenient to
run only certain tests from a test suite.  Specifically I am
thinking about the following two use cases:

 1. You are developing new functionality.  You add a test that
fails and then you add and modify code to make it pass.

 2. You have a failed test and you need to understand what is
wrong.

In the first case you when you run the test suite, you probably
want to run some setup tests and then only one test that you are
focused on.

For code written in C time between you make a change and see a
test result is considerably increased by the compilation.  But
for script code turn around time is mostly due to the run time of
the test suite itself. [1]

For the second case you actually want the test suite to stop
after the failing test, so that you can examine the trash
directory without any modifications from the subsequent tests.
You probably do not care about them.

In the previous patch I've added an environment variable to
control tests to be run in a test suite.  I thought that it would
be similar to an already existing GIT_SKIP_TESTS.  As I did not
provide a cover letter that caused some misunderstanding I think.

This patch adds new command line argument '--run' that accepts a
list of patterns and restrictions on the test numbers that would
be included or excluded from this run of the test suite.

During discussion of the previous patch there were some talks
about extending GIT_SKIP_TESTS syntax.  In particular here:

> That is
> 
> GIT_SKIP_TESTS='t9??? !t91??'
> 
> would skip nine-thousand series, but would run 91xx series, and all
> the others are not excluded.
> 
> Simple rules to consider:
> 
>  - If the list consists of _only_ negated patterns, pretend that
>there is "unless otherwise specified with negatives, skip all
>tests", i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
>would treat GIT_SKIP_TESTS='* !t91??'.
> 
>  - The orders should not matter for simplicity of the semantics;
>before running each test, check if it matches any negative (and
>run it if it matches, without looking at any positives), and
>otherwise check if it matches any positive (and skip it if it
>does not).
> 
> Hmm?

http://www.mail-archive.com/git%40vger.kernel.org/msg44922.html


I've used that as a basis, but the end result is somewhat
different.  Plus I've added boundary checks as in "<123".

Here are some examples of how functionality added by the patch
could be used.  In order to run setup tests and then only a
specific test (use case 1) one can do:

$ ./t-init.sh --run='1 2 25'

or:

$ ./t-init.sh --run='<3 25'

('<=' is also supported, as well as '>' and '>=').

In order to run up to a specific test (use case 2) one can do:

$ ./t-init.sh --run='<=34'

or:

$ ./t-init.sh --run='!>34'

Simple semantics described above is easy to implement, but at the
same time is probably not that convenient.  Rules implemented by
the patch are as follows:

 - Order does matter.  Whatever is specified on the right has
   higher precedence.

 - When the first pattern is positive the initial set of the
   tests to be run is empty.  You are adding to an empty set as
   in '1 2 25'.

   When the first pattern is negative the initial set of the
   tests to run contains all the tests.  You are subtracting
   from that set as in '!>34'.

It seems that for simple cases that gives simple syntax and is
almost unbiased if you think about preferring inclusion over
exclusion.  While it is unlikely it also allows for complicated
expressions.  And the implementation is quite simple as well.

Main functionality is in the third patch.  First two are just
minor fixes in related parts of the code.

P.S. I did not reply to the previous patch thread as this one is
quite different.


[1] Here are some times I see on Cygin:

$ touch builtin/rev-parse.c

$ time make
...

real0m10.382s
user0m3.829s
sys 0m5.269s

Running the t-init.sh test suite is like this:

$ time ./t0001-init.sh
[...]
1..36

real0m6.693s
user0m1.505s
sys 0m3.937s

If I run only the 1, 2, 4 and 5th tests, it only half the time to
run the tests:

$ time GIT_SKIP_TESTS='t0001.[36789] t0001.??' ./t0001-init.sh
[...]
1..36

real0m3.313s
user0m0.769s
sys 0m1.844s 

Overall the change is from 17 to 14 seconds it is not that big.

If you only consider the test suite, as you do while you develop
an sh based tool, for example, the change is from 6.6 to 3.3
seconds.  That is quite n

[PATCH 1/3] test-lib: Document short options in t/README

[Ilya Bobyr]

Most arguments that could be provided to a test have short forms.
Unless documented the only way to learn then is to read the code.

Signed-off-by: Ilya Bobyr 
---
 t/README |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/README b/t/README
index caeeb9d..ccb5989 100644
--- a/t/README
+++ b/t/README
@@ -71,7 +71,7 @@ You can pass --verbose (or -v), --debug (or -d), and 
--immediate
 (or -i) command line argument to the test, or by setting GIT_TEST_OPTS
 appropriately before running "make".
 
---verbose::
+-v,--verbose::
This makes the test more verbose.  Specifically, the
command being run and their output if any are also
output.
@@ -81,7 +81,7 @@ appropriately before running "make".
numbers matching .  The number matched against is
simply the running count of the test within the file.
 
---debug::
+-d,--debug::
This may help the person who is developing a new test.
It causes the command defined with test_debug to run.
The "trash" directory (used to store all temporary data
@@ -89,18 +89,18 @@ appropriately before running "make".
failed tests so that you can inspect its contents after
the test finished.
 
---immediate::
+-i,--immediate::
This causes the test to immediately exit upon the first
failed test. Cleanup commands requested with
test_when_finished are not executed if the test failed,
in order to keep the state for inspection by the tester
to diagnose the bug.
 
---long-tests::
+-l,--long-tests::
This causes additional long-running tests to be run (where
available), for more exhaustive testing.
 
---valgrind=::
+-v,--valgrind=::
Execute all Git binaries under valgrind tool  and exit
with status 126 on errors (just like regular tests, this will
only stop the test script when running under -i).
-- 
1.7.9

--
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 2/3] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(GIT_SKIP_TESTS)" instead.

Plus tests that check basic GIT_SKIP_TESTS functions.

Signed-off-by: Ilya Bobyr 
---
 t/t-basic.sh |   63 ++
 t/test-lib.sh|   13 ++
 2 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index a2bb63c..ae8874e 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -270,6 +270,69 @@ test_expect_success 'test --verbose-only' '
EOF
 '
 
+test_expect_success 'GIT_SKIP_TESTS' "
+   GIT_SKIP_TESTS='git.2' \
+   run_sub_test_lib_test git-skip-tests-basic \
+   'GIT_SKIP_TESTS' <<-\\EOF &&
+   for i in 1 2 3
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test git-skip-tests-basic <<-\\EOF
+   > ok 1 - passing test #1
+   > ok 2 # skip passing test #2 (GIT_SKIP_TESTS)
+   > ok 3 - passing test #3
+   > # passed all 3 test(s)
+   > 1..3
+   EOF
+"
+
+test_expect_success 'GIT_SKIP_TESTS several tests' "
+   GIT_SKIP_TESTS='git.2 git.5' \
+   run_sub_test_lib_test git-skip-tests-several \
+   'GIT_SKIP_TESTS several tests' <<-\\EOF &&
+   for i in 1 2 3 4 5 6
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test git-skip-tests-several <<-\\EOF
+   > ok 1 - passing test #1
+   > ok 2 # skip passing test #2 (GIT_SKIP_TESTS)
+   > ok 3 - passing test #3
+   > ok 4 - passing test #4
+   > ok 5 # skip passing test #5 (GIT_SKIP_TESTS)
+   > ok 6 - passing test #6
+   > # passed all 6 test(s)
+   > 1..6
+   EOF
+"
+
+test_expect_success 'GIT_SKIP_TESTS sh pattern' "
+   GIT_SKIP_TESTS='git.[2-5]' \
+   run_sub_test_lib_test git-skip-tests-sh-pattern \
+   'GIT_SKIP_TESTS sh pattern' <<-\\EOF &&
+   for i in 1 2 3 4 5 6
+   do
+   test_expect_success \"passing test #\$i\" 'true'
+   done
+   test_done
+   EOF
+   check_sub_test_lib_test git-skip-tests-sh-pattern <<-\\EOF
+   > ok 1 - passing test #1
+   > ok 2 # skip passing test #2 (GIT_SKIP_TESTS)
+   > ok 3 # skip passing test #3 (GIT_SKIP_TESTS)
+   > ok 4 # skip passing test #4 (GIT_SKIP_TESTS)
+   > ok 5 # skip passing test #5 (GIT_SKIP_TESTS)
+   > ok 6 - passing test #6
+   > # passed all 6 test(s)
+   > 1..6
+   EOF
+"
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 569b52d..e035f36 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -452,25 +452,28 @@ test_finish_ () {
 
 test_skip () {
to_skip=
+   skipped_reason=
if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
then
to_skip=t
+   skipped_reason="GIT_SKIP_TESTS"
fi
if test -z "$to_skip" && test -n "$test_prereq" &&
   ! test_have_prereq "$test_prereq"
then
to_skip=t
-   fi
-   case "$to_skip" in
-   t)
+
of_prereq=
if test "$missing_prereq" != "$test_prereq"
then
of_prereq=" of $test_prereq"
fi
-
+   skipped_reason="missing $missing_prereq${of_prereq}"
+   fi
+   case "$to_skip" in
+   t)
say_color skip >&3 "skipping test: $@"
-   say_color skip "ok $test_count # skip $1 (missing 
$missing_prereq${of_prereq})"
+   say_color skip "ok $test_count # skip $1 ($skipped_reason)"
: true
;;
*)
-- 
1.7.9

--
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][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()

[Aleksey Mokhovikov]

On 03/22/2014 04:13 AM, Michael Haggerty wrote:
> My expectation when I invented that microproject was that converting the
> code to be table-driven would be judged *not* to be an improvement.  I
> was hoping that a student would say "the 'if' statement is OK, but let's
> delete this ridiculous unreachable else branch".  Possibly they would
> convert the "if" chain into nested "if"s, which I think would allow some
> code consolidation in one of the branches.
>
> But not a single student agreed with me, so I must be in a minority of
> one (which, unfortunately, is the definition of lunacy).
>
> The multidimensional array lookup table is not so terrible, but I
> personally still prefer the "if".
>
> Michael
>

That was expectable. But the main goal for me was to participate in git
development process, to become familiar with it.
It looks hard to participate when not proposing a patch.
I thought about make a small change in if chain, but it looked to minor
to feel whole development process.
I've used git features for formatting and sending a patch to mailing list.
I've met the GNU gettext restrictions when proposed a first patch.
Proposed another patch and tried to show Pros and Cons.
It didn't look like applying a patch to git master branch was the main goal.
As for me that was quite interesting and useful.





--
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