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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2012, 21:11 UTC
Message-ID: <7vhau7ypk6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <a598bb8c20221679e295caa743197c86219eda68.1340136145.git.trast@student.ethz.ch>

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++) {