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

Jeff King [off-list ref] 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.

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

Jeff King [off-list ref] writes:

quoted

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

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

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

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

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

[...]

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

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 <redacted>
---
Jonathan Nieder wrote:

Jeff King wrote:

quoted

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

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:

quoted

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

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

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

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.

quoted

Jeff King wrote:

quoted

quoted

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 [off-list ref]
	Explained-by: Jeff King [off-list ref]

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.

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

Jeff King wrote:

quoted

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 [off-list ref]
	Explained-by: Jeff King [off-list ref]

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

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

Jeff King wrote:

quoted

                                               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

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

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 <redacted>
Explained-by: Jeff King [off-list ref]
---
 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

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

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

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

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

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

Jeff King wrote:

quoted

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

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.