{"thread":{"id":"45504","subject":"[PATCH v4 1/2] diff --no-index: optionally follow symlinks","startedAt":"2017-03-24T21:31:40Z","lastAt":"2017-03-26T03:43:20Z","messageCount":6,"participants":["Dennis Kaarsemaker","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"315334","messageId":"20170324213110.4331-2-dennis@kaarsemaker.net","threadId":"45504","inReplyTo":"20170324213110.4331-1-dennis@kaarsemaker.net","subject":"[PATCH v4 1/2] diff --no-index: optionally follow symlinks","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-03-24T21:31:09Z","receivedAt":"2017-03-24T21:31:40Z","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 be able to follow\nsymlinks, matching the behaviour of ordinary diff. A new --dereference\n(name copied from diff) option has been added to enable this behaviour.\n--no-dereference can be used to disable it again.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n Documentation/diff-options.txt |  9 +++++++++\n builtin/diff.c                 |  2 ++\n diff-no-index.c                |  7 ++++---\n diff.c                         | 12 ++++++++++--\n diff.h                         |  2 +-\n t/t4011-diff-symlink.sh        |  6 ++++++\n t/t4053-diff-no-index.sh       | 44 ++++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 76 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 2d77a19626..5a9d58b701 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -216,6 +216,15 @@ 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+--dereference::\n+--no-dereference::\n+\tNormally, \"git diff --no-index\" will compare symlinks by comparing what\n+\tthey point to. The `--dereference` option will make it compare the content\n+\tof the linked files. The `--no-dereference` option disables an earlier\n+\t`--dereference`.\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/builtin/diff.c b/builtin/diff.c\nindex 7f91f6d226..09e646060e 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -360,6 +360,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tif (nongit)\n \t\tdie(_(\"Not a git repository\"));\n \targc = setup_revisions(argc, argv, &rev, NULL);\n+\tif (DIFF_OPT_TST(&rev.diffopt, DEREFERENCE))\n+\t\tdie(_(\"--dereference can only be used together with --no-index\"));\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n \t\tdiff_setup_done(&rev.diffopt);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex f420786039..fe48f32ddd 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 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 (dereference ? stat(path, &st) : lstat(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, DEREFERENCE)) ||\n+\t\tget_mode(name2, &mode2, DIFF_OPT_TST(o, 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..2afecfb939 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,11 @@ 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, \"--dereference\"))\n+\t\tDIFF_OPT_SET(options, DEREFERENCE);\n+\telse if (!strcmp(arg, \"--no-dereference\"))\n+\t\tDIFF_OPT_CLR(options, 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..db33dc67f6 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_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/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 13e7f621ab..68ee39f26f 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -154,4 +154,10 @@ test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'diff does not accept --dereference without --no-index' '\n+    ln -s dest link1 &&\n+    ln -s dest link2 &&\n+\ttest_must_fail git diff --dereference link1 link2\n+'\n+\n test_done\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 453e6c35eb..8c87bffb34 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -127,4 +127,48 @@ 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 does not 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-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 3 4 | tail -n +3 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'diff --no-index --dereference does follows symlinks' '\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 --dereference 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.12.0-488-gd3584ba\n\n"},{"id":"315335","messageId":"20170324213110.4331-3-dennis@kaarsemaker.net","threadId":"45504","inReplyTo":"20170324213110.4331-1-dennis@kaarsemaker.net","subject":"[PATCH v4 2/2] diff --no-index: support reading from pipes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-03-24T21:31:10Z","receivedAt":"2017-03-24T21:31:51Z","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          |  9 +++++++++\n diff.c                   | 18 ++++++++++++++++--\n t/t4053-diff-no-index.sh | 10 ++++++++++\n t/test-lib.sh            |  4 ++++\n 4 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex fe48f32ddd..1262a587e5 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -83,6 +83,15 @@ 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 * To help the rest of the diff machinery along, we now override what\n+\t * canon_mode says. This is done here instead of in canon_mode, because the\n+\t * rest of git does not (and should not) support pipes.\n+\t */\n+\tif (S_ISFIFO(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 2afecfb939..4f74a54d74 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2765,6 +2765,11 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n \treturn 0;\n }\n \n+static int should_mmap_file_contents(struct stat *st)\n+{\n+\treturn S_ISREG(st->st_mode);\n+}\n+\n /*\n  * While doing rename detection and pickaxe operation, we may need to\n  * grab the data for the blob (or file) for our own in-core comparison.\n@@ -2839,9 +2844,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 (!should_mmap_file_contents(&st)) {\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 8c87bffb34..2d9b322315 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -171,4 +171,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 --dereference <(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.12.0-488-gd3584ba\n\n"},{"id":"315336","messageId":"20170324213110.4331-1-dennis@kaarsemaker.net","threadId":"45504","inReplyTo":null,"subject":"[PATCH v4 0/2] diff --no-index: support symlinks and pipes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-03-24T21:31:08Z","receivedAt":"2017-03-24T21:31:52Z","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/\nv2: http://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/\nv3: http://public-inbox.org/git/20170318210038.22638-1-dennis@kaarsemaker.net/\n\nChanges since v3:\nUsing the --dereference option without being in explicit or implicit no-index\nmode is no longer silently ignored, but an error. A test has been added for\nthis behaviour.\n\nDennis Kaarsemaker (2):\n  diff --no-index: optionally follow symlinks\n  diff --no-index: support reading from pipes\n\n Documentation/diff-options.txt |  9 +++++++\n builtin/diff.c                 |  2 ++\n diff-no-index.c                | 16 ++++++++++---\n diff.c                         | 30 +++++++++++++++++++----\n diff.h                         |  2 +-\n t/t4011-diff-symlink.sh        |  6 +++++\n t/t4053-diff-no-index.sh       | 54 ++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh                  |  4 ++++\n 8 files changed, 115 insertions(+), 8 deletions(-)\n\n-- \n2.12.0-488-gd3584ba\n\n"},{"id":"315344","messageId":"xmqqziga5lnn.fsf@gitster.mtv.corp.google.com","threadId":"45504","inReplyTo":"20170324213110.4331-2-dennis@kaarsemaker.net","subject":"Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T22:56:28Z","receivedAt":"2017-03-24T22:56:59Z","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> @@ -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 (dereference ? stat(path, &st) : lstat(path, &st))\n>  \t\treturn error(\"Could not access '%s'\", path);\n>  \telse\n>  \t\t*mode = st.st_mode;\n\nThis part makes sense---when the caller tells us to stat() we\nstat(), otherwise, we lstat().\n\n> diff --git a/diff.c b/diff.c\n> index be11e4ef2b..2afecfb939 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\nThis change is conceptually wrong.  s->mode (often) comes from the\nindex but in this codepath, after finding that s->oid is not valid\nor we want to read from the working tree instead (several lines\nbefore this part), we are committed to read from the working tree\nand check things with st.st_* fields, not s->mode, when we decide\nwhat to do with the thing we find on the filesystem, no?\n\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\nI suspect that this would conflict with a recent topic.  \n\nBut more importantly, this inserted code feels doubly wrong.\n\n - what allows us to unconditionally do \"ah, symbolic link on the\n   disk--find the target of the link, not the symbolic link itself\"?\n   We do not seem to be checking '--dereference' around here.\n\n - does this code do a reasonable thing when the path is a symbolic\n   link that points at a directory?  what does it mean to grab\n   st.st_size for such a thing (and then go on to open() and xmmap()\n   it)?\n\nPuzzled.\n\nThanks.\n\n\n   \n"},{"id":"315411","messageId":"1490477422.29662.3.camel@kaarsemaker.net","threadId":"45504","inReplyTo":"xmqqziga5lnn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2017-03-25T21:30:22Z","receivedAt":"2017-03-25T21:30:30Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Fri, 2017-03-24 at 15:56 -0700, Junio C Hamano wrote:\n\n> diff --git a/diff.c b/diff.c\n> > index be11e4ef2b..2afecfb939 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> \n> This change is conceptually wrong.  s->mode (often) comes from the\n> index but in this codepath, after finding that s->oid is not valid\n> or we want to read from the working tree instead (several lines\n> before this part), we are committed to read from the working tree\n> and check things with st.st_* fields, not s->mode, when we decide\n> what to do with the thing we find on the filesystem, no?\n\nHmm, true. It just accidentally does the right thing because s->mode\nhappens to always match the expectations of this code. I will pass on\nmore information into diff_populate_filespec so an explicit check can\nbe done here.\n\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> \n> I suspect that this would conflict with a recent topic.  \n\nPossibly. I used the same base commit for the newer versions as that\nseems to be your preference. If there is a merge conflict, do you want\nme to rebase against current master?\n\n> But more importantly, this inserted code feels doubly wrong.\n> \n>  - what allows us to unconditionally do \"ah, symbolic link on the\n>    disk--find the target of the link, not the symbolic link itself\"?\n>    We do not seem to be checking '--dereference' around here.\n\nThe implicit check above (which you already noted is faulty) allows us\nto do this. So fixing the check above will also involve fixing this.\n\n>  - does this code do a reasonable thing when the path is a symbolic\n>    link that points at a directory?  what does it mean to grab\n>    st.st_size for such a thing (and then go on to open() and xmmap()\n>    it)?\n\nNo, it does something entirely unreasonable. I hadn't even thought of\ntesting with symlinks to directories, as my ulterior motive was the\nnext commit that makes it work with pipes. This will be fixed.\n\nThanks very much for the thoroughness of your review!\n\nD.\n"},{"id":"315449","messageId":"CAPc5daV-12U2mNcvfXmy4UmYnEm2UQWGWPUX7EO3Uc15e1D-VQ@mail.gmail.com","threadId":"45504","inReplyTo":"1490477422.29662.3.camel@kaarsemaker.net","subject":"Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-26T03:42:51Z","receivedAt":"2017-03-26T03:43:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sat, Mar 25, 2017 at 2:30 PM, Dennis Kaarsemaker\n<dennis@kaarsemaker.net> wrote:\n>\n>>  - does this code do a reasonable thing when the path is a symbolic\n>>    link that points at a directory?  what does it mean to grab\n>>    st.st_size for such a thing (and then go on to open() and xmmap()\n>>    it)?\n>\n> No, it does something entirely unreasonable. I hadn't even thought of\n> testing with symlinks to directories, as my ulterior motive was the\n> next commit that makes it work with pipes. This will be fixed.\n\nTo be quite honest, I do not mind it if the \"toplevel pipe that came from the\ncommand line is treated as if it were a regular file\" was the only change in\nthis series, without doing anything for symbolic links. I do not use the\nprocess substitution myself, but I can see why sometimes it is handy to\npass two process invocations on the command line of \"diff\" (if it were only\none, then \"-\" with the usual redirection already works, but you cannot do\ntwo command using that syntax).\n\nPerhaps we can have only that part and perfect it first and have it ready for\nthe next release, postponing the symlink dereferencing, which is a different\nissue?\n"}]}