[bug] git checkout lies about number of ahead commits when switching from detached HEAD

[bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Piotr Krukowiecki]

It says "by 0 commits" when going back from detached HEAD to
master branch:


$ git checkout HEAD^

$ git checkout master
Previous HEAD position was af4c62a... Merge branch 'maint'
Switched to branch 'master'
Your branch is ahead of 'origin/master' by 0 commits.
                                        ^^^^^^^^^^^^

$ git status
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
                                          ^^^^^^^^^^^

#
nothing to commit (working directory clean)


-- 
Piotr Krukowiecki

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Jeff King]

On Sat, Mar 19, 2011 at 10:53:13PM +0100, Piotr Krukowiecki wrote:

> It says "by 0 commits" when going back from detached HEAD to
> master branch:
> 
> 
> $ git checkout HEAD^
> 
> $ git checkout master
> Previous HEAD position was af4c62a... Merge branch 'maint'
> Switched to branch 'master'
> Your branch is ahead of 'origin/master' by 0 commits.
>                                         ^^^^^^^^^^^^
> 
> $ git status
> # On branch master
> # Your branch is ahead of 'origin/master' by 1 commit.
>                                           ^^^^^^^^^^^

Hmm. My guess is that the new "check for connectivity when leaving
detached HEAD" test is polluting the commit flags for the ahead/behind
test.

[bisect bisect bisect]

Yep, it bisects to 8e2dc6a (commit: give final warning when reattaching
HEAD to leave commits behind, 2011-02-18). We probably need to clean the
uninteresting flags between the two traversals.

-Peff

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Jeff King]

On Sat, Mar 19, 2011 at 06:28:52PM -0400, Jeff King wrote:

> On Sat, Mar 19, 2011 at 10:53:13PM +0100, Piotr Krukowiecki wrote:
> 
> > It says "by 0 commits" when going back from detached HEAD to
> > master branch:
> > 
> > 
> > $ git checkout HEAD^
> > 
> > $ git checkout master
> > Previous HEAD position was af4c62a... Merge branch 'maint'
> > Switched to branch 'master'
> > Your branch is ahead of 'origin/master' by 0 commits.
> >                                         ^^^^^^^^^^^^
> > 
> > $ git status
> > # On branch master
> > # Your branch is ahead of 'origin/master' by 1 commit.
> >                                           ^^^^^^^^^^^
> 
> Hmm. My guess is that the new "check for connectivity when leaving
> detached HEAD" test is polluting the commit flags for the ahead/behind
> test.
> 
> [bisect bisect bisect]
> 
> Yep, it bisects to 8e2dc6a (commit: give final warning when reattaching
> HEAD to leave commits behind, 2011-02-18). We probably need to clean the
> uninteresting flags between the two traversals.

This patch fixes it:

diff --git a/remote.c b/remote.c
index ca42a12..92b7428 100644
--- a/remote.c
+++ b/remote.c
@@ -1560,6 +1560,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
 	strcpy(symmetric + 40, "...");
 	strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));
 
+	clear_commit_marks(ours, -1);
+	clear_commit_marks(theirs, -1);
 	init_revisions(&revs, NULL);
 	setup_revisions(rev_argc, rev_argv, &revs, NULL);
 	prepare_revision_walk(&revs);

but I'm not quite sure if this is the right place. Is it the
responsibility of the checkout-orphan-warning code to clean up after
itself, or is it the responsibility of a revision walker to clean up
before itself?

In the former case, things can be a bit tricky because you have to
remember the tips you started from to call clear_commit_marks(), which
means saving away the revs.pending list after setup_revisions but before
prepare_revision_walk. It might be worth having an alternate
clear_commit_marks() that didn't actually walk the tree but just cleared
the marks from every commit we've loaded, giving other walkers a total
clean slate.

If the latter case (clean up beforehand), should prepare_revision_walk
actually be clearing any existing marks?

I think I favor the prepare_revision_walk approach; in most cases
cleaning up afterwards is just a waste (since there usually isn't a
second walk). But I don't know if there is code that actually depends on
the intermediate results of a previous walk.

-Peff

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> but I'm not quite sure if this is the right place. Is it the
> responsibility of the checkout-orphan-warning code to clean up after
> itself, or is it the responsibility of a revision walker to clean up
> before itself?

Usually it is the former; the latter is generally impossible (unless it is
willing to clear everything), but the former knows where it started
traversal from.

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Jeff King]

On Sat, Mar 19, 2011 at 05:35:37PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > but I'm not quite sure if this is the right place. Is it the
> > responsibility of the checkout-orphan-warning code to clean up after
> > itself, or is it the responsibility of a revision walker to clean up
> > before itself?
> 
> Usually it is the former; the latter is generally impossible (unless it is
> willing to clear everything), but the former knows where it started
> traversal from.

For the case of 2 traversals, I suspect that clearing everything between
is not so different from clearing from the tips, since most everything
parsed was probably from the first traversal. But as we lib-ify more, we
may end up with more and more traversals in a single program, so it's
probably better to go the more efficient route from the beginning.

