{"thread":{"id":"44870","subject":"[PATCH v2 0/2] diff --no-index: support symlinks and pipes","startedAt":"2017-01-13T10:28:49Z","lastAt":"2017-01-13T23:37:44Z","messageCount":5,"participants":["Dennis Kaarsemaker","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"309348","messageId":"20170113102021.6054-1-dennis@kaarsemaker.net","threadId":"44870","inReplyTo":null,"subject":"[PATCH v2 0/2] diff --no-index: support symlinks and pipes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-01-13T10:20:19Z","receivedAt":"2017-01-13T10:28:49Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"git diff <(command1) <(command2) is less useful than it could be, all it outputs is:\n\ndiff --git a/dev/fd/63 b/dev/fd/62\nindex 9e6542b297..9f7b2c291b 120000\n--- a/dev/fd/63\n+++ b/dev/fd/62\n@@ -1 +1 @@\n-pipe:[464811685]\n\\ No newline at end of file\n+pipe:[464811687]\n\\ No newline at end of file\n\nNormal diff provides arguably better output: the diff of the output of the\ncommands. This series makes it possible for git diff --no-index to follow\nsymlinks and read from pipes, mimicking the behaviour of normal diff.\n\nv1: http://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/\n\nChanges since the RFC/v1 patch:\n- Following symlinks is now the default. I think an accurate summary of the\n  discussion on v1 is that this behaviour is useful enough to be the default,\n  but to add an escape hatch. That escape hatch is named --no-dereference, name\n  stolen from gnu diff.\n- Added tests and documentation\n\nSpecifically not changed:\nThese changes affect only diff --no-index. Using --no-dereference is an error\nwithout --no-index.\n\nDennis Kaarsemaker (2):\n  diff --no-index: follow symlinks\n  diff --no-index: support reading from pipes\n\n Documentation/diff-options.txt |  7 +++++++\n diff-no-index.c                | 15 ++++++++++++---\n diff.c                         | 23 +++++++++++++++++++----\n diff.h                         |  2 +-\n t/t4053-diff-no-index.sh       | 40 ++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh                  |  4 ++++\n 6 files changed, 83 insertions(+), 8 deletions(-)\n\n-- \n2.11.0-234-gaf85957\n\n"},{"id":"309349","messageId":"20170113102021.6054-3-dennis@kaarsemaker.net","threadId":"44870","inReplyTo":"20170113102021.6054-1-dennis@kaarsemaker.net","subject":"[PATCH v2 2/2] diff --no-index: support reading from pipes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-01-13T10:20:21Z","receivedAt":"2017-01-13T10:28:52Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"diff <(command1) <(command2) provides useful output, let's make it\npossible for git to do the same.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n diff-no-index.c          |  8 ++++++++\n diff.c                   | 13 +++++++++++--\n t/t4053-diff-no-index.sh | 10 ++++++++++\n t/test-lib.sh            |  4 ++++\n 4 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 826fe97ffc..2123da435b 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -83,6 +83,14 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\n \t\tname = \"/dev/null\";\n \ts = alloc_filespec(name);\n \tfill_filespec(s, null_sha1, 0, mode);\n+\t/*\n+\t * In --no-index mode, we support reading from pipes. canon_mode, called by\n+\t * fill_filespec, gets confused by this and thinks we now have subprojects.\n+\t * Detect this and tell the rest of the diff machinery to treat pipes as\n+\t * normal files.\n+\t */\n+\tif (S_ISGITLINK(s->mode))\n+\t\ts->mode = S_IFREG | ce_permissions(mode);\n \tif (name == file_from_standard_input)\n \t\tpopulate_from_stdin(s);\n \treturn s;\ndiff --git a/diff.c b/diff.c\nindex 2fc0226338..bb04eab331 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\tfd = open(s->path, O_RDONLY);\n \t\tif (fd < 0)\n \t\t\tgoto err_empty;\n-\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tif (!S_ISREG(st.st_mode)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_read(&sb, fd, 0);\n+\t\t\ts->size = sb.len;\n+\t\t\ts->data = strbuf_detach(&sb, NULL);\n+\t\t\ts->should_free = 1;\n+\t\t}\n+\t\telse {\n+\t\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\t\ts->should_munmap = 1;\n+\t\t}\n \t\tclose(fd);\n-\t\ts->should_munmap = 1;\n \n \t\t/*\n \t\t * Convert from working tree format to canonical git format\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex c6046fef19..ba343566c0 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -157,4 +157,14 @@ test_expect_success SYMLINKS 'diff --no-index --no-dereference does not follow s\n \ttest_cmp expect actual\n '\n \n+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '\n+\tcat >expect <<\\EOF\n+\t\t@@ -1 +1 @@\n+\t\t-1\n+\t\t+2\n+\tEOF\n+\ttest_expect_code 1 git diff --no-index <(echo 1) <(echo 2) | tail -n +5 > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 11562bde10..78f3d24651 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1128,3 +1128,7 @@ build_option () {\n test_lazy_prereq LONG_IS_64BIT '\n \ttest 8 -le \"$(build_option sizeof-long)\"\n '\n+\n+test_lazy_prereq PROCESS_SUBSTITUTION '\n+\teval \"foo=<(echo test)\" 2>/dev/null\n+'\n-- \n2.11.0-234-gaf85957\n\n"},{"id":"309350","messageId":"20170113102021.6054-2-dennis@kaarsemaker.net","threadId":"44870","inReplyTo":"20170113102021.6054-1-dennis@kaarsemaker.net","subject":"[PATCH v2 1/2] diff --no-index: follow symlinks","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-01-13T10:20:20Z","receivedAt":"2017-01-13T10:28:59Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Git's diff machinery does not follow symlinks, which makes sense as git\nitself also does not, but stores the symlink destination.\n\nIn --no-index mode however, it is useful for diff to to follow symlinks,\nmatching the behaviour of ordinary diff. A new --no-dereference (name\ncopied from diff) option has been added to disable this behaviour.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n Documentation/diff-options.txt |  7 +++++++\n diff-no-index.c                |  7 ++++---\n diff.c                         | 10 ++++++++--\n diff.h                         |  2 +-\n t/t4053-diff-no-index.sh       | 30 ++++++++++++++++++++++++++++++\n 5 files changed, 50 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 2d77a19626..48bcf3cc5e 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -216,6 +216,13 @@ any of those replacements occurred.\n \tcommit range.  Defaults to `diff.submodule` or the 'short' format\n \tif the config option is unset.\n \n+ifdef::git-diff[]\n+--no-dereference::\n+\tNormally, \"git diff --no-index\" will dereference symlinks and compare\n+\tthe contents of the linked files, mimicking ordinary diff. This\n+\toption disables that behaviour.\n+endif::git-diff[]\n+\n --color[=<when>]::\n \tShow colored diff.\n \t`--color` (i.e. without '=<when>') is the same as `--color=always`.\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex f420786039..826fe97ffc 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -40,7 +40,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 *mode, int no_dereference)\n {\n \tstruct stat st;\n \n@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)\n #endif\n \telse if (path == file_from_standard_input)\n \t\t*mode = create_ce_mode(0666);\n-\telse if (lstat(path, &st))\n+\telse if (no_dereference ? lstat(path, &st) : stat(path, &st))\n \t\treturn error(\"Could not access '%s'\", path);\n \telse\n \t\t*mode = st.st_mode;\n@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,\n {\n \tint mode1 = 0, mode2 = 0;\n \n-\tif (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n+\tif (get_mode(name1, &mode1, DIFF_OPT_TST(o, NO_DEREFERENCE)) ||\n+\t\tget_mode(name2, &mode2, DIFF_OPT_TST(o, NO_DEREFERENCE)))\n \t\treturn -1;\n \n \tif (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\ndiff --git a/diff.c b/diff.c\nindex be11e4ef2b..2fc0226338 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\ts->size = xsize_t(st.st_size);\n \t\tif (!s->size)\n \t\t\tgoto empty;\n-\t\tif (S_ISLNK(st.st_mode)) {\n+\t\tif (S_ISLNK(s->mode)) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t\tif (strbuf_readlink(&sb, s->path, s->size))\n@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n+\t\tif (S_ISLNK(st.st_mode)) {\n+\t\t\tstat(s->path, &st);\n+\t\t\ts->size = xsize_t(st.st_size);\n+\t\t}\n \t\tif (size_only)\n \t\t\treturn 0;\n \t\tif ((flags & CHECK_BINARY) &&\n@@ -3884,7 +3888,9 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-follow\")) {\n \t\tDIFF_OPT_CLR(options, FOLLOW_RENAMES);\n \t\tDIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);\n-\t} else if (!strcmp(arg, \"--color\"))\n+\t} else if (!strcmp(arg, \"--no-dereference\"))\n+\t\tDIFF_OPT_SET(options, NO_DEREFERENCE);\n+\telse if (!strcmp(arg, \"--color\"))\n \t\toptions->use_color = 1;\n \telse if (skip_prefix(arg, \"--color=\", &arg)) {\n \t\tint value = git_config_colorbool(NULL, arg);\ndiff --git a/diff.h b/diff.h\nindex 25ae60d5ff..74883db1eb 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)\n #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)\n #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)\n-/* (1 <<  9) unused */\n+#define DIFF_OPT_NO_DEREFERENCE      (1 <<  9)\n #define DIFF_OPT_HAS_CHANGES         (1 << 10)\n #define DIFF_OPT_QUICK               (1 << 11)\n #define DIFF_OPT_NO_INDEX            (1 << 12)\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 453e6c35eb..c6046fef19 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -127,4 +127,34 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit)\n \ttest_cmp expect actual.head\n '\n \n+test_expect_success SYMLINKS 'diff --no-index follows symlinks' '\n+\techo a >1 &&\n+\techo b >2 &&\n+\tln -s 1 3 &&\n+\tln -s 2 4 &&\n+\tcat >expect <<\\EOF\n+\t\t--- a/3\n+\t\t+++ b/4\n+\t\t@@ -1 +1 @@\n+\t\t-a\n+\t\t+b\n+\tEOF\n+\ttest_expect_code 1 git diff --no-index 3 4 | tail -n +3 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'diff --no-index --no-dereference does not follow symlinks' '\n+\tcat >expect <<\\EOF\n+\t\t--- a/3\n+\t\t+++ b/4\n+\t\t@@ -1 +1 @@\n+\t\t-1\n+\t\t\\ No newline at end of file\n+\t\t+2\n+\t\t\\ No newline at end of file\n+\tEOF\n+\ttest_expect_code 1 git diff --no-index --no-dereference 3 4 | tail -n +3 > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.11.0-234-gaf85957\n\n"},{"id":"309401","messageId":"xmqqo9zaee6t.fsf@gitster.mtv.corp.google.com","threadId":"44870","inReplyTo":"20170113102021.6054-3-dennis@kaarsemaker.net","subject":"Re: [PATCH v2 2/2] diff --no-index: support reading from pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-13T23:24:26Z","receivedAt":"2017-01-13T23:24:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> +\t/*\n> +\t * In --no-index mode, we support reading from pipes. canon_mode, called by\n> +\t * fill_filespec, gets confused by this and thinks we now have subprojects.\n> +\t * Detect this and tell the rest of the diff machinery to treat pipes as\n> +\t * normal files.\n> +\t */\n> +\tif (S_ISGITLINK(s->mode))\n> +\t\ts->mode = S_IFREG | ce_permissions(mode);\n\nHmph.  Pipes on your system may satisfy S_ISGITLINK() and confuse\nlater code, and this hack may work it around.  But a proper gitlink\nthat was thrown at this codepath (probably by mistake) will also be\ncaught and pretend as if it were a regular file.  Do we know for\ncertain that pipes everywhere will be munged to appear as\nS_ISGITLINK()?  Is it possible to do the \"are we looking at an end\nof a pipe?\" check _before_ canon_mode() munges and stores the result\nin s->mode in diff-no-index.c somewhere, perhaps inside get_mode()?\n\n> diff --git a/diff.c b/diff.c\n> index 2fc0226338..bb04eab331 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n>  \t\tfd = open(s->path, O_RDONLY);\n>  \t\tif (fd < 0)\n>  \t\t\tgoto err_empty;\n> -\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\t\tif (!S_ISREG(st.st_mode)) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_read(&sb, fd, 0);\n> +\t\t\ts->size = sb.len;\n> +\t\t\ts->data = strbuf_detach(&sb, NULL);\n> +\t\t\ts->should_free = 1;\n> +\t\t}\n> +\t\telse {\n> +\t\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\t\t\ts->should_munmap = 1;\n> +\t\t}\n>  \t\tclose(fd);\n> -\t\ts->should_munmap = 1;\n\nI like the fact that, by extending the !S_ISREG() check this patch\nintroduces, we can later use the new \"do not mmap but allocate to\nread\" codepath for small files, which may be more efficient.  We may\nwant to have a small helper\n\n\tstatic int should_mmap_file_contents(struct stat *st)\n\t{\n\t\treturn S_ISREG(st->st_mode);\n\t}\n\nso that we can do such an enhancement later more easily.\n\nSo, I am skeptical with the \"do we have pipe\" check in the earlier\nhunk, but otherwise I think what this patch wanted to solve is a\nreasonable problem to tackle.\n\nThanks.\n"},{"id":"309402","messageId":"xmqqk29yedkv.fsf@gitster.mtv.corp.google.com","threadId":"44870","inReplyTo":"20170113102021.6054-2-dennis@kaarsemaker.net","subject":"Re: [PATCH v2 1/2] diff --no-index: follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-13T23:37:36Z","receivedAt":"2017-01-13T23:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> Git's diff machinery does not follow symlinks, which makes sense as git\n> itself also does not, but stores the symlink destination.\n>\n> In --no-index mode however, it is useful for diff to to follow symlinks,\n> matching the behaviour of ordinary diff. A new --no-dereference (name\n> copied from diff) option has been added to disable this behaviour.\n\nIf you add a --no-dereference option, --dereference option should\nalso be there, so that \"--no-dereference\" earlier on the command\nline (perhaps coming from a configured alias) can be countermanded.\n\nWhile I am not opposed to giving an optional feature to treat a\nsymlink as if it is a regular file with the contents of its link\ntarget, I am not enthused that this patch tries to make that the\ndefault behaviour.  We are not matching the behaviour of ordinary\ndiff anyway [*1*].\n\nIt probably makes more sense for our first step to introduce this\nfeature that is only enabled when \"--dereference\" option is given.\nMaking it the default for \"--no-index\" case should be discussed as\na separate step.\n\n[Footnote]\n\n*1* E.g. \"git diff --no-index dirA/ dirB/\" does not say \"Only in\ndirA: file\".  It also recurses into subdirectories of dirA/ and\ndirB/ without the --recursive option.\n"}]}