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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2012, 17:47 UTC
Message-ID: <7vmx3xx4d0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87bokexwiq.fsf@thomas.inf.ethz.ch>

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.