So how about this?

  [1/3]: checkout: add basic tests for detached-orphan warning
  [2/3]: checkout: clear commit marks after detached-orphan check
  [3/3]: checkout: tweak detached-orphan warning format

3/3 is only somewhat related, but I had been meaning to do it anyway. We
can break it off into a separate topic if there's a lot of discussion
around the 2nd patch.

-Peff

[PATCH 1/3] checkout: add basic tests for detached-orphan warning

[Jeff King]

Commit 8e2dc6ac added a warning when we leave a detached
HEAD whose commit is not reachable from any ref tip. Let's
add a few basic tests to make sure it works.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t2020-checkout-detach.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 0042145..bfeb2a6 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -11,6 +11,14 @@ check_not_detached () {
 	git symbolic-ref -q HEAD >/dev/null
 }
 
+ORPHAN_WARNING='you are leaving .* commit.*behind'
+check_orphan_warning() {
+	grep "$ORPHAN_WARNING" "$1"
+}
+check_no_orphan_warning() {
+	! grep "$ORPHAN_WARNING" "$1"
+}
+
 reset () {
 	git checkout master &&
 	check_not_detached
@@ -19,6 +27,8 @@ reset () {
 test_expect_success 'setup' '
 	test_commit one &&
 	test_commit two &&
+	test_commit three && git tag -d three &&
+	test_commit four && git tag -d four &&
 	git branch branch &&
 	git tag tag
 '
@@ -92,4 +102,28 @@ test_expect_success 'checkout --detach moves HEAD' '
 	git diff --exit-code two
 '
 
+test_expect_success 'checkout warns on orphan commits' '
+	reset &&
+	git checkout --detach two &&
+	echo content >orphan &&
+	git add orphan &&
+	git commit -a -m orphan &&
+	git checkout master 2>stderr &&
+	check_orphan_warning stderr
+'
+
+test_expect_success 'checkout does not warn leaving ref tip' '
+	reset &&
+	git checkout --detach two &&
+	git checkout master 2>stderr &&
+	check_no_orphan_warning stderr
+'
+
+test_expect_success 'checkout does not warn leaving reachable commit' '
+	reset &&
+	git checkout --detach HEAD^ &&
+	git checkout master 2>stderr &&
+	check_no_orphan_warning stderr
+'
+
 test_done
-- 
1.7.2.5.10.g62fe7

[PATCH 2/3] checkout: clear commit marks after detached-orphan check

[Jeff King]

When leaving a detached HEAD, we do a revision walk to make
sure the commit we are leaving isn't being orphaned.
However, this leaves crufty marks in the commit objects
which can confuse later walkers, like the one in
stat_tracking_info.

Let's clean up after ourselves to prevent this conflict.

Signed-off-by: Jeff King <peff@peff.net>
---
This uses for_each_ref to re-find the list of commits we touched in our
traversal, which feels a little hacky. prepare_revision_walk already
generates the exact list of tips, but unfortunately writes it
into revs->commits, which then gets munged by limit_list in the second
half of prepare_revision_walk. I wonder if it should keep a copy
elsewhere in revs, and then we could add:

  clear_all_commit_marks(&revs);

to let callers clean up after themselves easily.

I also just clear all marks; we could do just the ones that the revision
walker marks, but this seemed more future-proof to me than a set list of
marks.

 builtin/checkout.c         |   13 +++++++++++++
 t/t2020-checkout-detach.sh |   13 +++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2bf02f2..f88d2c8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -603,6 +603,16 @@ static int add_one_ref_to_rev_list_arg(const char *refname,
 	return 0;
 }
 
+static int clear_commit_marks_from_one_ref(const char *refname,
+				      const unsigned char *sha1,
+				      int flags,
+				      void *cb_data)
+{
+	struct commit *commit = lookup_commit_reference_gently(sha1, 1);
+	if (commit)
+		clear_commit_marks(commit, -1);
+	return 0;
+}
 
 static void describe_one_orphan(struct strbuf *sb, struct commit *commit)
 {
@@ -674,6 +684,9 @@ static void orphaned_commit_warning(struct commit *commit)
 		suggest_reattach(commit, &revs);
 	else
 		describe_detached_head("Previous HEAD position was", commit);
+
+	clear_commit_marks(commit, -1);
+	for_each_ref(clear_commit_marks_from_one_ref, NULL);
 }
 
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index bfeb2a6..569b27f 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '
 	check_no_orphan_warning stderr
 '
 
+cat >expect <<'EOF'
+Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
+EOF
+test_expect_success 'tracking count is accurate after orphan check' '
+	reset &&
+	git branch child master^ &&
+	git config branch.child.remote . &&
+	git config branch.child.merge refs/heads/master &&
+	git checkout child^ &&
+	git checkout child >stdout &&
+	test_cmp expect stdout
+'
+
 test_done
-- 
1.7.2.5.10.g62fe7

[PATCH 3/3] checkout: tweak detached-orphan warning format

[Jeff King]

When orphaning a commit on a detached HEAD, the warning
currently looks like:

  Warning: you are leaving 3 commits behind, not connected to
  any of your branches:

   - commit subject 1
   - commit subject 2
   - commit subject 3

  If you want to keep them by creating a new branch, this
  may be a good time to do so with:

     git branch new_branch_name 933a615ab0bc566dcfd8c01ec8af159f770d3fe5

Instead of using the "-" list, let's provide a more
traditional oneline format, with the abbreviated sha1 before
each subject. Users are accustomed to seeing commits in this
format, and having the sha1 of each commit can be useful if
you want to cherry-pick instead of creating a new branch.

The new format looks like:

  Warning: you are leaving 3 commits behind, not connected to
  any of your branches:

    933a615 commit subject 1
    824fcde commit subject 2
    fa49b1a commit subject 3

Signed-off-by: Jeff King <peff@peff.net>
---
Other thoughts I had but didn't do were:

  - abbreviate the sha1 in the example "git branch" command; it looks
    very long to me.

  - colorize the oneline list in the usual way. This helps makes the
    subjects stand out, but it's a little weird since the rest of the
    warning is not colorized at all.

  - an advice.detachedOrphan option. I'm not sure what it should look
    like exactly (just shorten the message, remove the orphan check
    entirely, etc) so I figured we'd wait until somebody actually is
    annoyed by the verbosity of the message and see what we would want
    then.

  - when we don't detect an orphan commit, we still print the old
    "Previous HEAD was..." message. The point of that was to mention the
    tip in case it was important. Where I think important could be one
    of:

      1. you are orphaning some commits

      2. you might want to remember how to get back to some interesting
         spot in history

    We've already checked that (1) is not the case. There is perhaps
    still some value to (2), but I don't know that I've ever used it.
    And you can always pull the answer from the HEAD reflog (technically
    you can do that for (1), too, of course, but I think the warning is
    more appropriate in that instance).

    So we could perhaps get rid of that message entirely, and print
    nothing when leaving a detached HEAD that is not being orphaned.

 builtin/checkout.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f88d2c8..686d0ff 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -619,7 +619,10 @@ static void describe_one_orphan(struct strbuf *sb, struct commit *commit)
 	struct pretty_print_context ctx = { 0 };
 
 	parse_commit(commit);
-	strbuf_addstr(sb, " - ");
+	strbuf_addstr(sb, "  ");
+	strbuf_addstr(sb,
+		find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+	strbuf_addch(sb, ' ');
 	pretty_print_commit(CMIT_FMT_ONELINE, commit, sb, &ctx);
 	strbuf_addch(sb, '\n');
 }
-- 
1.7.2.5.10.g62fe7

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For the case of 2 traversals, I suspect that clearing everything between
> is not so different from clearing from the tips, since most everything
> parsed was probably from the first traversal. But as we lib-ify more, we
> may end up with more and more traversals in a single program, so it's
> probably better to go the more efficient route from the beginning.

"Let the one that has more information to do the clearing" is not about
performance but about correctness and reusability potential.

When you insert a new traversal B in the existing codeflow before another
traversal C, B knows not just which commits it started from (hence knows
which commits were marked by it), but more importantly it also knows what
mark bits were potentially modified. If the existing codeflow had another
traversal A before you added B, and C took the marks A left on the objects
into account while doing its work, the only sensible clean-up is to clear
marks B touched (and no other marks) immediately after B, and we obviously
do not want C (and any later traversals in other codepaths that we may
later want to insert B) to have too much knowledge of which marks may have
been clobbered by B.

Of course, C could be coded defensively to clear marks other than what it
cares about (namely, the ones other than what A would have left and the
ones that would affect itself e.g. UNINTERESTING), and whoever inserts B
into existing codeflow needs to make sure that B does not stomp on marks
deliberately left by earlier traversals like A for later users like C
regardless of how the latter is coded.

> So how about this?
>
>   [1/3]: checkout: add basic tests for detached-orphan warning
>   [2/3]: checkout: clear commit marks after detached-orphan check
>   [3/3]: checkout: tweak detached-orphan warning format

Looks very sensible, except for the clear_marks(-1) that clears everything
I have a slight doubt about.

I think 3/3 was way overdue and it was my fault. Thanks for cleaning up my
mess.

Re: [PATCH 2/3] checkout: clear commit marks after detached-orphan check

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This uses for_each_ref to re-find the list of commits we touched in our
> traversal, which feels a little hacky. prepare_revision_walk already
> generates the exact list of tips, but unfortunately writes it
> into revs->commits, which then gets munged by limit_list in the second
> half of prepare_revision_walk. I wonder if it should keep a copy
> elsewhere in revs...

There is a precedence in merge-bases codepath that tries to be reusable by
doing its own clearing (C.f. get_merge_bases_many() and its use of cleanup
parameter). If that codepath becomes cleaner if prepare_revision_walk()
left an extra copy in revs structure, it would be a strong sign that it is
worth doing, I think.

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Jeff King]

On Sun, Mar 20, 2011 at 12:00:38PM -0700, Junio C Hamano wrote:

> "Let the one that has more information to do the clearing" is not about
> performance but about correctness and reusability potential.
> 
> When you insert a new traversal B in the existing codeflow before another
> traversal C, B knows not just which commits it started from (hence knows
> which commits were marked by it), but more importantly it also knows what
> mark bits were potentially modified. If the existing codeflow had another
> traversal A before you added B, and C took the marks A left on the objects
> into account while doing its work, the only sensible clean-up is to clear
> marks B touched (and no other marks) immediately after B, and we obviously
> do not want C (and any later traversals in other codepaths that we may
> later want to insert B) to have too much knowledge of which marks may have
> been clobbered by B.

I see what you are saying, but from a software engineering perspective,
I doubt it works very well in practice. The commit flags are basically
global variables. So imagine you have two traversals, A and C, and C
cares about some intermediate result from A. You want to add a new one,
B, in between the two, and have it clean up after itself. This is going
to work only if one of the two is the case:

  1. B does not use any of the same marks as A. Otherwise, it will end
     up clearing part of A's result during cleanup (not to mention that
     it may get a bogus result because of what was left from A).

  2. B works on a totally disjoint set of history from A and C.

