{"thread":{"id":"37524","subject":"[PATCH] diff --no-index: allow pathspec after --","startedAt":"2014-09-09T09:56:24Z","lastAt":"2014-09-21T09:25:31Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249073","messageId":"1410256584-19562-1-git-send-email-pclouds@gmail.com","threadId":"37524","inReplyTo":null,"subject":"[PATCH] diff --no-index: allow pathspec after --","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-09T09:56:24Z","receivedAt":"2014-09-09T09:56:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Another patch to test the water before I put more effort into it.\n\nCommit d516c2d (Teach git-diff-files the new option `--no-index` -\n2007-02-22) brings the bells and whistles of git-diff to the world\noutside a git repository. This patch continues that direction and adds\na new syntax\n\n git diff --no-index [--] -- \n\nIt's easy to guess that the two directories will be filtered by\npathspec, which could be useful for filtering out generated files\n(negative pathspec to rescue!), or simply to limit diff output to a\nselection of files.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/diff.c | 26 ++++++++++++++------------\n diff-no-index.c | 49 ++++++++++++++++++++++++++++++++++++++-----------\n 2 files changed, 52 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0f247d2..4e37876 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -321,23 +321,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \n-\tif (no_index && argc != i + 2) {\n-\t\tif (no_index == DIFF_NO_INDEX_IMPLICIT) {\n-\t\t\t/*\n-\t\t\t * There was no --no-index and there were not two\n-\t\t\t * paths. It is possible that the user intended\n-\t\t\t * to do an inside-repository operation.\n-\t\t\t */\n-\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n-\t\t\tfprintf(stderr,\n-\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n-\t\t}\n+\tif (no_index == DIFF_NO_INDEX_IMPLICIT && argc != i + 2) {\n+\t\t/*\n+\t\t * There was no --no-index and there were not two\n+\t\t * paths. It is possible that the user intended\n+\t\t * to do an inside-repository operation.\n+\t\t */\n+\t\tfprintf(stderr, \"Not a git repository\\n\");\n+\t\tfprintf(stderr,\n+\t\t\t\"To compare two paths outside a working tree:\\n\");\n \t\t/* Give the usage message for non-repository usage and exit. */\n \t\tusagef(\"git diff %s \",\n \t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n \t\t \"--no-index\" : \"[--no-index]\");\n-\n \t}\n+\tif (no_index == DIFF_NO_INDEX_EXPLICIT && argc < i + 2)\n+\t\t/* Give the usage message for non-repository usage and exit. */\n+\t\tusagef(\"git diff %s \",\n+\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n+\t\t \"--no-index\" : \"[--no-index]\");\n \tif (no_index)\n \t\t/* If this is a no-index diff, just run it and exit there. */\n \t\tdiff_no_index(&rev, argc, argv, prefix);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 265709b..7f5cd0f 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -89,8 +89,20 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\n \treturn s;\n }\n \n+static int path_ok(struct diff_options *o, const char *name, int prefix)\n+{\n+\tif (!name)\n+\t\treturn 0;\n+\tname += prefix;\n+\tif (*name == '/')\n+\t\tname++;\n+\treturn match_pathspec(&o->pathspec, name, strlen(name),\n+\t\t\t 0, NULL, 0);\n+}\n+\n static int queue_diff(struct diff_options *o,\n-\t\t const char *name1, const char *name2)\n+\t\t const char *name1, int prefix1,\n+\t\t const char *name2, int prefix2)\n {\n \tint mode1 = 0, mode2 = 0;\n \n@@ -157,7 +169,7 @@ static int queue_diff(struct diff_options *o,\n \t\t\t\tn2 = buffer2.buf;\n \t\t\t}\n \n-\t\t\tret = queue_diff(o, n1, n2);\n+\t\t\tret = queue_diff(o, n1, prefix1, n2, prefix2);\n \t\t}\n \t\tstring_list_clear(&p1, 0);\n \t\tstring_list_clear(&p2, 0);\n@@ -165,7 +177,7 @@ static int queue_diff(struct diff_options *o,\n \t\tstrbuf_release(&buffer2);\n \n \t\treturn ret;\n-\t} else {\n+\t} else if (path_ok(o, name1, prefix1) || path_ok(o, name2, prefix2)) {\n \t\tstruct diff_filespec *d1, *d2;\n \n \t\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n@@ -180,13 +192,14 @@ static int queue_diff(struct diff_options *o,\n \t\tdiff_queue(&diff_queued_diff, d1, d2);\n \t\treturn 0;\n \t}\n+\treturn 0;\n }\n \n void diff_no_index(struct rev_info *revs,\n \t\t int argc, const char **argv,\n \t\t const char *prefix)\n {\n-\tint i, prefixlen;\n+\tint i, j, prefixlen;\n \tconst char *paths[2];\n \n \tdiff_setup(&revs->diffopt);\n@@ -194,19 +207,23 @@ void diff_no_index(struct rev_info *revs,\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"--\"))\n+\t\telse if (!strcmp(argv[i], \"--\")) {\n \t\t\ti++;\n-\t\telse {\n+\t\t\tbreak;\n+\t\t} else {\n \t\t\tj = diff_opt_parse(&revs->diffopt, argv + i, argc - i);\n-\t\t\tif (j <= 0)\n+\t\t\tif (j <= 0) {\n+\t\t\t\tif (argv[i][0] != '-' || argv[i][1] == '\\0')\n+\t\t\t\t\tbreak;\n \t\t\t\tdie(\"invalid diff option/value: %s\", argv[i]);\n+\t\t\t}\n \t\t\ti += j;\n \t\t}\n \t}\n \n \tprefixlen = prefix ? strlen(prefix) : 0;\n-\tfor (i = 0; i < 2; i++) {\n-\t\tconst char *p = argv[argc - 2 + i];\n+\tfor (j = 0; j < 2 && i < argc; j++, i++) {\n+\t\tconst char *p = argv[i];\n \t\tif (!strcmp(p, \"-\"))\n \t\t\t/*\n \t\t\t * stdin should be spelled as \"-\"; if you have\n@@ -215,7 +232,15 @@ void diff_no_index(struct rev_info *revs,\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefixlen)\n \t\t\tp = xstrdup(prefix_filename(prefix, prefixlen, p));\n-\t\tpaths[i] = p;\n+\t\tpaths[j] = p;\n+\t}\n+\tif (j < 2)\n+\t\tdie(\"two paths required\");\n+\tif (i < argc) {\n+\t\tconst char *p = argv[i];\n+\t\tif (strcmp(p, \"--\"))\n+\t\t\tdie(\"two paths required\");\n+\t\tparse_pathspec(&revs->diffopt.pathspec, 0, 0, \"\", argv + i + 1);\n \t}\n \trevs->diffopt.skip_stat_unmatch = 1;\n \tif (!revs->diffopt.output_format)\n@@ -229,7 +254,9 @@ void diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n \n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n+\tif (queue_diff(&revs->diffopt,\n+\t\t paths[0], strlen(paths[0]),\n+\t\t paths[1], strlen(paths[1])))\n \t\texit(1);\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n-- \n2.1.0.rc0.78.gc0d8480\n"},{"id":"249650","messageId":"xmqqha04o8k7.fsf@gitster.dls.corp.google.com","threadId":"37524","inReplyTo":"1410256584-19562-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] diff --no-index: allow pathspec after --","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-18T22:41:44Z","receivedAt":"2014-09-18T22:41:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Another patch to test the water before I put more effort into it.\n>\n> Commit d516c2d (Teach git-diff-files the new option `--no-index` -\n> 2007-02-22) brings the bells and whistles of git-diff to the world\n> outside a git repository. This patch continues that direction and adds\n> a new syntax\n>\n> git diff --no-index [--] -- \n>\n> It's easy to guess that the two directories will be filtered by\n> pathspec,...\n\nUgh. Nobody's diff works that way, and nowhere in our UI we use\ndouble-dashes that way, either.\n\nSo while I like the idea of \"I want to tell Git to compare these two\ndirectories with these pathspec\", I do not think we would want to\ntouch such a syntax with 10 foot pole X-<.\n\n> @@ -194,19 +207,23 @@ void diff_no_index(struct rev_info *revs,\n> \t\tint j;\n> \t\tif (!strcmp(argv[i], \"--no-index\"))\n> \t\t\ti++;\n> -\t\telse if (!strcmp(argv[i], \"--\"))\n> +\t\telse if (!strcmp(argv[i], \"--\")) {\n> \t\t\ti++;\n> -\t\telse {\n> +\t\t\tbreak;\n> +\t\t} else {\n\nI think this part is a good bugfix regardless of your new feature.\n\nThe general command line structure ought to be:\n\n $ git diff [--no-index] [--...] [--] \n\nbut the existing code is too loose in that \"--\" is just ignored.\nThe caller in builtin/diff.c makes sure \"--no-index\" comes before\n\"--\", the latter of which stops option processing, so it is not so\ngrave a bug, though.\n\nComing back to the command line syntax for the new feature, if I had\nto choose, I would say\n\n\tgit diff --no-index [-] [--] \n\nperhaps? As we never compare anything other than two things, we can\ndo the following:\n\n * Implicit --no-index heuristics will kick in _ONLY_ when we have\n two things at the end after parsing options in builtin/diff.c, as\n before;\n\n * In diff_no_index() after parsing options at its beginning,\n\n - if we have only two things left, they may be\n\n . two files;\n . a file and \"-\" or \"-\" and a file; or\n . two directories (fully traversed without pathspecs)\n\n just as before.\n\n - if we have more than two things left, the first two of these\n _MUST_ be directories, and the remainder is pathspec to filter\n the result of directory traversal.\n\nWluldn't that be cleaner and easier to understand?\n"},{"id":"249712","messageId":"CACsJy8CTwk_o7SCFas7q8di_4jZu717znx-44Eg-fxCHZnCTuw@mail.gmail.com","threadId":"37524","inReplyTo":"xmqqha04o8k7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff --no-index: allow pathspec after --","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-21T04:08:17Z","receivedAt":"2014-09-21T04:08:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 19, 2014 at 5:41 AM, Junio C Hamano wrote:\n>> @@ -194,19 +207,23 @@ void diff_no_index(struct rev_info *revs,\n>> int j;\n>> if (!strcmp(argv[i], \"--no-index\"))\n>> i++;\n>> - else if (!strcmp(argv[i], \"--\"))\n>> + else if (!strcmp(argv[i], \"--\")) {\n>> i++;\n>> - else {\n>> + break;\n>> + } else {\n>\n> I think this part is a good bugfix regardless of your new feature.\n>\n> The general command line structure ought to be:\n>\n> $ git diff [--no-index] [--...] [--] \n>\n> but the existing code is too loose in that \"--\" is just ignored.\n> The caller in builtin/diff.c makes sure \"--no-index\" comes before\n> \"--\", the latter of which stops option processing, so it is not so\n> grave a bug, though.\n\nYeah I pack everything in one patch because this is more of a design\nquestion. Will make separate cleanup patches.\n\n> Coming back to the command line syntax for the new feature, if I had\n> to choose, I would say\n>\n> git diff --no-index [-] [--] \n>\n> perhaps? As we never compare anything other than two things, we can\n> do the following:\n>\n> * Implicit --no-index heuristics will kick in _ONLY_ when we have\n> two things at the end after parsing options in builtin/diff.c, as\n> before;\n>\n> * In diff_no_index() after parsing options at its beginning,\n>\n> - if we have only two things left, they may be\n>\n> . two files;\n> . a file and \"-\" or \"-\" and a file; or\n> . two directories (fully traversed without pathspecs)\n>\n> just as before.\n>\n> - if we have more than two things left, the first two of these\n> _MUST_ be directories, and the remainder is pathspec to filter\n> the result of directory traversal.\n>\n> Wluldn't that be cleaner and easier to understand?\n\nLooks good.\n-- \nDuy\n"},{"id":"249713","messageId":"xmqq38blmr6g.fsf@gitster.dls.corp.google.com","threadId":"37524","inReplyTo":"xmqqha04o8k7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff --no-index: allow pathspec after --","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-21T06:19:19Z","receivedAt":"2014-09-21T06:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Coming back to the command line syntax for the new feature, if I had\n> to choose, I would say\n>\n> \tgit diff --no-index [-] [--] \n>\n> perhaps? As we never compare anything other than two things,...\n\nActually, I am not so sure about this anymore.\n\nIt is not entirely fair to say that\n\n\tgit diff --no-index [--options] A B C D\n\nas comparing A and B while using C and D as pathspec and declare a\nperson who expects otherwise a moron. To a person whose brain is\nnot rotten by years of use of Git, \"we never compare anything other\nthan two things with --no-index\" is not a given, I am afraid.\n\nIt is equally plausible that the same command line may be asking to\ncompare A with B and C with D, i.e. producing\n\n\tdiff --no-git a/A b/B\n ...\n diff --no-git a/C b/D\n ...\n\nIt also equally plausible (taking a cue from \"mv A B C Dir/\") that\nit is comparing A, B and C with D/A, D/B and D/C, respectively,\nproducing\n\n\tdiff --no-git a/A b/D/A\n ...\n diff --no-git a/B b/D/B\n ...\n diff --no-git a/C b/D/C\n ...\n\nThe only unambiguous syntax I can think of that avoids such\nalternative interpretations is something ugly like\n\n git diff --no-index [-] --src= --dst= [--] \n\nwhere \"src\" and \"dst\" are in line with the existing src/dst-prefix\noptions.\n\nPerhaps we could declare that this is the \"canonical\" way to spell\npathspec-filtered no-index comparison of two directories, but we\nallow the syntax suggested in the message I am responding to as a\nshort-hand, but I am not sure if that would fly well.\n\nThis --src/--dst thing could lead to even uglier tangent. We could\nuse these options to specify what is compared with what else\n\n --{src,dst}-path=\n --{src,dst}-index\n --{src,dst}-worktree\n --{src,dst}-tree=\n\nwhich allows us to express funky combinations like\n\n\tgit diff --src-index --dst-worktree [--] \n\tgit diff --src-tree=HEAD --dst-index [--] \n\tgit diff --src-tree=maint --dst-tree=master [--] \n\nThe above three would be ugly ways to spell the traditional \"short\nhands\", e.g.\n\n\tgit diff [--] \n git diff --cached [--] \n git diff maint master [--] \n\nrespectively. And an obvious fallout of the above is this command\nline:\n\n\tgit diff --src-path=A --dst-path=B [--] \n\nBecause this does not involve any index, tree or worktree, it cannot\nbe a Git operation, so we know that is --no-index mode; the command\nline does not need to say --no-index anywhere if we go that route.\n"},{"id":"249718","messageId":"CACsJy8BYSTWGLv666OetUC5Td+SF8T2yWrMNC_jXU5Vn9hoESA@mail.gmail.com","threadId":"37524","inReplyTo":"xmqq38blmr6g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff --no-index: allow pathspec after --","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-09-21T09:25:31Z","receivedAt":"2014-09-21T09:25:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 21, 2014 at 1:19 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Coming back to the command line syntax for the new feature, if I had\n>> to choose, I would say\n>>\n>> git diff --no-index [-] [--] \n>>\n>> perhaps? As we never compare anything other than two things,...\n>\n> Actually, I am not so sure about this anymore.\n>\n> It is not entirely fair to say that\n>\n> git diff --no-index [--options] A B C D\n>\n> as comparing A and B while using C and D as pathspec and declare a\n> person who expects otherwise a moron. To a person whose brain is\n> not rotten by years of use of Git, \"we never compare anything other\n> than two things with --no-index\" is not a given, I am afraid.\n>\n> It is equally plausible that the same command line may be asking to\n> compare A with B and C with D, i.e. producing\n>\n> diff --no-git a/A b/B\n> ...\n> diff --no-git a/C b/D\n> ...\n>\n\nHeh.. I wrote something like that [1]. An extension of this is feed\nall filename pairs via stdin.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/188435\n\n> It also equally plausible (taking a cue from \"mv A B C Dir/\") that\n> it is comparing A, B and C with D/A, D/B and D/C, respectively,\n> producing\n>\n> diff --no-git a/A b/D/A\n> ...\n> diff --no-git a/B b/D/B\n> ...\n> diff --no-git a/C b/D/C\n> ...\n\nThis mode is weird... In real world, \"A\" may match to D/some-path/A\nnot just D/A. Which brings it back to the previous mode.\n\n> The only unambiguous syntax I can think of that avoids such\n> alternative interpretations is something ugly like\n>\n> git diff --no-index [-] --src= --dst= [--] \n>\n> where \"src\" and \"dst\" are in line with the existing src/dst-prefix\n> options.\n\nOr let the user define a \"separator\", e.g.\n\ngit diff --no-index --pathspec-separator= [--] [\n]\n\nAnother option is pathspec magic (I'm not entirely sure how it looks\nlike at the implementaiton level yet), --src= could become\n:(source)path, --dst=path -> :(dst)path and so on..\n\n> Perhaps we could declare that this is the \"canonical\" way to spell\n> pathspec-filtered no-index comparison of two directories, but we\n> allow the syntax suggested in the message I am responding to as a\n> short-hand, but I am not sure if that would fly well.\n>\n> This --src/--dst thing could lead to even uglier tangent. We could\n> use these options to specify what is compared with what else\n>\n> --{src,dst}-path=\n> --{src,dst}-index\n> --{src,dst}-worktree\n> --{src,dst}-tree=\n>\n> which allows us to express funky combinations like\n>\n> git diff --src-index --dst-worktree [--] \n> git diff --src-tree=HEAD --dst-index [--] \n> git diff --src-tree=maint --dst-tree=master [--] \n>\n> The above three would be ugly ways to spell the traditional \"short\n> hands\", e.g.\n>\n> git diff [--] \n> git diff --cached [--] \n> git diff maint master [--] \n>\n> respectively. And an obvious fallout of the above is this command\n> line:\n>\n> git diff --src-path=A --dst-path=B [--] \n>\n> Because this does not involve any index, tree or worktree, it cannot\n> be a Git operation, so we know that is --no-index mode; the command\n> line does not need to say --no-index anywhere if we go that route.\n\nSounds a lot like Jeff's \"git-put\", except with -- syntax instead of a\nspecial one, and you only look at the diff, not move any content. Also\npeople who would go with \"git diff --no-index A B C D E F...\" usually\nscript it, not type it manually. And stdin would fit better than\ncommand line arguments.\n-- \nDuy\n"}]}