{"thread":{"id":"26798","subject":"[bug] git checkout lies about number of ahead commits when switching from detached HEAD","startedAt":"2011-03-19T21:53:13Z","lastAt":"2012-04-14T12:47:17Z","messageCount":28,"participants":["Piotr Krukowiecki","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"163783","messageId":"4D8525C9.2060203@gmail.com","threadId":"26798","inReplyTo":null,"subject":"[bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2011-03-19T21:53:13Z","receivedAt":"2011-03-19T21:53:13Z","isPatch":false,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"It says \"by 0 commits\" when going back from detached HEAD to\nmaster branch:\n\n\n$ git checkout HEAD^\n\n$ git checkout master\nPrevious HEAD position was af4c62a... Merge branch 'maint'\nSwitched to branch 'master'\nYour branch is ahead of 'origin/master' by 0 commits.\n ^^^^^^^^^^^^\n\n$ git status\n# On branch master\n# Your branch is ahead of 'origin/master' by 1 commit.\n ^^^^^^^^^^^\n\n#\nnothing to commit (working directory clean)\n\n\n-- \nPiotr Krukowiecki\n"},{"id":"163787","messageId":"20110319222852.GB7116@sigill.intra.peff.net","threadId":"26798","inReplyTo":"4D8525C9.2060203@gmail.com","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-19T22:28:52Z","receivedAt":"2011-03-19T22:28:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 19, 2011 at 10:53:13PM +0100, Piotr Krukowiecki wrote:\n\n> It says \"by 0 commits\" when going back from detached HEAD to\n> master branch:\n> \n> \n> $ git checkout HEAD^\n> \n> $ git checkout master\n> Previous HEAD position was af4c62a... Merge branch 'maint'\n> Switched to branch 'master'\n> Your branch is ahead of 'origin/master' by 0 commits.\n> ^^^^^^^^^^^^\n> \n> $ git status\n> # On branch master\n> # Your branch is ahead of 'origin/master' by 1 commit.\n> ^^^^^^^^^^^\n\nHmm. My guess is that the new \"check for connectivity when leaving\ndetached HEAD\" test is polluting the commit flags for the ahead/behind\ntest.\n\n[bisect bisect bisect]\n\nYep, it bisects to 8e2dc6a (commit: give final warning when reattaching\nHEAD to leave commits behind, 2011-02-18). We probably need to clean the\nuninteresting flags between the two traversals.\n\n-Peff\n"},{"id":"163788","messageId":"20110319224726.GC7116@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20110319222852.GB7116@sigill.intra.peff.net","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-19T22:47:26Z","receivedAt":"2011-03-19T22:47:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 19, 2011 at 06:28:52PM -0400, Jeff King wrote:\n\n> On Sat, Mar 19, 2011 at 10:53:13PM +0100, Piotr Krukowiecki wrote:\n> \n> > It says \"by 0 commits\" when going back from detached HEAD to\n> > master branch:\n> > \n> > \n> > $ git checkout HEAD^\n> > \n> > $ git checkout master\n> > Previous HEAD position was af4c62a... Merge branch 'maint'\n> > Switched to branch 'master'\n> > Your branch is ahead of 'origin/master' by 0 commits.\n> > ^^^^^^^^^^^^\n> > \n> > $ git status\n> > # On branch master\n> > # Your branch is ahead of 'origin/master' by 1 commit.\n> > ^^^^^^^^^^^\n> \n> Hmm. My guess is that the new \"check for connectivity when leaving\n> detached HEAD\" test is polluting the commit flags for the ahead/behind\n> test.\n> \n> [bisect bisect bisect]\n> \n> Yep, it bisects to 8e2dc6a (commit: give final warning when reattaching\n> HEAD to leave commits behind, 2011-02-18). We probably need to clean the\n> uninteresting flags between the two traversals.\n\nThis patch fixes it:\n\ndiff --git a/remote.c b/remote.c\nindex ca42a12..92b7428 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1560,6 +1560,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \tstrcpy(symmetric + 40, \"...\");\n \tstrcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));\n \n+\tclear_commit_marks(ours, -1);\n+\tclear_commit_marks(theirs, -1);\n \tinit_revisions(&revs, NULL);\n \tsetup_revisions(rev_argc, rev_argv, &revs, NULL);\n \tprepare_revision_walk(&revs);\n\nbut I'm not quite sure if this is the right place. Is it the\nresponsibility of the checkout-orphan-warning code to clean up after\nitself, or is it the responsibility of a revision walker to clean up\nbefore itself?\n\nIn the former case, things can be a bit tricky because you have to\nremember the tips you started from to call clear_commit_marks(), which\nmeans saving away the revs.pending list after setup_revisions but before\nprepare_revision_walk. It might be worth having an alternate\nclear_commit_marks() that didn't actually walk the tree but just cleared\nthe marks from every commit we've loaded, giving other walkers a total\nclean slate.\n\nIf the latter case (clean up beforehand), should prepare_revision_walk\nactually be clearing any existing marks?\n\nI think I favor the prepare_revision_walk approach; in most cases\ncleaning up afterwards is just a waste (since there usually isn't a\nsecond walk). But I don't know if there is code that actually depends on\nthe intermediate results of a previous walk.\n\n-Peff\n"},{"id":"163795","messageId":"7vy64avdba.fsf@alter.siamese.dyndns.org","threadId":"26798","inReplyTo":"20110319224726.GC7116@sigill.intra.peff.net","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-20T00:35:37Z","receivedAt":"2011-03-20T00:35:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> but I'm not quite sure if this is the right place. Is it the\n> responsibility of the checkout-orphan-warning code to clean up after\n> itself, or is it the responsibility of a revision walker to clean up\n> before itself?\n\nUsually it is the former; the latter is generally impossible (unless it is\nwilling to clear everything), but the former knows where it started\ntraversal from.\n"},{"id":"163807","messageId":"20110320090111.GA15641@sigill.intra.peff.net","threadId":"26798","inReplyTo":"7vy64avdba.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-20T09:01:11Z","receivedAt":"2011-03-20T09:01:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 19, 2011 at 05:35:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > but I'm not quite sure if this is the right place. Is it the\n> > responsibility of the checkout-orphan-warning code to clean up after\n> > itself, or is it the responsibility of a revision walker to clean up\n> > before itself?\n> \n> Usually it is the former; the latter is generally impossible (unless it is\n> willing to clear everything), but the former knows where it started\n> traversal from.\n\nFor the case of 2 traversals, I suspect that clearing everything between\nis not so different from clearing from the tips, since most everything\nparsed was probably from the first traversal. But as we lib-ify more, we\nmay end up with more and more traversals in a single program, so it's\nprobably better to go the more efficient route from the beginning.\n\nSo how about this?\n\n [1/3]: checkout: add basic tests for detached-orphan warning\n [2/3]: checkout: clear commit marks after detached-orphan check\n [3/3]: checkout: tweak detached-orphan warning format\n\n3/3 is only somewhat related, but I had been meaning to do it anyway. We\ncan break it off into a separate topic if there's a lot of discussion\naround the 2nd patch.\n\n-Peff\n"},{"id":"163808","messageId":"20110320090416.GA15948@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20110320090111.GA15641@sigill.intra.peff.net","subject":"[PATCH 1/3] checkout: add basic tests for detached-orphan warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-20T09:04:16Z","receivedAt":"2011-03-20T09:04:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 8e2dc6ac added a warning when we leave a detached\nHEAD whose commit is not reachable from any ref tip. Let's\nadd a few basic tests to make sure it works.\n\nSigned-off-by: Jeff King \n---\n t/t2020-checkout-detach.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 files changed, 34 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 0042145..bfeb2a6 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -11,6 +11,14 @@ check_not_detached () {\n \tgit symbolic-ref -q HEAD >/dev/null\n }\n \n+ORPHAN_WARNING='you are leaving .* commit.*behind'\n+check_orphan_warning() {\n+\tgrep \"$ORPHAN_WARNING\" \"$1\"\n+}\n+check_no_orphan_warning() {\n+\t! grep \"$ORPHAN_WARNING\" \"$1\"\n+}\n+\n reset () {\n \tgit checkout master &&\n \tcheck_not_detached\n@@ -19,6 +27,8 @@ reset () {\n test_expect_success 'setup' '\n \ttest_commit one &&\n \ttest_commit two &&\n+\ttest_commit three && git tag -d three &&\n+\ttest_commit four && git tag -d four &&\n \tgit branch branch &&\n \tgit tag tag\n '\n@@ -92,4 +102,28 @@ test_expect_success 'checkout --detach moves HEAD' '\n \tgit diff --exit-code two\n '\n \n+test_expect_success 'checkout warns on orphan commits' '\n+\treset &&\n+\tgit checkout --detach two &&\n+\techo content >orphan &&\n+\tgit add orphan &&\n+\tgit commit -a -m orphan &&\n+\tgit checkout master 2>stderr &&\n+\tcheck_orphan_warning stderr\n+'\n+\n+test_expect_success 'checkout does not warn leaving ref tip' '\n+\treset &&\n+\tgit checkout --detach two &&\n+\tgit checkout master 2>stderr &&\n+\tcheck_no_orphan_warning stderr\n+'\n+\n+test_expect_success 'checkout does not warn leaving reachable commit' '\n+\treset &&\n+\tgit checkout --detach HEAD^ &&\n+\tgit checkout master 2>stderr &&\n+\tcheck_no_orphan_warning stderr\n+'\n+\n test_done\n-- \n1.7.2.5.10.g62fe7\n"},{"id":"163809","messageId":"20110320090918.GB15948@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20110320090111.GA15641@sigill.intra.peff.net","subject":"[PATCH 2/3] checkout: clear commit marks after detached-orphan check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-20T09:09:18Z","receivedAt":"2011-03-20T09:09:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When leaving a detached HEAD, we do a revision walk to make\nsure the commit we are leaving isn't being orphaned.\nHowever, this leaves crufty marks in the commit objects\nwhich can confuse later walkers, like the one in\nstat_tracking_info.\n\nLet's clean up after ourselves to prevent this conflict.\n\nSigned-off-by: Jeff King \n---\nThis uses for_each_ref to re-find the list of commits we touched in our\ntraversal, which feels a little hacky. prepare_revision_walk already\ngenerates the exact list of tips, but unfortunately writes it\ninto revs->commits, which then gets munged by limit_list in the second\nhalf of prepare_revision_walk. I wonder if it should keep a copy\nelsewhere in revs, and then we could add:\n\n clear_all_commit_marks(&revs);\n\nto let callers clean up after themselves easily.\n\nI also just clear all marks; we could do just the ones that the revision\nwalker marks, but this seemed more future-proof to me than a set list of\nmarks.\n\n builtin/checkout.c | 13 +++++++++++++\n t/t2020-checkout-detach.sh | 13 +++++++++++++\n 2 files changed, 26 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2bf02f2..f88d2c8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -603,6 +603,16 @@ static int add_one_ref_to_rev_list_arg(const char *refname,\n \treturn 0;\n }\n \n+static int clear_commit_marks_from_one_ref(const char *refname,\n+\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct commit *commit = lookup_commit_reference_gently(sha1, 1);\n+\tif (commit)\n+\t\tclear_commit_marks(commit, -1);\n+\treturn 0;\n+}\n \n static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n {\n@@ -674,6 +684,9 @@ static void orphaned_commit_warning(struct commit *commit)\n \t\tsuggest_reattach(commit, &revs);\n \telse\n \t\tdescribe_detached_head(\"Previous HEAD position was\", commit);\n+\n+\tclear_commit_marks(commit, -1);\n+\tfor_each_ref(clear_commit_marks_from_one_ref, NULL);\n }\n \n static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex bfeb2a6..569b27f 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '\n \tcheck_no_orphan_warning stderr\n '\n \n+cat >expect <<'EOF'\n+Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n+EOF\n+test_expect_success 'tracking count is accurate after orphan check' '\n+\treset &&\n+\tgit branch child master^ &&\n+\tgit config branch.child.remote . &&\n+\tgit config branch.child.merge refs/heads/master &&\n+\tgit checkout child^ &&\n+\tgit checkout child >stdout &&\n+\ttest_cmp expect stdout\n+'\n+\n test_done\n-- \n1.7.2.5.10.g62fe7\n"},{"id":"163811","messageId":"20110320091900.GC15948@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20110320090111.GA15641@sigill.intra.peff.net","subject":"[PATCH 3/3] checkout: tweak detached-orphan warning format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-20T09:19:01Z","receivedAt":"2011-03-20T09:19:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When orphaning a commit on a detached HEAD, the warning\ncurrently looks like:\n\n Warning: you are leaving 3 commits behind, not connected to\n any of your branches:\n\n - commit subject 1\n - commit subject 2\n - commit subject 3\n\n If you want to keep them by creating a new branch, this\n may be a good time to do so with:\n\n git branch new_branch_name 933a615ab0bc566dcfd8c01ec8af159f770d3fe5\n\nInstead of using the \"-\" list, let's provide a more\ntraditional oneline format, with the abbreviated sha1 before\neach subject. Users are accustomed to seeing commits in this\nformat, and having the sha1 of each commit can be useful if\nyou want to cherry-pick instead of creating a new branch.\n\nThe new format looks like:\n\n Warning: you are leaving 3 commits behind, not connected to\n any of your branches:\n\n 933a615 commit subject 1\n 824fcde commit subject 2\n fa49b1a commit subject 3\n\nSigned-off-by: Jeff King \n---\nOther thoughts I had but didn't do were:\n\n - abbreviate the sha1 in the example \"git branch\" command; it looks\n very long to me.\n\n - colorize the oneline list in the usual way. This helps makes the\n subjects stand out, but it's a little weird since the rest of the\n warning is not colorized at all.\n\n - an advice.detachedOrphan option. I'm not sure what it should look\n like exactly (just shorten the message, remove the orphan check\n entirely, etc) so I figured we'd wait until somebody actually is\n annoyed by the verbosity of the message and see what we would want\n then.\n\n - when we don't detect an orphan commit, we still print the old\n \"Previous HEAD was...\" message. The point of that was to mention the\n tip in case it was important. Where I think important could be one\n of:\n\n 1. you are orphaning some commits\n\n 2. you might want to remember how to get back to some interesting\n spot in history\n\n We've already checked that (1) is not the case. There is perhaps\n still some value to (2), but I don't know that I've ever used it.\n And you can always pull the answer from the HEAD reflog (technically\n you can do that for (1), too, of course, but I think the warning is\n more appropriate in that instance).\n\n So we could perhaps get rid of that message entirely, and print\n nothing when leaving a detached HEAD that is not being orphaned.\n\n builtin/checkout.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f88d2c8..686d0ff 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -619,7 +619,10 @@ static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n \tstruct pretty_print_context ctx = { 0 };\n \n \tparse_commit(commit);\n-\tstrbuf_addstr(sb, \" - \");\n+\tstrbuf_addstr(sb, \" \");\n+\tstrbuf_addstr(sb,\n+\t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));\n+\tstrbuf_addch(sb, ' ');\n \tpretty_print_commit(CMIT_FMT_ONELINE, commit, sb, &ctx);\n \tstrbuf_addch(sb, '\\n');\n }\n-- \n1.7.2.5.10.g62fe7\n"},{"id":"163832","messageId":"7vd3llwrah.fsf@alter.siamese.dyndns.org","threadId":"26798","inReplyTo":"20110320090111.GA15641@sigill.intra.peff.net","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-20T19:00:38Z","receivedAt":"2011-03-20T19:00:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For the case of 2 traversals, I suspect that clearing everything between\n> is not so different from clearing from the tips, since most everything\n> parsed was probably from the first traversal. But as we lib-ify more, we\n> may end up with more and more traversals in a single program, so it's\n> probably better to go the more efficient route from the beginning.\n\n\"Let the one that has more information to do the clearing\" is not about\nperformance but about correctness and reusability potential.\n\nWhen you insert a new traversal B in the existing codeflow before another\ntraversal C, B knows not just which commits it started from (hence knows\nwhich commits were marked by it), but more importantly it also knows what\nmark bits were potentially modified. If the existing codeflow had another\ntraversal A before you added B, and C took the marks A left on the objects\ninto account while doing its work, the only sensible clean-up is to clear\nmarks B touched (and no other marks) immediately after B, and we obviously\ndo not want C (and any later traversals in other codepaths that we may\nlater want to insert B) to have too much knowledge of which marks may have\nbeen clobbered by B.\n\nOf course, C could be coded defensively to clear marks other than what it\ncares about (namely, the ones other than what A would have left and the\nones that would affect itself e.g. UNINTERESTING), and whoever inserts B\ninto existing codeflow needs to make sure that B does not stomp on marks\ndeliberately left by earlier traversals like A for later users like C\nregardless of how the latter is coded.\n\n> So how about this?\n>\n> [1/3]: checkout: add basic tests for detached-orphan warning\n> [2/3]: checkout: clear commit marks after detached-orphan check\n> [3/3]: checkout: tweak detached-orphan warning format\n\nLooks very sensible, except for the clear_marks(-1) that clears everything\nI have a slight doubt about.\n\nI think 3/3 was way overdue and it was my fault. Thanks for cleaning up my\nmess.\n"},{"id":"163833","messageId":"7v8vw9wr2i.fsf@alter.siamese.dyndns.org","threadId":"26798","inReplyTo":"20110320090918.GB15948@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] checkout: clear commit marks after detached-orphan check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-20T19:05:25Z","receivedAt":"2011-03-20T19:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This uses for_each_ref to re-find the list of commits we touched in our\n> traversal, which feels a little hacky. prepare_revision_walk already\n> generates the exact list of tips, but unfortunately writes it\n> into revs->commits, which then gets munged by limit_list in the second\n> half of prepare_revision_walk. I wonder if it should keep a copy\n> elsewhere in revs...\n\nThere is a precedence in merge-bases codepath that tries to be reusable by\ndoing its own clearing (C.f. get_merge_bases_many() and its use of cleanup\nparameter). If that codepath becomes cleaner if prepare_revision_walk()\nleft an extra copy in revs structure, it would be a strong sign that it is\nworth doing, I think.\n"},{"id":"163892","messageId":"20110321103549.GA16334@sigill.intra.peff.net","threadId":"26798","inReplyTo":"7vd3llwrah.fsf@alter.siamese.dyndns.org","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-21T10:35:49Z","receivedAt":"2011-03-21T10:35:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 20, 2011 at 12:00:38PM -0700, Junio C Hamano wrote:\n\n> \"Let the one that has more information to do the clearing\" is not about\n> performance but about correctness and reusability potential.\n> \n> When you insert a new traversal B in the existing codeflow before another\n> traversal C, B knows not just which commits it started from (hence knows\n> which commits were marked by it), but more importantly it also knows what\n> mark bits were potentially modified. If the existing codeflow had another\n> traversal A before you added B, and C took the marks A left on the objects\n> into account while doing its work, the only sensible clean-up is to clear\n> marks B touched (and no other marks) immediately after B, and we obviously\n> do not want C (and any later traversals in other codepaths that we may\n> later want to insert B) to have too much knowledge of which marks may have\n> been clobbered by B.\n\nI see what you are saying, but from a software engineering perspective,\nI doubt it works very well in practice. The commit flags are basically\nglobal variables. So imagine you have two traversals, A and C, and C\ncares about some intermediate result from A. You want to add a new one,\nB, in between the two, and have it clean up after itself. This is going\nto work only if one of the two is the case:\n\n 1. B does not use any of the same marks as A. Otherwise, it will end\n up clearing part of A's result during cleanup (not to mention that\n it may get a bogus result because of what was left from A).\n\n 2. B works on a totally disjoint set of history from A and C.\n\nI don't think (1) is that realistic in the general case. Sure, you might\nonly want to use TMP_MARK. But the revision walker behind the scenes is\nusing SEEN and UNINTERESTING, which is going to bring you into conflict.\n\nFor (2), there are cases where it works. In this recent bug, for\nexample, it would sometimes produce the correct results depending on the\nexact traversal done in the orphan-warning (e.g., if you were exploring\nway back in history near a tag, the traversal wouldn't come near the\ncommits needed to get the tracking info for the new branch).\n\nBut you can only know that it's a reasonable thing to do if you manually\nanalyze A, B, and C as a set, and even then only if you can know pretty\nspecifically what the inputs are. So there's no modularity, and in any\nreasonably complex case you are not going to have enough control of the\ninputs to be sure it isn't buggy.\n\nSo in my mind, the only way to keep sanity is to never insert a \"B\"\nbetween an A and C who care about each other's results. And unless you\nare specifically depending on a previous walk, each walk will want all\nmarks cleared (whether it's because the previous walker cleaned up, or\nbecause we are clearing them before starting).\n\n\nThis whole global marks thing is really pretty nasty when you come right\ndown to it. I know why we do it; it's faster to dereference the pointer\nrather than looking up the flags in some external per-revision-walker\nhash table. But I wonder if we could come up with something else close\nto it in speed that would allow per-walker flags.\n\nWe allocate commits from a custom allocator pool. I wonder if we could\nuse the offset from the commit pointer to the pool base to assign an\nindex to each commit, and then use that index to access an array of\nflags in struct rev_info. There are a few problems:\n\n 1. Our allocator has many pools, and we don't know which one is the\n right base. So I don't think the offset thing works, and we would\n be stuck assigning some index into each allocated object.\n\n 2. We'd have to grow the rev_info array dynamically. The growth itself\n is probably not a huge deal, as its amortized constant effort. But\n on each flag access we'd have to do a bounds-check. I don't know if\n that would be measurable or not.\n\n> > So how about this?\n> >\n> > [1/3]: checkout: add basic tests for detached-orphan warning\n> > [2/3]: checkout: clear commit marks after detached-orphan check\n> > [3/3]: checkout: tweak detached-orphan warning format\n> \n> Looks very sensible, except for the clear_marks(-1) that clears everything\n> I have a slight doubt about.\n\nI can change it to UNINTERESTING. As far as I can tell, that is the only\none set by prepare_revision_walk. It just felt like a leaky abstraction\nfor this code to have to know which flags might get set by the\nunderlying code. But obviously callers do need to know and care which\nflags might be set and when.\n\n-Peff\n"},{"id":"163913","messageId":"7v4o6wtsdz.fsf@alter.siamese.dyndns.org","threadId":"26798","inReplyTo":"20110321103549.GA16334@sigill.intra.peff.net","subject":"Re: [bug] git checkout lies about number of ahead commits when switching from detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-21T15:17:28Z","receivedAt":"2011-03-21T15:17:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. B does not use any of the same marks as A. Otherwise, it will end\n> up clearing part of A's result during cleanup (not to mention that\n> it may get a bogus result because of what was left from A).\n>\n> 2. B works on a totally disjoint set of history from A and C.\n>\n> I don't think (1) is that realistic in the general case. Sure, you might\n> only want to use TMP_MARK. But the revision walker behind the scenes is\n> using SEEN and UNINTERESTING, which is going to bring you into conflict.\n\nI suspect I didn't write it clearly enough, but I think we are saying the\nsame thing.\n\nOf course SEEN/UNINTERESTING needs to be reset for any traversal. That is\na given for anybody who uses get_revision() to traverse the history. \"B\nknows more about the bit it uses than other people, so it should be able\nto do a better job of cleaning after it than others\" is exactly about (1).\n\n> For (2), there are cases where it works. In this recent bug, for\n> example, it would sometimes produce the correct results depending on the\n> exact traversal done in the orphan-warning (e.g., if you were exploring\n> way back in history near a tag, the traversal wouldn't come near the\n> commits needed to get the tracking info for the new branch).\n\nAnd then \"B knows more about where in the history it travesed, so it\nshould be able to do a better job\" is about (2).\n\n> But you can only know that it's a reasonable thing to do if you manually\n> analyze A, B, and C as a set, and even then only if you can know pretty\n> specifically what the inputs are.\n\nOh, that is a given that whoever is inserting B between A and C needs to\nknow about their relationship, and whoever wishes to design B to be\nreusable in other different contexts needs to know what masks other people\nmay care about not to stomp on them. I had this idea of allocating masks\non-demand instead of the current static assignment in the back of my head\nfor a few years, but my gut feeling keeps me telling that the added\ncomplexity (and lax use of object bits by coders leading to memory\nfootprint bloat) may not be worth it.\n\n> I can change it to UNINTERESTING. As far as I can tell, that is the only\n> one set by prepare_revision_walk. It just felt like a leaky abstraction\n> for this code to have to know which flags might get set by the\n> underlying code. But obviously callers do need to know and care which\n> flags might be set and when.\n\nYes, the code has to know about the bits, and explicitly declaring it\nknows what it is doing and using is about being assuring, not being leaky.\n"},{"id":"189224","messageId":"20120413225901.GA13220@burratino","threadId":"26798","inReplyTo":"20110320090918.GB15948@sigill.intra.peff.net","subject":"[PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-13T22:59:01Z","receivedAt":"2012-04-13T22:59:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When v1.7.5-rc0~19^2~1 (checkout: clear commit marks after detached-orphan\ncheck, 2011-03-20) added a check that the human-readable message\n\n\tYour branch is behind 'master' by 1 commit, and can be fast-forwarded.\n\nis not suppressed by a previous commit walk, it forgot that the\nmessage might be different when git is configured to write output in\nanother language. Skip the output check in that case.\n\nWith this patch applied, the test passes with GETTEXT_POISON=true again.\n\nSigned-off-by: Jonathan Nieder \n---\nHi,\n\nJeff King wrote:\n\n> When leaving a detached HEAD, we do a revision walk to make\n> sure the commit we are leaving isn't being orphaned.\n> However, this leaves crufty marks in the commit objects\n> which can confuse later walkers, like the one in\n> stat_tracking_info.\n>\n> Let's clean up after ourselves to prevent this conflict.\n\nVery nice thing to do. Thanks.\n\n[...]\n> --- a/t/t2020-checkout-detach.sh\n> +++ b/t/t2020-checkout-detach.sh\n> @@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '\n> \tcheck_no_orphan_warning stderr\n> '\n>\n> +cat >expect <<'EOF'\n> +Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n> +EOF\n> +test_expect_success 'tracking count is accurate after orphan check' '\n> +\treset &&\n> +\tgit branch child master^ &&\n> +\tgit config branch.child.remote . &&\n> +\tgit config branch.child.merge refs/heads/master &&\n> +\tgit checkout child^ &&\n> +\tgit checkout child >stdout &&\n> +\ttest_cmp expect stdout\n\nShould use test_i18ncmp to handle people who force tests to run in\nother locales (like the fake GETTEXT_POISON locale). Quick patch\nfollows.\n\n t/t2020-checkout-detach.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 068fba4c..b37ce25c 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -148,7 +148,7 @@ test_expect_success 'tracking count is accurate after orphan check' '\n \tgit config branch.child.merge refs/heads/master &&\n \tgit checkout child^ &&\n \tgit checkout child >stdout &&\n-\ttest_cmp expect stdout\n+\ttest_i18ncmp expect stdout\n '\n \n test_done\n-- \n1.7.10\n"},{"id":"189225","messageId":"20120413232535.GB13995@burratino","threadId":"26798","inReplyTo":"20120413225901.GA13220@burratino","subject":"[PATCH/RFC] checkout --detached test: write supporting files before start of tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-13T23:25:35Z","receivedAt":"2012-04-13T23:25:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"As a general rule, git's tests use the following layout:\n\n - first, setting the --help description and including test-lib\n and other libraries\n\n - next, writing static files and setting variables that will last\n through the entire script, and defining helper functions\n\n - next, the test assertions themselves\n\nThis way it is visually obvious where the code for each test assertion\nbegins and ends and there is no temptation to use command substitution\nto do nontrivial work outside of the test_expect_success / failure\nblocks.\n\nSigned-off-by: Jonathan Nieder \n---\nJonathan Nieder wrote:\n> Jeff King wrote:\n\n>> --- a/t/t2020-checkout-detach.sh\n>> +++ b/t/t2020-checkout-detach.sh\n>> @@ -126,4 +126,17 @@ test_expect_success 'checkout does not warn leaving reachable commit' '\n>> \tcheck_no_orphan_warning stderr\n>> '\n>>\n>> +cat >expect <<'EOF'\n>> +Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n>> +EOF\n>> +test_expect_success 'tracking count is accurate after orphan check' '\n>> +\treset &&\n>> +\tgit branch child master^ &&\n>> +\tgit config branch.child.remote . &&\n>> +\tgit config branch.child.merge refs/heads/master &&\n>> +\tgit checkout child^ &&\n>> +\tgit checkout child >stdout &&\n>> +\ttest_cmp expect stdout\n>\n> Should use test_i18ncmp to handle people who force tests to run in\n> other locales (like the fake GETTEXT_POISON locale). Quick patch\n> follows.\n\n... and here's a patch on top to address an unrelated nit.\n\n t/t2020-checkout-detach.sh | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex b37ce25c..c2c34664 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -3,6 +3,10 @@\n test_description='checkout into detached HEAD state'\n . ./test-lib.sh\n \n+cat >master-1-ahead.message <<'EOF'\n+Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n+EOF\n+\n check_detached () {\n \ttest_must_fail git symbolic-ref -q HEAD >/dev/null\n }\n@@ -138,10 +142,8 @@ test_expect_success 'checkout does not warn leaving reachable commit' '\n \tcheck_no_orphan_warning stderr\n '\n \n-cat >expect <<'EOF'\n-Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n-EOF\n test_expect_success 'tracking count is accurate after orphan check' '\n+\tcp master-1-ahead.message expect &&\n \treset &&\n \tgit branch child master^ &&\n \tgit config branch.child.remote . &&\n-- \n1.7.10\n"},{"id":"189226","messageId":"20120413233010.GA16663@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120413225901.GA13220@burratino","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T23:30:10Z","receivedAt":"2012-04-13T23:30:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 05:59:01PM -0500, Jonathan Nieder wrote:\n\n> When v1.7.5-rc0~19^2~1 (checkout: clear commit marks after detached-orphan\n> check, 2011-03-20) added a check that the human-readable message\n> \n> \tYour branch is behind 'master' by 1 commit, and can be fast-forwarded.\n> \n> is not suppressed by a previous commit walk, it forgot that the\n> message might be different when git is configured to write output in\n> another language. Skip the output check in that case.\n\nI think it is not \"forgot\" but \"predates\" in this case. The commit\nintroducing the problem is 8a5b749 (i18n: format_tracking_info \"Your\nbranch is behind\" message, 2012-02-02). But obviously your fix is\ncorrect either way.\n\n> ---\n> Hi,\n> \n> Jeff King wrote:\n> \n> > When leaving a detached HEAD, we do a revision walk to make\n> > sure the commit we are leaving isn't being orphaned.\n> > However, this leaves crufty marks in the commit objects\n> > which can confuse later walkers, like the one in\n> > stat_tracking_info.\n> >\n> > Let's clean up after ourselves to prevent this conflict.\n> \n> Very nice thing to do. Thanks.\n\nA minor complaint, but the format of your email left me confused for\nseveral minutes, as I didn't remember writing that or working in this\narea recently. It turns out that it is because this commit was from over\na year ago.\n\n-Peff\n"},{"id":"189227","messageId":"20120413233346.GB16663@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120413232535.GB13995@burratino","subject":"Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T23:33:46Z","receivedAt":"2012-04-13T23:33:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 06:25:35PM -0500, Jonathan Nieder wrote:\n\n> As a general rule, git's tests use the following layout:\n> \n> - first, setting the --help description and including test-lib\n> and other libraries\n> \n> - next, writing static files and setting variables that will last\n> through the entire script, and defining helper functions\n> \n> - next, the test assertions themselves\n> \n> This way it is visually obvious where the code for each test assertion\n> begins and ends and there is no temptation to use command substitution\n> to do nontrivial work outside of the test_expect_success / failure\n> blocks.\n> \n> Signed-off-by: Jonathan Nieder \n\nI agree with that general rule, although:\n\n> -cat >expect <<'EOF'\n> -Your branch is behind 'master' by 1 commit, and can be fast-forwarded.\n> -EOF\n> test_expect_success 'tracking count is accurate after orphan check' '\n> +\tcp master-1-ahead.message expect &&\n> \treset &&\n> \tgit branch child master^ &&\n> \tgit config branch.child.remote . &&\n\nit is quote common to keep expected output closer to its test, and this\nexpectation is only useful for this one test. If anything, should this\nnot be moving the cat inside the test_expect_success? Or is there some\nfollow-on patch I am missing that is also going to use the message?\n\n-Peff\n"},{"id":"189231","messageId":"20120413234607.GE13995@burratino","threadId":"26798","inReplyTo":"20120413233010.GA16663@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-13T23:46:07Z","receivedAt":"2012-04-13T23:46:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I think it is not \"forgot\" but \"predates\" in this case. The commit\n> introducing the problem is 8a5b749 (i18n: format_tracking_info \"Your\n> branch is behind\" message, 2012-02-02). But obviously your fix is\n> correct either way.\n\nOh, that makes sense. I wonder why we didn't notice this before.\nGETTEXT_POISON support hit \"master\" in 2011-05-23.\n\n>> Jeff King wrote:\n\n>>> When leaving a detached HEAD, we do a revision walk to make\n>>> sure the commit we are leaving isn't being orphaned.\n>>> However, this leaves crufty marks in the commit objects\n>>> which can confuse later walkers, like the one in\n>>> stat_tracking_info.\n>>>\n>>> Let's clean up after ourselves to prevent this conflict.\n>>\n>> Very nice thing to do. Thanks.\n>\n> A minor complaint, but the format of your email left me confused for\n> several minutes, as I didn't remember writing that or working in this\n> area recently. It turns out that it is because this commit was from over\n> a year ago.\n\nYeah, I should have paid attention to the date. A better diagnosis\nwould be\n\n\tWhen v1.7.9.2~28^2 (2012-02-02) marked the \"Your branch is behind\n\tmessage for translation, it forgot to adjust tests to stop checking\n\tfor that message when tests are being run with git configured to write\n\tits output in another language.\n\n\tWith this patch applied, tests pass with GETTEXT_POISON=YesPlease\n\tagain.\n\n\tSigned-off-by: Jonathan Nieder \n\tExplained-by: Jeff King \n"},{"id":"189232","messageId":"20120413234949.GF13995@burratino","threadId":"26798","inReplyTo":"20120413233346.GB16663@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-13T23:49:49Z","receivedAt":"2012-04-13T23:49:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> If anything, should this\n> not be moving the cat inside the test_expect_success?\n\nThat would be fine with me. It would involve changing the ' around\n\"master\" to '\\'' and would mean that if some later patch wants to use\nthe same message, the author will have to remember to factor it out.\n\nOn the other hand, nothing about the message is specific to that test\nassertion, so I am ok with the patch I sent, too.\n"},{"id":"189235","messageId":"20120414022452.GA17535@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120413234607.GE13995@burratino","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-14T02:24:52Z","receivedAt":"2012-04-14T02:24:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 06:46:07PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I think it is not \"forgot\" but \"predates\" in this case. The commit\n> > introducing the problem is 8a5b749 (i18n: format_tracking_info \"Your\n> > branch is behind\" message, 2012-02-02). But obviously your fix is\n> > correct either way.\n> \n> Oh, that makes sense. I wonder why we didn't notice this before.\n> GETTEXT_POISON support hit \"master\" in 2011-05-23.\n\nYes, but this wasn't gettext-ed at all until 2012-02-02, and then it\ndidn't get merged into master until the week after. How often do you\nrun GETTEXT_POISON tests? I know I don't, and obviously Junio does not\ninclude them as part of his process before pushing out master.\n\n> Yeah, I should have paid attention to the date. A better diagnosis\n> would be\n> \n> \tWhen v1.7.9.2~28^2 (2012-02-02) marked the \"Your branch is behind\n> \tmessage for translation, it forgot to adjust tests to stop checking\n> \tfor that message when tests are being run with git configured to write\n> \tits output in another language.\n> \n> \tWith this patch applied, tests pass with GETTEXT_POISON=YesPlease\n> \tagain.\n> \n> \tSigned-off-by: Jonathan Nieder \n> \tExplained-by: Jeff King \n\nYeah, that makes sense to me. It seems like t6040 is broken under\nGETTEXT_POISON by that commit, too (and is still broken). Probably your\npatch should deal with both breakages, as their root cause is the same.\n\n-Peff\n"},{"id":"189236","messageId":"20120414022604.GB17535@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120413234949.GF13995@burratino","subject":"Re: [PATCH/RFC] checkout --detached test: write supporting files before start of tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-14T02:26:04Z","receivedAt":"2012-04-14T02:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 06:49:49PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > If anything, should this\n> > not be moving the cat inside the test_expect_success?\n> \n> That would be fine with me. It would involve changing the ' around\n> \"master\" to '\\'' and would mean that if some later patch wants to use\n> the same message, the author will have to remember to factor it out.\n\nAh, yeah, that is probably why I left it outside the test in the first\nplace.\n\n> On the other hand, nothing about the message is specific to that test\n> assertion, so I am ok with the patch I sent, too.\n\nI don't care much either way. I just know that there are a million other\nplaces that set up \"expect\" right before the test, so it seems like a\ncommon exception to our rule.\n\n-Peff\n"},{"id":"189237","messageId":"20120414044413.GA1791@burratino","threadId":"26798","inReplyTo":"20120414022452.GA17535@sigill.intra.peff.net","subject":"[PATCH v2 0/3] Re: i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T04:44:13Z","receivedAt":"2012-04-14T04:44:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Yeah, that makes sense to me. It seems like t6040 is broken under\n> GETTEXT_POISON by that commit, too (and is still broken). Probably your\n> patch should deal with both breakages, as their root cause is the same.\n\nYep.\n\nHere's a series doing that. Patch 3 is a bonus patch unrelated to the\ncause: it fixes a test buglet noticed while working on the other two.\n\nJonathan Nieder (3):\n test: do not rely on US English tracking-info messages\n test: use test_i18ncmp for \"Patch format detection failed\" message\n test: am of empty patch should not succeed\n\n t/t2020-checkout-detach.sh | 2 +-\n t/t4150-am.sh | 4 ++--\n t/t6040-tracking-info.sh | 10 +++++-----\n 3 files changed, 8 insertions(+), 8 deletions(-)\n"},{"id":"189238","messageId":"20120414044531.GB1791@burratino","threadId":"26798","inReplyTo":"20120414044413.GA1791@burratino","subject":"[PATCH 1/3] test: do not rely on US English tracking-info messages","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T04:45:31Z","receivedAt":"2012-04-14T04:45:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When v1.7.9.2~28^2 (2012-02-02) marked \"Your branch is behind\" and\nfriends for translation, it forgot to adjust tests not to check those\nmessages when tests are being run with git configured to write its\noutput in another language.\n\nWith this patch applied, t2020 and t6040 pass again with\nGETTEXT_POISON=YesPlease.\n\nSigned-off-by: Jonathan Nieder \nExplained-by: Jeff King \n---\n t/t2020-checkout-detach.sh | 2 +-\n t/t6040-tracking-info.sh | 10 +++++-----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 068fba4c..b37ce25c 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -148,7 +148,7 @@ test_expect_success 'tracking count is accurate after orphan check' '\n \tgit config branch.child.merge refs/heads/master &&\n \tgit checkout child^ &&\n \tgit checkout child >stdout &&\n-\ttest_cmp expect stdout\n+\ttest_i18ncmp expect stdout\n '\n \n test_done\ndiff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh\nindex 19272bc5..ec2b516c 100755\n--- a/t/t6040-tracking-info.sh\n+++ b/t/t6040-tracking-info.sh\n@@ -71,13 +71,13 @@ test_expect_success 'checkout' '\n \t(\n \t\tcd test && git checkout b1\n \t) >actual &&\n-\tgrep \"have 1 and 1 different\" actual\n+\ttest_i18ngrep \"have 1 and 1 different\" actual\n '\n \n test_expect_success 'checkout with local tracked branch' '\n \tgit checkout master &&\n \tgit checkout follower >actual &&\n-\tgrep \"is ahead of\" actual\n+\ttest_i18ngrep \"is ahead of\" actual\n '\n \n test_expect_success 'status' '\n@@ -87,14 +87,14 @@ test_expect_success 'status' '\n \t\t# reports nothing to commit\n \t\ttest_must_fail git commit --dry-run\n \t) >actual &&\n-\tgrep \"have 1 and 1 different\" actual\n+\ttest_i18ngrep \"have 1 and 1 different\" actual\n '\n \n test_expect_success 'fail to track lightweight tags' '\n \tgit checkout master &&\n \tgit tag light &&\n \ttest_must_fail git branch --track lighttrack light >actual &&\n-\ttest_must_fail grep \"set up to track\" actual &&\n+\ttest_i18ngrep ! \"set up to track\" actual &&\n \ttest_must_fail git checkout lighttrack\n '\n \n@@ -102,7 +102,7 @@ test_expect_success 'fail to track annotated tags' '\n \tgit checkout master &&\n \tgit tag -m heavy heavy &&\n \ttest_must_fail git branch --track heavytrack heavy >actual &&\n-\ttest_must_fail grep \"set up to track\" actual &&\n+\ttest_i18ngrep ! \"set up to track\" actual &&\n \ttest_must_fail git checkout heavytrack\n '\n \n-- \n1.7.10\n"},{"id":"189239","messageId":"20120414044618.GC1791@burratino","threadId":"26798","inReplyTo":"20120414044413.GA1791@burratino","subject":"[PATCH 2/3] test: use test_i18ncmp for \"Patch format detection failed\" message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T04:46:18Z","receivedAt":"2012-04-14T04:46:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"v1.7.8.5~2 (am: don't infloop for an empty input file, 2012-02-25)\nadded a check for the human-readable message \"Patch format detection\nfailed.\" but we forgot to suppress that check when running tests with\ngit configured to write output in another language.\n\nNoticed by running tests with GETTEXT_POISON=YesPlease.\n\nSigned-off-by: Jonathan Nieder \n---\n t/t4150-am.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex ccc0280f..ebb4a26a 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -527,7 +527,7 @@ test_expect_success 'am empty-file does not infloop' '\n \ttest_tick &&\n \t{ git am empty-file > actual 2>&1 && false || :; } &&\n \techo Patch format detection failed. >expected &&\n-\ttest_cmp expected actual\n+\ttest_i18ncmp expected actual\n '\n \n test_done\n-- \n1.7.10\n"},{"id":"189240","messageId":"20120414044813.GD1791@burratino","threadId":"26798","inReplyTo":"20120414044413.GA1791@burratino","subject":"[PATCH 3/3] test: am of empty patch should not succeed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T04:48:13Z","receivedAt":"2012-04-14T04:48:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The \"git am empty\" test uses the construct\n\n\tgit am empty-file && false || :\n\nwhich unconditionally returns true. Use test_must_fail instead, which\nalso has the benefit of noticing if \"git am\" has segfaulted.\n\nWhile at it, tighten the test to check that the diagnostic appears on\nstderr and not stdout.\n\nSigned-off-by: Jonathan Nieder \n---\nTested this time. Thanks for reading.\n\n t/t4150-am.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex ebb4a26a..cdafd7e7 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -525,7 +525,7 @@ test_expect_success 'am empty-file does not infloop' '\n \tgit reset --hard &&\n \ttouch empty-file &&\n \ttest_tick &&\n-\t{ git am empty-file > actual 2>&1 && false || :; } &&\n+\ttest_must_fail git am empty-file 2>actual &&\n \techo Patch format detection failed. >expected &&\n \ttest_i18ncmp expected actual\n '\n-- \n1.7.10\n"},{"id":"189242","messageId":"20120414050234.GE1791@burratino","threadId":"26798","inReplyTo":"20120414022452.GA17535@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T05:02:34Z","receivedAt":"2012-04-14T05:02:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Yes, but this wasn't gettext-ed at all until 2012-02-02, and then it\n> didn't get merged into master until the week after. How often do you\n> run GETTEXT_POISON tests? I know I don't, and obviously Junio does not\n> include them as part of his process before pushing out master.\n\nThe theory is that it should be convenient to run them when we are\nconsidering a \"gettextize\" patch.\n\nMaybe something like the following would make it easier for some\npeople to always build with GETTEXT_POISON and run tests with\nGIT_GETTEXT_POISON only occasionally. I'd rather have a real poison\nlocale since this would not require any runtime support in the git\nbinary, though. Does the value of LC_MESSAGES have to be a valid\nlocale? Would something like en_US@poison work?\n\nSigned-off-by: Jonathan Nieder \n---\n t/test-lib.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git i/t/test-lib.sh w/t/test-lib.sh\nindex b7d7100c..83f7362a 100644\n--- i/t/test-lib.sh\n+++ w/t/test-lib.sh\n@@ -51,6 +51,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(perl -e '\n \tmy $ok = join(\"|\", qw(\n \t\tTRACE\n \t\tDEBUG\n+\t\tGETTEXT_POISON\n \t\tUSE_LOOKUP\n \t\tTEST\n \t\t.*_TEST\n@@ -621,10 +622,8 @@ test -n \"$USE_LIBPCRE\" && test_set_prereq LIBPCRE\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n+if test -n \"$GIT_GETTEXT_POISON\"\n then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n \ttest_set_prereq GETTEXT_POISON\n else\n \ttest_set_prereq C_LOCALE_OUTPUT\n@@ -635,7 +634,7 @@ fi\n # under GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\ttest -n \"$GIT_GETTEXT_POISON\" || test_cmp \"$@\"\n }\n \n # Use this instead of \"grep expected-string actual\" to see if the\n@@ -644,7 +643,7 @@ test_i18ncmp () {\n # under GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n-\tif test -n \"$GETTEXT_POISON\"\n+\tif test -n \"$GIT_GETTEXT_POISON\"\n \tthen\n \t : # pretend success\n \telif test \"x!\" = \"x$1\"\n"},{"id":"189251","messageId":"20120414081551.GB11124@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120414044413.GA1791@burratino","subject":"Re: [PATCH v2 0/3] Re: i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-14T08:15:51Z","receivedAt":"2012-04-14T08:15:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 11:44:13PM -0500, Jonathan Nieder wrote:\n\n> Here's a series doing that. Patch 3 is a bonus patch unrelated to the\n> cause: it fixes a test buglet noticed while working on the other two.\n> \n> Jonathan Nieder (3):\n> test: do not rely on US English tracking-info messages\n> test: use test_i18ncmp for \"Patch format detection failed\" message\n> test: am of empty patch should not succeed\n\nThanks, all 3 look reasonable to me (the code you are replacing in the\nthird one is sufficiently confusing-looking that I wondered if something\nelse was going on, but I can't see anything, and the original thread has\nno more discussion).\n\n-Peff\n"},{"id":"189252","messageId":"20120414082213.GC11124@sigill.intra.peff.net","threadId":"26798","inReplyTo":"20120414050234.GE1791@burratino","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-14T08:22:14Z","receivedAt":"2012-04-14T08:22:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 14, 2012 at 12:02:34AM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Yes, but this wasn't gettext-ed at all until 2012-02-02, and then it\n> > didn't get merged into master until the week after. How often do you\n> > run GETTEXT_POISON tests? I know I don't, and obviously Junio does not\n> > include them as part of his process before pushing out master.\n> \n> The theory is that it should be convenient to run them when we are\n> considering a \"gettextize\" patch.\n\nI was thinking when I wrote the above that it might be something worth\nrunning on every test run. But it's really not. It really only matters\nif you are gettextizing a string (or introducing a new string). And we\nshould generally catch that in code review, I would think.\n\nSo it's still a good thing to run once in a while, and when there is a\nbig gettext patch, but it's probably overkill to run it all the time.\n\n> Maybe something like the following would make it easier for some\n> people to always build with GETTEXT_POISON and run tests with\n> GIT_GETTEXT_POISON only occasionally.\n\nI found \"make GETTEXT_POISON=1 test\" to be sufficiently easy (and then\nmy next \"make\" will un-poison.\n\n> I'd rather have a real poison locale since this would not require any\n> runtime support in the git binary, though. Does the value of\n> LC_MESSAGES have to be a valid locale? Would something like\n> en_US@poison work?\n\nThat would nice. Poisoning seems like it should be a property\nof the test run rather than the build. It seems like putting junk into\nLC_MESSAGES will just cause the default messages to be shown (seems like\na sane thing to do), so I suspect you'd have to actually make a poison\nlocale (though you could probably generate one via script without too\nmuch effort).\n\n-Peff\n"},{"id":"189259","messageId":"20120414124716.GB20076@burratino","threadId":"26798","inReplyTo":"20120414082213.GC11124@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: use test_i18ncmp in t2020 (checkout --detach)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-14T12:47:17Z","receivedAt":"2012-04-14T12:47:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I suspect you'd have to actually make a poison\n> locale (though you could probably generate one via script without too\n> much effort).\n\nYes, that's the easy part. The hard part is convincing gettext to use\nit.\n"}]}