I don't think (1) is that realistic in the general case. Sure, you might
only want to use TMP_MARK. But the revision walker behind the scenes is
using SEEN and UNINTERESTING, which is going to bring you into conflict.

For (2), there are cases where it works. In this recent bug, for
example, it would sometimes produce the correct results depending on the
exact traversal done in the orphan-warning (e.g., if you were exploring
way back in history near a tag, the traversal wouldn't come near the
commits needed to get the tracking info for the new branch).

But you can only know that it's a reasonable thing to do if you manually
analyze A, B, and C as a set, and even then only if you can know pretty
specifically what the inputs are. So there's no modularity, and in any
reasonably complex case you are not going to have enough control of the
inputs to be sure it isn't buggy.

So in my mind, the only way to keep sanity is to never insert a "B"
between an A and C who care about each other's results. And unless you
are specifically depending on a previous walk, each walk will want all
marks cleared (whether it's because the previous walker cleaned up, or
because we are clearing them before starting).


This whole global marks thing is really pretty nasty when you come right
down to it. I know why we do it; it's faster to dereference the pointer
rather than looking up the flags in some external per-revision-walker
hash table. But I wonder if we could come up with something else close
to it in speed that would allow per-walker flags.

We allocate commits from a custom allocator pool. I wonder if we could
use the offset from the commit pointer to the pool base to assign an
index to each commit, and then use that index to access an array of
flags in struct rev_info. There are a few problems:

  1. Our allocator has many pools, and we don't know which one is the
     right base. So I don't think the offset thing works, and we would
     be stuck assigning some index into each allocated object.

  2. We'd have to grow the rev_info array dynamically. The growth itself
     is probably not a huge deal, as its amortized constant effort. But
     on each flag access we'd have to do a bounds-check. I don't know if
     that would be measurable or not.

