[PATCH 1/2] Demonstrate git-show is broken with ranges

[PATCH 1/2] Demonstrate git-show is broken with ranges

[Thomas Rast]

The logic of git-show has remained largely unchanged since around
5d7eeee (git-show: grok blobs, trees and tags, too, 2006-12-14): start
a revision walker with no_walk=1, look at its pending objects and
handle them one-by-one.  For commits, this means stuffing them into a
new queue all alone, and running the walker.

Then Linus's f222abd (Make 'git show' more useful, 2009-07-13) came
along and set no_walk=0 whenever the user specifies a range.  Which
appears to work fine, until you actually prod it hard enough, as the
preceding commit shows: UNINTERESTING commits will be marked as such,
but not walked further to propagate the marks.

Demonstrate this with the main tests of this patch: 'showing a range
walks (Y shape)'.  The Y shape of history ensures that propagating the
UNINTERESTING marks is necessary to correctly exclude the main1
commit.  The only example I could find actually requires that the
negative revisions are listed later, and in this scenario a dotted
range actually works.  However, it is easy to find examples in git.git
where a dotted range is wrong, e.g.

  $ git show v1.7.0..v1.7.1 | grep ^commit | wc -l
  1297
  $ git rev-list v1.7.0..v1.7.1 | wc -l
  702

While there, also test a few other things that are not covered so far:
the -N way of triggering a range (added in 5853cae, DWIM 'git show -5'
to 'git show --do-walk -5', 2010-06-01), and the interactions of tags,
commits and ranges.

Pointed out by Dr_Memory on #git.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t7007-show.sh | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index cce222f..1c43963 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -17,4 +17,96 @@ test_expect_success 'showing a tag that point at a missing object' '
 	test_must_fail git --no-pager show foo-tag
 '
 
+test_expect_success 'set up a bit of history' '
+	test_commit main1 &&
+	test_commit main2 &&
+	test_commit main3 &&
+	git tag -m "annotated tag" annotated &&
+	git checkout -b side HEAD^^ &&
+	test_commit side2 &&
+	test_commit side3
+'
+
+test_expect_success 'showing two commits' '
+	cat >expect <<EOF &&
+commit $(git rev-parse main2)
+commit $(git rev-parse main3)
+EOF
+	git show main2 main3 >actual &&
+	grep ^commit actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing a range walks (linear)' '
+	cat >expect <<EOF &&
+commit $(git rev-parse main3)
+commit $(git rev-parse main2)
+EOF
+	git show main1..main3 >actual &&
+	grep ^commit actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing a range walks (Y shape, ^ first)' '
+	cat >expect <<EOF &&
+commit $(git rev-parse main3)
+commit $(git rev-parse main2)
+EOF
+	git show ^side3 main3 >actual &&
+	grep ^commit actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_failure 'showing a range walks (Y shape, ^ last)' '
+	cat >expect <<EOF &&
+commit $(git rev-parse main3)
+commit $(git rev-parse main2)
+EOF
+	git show main3 ^side3 >actual &&
+	grep ^commit actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing with -N walks' '
+	cat >expect <<EOF &&
+commit $(git rev-parse main3)
+commit $(git rev-parse main2)
+EOF
+	git show -2 main3 >actual &&
+	grep ^commit actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing annotated tag' '
+	cat >expect <<EOF &&
+tag annotated
+commit $(git rev-parse annotated^{commit})
+EOF
+	git show annotated >actual &&
+	grep -E "^(commit|tag)" actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing annotated tag plus commit' '
+	cat >expect <<EOF &&
+tag annotated
+commit $(git rev-parse annotated^{commit})
+commit $(git rev-parse side3)
+EOF
+	git show annotated side3 >actual &&
+	grep -E "^(commit|tag)" actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
+test_expect_success 'showing annotated tag in range' '
+	cat >expect <<EOF &&
+tag annotated
+commit $(git rev-parse main3)
+commit $(git rev-parse main2)
+EOF
+	git show ^side3 annotated >actual &&
+	grep -E "^(commit|tag)" actual >actual.filtered &&
+	test_cmp expect actual.filtered
+'
+
 test_done
