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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2014, 22:41 UTC
Message-ID: <xmqqha04o8k7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1410256584-19562-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 11 quoted lines

> Another patch to test the water before I put more effort into it.
>
> Commit d516c2d (Teach git-diff-files the new option `--no-index` -
> 2007-02-22) brings the bells and whistles of git-diff to the world
> outside a git repository. This patch continues that direction and adds
> a new syntax
>
>     git diff --no-index [--] <path> <path> -- <pathspec>
>
> It's easy to guess that the two directories will be filtered by
> pathspec,...

Ugh. Nobody's diff works that way, and nowhere in our UI we use double-dashes that way, either.

So while I like the idea of "I want to tell Git to compare these two directories with these pathspec", I do not think we would want to touch such a syntax with 10 foot pole X-<.

Show 10 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.

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?