> > So how about this?
> >
> >   [1/3]: checkout: add basic tests for detached-orphan warning
> >   [2/3]: checkout: clear commit marks after detached-orphan check
> >   [3/3]: checkout: tweak detached-orphan warning format
> 
> Looks very sensible, except for the clear_marks(-1) that clears everything
> I have a slight doubt about.

I can change it to UNINTERESTING. As far as I can tell, that is the only
one set by prepare_revision_walk. It just felt like a leaky abstraction
for this code to have to know which flags might get set by the
underlying code. But obviously callers do need to know and care which
flags might be set and when.

-Peff

Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   1. B does not use any of the same marks as A. Otherwise, it will end
>      up clearing part of A's result during cleanup (not to mention that
>      it may get a bogus result because of what was left from A).
>
>   2. B works on a totally disjoint set of history from A and C.
>
> I don't think (1) is that realistic in the general case. Sure, you might
> only want to use TMP_MARK. But the revision walker behind the scenes is
> using SEEN and UNINTERESTING, which is going to bring you into conflict.

I suspect I didn't write it clearly enough, but I think we are saying the
same thing.

Of course SEEN/UNINTERESTING needs to be reset for any traversal.  That is
a given for anybody who uses get_revision() to traverse the history.  "B
knows more about the bit it uses than other people, so it should be able
to do a better job of cleaning after it than others" is exactly about (1).

> For (2), there are cases where it works. In this recent bug, for
> example, it would sometimes produce the correct results depending on the
> exact traversal done in the orphan-warning (e.g., if you were exploring
> way back in history near a tag, the traversal wouldn't come near the
> commits needed to get the tracking info for the new branch).

And then "B knows more about where in the history it travesed, so it
should be able to do a better job" is about (2).

> But you can only know that it's a reasonable thing to do if you manually
> analyze A, B, and C as a set, and even then only if you can know pretty
> specifically what the inputs are.

Oh, that is a given that whoever is inserting B between A and C needs to
know about their relationship, and whoever wishes to design B to be
reusable in other different contexts needs to know what masks other people
may care about not to stomp on them.  I had this idea of allocating masks
on-demand instead of the current static assignment in the back of my head
for a few years, but my gut feeling keeps me telling that the added
complexity (and lax use of object bits by coders leading to memory
footprint bloat) may not be worth it.

