{"thread":{"id":"59917","subject":"[PATCH 0/3] diff --no-index: support reading from named pipes","startedAt":"2023-06-27T14:11:16Z","lastAt":"2023-08-10T12:56:38Z","messageCount":17,"participants":["Phillip Wood","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"478883","messageId":"cover.1687874975.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":null,"subject":"[PATCH 0/3] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-27T14:10:13Z","receivedAt":"2023-06-27T14:11:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIn 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. On macOS, the arguments to the command are named pipes\nunder /dev/fd, and git diff doesn't know how to handle a named\npipe. On Linux, the arguments are symlinks to pipes, so git diff\n\"helpfully\" diffs these symlinks, comparing their targets like\n\"pipe:[1234]\" and \"pipe:[5678]\".\n\nThere have been at least three previous attempts [1-3] to address this\nissue. They all seem to have received broad support for the aim of\nsupporting process substitutions but have foundered on details of the\nimplementation. In an effort to avoid the same fate this series is\nnarrowly focussed on making command substitutions work with \"diff\n--no-index\" and does not attempt to add a general facility for\nde-referencing symbolic links or reading from pipes to the diff\nmachinery.\n\nThe only functional change is that if a path given on the commandline\nis a named pipe or a symbolic link that resolves to a named pipe then\nwe read the data to diff from that pipe. The first two patches improve\nthe error handling when reading from stdin and add a test. The third\npatch implements support for reading from pipes.\n\nThis cover letter and the commit message for the third patch are\nlargely copied from brian’s patch[2] - do we have a standard commit\ntrailer for \"I stole the commit message from someone else's patch\"?\n\nI've cc’d the participants of the discussion of the last attempt[1] to\nfix this.\n\n[1] https://lore.kernel.org/git/20200918113256.8699-3-tguyot@gmail.com/\n[2] https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n[3] https://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/\n\n\nBase-Commit: 94486b6763c29144c60932829a65fec0597e17b3\nPublished-As: https://github.com/phillipwood/git/releases/tag/diff-no-index-pipes%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/94486b676...990e71882\nFetch-It-Via: git fetch https://github.com/phillipwood/git diff-no-index-pipes/v1\n\nPhillip Wood (3):\n  diff --no-index: die on error reading stdin\n  t4054: test diff --no-index with stdin\n  diff --no-index: support reading from named pipes\n\n diff-no-index.c          | 83 ++++++++++++++++++++++++----------------\n t/t4053-diff-no-index.sh | 43 +++++++++++++++++++++\n 2 files changed, 92 insertions(+), 34 deletions(-)\n\n-- \n2.40.1.852.g22d29fd9ba\n\n"},{"id":"478884","messageId":"5dad728f3b654f0cee59903cb7563298b2d743e9.1687874975.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1687874975.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 1/3] diff --no-index: die on error reading stdin","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-27T14:10:14Z","receivedAt":"2023-06-27T14:11:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf there is an error when reading from stdin then \"diff --no-index\"\nprints an error message but continues to try and diff a file named \"-\"\nresulting in an error message that looks like\n\n    error: error while reading from stdin: Invalid argument\n    fatal: stat '-': No such file or directory\n\nassuming that no file named \"-\" exists. If such a file exists it prints\nthe first error message and generates the diff from that file which is\nnot what the user wanted. Instead just die() straight away if we cannot\nread from stdin.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff-no-index.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 4296940f90..7b9327f8f3 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -60,20 +60,19 @@ static int get_mode(const char *path, int *mode)\n \treturn 0;\n }\n \n-static int populate_from_stdin(struct diff_filespec *s)\n+static void populate_from_stdin(struct diff_filespec *s)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t size = 0;\n \n \tif (strbuf_read(&buf, 0, 0) < 0)\n-\t\treturn error_errno(\"error while reading from stdin\");\n+\t\tdie_errno(\"error while reading from stdin\");\n \n \ts->should_munmap = 0;\n \ts->data = strbuf_detach(&buf, &size);\n \ts->size = size;\n \ts->should_free = 1;\n \ts->is_stdin = 1;\n-\treturn 0;\n }\n \n static struct diff_filespec *noindex_filespec(const char *name, int mode)\n-- \n2.40.1.852.g22d29fd9ba\n\n"},{"id":"478885","messageId":"b94d59034f53c9f49fda815a80d1a4795722f058.1687874975.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1687874975.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 2/3] t4054: test diff --no-index with stdin","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-27T14:10:15Z","receivedAt":"2023-06-27T14:11:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\"git diff --no-index\" supports reading from stdin with the path \"-\".\nThere is no test coverage for this so add a regression test before\nchanging the code in the next commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t4053-diff-no-index.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 4e9fa0403d..d14b194ea2 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -205,4 +205,22 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik\n \ttest_cmp expected actual\n '\n \n+test_expect_success \"diff --no-index treats '-' as stdin\" '\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/- b/a/1\n+\tindex $ZERO_OID..$(git hash-object --stdin <a/1) 100644\n+\t--- a/-\n+\t+++ b/a/1\n+\t@@ -1 +1 @@\n+\t-x\n+\t+1\n+\tEOF\n+\n+\ttest_write_lines x | test_expect_code 1 \\\n+\t\tgit -c core.abbrev=no diff --no-index -- - a/1 >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_write_lines 1 | git diff --no-index -- a/1 - >actual &&\n+\ttest_must_be_empty actual\n+'\n test_done\n-- \n2.40.1.852.g22d29fd9ba\n\n"},{"id":"478886","messageId":"990e71882bfdc697285c5b04b92c290679ca22ab.1687874975.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1687874975.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 3/3] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-27T14:10:16Z","receivedAt":"2023-06-27T14:11:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIn 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\". On macOS, the arguments to the command are named pipes\nunder /dev/fd, and git diff doesn't know how to handle a named pipe. On\nLinux, the arguments are symlinks to pipes, so git diff \"helpfully\"\ndiffs these symlinks, comparing their targets like \"pipe:[1234]\" and\n\"pipe:[5678]\".\n\nTo address this \"diff --no-index\" is changed so that if a path given on\nthe commandline is a named pipe or a symbolic link that resolves to a\nnamed pipe then we read the data to diff from that pipe. This is\nimplemented by generalizing the code that already exists to handle\nreading from stdin when the user passes the path \"-\".\n\nAs process substitution is not support by POSIX this change is tested by\nusing a pipe and a symbolic link to a pipe.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff-no-index.c          | 80 ++++++++++++++++++++++++----------------\n t/t4053-diff-no-index.sh | 25 +++++++++++++\n 2 files changed, 73 insertions(+), 32 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7b9327f8f3..45fbd7cdbe 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -41,7 +41,7 @@ static int read_directory_contents(const char *path, struct string_list *list)\n  */\n static const char file_from_standard_input[] = \"-\";\n \n-static int get_mode(const char *path, int *mode)\n+static int get_mode(const char *path, int is_pipe, int *mode)\n {\n \tstruct stat st;\n \n@@ -51,7 +51,7 @@ static int get_mode(const char *path, int *mode)\n \telse if (!strcasecmp(path, \"nul\"))\n \t\t*mode = 0;\n #endif\n-\telse if (path == file_from_standard_input)\n+\telse if (is_pipe)\n \t\t*mode = create_ce_mode(0666);\n \telse if (lstat(path, &st))\n \t\treturn error(\"Could not access '%s'\", path);\n@@ -60,13 +60,18 @@ static int get_mode(const char *path, int *mode)\n \treturn 0;\n }\n \n-static void populate_from_stdin(struct diff_filespec *s)\n+static void populate_from_pipe(struct diff_filespec *s, int is_stdin)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t size = 0;\n+\tint fd = 0;\n \n-\tif (strbuf_read(&buf, 0, 0) < 0)\n+\tif (!is_stdin)\n+\t\tfd = xopen(s->path, O_RDONLY);\n+\tif (strbuf_read(&buf, fd, 0) < 0)\n \t\tdie_errno(\"error while reading from stdin\");\n+\tif (!is_stdin)\n+\t\tclose(fd);\n \n \ts->should_munmap = 0;\n \ts->data = strbuf_detach(&buf, &size);\n@@ -75,40 +80,43 @@ static void populate_from_stdin(struct diff_filespec *s)\n \ts->is_stdin = 1;\n }\n \n-static struct diff_filespec *noindex_filespec(const char *name, int mode)\n+static struct diff_filespec *noindex_filespec(const char *name, int is_pipe,\n+\t\t\t\t\t      int mode)\n {\n \tstruct diff_filespec *s;\n \n \tif (!name)\n \t\tname = \"/dev/null\";\n \ts = alloc_filespec(name);\n \tfill_filespec(s, null_oid(), 0, mode);\n-\tif (name == file_from_standard_input)\n-\t\tpopulate_from_stdin(s);\n+\tif (is_pipe)\n+\t\tpopulate_from_pipe(s, name == file_from_standard_input);\n \treturn s;\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 is_pipe1,\n+\t\t      const char *name2, int is_pipe2)\n {\n \tint mode1 = 0, mode2 = 0;\n \n-\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n+\tif (get_mode(name1, is_pipe1, &mode1) ||\n+\t    get_mode(name2, is_pipe2, &mode2))\n \t\treturn -1;\n \n \tif (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n \t\tstruct diff_filespec *d1, *d2;\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, 0);\n+\t\t\td2 = noindex_filespec(name2, is_pipe2, mode2);\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, is_pipe1, mode1);\n+\t\t\td2 = noindex_filespec(NULL, 0, 0);\n \t\t\tname1 = NULL;\n \t\t\tmode1 = 0;\n \t\t}\n@@ -173,7 +181,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, 0, n2, 0);\n \t\t}\n \t\tstring_list_clear(&p1, 0);\n \t\tstring_list_clear(&p2, 0);\n@@ -189,8 +197,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, is_pipe1, mode1);\n+\t\td2 = noindex_filespec(name2, is_pipe2, mode2);\n \t\tdiff_queue(&diff_queued_diff, d1, d2);\n \t\treturn 0;\n \t}\n@@ -213,18 +221,11 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n  * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n  * becomes \"diff a/b/file D/file\", not \"diff a/b/file D/a/b/file\".\n  */\n-static void fixup_paths(const char **path, struct strbuf *replacement)\n+static void fixup_paths(const char **path, int *is_dir, struct strbuf *replacement)\n {\n-\tunsigned int isdir0, isdir1;\n-\n-\tif (path[0] == file_from_standard_input ||\n-\t    path[1] == file_from_standard_input)\n-\t\treturn;\n-\tisdir0 = is_directory(path[0]);\n-\tisdir1 = is_directory(path[1]);\n-\tif (isdir0 == isdir1)\n-\t\treturn;\n-\tif (isdir0) {\n+\tif (is_dir[0] == is_dir[1])\n+\t\treturn;\n+\tif (is_dir[0]) {\n \t\tappend_basename(replacement, path[0], path[1]);\n \t\tpath[0] = replacement->buf;\n \t} else {\n@@ -246,6 +247,8 @@ int diff_no_index(struct rev_info *revs,\n \tint ret = 1;\n \tconst char *paths[2];\n \tchar *to_free[ARRAY_SIZE(paths)] = { 0 };\n+\tint is_dir[ARRAY_SIZE(paths)] = { 0 };\n+\tint is_pipe[ARRAY_SIZE(paths)] = { 0 };\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n@@ -267,18 +270,30 @@ int diff_no_index(struct rev_info *revs,\n \tFREE_AND_NULL(options);\n \tfor (i = 0; i < 2; i++) {\n \t\tconst char *p = argv[i];\n-\t\tif (!strcmp(p, \"-\"))\n+\t\tif (!strcmp(p, \"-\")) {\n \t\t\t/*\n \t\t\t * stdin should be spelled as \"-\"; if you have\n \t\t\t * path that is \"-\", spell it as \"./-\".\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n-\t\telse if (prefix)\n-\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n+\t\t\tis_pipe[i] = 1;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\n+\t\t\tif (prefix)\n+\t\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n+\t\t\tif (stat(p, &st))\n+\t\t\t\t;\n+\t\t\telse if (S_ISDIR(st.st_mode))\n+\t\t\t\tis_dir[i] = 1;\n+\t\t\telse if (S_ISFIFO(st.st_mode))\n+\t\t\t\tis_pipe[i] = 1;\n+\t\t}\n \t\tpaths[i] = p;\n \t}\n \n-\tfixup_paths(paths, &replacement);\n+\tif (!is_pipe[0] && !is_pipe[1])\n+\t\tfixup_paths(paths, is_dir, &replacement);\n \n \trevs->diffopt.skip_stat_unmatch = 1;\n \tif (!revs->diffopt.output_format)\n@@ -295,7 +310,8 @@ int diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n \n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n+\tif (queue_diff(&revs->diffopt,\n+\t\t       paths[0], is_pipe[0], paths[1], is_pipe[1]))\n \t\tgoto out;\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex d14b194ea2..30b9e2c2f0 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -223,4 +223,29 @@ test_expect_success \"diff --no-index treats '-' as stdin\" '\n \ttest_write_lines 1 | git diff --no-index -- a/1 - >actual &&\n \ttest_must_be_empty actual\n '\n+\n+test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n+\tmkfifo old &&\n+\tmkfifo new &&\n+\tln -s new new-link &&\n+\t{\n+\t\t(test_write_lines a b c >old) &\n+\t\t(test_write_lines a x c >new) &\n+\t} &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/old b/new-link\n+\t--- a/old\n+\t+++ b/new-link\n+\t@@ -1,3 +1,3 @@\n+\t a\n+\t-b\n+\t+x\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index old new-link >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.1.852.g22d29fd9ba\n\n"},{"id":"478918","messageId":"xmqqy1k4g068.fsf@gitster.g","threadId":"59917","inReplyTo":"990e71882bfdc697285c5b04b92c290679ca22ab.1687874975.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 3/3] diff --no-index: support reading from named pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-27T19:44:31Z","receivedAt":"2023-06-27T19:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\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\". On macOS, the arguments to the command are named pipes\n> under /dev/fd, and git diff doesn't know how to handle a named pipe. On\n> Linux, the arguments are symlinks to pipes, so git diff \"helpfully\"\n> diffs these symlinks, comparing their targets like \"pipe:[1234]\" and\n> \"pipe:[5678]\".\n>\n> To address this \"diff --no-index\" is changed so that if a path given on\n> the commandline is a named pipe or a symbolic link that resolves to a\n> named pipe then we read the data to diff from that pipe. This is\n> implemented by generalizing the code that already exists to handle\n> reading from stdin when the user passes the path \"-\".\n>\n> As process substitution is not support by POSIX this change is tested by\n> using a pipe and a symbolic link to a pipe.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  diff-no-index.c          | 80 ++++++++++++++++++++++++----------------\n>  t/t4053-diff-no-index.sh | 25 +++++++++++++\n>  2 files changed, 73 insertions(+), 32 deletions(-)\n\nThis looks good, if a bit invasive, to a cursory read, at least to\nme.  It is very focused to the real problem at hand, and shows that\nthe way we split the \"no-index\" mode out to its own implementation\nof filespec population code does make sense.\n\n> -static void populate_from_stdin(struct diff_filespec *s)\n> +static void populate_from_pipe(struct diff_filespec *s, int is_stdin)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tsize_t size = 0;\n> +\tint fd = 0;\n>  \n> -\tif (strbuf_read(&buf, 0, 0) < 0)\n> +\tif (!is_stdin)\n> +\t\tfd = xopen(s->path, O_RDONLY);\n> +\tif (strbuf_read(&buf, fd, 0) < 0)\n>  \t\tdie_errno(\"error while reading from stdin\");\n> +\tif (!is_stdin)\n> +\t\tclose(fd);\n\nGiven that the error message explicitly says \"stdin\", and there are\nmany \"if ([!]is_stdin)\" sprinkled in the code, I actually suspect\nthat there should be two separate helpers, one for stdin and one for\nnon-stdin pipe.  It is especially true since there is only one\ncaller that does this:\n\n> +\tif (is_pipe)\n> +\t\tpopulate_from_pipe(s, name == file_from_standard_input);\n\nwhich can be\n\n\tif (is_pipe) {\n\t\tif (name == file_from_standard_input)\n\t\t\tpopulate_from_stdin(s);\n\t\telse\n\t\t\tpopulate_from_pipe(s);\n\t}\n\nwithout losing clarity.  The code that you are sharing by forcing\nthem to be a single helper to wrap up a handful of members in the s\nstructure can become its own helper that is called from these two\nhelper functions.\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 is_pipe1,\n> +\t\t      const char *name2, int is_pipe2)\n>  {\n>  \tint mode1 = 0, mode2 = 0;\n>  \n> -\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n> +\tif (get_mode(name1, is_pipe1, &mode1) ||\n> +\t    get_mode(name2, is_pipe2, &mode2))\n>  \t\treturn -1;\n\nMakes me wonder why the caller of queue_diff() even needs to know if\nthese two names are pipes; we are calling get_mode() which would run\nstat(2) anyway, and the result from stat(2) is what you use (in the\ncaller) to determine the values of is_pipeN.  Wouldn't it be more\nappropriate to leave the caller oblivious of special casing of the\npipes and let get_mode() handle this?  After all, that is how the\nexisting code special cases the standard input so there is a strong\nprecedence.\n\nIf we go that route, it may make sense to further isolate the\n\"address comparison\" trick used for the standard input mode.\nPerhaps we can and do something like\n\n    static int get_mode(const char *path, int *mode, int *special)\n    {\n\tstruct stat st;\n\n+\t*special = 0; /* default - nothing special */\n\t...\n\telse if (path == file_from_standard_input) {\n\t\t*mode = create_ce_mode(0666);\n+\t\t*pipe_kind = 1; /* STDIN */\n+\t} else if (stat(path, &st)) {\n+\t\t... error ...\n+\t} else if (S_ISFIFO(st.st_mode)) {\n+\t\t*mode = create_ce_mode(0666);\n+\t\t*pipe_kind = 2; /* FIFO */\n\t} else if (lstat(path, &st)) {\n\t\t... error ...\n\t} else {\n\t\t*mode = st.st_mode;\n\t}\n\nand have the caller act on \"special\" to choose among calling\npopulate_from_stdin(), populate_from_pipe(), or do nothing for\nthe regular files?\n\n    Side note: this has an added benefit of highlighting that we do\n    stat() and lstat() because of dereferencing.  What I suspect is\n    that \"git diff --no-index\" mode was primarily to give Git\n    niceties like rename detection and diff algorithms to those who\n    wanted to use in contexts (i.e. contents not tracked by Git)\n    they use \"diff\" by other people like GNU, without bothering to\n    update \"diff\" by other people.  I further suspect that \"compare\n    the readlink contents\", which is very much necessary within the\n    Git context, may not fall into the \"Git niceties\" when they\n    invoke \"--no-index\" mode.  Which leads me to imagine a future\n    direction where we only use stat() and not lstat() in the\n    \"--no-index\" codepath.  Having everything including these\n    lstat() and stat() calls inside get_mode() will allow such a\n    future transition hopefully simpler.\n\nI do not quite see why you decided to move the \"is_dir\" processing\nup and made the caller responsible.  Specifically,\n\n> -\tfixup_paths(paths, &replacement);\n> +\tif (!is_pipe[0] && !is_pipe[1])\n> +\t\tfixup_paths(paths, is_dir, &replacement);\n\nthis seems fishy when one side is pipe and the other one is not.\nWhen the user says\n\n    $ git diff --no-index <(command) path\n\nfixup_paths() are bypassed because one of them is pipe.  It makes me\nsuspect that it should be an error if \"path\" is a directory.  I do\nnot know if fixup_paths() is the best place for doing such checking,\nbut somebody should be doing that, no?\n\n\n"},{"id":"478954","messageId":"457ff920-865e-f018-1d79-f3cb1121d199@gmail.com","threadId":"59917","inReplyTo":"xmqqy1k4g068.fsf@gitster.g","subject":"Re: [PATCH 3/3] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-28T10:05:46Z","receivedAt":"2023-06-28T10:12:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 27/06/2023 20:44, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\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\". On macOS, the arguments to the command are named pipes\n>> under /dev/fd, and git diff doesn't know how to handle a named pipe. On\n>> Linux, the arguments are symlinks to pipes, so git diff \"helpfully\"\n>> diffs these symlinks, comparing their targets like \"pipe:[1234]\" and\n>> \"pipe:[5678]\".\n>>\n>> To address this \"diff --no-index\" is changed so that if a path given on\n>> the commandline is a named pipe or a symbolic link that resolves to a\n>> named pipe then we read the data to diff from that pipe. This is\n>> implemented by generalizing the code that already exists to handle\n>> reading from stdin when the user passes the path \"-\".\n>>\n>> As process substitution is not support by POSIX this change is tested by\n>> using a pipe and a symbolic link to a pipe.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>   diff-no-index.c          | 80 ++++++++++++++++++++++++----------------\n>>   t/t4053-diff-no-index.sh | 25 +++++++++++++\n>>   2 files changed, 73 insertions(+), 32 deletions(-)\n> \n> This looks good, if a bit invasive, to a cursory read, at least to\n> me.  It is very focused to the real problem at hand, and shows that\n> the way we split the \"no-index\" mode out to its own implementation\n> of filespec population code does make sense.\n\nYes, it is more invasive than I'd like but I think that stems from \nneeding to treat paths given on the commandline differently to the paths \nwe find when recursing directories.\n\n>> -static void populate_from_stdin(struct diff_filespec *s)\n>> +static void populate_from_pipe(struct diff_filespec *s, int is_stdin)\n>>   {\n>>   \tstruct strbuf buf = STRBUF_INIT;\n>>   \tsize_t size = 0;\n>> +\tint fd = 0;\n>>   \n>> -\tif (strbuf_read(&buf, 0, 0) < 0)\n>> +\tif (!is_stdin)\n>> +\t\tfd = xopen(s->path, O_RDONLY);\n>> +\tif (strbuf_read(&buf, fd, 0) < 0)\n>>   \t\tdie_errno(\"error while reading from stdin\");\n>> +\tif (!is_stdin)\n>> +\t\tclose(fd);\n> \n> Given that the error message explicitly says \"stdin\", and there are\n> many \"if ([!]is_stdin)\" sprinkled in the code, I actually suspect\n> that there should be two separate helpers, one for stdin and one for\n> non-stdin pipe.  It is especially true since there is only one\n> caller that does this:\n> \n>> +\tif (is_pipe)\n>> +\t\tpopulate_from_pipe(s, name == file_from_standard_input);\n> \n> which can be\n> \n> \tif (is_pipe) {\n> \t\tif (name == file_from_standard_input)\n> \t\t\tpopulate_from_stdin(s);\n> \t\telse\n> \t\t\tpopulate_from_pipe(s);\n> \t}\n> \n> without losing clarity.  The code that you are sharing by forcing\n> them to be a single helper to wrap up a handful of members in the s\n> structure can become its own helper that is called from these two\n> helper functions.\n\nSure, and thanks for pointing out the error message, I'd overlooked that.\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 is_pipe1,\n>> +\t\t      const char *name2, int is_pipe2)\n>>   {\n>>   \tint mode1 = 0, mode2 = 0;\n>>   \n>> -\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>> +\tif (get_mode(name1, is_pipe1, &mode1) ||\n>> +\t    get_mode(name2, is_pipe2, &mode2))\n>>   \t\treturn -1;\n> \n> Makes me wonder why the caller of queue_diff() even needs to know if\n> these two names are pipes; we are calling get_mode() which would run\n> stat(2) anyway, and the result from stat(2) is what you use (in the\n> caller) to determine the values of is_pipeN.  Wouldn't it be more\n> appropriate to leave the caller oblivious of special casing of the\n> pipes and let get_mode() handle this?  After all, that is how the\n> existing code special cases the standard input so there is a strong\n> precedence.\n\nMaybe is_pipeN should be called force_pipeN. We only want to \nde-reference symbolic links to pipes for paths that are given on the \ncommand line, not when the the user asked to compare two directories. \nThat means we need to pass some kind of flag around to say whether we're \nrecursing or not. An earlier draft of this series had a recursing flag \nrather than is_pipeN like this\n\n-static int get_mode(const char *path, int *mode)\n+static int get_mode(const char *path, int *mode, int recursing)\n  {\n          struct stat st;\n\n          if (!path || !strcmp(path, \"/dev/null\"))\n                  *mode = 0;\n  #ifdef GIT_WINDOWS_NATIVE\n          else if (!strcasecmp(path, \"nul\"))\n                  *mode = 0;\n  #endif\n          else if (path == file_from_standard_input)\n                  *mode = create_ce_mode(0666);\n          else if (lstat(path, &st))\n                  return error(\"Could not access '%s'\", path);\n          else\n                  *mode = st.st_mode;\n+\n+        /*\n+\t  * For paths given on the command line de-reference symbolic\n+\t  * links that resolve to a fifo.\n+\t  */\n+        if (!recursing &&\n+            S_ISLNK(*mode) && !stat(path, &st) && S_ISFIFO(st.st_mode))\n+                *mode = st.st_mode;\n+\n          return 0;\n  }\n\nI dropped it in favor of is_pipeN after I realized we need to check if \nthe paths on the command line are pipes before calling fixup_paths(). I \nthink we could use the \"special\" parameter you suggest below as a \nrecursion indicator by setting it to NULL when recursing.\n\n> If we go that route, it may make sense to further isolate the\n> \"address comparison\" trick used for the standard input mode.\n> Perhaps we can and do something like\n> \n>      static int get_mode(const char *path, int *mode, int *special)\n>      {\n> \tstruct stat st;\n> \n> +\t*special = 0; /* default - nothing special */\n> \t...\n> \telse if (path == file_from_standard_input) {\n> \t\t*mode = create_ce_mode(0666);\n> +\t\t*pipe_kind = 1; /* STDIN */\n> +\t} else if (stat(path, &st)) {\n> +\t\t... error ...\n> +\t} else if (S_ISFIFO(st.st_mode)) {\n> +\t\t*mode = create_ce_mode(0666);\n> +\t\t*pipe_kind = 2; /* FIFO */\n> \t} else if (lstat(path, &st)) {\n> \t\t... error ...\n> \t} else {\n> \t\t*mode = st.st_mode;\n> \t}\n> \n> and have the caller act on \"special\" to choose among calling\n> populate_from_stdin(), populate_from_pipe(), or do nothing for\n> the regular files?\n> \n>      Side note: this has an added benefit of highlighting that we do\n>      stat() and lstat() because of dereferencing.  What I suspect is\n>      that \"git diff --no-index\" mode was primarily to give Git\n>      niceties like rename detection and diff algorithms to those who\n>      wanted to use in contexts (i.e. contents not tracked by Git)\n>      they use \"diff\" by other people like GNU, without bothering to\n>      update \"diff\" by other people.  I further suspect that \"compare\n>      the readlink contents\", which is very much necessary within the\n>      Git context, may not fall into the \"Git niceties\" when they\n>      invoke \"--no-index\" mode.  Which leads me to imagine a future\n>      direction where we only use stat() and not lstat() in the\n>      \"--no-index\" codepath.  Having everything including these\n>      lstat() and stat() calls inside get_mode() will allow such a\n>      future transition hopefully simpler.\n> \n> I do not quite see why you decided to move the \"is_dir\" processing\n> up and made the caller responsible.\n\nTo avoid a second stat() call in is_directory() after we've already \ncalled stat() to see if the path is a pipe.\n\n>  Specifically,\n> \n>> -\tfixup_paths(paths, &replacement);\n>> +\tif (!is_pipe[0] && !is_pipe[1])\n>> +\t\tfixup_paths(paths, is_dir, &replacement);\n> \n> this seems fishy when one side is pipe and the other one is not.\n> When the user says\n> \n>      $ git diff --no-index <(command) path\n> \n> fixup_paths() are bypassed because one of them is pipe.  It makes me\n> suspect that it should be an error if \"path\" is a directory.  I do\n> not know if fixup_paths() is the best place for doing such checking,\n> but somebody should be doing that, no?\n\nI  wasn't sure what was best to do in that case. In the end I went with \nmaking the behavior the same as \"git diff --no-index -- - directory\". \nI'm happy to make both an error.\n\nThanks for your comments, I'll leave it a couple of days to see if there \nare any more comments and then re-roll.\n\nPhillip\n\n"},{"id":"479230","messageId":"cover.1688586536.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1687874975.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 0/4] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-05T19:49:26Z","receivedAt":"2023-07-05T19:50:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIn 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. On macOS, the arguments to the command are named pipes\nunder /dev/fd, and git diff doesn't know how to handle a named\npipe. On Linux, the arguments are symlinks to pipes, so git diff\n\"helpfully\" diffs these symlinks, comparing their targets like\n\"pipe:[1234]\" and \"pipe:[5678]\".\n\nThere have been at least three previous attempts [1-3] to address this\nissue. They all seem to have received broad support for the aim of\nsupporting process substitutions but have foundered on details of the\nimplementation. In an effort to avoid the same fate this series is\nnarrowly focussed on making command substitutions work with \"diff\n--no-index\" and does not attempt to add a general facility for\nde-referencing symbolic links or reading from pipes to the diff\nmachinery.\n\nThe only functional change is that if a path given on the commandline\nis a named pipe or a symbolic link that resolves to a named pipe then\nwe read the data to diff from that pipe. The first two patches improve\nthe error handling when reading from stdin and add a test. The third\npatch implements support for reading from pipes.\n\nThis cover letter and the commit message for the third patch are\nlargely copied from brian’s patch[2] - do we have a standard commit\ntrailer for \"I stole the commit message from someone else's patch\"?\n\nI've cc’d the participants of the discussion of the last attempt[1] to\nfix this.\n\nThanks to Junio for his comments, the changes since v1 are:\n\n* Patch 1: - This is new and changes the code to error out if the user\n\t     tries to compare stdin to a directory.\n\n* Patch 4: - Changed the implementation of get_mode() to treat stdin and\n\t     named pipes as special. (suggested by Junio)\n\t   - Added separate functions to populate a diff_filespec from\n\t     a stdin or a named pipe. (suggested by Junio)\n\t   - It now dies if the user tries to compare a named pipe to a\n\t     directory and added a test for this. (suggested by Junio)\n\t   - The test for comparing named pipes now cleans up the\n\t     background processes if it fails.\n\n[1] https://lore.kernel.org/git/20200918113256.8699-3-tguyot@gmail.com/\n[2] https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n[3] https://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/\n\n\nbase-commit: 94486b6763c29144c60932829a65fec0597e17b3\nPublished-As: https://github.com/phillipwood/git/releases/tag/diff-no-index-pipes%2Fv2\nView-Changes-At: https://github.com/phillipwood/git/compare/94486b676...4e05a0be5\nFetch-It-Via: git fetch https://github.com/phillipwood/git diff-no-index-pipes/v2\n\nPhillip Wood (4):\n  diff --no-index: refuse to compare stdin to a directory\n  diff --no-index: die on error reading stdin\n  t4054: test diff --no-index with stdin\n  diff --no-index: support reading from named pipes\n\n diff-no-index.c          | 126 ++++++++++++++++++++++++++++-----------\n t/t4053-diff-no-index.sh |  64 ++++++++++++++++++++\n 2 files changed, 156 insertions(+), 34 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  5e65a15223 diff --no-index: refuse to compare stdin to a directory\n1:  5dad728f3b = 2:  be1d666769 diff --no-index: die on error reading stdin\n2:  b94d59034f ! 3:  1c7db4dbe2 t4054: test diff --no-index with stdin\n    @@ t/t4053-diff-no-index.sh: test_expect_success POSIXPERM,SYMLINKS 'diff --no-inde\n     +\ttest_write_lines 1 | git diff --no-index -- a/1 - >actual &&\n     +\ttest_must_be_empty actual\n     +'\n    - test_done\n    ++\n    + test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n    + \ttest_must_fail git diff --no-index -- - a </dev/null 2>err &&\n    + \tgrep \"fatal: cannot compare stdin to a directory\" err\n3:  990e71882b < -:  ---------- diff --no-index: support reading from named pipes\n-:  ---------- > 4:  4e05a0be54 diff --no-index: support reading from named pipes\n\nbase-commit: 94486b6763c29144c60932829a65fec0597e17b3\n-- \n2.40.1.852.g0a1e0755a6\n\n"},{"id":"479231","messageId":"5e65a15223bc42293516308eb31ade5395609c55.1688586536.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1688586536.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 1/4] diff --no-index: refuse to compare stdin to a directory","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-05T19:49:27Z","receivedAt":"2023-07-05T19:50:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen the user runs\n\n    git diff --no-index file directory\n\nwe follow the behavior of POSIX diff and rewrite the arguments as\n\n    git diff --no-index file directory/file\n\nDoing that when \"file\" is \"-\" (which means \"read from stdin\") does not\nmake sense so we should error out if the user asks us to compare \"-\" to\na directory. This matches the behavior of GNU diff and diff on *BSD.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff-no-index.c          | 12 +++++++-----\n t/t4053-diff-no-index.sh |  5 +++++\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 4296940f90..77462ac2a9 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -218,11 +218,13 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n {\n \tunsigned int isdir0, isdir1;\n \n-\tif (path[0] == file_from_standard_input ||\n-\t    path[1] == file_from_standard_input)\n-\t\treturn;\n-\tisdir0 = is_directory(path[0]);\n-\tisdir1 = is_directory(path[1]);\n+\tisdir0 = path[0] != file_from_standard_input && is_directory(path[0]);\n+\tisdir1 = path[1] != file_from_standard_input && is_directory(path[1]);\n+\n+\tif ((path[0] == file_from_standard_input && isdir1) ||\n+\t    (isdir0 && path[1] == file_from_standard_input))\n+\t\tdie(_(\"cannot compare stdin to a directory\"));\n+\n \tif (isdir0 == isdir1)\n \t\treturn;\n \tif (isdir0) {\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 4e9fa0403d..5bfb282e98 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -205,4 +205,9 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n+\ttest_must_fail git diff --no-index -- - a </dev/null 2>err &&\n+\tgrep \"fatal: cannot compare stdin to a directory\" err\n+'\n+\n test_done\n-- \n2.40.1.852.g0a1e0755a6\n\n"},{"id":"479232","messageId":"1c7db4dbe26715912672630c1a5659591931cb2d.1688586536.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1688586536.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 3/4] t4054: test diff --no-index with stdin","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-05T19:49:29Z","receivedAt":"2023-07-05T19:50:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\"git diff --no-index\" supports reading from stdin with the path \"-\".\nThere is no test coverage for this so add a regression test before\nchanging the code in the next commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t4053-diff-no-index.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 5bfb282e98..4870443609 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -205,6 +205,25 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik\n \ttest_cmp expected actual\n '\n \n+test_expect_success \"diff --no-index treats '-' as stdin\" '\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/- b/a/1\n+\tindex $ZERO_OID..$(git hash-object --stdin <a/1) 100644\n+\t--- a/-\n+\t+++ b/a/1\n+\t@@ -1 +1 @@\n+\t-x\n+\t+1\n+\tEOF\n+\n+\ttest_write_lines x | test_expect_code 1 \\\n+\t\tgit -c core.abbrev=no diff --no-index -- - a/1 >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_write_lines 1 | git diff --no-index -- a/1 - >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n \ttest_must_fail git diff --no-index -- - a </dev/null 2>err &&\n \tgrep \"fatal: cannot compare stdin to a directory\" err\n-- \n2.40.1.852.g0a1e0755a6\n\n"},{"id":"479233","messageId":"4e05a0be54f66f2b394642762832e426a545426c.1688586536.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1688586536.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 4/4] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-05T19:49:30Z","receivedAt":"2023-07-05T19:50:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIn 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\". On macOS, the arguments to the command are named pipes\nunder /dev/fd, and git diff doesn't know how to handle a named pipe. On\nLinux, the arguments are symlinks to pipes, so git diff \"helpfully\"\ndiffs these symlinks, comparing their targets like \"pipe:[1234]\" and\n\"pipe:[5678]\".\n\nTo address this \"diff --no-index\" is changed so that if a path given on\nthe commandline is a named pipe or a symbolic link that resolves to a\nnamed pipe then we read the data to diff from that pipe. This is\nimplemented by generalizing the code that already exists to handle\nreading from stdin when the user passes the path \"-\".\n\nIf the user tries to compare a named pipe to a directory then we die as\nwe do when trying to compare stdin to a directory.\n\nAs process substitution is not support by POSIX this change is tested by\nusing a pipe and a symbolic link to a pipe.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff-no-index.c          | 113 +++++++++++++++++++++++++++++----------\n t/t4053-diff-no-index.sh |  40 ++++++++++++++\n 2 files changed, 125 insertions(+), 28 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 4470e0271d..4771cf02aa 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -41,74 +41,119 @@ static int read_directory_contents(const char *path, struct string_list *list)\n  */\n static const char file_from_standard_input[] = \"-\";\n \n-static int get_mode(const char *path, int *mode)\n+/*\n+ * For paths given on the command-line we treat \"-\" as stdin and named\n+ * pipes and symbolic links to named pipes specially.\n+ */\n+enum special {\n+\tSPECIAL_NONE,\n+\tSPECIAL_STDIN,\n+\tSPECIAL_PIPE,\n+};\n+\n+static int get_mode(const char *path, int *mode, enum special *special)\n {\n \tstruct stat st;\n \n-\tif (!path || !strcmp(path, \"/dev/null\"))\n+\tif (!path || !strcmp(path, \"/dev/null\")) {\n \t\t*mode = 0;\n #ifdef GIT_WINDOWS_NATIVE\n-\telse if (!strcasecmp(path, \"nul\"))\n+\t} else if (!strcasecmp(path, \"nul\")) {\n \t\t*mode = 0;\n #endif\n-\telse if (path == file_from_standard_input)\n+\t} else if (path == file_from_standard_input) {\n \t\t*mode = create_ce_mode(0666);\n-\telse if (lstat(path, &st))\n+\t\t*special = SPECIAL_STDIN;\n+\t} else if (lstat(path, &st)) {\n \t\treturn error(\"Could not access '%s'\", path);\n-\telse\n+\t} else {\n \t\t*mode = st.st_mode;\n+\t}\n+\t/*\n+\t * For paths on the command-line treat named pipes and symbolic\n+\t * links that resolve to a named pipe specially.\n+\t */\n+\tif (special &&\n+\t    (S_ISFIFO(*mode) ||\n+\t     (S_ISLNK(*mode) && !stat(path, &st) && S_ISFIFO(st.st_mode)))) {\n+\t\t*mode = create_ce_mode(0666);\n+\t\t*special = SPECIAL_PIPE;\n+\t}\n+\n \treturn 0;\n }\n \n-static void populate_from_stdin(struct diff_filespec *s)\n+static void populate_common(struct diff_filespec *s, struct strbuf *buf)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n \tsize_t size = 0;\n \n-\tif (strbuf_read(&buf, 0, 0) < 0)\n-\t\tdie_errno(\"error while reading from stdin\");\n-\n \ts->should_munmap = 0;\n-\ts->data = strbuf_detach(&buf, &size);\n+\ts->data = strbuf_detach(buf, &size);\n \ts->size = size;\n \ts->should_free = 1;\n \ts->is_stdin = 1;\n }\n \n-static struct diff_filespec *noindex_filespec(const char *name, int mode)\n+static void populate_from_pipe(struct diff_filespec *s)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd = xopen(s->path, O_RDONLY);\n+\n+\tif (strbuf_read(&buf, fd, 0) < 0)\n+\t\tdie_errno(\"error while reading from '%s'\", s->path);\n+\tclose(fd);\n+\tpopulate_common(s, &buf);\n+}\n+\n+static void populate_from_stdin(struct diff_filespec *s)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (strbuf_read(&buf, 0, 0) < 0)\n+\t\tdie_errno(\"error while reading from stdin\");\n+\tpopulate_common(s, &buf);\n+}\n+\n+static struct diff_filespec *noindex_filespec(const char *name, int mode,\n+\t\t\t\t\t      enum special special)\n {\n \tstruct diff_filespec *s;\n \n \tif (!name)\n \t\tname = \"/dev/null\";\n \ts = alloc_filespec(name);\n \tfill_filespec(s, null_oid(), 0, mode);\n-\tif (name == file_from_standard_input)\n+\tif (special == SPECIAL_STDIN)\n \t\tpopulate_from_stdin(s);\n+\telse if (special == SPECIAL_PIPE)\n+\t\tpopulate_from_pipe(s);\n \treturn s;\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, const char *name2, int recursing)\n {\n \tint mode1 = 0, mode2 = 0;\n+\tenum special special1 = SPECIAL_NONE, special2 = SPECIAL_NONE;\n \n-\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n+\t/* Paths can only be special if we're not recursing. */\n+\tif (get_mode(name1, &mode1, recursing ? NULL : &special1) ||\n+\t    get_mode(name2, &mode2, recursing ? NULL : &special2))\n \t\treturn -1;\n \n \tif (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n \t\tstruct diff_filespec *d1, *d2;\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, SPECIAL_NONE);\n+\t\t\td2 = noindex_filespec(name2, mode2, special2);\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, special1);\n+\t\t\td2 = noindex_filespec(NULL, 0, SPECIAL_NONE);\n \t\t\tname1 = NULL;\n \t\t\tmode1 = 0;\n \t\t}\n@@ -173,7 +218,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, n2, 1);\n \t\t}\n \t\tstring_list_clear(&p1, 0);\n \t\tstring_list_clear(&p2, 0);\n@@ -189,8 +234,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, special1);\n+\t\td2 = noindex_filespec(name2, mode2, special2);\n \t\tdiff_queue(&diff_queued_diff, d1, d2);\n \t\treturn 0;\n \t}\n@@ -215,15 +260,27 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n  */\n static void fixup_paths(const char **path, struct strbuf *replacement)\n {\n-\tunsigned int isdir0, isdir1;\n-\n-\tisdir0 = path[0] != file_from_standard_input && is_directory(path[0]);\n-\tisdir1 = path[1] != file_from_standard_input && is_directory(path[1]);\n+\tstruct stat st;\n+\tunsigned int isdir0 = 0, isdir1 = 0;\n+\tunsigned int ispipe0 = 0, ispipe1 = 0;\n+\n+\tif (path[0] != file_from_standard_input && !stat(path[0], &st)) {\n+\t\tisdir0 = S_ISDIR(st.st_mode);\n+\t\tispipe0 = S_ISFIFO(st.st_mode);\n+\t}\n+\n+\tif (path[1] != file_from_standard_input && !stat(path[1], &st)) {\n+\t\tisdir1 = S_ISDIR(st.st_mode);\n+\t\tispipe1 = S_ISFIFO(st.st_mode);\n+\t}\n \n \tif ((path[0] == file_from_standard_input && isdir1) ||\n \t    (isdir0 && path[1] == file_from_standard_input))\n \t\tdie(_(\"cannot compare stdin to a directory\"));\n \n+\tif ((isdir0 && ispipe1) || (ispipe0 && isdir1))\n+\t\tdie(_(\"cannot compare a named pipe to a directory\"));\n+\n \tif (isdir0 == isdir1)\n \t\treturn;\n \tif (isdir0) {\n@@ -297,7 +354,7 @@ int diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n \n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n+\tif (queue_diff(&revs->diffopt, paths[0], paths[1], 0))\n \t\tgoto out;\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 4870443609..a28b9ff243 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -229,4 +229,44 @@ test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n \tgrep \"fatal: cannot compare stdin to a directory\" err\n '\n \n+test_expect_success PIPE 'diff --no-index refuses to diff a named pipe and a directory' '\n+\ttest_when_finished \"rm -f pipe\" &&\n+\tmkfifo pipe &&\n+\t{\n+\t\t(>pipe) &\n+\t} &&\n+\ttest_when_finished \"kill $!\" &&\n+\ttest_must_fail git diff --no-index -- pipe a 2>err &&\n+\tgrep \"fatal: cannot compare a named pipe to a directory\" err\n+'\n+\n+test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n+\ttest_when_finished \"rm -f old new new-link\" &&\n+\tmkfifo old &&\n+\tmkfifo new &&\n+\tln -s new new-link &&\n+\t{\n+\t\t(test_write_lines a b c >old) &\n+\t} &&\n+\ttest_when_finished \"! kill $!\" &&\n+\t{\n+\t\t(test_write_lines a x c >new) &\n+\t} &&\n+\ttest_when_finished \"! kill $!\" &&\n+\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/old b/new-link\n+\t--- a/old\n+\t+++ b/new-link\n+\t@@ -1,3 +1,3 @@\n+\t a\n+\t-b\n+\t+x\n+\t c\n+\tEOF\n+\n+\ttest_expect_code 1 git diff --no-index old new-link >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.1.852.g0a1e0755a6\n\n"},{"id":"479234","messageId":"be1d666769f2d6dd028c7d5ade0e9f563e953ab5.1688586536.git.phillip.wood@dunelm.org.uk","threadId":"59917","inReplyTo":"cover.1688586536.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 2/4] diff --no-index: die on error reading stdin","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-05T19:49:28Z","receivedAt":"2023-07-05T19:50:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf there is an error when reading from stdin then \"diff --no-index\"\nprints an error message but continues to try and diff a file named \"-\"\nresulting in an error message that looks like\n\n    error: error while reading from stdin: Invalid argument\n    fatal: stat '-': No such file or directory\n\nassuming that no file named \"-\" exists. If such a file exists it prints\nthe first error message and generates the diff from that file which is\nnot what the user wanted. Instead just die() straight away if we cannot\nread from stdin.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff-no-index.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 77462ac2a9..4470e0271d 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -60,20 +60,19 @@ static int get_mode(const char *path, int *mode)\n \treturn 0;\n }\n \n-static int populate_from_stdin(struct diff_filespec *s)\n+static void populate_from_stdin(struct diff_filespec *s)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t size = 0;\n \n \tif (strbuf_read(&buf, 0, 0) < 0)\n-\t\treturn error_errno(\"error while reading from stdin\");\n+\t\tdie_errno(\"error while reading from stdin\");\n \n \ts->should_munmap = 0;\n \ts->data = strbuf_detach(&buf, &size);\n \ts->size = size;\n \ts->should_free = 1;\n \ts->is_stdin = 1;\n-\treturn 0;\n }\n \n static struct diff_filespec *noindex_filespec(const char *name, int mode)\n-- \n2.40.1.852.g0a1e0755a6\n\n"},{"id":"479245","messageId":"xmqqmt0axdlz.fsf@gitster.g","threadId":"59917","inReplyTo":"5e65a15223bc42293516308eb31ade5395609c55.1688586536.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 1/4] diff --no-index: refuse to compare stdin to a directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T21:17:12Z","receivedAt":"2023-07-05T21:17:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> When the user runs\n>\n>     git diff --no-index file directory\n>\n> we follow the behavior of POSIX diff and rewrite the arguments as\n>\n>     git diff --no-index file directory/file\n>\n> Doing that when \"file\" is \"-\" (which means \"read from stdin\") does not\n> make sense so we should error out if the user asks us to compare \"-\" to\n> a directory. This matches the behavior of GNU diff and diff on *BSD.\n\n\"git diff --no-index directory file\" would also be the same way.\n\nMakes sense.\n\n> -\tif (path[0] == file_from_standard_input ||\n> -\t    path[1] == file_from_standard_input)\n> -\t\treturn;\n> -\tisdir0 = is_directory(path[0]);\n> -\tisdir1 = is_directory(path[1]);\n\nWe used to silently did nonsense, I guess.\n\n> +\tisdir0 = path[0] != file_from_standard_input && is_directory(path[0]);\n> +\tisdir1 = path[1] != file_from_standard_input && is_directory(path[1]);\n> +\n> +\tif ((path[0] == file_from_standard_input && isdir1) ||\n> +\t    (isdir0 && path[1] == file_from_standard_input))\n> +\t\tdie(_(\"cannot compare stdin to a directory\"));\n\nOK.  It is much better than turning \"diff - D\" into \"diff - D/-\".\nIf D were a missing or a misspelt directory name, the rest of the\nfunction will just leave the original intact and let comparison\nbetween the standard input and D that does not exist, and will fail\ncorrectly anyway.  Good.\n\n>  \tif (isdir0 == isdir1)\n>  \t\treturn;\n>  \tif (isdir0) {\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 4e9fa0403d..5bfb282e98 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -205,4 +205,9 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n> +\ttest_must_fail git diff --no-index -- - a </dev/null 2>err &&\n> +\tgrep \"fatal: cannot compare stdin to a directory\" err\n> +'\n> +\n>  test_done\n"},{"id":"479246","messageId":"xmqqilayxdjx.fsf@gitster.g","threadId":"59917","inReplyTo":"be1d666769f2d6dd028c7d5ade0e9f563e953ab5.1688586536.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 2/4] diff --no-index: die on error reading stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T21:18:26Z","receivedAt":"2023-07-05T21:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> If there is an error when reading from stdin then \"diff --no-index\"\n> prints an error message but continues to try and diff a file named \"-\"\n> resulting in an error message that looks like\n>\n>     error: error while reading from stdin: Invalid argument\n>     fatal: stat '-': No such file or directory\n>\n> assuming that no file named \"-\" exists. If such a file exists it prints\n> the first error message and generates the diff from that file which is\n> not what the user wanted. Instead just die() straight away if we cannot\n> read from stdin.\n\nMakes sense.\n\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  diff-no-index.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 77462ac2a9..4470e0271d 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -60,20 +60,19 @@ static int get_mode(const char *path, int *mode)\n>  \treturn 0;\n>  }\n>  \n> -static int populate_from_stdin(struct diff_filespec *s)\n> +static void populate_from_stdin(struct diff_filespec *s)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tsize_t size = 0;\n>  \n>  \tif (strbuf_read(&buf, 0, 0) < 0)\n> -\t\treturn error_errno(\"error while reading from stdin\");\n> +\t\tdie_errno(\"error while reading from stdin\");\n>  \n>  \ts->should_munmap = 0;\n>  \ts->data = strbuf_detach(&buf, &size);\n>  \ts->size = size;\n>  \ts->should_free = 1;\n>  \ts->is_stdin = 1;\n> -\treturn 0;\n>  }\n>  \n>  static struct diff_filespec *noindex_filespec(const char *name, int mode)\n"},{"id":"479247","messageId":"xmqqbkgqxdck.fsf@gitster.g","threadId":"59917","inReplyTo":"1c7db4dbe26715912672630c1a5659591931cb2d.1688586536.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 3/4] t4054: test diff --no-index with stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T21:22:51Z","receivedAt":"2023-07-05T21:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> +\tcat >expect <<-EOF &&\n> +\tdiff --git a/- b/a/1\n> +\tindex $ZERO_OID..$(git hash-object --stdin <a/1) 100644\n> +\t--- a/-\n> +\t+++ b/a/1\n> +\t@@ -1 +1 @@\n> +\t-x\n> +\t+1\n> +\tEOF\n> +\n> +\ttest_write_lines x | test_expect_code 1 \\\n> +\t\tgit -c core.abbrev=no diff --no-index -- - a/1 >actual &&\n\nOr \"git diff --no-index --full-index -- - a/1\", which looks somewhat\nodd but may be correct and a bit shorter ;-)\n\nNot a suggestion to use --full-index instead of core.abbrev; both\nare equally valid.\n\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest_write_lines 1 | git diff --no-index -- a/1 - >actual &&\n> +\ttest_must_be_empty actual\n> +'\n\nGood.\n\n>  test_expect_success 'diff --no-index refuses to diff stdin and a directory' '\n>  \ttest_must_fail git diff --no-index -- - a </dev/null 2>err &&\n>  \tgrep \"fatal: cannot compare stdin to a directory\" err\n"},{"id":"479248","messageId":"xmqq1qhmvw6i.fsf@gitster.g","threadId":"59917","inReplyTo":"4e05a0be54f66f2b394642762832e426a545426c.1688586536.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 4/4] diff --no-index: support reading from named pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T22:19:01Z","receivedAt":"2023-07-05T22:19:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\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\"command substitution\" -> \"process substitution\", I think, are the\nwords found in the manual pages of both shells cited as examples.\n\n> However, this syntax does not produce useful results with \"git diff\n> --no-index\". On macOS, the arguments to the command are named pipes\n> under /dev/fd, and git diff doesn't know how to handle a named pipe. On\n> Linux, the arguments are symlinks to pipes, so git diff \"helpfully\"\n> diffs these symlinks, comparing their targets like \"pipe:[1234]\" and\n> \"pipe:[5678]\".\n\nConcisely and clearly described.  Very nice.\n\n> To address this \"diff --no-index\" is changed so that if a path given on\n\n\"this\" -> \"this,\", probably.\n\n> the commandline is a named pipe or a symbolic link that resolves to a\n> named pipe then we read the data to diff from that pipe. This is\n> implemented by generalizing the code that already exists to handle\n> reading from stdin when the user passes the path \"-\".\n\nGreat.\n\n> If the user tries to compare a named pipe to a directory then we die as\n> we do when trying to compare stdin to a directory.\n>\n> As process substitution is not support by POSIX this change is tested by\n> using a pipe and a symbolic link to a pipe.\n\nPeople who are interested to more directly test the intended use\ncase can add a test \"diff --no-index <(one) <(two)\" under some\nprerequisite (like BASH).  The test on the underlying mechansim like\nthis patch does is just fine in the meantime.\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 4470e0271d..4771cf02aa 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -41,74 +41,119 @@ static int read_directory_contents(const char *path, struct string_list *list)\n>   */\n>  static const char file_from_standard_input[] = \"-\";\n>  \n> -static int get_mode(const char *path, int *mode)\n> +/*\n> + * For paths given on the command-line we treat \"-\" as stdin and named\n> + * pipes and symbolic links to named pipes specially.\n> + */\n> +enum special {\n> +\tSPECIAL_NONE,\n> +\tSPECIAL_STDIN,\n> +\tSPECIAL_PIPE,\n> +};\n> +\n> +static int get_mode(const char *path, int *mode, enum special *special)\n>  {\n>  \tstruct stat st;\n>  \n> -\tif (!path || !strcmp(path, \"/dev/null\"))\n> +\tif (!path || !strcmp(path, \"/dev/null\")) {\n>  \t\t*mode = 0;\n>  #ifdef GIT_WINDOWS_NATIVE\n> -\telse if (!strcasecmp(path, \"nul\"))\n> +\t} else if (!strcasecmp(path, \"nul\")) {\n>  \t\t*mode = 0;\n>  #endif\n> -\telse if (path == file_from_standard_input)\n> +\t} else if (path == file_from_standard_input) {\n>  \t\t*mode = create_ce_mode(0666);\n> -\telse if (lstat(path, &st))\n> +\t\t*special = SPECIAL_STDIN;\n> +\t} else if (lstat(path, &st)) {\n>  \t\treturn error(\"Could not access '%s'\", path);\n> -\telse\n> +\t} else {\n>  \t\t*mode = st.st_mode;\n> +\t}\n> +\t/*\n> +\t * For paths on the command-line treat named pipes and symbolic\n> +\t * links that resolve to a named pipe specially.\n> +\t */\n\nExcellent.  The code structure is very clear.  We do what we have\nalready done, and in addition, treat pipes and symlinks to pipes\nspecially.\n\n> +\tif (special &&\n> +\t    (S_ISFIFO(*mode) ||\n> +\t     (S_ISLNK(*mode) && !stat(path, &st) && S_ISFIFO(st.st_mode)))) {\n> +\t\t*mode = create_ce_mode(0666);\n> +\t\t*special = SPECIAL_PIPE;\n> +\t}\n\nWhat should we do when !stat(path, &st) fails here?  It may be a\ndangling symbolic link, causes stat() to fail and we want to be\nsilent about it without changing *mode.  And that is exactly what\nthe code does.  Great.\n\nUnder what circumstances does a caller pass NULL to the special?  If\nwe were given a fifo and the caller passes NULL to the special,\n*mode at this point would be S_ISFIFO() and not 0666.  It may be\nwhat we want, but I do not immediately see why (I'll realize why\nwhile reading the \"recursing?\" bit below).\n\n> -static void populate_from_stdin(struct diff_filespec *s)\n> +static void populate_common(struct diff_filespec *s, struct strbuf *buf)\n>  {\n> -\tstruct strbuf buf = STRBUF_INIT;\n>  \tsize_t size = 0;\n\nNot a new issue, but this initialization assignment is useless as\nstrbuf_detach() would always overwrite it (or die and would not come\nback to us).\n\n> -\tif (strbuf_read(&buf, 0, 0) < 0)\n> -\t\tdie_errno(\"error while reading from stdin\");\n> -\n>  \ts->should_munmap = 0;\n> -\ts->data = strbuf_detach(&buf, &size);\n> +\ts->data = strbuf_detach(buf, &size);\n>  \ts->size = size;\n\nAgain not a new issue, but it is unfortunate that s->size is not\nsize_t but ulong, which requires us to use a temporary variable of\nthe correct type, only to assign to it.\n\n>  \ts->should_free = 1;\n>  \ts->is_stdin = 1;\n>  }\n>  \n> -static struct diff_filespec *noindex_filespec(const char *name, int mode)\n> +static void populate_from_pipe(struct diff_filespec *s)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint fd = xopen(s->path, O_RDONLY);\n> +\n> +\tif (strbuf_read(&buf, fd, 0) < 0)\n> +\t\tdie_errno(\"error while reading from '%s'\", s->path);\n> +\tclose(fd);\n> +\tpopulate_common(s, &buf);\n> +}\n> +\n> +static void populate_from_stdin(struct diff_filespec *s)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (strbuf_read(&buf, 0, 0) < 0)\n> +\t\tdie_errno(\"error while reading from stdin\");\n> +\tpopulate_common(s, &buf);\n> +}\n\nBoth look OK.  I would have named \"_common\" -> \"_finalize\" or\nsomething to signal that this is the shared \"tail\" part of the two\ncallers, but it is minor.\n\n> +static struct diff_filespec *noindex_filespec(const char *name, int mode,\n> +\t\t\t\t\t      enum special special)\n>  {\n>  \tstruct diff_filespec *s;\n>  \n>  \tif (!name)\n>  \t\tname = \"/dev/null\";\n>  \ts = alloc_filespec(name);\n>  \tfill_filespec(s, null_oid(), 0, mode);\n> -\tif (name == file_from_standard_input)\n> +\tif (special == SPECIAL_STDIN)\n>  \t\tpopulate_from_stdin(s);\n> +\telse if (special == SPECIAL_PIPE)\n> +\t\tpopulate_from_pipe(s);\n>  \treturn s;\n>  }\n\nGreat.\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, const char *name2, int recursing)\n>  {\n>  \tint mode1 = 0, mode2 = 0;\n> +\tenum special special1 = SPECIAL_NONE, special2 = SPECIAL_NONE;\n>  \n> -\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n> +\t/* Paths can only be special if we're not recursing. */\n> +\tif (get_mode(name1, &mode1, recursing ? NULL : &special1) ||\n> +\t    get_mode(name2, &mode2, recursing ? NULL : &special2))\n>  \t\treturn -1;\n\nAhh, OK.  If we are told to \"diff --no-index D1 D2\", even if we find\na fifo or symlink to a fifo, they aren't \"<(foo)\" process\nsubstitutions.  Makes sense.  And because we do not handle FIFO in\nsuch a case, we'd leave *mode (in get_mode()) to a value that we\nnever use (i.e. S_ISFIFO(*mode) is true); hopefully later part of\nthe control flow somewhere we will notice it and die?\n\n>  \tif (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n>  \t\tstruct diff_filespec *d1, *d2;\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, SPECIAL_NONE);\n> +\t\t\td2 = noindex_filespec(name2, mode2, special2);\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, special1);\n> +\t\t\td2 = noindex_filespec(NULL, 0, SPECIAL_NONE);\n>  \t\t\tname1 = NULL;\n>  \t\t\tmode1 = 0;\n>  \t\t}\n> @@ -173,7 +218,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, n2, 1);\n>  \t\t}\n>  \t\tstring_list_clear(&p1, 0);\n>  \t\tstring_list_clear(&p2, 0);\n> @@ -189,8 +234,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, special1);\n> +\t\td2 = noindex_filespec(name2, mode2, special2);\n>  \t\tdiff_queue(&diff_queued_diff, d1, d2);\n>  \t\treturn 0;\n>  \t}\n\nLooks quite straight-forward.\n\n> @@ -215,15 +260,27 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n>   */\n>  static void fixup_paths(const char **path, struct strbuf *replacement)\n>  {\n> -\tunsigned int isdir0, isdir1;\n> -\n> -\tisdir0 = path[0] != file_from_standard_input && is_directory(path[0]);\n> -\tisdir1 = path[1] != file_from_standard_input && is_directory(path[1]);\n> +\tstruct stat st;\n> +\tunsigned int isdir0 = 0, isdir1 = 0;\n> +\tunsigned int ispipe0 = 0, ispipe1 = 0;\n> +\n> +\tif (path[0] != file_from_standard_input && !stat(path[0], &st)) {\n> +\t\tisdir0 = S_ISDIR(st.st_mode);\n> +\t\tispipe0 = S_ISFIFO(st.st_mode);\n> +\t}\n> +\n> +\tif (path[1] != file_from_standard_input && !stat(path[1], &st)) {\n> +\t\tisdir1 = S_ISDIR(st.st_mode);\n> +\t\tispipe1 = S_ISFIFO(st.st_mode);\n> +\t}\n>  \n>  \tif ((path[0] == file_from_standard_input && isdir1) ||\n>  \t    (isdir0 && path[1] == file_from_standard_input))\n>  \t\tdie(_(\"cannot compare stdin to a directory\"));\n>  \n> +\tif ((isdir0 && ispipe1) || (ispipe0 && isdir1))\n> +\t\tdie(_(\"cannot compare a named pipe to a directory\"));\n> +\n\nNice.\n\n"},{"id":"480382","messageId":"20230809171731.GA3663609@coredump.intra.peff.net","threadId":"59917","inReplyTo":"4e05a0be54f66f2b394642762832e426a545426c.1688586536.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 4/4] diff --no-index: support reading from named pipes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-09T17:17:31Z","receivedAt":"2023-08-09T17:17:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 05, 2023 at 08:49:30PM +0100, Phillip Wood wrote:\n\n> +test_expect_success PIPE 'diff --no-index refuses to diff a named pipe and a directory' '\n> +\ttest_when_finished \"rm -f pipe\" &&\n> +\tmkfifo pipe &&\n> +\t{\n> +\t\t(>pipe) &\n> +\t} &&\n> +\ttest_when_finished \"kill $!\" &&\n> +\ttest_must_fail git diff --no-index -- pipe a 2>err &&\n> +\tgrep \"fatal: cannot compare a named pipe to a directory\" err\n> +'\n> +\n> +test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n> +\ttest_when_finished \"rm -f old new new-link\" &&\n> +\tmkfifo old &&\n> +\tmkfifo new &&\n> +\tln -s new new-link &&\n> +\t{\n> +\t\t(test_write_lines a b c >old) &\n> +\t} &&\n> +\ttest_when_finished \"! kill $!\" &&\n> +\t{\n> +\t\t(test_write_lines a x c >new) &\n> +\t} &&\n> +\ttest_when_finished \"! kill $!\" &&\n> +\n> +\tcat >expect <<-EOF &&\n> +\tdiff --git a/old b/new-link\n> +\t--- a/old\n> +\t+++ b/new-link\n> +\t@@ -1,3 +1,3 @@\n> +\t a\n> +\t-b\n> +\t+x\n> +\t c\n> +\tEOF\n> +\n> +\ttest_expect_code 1 git diff --no-index old new-link >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI've had t4053 hang for me once or twice in the last few days. I haven't\nmanaged to pinpoint the problem, but I did notice that running it with\n--stress seems to occasionally fail in thie \"reads from pipes\" test.\n\nThe problem is that the \"kill\" is racy. Even after we've read all of the\ninput from those sub-processes, they might still be hanging around\nwaiting to exit when our test_when_finished runs. And then kill will\nreturn \"0\". So I think we need to either:\n\n  1. Soften the when_finished to \"kill $! || true\" so that we are OK if\n     they are still there.\n\n  2. If the diff command completed as expected, it should be safe to\n     \"wait\" for each of them to confirm that they successfully wrote\n     everything. I'm not sure it buys us much over testing the output\n     from \"diff\", though.\n\nI still don't see where the hang comes from, though. It might be\nunrelated. I'll try to examine more next time I catch it in the act.\n\n-Peff\n"},{"id":"480434","messageId":"148cf4e2-e6ce-4c10-a08a-bf946ce3b95d@gmail.com","threadId":"59917","inReplyTo":"20230809171731.GA3663609@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] diff --no-index: support reading from named pipes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-10T12:56:31Z","receivedAt":"2023-08-10T12:56:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eff\n\nThanks for reporting this\n\nOn 09/08/2023 18:17, Jeff King wrote:\n> On Wed, Jul 05, 2023 at 08:49:30PM +0100, Phillip Wood wrote:\n> \n>> +test_expect_success PIPE 'diff --no-index refuses to diff a named pipe and a directory' '\n>> +\ttest_when_finished \"rm -f pipe\" &&\n>> +\tmkfifo pipe &&\n>> +\t{\n>> +\t\t(>pipe) &\n>> +\t} &&\n>> +\ttest_when_finished \"kill $!\" &&\n>> +\ttest_must_fail git diff --no-index -- pipe a 2>err &&\n>> +\tgrep \"fatal: cannot compare a named pipe to a directory\" err\n>> +'\n>> +\n>> +test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n>> +\ttest_when_finished \"rm -f old new new-link\" &&\n>> +\tmkfifo old &&\n>> +\tmkfifo new &&\n>> +\tln -s new new-link &&\n>> +\t{\n>> +\t\t(test_write_lines a b c >old) &\n>> +\t} &&\n>> +\ttest_when_finished \"! kill $!\" &&\n>> +\t{\n>> +\t\t(test_write_lines a x c >new) &\n>> +\t} &&\n>> +\ttest_when_finished \"! kill $!\" &&\n>> +\n>> +\tcat >expect <<-EOF &&\n>> +\tdiff --git a/old b/new-link\n>> +\t--- a/old\n>> +\t+++ b/new-link\n>> +\t@@ -1,3 +1,3 @@\n>> +\t a\n>> +\t-b\n>> +\t+x\n>> +\t c\n>> +\tEOF\n>> +\n>> +\ttest_expect_code 1 git diff --no-index old new-link >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n> \n> I've had t4053 hang for me once or twice in the last few days. I haven't\n> managed to pinpoint the problem, but I did notice that running it with\n> --stress seems to occasionally fail in thie \"reads from pipes\" test.\n> \n> The problem is that the \"kill\" is racy. Even after we've read all of the\n> input from those sub-processes, they might still be hanging around\n> waiting to exit when our test_when_finished runs. And then kill will\n> return \"0\". So I think we need to either:\n> \n>    1. Soften the when_finished to \"kill $! || true\" so that we are OK if\n>       they are still there.\n\nI think this is the easiest option, I'll send a patch later today\n\n>    2. If the diff command completed as expected, it should be safe to\n>       \"wait\" for each of them to confirm that they successfully wrote\n>       everything. I'm not sure it buys us much over testing the output\n>       from \"diff\", though.\n\nIf the diff output is OK that's I think that's all we really care about.\n\n> I still don't see where the hang comes from, though. It might be\n> unrelated. I'll try to examine more next time I catch it in the act.\n\nI had a look at the tests again and nothing jumped out at me. If you do \nmanage to catch it hanging we should at least we should be able to tell \nwhich test is causing the problem.\n\nThanks again,\n\nPhillip\n"}]}