{"thread":{"id":"50072","subject":"[PATCH] diff: add support for reading files literally with --no-index","startedAt":"2018-12-20T00:26:21Z","lastAt":"2019-01-05T17:39:55Z","messageCount":18,"participants":["brian m. carlson","Jeff King","Duy Nguyen","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365614","messageId":"20181220002610.43832-1-sandals@crustytoothpaste.net","threadId":"50072","inReplyTo":null,"subject":"[PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-20T00:26:10Z","receivedAt":"2018-12-20T00:26:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In some shells, such as bash and zsh, it's possible to use a command\nsubstitution to provide the output of a command as a file argument to\nanother process, like so:\n\n  diff -u <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n\nHowever, this syntax does not produce useful results with git diff\n--no-index.\n\nOn macOS, the arguments to the command are named pipes under /dev/fd,\nand git diff doesn't know how to handle a named pipe. On Linux, the\narguments are symlinks to pipes, so git diff \"helpfully\" diffs these\nsymlinks, comparing their targets like \"pipe:[1234]\" and \"pipe:[5678]\".\n\nBecause this behavior is not very helpful, and because git diff has many\nfeatures that people would like to use even on non-Git files, add an\noption to git diff --no-index to read files literally, dereferencing\nsymlinks and reading them as a normal file.\n\nNote that this behavior requires that the files be read entirely into\nmemory, just as we do when reading from standard input.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nThis is a long-standing annoyance of mine, and it also makes some use\ncases possible (for example, diffing filtered and non-filtered objects).\n\nWe don't include a test for the pipe scenario because I couldn't get\nthat case to work in portable shell (although of course it works in\nbash). I have, however, tested it on both macOS and Linux. No clue how\nthis works on Windows.\n\n Documentation/git-diff.txt |  5 +++++\n diff-no-index.c            | 34 +++++++++++++++++++++++++++-------\n diff.c                     | 24 +++++++++++++-----------\n diff.h                     |  1 +\n diffcore.h                 |  1 +\n t/t4053-diff-no-index.sh   | 28 ++++++++++++++++++++++++++++\n 6 files changed, 75 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 030f162f30..4c4695c88d 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -111,6 +111,11 @@ include::diff-options.txt[]\n \t\"Unmerged\".  Can be used only when comparing the working tree\n \twith the index.\n \n+--literally::\n+  Read the specified files literally, as `diff` would,\n+  dereferencing any symlinks and reading data from pipes.\n+  This option only works with `--no-index`.\n+\n <path>...::\n \tThe <paths> parameters, when given, are used to limit\n \tthe diff to the named paths (you can give directory\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9414e922d1..2707206aee 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)\n \treturn 0;\n }\n \n-static struct diff_filespec *noindex_filespec(const char *name, int mode)\n+static int populate_literally(struct diff_filespec *s)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t size = 0;\n+\tint fd = xopen(s->path, O_RDONLY);\n+\n+\tif (strbuf_read(&buf, fd, 0) < 0)\n+\t\treturn error_errno(\"error while reading from '%s'\", s->path);\n+\n+\ts->should_munmap = 0;\n+\ts->data = strbuf_detach(&buf, &size);\n+\ts->size = size;\n+\ts->should_free = 1;\n+\ts->read_literally = 1;\n+\treturn 0;\n+}\n+\n+static struct diff_filespec *noindex_filespec(const char *name, int mode,\n+\t\t\t\t\t      struct diff_options *o)\n {\n \tstruct diff_filespec *s;\n \n@@ -85,6 +103,8 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\n \tfill_filespec(s, &null_oid, 0, mode);\n \tif (name == file_from_standard_input)\n \t\tpopulate_from_stdin(s);\n+\telse if (o->flags.read_literally)\n+\t\tpopulate_literally(s);\n \treturn s;\n }\n \n@@ -101,14 +121,14 @@ static int queue_diff(struct diff_options *o,\n \n \t\tif (S_ISDIR(mode1)) {\n \t\t\t/* 2 is file that is created */\n-\t\t\td1 = noindex_filespec(NULL, 0);\n-\t\t\td2 = noindex_filespec(name2, mode2);\n+\t\t\td1 = noindex_filespec(NULL, 0, o);\n+\t\t\td2 = noindex_filespec(name2, mode2, o);\n \t\t\tname2 = NULL;\n \t\t\tmode2 = 0;\n \t\t} else {\n \t\t\t/* 1 is file that is deleted */\n-\t\t\td1 = noindex_filespec(name1, mode1);\n-\t\t\td2 = noindex_filespec(NULL, 0);\n+\t\t\td1 = noindex_filespec(name1, mode1, o);\n+\t\t\td2 = noindex_filespec(NULL, 0, o);\n \t\t\tname1 = NULL;\n \t\t\tmode1 = 0;\n \t\t}\n@@ -189,8 +209,8 @@ static int queue_diff(struct diff_options *o,\n \t\t\tSWAP(name1, name2);\n \t\t}\n \n-\t\td1 = noindex_filespec(name1, mode1);\n-\t\td2 = noindex_filespec(name2, mode2);\n+\t\td1 = noindex_filespec(name1, mode1, o);\n+\t\td2 = noindex_filespec(name2, mode2, o);\n \t\tdiff_queue(&diff_queued_diff, d1, d2);\n \t\treturn 0;\n \t}\ndiff --git a/diff.c b/diff.c\nindex dc9965e836..740d0087b9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4282,18 +4282,18 @@ static void run_diff_cmd(const char *pgm,\n \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n }\n \n-static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *istate)\n+static void diff_fill_oid_info(struct diff_filespec *one, struct diff_options *o)\n {\n \tif (DIFF_FILE_VALID(one)) {\n \t\tif (!one->oid_valid) {\n \t\t\tstruct stat st;\n-\t\t\tif (one->is_stdin) {\n+\t\t\tif (one->is_stdin || one->read_literally) {\n \t\t\t\toidclr(&one->oid);\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tif (lstat(one->path, &st) < 0)\n \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n-\t\t\tif (index_path(istate, &one->oid, one->path, &st, 0))\n+\t\t\tif (index_path(o->repo->index, &one->oid, one->path, &st, 0))\n \t\t\t\tdie(\"cannot hash %s\", one->path);\n \t\t}\n \t}\n@@ -4341,8 +4341,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n \t\treturn;\n \t}\n \n-\tdiff_fill_oid_info(one, o->repo->index);\n-\tdiff_fill_oid_info(two, o->repo->index);\n+\tdiff_fill_oid_info(one, o);\n+\tdiff_fill_oid_info(two, o);\n \n \tif (!pgm &&\n \t    DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&\n@@ -4389,8 +4389,8 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n-\tdiff_fill_oid_info(p->one, o->repo->index);\n-\tdiff_fill_oid_info(p->two, o->repo->index);\n+\tdiff_fill_oid_info(p->one, o);\n+\tdiff_fill_oid_info(p->two, o);\n \n \tbuiltin_diffstat(name, other, p->one, p->two,\n \t\t\t diffstat, o, p);\n@@ -4414,8 +4414,8 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n-\tdiff_fill_oid_info(p->one, o->repo->index);\n-\tdiff_fill_oid_info(p->two, o->repo->index);\n+\tdiff_fill_oid_info(p->one, o);\n+\tdiff_fill_oid_info(p->two, o);\n \n \tbuiltin_checkdiff(name, other, attr_path, p->one, p->two, o);\n }\n@@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->flags.funccontext = 1;\n \telse if (!strcmp(arg, \"--no-function-context\"))\n \t\toptions->flags.funccontext = 0;\n+\telse if (!strcmp(arg, \"--literally\"))\n+\t\toptions->flags.read_literally = 1;\n \telse if ((argcount = parse_long_opt(\"output\", av, &optarg))) {\n \t\tchar *path = prefix_filename(prefix, optarg);\n \t\toptions->file = xfopen(path, \"w\");\n@@ -5720,8 +5722,8 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\tif (DIFF_PAIR_UNMERGED(p))\n \t\t\tcontinue;\n \n-\t\tdiff_fill_oid_info(p->one, options->repo->index);\n-\t\tdiff_fill_oid_info(p->two, options->repo->index);\n+\t\tdiff_fill_oid_info(p->one, options);\n+\t\tdiff_fill_oid_info(p->two, options);\n \n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\ndiff --git a/diff.h b/diff.h\nindex ce5e8a8183..7dedd3bcd1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -97,6 +97,7 @@ struct diff_flags {\n \tunsigned stat_with_summary:1;\n \tunsigned suppress_diff_headers:1;\n \tunsigned dual_color_diffed_diffs:1;\n+\tunsigned read_literally:1;\n };\n \n static inline void diff_flags_or(struct diff_flags *a,\ndiff --git a/diffcore.h b/diffcore.h\nindex b651061c0e..363869447a 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -48,6 +48,7 @@ struct diff_filespec {\n #define DIRTY_SUBMODULE_UNTRACKED 1\n #define DIRTY_SUBMODULE_MODIFIED  2\n \tunsigned is_stdin : 1;\n+\tunsigned read_literally : 1;\n \tunsigned has_more_entries : 1; /* only appear in combined diff */\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n \tsigned int is_binary : 2;\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 6e0dd6f9e5..53e6bcdc19 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -137,4 +137,32 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff --no-index --literally' '\n+\techo \"diff --git a/../../non/git/a b/../../non/git/b\" >expect &&\n+\ttest_expect_code 1 \\\n+\t\tgit -C repo/sub \\\n+\t\tdiff --literally ../../non/git/a ../../non/git/b >actual &&\n+\thead -n 1 <actual >actual.head &&\n+\ttest_cmp expect actual.head\n+'\n+\n+test_expect_success SYMLINKS 'diff --no-index --literally with symlinks' '\n+\ttest_write_lines a b c >f1 &&\n+\ttest_write_lines a d c >f2 &&\n+\tln -s f1 s1 &&\n+\tln -s f2 s2 &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/s1 b/s2\n+\t--- a/s1\n+\t+++ b/s2\n+\t@@ -1,3 +1,3 @@\n+\t a\n+\t-b\n+\t+d\n+\t c\n+\tEOF\n+\ttest_expect_code 1 git diff --no-index --literally s1 s2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"365647","messageId":"20181220154841.GE27361@sigill.intra.peff.net","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T15:48:41Z","receivedAt":"2018-12-20T15:48:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 12:26:10AM +0000, brian m. carlson wrote:\n\n> In some shells, such as bash and zsh, it's possible to use a command\n> substitution to provide the output of a command as a file argument to\n> another process, like so:\n> \n>   diff -u <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n> \n> However, this syntax does not produce useful results with git diff\n> --no-index.\n> \n> On macOS, the arguments to the command are named pipes under /dev/fd,\n> and git diff doesn't know how to handle a named pipe. On Linux, the\n> arguments are symlinks to pipes, so git diff \"helpfully\" diffs these\n> symlinks, comparing their targets like \"pipe:[1234]\" and \"pipe:[5678]\".\n> \n> Because this behavior is not very helpful, and because git diff has many\n> features that people would like to use even on non-Git files, add an\n> option to git diff --no-index to read files literally, dereferencing\n> symlinks and reading them as a normal file.\n> \n> Note that this behavior requires that the files be read entirely into\n> memory, just as we do when reading from standard input.\n\nYeah, I think this is a useful feature to have. It seemed familiar, and\nindeed there were some patches a while back:\n\n  https://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/\n\nCompared to that series, the implementation here seems simpler and less\nlikely to have unexpected effects.\n\nReading the patch, one thing I _thought_ it did (but it does not) is to\nimpact only the actual arguments on the command line, not entries we\nfind from recursing trees.\n\nI.e., if I said:\n\n  ln -s bar a/foo\n  ln -s baz b/foo\n  git diff --no-index --literally a/foo b/foo\n  git diff --no-index --literally a/ b/\n\nshould I still see a/foo as a symlink in the second one, or not?\n\nThe distinction is a bit subtle, but I think treating only the actual\ntop-level arguments as symlinks would solve your problem, but still\nallow a more detailed diff for the recursive cases.\n\nThere's some more discussion on this in the earlier iteration of that\nseries:\n\n  https://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/\n\nI also suspect people might want a config option to make this the\ndefault (which should be OK, as git-diff is, after all, porcelain). But\neither way, a command line option is the first step.\n\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> This is a long-standing annoyance of mine, and it also makes some use\n> cases possible (for example, diffing filtered and non-filtered objects).\n> \n> We don't include a test for the pipe scenario because I couldn't get\n> that case to work in portable shell (although of course it works in\n> bash). I have, however, tested it on both macOS and Linux. No clue how\n> this works on Windows.\n\nI think focusing on symlinks makes sense. You could probably test pipes\nwith mkfifo, but looking at your implementation, it's pretty clear that\nit will operate on anything that works with read().\n\n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index 030f162f30..4c4695c88d 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -111,6 +111,11 @@ include::diff-options.txt[]\n>  \t\"Unmerged\".  Can be used only when comparing the working tree\n>  \twith the index.\n>  \n> +--literally::\n> +  Read the specified files literally, as `diff` would,\n> +  dereferencing any symlinks and reading data from pipes.\n> +  This option only works with `--no-index`.\n> +\n\nLooks like spaces for indent, whereas the context uses tabs.\n\nI think \"literal\" is a good way to describe this concept. If we do grow\na config option to make this the default, then countermanding it would\nbe \"--no-literally\", which parses a bit funny as English.\n\nIf you agree with my \"only the top-level\" line of reasoning above, maybe\n\"--literal-arguments\" and \"--no-literal-arguments\" might make sense.\n\nWe could also in theory offer several levels: no literals, top-level\nliterals, everything literal, at which point it becomes a tri-state.\n\n> -static struct diff_filespec *noindex_filespec(const char *name, int mode)\n> +static int populate_literally(struct diff_filespec *s)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tsize_t size = 0;\n> +\tint fd = xopen(s->path, O_RDONLY);\n> +\n> +\tif (strbuf_read(&buf, fd, 0) < 0)\n> +\t\treturn error_errno(\"error while reading from '%s'\", s->path);\n> +\n> +\ts->should_munmap = 0;\n> +\ts->data = strbuf_detach(&buf, &size);\n> +\ts->size = size;\n> +\ts->should_free = 1;\n> +\ts->read_literally = 1;\n> +\treturn 0;\n> +}\n> +\n> +static struct diff_filespec *noindex_filespec(const char *name, int mode,\n> +\t\t\t\t\t      struct diff_options *o)\n> [...]\n> +\telse if (o->flags.read_literally)\n> +\t\tpopulate_literally(s);\n\nThe implementation is pleasantly simple. If we want to do the \"only\ntop-level\" thing, we'd just have to pass in a depth flag here.\n\n> diff --git a/diff.c b/diff.c\n> index dc9965e836..740d0087b9 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4282,18 +4282,18 @@ static void run_diff_cmd(const char *pgm,\n>  \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n>  }\n>  \n> -static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *istate)\n> +static void diff_fill_oid_info(struct diff_filespec *one, struct diff_options *o)\n\nIt might be worth breaking these \"pass the options around\" hunks into a\nseparate preparatory patch.\n\n-Peff\n"},{"id":"365657","messageId":"CACsJy8CScTBbYJt_LLp-rBdmJubEQOZqkPQeszzax9YpbCPUkg@mail.gmail.com","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-12-20T17:06:11Z","receivedAt":"2018-12-20T17:06:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 20, 2018 at 1:26 AM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> @@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,\n>                 options->flags.funccontext = 1;\n>         else if (!strcmp(arg, \"--no-function-context\"))\n>                 options->flags.funccontext = 0;\n> +       else if (!strcmp(arg, \"--literally\"))\n> +               options->flags.read_literally = 1;\n\nYou probably want to check in diff_setup_done() that if\nflags.read_literally is set but flags.no_index is not, then abandon\nship and die() because --literally is not used with --no-index. Even\nwhen --no-index is implicit, flags.no_index should be set.\n\nI wonder if --follow-symlinks would be a good alternative for this\n(then if the final destination is unmmapable then we just read the\nfile whole in memory without the user asking, so it will work with\npipes). --follow-symlinks then could be made work with non-\"no-index\"\ncase too. But --literally is also ok.\n-- \nDuy\n"},{"id":"365659","messageId":"20181220171725.GB6684@sigill.intra.peff.net","threadId":"50072","inReplyTo":"CACsJy8CScTBbYJt_LLp-rBdmJubEQOZqkPQeszzax9YpbCPUkg@mail.gmail.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T17:17:25Z","receivedAt":"2018-12-20T17:17:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 06:06:11PM +0100, Duy Nguyen wrote:\n\n> On Thu, Dec 20, 2018 at 1:26 AM brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n> > @@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,\n> >                 options->flags.funccontext = 1;\n> >         else if (!strcmp(arg, \"--no-function-context\"))\n> >                 options->flags.funccontext = 0;\n> > +       else if (!strcmp(arg, \"--literally\"))\n> > +               options->flags.read_literally = 1;\n> \n> You probably want to check in diff_setup_done() that if\n> flags.read_literally is set but flags.no_index is not, then abandon\n> ship and die() because --literally is not used with --no-index. Even\n> when --no-index is implicit, flags.no_index should be set.\n\nYeah, good catch. \"git diff --literally HEAD\" should report an error.\n\n> I wonder if --follow-symlinks would be a good alternative for this\n> (then if the final destination is unmmapable then we just read the\n> file whole in memory without the user asking, so it will work with\n> pipes). --follow-symlinks then could be made work with non-\"no-index\"\n> case too. But --literally is also ok.\n\nIt's more than symlinks, though. Reading from a named pipe, we'd want to\nsee the actual contents with --literally (and not \"oops, I don't know\nhow to represent a named pipe\").\n\n-Peff\n"},{"id":"365660","messageId":"CACsJy8AFwSgL-YAc2YU2XfqKFkXf-W+2V7tMy3ZOYvm_zhv5Bg@mail.gmail.com","threadId":"50072","inReplyTo":"20181220171725.GB6684@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-12-20T17:23:42Z","receivedAt":"2018-12-20T17:24:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:\n> > I wonder if --follow-symlinks would be a good alternative for this\n> > (then if the final destination is unmmapable then we just read the\n> > file whole in memory without the user asking, so it will work with\n> > pipes). --follow-symlinks then could be made work with non-\"no-index\"\n> > case too. But --literally is also ok.\n>\n> It's more than symlinks, though. Reading from a named pipe, we'd want to\n> see the actual contents with --literally (and not \"oops, I don't know\n> how to represent a named pipe\").\n\nYes, but I think at least --no-index it makes sense to just fall back\nto read() if we can't mmap(). mmap is more of an optimization than a\nrequirement. There's no loss going from \"oops I don't know how to\nrepresent it\" to \"here's the content from whatever what that device\nis\". Symlinks are different because we have two possible\nrepresentations and the user should choose.\n-- \nDuy\n"},{"id":"365661","messageId":"20181220173204.GC6684@sigill.intra.peff.net","threadId":"50072","inReplyTo":"CACsJy8AFwSgL-YAc2YU2XfqKFkXf-W+2V7tMy3ZOYvm_zhv5Bg@mail.gmail.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T17:32:05Z","receivedAt":"2018-12-20T17:32:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 06:23:42PM +0100, Duy Nguyen wrote:\n\n> On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:\n> > > I wonder if --follow-symlinks would be a good alternative for this\n> > > (then if the final destination is unmmapable then we just read the\n> > > file whole in memory without the user asking, so it will work with\n> > > pipes). --follow-symlinks then could be made work with non-\"no-index\"\n> > > case too. But --literally is also ok.\n> >\n> > It's more than symlinks, though. Reading from a named pipe, we'd want to\n> > see the actual contents with --literally (and not \"oops, I don't know\n> > how to represent a named pipe\").\n> \n> Yes, but I think at least --no-index it makes sense to just fall back\n> to read() if we can't mmap(). mmap is more of an optimization than a\n> requirement. There's no loss going from \"oops I don't know how to\n> represent it\" to \"here's the content from whatever what that device\n> is\". Symlinks are different because we have two possible\n> representations and the user should choose.\n\nOh, I see. I misread your paragraph. Yeah, I think that is the behavior\nI would want by default, too, though the previous thread makes me thing\nsome people would literally rather have the \"I can't represent this\"\nbehavior (because they really do want a diff that can reconstruct the\nfilesystem state, and an error if not).\n\n-Peff\n"},{"id":"365663","messageId":"CACsJy8C6JZR=6bh0M2=9P_LD42tW1qz78+xL3aqTw5A0fJCrqg@mail.gmail.com","threadId":"50072","inReplyTo":"20181220173204.GC6684@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-12-20T17:37:07Z","receivedAt":"2018-12-20T17:37:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 20, 2018 at 6:32 PM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Dec 20, 2018 at 06:23:42PM +0100, Duy Nguyen wrote:\n>\n> > On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:\n> > > > I wonder if --follow-symlinks would be a good alternative for this\n> > > > (then if the final destination is unmmapable then we just read the\n> > > > file whole in memory without the user asking, so it will work with\n> > > > pipes). --follow-symlinks then could be made work with non-\"no-index\"\n> > > > case too. But --literally is also ok.\n> > >\n> > > It's more than symlinks, though. Reading from a named pipe, we'd want to\n> > > see the actual contents with --literally (and not \"oops, I don't know\n> > > how to represent a named pipe\").\n> >\n> > Yes, but I think at least --no-index it makes sense to just fall back\n> > to read() if we can't mmap(). mmap is more of an optimization than a\n> > requirement. There's no loss going from \"oops I don't know how to\n> > represent it\" to \"here's the content from whatever what that device\n> > is\". Symlinks are different because we have two possible\n> > representations and the user should choose.\n>\n> Oh, I see. I misread your paragraph. Yeah, I think that is the behavior\n> I would want by default, too, though the previous thread makes me thing\n> some people would literally rather have the \"I can't represent this\"\n> behavior (because they really do want a diff that can reconstruct the\n> filesystem state, and an error if not).\n\nI can't see a good use case for that. But yeah it would not harm to be\na bit more conservative, then make --literally default later and leave\n--no-literally as an escape hatch.\n-- \nDuy\n"},{"id":"365704","messageId":"871s6bg9hx.fsf@evledraar.gmail.com","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-20T21:43:06Z","receivedAt":"2018-12-20T21:43:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 20 2018, brian m. carlson wrote:\n\n> We don't include a test for the pipe scenario because I couldn't get\n> that case to work in portable shell (although of course it works in\n> bash). I have, however, tested it on both macOS and Linux. No clue how\n> this works on Windows.\n\nYou can (and should) add tests using the PROCESS_SUBSTITUTION prereq\nDennis used in an earlier version of this:\nhttps://public-inbox.org/git/20170113102021.6054-3-dennis@kaarsemaker.net/\n\nEven if that wasn't possible a bash-specific test is better than no\ntest, and can be had by including t/lib-bash.sh in your test, see\ne.g. the completion tests.\n"},{"id":"365712","messageId":"20181220235438.GE35965@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"871s6bg9hx.fsf@evledraar.gmail.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-20T23:54:38Z","receivedAt":"2018-12-20T23:54:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Dec 20, 2018 at 10:43:06PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Dec 20 2018, brian m. carlson wrote:\n> \n> > We don't include a test for the pipe scenario because I couldn't get\n> > that case to work in portable shell (although of course it works in\n> > bash). I have, however, tested it on both macOS and Linux. No clue how\n> > this works on Windows.\n> \n> You can (and should) add tests using the PROCESS_SUBSTITUTION prereq\n> Dennis used in an earlier version of this:\n> https://public-inbox.org/git/20170113102021.6054-3-dennis@kaarsemaker.net/\n\nI'm happy to steal that code. I didn't know this had come up before, so\nI didn't think of it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365715","messageId":"20181221002527.GF35965@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"20181220154841.GE27361@sigill.intra.peff.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-21T00:25:28Z","receivedAt":"2018-12-21T00:25:37Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Dec 20, 2018 at 10:48:41AM -0500, Jeff King wrote:\n> The distinction is a bit subtle, but I think treating only the actual\n> top-level arguments as symlinks would solve your problem, but still\n> allow a more detailed diff for the recursive cases.\n\nYeah, I think that would be better. I'll add a test.\n\n> Looks like spaces for indent, whereas the context uses tabs.\n\nWill fix.\n\n> I think \"literal\" is a good way to describe this concept. If we do grow\n> a config option to make this the default, then countermanding it would\n> be \"--no-literally\", which parses a bit funny as English.\n> \n> If you agree with my \"only the top-level\" line of reasoning above, maybe\n> \"--literal-arguments\" and \"--no-literal-arguments\" might make sense.\n> \n> We could also in theory offer several levels: no literals, top-level\n> literals, everything literal, at which point it becomes a tri-state.\n\nYeah, this is what the POSIX symlink options (-H, -L, -P) do. I\noriginally came up with \"--dereference\" as the name, but I decided to\nchange it, because it doesn't affect just symlinks. I think\n--literal-arguments is better.\n\nTheoretically, we could adopt the POSIX options for symlink/pipe\nhandling if we want to in the future, but I think that's a decision we\nshould make later.\n\n> > diff --git a/diff.c b/diff.c\n> > index dc9965e836..740d0087b9 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -4282,18 +4282,18 @@ static void run_diff_cmd(const char *pgm,\n> >  \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n> >  }\n> >  \n> > -static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *istate)\n> > +static void diff_fill_oid_info(struct diff_filespec *one, struct diff_options *o)\n> \n> It might be worth breaking these \"pass the options around\" hunks into a\n> separate preparatory patch.\n\nI can do that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"365726","messageId":"nycvar.QRO.7.76.6.1812211246050.41@tvgsbejvaqbjf.bet","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-21T11:52:04Z","receivedAt":"2018-12-21T11:52:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Thu, 20 Dec 2018, brian m. carlson wrote:\n\n> In some shells, such as bash and zsh, it's possible to use a command\n> substitution to provide the output of a command as a file argument to\n> another process, like so:\n> \n>   diff -u <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n> \n> However, this syntax does not produce useful results with git diff\n> --no-index.\n> \n> On macOS, the arguments to the command are named pipes under /dev/fd,\n> and git diff doesn't know how to handle a named pipe. On Linux, the\n> arguments are symlinks to pipes, so git diff \"helpfully\" diffs these\n> symlinks, comparing their targets like \"pipe:[1234]\" and \"pipe:[5678]\".\n\n\n... and in Git for Windows' Bash, it would result in something like:\n\n\t$ git -P diff --no-index <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n\terror: Could not access '/proc/24012/fd/63'\n\n... because the Bash is \"MSYS2-aware\" and knows about `/proc/` while\n`git.exe` is a pure Win32 executable that has no idea what Bash is talking\nabout.\n\nSadly, your patch does not change the situation one bit (because it does\nnot change the fact that the MSYS2 Bash passes a path to `git.exe` that is\nnot a valid Windows path, and neither could it, but that's not the problem\nof your patch).\n\nI reviewed your patch and it looks good to me!\n\nThanks,\nDscho\n\n> Because this behavior is not very helpful, and because git diff has many\n> features that people would like to use even on non-Git files, add an\n> option to git diff --no-index to read files literally, dereferencing\n> symlinks and reading them as a normal file.\n> \n> Note that this behavior requires that the files be read entirely into\n> memory, just as we do when reading from standard input.\n> \n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> This is a long-standing annoyance of mine, and it also makes some use\n> cases possible (for example, diffing filtered and non-filtered objects).\n> \n> We don't include a test for the pipe scenario because I couldn't get\n> that case to work in portable shell (although of course it works in\n> bash). I have, however, tested it on both macOS and Linux. No clue how\n> this works on Windows.\n> \n>  Documentation/git-diff.txt |  5 +++++\n>  diff-no-index.c            | 34 +++++++++++++++++++++++++++-------\n>  diff.c                     | 24 +++++++++++++-----------\n>  diff.h                     |  1 +\n>  diffcore.h                 |  1 +\n>  t/t4053-diff-no-index.sh   | 28 ++++++++++++++++++++++++++++\n>  6 files changed, 75 insertions(+), 18 deletions(-)\n> \n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index 030f162f30..4c4695c88d 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -111,6 +111,11 @@ include::diff-options.txt[]\n>  \t\"Unmerged\".  Can be used only when comparing the working tree\n>  \twith the index.\n>  \n> +--literally::\n> +  Read the specified files literally, as `diff` would,\n> +  dereferencing any symlinks and reading data from pipes.\n> +  This option only works with `--no-index`.\n> +\n>  <path>...::\n>  \tThe <paths> parameters, when given, are used to limit\n>  \tthe diff to the named paths (you can give directory\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9414e922d1..2707206aee 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)\n>  \treturn 0;\n>  }\n>  \n> -static struct diff_filespec *noindex_filespec(const char *name, int mode)\n> +static int populate_literally(struct diff_filespec *s)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tsize_t size = 0;\n> +\tint fd = xopen(s->path, O_RDONLY);\n> +\n> +\tif (strbuf_read(&buf, fd, 0) < 0)\n> +\t\treturn error_errno(\"error while reading from '%s'\", s->path);\n> +\n> +\ts->should_munmap = 0;\n> +\ts->data = strbuf_detach(&buf, &size);\n> +\ts->size = size;\n> +\ts->should_free = 1;\n> +\ts->read_literally = 1;\n> +\treturn 0;\n> +}\n> +\n> +static struct diff_filespec *noindex_filespec(const char *name, int mode,\n> +\t\t\t\t\t      struct diff_options *o)\n>  {\n>  \tstruct diff_filespec *s;\n>  \n> @@ -85,6 +103,8 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\n>  \tfill_filespec(s, &null_oid, 0, mode);\n>  \tif (name == file_from_standard_input)\n>  \t\tpopulate_from_stdin(s);\n> +\telse if (o->flags.read_literally)\n> +\t\tpopulate_literally(s);\n>  \treturn s;\n>  }\n>  \n> @@ -101,14 +121,14 @@ static int queue_diff(struct diff_options *o,\n>  \n>  \t\tif (S_ISDIR(mode1)) {\n>  \t\t\t/* 2 is file that is created */\n> -\t\t\td1 = noindex_filespec(NULL, 0);\n> -\t\t\td2 = noindex_filespec(name2, mode2);\n> +\t\t\td1 = noindex_filespec(NULL, 0, o);\n> +\t\t\td2 = noindex_filespec(name2, mode2, o);\n>  \t\t\tname2 = NULL;\n>  \t\t\tmode2 = 0;\n>  \t\t} else {\n>  \t\t\t/* 1 is file that is deleted */\n> -\t\t\td1 = noindex_filespec(name1, mode1);\n> -\t\t\td2 = noindex_filespec(NULL, 0);\n> +\t\t\td1 = noindex_filespec(name1, mode1, o);\n> +\t\t\td2 = noindex_filespec(NULL, 0, o);\n>  \t\t\tname1 = NULL;\n>  \t\t\tmode1 = 0;\n>  \t\t}\n> @@ -189,8 +209,8 @@ static int queue_diff(struct diff_options *o,\n>  \t\t\tSWAP(name1, name2);\n>  \t\t}\n>  \n> -\t\td1 = noindex_filespec(name1, mode1);\n> -\t\td2 = noindex_filespec(name2, mode2);\n> +\t\td1 = noindex_filespec(name1, mode1, o);\n> +\t\td2 = noindex_filespec(name2, mode2, o);\n>  \t\tdiff_queue(&diff_queued_diff, d1, d2);\n>  \t\treturn 0;\n>  \t}\n> diff --git a/diff.c b/diff.c\n> index dc9965e836..740d0087b9 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4282,18 +4282,18 @@ static void run_diff_cmd(const char *pgm,\n>  \t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n>  }\n>  \n> -static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *istate)\n> +static void diff_fill_oid_info(struct diff_filespec *one, struct diff_options *o)\n>  {\n>  \tif (DIFF_FILE_VALID(one)) {\n>  \t\tif (!one->oid_valid) {\n>  \t\t\tstruct stat st;\n> -\t\t\tif (one->is_stdin) {\n> +\t\t\tif (one->is_stdin || one->read_literally) {\n>  \t\t\t\toidclr(&one->oid);\n>  \t\t\t\treturn;\n>  \t\t\t}\n>  \t\t\tif (lstat(one->path, &st) < 0)\n>  \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n> -\t\t\tif (index_path(istate, &one->oid, one->path, &st, 0))\n> +\t\t\tif (index_path(o->repo->index, &one->oid, one->path, &st, 0))\n>  \t\t\t\tdie(\"cannot hash %s\", one->path);\n>  \t\t}\n>  \t}\n> @@ -4341,8 +4341,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n>  \t\treturn;\n>  \t}\n>  \n> -\tdiff_fill_oid_info(one, o->repo->index);\n> -\tdiff_fill_oid_info(two, o->repo->index);\n> +\tdiff_fill_oid_info(one, o);\n> +\tdiff_fill_oid_info(two, o);\n>  \n>  \tif (!pgm &&\n>  \t    DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&\n> @@ -4389,8 +4389,8 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n>  \tif (o->prefix_length)\n>  \t\tstrip_prefix(o->prefix_length, &name, &other);\n>  \n> -\tdiff_fill_oid_info(p->one, o->repo->index);\n> -\tdiff_fill_oid_info(p->two, o->repo->index);\n> +\tdiff_fill_oid_info(p->one, o);\n> +\tdiff_fill_oid_info(p->two, o);\n>  \n>  \tbuiltin_diffstat(name, other, p->one, p->two,\n>  \t\t\t diffstat, o, p);\n> @@ -4414,8 +4414,8 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)\n>  \tif (o->prefix_length)\n>  \t\tstrip_prefix(o->prefix_length, &name, &other);\n>  \n> -\tdiff_fill_oid_info(p->one, o->repo->index);\n> -\tdiff_fill_oid_info(p->two, o->repo->index);\n> +\tdiff_fill_oid_info(p->one, o);\n> +\tdiff_fill_oid_info(p->two, o);\n>  \n>  \tbuiltin_checkdiff(name, other, attr_path, p->one, p->two, o);\n>  }\n> @@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,\n>  \t\toptions->flags.funccontext = 1;\n>  \telse if (!strcmp(arg, \"--no-function-context\"))\n>  \t\toptions->flags.funccontext = 0;\n> +\telse if (!strcmp(arg, \"--literally\"))\n> +\t\toptions->flags.read_literally = 1;\n>  \telse if ((argcount = parse_long_opt(\"output\", av, &optarg))) {\n>  \t\tchar *path = prefix_filename(prefix, optarg);\n>  \t\toptions->file = xfopen(path, \"w\");\n> @@ -5720,8 +5722,8 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n>  \t\tif (DIFF_PAIR_UNMERGED(p))\n>  \t\t\tcontinue;\n>  \n> -\t\tdiff_fill_oid_info(p->one, options->repo->index);\n> -\t\tdiff_fill_oid_info(p->two, options->repo->index);\n> +\t\tdiff_fill_oid_info(p->one, options);\n> +\t\tdiff_fill_oid_info(p->two, options);\n>  \n>  \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n>  \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n> diff --git a/diff.h b/diff.h\n> index ce5e8a8183..7dedd3bcd1 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -97,6 +97,7 @@ struct diff_flags {\n>  \tunsigned stat_with_summary:1;\n>  \tunsigned suppress_diff_headers:1;\n>  \tunsigned dual_color_diffed_diffs:1;\n> +\tunsigned read_literally:1;\n>  };\n>  \n>  static inline void diff_flags_or(struct diff_flags *a,\n> diff --git a/diffcore.h b/diffcore.h\n> index b651061c0e..363869447a 100644\n> --- a/diffcore.h\n> +++ b/diffcore.h\n> @@ -48,6 +48,7 @@ struct diff_filespec {\n>  #define DIRTY_SUBMODULE_UNTRACKED 1\n>  #define DIRTY_SUBMODULE_MODIFIED  2\n>  \tunsigned is_stdin : 1;\n> +\tunsigned read_literally : 1;\n>  \tunsigned has_more_entries : 1; /* only appear in combined diff */\n>  \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n>  \tsigned int is_binary : 2;\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 6e0dd6f9e5..53e6bcdc19 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -137,4 +137,32 @@ test_expect_success 'diff --no-index from repo subdir with absolute paths' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'diff --no-index --literally' '\n> +\techo \"diff --git a/../../non/git/a b/../../non/git/b\" >expect &&\n> +\ttest_expect_code 1 \\\n> +\t\tgit -C repo/sub \\\n> +\t\tdiff --literally ../../non/git/a ../../non/git/b >actual &&\n> +\thead -n 1 <actual >actual.head &&\n> +\ttest_cmp expect actual.head\n> +'\n> +\n> +test_expect_success SYMLINKS 'diff --no-index --literally with symlinks' '\n> +\ttest_write_lines a b c >f1 &&\n> +\ttest_write_lines a d c >f2 &&\n> +\tln -s f1 s1 &&\n> +\tln -s f2 s2 &&\n> +\tcat >expect <<-\\EOF &&\n> +\tdiff --git a/s1 b/s2\n> +\t--- a/s1\n> +\t+++ b/s2\n> +\t@@ -1,3 +1,3 @@\n> +\t a\n> +\t-b\n> +\t+d\n> +\t c\n> +\tEOF\n> +\ttest_expect_code 1 git diff --no-index --literally s1 s2 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> \n"},{"id":"365752","messageId":"20181221232025.GA10611@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"nycvar.QRO.7.76.6.1812211246050.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-21T23:20:25Z","receivedAt":"2018-12-21T23:21:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Dec 21, 2018 at 12:52:04PM +0100, Johannes Schindelin wrote:\n> ... and in Git for Windows' Bash, it would result in something like:\n> \n> \t$ git -P diff --no-index <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n> \terror: Could not access '/proc/24012/fd/63'\n> \n> ... because the Bash is \"MSYS2-aware\" and knows about `/proc/` while\n> `git.exe` is a pure Win32 executable that has no idea what Bash is talking\n> about.\n> \n> Sadly, your patch does not change the situation one bit (because it does\n> not change the fact that the MSYS2 Bash passes a path to `git.exe` that is\n> not a valid Windows path, and neither could it, but that's not the problem\n> of your patch).\n\nThat tells me that I need to exclude Windows (excepting Cygwin) if I add\na PROCESS_SUBSTITUTION test like Ævar suggested, and that indeed is\nhelpful information. I'm sorry my patch won't be useful on Windows,\nthough.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"366050","messageId":"xmqqr2dukhw2.fsf@gitster-ct.c.googlers.com","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-02T18:56:45Z","receivedAt":"2019-01-02T18:56:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> +test_expect_success SYMLINKS 'diff --no-index --literally with symlinks' '\n> +\ttest_write_lines a b c >f1 &&\n> +\ttest_write_lines a d c >f2 &&\n> +\tln -s f1 s1 &&\n> +\tln -s f2 s2 &&\n> +\tcat >expect <<-\\EOF &&\n> +\tdiff --git a/s1 b/s2\n> +\t--- a/s1\n> +\t+++ b/s2\n> +\t@@ -1,3 +1,3 @@\n> +\t a\n> +\t-b\n> +\t+d\n> +\t c\n> +\tEOF\n> +\ttest_expect_code 1 git diff --no-index --literally s1 s2 >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis is good as a goal, but the implementation seems to be overly\neager to dereference any symlink or non-regular file found in any\nlevel of recursion.  The use case presented as the justification in\nthe proposed log message, and the explanation in the documentation,\nsuggests that only the paths given from the command line are treated\nthis way.\n\nIt may make sense to do this as two orthgonal flags.  Dereference\nsymlinks and named pipes given from the command line is one use\ncase.  Dereference any symlinks encountered during recursion is\nanother.  And the latter might be useful even inside a repository\nas an option, even though the former would never make sense unless\nrunning in --no-index mode.\n\nThanks.\n\n\n"},{"id":"366126","messageId":"20190104020808.GF423984@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"xmqqr2dukhw2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-01-04T02:08:08Z","receivedAt":"2019-01-04T02:12:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Jan 02, 2019 at 10:56:45AM -0800, Junio C Hamano wrote:\n> This is good as a goal, but the implementation seems to be overly\n> eager to dereference any symlink or non-regular file found in any\n> level of recursion.  The use case presented as the justification in\n> the proposed log message, and the explanation in the documentation,\n> suggests that only the paths given from the command line are treated\n> this way.\n> \n> It may make sense to do this as two orthgonal flags.  Dereference\n> symlinks and named pipes given from the command line is one use\n> case.  Dereference any symlinks encountered during recursion is\n> another.  And the latter might be useful even inside a repository\n> as an option, even though the former would never make sense unless\n> running in --no-index mode.\n\nYeah, I definitely think I'll be doing a reroll to address the recursion\nissue as Peff suggested. I've just been on vacation and will be\ntraveling again soon, so I haven't gotten to it yet. Feel free to wait\nfor the reroll before picking it up.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"366127","messageId":"20190104021855.GB97188@google.com","threadId":"50072","inReplyTo":"20181220002610.43832-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-04T02:18:55Z","receivedAt":"2019-01-04T02:19:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n\n> In some shells, such as bash and zsh, it's possible to use a command\n> substitution to provide the output of a command as a file argument to\n> another process, like so:\n>\n>   diff -u <(printf \"a\\nb\\n\") <(printf \"a\\nc\\n\")\n>\n> However, this syntax does not produce useful results with git diff\n> --no-index.\n\nThanks much for fixing this.  It's something I've run into, too.\n\n[...]\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -111,6 +111,11 @@ include::diff-options.txt[]\n>  \t\"Unmerged\".  Can be used only when comparing the working tree\n>  \twith the index.\n>  \n> +--literally::\n> +  Read the specified files literally, as `diff` would,\n> +  dereferencing any symlinks and reading data from pipes.\n> +  This option only works with `--no-index`.\n\nI may be a minority in this opinion, but I had trouble understanding\nwhat --literally would do from its name.  I suspect we can come up\nwith a better name.\n\nUnfortunately, I'm terrible at coming up with names. :-P\n\n--dereference would be a good name when it comes to symlinks, but\nit's not a good name for reading what is on the other side of a pipe.\nOn the plus side, it matches \"diff\" and \"cp\"'s name for the \"follow\nsymbolic links\" option.\n\n--plain captures the desire a little better --- we want a plain\nread(2) from the file instead of trying to be smart and look at\nwhether it is e.g. a block device.  But in the context of \"diff\", that\nwould seem more like an option that affects the output.\n\nWhat would you think of\n\n - always reading from fifos instead of describing them, since I've\n   never encountered a use case where people want the latter\n\n - --dereference to control whether to follow symlinks\n\n?\n\n[...]\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)\n>  \treturn 0;\n>  }\n>  \n> -static struct diff_filespec *noindex_filespec(const char *name, int mode)\n> +static int populate_literally(struct diff_filespec *s)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tsize_t size = 0;\n> +\tint fd = xopen(s->path, O_RDONLY);\n> +\n> +\tif (strbuf_read(&buf, fd, 0) < 0)\n> +\t\treturn error_errno(\"error while reading from '%s'\", s->path);\n> +\n> +\ts->should_munmap = 0;\n> +\ts->data = strbuf_detach(&buf, &size);\n> +\ts->size = size;\n> +\ts->should_free = 1;\n> +\ts->read_literally = 1;\n\nOh!  --read-literally works perfectly for me as a name. :)\n\nJonathan\n"},{"id":"366129","messageId":"20190104025724.GG423984@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"20190104021855.GB97188@google.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-01-04T02:57:25Z","receivedAt":"2019-01-04T02:57:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Jan 03, 2019 at 06:18:55PM -0800, Jonathan Nieder wrote:\n> I may be a minority in this opinion, but I had trouble understanding\n> what --literally would do from its name.  I suspect we can come up\n> with a better name.\n> \n> Unfortunately, I'm terrible at coming up with names. :-P\n> \n> --dereference would be a good name when it comes to symlinks, but\n> it's not a good name for reading what is on the other side of a pipe.\n> On the plus side, it matches \"diff\" and \"cp\"'s name for the \"follow\n> symbolic links\" option.\n> \n> --plain captures the desire a little better --- we want a plain\n> read(2) from the file instead of trying to be smart and look at\n> whether it is e.g. a block device.  But in the context of \"diff\", that\n> would seem more like an option that affects the output.\n> \n> What would you think of\n> \n>  - always reading from fifos instead of describing them, since I've\n>    never encountered a use case where people want the latter\n> \n>  - --dereference to control whether to follow symlinks\n\nThis is actually surprisingly difficult. The reason I implemented this\nonly for no-index mode is because there are actually several places we\ncan stat a file in the diff code, and implementing a --dereference\noption that catches all of those cases and getting the option passed\ndown to them is non-trivial.\n\nSince my primary environment is on Linux, I need to implement functional\n--dereference semantics before implementing the FIFO functionality to\nknow if it works in my use case.\n\nI agree that would be a nicer approach overall, so let me see if I can\ntake another stab at it in a way that isn't excessively complex. It\nwould also make the name less controversial, since \"--dereference\" is\nthe obvious choice for dereferencing symlinks.\n\n> [...]\n> > --- a/diff-no-index.c\n> > +++ b/diff-no-index.c\n> > @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)\n> >  \treturn 0;\n> >  }\n> >  \n> > -static struct diff_filespec *noindex_filespec(const char *name, int mode)\n> > +static int populate_literally(struct diff_filespec *s)\n> > +{\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tsize_t size = 0;\n> > +\tint fd = xopen(s->path, O_RDONLY);\n> > +\n> > +\tif (strbuf_read(&buf, fd, 0) < 0)\n> > +\t\treturn error_errno(\"error while reading from '%s'\", s->path);\n> > +\n> > +\ts->should_munmap = 0;\n> > +\ts->data = strbuf_detach(&buf, &size);\n> > +\ts->size = size;\n> > +\ts->should_free = 1;\n> > +\ts->read_literally = 1;\n> \n> Oh!  --read-literally works perfectly for me as a name. :)\n\nIf I can't manage to split this out into two pieces, I'll pick that as\nthe name for the reroll.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"366156","messageId":"xmqq36q8cjgf.fsf@gitster-ct.c.googlers.com","threadId":"50072","inReplyTo":"20190104025724.GG423984@genre.crustytoothpaste.net","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T19:26:56Z","receivedAt":"2019-01-04T19:27:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>>  - --dereference to control whether to follow symlinks\n>\n> This is actually surprisingly difficult. The reason I implemented this\n> only for no-index mode is because there are actually several places we\n> can stat a file in the diff code, and implementing a --dereference\n> option that catches all of those cases and getting the option passed\n> down to them is non-trivial.\n\nAnother thing to worry about is symlinks that point outside the\nworking tree.  When a tracked content \"dir/link\" is a symlink to\n\"/etc/motd\", it probably makes sense to open(\"/etc/motd\") and read()\nit on the working tree side of the diff, and probably even on the\nindex side of the diff, but what about obtaining contents for\n\"dir/link\" in a year-old commit under --deference mode?  I am not\nsure if it makes sense to read from the filesystem in such a case.\n\nI personally am perfectly fine if this \"do not compare readlink(2),\nbut read contents literally\" is limited to the --no-index mode.\n\nThanks.\n"},{"id":"366178","messageId":"20190105173944.GH423984@genre.crustytoothpaste.net","threadId":"50072","inReplyTo":"xmqq36q8cjgf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: add support for reading files literally with --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-01-05T17:39:44Z","receivedAt":"2019-01-05T17:39:55Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jan 04, 2019 at 11:26:56AM -0800, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> >>  - --dereference to control whether to follow symlinks\n> >\n> > This is actually surprisingly difficult. The reason I implemented this\n> > only for no-index mode is because there are actually several places we\n> > can stat a file in the diff code, and implementing a --dereference\n> > option that catches all of those cases and getting the option passed\n> > down to them is non-trivial.\n> \n> Another thing to worry about is symlinks that point outside the\n> working tree.  When a tracked content \"dir/link\" is a symlink to\n> \"/etc/motd\", it probably makes sense to open(\"/etc/motd\") and read()\n> it on the working tree side of the diff, and probably even on the\n> index side of the diff, but what about obtaining contents for\n> \"dir/link\" in a year-old commit under --deference mode?  I am not\n> sure if it makes sense to read from the filesystem in such a case.\n> \n> I personally am perfectly fine if this \"do not compare readlink(2),\n> but read contents literally\" is limited to the --no-index mode.\n\nThat's a good point. I think I'll stick with the current design, then,\nsince that seems like the least surprising way forward. It also means\nthat we don't read outside of the working tree unless --no-index is in\nuse, which may be beneficial for security purposes.\n\nThanks for a helpful perspective.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}