> I can change it to UNINTERESTING. As far as I can tell, that is the only
> one set by prepare_revision_walk. It just felt like a leaky abstraction
> for this code to have to know which flags might get set by the
> underlying code. But obviously callers do need to know and care which
> flags might be set and when.

Yes, the code has to know about the bits, and explicitly declaring it
knows what it is doing and using is about being assuring, not being leaky.

[PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jonathan Nieder]

When v1.7.5-rc0~19^2~1 (checkout: clear commit marks after detached-orphan
check, 2011-03-20) added a check that the human-readable message

	Your branch is behind 'master' by 1 commit, and can be fast-forwarded.

is not suppressed by a previous commit walk, it forgot that the
message might be different when git is configured to write output in
another language.  Skip the output check in that case.

With this patch applied, the test passes with GETTEXT_POISON=true again.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Jeff King wrote:

> When leaving a detached HEAD, we do a revision walk to make
> sure the commit we are leaving isn't being orphaned.
> However, this leaves crufty marks in the commit objects
> which can confuse later walkers, like the one in
> stat_tracking_info.
>
> Let's clean up after ourselves to prevent this conflict.

Very nice thing to do.  Thanks.

[...]
> --- a/t/t2020-checkout-detach.sh
> +++ b/t/t2020-checkout-detach.sh
> @@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '
>  	check_no_orphan_warning stderr
>  '
>
> +cat >expect <<'EOF'
> +Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
> +EOF
> +test_expect_success 'tracking count is accurate after orphan check' '
> +	reset &&
> +	git branch child master^ &&
> +	git config branch.child.remote . &&
> +	git config branch.child.merge refs/heads/master &&
> +	git checkout child^ &&
> +	git checkout child >stdout &&
> +	test_cmp expect stdout

Should use test_i18ncmp to handle people who force tests to run in
other locales (like the fake GETTEXT_POISON locale).  Quick patch
follows.

 t/t2020-checkout-detach.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 068fba4c..b37ce25c 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -148,7 +148,7 @@ test_expect_success 'tracking count is accurate after orphan check' '
 	git config branch.child.merge refs/heads/master &&
 	git checkout child^ &&
 	git checkout child >stdout &&
-	test_cmp expect stdout
+	test_i18ncmp expect stdout
 '
 
 test_done
-- 
1.7.10

[PATCH/RFC] checkout --detached test: write supporting files before start of tests

[Jonathan Nieder]

As a general rule, git's tests use the following layout:

 - first, setting the --help description and including test-lib
   and other libraries

 - next, writing static files and setting variables that will last
   through the entire script, and defining helper functions

 - next, the test assertions themselves

This way it is visually obvious where the code for each test assertion
begins and ends and there is no temptation to use command substitution
to do nontrivial work outside of the test_expect_success / failure
blocks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
> Jeff King wrote:

>> --- a/t/t2020-checkout-detach.sh
>> +++ b/t/t2020-checkout-detach.sh
>> @@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '
>>  	check_no_orphan_warning stderr
>>  '
>>
>> +cat >expect <<'EOF'
>> +Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
>> +EOF
>> +test_expect_success 'tracking count is accurate after orphan check' '
>> +	reset &&
>> +	git branch child master^ &&
>> +	git config branch.child.remote . &&
>> +	git config branch.child.merge refs/heads/master &&
>> +	git checkout child^ &&
>> +	git checkout child >stdout &&
>> +	test_cmp expect stdout
>
> Should use test_i18ncmp to handle people who force tests to run in
> other locales (like the fake GETTEXT_POISON locale).  Quick patch
> follows.

... and here's a patch on top to address an unrelated nit.

 t/t2020-checkout-detach.sh |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index b37ce25c..c2c34664 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -3,6 +3,10 @@
 test_description='checkout into detached HEAD state'
 . ./test-lib.sh
 
+cat >master-1-ahead.message <<'EOF'
+Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
+EOF
+
 check_detached () {
 	test_must_fail git symbolic-ref -q HEAD >/dev/null
 }
@@ -138,10 +142,8 @@ test_expect_success 'checkout does not warn leaving reachable commit' '
 	check_no_orphan_warning stderr
 '
 
-cat >expect <<'EOF'
-Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
-EOF
 test_expect_success 'tracking count is accurate after orphan check' '
+	cp master-1-ahead.message expect &&
 	reset &&
 	git branch child master^ &&
 	git config branch.child.remote . &&
-- 
1.7.10

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jeff King]

On Fri, Apr 13, 2012 at 05:59:01PM -0500, Jonathan Nieder wrote:

> When v1.7.5-rc0~19^2~1 (checkout: clear commit marks after detached-orphan
> check, 2011-03-20) added a check that the human-readable message
> 
> 	Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
> 
> is not suppressed by a previous commit walk, it forgot that the
> message might be different when git is configured to write output in
> another language.  Skip the output check in that case.