-- 
1.7.11.266.g2b10bc0

[PATCH 2/2] Fix ranges with git-show

[Thomas Rast]

As explained in the previous commit, git-show's DWIM walking mode
fails with ranges where propagating the UNINTERESTING marks is needed
for correctness.

To fix this issue, use a new strategy for of dealing with commits:
handle everything else first, then pass all commits to a single
revision walker "in bulk".  This keeps the UNINTERESTING commits and
correctly shows all ranges.

Sadly we can use this only when actually walking.  In other cases,
such as 'git show A B', it would seem more important to keep the order
of the objects shown consistent with the command line.  There are no
such guarantees when running the walker with several commits (even in
no-walk mode), so we still feed them one by one.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/log.c   | 16 ++++++++++++----
 t/t7007-show.sh |  2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4f1b42a..26f6c01 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -444,6 +444,7 @@ static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt *
 int cmd_show(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
+	struct object_array commits = {0};
 	struct object_array_entry *objects;
 	struct setup_revision_opt opt;
 	struct pathspec match_all;
@@ -505,15 +506,22 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 			rev.shown_one = 1;
 			break;
 		case OBJ_COMMIT:
-			rev.pending.nr = rev.pending.alloc = 0;
-			rev.pending.objects = NULL;
-			add_object_array(o, name, &rev.pending);
-			ret = cmd_log_walk(&rev);
+			add_object_array(o, name, &commits);
+			if (rev.no_walk) {
+				rev.pending = commits;
+				ret = cmd_log_walk(&rev);
+				commits.nr = commits.alloc = 0;
+				commits.objects = NULL;
+			}
 			break;
 		default:
 			ret = error(_("Unknown type: %d"), o->type);
 		}
 	}
+	if (!ret && commits.nr) {
+		rev.pending = commits;
+		ret = cmd_log_walk(&rev);
+	}
 	free(objects);
 	return ret;
 }
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index 1c43963..3936ff7 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -57,7 +57,7 @@ EOF
 	test_cmp expect actual.filtered
 '
 
-test_expect_failure 'showing a range walks (Y shape, ^ last)' '
+test_expect_success 'showing a range walks (Y shape, ^ last)' '
 	cat >expect <<EOF &&
 commit $(git rev-parse main3)
 commit $(git rev-parse main2)
-- 
1.7.11.266.g2b10bc0

Re: [PATCH 2/2] Fix ranges with git-show

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> As explained in the previous commit, git-show's DWIM walking mode
> fails with ranges where propagating the UNINTERESTING marks is needed
> for correctness.

Finally ;-)

