threads / patch / 30843

patch, 2 partsDemonstrate git-show is broken with ranges

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

## tl;dr

7 messages between Jun 19, 2012 and Jun 20, 2012. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Thomas Rast· Jun 19, 2012, 20:04 UTC · lore

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(+)
Show changes to t/t7007-show.sh +92 −0
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
Thomas Rast· Jun 19, 2012, 20:04 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 2 files +13 −5

builtin/log.c, t/t7007-show.sh

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
Junio C Hamano· Jun 19, 2012, 21:11 UTC · re: Thomas Rast · lore

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

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

Show changes to builtin/log.c +3 −0
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++) {
Junio C Hamano· Jun 19, 2012, 21:31 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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.

Junio C Hamano· Jun 19, 2012, 21:36 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 50 quoted lines
> 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.

Show 11 quoted lines
>  (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.
Thomas Rast· Jun 20, 2012, 07:39 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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?
[...]
Show 9 quoted lines
>  	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

Show 7 quoted lines
> 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
Junio C Hamano· Jun 20, 2012, 17:47 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 12 quoted lines
> 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.

← back to recent threads