I think it is not "forgot" but "predates" in this case. The commit
introducing the problem is 8a5b749 (i18n: format_tracking_info "Your
branch is behind" message, 2012-02-02). But obviously your fix is
correct either way.

> ---
> Hi,
> 
> Jeff King wrote:
> 
> > When leaving a detached HEAD, we do a revision walk to make
> > sure the commit we are leaving isn't being orphaned.
> > However, this leaves crufty marks in the commit objects
> > which can confuse later walkers, like the one in
> > stat_tracking_info.
> >
> > Let's clean up after ourselves to prevent this conflict.
> 
> Very nice thing to do.  Thanks.

A minor complaint, but the format of your email left me confused for
several minutes, as I didn't remember writing that or working in this
area recently. It turns out that it is because this commit was from over
a year ago.

-Peff

Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests

[Jeff King]

On Fri, Apr 13, 2012 at 06:25:35PM -0500, Jonathan Nieder wrote:

> As a general rule, git's tests use the following layout:
> 
>  - first, setting the --help description and including test-lib
>    and other libraries
> 
>  - next, writing static files and setting variables that will last
>    through the entire script, and defining helper functions
> 
>  - next, the test assertions themselves
> 
> This way it is visually obvious where the code for each test assertion
> begins and ends and there is no temptation to use command substitution
> to do nontrivial work outside of the test_expect_success / failure
> blocks.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

I agree with that general rule, although:

> -cat >expect <<'EOF'
> -Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
> -EOF
>  test_expect_success 'tracking count is accurate after orphan check' '
> +	cp master-1-ahead.message expect &&
>  	reset &&
>  	git branch child master^ &&
>  	git config branch.child.remote . &&

it is quote common to keep expected output closer to its test, and this
expectation is only useful for this one test.  If anything, should this
not be moving the cat inside the test_expect_success? Or is there some
follow-on patch I am missing that is also going to use the message?

-Peff

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jonathan Nieder]

Jeff King wrote:

> I think it is not "forgot" but "predates" in this case. The commit
> introducing the problem is 8a5b749 (i18n: format_tracking_info "Your
> branch is behind" message, 2012-02-02). But obviously your fix is
> correct either way.

Oh, that makes sense.  I wonder why we didn't notice this before.
GETTEXT_POISON support hit "master" in 2011-05-23.

>> Jeff King wrote:

>>> When leaving a detached HEAD, we do a revision walk to make
>>> sure the commit we are leaving isn't being orphaned.
>>> However, this leaves crufty marks in the commit objects
>>> which can confuse later walkers, like the one in
>>> stat_tracking_info.
>>>
>>> Let's clean up after ourselves to prevent this conflict.
>>
>> Very nice thing to do.  Thanks.
>
> A minor complaint, but the format of your email left me confused for
> several minutes, as I didn't remember writing that or working in this
> area recently. It turns out that it is because this commit was from over
> a year ago.

Yeah, I should have paid attention to the date.  A better diagnosis
would be

	When v1.7.9.2~28^2 (2012-02-02) marked the "Your branch is behind
	message for translation, it forgot to adjust tests to stop checking
	for that message when tests are being run with git configured to write
	its output in another language.

	With this patch applied, tests pass with GETTEXT_POISON=YesPlease
	again.

	Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
	Explained-by: Jeff King <peff@peff.net>

Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests

[Jonathan Nieder]

Jeff King wrote:

>                                                If anything, should this
> not be moving the cat inside the test_expect_success?

That would be fine with me.  It would involve changing the ' around
"master" to '\'' and would mean that if some later patch wants to use
the same message, the author will have to remember to factor it out.

On the other hand, nothing about the message is specific to that test
assertion, so I am ok with the patch I sent, too.

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jeff King]

On Fri, Apr 13, 2012 at 06:46:07PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > I think it is not "forgot" but "predates" in this case. The commit
> > introducing the problem is 8a5b749 (i18n: format_tracking_info "Your
> > branch is behind" message, 2012-02-02). But obviously your fix is
> > correct either way.
> 
> Oh, that makes sense.  I wonder why we didn't notice this before.
> GETTEXT_POISON support hit "master" in 2011-05-23.

Yes, but this wasn't gettext-ed at all until 2012-02-02, and then it
didn't get merged into master until the week after. How often do you
run GETTEXT_POISON tests? I know I don't, and obviously Junio does not
include them as part of his process before pushing out master.

> Yeah, I should have paid attention to the date.  A better diagnosis
> would be
> 
> 	When v1.7.9.2~28^2 (2012-02-02) marked the "Your branch is behind
> 	message for translation, it forgot to adjust tests to stop checking
> 	for that message when tests are being run with git configured to write
> 	its output in another language.
> 
> 	With this patch applied, tests pass with GETTEXT_POISON=YesPlease
> 	again.
> 
> 	Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> 	Explained-by: Jeff King <peff@peff.net>

Yeah, that makes sense to me. It seems like t6040 is broken under
GETTEXT_POISON by that commit, too (and is still broken). Probably your
patch should deal with both breakages, as their root cause is the same.

-Peff

Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests

[Jeff King]

On Fri, Apr 13, 2012 at 06:49:49PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> >                                                If anything, should this
> > not be moving the cat inside the test_expect_success?
> 
> That would be fine with me.  It would involve changing the ' around
> "master" to '\'' and would mean that if some later patch wants to use
> the same message, the author will have to remember to factor it out.

Ah, yeah, that is probably why I left it outside the test in the first
place.

> On the other hand, nothing about the message is specific to that test
> assertion, so I am ok with the patch I sent, too.

I don't care much either way. I just know that there are a million other
places that set up "expect" right before the test, so it seems like a
common exception to our rule.

-Peff

[PATCH v2 0/3] Re: i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jonathan Nieder]

