Linus Torvalds <torvalds@linux-foundation.org> writes:

quoted text
> So I think the real problem here is not that the logic is wrong in 
> general, but that there is one *special* case where the logic to break out 
> is wrong.
>
> And that special case is when we hit the root commit which isn't negative.
>
> That case is special because *normally*, if we have a positive commit, we 
> will always continue to walk the parents of that positive commit, so the 
> "everybody_interesting()" check will not trigger. BUT! If we hit a root 
> commit and it is positive, that won't happen (since, by definition, it has 
> no parents to keep the list populated with), and now we break out early.
>
> So I think your fix is wrong, but it's "close" to right: I suspect that we 
> can fix it by marking the "we hit the root commit" case, and just 
> disabling it for that case.

Ahh, I was preparing a response that begins with "Wow, a joy of
working in a mailing list with people more clever than me!  It's
so obvious but I did not think of it."  I've written something
like that more than a few times on this list responding to
several people, I think.

However, I am afraid that is not quite enough.  It is not just
"when we hit the root".

Consider the same topology in the small test (1-2-3-4) but with
three additional commits:

         B---C
        /
    ---A---1---2---3---4

Again, 2-3-4 are in nice chronological order, but 1 has the
younguest timestamp, and A-B-C are all younger than 1.

	$ rev-list 1 ^4 ^A
        $ rev-list 1 ^4 ^B

These two would both mark A as uninteresting while processing
the command line (revision.c::handle_commit()).  When we pop 1
off, the call to add_parents_to_list() for it will not add
anything positive back.

	$ rev-list 1 ^4 ^C

This would not mark A as uninteresting immediately, but by the
time 1 gets its turn, A is marked uninteresting.

So I think the rule to notice this situation with "hit-root"
flag is something like:

    when we pop a positive commit that does not have any
    positive parent left (root is a special case of this), and
    the negative parents were contaminated either by:

        (1) being listed as negative on the command line or being a
            direct parent of a negative commit listed on the
            command line; or by

        (2) traversing the list of negative commits who are all
            younger than the positive commit in question.

---

 t/t6009-rev-list-parent.sh |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index be3d238..0bb5ac4 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -16,6 +16,14 @@ test_expect_success setup '
 	touch file &&
 	git add file &&
 
+	commit zero &&
+	commit A &&
+	commit B &&
+	commit C &&
+
+	git reset --hard A &&
+
+	test_tick=$(($test_tick - 1200))
 	commit one &&
 
 	test_tick=$(($test_tick - 2400))
@@ -27,9 +35,33 @@ test_expect_success setup '
 	git log --pretty=oneline --abbrev-commit
 '
 
-test_expect_failure 'one is ancestor of others and should not be shown' '
+test_expect_failure '"zero ^four" should be empty' '
+
+	git rev-list zero --not four >result &&
+	>expect &&
+	diff -u expect result
+
+'
+
+test_expect_failure '"one ^four ^A" should be empty' '
+
+	git rev-list one --not four A >result &&
+	>expect &&
+	diff -u expect result
+
+'
+
+test_expect_failure '"one ^four ^B should be empty' '
+
+	git rev-list one --not four B >result &&
+	>expect &&
+	diff -u expect result
+
+'
+
+test_expect_failure '"one ^four ^C should be empty' '
 
-	git rev-list one --not four >result &&
+	git rev-list one --not four C >result &&
 	>expect &&
 	diff -u expect result
 




-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html