Another bad thing about the Linus's walking hackery is it depends on
the order of parameters (e.g "show maint master..next" and "show
master..next maint" gives us vastly different results).

> To fix this issue, use a new strategy for of dealing with commits:
> handle everything else first, then pass all commits to a single
> revision walker "in bulk".  This keeps the UNINTERESTING commits and
> correctly shows all ranges.

In other words, do not implement a half-hearted revision walking
itself, but let the proper call to cmd_log_walk() take care of the
walking.  Good.

> Sadly we can use this only when actually walking.

I do not think it is sad at all.  If we were told to walk, we should
walk just like "git log" does, and otherwise we just show them one
by one.  It might be even cleaner if we separate the command line
parsing and showing into separate phases, instead of the current
loop structure of parsing one object and deciding how to show.

How about doing it this way without applying your 2/2?

Among your new tests:

	git show ^side3 annotated

which is equivalent to

	git show side3..annotate

must change, as it is not asking to show individual objects (in
other words, I think the test expects a wrong thing), but everything
else should work as expected (I didn't check).

diff --git a/builtin/log.c b/builtin/log.c
index 56bc555..9ea2eb7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -451,6 +451,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	opt.tweak = show_rev_tweak_rev;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
 
+	if (!rev.no_walk)
+		return cmd_log_walk(&rev);
+
 	count = rev.pending.nr;
 	objects = rev.pending.objects;
 	for (i = 0; i < count && !ret; i++) {

Re: [PATCH 2/2] Fix ranges with git-show

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> As explained in the previous commit, git-show's DWIM walking mode
>> fails with ranges where propagating the UNINTERESTING marks is needed
>> for correctness.
>
> Finally ;-)
>
> Another bad thing about the Linus's walking hackery is it depends on
> the order of parameters (e.g "show maint master..next" and "show
> master..next maint" gives us vastly different results).

By the way, this reminds me of a totally separate topic we discussed
here recently.

Imagine you were on the maint-1.7.9 branch and gave this command.

	git cherry-pick maint master..topic

The range makes the command walk, and because of that, the above
does not result in transplanting a topic that depends on the commit
at the tip of maint to an older maint-1.7.9 branch.  You would
instead need to write something like:

	git cherry-pick maint $(git rev-list --reverse master..topic)

to prevent cherry-pick from walking.

But we could introduce a new "mode" to the revision machinery so
that a single token on the command line is parsed as a set of
commits, and causes the "walking" machinery not to walk the commits
in the union of the sets of commits specified from the command line,
e.g. (pardon my terrible option name)

    git cherry-pick --union-no-walk maint master..topic1 master..topic2

which would instruct the revision machinery that:

 (1) "maint" (the first token) yields a set that consists of a
     single commit at the tip of "maint";

 (2) "master..topic1" (another token) yields a separate set that
     consists of commits on the "topic1" branch since it forked from
     the "master" (similarly for "topic2");

 (3) the revision machinery is to compute these two sets
     _independently_ (meaning, object flags like UNINTERESTING are
     cleared after each run);

 (4) take union of the set, eliminating duplication (if "topic1" and
     "topic2" share some commits at their bottom, they are
     de-duped); and

 (5) the call to get_revision() will not walk, and instead return
     the elements of the resulting union one-by-one.

which would give us what most users of cherry-pick would naturally
expect.  For some commands (like cherry-pick and show), we might
even want default to --union-no-walk behaviour, but that is probably
a Git 2.0 topic after we gain enough confidence with experience.

Re: [PATCH 2/2] Fix ranges with git-show

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> As explained in the previous commit, git-show's DWIM walking mode
>>> fails with ranges where propagating the UNINTERESTING marks is needed
>>> for correctness.
>>
>> Finally ;-)
>>
>> Another bad thing about the Linus's walking hackery is it depends on
>> the order of parameters (e.g "show maint master..next" and "show
>> master..next maint" gives us vastly different results).
>
> By the way, this reminds me of a totally separate topic we discussed
> here recently.
>
> Imagine you were on the maint-1.7.9 branch and gave this command.
>
> 	git cherry-pick maint master..topic
>
> The range makes the command walk, and because of that, the above
> does not result in transplanting a topic that depends on the commit
> at the tip of maint to an older maint-1.7.9 branch.  You would
> instead need to write something like:
>
> 	git cherry-pick maint $(git rev-list --reverse master..topic)
>
> to prevent cherry-pick from walking.
>
> But we could introduce a new "mode" to the revision machinery so
> that a single token on the command line is parsed as a set of
> commits, and causes the "walking" machinery not to walk the commits
> in the union of the sets of commits specified from the command line,
> e.g. (pardon my terrible option name)
>
>     git cherry-pick --union-no-walk maint master..topic1 master..topic2
>
> which would instruct the revision machinery that:
>
>  (1) "maint" (the first token) yields a set that consists of a
>      single commit at the tip of "maint";
>
>  (2) "master..topic1" (another token) yields a separate set that
>      consists of commits on the "topic1" branch since it forked from
>      the "master" (similarly for "topic2");
>
>  (3) the revision machinery is to compute these two sets
>      _independently_ (meaning, object flags like UNINTERESTING are
>      cleared after each run);

Sorry, s/two sets/three sets/; I originally wrote without "topic2"
and forgot to clean it up.

>  (4) take union of the set, eliminating duplication (if "topic1" and
>      "topic2" share some commits at their bottom, they are
>      de-duped); and
>
>  (5) the call to get_revision() will not walk, and instead return
>      the elements of the resulting union one-by-one.
>
> which would give us what most users of cherry-pick would naturally
> expect.  For some commands (like cherry-pick and show), we might
> even want default to --union-no-walk behaviour, but that is probably
> a Git 2.0 topic after we gain enough confidence with experience.

Re: [PATCH 2/2] Fix ranges with git-show

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> I do not think it is sad at all.  If we were told to walk, we should
> walk just like "git log" does, and otherwise we just show them one
> by one.  It might be even cleaner if we separate the command line
> parsing and showing into separate phases, instead of the current
> loop structure of parsing one object and deciding how to show.
>
> How about doing it this way without applying your 2/2?
[...]
>  	opt.tweak = show_rev_tweak_rev;
>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
>  
> +	if (!rev.no_walk)
> +		return cmd_log_walk(&rev);
> +
>  	count = rev.pending.nr;
>  	objects = rev.pending.objects;
>  	for (i = 0; i < count && !ret; i++) {

Clever.  But it eliminates all possibility of *simultaneously* showing a
range and some other objects, of which

> Among your new tests:
>
> 	git show ^side3 annotated
>
> must change, as it is not asking to show individual objects (in
> other words, I think the test expects a wrong thing), but everything
> else should work as expected (I didn't check).

is just a symptom of.  Do you want to change it?  I'd be all for it, but
it changes the meaning somewhat.  So far, showing "anything plus ranges"
is broken only as far as the ranges are concerned.

If you do make this change, can we merge the log and show code?
Granted, show defaults to -p --cc --no-walk, and log does not, but can
we then unify the main logic?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/2] Fix ranges with git-show

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Clever.  But it eliminates all possibility of *simultaneously* showing a
> range and some other objects, of which
>
>> Among your new tests:
>>
>> 	git show ^side3 annotated
>>
>> must change, as it is not asking to show individual objects (in
>> other words, I think the test expects a wrong thing), but everything
>> else should work as expected (I didn't check).
>
> is just a symptom of.  Do you want to change it?  

I do not understand the question.  Do I think your patch is broken
that "git show ^side3 annotated" shows the annotated tag?  Yes, I
think it is broken, and it is the symptom of the same breakage as
the "If we have a range, let's walk it outselves without doing
exactly the same thing as the normal 'log' walk".  In other words,
"git show" is to show individual objects unless you feed ranges.  If
you feed ranges, it will act as "git log".

> So far, showing "anything plus ranges"
> is broken only as far as the ranges are concerned.

Yes, I think the command works correctly only when you do not give
any range, and once you give any range, its output is utterly
broken.

> If you do make this change, can we merge the log and show code?
> Granted, show defaults to -p --cc --no-walk, and log does not, but can
> we then unify the main logic?

What "main logic" remains?  

I offhand do not think of a need to do anything more than the above
patch, but I may not be getting the benefit you are seeing.  

The remainder of cmd_show() are about showing individual objects
(including commits), which has very different semantics from what
"log" does, no?

"log" asks get_revision() to grab each commit while digging the
history deeper with add_parents_to_list() and feed the returned
commit to the internal diff-tree , but "show" does not have to use
get_revision() because it already has list of objects it wants to
show, and calling into get_revision() is not even necessary as it
does not want it to do add_parents_to_list() to find more commits to
show.