Jeff King wrote:

> Yeah, that makes sense to me. It seems like t6040 is broken under
> GETTEXT_POISON by that commit, too (and is still broken). Probably your
> patch should deal with both breakages, as their root cause is the same.

Yep.

Here's a series doing that.  Patch 3 is a bonus patch unrelated to the
cause: it fixes a test buglet noticed while working on the other two.

Jonathan Nieder (3):
  test: do not rely on US English tracking-info messages
  test: use test_i18ncmp for "Patch format detection failed" message
  test: am of empty patch should not succeed

 t/t2020-checkout-detach.sh |    2 +-
 t/t4150-am.sh              |    4 ++--
 t/t6040-tracking-info.sh   |   10 +++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)

[PATCH 1/3] test: do not rely on US English tracking-info messages

[Jonathan Nieder]

When v1.7.9.2~28^2 (2012-02-02) marked "Your branch is behind" and
friends for translation, it forgot to adjust tests not to check those
messages when tests are being run with git configured to write its
output in another language.

With this patch applied, t2020 and t6040 pass again with
GETTEXT_POISON=YesPlease.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Explained-by: Jeff King <peff@peff.net>
---
 t/t2020-checkout-detach.sh |    2 +-
 t/t6040-tracking-info.sh   |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 068fba4c..b37ce25c 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -148,7 +148,7 @@ test_expect_success 'tracking count is accurate after orphan check' '
 	git config branch.child.merge refs/heads/master &&
 	git checkout child^ &&
 	git checkout child >stdout &&
-	test_cmp expect stdout
+	test_i18ncmp expect stdout
 '
 
 test_done
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 19272bc5..ec2b516c 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -71,13 +71,13 @@ test_expect_success 'checkout' '
 	(
 		cd test && git checkout b1
 	) >actual &&
-	grep "have 1 and 1 different" actual
+	test_i18ngrep "have 1 and 1 different" actual
 '
 
 test_expect_success 'checkout with local tracked branch' '
 	git checkout master &&
 	git checkout follower >actual &&
-	grep "is ahead of" actual
+	test_i18ngrep "is ahead of" actual
 '
 
 test_expect_success 'status' '
@@ -87,14 +87,14 @@ test_expect_success 'status' '
 		# reports nothing to commit
 		test_must_fail git commit --dry-run
 	) >actual &&
-	grep "have 1 and 1 different" actual
+	test_i18ngrep "have 1 and 1 different" actual
 '
 
 test_expect_success 'fail to track lightweight tags' '
 	git checkout master &&
 	git tag light &&
 	test_must_fail git branch --track lighttrack light >actual &&
-	test_must_fail grep "set up to track" actual &&
+	test_i18ngrep ! "set up to track" actual &&
 	test_must_fail git checkout lighttrack
 '
 
@@ -102,7 +102,7 @@ test_expect_success 'fail to track annotated tags' '
 	git checkout master &&
 	git tag -m heavy heavy &&
 	test_must_fail git branch --track heavytrack heavy >actual &&
-	test_must_fail grep "set up to track" actual &&
+	test_i18ngrep ! "set up to track" actual &&
 	test_must_fail git checkout heavytrack
 '
 
-- 
1.7.10

[PATCH 2/3] test: use test_i18ncmp for "Patch format detection failed" message

[Jonathan Nieder]

v1.7.8.5~2 (am: don't infloop for an empty input file, 2012-02-25)
added a check for the human-readable message "Patch format detection
failed." but we forgot to suppress that check when running tests with
git configured to write output in another language.

Noticed by running tests with GETTEXT_POISON=YesPlease.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4150-am.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index ccc0280f..ebb4a26a 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -527,7 +527,7 @@ test_expect_success 'am empty-file does not infloop' '
 	test_tick &&
 	{ git am empty-file > actual 2>&1 && false || :; } &&
 	echo Patch format detection failed. >expected &&
-	test_cmp expected actual
+	test_i18ncmp expected actual
 '
 
 test_done
-- 
1.7.10

[PATCH 3/3] test: am of empty patch should not succeed

[Jonathan Nieder]

The "git am empty" test uses the construct

	git am empty-file && false || :

which unconditionally returns true.  Use test_must_fail instead, which
also has the benefit of noticing if "git am" has segfaulted.

While at it, tighten the test to check that the diagnostic appears on
stderr and not stdout.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Tested this time.  Thanks for reading.

 t/t4150-am.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index ebb4a26a..cdafd7e7 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -525,7 +525,7 @@ test_expect_success 'am empty-file does not infloop' '
 	git reset --hard &&
 	touch empty-file &&
 	test_tick &&
-	{ git am empty-file > actual 2>&1 && false || :; } &&
+	test_must_fail git am empty-file 2>actual &&
 	echo Patch format detection failed. >expected &&
 	test_i18ncmp expected actual
 '
-- 
1.7.10

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jonathan Nieder]

