Re: [PATCH] diff --no-index: allow pathspec after --

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 21, 2014, 04:08 UTC
Message-ID: <CACsJy8CTwk_o7SCFas7q8di_4jZu717znx-44Eg-fxCHZnCTuw@mail.gmail.com>
In-Reply-To: <xmqqha04o8k7.fsf@gitster.dls.corp.google.com>

On Fri, Sep 19, 2014 at 5:41 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>> @@ -194,19 +207,23 @@ void diff_no_index(struct rev_info *revs,
>>               int j;
>>               if (!strcmp(argv[i], "--no-index"))
>>                       i++;
>> -             else if (!strcmp(argv[i], "--"))
>> +             else if (!strcmp(argv[i], "--")) {
>>                       i++;
>> -             else {
>> +                     break;
>> +             } else {
>
> I think this part is a good bugfix regardless of your new feature.
>
> The general command line structure ought to be:
>
>    $ git diff [--no-index] [--<diff options>...] [--] <path> <path>
>
> but the existing code is too loose in that "--" is just ignored.
> The caller in builtin/diff.c makes sure "--no-index" comes before
> "--", the latter of which stops option processing, so it is not so
> grave a bug, though.

Yeah I pack everything in one patch because this is more of a design question. Will make separate cleanup patches.

Show 27 quoted lines

> Coming back to the command line syntax for the new feature, if I had
> to choose, I would say
>
>         git diff --no-index [-<options>] [--] <path> <path> <pathspec>
>
> perhaps?  As we never compare anything other than two things, we can
> do the following:
>
>  * Implicit --no-index heuristics will kick in _ONLY_ when we have
>    two things at the end after parsing options in builtin/diff.c, as
>    before;
>
>  * In diff_no_index() after parsing options at its beginning,
>
>   - if we have only two things left, they may be
>
>     . two files;
>     . a file and "-" or "-" and a file; or
>     . two directories (fully traversed without pathspecs)
>
>     just as before.
>
>   - if we have more than two things left, the first two of these
>     _MUST_ be directories, and the remainder is pathspec to filter
>     the result of directory traversal.
>
> Wluldn't that be cleaner and easier to understand?

Looks good.

-- 
Duy