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

From: Thomas Rast <trast@inf.ethz.ch>
Date: Jun 20, 2012, 07:39 UTC
Message-ID: <87bokexwiq.fsf@thomas.inf.ethz.ch>
In-Reply-To: <7vhau7ypk6.fsf@alter.siamese.dyndns.org>

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