Re: [PATCH v7 0/5] git log -L, all new and shiny

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2012, 15:23 UTC
Message-ID: <7v1ulgd2f5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <8762as4sax.fsf@thomas.inf.ethz.ch>

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

Show 20 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> I too thought it would never happen -- but then again this is still
>>> not ready, I'm just trying to give it some exposure.
>>> ...
>>> There's also a longer-term wishlist hinted at in the commit message of
>>> the main patch: the diff machinery currently makes no provisions for
>>> chaining its various bells and whistles.
>>
>> I am not convinced that it is "diff machinery makes no provivsions"
>> that is the problem. Isn't it coming from the way the series limits
>> the output line range and reimplements its own output routine?
>
> Well, in a very circular logic sense, yes: I reimplement the output
> routine because that's the only way I could think of doing it right now :-)
>
> However, notice that word-diff also reimplements its own output routine,
> though it probably has a better standing since it is a different format.

Also notice that word-diff uses the same xdi_diff_outf() machinery to grab line-oriented diff as its input (cf. fn_out_consume), and then does its thing on it. If you limit what fn_out_consume sees, you can have word-diff do exactly what you want, no?

> This would be the first backwards coupling between the revision-walk and
> the diff generation parts, at least that I know of.

I am not convinced if you need to have any unusual back-coupling to begin with, by the way.

If you say "git log -p [--options] -- pathspec", the revision machinery does filter commits that do not touch any paths that patch pathspec with the TREESAME logic, but that does not necessarily mean you will see _all_ the commits that are not TREESAME. If you for example use ignore-space-change options, even the preimage and the postimage differ at the object name level (hence not TREESAME), the diff machinery already knows how to tell the revision machinery not to show the log message and stuff, causing the commit to be skipped from the output, no?

I do not know why you think you would need to do the filtering "range comparison and union" computation more than necessary. If the user asks "log -p", you need to do it once per parent-child pair that is not TREESAME at the place the current code calls run_diff(). I suspect that "log -p --stat" could be improved to eliminate the separate call to run_diffstat() by restructuring the code so that the statistics is gathered inside run_diff(), but that is independent of this series. If this series hooked into the level I hinted in my earlier message, such an optimization will reduce calls to your "range comparision and union" computation for free.