Jeff King wrote:

> Yes, but this wasn't gettext-ed at all until 2012-02-02, and then it
> didn't get merged into master until the week after. How often do you
> run GETTEXT_POISON tests? I know I don't, and obviously Junio does not
> include them as part of his process before pushing out master.

The theory is that it should be convenient to run them when we are
considering a "gettextize" patch.

Maybe something like the following would make it easier for some
people to always build with GETTEXT_POISON and run tests with
GIT_GETTEXT_POISON only occasionally.  I'd rather have a real poison
locale since this would not require any runtime support in the git
binary, though.  Does the value of LC_MESSAGES have to be a valid
locale?  Would something like en_US@poison work?

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git i/t/test-lib.sh w/t/test-lib.sh
index b7d7100c..83f7362a 100644
--- i/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -51,6 +51,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(perl -e '
 	my $ok = join("|", qw(
 		TRACE
 		DEBUG
+		GETTEXT_POISON
 		USE_LOOKUP
 		TEST
 		.*_TEST
@@ -621,10 +622,8 @@ test -n "$USE_LIBPCRE" && test_set_prereq LIBPCRE
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
 
 # Can we rely on git's output in the C locale?
-if test -n "$GETTEXT_POISON"
+if test -n "$GIT_GETTEXT_POISON"
 then
-	GIT_GETTEXT_POISON=YesPlease
-	export GIT_GETTEXT_POISON
 	test_set_prereq GETTEXT_POISON
 else
 	test_set_prereq C_LOCALE_OUTPUT
@@ -635,7 +634,7 @@ fi
 # under GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ncmp () {
-	test -n "$GETTEXT_POISON" || test_cmp "$@"
+	test -n "$GIT_GETTEXT_POISON" || test_cmp "$@"
 }
 
 # Use this instead of "grep expected-string actual" to see if the
@@ -644,7 +643,7 @@ test_i18ncmp () {
 # under GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ngrep () {
-	if test -n "$GETTEXT_POISON"
+	if test -n "$GIT_GETTEXT_POISON"
 	then
 	    : # pretend success
 	elif test "x!" = "x$1"

Re: [PATCH v2 0/3] Re: i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jeff King]

On Fri, Apr 13, 2012 at 11:44:13PM -0500, Jonathan Nieder wrote:

> Here's a series doing that.  Patch 3 is a bonus patch unrelated to the
> cause: it fixes a test buglet noticed while working on the other two.
> 
> Jonathan Nieder (3):
>   test: do not rely on US English tracking-info messages
>   test: use test_i18ncmp for "Patch format detection failed" message
>   test: am of empty patch should not succeed

Thanks, all 3 look reasonable to me (the code you are replacing in the
third one is sufficiently confusing-looking that I wondered if something
else was going on, but I can't see anything, and the original thread has
no more discussion).

-Peff

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jeff King]

On Sat, Apr 14, 2012 at 12:02:34AM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Yes, but this wasn't gettext-ed at all until 2012-02-02, and then it
> > didn't get merged into master until the week after. How often do you
> > run GETTEXT_POISON tests? I know I don't, and obviously Junio does not
> > include them as part of his process before pushing out master.
> 
> The theory is that it should be convenient to run them when we are
> considering a "gettextize" patch.

I was thinking when I wrote the above that it might be something worth
running on every test run. But it's really not. It really only matters
if you are gettextizing a string (or introducing a new string). And we
should generally catch that in code review, I would think.

So it's still a good thing to run once in a while, and when there is a
big gettext patch, but it's probably overkill to run it all the time.

> Maybe something like the following would make it easier for some
> people to always build with GETTEXT_POISON and run tests with
> GIT_GETTEXT_POISON only occasionally.

I found "make GETTEXT_POISON=1 test" to be sufficiently easy (and then
my next "make" will un-poison.

> I'd rather have a real poison locale since this would not require any
> runtime support in the git binary, though.  Does the value of
> LC_MESSAGES have to be a valid locale?  Would something like
> en_US@poison work?

That would nice. Poisoning seems like it should be a property
of the test run rather than the build. It seems like putting junk into
LC_MESSAGES will just cause the default messages to be shown (seems like
a sane thing to do), so I suspect you'd have to actually make a poison
locale (though you could probably generate one via script without too
much effort).

-Peff

Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)

[Jonathan Nieder]

Jeff King wrote:

>                         I suspect you'd have to actually make a poison
> locale (though you could probably generate one via script without too
> much effort).

Yes, that's the easy part.  The hard part is convincing gettext to use
it.