Re: [PATCH v8 4/5] Implement line-history search (git log -L)

From: Thomas Rast <trast@student.ethz.ch>
Date: Feb 28, 2013, 21:41 UTC
Message-ID: <87hakw3yma.fsf@pctrast.inf.ethz.ch>
In-Reply-To: <7v38wgi38z.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 23 quoted lines

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> +/*
>>>> + * NEEDSWORK: manually building a diff here is not the Right
>>>> + * Thing(tm).  log -L should be built into the diff pipeline.
>>>
>>> I am not sure about this design, and do not necessarily agree that
>>> wedging this to the diff pipeline is the right future direction.
>>>
>>> I have a feeling that "log -L" should actually be built around
>>> "blame".  You let blame to hit the first parent to take the blame,
>>> and then turn around to show a single "diff-tree" between the child
>>> and the parent with whatever other diff pipeline gizmo the user can
>>> give you from the command line.  The blame also tells you what the
>>> "interesting" line range were at the first parent commit you found,
>>> so you can re-run the same thing with an updated range.
>>
>> Hrm, now that you mention it, this is actually a brilliant idea.
>
> I don't know.  That is just me handwaving without giving a serious
> end-to-end thought.

Having thought about it for some time, I think we need to figure out if/what can be shared. I can't shake off the feeling that *something* should be common between blame and log -L, but I can't exactly say what so far.

Your suggestion of looking at the first blame hit is almost there. (And in fact if it did work, it should be rather easy to prototype from blame --incremental.) However, it works only for additions, not removals. Lines that were removed do not show up in blame at all, and since a patch can also _only_ remove lines, blame would not find it even if we adjust the blamed range at every found commit.

It may be possible to fix that by doing a reverse blame, but I suspect that runs into yet more trouble when trying to reverse-blame from two different sides of history.

Then there's a different issue if the order of code flips. Suppose you have

  A1
  A2
  B1
  B2

and later change to

  B1
  B2
  A1
  A2

Diffs can fundamentally not express this; they'll only see one side as unchanged, and the other as completely new. Ideally we'd be able to track this case correctly -- as blame does? -- no matter which part is within our tracked range.

Anyway, I believe this should be booked under "future improvements". I've had it in my own tree for ages and it already does the right thing most of the time :-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch