Re: [PATCH] log: allow to specify diff pathspec in addition to prune pathspec

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Nov 5, 2011, 03:25 UTC
Message-ID: <CACsJy8CYEN5Li43C0H2hLbkAzEmGSUBiN+qyQ8tcOLx=3Cox0Q@mail.gmail.com>
In-Reply-To: <7vwrbfa6j6.fsf@alter.siamese.dyndns.org>

2011/11/5 Junio C Hamano <gitster@pobox.com>:

Show 17 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Pathspec in "git log -p <pathspec>" is used for both commit pruning
>> and diff generation. If --full-diff is given, then diff pathspec is
>> reset to generate complete diff.
>>
>> This patch gives more control to diff generation.  The first pathspec in
>> "git log -p -- <pathspec> -- <pathspec>" is used as commit pruning
>> as usual. The second one is used for diff generation. So --full-diff
>> now is essentially "git log -p -- <pathspec> --".
>
> I agree that giving more control to diff generation is a good idea, and
> this certainly is better than the previous round that nobody reviewed
> before you rerolled this round.
>
> But I have doubts about declaring "--full-diff is equivalent to giving the
> 'output' pathspec that is empty".

The equivalent command should be "git log -p -- <pathspec> -- :/" (in case you're in subdir). What doubts specifically?

> Have you thought about the interaction between this and -M/-C options?

No, yes I may have problems there.

Show 13 quoted lines

> This is not a new "problem" (and it is probably not a problem to begin
> with).  If you had "--" in the working tree and you wanted to treat it as
> a path, you said either one of these:
>
>    $ git log HEAD ./--
>    $ git log HEAD -- --
>
> The latter is what your patch breaks, I suspect.
>
> Also it forces existing scripts and programs that knew "everything in this
> array is a pathspec" and added "--" before adding the contents of the
> array to form a command line to drive "git log" family of commands to be
> updated.

We could only recognize the second "--" when a new argument is given so it won't break existing use. But I'll need to look at -C/-M first. Smells like --follow problem.

-- 
Duy