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

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

[Nguyễn Thái Ngọc Duy]

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, which could be useful for filtering out generated files
(negative pathspec to rescue!), or simply to limit diff output to a
selection of files.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/diff.c  | 26 ++++++++++++++------------
 diff-no-index.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 0f247d2..4e37876 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -321,23 +321,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	init_revisions(&rev, prefix);
 
-	if (no_index && argc != i + 2) {
-		if (no_index == DIFF_NO_INDEX_IMPLICIT) {
-			/*
-			 * There was no --no-index and there were not two
-			 * paths. It is possible that the user intended
-			 * to do an inside-repository operation.
-			 */
-			fprintf(stderr, "Not a git repository\n");
-			fprintf(stderr,
-				"To compare two paths outside a working tree:\n");
-		}
+	if (no_index == DIFF_NO_INDEX_IMPLICIT && argc != i + 2) {
+		/*
+		 * There was no --no-index and there were not two
+		 * paths. It is possible that the user intended
+		 * to do an inside-repository operation.
+		 */
+		fprintf(stderr, "Not a git repository\n");
+		fprintf(stderr,
+			"To compare two paths outside a working tree:\n");
 		/* Give the usage message for non-repository usage and exit. */
 		usagef("git diff %s <path> <path>",
 		       no_index == DIFF_NO_INDEX_EXPLICIT ?
 		       "--no-index" : "[--no-index]");
-
 	}
+	if (no_index == DIFF_NO_INDEX_EXPLICIT && argc < i + 2)
+		/* Give the usage message for non-repository usage and exit. */
+		usagef("git diff %s <path> <path>",
+		       no_index == DIFF_NO_INDEX_EXPLICIT ?
+		       "--no-index" : "[--no-index]");
 	if (no_index)
 		/* If this is a no-index diff, just run it and exit there. */
 		diff_no_index(&rev, argc, argv, prefix);
diff --git a/diff-no-index.c b/diff-no-index.c
index 265709b..7f5cd0f 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -89,8 +89,20 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)
 	return s;
 }
 
+static int path_ok(struct diff_options *o, const char *name, int prefix)
+{
+	if (!name)
+		return 0;
+	name += prefix;
+	if (*name == '/')
+		name++;
+	return match_pathspec(&o->pathspec, name, strlen(name),
+			      0, NULL, 0);
+}
+
 static int queue_diff(struct diff_options *o,
-		      const char *name1, const char *name2)
+		      const char *name1, int prefix1,
+		      const char *name2, int prefix2)
 {
 	int mode1 = 0, mode2 = 0;
 
@@ -157,7 +169,7 @@ static int queue_diff(struct diff_options *o,
 				n2 = buffer2.buf;
 			}
 
-			ret = queue_diff(o, n1, n2);
+			ret = queue_diff(o, n1, prefix1, n2, prefix2);
 		}
 		string_list_clear(&p1, 0);
 		string_list_clear(&p2, 0);
@@ -165,7 +177,7 @@ static int queue_diff(struct diff_options *o,
 		strbuf_release(&buffer2);
 
 		return ret;
-	} else {
+	} else if (path_ok(o, name1, prefix1) || path_ok(o, name2, prefix2)) {
 		struct diff_filespec *d1, *d2;
 
 		if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
@@ -180,13 +192,14 @@ static int queue_diff(struct diff_options *o,
 		diff_queue(&diff_queued_diff, d1, d2);
 		return 0;
 	}
+	return 0;
 }
 
 void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv,
 		   const char *prefix)
 {
-	int i, prefixlen;
+	int i, j, prefixlen;
 	const char *paths[2];
 
 	diff_setup(&revs->diffopt);
@@ -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 {
 			j = diff_opt_parse(&revs->diffopt, argv + i, argc - i);
-			if (j <= 0)
+			if (j <= 0) {
+				if (argv[i][0] != '-' || argv[i][1] == '\0')
+					break;
 				die("invalid diff option/value: %s", argv[i]);
+			}
 			i += j;
 		}
 	}
 
 	prefixlen = prefix ? strlen(prefix) : 0;
-	for (i = 0; i < 2; i++) {
-		const char *p = argv[argc - 2 + i];
+	for (j = 0; j < 2 && i < argc; j++, i++) {
+		const char *p = argv[i];
 		if (!strcmp(p, "-"))
 			/*
 			 * stdin should be spelled as "-"; if you have
@@ -215,7 +232,15 @@ void diff_no_index(struct rev_info *revs,
 			p = file_from_standard_input;
 		else if (prefixlen)
 			p = xstrdup(prefix_filename(prefix, prefixlen, p));
-		paths[i] = p;
+		paths[j] = p;
+	}
+	if (j < 2)
+		die("two paths required");
+	if (i < argc) {
+		const char *p = argv[i];
+		if (strcmp(p, "--"))
+			die("two paths required");
+		parse_pathspec(&revs->diffopt.pathspec, 0, 0, "", argv + i + 1);
 	}
 	revs->diffopt.skip_stat_unmatch = 1;
 	if (!revs->diffopt.output_format)
@@ -229,7 +254,9 @@ void diff_no_index(struct rev_info *revs,
 	setup_diff_pager(&revs->diffopt);
 	DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
 
-	if (queue_diff(&revs->diffopt, paths[0], paths[1]))
+	if (queue_diff(&revs->diffopt,
+		       paths[0], strlen(paths[0]),
+		       paths[1], strlen(paths[1])))
 		exit(1);
 	diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
 	diffcore_std(&revs->diffopt);
-- 
2.1.0.rc0.78.gc0d8480

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

[Junio C Hamano]

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

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

> @@ -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?

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

[Duy Nguyen]

On Fri, Sep 19, 2014 at 5:41 AM, Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

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

> 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,...

Actually, I am not so sure about this anymore.

It is not entirely fair to say that

	git diff --no-index [--options] A B C D

as comparing A and B while using C and D as pathspec and declare a
person who expects otherwise a moron.  To a person whose brain is
not rotten by years of use of Git, "we never compare anything other
than two things with --no-index" is not a given, I am afraid.

It is equally plausible that the same command line may be asking to
compare A with B and C with D, i.e. producing

	diff --no-git a/A b/B
        ...
        diff --no-git a/C b/D
        ...

It also equally plausible (taking a cue from "mv A B C Dir/") that
it is comparing A, B and C with D/A, D/B and D/C, respectively,
producing

	diff --no-git a/A b/D/A
        ...
        diff --no-git a/B b/D/B
        ...
        diff --no-git a/C b/D/C
        ...

The only unambiguous syntax I can think of that avoids such
alternative interpretations is something ugly like

    git diff --no-index [-<options>] --src=<path> --dst=<path> [--] <pathspec>

where "src" and "dst" are in line with the existing src/dst-prefix
options.

Perhaps we could declare that this is the "canonical" way to spell
pathspec-filtered no-index comparison of two directories, but we
allow the syntax suggested in the message I am responding to as a
short-hand, but I am not sure if that would fly well.

This --src/--dst thing could lead to even uglier tangent.  We could
use these options to specify what is compared with what else

    --{src,dst}-path=<directory in the working tree>
    --{src,dst}-index
    --{src,dst}-worktree
    --{src,dst}-tree=<tree object>

which allows us to express funky combinations like

	git diff --src-index --dst-worktree [--] <pathspec>
	git diff --src-tree=HEAD --dst-index [--] <pathspec>
	git diff --src-tree=maint --dst-tree=master [--] <pathspec>

The above three would be ugly ways to spell the traditional "short
hands", e.g.

	git diff [--] <pathspec>
        git diff --cached [--] <pathspec>
        git diff maint master [--] <pathspec>

respectively.  And an obvious fallout of the above is this command
line:

	git diff --src-path=A --dst-path=B [--] <pathspec>

Because this does not involve any index, tree or worktree, it cannot
be a Git operation, so we know that is --no-index mode; the command
line does not need to say --no-index anywhere if we go that route.

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

[Duy Nguyen]

On Sun, Sep 21, 2014 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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,...
>
> Actually, I am not so sure about this anymore.
>
> It is not entirely fair to say that
>
>         git diff --no-index [--options] A B C D
>
> as comparing A and B while using C and D as pathspec and declare a
> person who expects otherwise a moron.  To a person whose brain is
> not rotten by years of use of Git, "we never compare anything other
> than two things with --no-index" is not a given, I am afraid.
>
> It is equally plausible that the same command line may be asking to
> compare A with B and C with D, i.e. producing
>
>         diff --no-git a/A b/B
>         ...
>         diff --no-git a/C b/D
>         ...
>

Heh.. I wrote something like that [1]. An extension of this is feed
all filename pairs via stdin.

[1] http://thread.gmane.org/gmane.comp.version-control.git/188435

> It also equally plausible (taking a cue from "mv A B C Dir/") that
> it is comparing A, B and C with D/A, D/B and D/C, respectively,
> producing
>
>         diff --no-git a/A b/D/A
>         ...
>         diff --no-git a/B b/D/B
>         ...
>         diff --no-git a/C b/D/C
>         ...

This mode is weird... In real world, "A" may match to D/some-path/A
not just D/A. Which brings it back to the previous mode.

> The only unambiguous syntax I can think of that avoids such
> alternative interpretations is something ugly like
>
>     git diff --no-index [-<options>] --src=<path> --dst=<path> [--] <pathspec>
>
> where "src" and "dst" are in line with the existing src/dst-prefix
> options.

Or let the user define a "separator", e.g.

git diff --no-index --pathspec-separator=<sep> [--] <paths...> [<sep>
<pathspec>]

Another option is pathspec magic (I'm not entirely sure how it looks
like at the implementaiton level yet), --src=<path> could become
:(source)path, --dst=path -> :(dst)path and so on..

> Perhaps we could declare that this is the "canonical" way to spell
> pathspec-filtered no-index comparison of two directories, but we
> allow the syntax suggested in the message I am responding to as a
> short-hand, but I am not sure if that would fly well.
>
> This --src/--dst thing could lead to even uglier tangent.  We could
> use these options to specify what is compared with what else
>
>     --{src,dst}-path=<directory in the working tree>
>     --{src,dst}-index
>     --{src,dst}-worktree
>     --{src,dst}-tree=<tree object>
>
> which allows us to express funky combinations like
>
>         git diff --src-index --dst-worktree [--] <pathspec>
>         git diff --src-tree=HEAD --dst-index [--] <pathspec>
>         git diff --src-tree=maint --dst-tree=master [--] <pathspec>
>
> The above three would be ugly ways to spell the traditional "short
> hands", e.g.
>
>         git diff [--] <pathspec>
>         git diff --cached [--] <pathspec>
>         git diff maint master [--] <pathspec>
>
> respectively.  And an obvious fallout of the above is this command
> line:
>
>         git diff --src-path=A --dst-path=B [--] <pathspec>
>
> Because this does not involve any index, tree or worktree, it cannot
> be a Git operation, so we know that is --no-index mode; the command
> line does not need to say --no-index anywhere if we go that route.

Sounds a lot like Jeff's "git-put", except with -- syntax instead of a
special one, and you only look at the diff, not move any content. Also
people who would go with "git diff --no-index A B C D E F..." usually
script it, not type it manually. And stdin would fit better than
command line arguments.
-- 
Duy