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

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

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

This patch fixes it:

index ca42a12..92b7428 100644
@@ -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

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.

---
 t/t2020-checkout-detach.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

index 0042145..bfeb2a6 100755
@@ -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.

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

index 2bf02f2..f88d2c8 100644
@@ -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)

index bfeb2a6..569b27f 100755
@@ -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

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.

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.

---
Hi,

Very nice thing to do. Thanks.

[...]

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

index 068fba4c..b37ce25c 100755
@@ -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.

---

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

index b37ce25c..c2c34664 100755
@@ -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

I agree with that general rule, although:

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.

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

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

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.

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

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

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.

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

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.

---
 t/t2020-checkout-detach.sh |    2 +-
 t/t6040-tracking-info.sh   |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

index 068fba4c..b37ce25c 100755
@@ -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

index 19272bc5..ec2b516c 100755
@@ -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.

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

index ccc0280f..ebb4a26a 100755
@@ -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.

---
Tested this time. Thanks for reading.

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

index ebb4a26a..cdafd7e7 100755
@@ -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

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

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?

---
 t/test-lib.sh |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index b7d7100c..83f7362a 100644
@@ -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"

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.

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

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

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

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

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

index f88d2c8..686d0ff 100644
@@ -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

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

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.

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.

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

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

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

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.

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.