On Fri, Sep 19, 2014 at 5:41 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> @@ -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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html