threads / patch / 37524

patchdiff --no-index: allow pathspec after --

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

## tl;dr

5 messages between Sep 9, 2014 and Sep 21, 2014. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Sep 9, 2014, 09:56 UTC · lore
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(-)
Show changes to 2 files +52 −23

builtin/diff.c, diff-no-index.c

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
Junio C Hamano· Sep 18, 2014, 22:41 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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?
Duy Nguyen· Sep 21, 2014, 04:08 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Sep 21, 2014, 06:19 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 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,...
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.

Duy Nguyen· Sep 21, 2014, 09:25 UTC · re: Junio C Hamano · lore

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

On Sun, Sep 21, 2014 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> 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
Show 10 quoted lines
> 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.

Show 7 quoted lines
> 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..

Show 34 quoted lines
> 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

← back to recent threads