{"thread":{"id":"54257","subject":"[PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","startedAt":"2020-09-18T11:39:47Z","lastAt":"2020-09-24T17:39:03Z","messageCount":50,"participants":["Thomas Guyot-Sionnest","Taylor Blau","Jeff King","Junio C Hamano","brian m. carlson","Thomas Guyot","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"405852","messageId":"20200918113256.8699-3-tguyot@gmail.com","threadId":"54257","inReplyTo":"20200918113256.8699-1-tguyot@gmail.com","subject":"[PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T11:32:56Z","receivedAt":"2020-09-18T11:39:47Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"A very handy way to pass data to applications is to use the <() process\nsubstitution syntax in bash variants. It allow comparing files streamed\nfrom a remote server or doing on-the-fly stream processing to alter the\ndiff. These are usually implemented as a symlink that points to a bogus\nname (ex \"pipe:[209326419]\") but opens as a pipe.\n\nGit normally tracks symlinks targets. This patch makes it detect such\npipes in --no-index mode and read the file normally like it would do for\nstdin (\"-\"), so they can be compared directly.\n\nSigned-off-by: Thomas Guyot-Sionnest \n---\n diff-no-index.c | 56 ++++++++++--\n t/t4053-diff-no-index.sh | 189 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 238 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7814eabfe0..779c686d23 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -41,6 +41,33 @@ static int read_directory_contents(const char *path, struct string_list *list)\n */\n static const char file_from_standard_input[] = \"-\";\n \n+/* Check that file is - (STDIN) or unnamed pipe - explicitly\n+ * avoid on-disk named pipes which could block\n+ */\n+static int ispipe(const char *name)\n+{\n+\tstruct stat st;\n+\n+\tif (name == file_from_standard_input)\n+\t\treturn 1; /* STDIN */\n+\n+\tif (!lstat(name, &st)) {\n+\t\tif (S_ISLNK(st.st_mode)) {\n+\t\t\t/* symlink - read it and check it doesn't exists\n+\t\t\t * as a file yet link to a pipe */\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_realpath(&sb, name, 0);\n+\t\t\t/* We're abusing strbuf_realpath here, it may append\n+\t\t\t * pipe:[NNNNNNNNN] to an abs path */\n+\t\t\tif (!stat(sb.buf, &st))\n+\t\t\t\treturn 0; /* link target exists , not pipe */\n+\t\t\tif (!stat(name, &st))\n+\t\t\t\treturn S_ISFIFO(st.st_mode);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static int get_mode(const char *path, int *mode)\n {\n \tstruct stat st;\n@@ -51,7 +78,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 (ispipe(path))\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 +87,13 @@ 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 int populate_from_fd(struct diff_filespec *s, int fd)\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+\tif (strbuf_read(&buf, fd, 0) < 0)\n+\t\treturn error_errno(_(\"error while reading from fd %i\"), fd);\n \n \ts->should_munmap = 0;\n \ts->data = strbuf_detach(&buf, &size);\n@@ -76,6 +103,20 @@ static int populate_from_stdin(struct diff_filespec *s)\n \treturn 0;\n }\n \n+static int populate_from_pipe(struct diff_filespec *s, const char *name)\n+{\n+\tint ret;\n+\tFILE *f;\n+\n+\tf = fopen(name, \"r\");\n+\tif (!f)\n+\t\treturn error_errno(_(\"cannot open %s\"), name);\n+\n+\tret = populate_from_fd(s, fileno(f));\n+\tfclose(f);\n+\treturn ret;\n+}\n+\n static struct diff_filespec *noindex_filespec(const char *name, int mode)\n {\n \tstruct diff_filespec *s;\n@@ -85,7 +126,9 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)\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+\t\tpopulate_from_fd(s, 0);\n+\telse if (ispipe(name))\n+\t\tpopulate_from_pipe(s, name);\n \treturn s;\n }\n \n@@ -218,8 +261,7 @@ 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+\tif (ispipe(path[0]) || ispipe(path[1]))\n \t\treturn;\n \tisdir0 = is_directory(path[0]);\n \tisdir1 = is_directory(path[1]);\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 0168946b63..e49f773515 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -144,4 +144,193 @@ test_expect_success 'diff --no-index allows external diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff --no-index can diff piped subshells' '\n+\techo 1 >non/git/c &&\n+\ttest_expect_code 0 git diff --no-index non/git/b <(cat non/git/c) &&\n+\ttest_expect_code 0 git diff --no-index <(cat non/git/b) non/git/c &&\n+\ttest_expect_code 0 git diff --no-index <(cat non/git/b) <(cat non/git/c) &&\n+\ttest_expect_code 0 cat non/git/b | git diff --no-index - non/git/c &&\n+\ttest_expect_code 0 cat non/git/c | git diff --no-index non/git/b - &&\n+\ttest_expect_code 0 cat non/git/b | git diff --no-index - <(cat non/git/c) &&\n+\ttest_expect_code 0 cat non/git/c | git diff --no-index <(cat non/git/b) -\n+'\n+\n+test_expect_success 'diff --no-index finds diff in piped subshells' '\n+\t(\n+\t\tset -- <(cat /dev/null) <(cat /dev/null)\n+\t\tcat <<-EOF >expect\n+\t\t\tdiff --git a$1 b$2\n+\t\t\t--- a$1\n+\t\t\t+++ b$2\n+\t\t\t@@ -1 +1 @@\n+\t\t\t-1\n+\t\t\t+2\n+\t\tEOF\n+\t) &&\n+\ttest_expect_code 1 \\\n+\t\tgit diff --no-index <(cat non/git/b) <(sed s/1/2/ non/git/c) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --no-index with stat and numstat' '\n+\t(\n+\t\tset -- <(cat /dev/null) <(cat /dev/null)\n+\t\tmin=$((${#1} < ${#2} ? ${#1} : ${#2}))\n+\t\tfor ((i=0; iexpect1\n+\t\t\t $base{${1#$base} => ${2#$base}} | 2 +-\n+\t\t\t 1 file changed, 1 insertion(+), 1 deletion(-)\n+\t\tEOF\n+\t\tcat <<-EOF >expect2\n+\t\t\t1\t1\t$base{${1#$base} => ${2#$base}}\n+\t\tEOF\n+\t) &&\n+\ttest_expect_code 1 \\\n+\t\tgit diff --no-index --stat <(cat non/git/a) <(sed s/1/2/ non/git/b) >actual &&\n+\ttest_cmp expect1 actual &&\n+\ttest_expect_code 1 \\\n+\t\tgit diff --no-index --numstat <(cat non/git/a) <(sed s/1/2/ non/git/b) >actual &&\n+\ttest_cmp expect2 actual\n+'\n+\n+test_expect_success PIPE 'diff --no-index on filesystem pipes' '\n+\t(\n+\t\tcd non/git &&\n+\t\tmkdir f g &&\n+\t\tmkfifo f/1 g/1 &&\n+\t\ttest_expect_code 128 git diff --no-index f g &&\n+\t\ttest_expect_code 128 git diff --no-index ../../a f &&\n+\t\ttest_expect_code 128 git diff --no-index g ../../a &&\n+\t\ttest_expect_code 128 git diff --no-index f/1 g/1 &&\n+\t\ttest_expect_code 128 git diff --no-index f/1 ../../a/1 &&\n+\t\ttest_expect_code 128 git diff --no-index ../../a/1 g/1\n+\t)\n+'\n+\n+test_expect_success PIPE 'diff --no-index reads symlinks to named pipes as symlinks' '\n+\t(\n+\t\tcd non/git &&\n+\t\tmkdir h i &&\n+\t\tln -s ../f/1 h/1 &&\n+\t\tln -s ../g/1 i/1 &&\n+\t\ttest_expect_code 1 git diff --no-index h i >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/h/1 b/i/1\n+\t\t\tindex d0b5850..d8b9c34 120000\n+\t\t\t--- a/h/1\n+\t\t\t+++ b/i/1\n+\t\t\t@@ -1 +1 @@\n+\t\t\t-../f/1\n+\t\t\t\\ No newline at end of file\n+\t\t\t+../g/1\n+\t\t\t\\ No newline at end of file\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\ttest_expect_code 1 git diff --no-index ../../a h >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/../../a/1 b/../../a/1\n+\t\t\tdeleted file mode 100644\n+\t\t\tindex d00491f..0000000\n+\t\t\t--- a/../../a/1\n+\t\t\t+++ /dev/null\n+\t\t\t@@ -1 +0,0 @@\n+\t\t\t-1\n+\t\t\tdiff --git a/h/1 b/h/1\n+\t\t\tnew file mode 120000\n+\t\t\tindex 0000000..d0b5850\n+\t\t\t--- /dev/null\n+\t\t\t+++ b/h/1\n+\t\t\t@@ -0,0 +1 @@\n+\t\t\t+../f/1\n+\t\t\t\\ No newline at end of file\n+\t\t\tdiff --git a/../../a/2 b/../../a/2\n+\t\t\tdeleted file mode 100644\n+\t\t\tindex 0cfbf08..0000000\n+\t\t\t--- a/../../a/2\n+\t\t\t+++ /dev/null\n+\t\t\t@@ -1 +0,0 @@\n+\t\t\t-2\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\ttest_expect_code 1 git diff --no-index i ../../a >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/i/1 b/i/1\n+\t\t\tdeleted file mode 120000\n+\t\t\tindex d8b9c34..0000000\n+\t\t\t--- a/i/1\n+\t\t\t+++ /dev/null\n+\t\t\t@@ -1 +0,0 @@\n+\t\t\t-../g/1\n+\t\t\t\\ No newline at end of file\n+\t\t\tdiff --git a/../../a/1 b/../../a/1\n+\t\t\tnew file mode 100644\n+\t\t\tindex 0000000..d00491f\n+\t\t\t--- /dev/null\n+\t\t\t+++ b/../../a/1\n+\t\t\t@@ -0,0 +1 @@\n+\t\t\t+1\n+\t\t\tdiff --git a/../../a/2 b/../../a/2\n+\t\t\tnew file mode 100644\n+\t\t\tindex 0000000..0cfbf08\n+\t\t\t--- /dev/null\n+\t\t\t+++ b/../../a/2\n+\t\t\t@@ -0,0 +1 @@\n+\t\t\t+2\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\ttest_expect_code 1 git diff --no-index h/1 i/1 >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/h/1 b/i/1\n+\t\t\tindex d0b5850..d8b9c34 120000\n+\t\t\t--- a/h/1\n+\t\t\t+++ b/i/1\n+\t\t\t@@ -1 +1 @@\n+\t\t\t-../f/1\n+\t\t\t\\ No newline at end of file\n+\t\t\t+../g/1\n+\t\t\t\\ No newline at end of file\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\ttest_expect_code 1 git diff --no-index h/1 ../../a/1 >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/h/1 b/h/1\n+\t\t\tdeleted file mode 120000\n+\t\t\tindex d0b5850..0000000\n+\t\t\t--- a/h/1\n+\t\t\t+++ /dev/null\n+\t\t\t@@ -1 +0,0 @@\n+\t\t\t-../f/1\n+\t\t\t\\ No newline at end of file\n+\t\t\tdiff --git a/../../a/1 b/../../a/1\n+\t\t\tnew file mode 100644\n+\t\t\tindex 0000000..d00491f\n+\t\t\t--- /dev/null\n+\t\t\t+++ b/../../a/1\n+\t\t\t@@ -0,0 +1 @@\n+\t\t\t+1\n+\t\tEOF\n+\t\ttest_cmp expect actual &&\n+\t\ttest_expect_code 1 git diff --no-index ../../a/1 i/1 >actual &&\n+\t\tcat <<-EOF >expect &&\n+\t\t\tdiff --git a/../../a/1 b/../../a/1\n+\t\t\tdeleted file mode 100644\n+\t\t\tindex d00491f..0000000\n+\t\t\t--- a/../../a/1\n+\t\t\t+++ /dev/null\n+\t\t\t@@ -1 +0,0 @@\n+\t\t\t-1\n+\t\t\tdiff --git a/i/1 b/i/1\n+\t\t\tnew file mode 120000\n+\t\t\tindex 0000000..d8b9c34\n+\t\t\t--- /dev/null\n+\t\t\t+++ b/i/1\n+\t\t\t@@ -0,0 +1 @@\n+\t\t\t+../g/1\n+\t\t\t\\ No newline at end of file\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.20.1\n\n"},{"id":"405853","messageId":"20200918113256.8699-2-tguyot@gmail.com","threadId":"54257","inReplyTo":"20200918113256.8699-1-tguyot@gmail.com","subject":"[PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T11:32:55Z","receivedAt":"2020-09-18T11:39:48Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"In builtin_diffstat(), when both files are coming from \"stdin\" (which\ncould be better described as the file's content being written directly\ninto the file object), oideq() compares two null hashes and ignores the\nactual differences for the statistics.\n\nThis patch checks if is_stdin flag is set on both sides and compare\ncontents directly.\n\nSigned-off-by: Thomas Guyot-Sionnest \n---\n diff.c | 5 ++++-\n t/t3206-range-diff.sh | 8 ++++----\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a5114fa864..2995527896 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3681,7 +3681,10 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n-\tsame_contents = oideq(&one->oid, &two->oid);\n+\tif (one->is_stdin && two->is_stdin)\n+\t\tsame_contents = !strcmp(one->data, two->data);\n+\telse\n+\t\tsame_contents = oideq(&one->oid, &two->oid);\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e024cff65c..4715e75b68 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -258,11 +258,11 @@ test_expect_success 'changed commit with --stat diff option' '\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \t4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \tEOF\n \ttest_cmp expect actual\n '\n-- \n2.20.1\n\n"},{"id":"405854","messageId":"20200918113256.8699-1-tguyot@gmail.com","threadId":"54257","inReplyTo":null,"subject":"Allow passing pipes to diff --no-index + bugfix","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T11:32:54Z","receivedAt":"2020-09-18T11:39:49Z","isPatch":false,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hello,\n\nThis set of patches adds the ability to generate diffs directly from\nshell process substitution using the <(...) syntax. This is extremely\nuseful to generate diffs with files streamed directly form remote\nsystems or when it may be useful to filter them on the fly to generate\nthe diffs.\n\nFor example:\n\n $ git diff --stat \\\n <(sed -r 's/^\\S+\\s//' /boot/System.map-4.19.0-8-amd64|sort) \\\n\t<(sed -r 's/^\\S+\\s//' /boot/System.map-4.19.0-9-amd64|sort)\n\n /dev/fd/{63 => 62} | 9500 ++++++++++++++++++++++----------------------\n 1 file changed, 4789 insertions(+), 4711 deletions(-)\n\n\nAlong with it a small fix in --stat and --numstat that affected one one\ngit range-diff test, where added/removed lines stts were missing (needed\nfor difffing the pipes too)\n\nRegards,\n\nThomas Guyot-Sionnest\n\n\n"},{"id":"405866","messageId":"20200918143647.GB1606445@nand.local","threadId":"54257","inReplyTo":"20200918113256.8699-3-tguyot@gmail.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-18T14:36:47Z","receivedAt":"2020-09-18T14:36:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Thomas,\n\nOn Fri, Sep 18, 2020 at 07:32:56AM -0400, Thomas Guyot-Sionnest wrote:\n> A very handy way to pass data to applications is to use the <() process\n> substitution syntax in bash variants. It allow comparing files streamed\n> from a remote server or doing on-the-fly stream processing to alter the\n> diff. These are usually implemented as a symlink that points to a bogus\n> name (ex \"pipe:[209326419]\") but opens as a pipe.\n\nThis is true in bash, but sh does not support process substitution with\n<().\n\n> Git normally tracks symlinks targets. This patch makes it detect such\n> pipes in --no-index mode and read the file normally like it would do for\n> stdin (\"-\"), so they can be compared directly.\n>\n> Signed-off-by: Thomas Guyot-Sionnest \n> ---\n> diff-no-index.c | 56 ++++++++++--\n> t/t4053-diff-no-index.sh | 189 +++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 238 insertions(+), 7 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 7814eabfe0..779c686d23 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -41,6 +41,33 @@ static int read_directory_contents(const char *path, struct string_list *list)\n> */\n> static const char file_from_standard_input[] = \"-\";\n>\n> +/* Check that file is - (STDIN) or unnamed pipe - explicitly\n> + * avoid on-disk named pipes which could block\n> + */\n> +static int ispipe(const char *name)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (name == file_from_standard_input)\n> +\t\treturn 1; /* STDIN */\n> +\n> +\tif (!lstat(name, &st)) {\n> +\t\tif (S_ISLNK(st.st_mode)) {\n\nI had to read this a few times to make sure that I got it; you want to\nstat the link itself, and then check that it links to a pipe.\n\nI'm not sure why, though. Do you want to avoid handling named FIFOs in\nthe code below? Your comment that they \"could block\" makes me think you\ndo, but I don't know why that would be a problem.\n\n> +\t\t\t/* symlink - read it and check it doesn't exists\n> +\t\t\t * as a file yet link to a pipe */\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_realpath(&sb, name, 0);\n> +\t\t\t/* We're abusing strbuf_realpath here, it may append\n> +\t\t\t * pipe:[NNNNNNNNN] to an abs path */\n> +\t\t\tif (!stat(sb.buf, &st))\n\nStatting sb.buf is confusing to me (especially when followed up by\nanother stat right below. Could you explain?\n\n> +test_expect_success 'diff --no-index can diff piped subshells' '\n> +\techo 1 >non/git/c &&\n> +\ttest_expect_code 0 git diff --no-index non/git/b <(cat non/git/c) &&\n> +\ttest_expect_code 0 git diff --no-index <(cat non/git/b) non/git/c &&\n> +\ttest_expect_code 0 git diff --no-index <(cat non/git/b) <(cat non/git/c) &&\n> +\ttest_expect_code 0 cat non/git/b | git diff --no-index - non/git/c &&\n> +\ttest_expect_code 0 cat non/git/c | git diff --no-index non/git/b - &&\n> +\ttest_expect_code 0 cat non/git/b | git diff --no-index - <(cat non/git/c) &&\n> +\ttest_expect_code 0 cat non/git/c | git diff --no-index <(cat non/git/b) -\n> +'\n\nIndeed this test fails (Git thinks that the HERE-DOC is broken, but I\nsuspect it's just getting confused by the '<()'). This test (like almost\nall other tests in Git) use /bin/sh as its shebang. Does your /bin/sh\nactually point to bash?\n\nIf you did want to test something like this, you'd need to source\nt/lib-bash.sh instead of t/test-lib.sh.\n\nUnrelated to the above comment, but there are a few small style nits\nthat I notice:\n\n - There is no need to run with 'test_expect_code 0' since the test is\n marked as 'test_expect_success' and the commands are all in an '&&'\n chain. (This does appear to be common style for others in t4053, so\n you may just be matching it--which is fine--but an additional\n clean-up on top to modernize would be appreciated, too).\n\n - The cat pipe is unnecessary, and is also violating a rule that we\n don't place 'git' on the right-hand side of a pipe (can you redirect\n the file at the end instead?).\n\nDocumentation/CodingGuidelines is a great place to look if you are ever\ncurious about whether something is in good style.\n\n> +test_expect_success 'diff --no-index finds diff in piped subshells' '\n> +\t(\n> +\t\tset -- <(cat /dev/null) <(cat /dev/null)\n\nWhy is this necessary?\n\n> +\t\tcat <<-EOF >expect\n> +\t\t\tdiff --git a$1 b$2\n> +\t\t\t--- a$1\n> +\t\t\t+++ b$2\n> +\t\t\t@@ -1 +1 @@\n> +\t\t\t-1\n> +\t\t\t+2\n> +\t\tEOF\n> +\t) &&\n> +\ttest_expect_code 1 \\\n> +\t\tgit diff --no-index <(cat non/git/b) <(sed s/1/2/ non/git/c) >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThanks,\nTaylor\n"},{"id":"405867","messageId":"20200918144651.GA1612043@nand.local","threadId":"54257","inReplyTo":"20200918113256.8699-2-tguyot@gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-18T14:46:51Z","receivedAt":"2020-09-18T14:46:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 18, 2020 at 07:32:55AM -0400, Thomas Guyot-Sionnest wrote:\n> In builtin_diffstat(), when both files are coming from \"stdin\" (which\n> could be better described as the file's content being written directly\n> into the file object), oideq() compares two null hashes and ignores the\n> actual differences for the statistics.\n>\n> This patch checks if is_stdin flag is set on both sides and compare\n> contents directly.\n>\n> Signed-off-by: Thomas Guyot-Sionnest \n> ---\n> diff.c | 5 ++++-\n> t/t3206-range-diff.sh | 8 ++++----\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index a5114fa864..2995527896 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3681,7 +3681,10 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\treturn;\n> \t}\n>\n> -\tsame_contents = oideq(&one->oid, &two->oid);\n> +\tif (one->is_stdin && two->is_stdin)\n> +\t\tsame_contents = !strcmp(one->data, two->data);\n\nHmm. A couple of thoughts here:\n\n - strcmp seems like a slow-down here, since we'll have to go through\n at worst the smaller of one->data and two->data to compare each of\n them.\n\n - strcmp is likely not the right way to do that, since we could be\n diffing binary content, in which case we'd want to continue over\n NULs and instead stop at a fixed length (the minimum of the length\n of one->data and two->data, specifically). I'd have expected memcmp\n here instead.\n\n - Why do we have to do this at all all the way up in\n 'builtin_diffstat'? I would expect these to contain the right\n OIDs by the time they are given back to us from the call to\n 'diff_fill_oid_info' in 'run_diffstat'.\n\nSo, my last point is the most important of the three. I'd expect\nsomething more along the lines of:\n\n 1. diff_fill_oid_info resolve the link to the pipe, and\n 2. index_path handles the resolved fd.\n\n...but it looks like that is already what it's doing? I'm confused why\nthis doesn't work as-is.\n\n> +\telse\n> +\t\tsame_contents = oideq(&one->oid, &two->oid);\n>\n> \tif (diff_filespec_is_binary(o->repo, one) ||\n> \t diff_filespec_is_binary(o->repo, two)) {\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index e024cff65c..4715e75b68 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -258,11 +258,11 @@ test_expect_success 'changed commit with --stat diff option' '\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n> -\t a => b | 0\n> -\t 1 file changed, 0 insertions(+), 0 deletions(-)\n> +\t a => b | 2 +-\n> +\t 1 file changed, 1 insertion(+), 1 deletion(-)\n> \t4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n> -\t a => b | 0\n> -\t 1 file changed, 0 insertions(+), 0 deletions(-)\n> +\t a => b | 2 +-\n> +\t 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThe tests definitely demonstrate that the old behavior was wrong,\nthough...\n\nThanks,\nTaylor\n"},{"id":"405869","messageId":"CALqVohfQZu=itUyfU7nubJpgBETh2q7W1TVx=c2E32ey2cFZkA@mail.gmail.com","threadId":"54257","inReplyTo":"20200918144651.GA1612043@nand.local","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T15:10:45Z","receivedAt":"2020-09-18T15:11:00Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On Fri, 18 Sep 2020 at 10:46, Taylor Blau wrote:\n>\n> On Fri, Sep 18, 2020 at 07:32:55AM -0400, Thomas Guyot-Sionnest wrote:\n> > - same_contents = oideq(&one->oid, &two->oid);\n> > + if (one->is_stdin && two->is_stdin)\n> > + same_contents = !strcmp(one->data, two->data);\n>\n> Hmm. A couple of thoughts here:\n>\n> - strcmp seems like a slow-down here, since we'll have to go through\n> at worst the smaller of one->data and two->data to compare each of\n> them.\n>\n> - strcmp is likely not the right way to do that, since we could be\n> diffing binary content, in which case we'd want to continue over\n> NULs and instead stop at a fixed length (the minimum of the length\n> of one->data and two->data, specifically). I'd have expected memcmp\n> here instead.\n>\n\nYou're absolutely right - this is a bug I managed to figure out last\nnight - first real incursion into git C code - and I definitely didn't\nthink this through. TBH so far I coded mostly with tools dealing in\nplaintext and C strings.\n\n> - Why do we have to do this at all all the way up in\n> 'builtin_diffstat'? I would expect these to contain the right\n> OIDs by the time they are given back to us from the call to\n> 'diff_fill_oid_info' in 'run_diffstat'.\n>\n> So, my last point is the most important of the three. I'd expect\n> something more along the lines of:\n>\n> 1. diff_fill_oid_info resolve the link to the pipe, and\n> 2. index_path handles the resolved fd.\n>\n> ...but it looks like that is already what it's doing? I'm confused why\n> this doesn't work as-is.\n\nSo the idea is to checksum the data and write a valid oid. I'll see if\nI can figure that out. Thanks for the hint though else I would likely\nhave gone with a buffer and memcmp. Your solution seems cleaner, and\nthere is a few other uses of oideq's that look dubious at best with\nthe case of null oids / buffered data so it's definitely a better\napproach.\n\n> > + else\n> > + same_contents = oideq(&one->oid, &two->oid);\n> >\n> > if (diff_filespec_is_binary(o->repo, one) ||\n> > diff_filespec_is_binary(o->repo, two)) {\n> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> > index e024cff65c..4715e75b68 100755\n> > --- a/t/t3206-range-diff.sh\n> > +++ b/t/t3206-range-diff.sh\n> > @@ -258,11 +258,11 @@ test_expect_success 'changed commit with --stat diff option' '\n> > a => b | 0\n> > 1 file changed, 0 insertions(+), 0 deletions(-)\n> > 3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n> > - a => b | 0\n> > - 1 file changed, 0 insertions(+), 0 deletions(-)\n> > + a => b | 2 +-\n> > + 1 file changed, 1 insertion(+), 1 deletion(-)\n> > 4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n> > - a => b | 0\n> > - 1 file changed, 0 insertions(+), 0 deletions(-)\n> > + a => b | 2 +-\n> > + 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> The tests definitely demonstrate that the old behavior was wrong,\n> though...\n>\n\nFor the records I verified the actual diffs (I think they're even\nhardcoded in the earlier tests) and the remaining 0-add/del's are also\nvalid.\n\nRegards,\n\nThomas\n"},{"id":"405879","messageId":"CALqVohfFjsh-2jZLNNwON_V95Dfh-aEh1aMb53t4NQrM0qz1tQ@mail.gmail.com","threadId":"54257","inReplyTo":"20200918143647.GB1606445@nand.local","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T16:34:48Z","receivedAt":"2020-09-18T16:35:03Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi Taylor,\n\nOn Fri, 18 Sep 2020 at 10:36, Taylor Blau wrote:\n> On Fri, Sep 18, 2020 at 07:32:56AM -0400, Thomas Guyot-Sionnest wrote:\n> > A very handy way to pass data to applications is to use the <() process\n> > substitution syntax in bash variants. It allow comparing files streamed\n> > from a remote server or doing on-the-fly stream processing to alter the\n> > diff. These are usually implemented as a symlink that points to a bogus\n> > name (ex \"pipe:[209326419]\") but opens as a pipe.\n>\n> This is true in bash, but sh does not support process substitution with\n> <().\n\nBash, ksh, zsh and likely any more moden shell. Other programming\nlanguages also setup such pipes. It's much cleaner than creating temp\nfiles and cleaning them up and in some cases faster too (I've ran\ndiff's like this over GB's of test data, it's very handy to remove\nknown patterns that would cause needless diffs).\n\n> > +/* Check that file is - (STDIN) or unnamed pipe - explicitly\n> > + * avoid on-disk named pipes which could block\n> > + */\n> > +static int ispipe(const char *name)\n> > +{\n> > + struct stat st;\n> > +\n> > + if (name == file_from_standard_input)\n> > + return 1; /* STDIN */\n> > +\n> > + if (!lstat(name, &st)) {\n> > + if (S_ISLNK(st.st_mode)) {\n>\n> I had to read this a few times to make sure that I got it; you want to\n> stat the link itself, and then check that it links to a pipe.\n>\n> I'm not sure why, though. Do you want to avoid handling named FIFOs in\n> the code below? Your comment that they \"could block\" makes me think you\n> do, but I don't know why that would be a problem.\n\nI'll admit the comment was written first and is a bit naive - i'll\nrephrase that. Yes you don't want to block on pipes like if you run a\n\"grep -R\" on a subtree that has fifos - but as I coded this I realized\nthe obvious: git tracks symlinks name so the real bugger would be to\ndetect one as a pipe and try reading it instead or calling readlink().\n\n> > + /* symlink - read it and check it doesn't exists\n> > + * as a file yet link to a pipe */\n> > + struct strbuf sb = STRBUF_INIT;\n> > + strbuf_realpath(&sb, name, 0);\n> > + /* We're abusing strbuf_realpath here, it may append\n> > + * pipe:[NNNNNNNNN] to an abs path */\n> > + if (!stat(sb.buf, &st))\n>\n> Statting sb.buf is confusing to me (especially when followed up by\n> another stat right below. Could you explain?\n\nThe whole block is under lstat/S_ISLNK (see previous chunk), so the\npath provided to us was a symlink.\n\nInitially I looked at what differentiate these - mainly, stat() st_dev\n- but that struct is os-specific, you'd want to check major(st_dev) ==\n0 (at least on linux) and even if we knew how each os behaves, the\ncode isn't portable and would be a pain to support. Gnu's difftools\nhave very incomplete historical source code in git but there's\nindications they have gotten rid of it too.\n\nSo what I'm doing instead is trying to resolve the link and see if the\ndestination exists (a clear no). Luckily strbuf_realpath does the\nheavy lifting and leaves me with a real path to the file the symlink\npoints to (especially useful for relative links), which is bogus for\nthe special pipes we're interested in.\n\nThen the block right after (not shown) do a stat() on the initial name\nand return whenever it's a fifo or not (if it is, but the link is\nbroken, we know it's a special device).\n\nNow you mention it, maybe I could do that stat first, rule this out\nfrom the beginning... less work for the general case.\n\n*untested*:\n\n if (!lstat(name, &st)) {\n if (!S_ISLNK(st.st_mode))\n return(0);\n if (!stat(name, &st)) {\n if (!S_ISFIFO(st.st_mode))\n return(0);\n\n /* We have a symlink that points to a pipe. If it's resolved\n * target doesn't really exist we can safely assume it's a\n * special file and use it */\n struct strbuf sb = STRBUF_INIT;\n strbuf_realpath(&sb, name, 0);\n /* We're abusing strbuf_realpath here, it may append\n * pipe:[NNNNNNNNN] to an abs path */\n if (stat(sb.buf, &st))\n return(1); /* stat failed, special one */\n }\n }\n return(0);\n\nTL;DR - the conditions we need:\n\n- lstat(name) == 0 // name exists\n- islink(lstat(name)) // name is a symlink\n- stat(name) == 0 // target of name is reachable\n- isfifo(stat(name)) // Target of name is a fifo\n- stat(realpath(readlink(name))) != 0 // Although we can reach it,\nname's destination doesn't actually exist.\n\nBTW is st/sb too confusing ? I took examples elsewhere in the code, I\ncan rename them if it's easier to read.\n\n> > +test_expect_success 'diff --no-index can diff piped subshells' '\n> > + echo 1 >non/git/c &&\n> > + test_expect_code 0 git diff --no-index non/git/b <(cat non/git/c) &&\n> > + test_expect_code 0 git diff --no-index <(cat non/git/b) non/git/c &&\n> > + test_expect_code 0 git diff --no-index <(cat non/git/b) <(cat non/git/c) &&\n> > + test_expect_code 0 cat non/git/b | git diff --no-index - non/git/c &&\n> > + test_expect_code 0 cat non/git/c | git diff --no-index non/git/b - &&\n> > + test_expect_code 0 cat non/git/b | git diff --no-index - <(cat non/git/c) &&\n> > + test_expect_code 0 cat non/git/c | git diff --no-index <(cat non/git/b) -\n> > +'\n>\n> Indeed this test fails (Git thinks that the HERE-DOC is broken, but I\n> suspect it's just getting confused by the '<()'). This test (like almost\n> all other tests in Git) use /bin/sh as its shebang. Does your /bin/sh\n> actually point to bash?\n>\n> If you did want to test something like this, you'd need to source\n> t/lib-bash.sh instead of t/test-lib.sh.\n\nThanks for the tip - indeed I think I ran the testsuite directly with\nback, but the make test failed.\n\n> Unrelated to the above comment, but there are a few small style nits\n> that I notice:\n>\n> - There is no need to run with 'test_expect_code 0' since the test is\n> marked as 'test_expect_success' and the commands are all in an '&&'\n> chain. (This does appear to be common style for others in t4053, so\n> you may just be matching it--which is fine--but an additional\n> clean-up on top to modernize would be appreciated, too).\n>\n> - The cat pipe is unnecessary, and is also violating a rule that we\n> don't place 'git' on the right-hand side of a pipe (can you redirect\n> the file at the end instead?).\n\nCleanup, no pipelines (I read too fast / assumed last command was ok) - will do!\n\n> Documentation/CodingGuidelines is a great place to look if you are ever\n> curious about whether something is in good style.\n>\n> > +test_expect_success 'diff --no-index finds diff in piped subshells' '\n> > + (\n> > + set -- <(cat /dev/null) <(cat /dev/null)\n\nPrecautions/portability. The file names are somewhat dynamic (at least\nthe fd part...) this is to be sure I capture the names of the pipes\nthat will be used (assuming the fd's will be reallocated in the same\norder which I think is fairly safe). An alternative is to sed \"actual\"\nto remove known variables, but then I hope it would be reliable (and\ncan I use sed -r?). IIRC earlier versions of bash or on some systems a\ntemp file could be used for these - although it defeats the purpose\nit's not a reason to fail....\n\nI cannot develop this on other systems but I tested the pipe names on\nWindows and Sunos, and also using ksh and zsh on Linux (zsh uses /proc\ndirectly, kss uses lower fd's which means it can easily clash with\nscripts if you don't use named fd's, but not our problem....)\n\nThanks,\n\nThomas\n"},{"id":"405886","messageId":"20200918171950.GA183026@coredump.intra.peff.net","threadId":"54257","inReplyTo":"CALqVohfFjsh-2jZLNNwON_V95Dfh-aEh1aMb53t4NQrM0qz1tQ@mail.gmail.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T17:19:50Z","receivedAt":"2020-09-18T17:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 12:34:48PM -0400, Thomas Guyot-Sionnest wrote:\n\n> Hi Taylor,\n> \n> On Fri, 18 Sep 2020 at 10:36, Taylor Blau wrote:\n> > On Fri, Sep 18, 2020 at 07:32:56AM -0400, Thomas Guyot-Sionnest wrote:\n> > > A very handy way to pass data to applications is to use the <() process\n> > > substitution syntax in bash variants. It allow comparing files streamed\n> > > from a remote server or doing on-the-fly stream processing to alter the\n> > > diff. These are usually implemented as a symlink that points to a bogus\n> > > name (ex \"pipe:[209326419]\") but opens as a pipe.\n> >\n> > This is true in bash, but sh does not support process substitution with\n> > <().\n> \n> Bash, ksh, zsh and likely any more moden shell. Other programming\n> languages also setup such pipes. It's much cleaner than creating temp\n> files and cleaning them up and in some cases faster too (I've ran\n> diff's like this over GB's of test data, it's very handy to remove\n> known patterns that would cause needless diffs).\n\nYeah, it's definitely a reasonable thing to want (see below). And from a\nportability perspective, it is outside of Git's scope; users with those\nshells can use the feature, and people on other shells don't have to\ncare.\n\nBut we do have to account for this in the test suite, which must be able\nto run under a vanilla POSIX shell. So you'd probably want to set up a\nprerequisite that lets us skip these tests on other shells, like:\n\n test_lazy_prereq PROCESS_SUBSTITUTION '\n\techo foo >expect &&\n\tcat >actual <(echo foo) &&\n\ttest_cmp expect actual\n '\n\n test_expect_success PROCESS_SUBSTITUTION 'some test...' '\n\t# safe because we skip this test on shells that do not support it\n\tgit diff --no-index <(cat whatever)\n '\n\nThough it is a little sad that people running the suite with a vanilla\n/bin/sh like dash wouldn't ever run the tests. I wonder if there's a\nmore portable way to formulate it.\n\nGetting back to the overall feature, this is definitely something that\nhas come up before. The last I know of is:\n\n https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n\nwhich everybody seemed to like the direction of; I suspect the original\nauthor (cc'd) just never got around to it again. Compared to this\napproach, it uses a command-line option to avoid dereferencing symlinks.\nThat puts an extra burden on the caller to pass the option, but it's way\nless magical; you could drop all of the \"does this look like a symlink\nto a pipe\" heuristics. It would also be much easier to test. ;)\n\n-Peff\n"},{"id":"405887","messageId":"20200918172044.GB183026@coredump.intra.peff.net","threadId":"54257","inReplyTo":"20200918143647.GB1606445@nand.local","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T17:20:44Z","receivedAt":"2020-09-18T17:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 10:36:47AM -0400, Taylor Blau wrote:\n\n> - The cat pipe is unnecessary, and is also violating a rule that we\n> don't place 'git' on the right-hand side of a pipe (can you redirect\n> the file at the end instead?).\n\nWhat's wrong with git on the right-hand side of a pipe?\n\nOn the left-hand side we lose its exit code, which is bad. But on the\nright hand side, we are only losing the exit code of \"cat\", which we\ndon't care about.\n\n(Though I agree that \"cat\" is pointless here; we could just be\nredirecting from a file).\n\n-Peff\n"},{"id":"405888","messageId":"20200918172133.GC183026@coredump.intra.peff.net","threadId":"54257","inReplyTo":"20200918171950.GA183026@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T17:21:33Z","receivedAt":"2020-09-18T17:21:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 01:19:50PM -0400, Jeff King wrote:\n\n> Getting back to the overall feature, this is definitely something that\n> has come up before. The last I know of is:\n> \n> https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n> \n> which everybody seemed to like the direction of; I suspect the original\n> author (cc'd) just never got around to it again. Compared to this\n> approach, it uses a command-line option to avoid dereferencing symlinks.\n> That puts an extra burden on the caller to pass the option, but it's way\n> less magical; you could drop all of the \"does this look like a symlink\n> to a pipe\" heuristics. It would also be much easier to test. ;)\n\nOf course I forgot to add the cc. +cc brian.\n"},{"id":"405890","messageId":"20200918172747.GD183026@coredump.intra.peff.net","threadId":"54257","inReplyTo":"20200918113256.8699-2-tguyot@gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T17:27:47Z","receivedAt":"2020-09-18T17:27:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 07:32:55AM -0400, Thomas Guyot-Sionnest wrote:\n\n> In builtin_diffstat(), when both files are coming from \"stdin\" (which\n> could be better described as the file's content being written directly\n> into the file object), oideq() compares two null hashes and ignores the\n> actual differences for the statistics.\n> \n> This patch checks if is_stdin flag is set on both sides and compare\n> contents directly.\n\nI'm somewhat puzzled how we could have two filespecs that came from\nstdin, since we'd generally read to EOF. But looking at the test, it\nseems this is a weird range-diff hack to set is_stdin.\n\nLooking at your patch:\n\n> diff --git a/diff.c b/diff.c\n> index a5114fa864..2995527896 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3681,7 +3681,10 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\treturn;\n> \t}\n> \n> -\tsame_contents = oideq(&one->oid, &two->oid);\n> +\tif (one->is_stdin && two->is_stdin)\n> +\t\tsame_contents = !strcmp(one->data, two->data);\n> +\telse\n> +\t\tsame_contents = oideq(&one->oid, &two->oid);\n\n...should this actually be checking the oid_valid flag in each filespec?\nThat would presumably cover the is_stdin case, too. I also wonder\nwhether range-diff ought to be using that flag instead of is_stdin.\n\n-Peff\n"},{"id":"405891","messageId":"20200918173739.GE183026@coredump.intra.peff.net","threadId":"54257","inReplyTo":"CALqVohfQZu=itUyfU7nubJpgBETh2q7W1TVx=c2E32ey2cFZkA@mail.gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T17:37:39Z","receivedAt":"2020-09-18T17:37:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 11:10:45AM -0400, Thomas Guyot-Sionnest wrote:\n\n> > So, my last point is the most important of the three. I'd expect\n> > something more along the lines of:\n> >\n> > 1. diff_fill_oid_info resolve the link to the pipe, and\n> > 2. index_path handles the resolved fd.\n> >\n> > ...but it looks like that is already what it's doing? I'm confused why\n> > this doesn't work as-is.\n> \n> So the idea is to checksum the data and write a valid oid. I'll see if\n> I can figure that out. Thanks for the hint though else I would likely\n> have gone with a buffer and memcmp. Your solution seems cleaner, and\n> there is a few other uses of oideq's that look dubious at best with\n> the case of null oids / buffered data so it's definitely a better\n> approach.\n\nYou're generally better off not to compute the oid just to compare two\nbuffers:\n\n - a byte-by-byte comparison can quit early as soon as it sees a\n difference, whereas computing two hashes has to cover each byte\n\n - even in the worst case that the byte comparison has to go all the\n way to the end, it's way faster than computing a sha1\n\nSo generally in the diff code we compare oids if we got them for free\n(from the index, or from a tree), but otherwise it's OK to have\nfilespecs without the oid_valid flag set, and compare them byte-wise\nwhen necessary. And there something like:\n\n if (one->size == two->size &&\n !memcmp(one->data, two->data, one->size))\n\nis what you'd want.\n\nNote that filespecs may not have their data or size loaded yet, though.\nLooking at that part of builtin_diffstat(), I'm pretty sure that is\npossible here (see how later code calls diff_populate_filespec() to make\nsure it has data). OTOH, I guess if they're from stdin we'd always have\nthe data (since we'd have no oid to load from), so it might be OK under\nthat conditional.\n\n-Peff\n"},{"id":"405892","messageId":"CALqVoheztgciT1PBGmWu-M-Y_Lt13fhbSyXrMfoqmAotVCthNA@mail.gmail.com","threadId":"54257","inReplyTo":"20200918171950.GA183026@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T17:39:23Z","receivedAt":"2020-09-18T17:39:38Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi Jeff,\n\nOn Fri, 18 Sep 2020 at 13:19, Jeff King wrote:\n> On Fri, Sep 18, 2020 at 12:34:48PM -0400, Thomas Guyot-Sionnest wrote:\n> But we do have to account for this in the test suite, which must be able\n> to run under a vanilla POSIX shell. So you'd probably want to set up a\n> prerequisite that lets us skip these tests on other shells, like:\n\nIndeed, the bash test library that was suggested earlier may be better\nas it exec() bash rather than skipping tests. Testing as a prereq\nworks, another approach which may be harder to swallow but allow\ntesting when default shell is /bin/sh is to run each git command\nthrough bash - could be coupled with a dep if bash isn't installed at\nall.\n\n> Though it is a little sad that people running the suite with a vanilla\n> /bin/sh like dash wouldn't ever run the tests. I wonder if there's a\n> more portable way to formulate it.\n\nDebian defaults to dash, a minimalistic and afaik POSIX-compliant shell.\n\n> Getting back to the overall feature, this is definitely something that\n> has come up before. The last I know of is:\n>\n> https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n>\n> which everybody seemed to like the direction of; I suspect the original\n> author (cc'd) just never got around to it again. Compared to this\n> approach, it uses a command-line option to avoid dereferencing symlinks.\n> That puts an extra burden on the caller to pass the option, but it's way\n> less magical; you could drop all of the \"does this look like a symlink\n> to a pipe\" heuristics. It would also be much easier to test. ;)\n\nThanks for the info. Another consideration is how other commands -\ndiff, vim, sed, curl, etc can take input the same way, so being able\nto swap-in git is a plus imho,and one less switch to learn (or even\nlearn about, I never looked for one for this issue).\n\nRegards,\n\nThomas\n"},{"id":"405894","messageId":"xmqqbli3m0w6.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200918171950.GA183026@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T17:48:41Z","receivedAt":"2020-09-18T17:48:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Getting back to the overall feature, this is definitely something that\n> has come up before. The last I know of is:\n>\n> https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n>\n> which everybody seemed to like the direction of; I suspect the original\n> author (cc'd) just never got around to it again. Compared to this\n> approach, it uses a command-line option to avoid dereferencing symlinks.\n> That puts an extra burden on the caller to pass the option, but it's way\n> less magical; you could drop all of the \"does this look like a symlink\n> to a pipe\" heuristics. It would also be much easier to test. ;)\n\nYes, I do remember liking the approach very much and wanted to take\nit once the \"do not dereference symlinks everywhere---just limit it\nto what was given from the command line\" was done.\n\nTo be quite honest, I think \"git diff --no-index A B\" should\nunconditionally dereference A and/or B if they are symlinks, whether\nthey are symlinks to pipes, regular files or directories, and\notherwise treat symlinks in A and B the same way as \"git diff\" if A\nand B are directories. But that is a design guideline that becomes\nneeded only after we start resurrecting Brian's effort, not with\nthese patches that started this thread.\n\nThanks.\n\n"},{"id":"405895","messageId":"xmqq7dsrm0r8.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200918113256.8699-1-tguyot@gmail.com","subject":"Re: Allow passing pipes to diff --no-index + bugfix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T17:51:39Z","receivedAt":"2020-09-18T17:51:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Guyot-Sionnest writes:\n\n> Along with it a small fix in --stat and --numstat that affected one one\n> git range-diff test, where added/removed lines stts were missing (needed\n> for difffing the pipes too)\n\nNext time, please send each of such unrelated patches independently,\nnot as a two-patch series that gives a (false) impression that the\nsecond one needs the first one to work correctly.\n\nThanks.\n"},{"id":"405896","messageId":"CALqVohcZrBcjmonw-peVxUNM1kgEheCr3nAk9ZvajGpbpXsNaQ@mail.gmail.com","threadId":"54257","inReplyTo":"20200918172747.GD183026@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T17:52:11Z","receivedAt":"2020-09-18T17:52:26Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On Fri, 18 Sep 2020 at 13:27, Jeff King wrote:\n> On Fri, Sep 18, 2020 at 07:32:55AM -0400, Thomas Guyot-Sionnest wrote:\n> > This patch checks if is_stdin flag is set on both sides and compare\n> > contents directly.\n>\n> I'm somewhat puzzled how we could have two filespecs that came from\n> stdin, since we'd generally read to EOF. But looking at the test, it\n> seems this is a weird range-diff hack to set is_stdin.\n\n\"is_stdin\" is actually set manually by a function that copies stdin to\ndiff_filespec->data. We can get an arbitrary number of pipes from\ncommand line arguments - only difference with stdin is that we have to\nopen them before read.\n\nThe flag seems to have been leveraged by diff-range - the first patch\nfixes that tool alone, and 2nd adds support for multiple pipes in\n--no-index. Both are independent but you would not be able to --stat\ntwo pipes without the first patch.\n\n> > diff --git a/diff.c b/diff.c\n> > index a5114fa864..2995527896 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -3681,7 +3681,10 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> > return;\n> > }\n> >\n> > - same_contents = oideq(&one->oid, &two->oid);\n> > + if (one->is_stdin && two->is_stdin)\n> > + same_contents = !strcmp(one->data, two->data);\n> > + else\n> > + same_contents = oideq(&one->oid, &two->oid);\n>\n> ...should this actually be checking the oid_valid flag in each filespec?\n> That would presumably cover the is_stdin case, too. I also wonder\n> whether range-diff ought to be using that flag instead of is_stdin.\n\nI considered that, but IIRC when run under a debugger oid_valid was\nset to 0 - it seemed to be used for something different that i'm not\nfamiliar with, maybe it's an indication the object is in git datastore\n(whereas with --no-index outside files will only be hashed for\ncomparison).\n\nI think is_stdin is a misnomer, but if we want to refactor that i'd\nrather do it after.\n\nRegards,\n\nThomas\n"},{"id":"405897","messageId":"20200918175836.GA149847@nand.local","threadId":"54257","inReplyTo":"CALqVohfFjsh-2jZLNNwON_V95Dfh-aEh1aMb53t4NQrM0qz1tQ@mail.gmail.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-18T17:58:36Z","receivedAt":"2020-09-18T17:58:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Thomas,\n\nOn Fri, Sep 18, 2020 at 12:34:48PM -0400, Thomas Guyot-Sionnest wrote:\n> Hi Taylor,\n>\n> On Fri, 18 Sep 2020 at 10:36, Taylor Blau wrote:\n> > On Fri, Sep 18, 2020 at 07:32:56AM -0400, Thomas Guyot-Sionnest wrote:\n> > > A very handy way to pass data to applications is to use the <() process\n> > > substitution syntax in bash variants. It allow comparing files streamed\n> > > from a remote server or doing on-the-fly stream processing to alter the\n> > > diff. These are usually implemented as a symlink that points to a bogus\n> > > name (ex \"pipe:[209326419]\") but opens as a pipe.\n> >\n> > This is true in bash, but sh does not support process substitution with\n> > <().\n>\n> Bash, ksh, zsh and likely any more moden shell. Other programming\n> languages also setup such pipes. It's much cleaner than creating temp\n> files and cleaning them up and in some cases faster too (I've ran\n> diff's like this over GB's of test data, it's very handy to remove\n> known patterns that would cause needless diffs).\n\nOh, yes, I definitely agree that it will be useful for callers who use\nmore modern shells. I was making sure that we would still be able to run\nthis in the test suite (for us, that means making a new file that\nsources lib-bash and tests only in environments where bash is\nsupported).\n\n> Now you mention it, maybe I could do that stat first, rule this out\n> from the beginning... less work for the general case.\n>\n> *untested*:\n>\n> if (!lstat(name, &st)) {\n> if (!S_ISLNK(st.st_mode))\n> return(0);\n> if (!stat(name, &st)) {\n> if (!S_ISFIFO(st.st_mode))\n> return(0);\n\nI still don't think I quite understand why FIFOs aren't allowed...\n>\n> /* We have a symlink that points to a pipe. If it's resolved\n> * target doesn't really exist we can safely assume it's a\n> * special file and use it */\n> struct strbuf sb = STRBUF_INIT;\n> strbuf_realpath(&sb, name, 0);\n> /* We're abusing strbuf_realpath here, it may append\n> * pipe:[NNNNNNNNN] to an abs path */\n> if (stat(sb.buf, &st))\n> return(1); /* stat failed, special one */\n\nBy the time that I got to this point I think that what you wrote is much\neasier to follow. Thanks.\n\n> }\n> }\n> return(0);\n>\n> TL;DR - the conditions we need:\n>\n> - lstat(name) == 0 // name exists\n> - islink(lstat(name)) // name is a symlink\n> - stat(name) == 0 // target of name is reachable\n> - isfifo(stat(name)) // Target of name is a fifo\n> - stat(realpath(readlink(name))) != 0 // Although we can reach it,\n> name's destination doesn't actually exist.\n>\n> BTW is st/sb too confusing ? I took examples elsewhere in the code, I\n> can rename them if it's easier to read.\n\nNo, it's fine. I think anecdotally I read 'struct strbuf buf' more than\nI read 'struct strbuf sb', but I guess that's just the areas of code\nthat I happen to frequent, since some quick grepping around shows 462\nuses of 'sb' followed by 425 uses of 'buf' (the next most common names\nare 'err' and 'out', with 191 and 121 uses, respectively).\n\n> > > +test_expect_success 'diff --no-index finds diff in piped subshells' '\n> > > + (\n> > > + set -- <(cat /dev/null) <(cat /dev/null)\n>\n> Precautions/portability. The file names are somewhat dynamic (at least\n> the fd part...) this is to be sure I capture the names of the pipes\n> that will be used (assuming the fd's will be reallocated in the same\n> order which I think is fairly safe). An alternative is to sed \"actual\"\n> to remove known variables, but then I hope it would be reliable (and\n> can I use sed -r?). IIRC earlier versions of bash or on some systems a\n> temp file could be used for these - although it defeats the purpose\n> it's not a reason to fail....\n\nOK.\n\n> I cannot develop this on other systems but I tested the pipe names on\n> Windows and Sunos, and also using ksh and zsh on Linux (zsh uses /proc\n> directly, kss uses lower fd's which means it can easily clash with\n> scripts if you don't use named fd's, but not our problem....)\n>\n> Thanks,\n>\n> Thomas\n\nThanks,\nTaylor\n"},{"id":"405898","messageId":"CALqVohefFm-AjVh9-VvUcOO94fEhivZAv=vptQw+tT4E7wsCbw@mail.gmail.com","threadId":"54257","inReplyTo":"20200918173739.GE183026@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T18:00:30Z","receivedAt":"2020-09-18T18:00:46Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On Fri, 18 Sep 2020 at 13:37, Jeff King wrote:\n> On Fri, Sep 18, 2020 at 11:10:45AM -0400, Thomas Guyot-Sionnest wrote:\n>\n> if (one->size == two->size &&\n> !memcmp(one->data, two->data, one->size))\n>\n> is what you'd want.\n>\n\nI think the other approach has its merits too - AFAIK if you run this\nfrom a git repo and one of the files is tracked by it (or even both if\nyou compare two files within the repo) the oid will be readily\navailable and usable if the file hasn't been modified. If there is no\nbig objection I could stick with the hybrid approach, using memcmp of\ncourse. - it's also the easiest fix.\n\n--\nThomas\n"},{"id":"405899","messageId":"20200918180043.GB149847@nand.local","threadId":"54257","inReplyTo":"20200918172044.GB183026@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-18T18:00:43Z","receivedAt":"2020-09-18T18:00:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 18, 2020 at 01:20:44PM -0400, Jeff King wrote:\n> On Fri, Sep 18, 2020 at 10:36:47AM -0400, Taylor Blau wrote:\n>\n> > - The cat pipe is unnecessary, and is also violating a rule that we\n> > don't place 'git' on the right-hand side of a pipe (can you redirect\n> > the file at the end instead?).\n>\n> What's wrong with git on the right-hand side of a pipe?\n\nAck, ignore me. The problem would be on the left-hand side only, without\nset -o pipefail, which we don't do.\n\n> On the left-hand side we lose its exit code, which is bad. But on the\n> right hand side, we are only losing the exit code of \"cat\", which we\n> don't care about.\n>\n> (Though I agree that \"cat\" is pointless here; we could just be\n> redirecting from a file).\n\nYep, but an easy mistake to make nonetheless.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"405900","messageId":"20200918180239.GA186717@coredump.intra.peff.net","threadId":"54257","inReplyTo":"xmqqbli3m0w6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T18:02:39Z","receivedAt":"2020-09-18T18:02:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 10:48:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Getting back to the overall feature, this is definitely something that\n> > has come up before. The last I know of is:\n> >\n> > https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n> >\n> > which everybody seemed to like the direction of; I suspect the original\n> > author (cc'd) just never got around to it again. Compared to this\n> > approach, it uses a command-line option to avoid dereferencing symlinks.\n> > That puts an extra burden on the caller to pass the option, but it's way\n> > less magical; you could drop all of the \"does this look like a symlink\n> > to a pipe\" heuristics. It would also be much easier to test. ;)\n> \n> Yes, I do remember liking the approach very much and wanted to take\n> it once the \"do not dereference symlinks everywhere---just limit it\n> to what was given from the command line\" was done.\n> \n> To be quite honest, I think \"git diff --no-index A B\" should\n> unconditionally dereference A and/or B if they are symlinks, whether\n> they are symlinks to pipes, regular files or directories, and\n> otherwise treat symlinks in A and B the same way as \"git diff\" if A\n> and B are directories. But that is a design guideline that becomes\n> needed only after we start resurrecting Brian's effort, not with\n> these patches that started this thread.\n\nYeah, I think I'd be fine with that approach, too. It makes \"git diff\n--no-index\" more like other tools out of the box. And if we took brian's\npatch first, then we'd just be flipping its default, and the option it\nadds would give an easy escape hatch for somebody who really wants to\ndiff two maybe-symlinks.\n\n-Peff\n"},{"id":"405901","messageId":"20200918180508.GB186717@coredump.intra.peff.net","threadId":"54257","inReplyTo":"20200918175836.GA149847@nand.local","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T18:05:08Z","receivedAt":"2020-09-18T18:05:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 18, 2020 at 01:58:36PM -0400, Taylor Blau wrote:\n\n> > *untested*:\n> >\n> > if (!lstat(name, &st)) {\n> > if (!S_ISLNK(st.st_mode))\n> > return(0);\n> > if (!stat(name, &st)) {\n> > if (!S_ISFIFO(st.st_mode))\n> > return(0);\n> \n> I still don't think I quite understand why FIFOs aren't allowed...\n\nIt's the other way around. Non-fifos return \"0\" from this \"is it a pipe\"\nfunction.\n\nI think it is to prevent a false positive with:\n\n rm bar\n ln -s foo bar\n git diff --no-index foo something-else\n\nWe'd still want to treat \"foo\" as a symlink there. I.e., the heuristic\nfor guessing it's a process substitution pipe is:\n\n - it's a symlink that doesn't point anywhere\n - it's also a fifo\n\n-Peff\n"},{"id":"405902","messageId":"xmqq363fm02a.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"CALqVohcZrBcjmonw-peVxUNM1kgEheCr3nAk9ZvajGpbpXsNaQ@mail.gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T18:06:37Z","receivedAt":"2020-09-18T18:06:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Guyot-Sionnest writes:\n\n>> > - same_contents = oideq(&one->oid, &two->oid);\n>> > + if (one->is_stdin && two->is_stdin)\n>> > + same_contents = !strcmp(one->data, two->data);\n>> > + else\n>> > + same_contents = oideq(&one->oid, &two->oid);\n>>\n>> ...should this actually be checking the oid_valid flag in each filespec?\n>> That would presumably cover the is_stdin case, too. I also wonder\n>> whether range-diff ought to be using that flag instead of is_stdin.\n>\n> I considered that, but IIRC when run under a debugger oid_valid was\n> set to 0 - it seemed to be used for something different that i'm not\n> familiar with, maybe it's an indication the object is in git datastore\n> (whereas with --no-index outside files will only be hashed for\n> comparison).\n\nIf it says !oid_valid, I think you are getting what you do want.\nThe contents from the outside world, be it what was read from the\nstandard input or a pipe, a regular file that is not up-to-date with\nthe index, may not have a usable oid computed for it, and oid_valid\nbeing false signals you that you need byte-for-byte comparison. As\nsuggested by Peff in another message, you can take that signal and\ncompare the size and then the contents with memcmp() to see if they\nare the same.\n\n"},{"id":"405907","messageId":"CALqVohdQwPppBsdsJjUhpXGZsZ=XCY_he7oFj1He1T8PjRLULw@mail.gmail.com","threadId":"54257","inReplyTo":"xmqq7dsrm0r8.fsf@gitster.c.googlers.com","subject":"Re: Allow passing pipes to diff --no-index + bugfix","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-18T18:24:50Z","receivedAt":"2020-09-18T18:25:20Z","isPatch":false,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On Fri, 18 Sep 2020 at 13:51, Junio C Hamano wrote:\n>\n> Next time, please send each of such unrelated patches independently,\n> not as a two-patch series that gives a (false) impression that the\n> second one needs the first one to work correctly.\n\nHi Junio,\n\nMy apologies for not making it clear enough - the fix in the first\npatch is for an edgy case of git diff-range, but the same fix applies\nto the 2nd patch. Any diff --stat comparison of two pipes would return\n0-line diffs although the files would be marked as changed.\n\nIt was originally just one commit; I splitted it up when I realized a\ntest was actually triggered by this fix (false negative without the\nfix, false positive with it) as it's still independent enough to be\nreviewed/merged alone.\n\nRegards,\n\nThomas\n"},{"id":"405915","messageId":"20200918215623.GE67496@camp.crustytoothpaste.net","threadId":"54257","inReplyTo":"20200918113256.8699-3-tguyot@gmail.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-18T21:56:23Z","receivedAt":"2020-09-18T21:57:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-18 at 11:32:56, Thomas Guyot-Sionnest wrote:\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 7814eabfe0..779c686d23 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -41,6 +41,33 @@ static int read_directory_contents(const char *path, struct string_list *list)\n> */\n> static const char file_from_standard_input[] = \"-\";\n> \n> +/* Check that file is - (STDIN) or unnamed pipe - explicitly\n> + * avoid on-disk named pipes which could block\n> + */\n> +static int ispipe(const char *name)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (name == file_from_standard_input)\n> +\t\treturn 1; /* STDIN */\n> +\n> +\tif (!lstat(name, &st)) {\n> +\t\tif (S_ISLNK(st.st_mode)) {\n> +\t\t\t/* symlink - read it and check it doesn't exists\n> +\t\t\t * as a file yet link to a pipe */\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_realpath(&sb, name, 0);\n> +\t\t\t/* We're abusing strbuf_realpath here, it may append\n> +\t\t\t * pipe:[NNNNNNNNN] to an abs path */\n> +\t\t\tif (!stat(sb.buf, &st))\n> +\t\t\t\treturn 0; /* link target exists , not pipe */\n> +\t\t\tif (!stat(name, &st))\n> +\t\t\t\treturn S_ISFIFO(st.st_mode);\n\nThis makes a lot of assumptions about the implementation which are\nspecific to Linux, namely that an anonymous pipe will be a symlink to a\nFIFO. That's not the case on macOS, where the /dev/fd entries are\nactually named pipes themselves.\n\nGranted, in that case, Git just chokes and fails instead of diffing the\nsymlink values, but I suspect you'll want this to work on macOS as well.\nI don't use macOS that often, but I would appreciate it if it worked\nwhen I did, and I'm sure others will as well.\n\nI think we can probably get away with just doing a regular stat and\nseeing if S_ISFIFO is true, which is both simpler and cheaper.\n\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 0168946b63..e49f773515 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -144,4 +144,193 @@ test_expect_success 'diff --no-index allows external diff' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'diff --no-index can diff piped subshells' '\n> +\techo 1 >non/git/c &&\n> +\ttest_expect_code 0 git diff --no-index non/git/b <(cat non/git/c) &&\n> +\ttest_expect_code 0 git diff --no-index <(cat non/git/b) non/git/c &&\n> +\ttest_expect_code 0 git diff --no-index <(cat non/git/b) <(cat non/git/c) &&\n> +\ttest_expect_code 0 cat non/git/b | git diff --no-index - non/git/c &&\n> +\ttest_expect_code 0 cat non/git/c | git diff --no-index non/git/b - &&\n> +\ttest_expect_code 0 cat non/git/b | git diff --no-index - <(cat non/git/c) &&\n> +\ttest_expect_code 0 cat non/git/c | git diff --no-index <(cat non/git/b) -\n> +'\n\nAs mentioned by others, this requires non-POSIX syntax. /bin/sh on my\nDebian system is dash, which doesn't support this. You can either use a\nprerequisite, or just test by piping from standard input and assume that\nGit can handle the rest. I would recommend at least adding some POSIX\ntestcases that use only a pipe from standard input to avoid regressing\nthis behavior on Debian and Ubuntu.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"405965","messageId":"f4c4cb48-f4b5-3d4d-066d-b94e961dcbb5@gmail.com","threadId":"54257","inReplyTo":"CALqVohfQZu=itUyfU7nubJpgBETh2q7W1TVx=c2E32ey2cFZkA@mail.gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-20T04:53:15Z","receivedAt":"2020-09-20T05:01:56Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi... Added Jeff as he got involved later and comments below are\nrelevant to his questions.\n\nOn 2020-09-18 11:10, Thomas Guyot-Sionnest wrote:\n> On Fri, 18 Sep 2020 at 10:46, Taylor Blau wrote:\n>>\n>> - Why do we have to do this at all all the way up in\n>> 'builtin_diffstat'? I would expect these to contain the right\n>> OIDs by the time they are given back to us from the call to\n>> 'diff_fill_oid_info' in 'run_diffstat'.\n>>\n>> So, my last point is the most important of the three. I'd expect\n>> something more along the lines of:\n>>\n>> 1. diff_fill_oid_info resolve the link to the pipe, and\n>> 2. index_path handles the resolved fd.\n>>\n>> ...but it looks like that is already what it's doing? I'm confused why\n>> this doesn't work as-is.\n> \n> So the idea is to checksum the data and write a valid oid. I'll see if\n> I can figure that out. Thanks for the hint though else I would likely\n> have gone with a buffer and memcmp. Your solution seems cleaner, and\n> there is a few other uses of oideq's that look dubious at best with\n> the case of null oids / buffered data so it's definitely a better\n> approach.\n> \n\nAfter looking further at the code I understand your point, although\npipes can only ever be read once, so even if we do that we would have to\nbuffer on first read. It appears the files are first read by\ndiff_populate_filespec() - builtin_diffstat isn't even called if the\nfiles match (even for two pipes).\n\nJeff, on your suggestion to compare size, the size is set even if data\nis null. Files in-tree appears to be mmapped on demand for reads.\n\ndiff_fill_oid_info explicitly resets oids for is_stdin and return, and\nif the file's been read already and it's a pipe, we would *have* to have\nbuffered the data already so I don't really see what else we can do\nbesides memcmp() (technically we should be able to tell if the files\nhave been modified at this point but apparently that information isn't\ntransmitted to builtin_diffstat - it's assumed and I won't make complex\nchange for that odd case of diffing two pipes. That's what I have now:\n\n /* What is_stdin really means is that the file's content is only\n * in the filespec's buffer and its oid is zero. We can't compare\n * oid's if both are null and we can just diff the buffers */\n if (one->is_stdin && two->is_stdin)\n same_contents = (one->size == two->size ?\n !memcmp(one->data, two->data, one->size) : 0);\n else\n same_contents = oideq(&one->oid, &two->oid);\n\n\nEven when we implement the --literally switch, considering we can't\nguarantee a single read per file, for now I'd keep using the is_stdin\nflag as an indication of in-memory data, and we'll have to read in all\npipes we diff (like earlier patch). It could be a concern if we\n--literally diff a whole subtree of large pipes. In that case the only\nfix I can see is to reorder the operations to generate the stats on each\nfile diff (or at least keep the diffs around for the stats pass).\n\n\nRegards,\n\nThomas\n\n\n"},{"id":"405990","messageId":"f338b63f-fd89-095c-b036-8d548fd2470c@gmail.com","threadId":"54257","inReplyTo":"20200918180239.GA186717@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-20T12:54:53Z","receivedAt":"2020-09-20T13:07:26Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On 2020-09-18 14:02, Jeff King wrote:\n> On Fri, Sep 18, 2020 at 10:48:41AM -0700, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> Getting back to the overall feature, this is definitely something that\n>>> has come up before. The last I know of is:\n>>>\n>>> https://lore.kernel.org/git/20181220002610.43832-1-sandals@crustytoothpaste.net/\n>>>\n>>> which everybody seemed to like the direction of; I suspect the original\n>>> author (cc'd) just never got around to it again. Compared to this\n>>> approach, it uses a command-line option to avoid dereferencing symlinks.\n>>> That puts an extra burden on the caller to pass the option, but it's way\n>>> less magical; you could drop all of the \"does this look like a symlink\n>>> to a pipe\" heuristics. It would also be much easier to test. ;)\n>>\n>> Yes, I do remember liking the approach very much and wanted to take\n>> it once the \"do not dereference symlinks everywhere---just limit it\n>> to what was given from the command line\" was done.\n>>\n>> To be quite honest, I think \"git diff --no-index A B\" should\n>> unconditionally dereference A and/or B if they are symlinks, whether\n>> they are symlinks to pipes, regular files or directories, and\n>> otherwise treat symlinks in A and B the same way as \"git diff\" if A\n>> and B are directories. But that is a design guideline that becomes\n>> needed only after we start resurrecting Brian's effort, not with\n>> these patches that started this thread.\n> \n> Yeah, I think I'd be fine with that approach, too. It makes \"git diff\n> --no-index\" more like other tools out of the box. And if we took brian's\n> patch first, then we'd just be flipping its default, and the option it\n> adds would give an easy escape hatch for somebody who really wants to\n> diff two maybe-symlinks.\n\nConsidering the issue with MacOS I'm starting to think the best solution\nis to not use any heuristic and read passed-in files directly. That\nsaid, I don't think it makes much change either way (if I resurrect\nBrian's patch is will probably end up being a hybrid between the two as\nboth read the pipe at the same place and my approach was simpler further\ndown).\n\nI'm not sure which way I prefer to start first - will you accept a patch\nthat reads passed in files as-is if I I start with this one?\n\nIn the mean time I will submit the first patch fixed) as a standalone one.\n\nRegards,\n\nThomas\n\n"},{"id":"405991","messageId":"20200920130945.26399-1-tguyot@gmail.com","threadId":"54257","inReplyTo":"20200918113256.8699-2-tguyot@gmail.com","subject":"[PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-20T13:09:46Z","receivedAt":"2020-09-20T13:10:47Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"In builtin_diffstat(), when both files are coming from \"stdin\" (which\ncould be better described as the file's content being written directly\ninto the file object), oideq() compares two null hashes and ignores the\nactual differences for the statistics.\n\nThis patch checks if is_stdin flag is set on both sides and compare\ncontents directly.\n\nSigned-off-by: Thomas Guyot-Sionnest \n---\nRange-diff:\n1: 479c2835fc ! 1: 1f25713d44 diff: Fix modified lines stats with --stat and --numstat\n @@ -20,8 +20,12 @@\n \t}\n \n -\tsame_contents = oideq(&one->oid, &two->oid);\n ++\t/* What is_stdin really means is that the file's content is only\n ++\t * in the filespec's buffer and its oid is zero. We can't compare\n ++\t * oid's if both are null and we can just diff the buffers */\n +\tif (one->is_stdin && two->is_stdin)\n -+\t\tsame_contents = !strcmp(one->data, two->data);\n ++\t\tsame_contents = (one->size == two->size ?\n ++\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n +\telse\n +\t\tsame_contents = oideq(&one->oid, &two->oid);\n \n\n diff.c | 9 ++++++++-\n t/t3206-range-diff.sh | 8 ++++----\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ee8e8189e9..2e47bf824e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3681,7 +3681,14 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n-\tsame_contents = oideq(&one->oid, &two->oid);\n+\t/* What is_stdin really means is that the file's content is only\n+\t * in the filespec's buffer and its oid is zero. We can't compare\n+\t * oid's if both are null and we can just diff the buffers */\n+\tif (one->is_stdin && two->is_stdin)\n+\t\tsame_contents = (one->size == two->size ?\n+\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n+\telse\n+\t\tsame_contents = oideq(&one->oid, &two->oid);\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e024cff65c..4715e75b68 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -258,11 +258,11 @@ test_expect_success 'changed commit with --stat diff option' '\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \t4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \tEOF\n \ttest_cmp expect actual\n '\n-- \n2.20.1\n\n"},{"id":"405993","messageId":"20200920153915.GB2726066@nand.local","threadId":"54257","inReplyTo":"20200920130945.26399-1-tguyot@gmail.com","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-20T15:39:15Z","receivedAt":"2020-09-20T15:39:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 20, 2020 at 09:09:46AM -0400, Thomas Guyot-Sionnest wrote:\n> In builtin_diffstat(), when both files are coming from \"stdin\" (which\n> could be better described as the file's content being written directly\n> into the file object), oideq() compares two null hashes and ignores the\n> actual differences for the statistics.\n>\n> This patch checks if is_stdin flag is set on both sides and compare\n> contents directly.\n>\n> Signed-off-by: Thomas Guyot-Sionnest \n> ---\n> Range-diff:\n> 1: 479c2835fc ! 1: 1f25713d44 diff: Fix modified lines stats with --stat and --numstat\n> @@ -20,8 +20,12 @@\n> \t}\n>\n> -\tsame_contents = oideq(&one->oid, &two->oid);\n> ++\t/* What is_stdin really means is that the file's content is only\n> ++\t * in the filespec's buffer and its oid is zero. We can't compare\n> ++\t * oid's if both are null and we can just diff the buffers */\n> +\tif (one->is_stdin && two->is_stdin)\n> -+\t\tsame_contents = !strcmp(one->data, two->data);\n> ++\t\tsame_contents = (one->size == two->size ?\n> ++\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n> +\telse\n> +\t\tsame_contents = oideq(&one->oid, &two->oid);\n\nAfter reading your explanation in [1], this version makes more sense to\nme.\n\nThanks.\n\n[1]: https://lore.kernel.org/git/f4c4cb48-f4b5-3d4d-066d-b94e961dcbb5@gmail.com/\n\nTaylor\n"},{"id":"405997","messageId":"a126bcf6-13f7-48f0-95d9-d934d042d7fd@gmail.com","threadId":"54257","inReplyTo":"20200920153915.GB2726066@nand.local","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-20T16:38:20Z","receivedAt":"2020-09-20T16:38:25Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"On 2020-09-20 11:39, Taylor Blau wrote:\n> On Sun, Sep 20, 2020 at 09:09:46AM -0400, Thomas Guyot-Sionnest wrote:\n>> In builtin_diffstat(), when both files are coming from \"stdin\" (which\n>> could be better described as the file's content being written directly\n>> into the file object), oideq() compares two null hashes and ignores the\n>> actual differences for the statistics.\n>>\n>> This patch checks if is_stdin flag is set on both sides and compare\n>> contents directly.\n>>\n>> Signed-off-by: Thomas Guyot-Sionnest \n>> ---\n>> Range-diff:\n>> 1: 479c2835fc ! 1: 1f25713d44 diff: Fix modified lines stats with --stat and --numstat\n>> @@ -20,8 +20,12 @@\n>> \t}\n>>\n>> -\tsame_contents = oideq(&one->oid, &two->oid);\n>> ++\t/* What is_stdin really means is that the file's content is only\n>> ++\t * in the filespec's buffer and its oid is zero. We can't compare\n>> ++\t * oid's if both are null and we can just diff the buffers */\n>> +\tif (one->is_stdin && two->is_stdin)\n>> -+\t\tsame_contents = !strcmp(one->data, two->data);\n>> ++\t\tsame_contents = (one->size == two->size ?\n>> ++\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n>> +\telse\n>> +\t\tsame_contents = oideq(&one->oid, &two->oid);\n> \n> After reading your explanation in [1], this version makes more sense to\n> me.\n> \n> Thanks.\n> \n> [1]: https://lore.kernel.org/git/f4c4cb48-f4b5-3d4d-066d-b94e961dcbb5@gmail.com/\n\nThere's a little bit missing... Just before the new code example:,\nprevious to last paragraph:\n\n> it's assumed [we can just call oidcmp()] and I won't make complex\n\n--\nThomas\n"},{"id":"406000","messageId":"xmqqlfh4gt5z.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200920153915.GB2726066@nand.local","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-20T19:11:20Z","receivedAt":"2020-09-20T19:11:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Sun, Sep 20, 2020 at 09:09:46AM -0400, Thomas Guyot-Sionnest wrote:\n>> In builtin_diffstat(), when both files are coming from \"stdin\" (which\n>> could be better described as the file's content being written directly\n>> into the file object), oideq() compares two null hashes and ignores the\n>> actual differences for the statistics.\n>>\n>> This patch checks if is_stdin flag is set on both sides and compare\n>> contents directly.\n>>\n>> Signed-off-by: Thomas Guyot-Sionnest \n>> ---\n>> Range-diff:\n>> 1: 479c2835fc ! 1: 1f25713d44 diff: Fix modified lines stats with --stat and --numstat\n>> @@ -20,8 +20,12 @@\n>> \t}\n>>\n>> -\tsame_contents = oideq(&one->oid, &two->oid);\n>> ++\t/* What is_stdin really means is that the file's content is only\n>> ++\t * in the filespec's buffer and its oid is zero. We can't compare\n>> ++\t * oid's if both are null and we can just diff the buffers */\n>> +\tif (one->is_stdin && two->is_stdin)\n>> -+\t\tsame_contents = !strcmp(one->data, two->data);\n>> ++\t\tsame_contents = (one->size == two->size ?\n>> ++\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n>> +\telse\n>> +\t\tsame_contents = oideq(&one->oid, &two->oid);\n>\n> After reading your explanation in [1], this version makes more sense to\n> me.\n\nThese oid fields are prepared by calling diff_fill_oid_info(), and\neven for paths that are dirty (hence no \"free\" oid available from\nindex or tree entry), an appropriate oid is computed.\n\nBut there are paths for which oid cannot be computed without\ndestroying their contents. Such paths are marked by the function\nwith null_oid.\n\nIt happens to be that stdin is the only class of paths that are\ntreated that way _right_ _now_, but future code may support\ndifferent kind of paths that share the same trait.\n\nWhen we want to know \"is comparing the oid sufficient?\", we\nshouldn't inspect the is_stdin flag ourselves in a caller of\ndiff_fill_oid_info(), because the helper _is_ responsible for\nknowing what kind of paths are special, and signals that \"assume\nthis would not be equal to anything else\" by giving null_oid back.\n\nThe caller should use the info left by diff_fill_oid_info(), namely,\n\"even if the oid on both sides are the same, if it is null_oid, then\nwe know diff_fill_oid_info() didn't actually compute the oid, and we\nneed to compare the blob ourselves\".\n\nAnd there is no point in doing memcmp() here, I think. \n\nThe same_contents() check is done as an optimization to avoid xdl.\nEven if the two sides were thought to be different at the oid level,\nxdl comparison may find that there is no difference after all\n(e.g. think of whitespace ignoring comparison), so we should assume\nand rely on that the downstream code MUST BE prepared to handle\nfalse negatives (i.e. same_contents says they are different, but\nthey actually produce no diffstat). Running memcmp() over contents\nin potentially a large buffer to find that they are different, and\nthen have xdl process that large buffer again, would be a waste.\n\nSummarizing the above, I think the second best fix is this (which\nmeans that the posted patch is the third best):\n\n\t/*\n\t * diff_fill_oid_info() marked one/two->oid with null_oid\n\t * for a path whose oid is not available. Disable early\n\t * return optimization for them.\n\t */\n\tif (oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n\t\tsame_contents = 0; /* could be different */\n\telse if (oideq(&one->oid, &two->oid))\n\t\tsame_contents = 1; /* definitely the same */\n\telse\n\t\tsame_contents = 0; /* definitely different */\n\nBut I suspect that the best fix is to teach diff_fill_oid_info() to\nhash the in-memory data to compute the oid, instead of punting and\nfilling the oid field with null_oid. If function builtin_diffstat()\nis allowed to look at the contents and run memcmp() here, the 'data'\nfield should have been filled and valid when diff_fill_oid_info()\nlooked at it already.\n\nThe \"best\" fix will have wider consequences, so we may not want to\njump to it right away without careful consideration.\n\nFor example, the \"best\" fix will fix another bug. The 'index'\nheader shows a NULL object name in normal \"diff --patch\" output for\nthese paths in the current code, which means they cannot be used\nwith \"apply --3way\". That way, this codepath does not have to know\nanything about the \"null means we don't know\" convention. \n\nTry:\n\n $ (cat COPYING; echo) >RENAMING\n $ git diff --no-index COPYING - writes:\n\n> But I suspect that the best fix is to teach diff_fill_oid_info() to\n> hash the in-memory data to compute the oid, instead of punting and\n> filling the oid field with null_oid. If function builtin_diffstat()\n> is allowed to look at the contents and run memcmp() here, the 'data'\n> field should have been filled and valid when diff_fill_oid_info()\n> looked at it already.\n>\n> The \"best\" fix will have wider consequences, so we may not want to\n> jump to it right away without careful consideration.\n\nAnd then after giving a bit more thought, I don't recommend to go\nwith this approach, because it breaks an established convention that\nobjects with unknown name is perfectly OK and shown with the null\noid.\n\nIn other words, I'd suggest to use the \"second best\" one I gave in\nthe message I am responding to.\n\nThanks.\n\n"},{"id":"406002","messageId":"xmqqd02ggp84.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"xmqqlfh4gt5z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-20T20:36:27Z","receivedAt":"2020-09-20T20:36:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Summarizing the above, I think the second best fix is this (which\n> means that the posted patch is the third best):\n>\n> \t/*\n> \t * diff_fill_oid_info() marked one/two->oid with null_oid\n> \t * for a path whose oid is not available. Disable early\n> \t * return optimization for them.\n> \t */\n> \tif (oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n> \t\tsame_contents = 0; /* could be different */\n> \telse if (oideq(&one->oid, &two->oid))\n> \t\tsame_contents = 1; /* definitely the same */\n> \telse\n> \t\tsame_contents = 0; /* definitely different */\n\nA tangent.\n\nThere is this code in diff.c::fill_metainfo() that is used to\npopulate the \"index\" header element of \"diff --patch\" output:\n\n\tif (one && two && !oideq(&one->oid, &two->oid)) {\n\t\tconst unsigned hexsz = the_hash_algo->hexsz;\n\t\tint abbrev = o->abbrev ? o->abbrev : DEFAULT_ABBREV;\n\n\t\tif (o->flags.full_index)\n\t\t\tabbrev = hexsz;\n\n\t\tif (o->flags.binary) {\n\t\t\tmmfile_t mf;\n\t\t\tif ((!fill_mmfile(o->repo, &mf, one) &&\n\t\t\t diff_filespec_is_binary(o->repo, one)) ||\n\t\t\t (!fill_mmfile(o->repo, &mf, two) &&\n\t\t\t diff_filespec_is_binary(o->repo, two)))\n\t\t\t\tabbrev = hexsz;\n\t\t}\n\t\tstrbuf_addf(msg, \"%s%sindex %s..%s\", line_prefix, set,\n\t\t\t diff_abbrev_oid(&one->oid, abbrev),\n\t\t\t diff_abbrev_oid(&two->oid, abbrev));\n\t\tif (one->mode == two->mode)\n\t\t\tstrbuf_addf(msg, \" %06o\", one->mode);\n\t\tstrbuf_addf(msg, \"%s\\n\", reset);\n\t}\n\nCurrently it is OK because there can only be one side that\ndiff_fill_oid_info() would mark as \"oid unavailable\" (e.g. reading\nstandard input stream). If a new feature is introducing a situation\nwhere both ends have null_oid, which was so far been impossible,\nwe'd probably need to factor out the condition used in the above\ninto a helper function, e.g.\n\n static int cannot_be_the_same(struct diff_filespec *one, struct diff_filespec *two)\n {\n\tif ((oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n\t\treturn 1;\n\telse if (oideq(&one->oid, &two->oid))\n\t\treturn 0;\n\telse\n\t\treturn 1;\n }\n\nand rewrite the conditional in fill_metainfo() to\n\n\tif (one && two && cannot_be_the_same(one, two)) {\n\t\t...\n\nThe \"second best fix\" could then become a single liner:\n\n\tsame_contents = !cannot_be_the_same(one, two);\n\nusing the helper.\n\n"},{"id":"406004","messageId":"xmqq8sd4gkn3.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"xmqqd02ggp84.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-20T22:15:28Z","receivedAt":"2020-09-20T22:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\nSorry, this will be the last message from me on this topic for now.\n\n> we'd probably need to factor out the condition used in the above\n> into a helper function, e.g.\n>\n> static int cannot_be_the_same(struct diff_filespec *one, struct diff_filespec *two)\n\nThe naming of this helper is tricky. In both potential callers,\nwhat we want to see is \"one and two may be different, we cannot say\nthey are the same with certainty\", so \"cannot be the same\" is a\nmisnomer. Worse, the negated form is hard to grok.\n\nPerhaps \"may_differ()\" is a more correct name. If either side is\nNULL oid, we cannot say they are the same, so it is true. If two\noid that are not NULL oid are the same, there is no possibility that\nthey are different, so we return false. And two oid that are not\nNULL oid are different, we know they are different, so we return\ntrue.\n\n> {\n> \tif ((oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n> \t\treturn 1;\n> \telse if (oideq(&one->oid, &two->oid))\n> \t\treturn 0;\n> \telse\n> \t\treturn 1;\n> }\n>\n> and rewrite the conditional in fill_metainfo() to\n>\n> \tif (one && two && cannot_be_the_same(one, two)) {\n> \t\t...\n\nAnd this becomes much easier to read and understand, i.e.\n\n\tif (one && two && may_differ(one, two)) {\n\t\t... create the index one->oid..two->oid header ...\n\n> The \"second best fix\" could then become a single liner:\n>\n> \tsame_contents = !cannot_be_the_same(one, two);\n>\n> using the helper.\n\nAnd this becomes\n\n\tsame_contents = !may_differ(one, two);\n\nmeaning that \"there is no possibility that one and two are\ndifferent\". That allows us to optimize out the invocation of the\nxdl machinery.\n\n"},{"id":"406065","messageId":"20200921192630.GA2399334@coredump.intra.peff.net","threadId":"54257","inReplyTo":"xmqqlfh4gt5z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-21T19:26:30Z","receivedAt":"2020-09-21T19:26:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 20, 2020 at 12:11:20PM -0700, Junio C Hamano wrote:\n\n> > After reading your explanation in [1], this version makes more sense to\n> > me.\n> \n> These oid fields are prepared by calling diff_fill_oid_info(), and\n> even for paths that are dirty (hence no \"free\" oid available from\n> index or tree entry), an appropriate oid is computed.\n\nThis is the part that confused me earlier. I expected these \"stdin\"\nentries, just like other some other entries (e.g., stat dirty ones for\ndiff-files, or anything for \"diff --no-index\") to have bogus oids.\n\nBut that diff_fill_oid_info() is what actually computes the sha1 from\nscratch for them. I get why that is needed for generating a git diff, as\nwe have an \"index from...to\" line there that we'd want to fill.\n\nFor diffstat, though, it seems like a waste of time; we don't care what\nthe object hash is. I.e., if we were to do this:\n\ndiff --git a/diff.c b/diff.c\nindex 16eeaf6645..1934af29a5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4564,9 +4564,6 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n \tif (o->prefix_length)\n \t\tstrip_prefix(o->prefix_length, &name, &other);\n \n-\tdiff_fill_oid_info(p->one, o->repo->index);\n-\tdiff_fill_oid_info(p->two, o->repo->index);\n-\n \tbuiltin_diffstat(name, other, p->one, p->two,\n \t\t\t diffstat, o, p);\n }\n\nthen everything seems to work fine _except_ a \"git diff --stat\n--no-index\", exactly because it hits this \"same_contents\" check we've\nbeen discussing. And once that is fixed properly (to handle any case\nwhere we have no oid, not just when the stdin flag is set), then perhaps\nit is worth doing.\n\nOr perhaps not. Even if we have to memcmp sometimes in\nbuiltin_diffstat(), it would be faster than computing the individual\nhashes. But it may not be measurably so, and it would be no difference\nfor the common case of filespecs for which we do know the oid for free.\nI also suspect we'd need to be a little smarter about combined formats\n(e.g., \"--stat --patch\" might as well compute the oid as early as\npossible, since we'll need it eventually for the patch; but we'd hit the\ncall in builtin_diffstat() before the one in run_diff()).\n\n> But there are paths for which oid cannot be computed without\n> destroying their contents. Such paths are marked by the function\n> with null_oid.\n\nI'm not clear how computing the oid destroys the contents. We have them\nin an in-memory buffer at this point, don't we? So we _could_ generate\nan oid even for stdin, like this:\n\ndiff --git a/cache.h b/cache.h\nindex 55d7f61087..1ace143eac 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -858,6 +858,7 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *,\n #define HASH_RENORMALIZE 4\n int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags);\n int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags);\n+int index_mem(struct index_state *istate, struct object_id *oid, void *buf, size_t size, enum object_type type, const char *path, unsigned flags);\n \n /*\n * Record to sd the data from st that we use to check whether a file\ndiff --git a/diff.c b/diff.c\nindex 16eeaf6645..181b632114 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4463,7 +4463,10 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is\n \t\tif (!one->oid_valid) {\n \t\t\tstruct stat st;\n \t\t\tif (one->is_stdin) {\n-\t\t\t\toidclr(&one->oid);\n+\t\t\t\tif (index_mem(istate, &one->oid,\n+\t\t\t\t\t one->data, one->size,\n+\t\t\t\t\t OBJ_BLOB, one->path, 0))\n+\t\t\t\t\tdie(\"cannot hash diff file from stdin\");\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tif (lstat(one->path, &st) < 0)\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 770501d6d1..c7d017b3e0 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -2046,10 +2046,10 @@ static void check_tag(const void *buf, size_t size)\n \t\tdie(_(\"corrupt tag\"));\n }\n \n-static int index_mem(struct index_state *istate,\n-\t\t struct object_id *oid, void *buf, size_t size,\n-\t\t enum object_type type,\n-\t\t const char *path, unsigned flags)\n+int index_mem(struct index_state *istate,\n+\t struct object_id *oid, void *buf, size_t size,\n+\t enum object_type type,\n+\t const char *path, unsigned flags)\n {\n \tint ret, re_allocated = 0;\n \tint write_object = flags & HASH_WRITE_OBJECT;\n\nwhich is basically your \"best fix\" from below. It fixes the bug here,\nand it gives you a non-null index line. I'd consider coupling it with\ncalling fill_oid less often, though (something like range-diff computes\na bunch of fake-stdin diffs, and doesn't need to waste time computing\nthe oids at all).\n\n> Summarizing the above, I think the second best fix is this (which\n> means that the posted patch is the third best):\n> \n> \t/*\n> \t * diff_fill_oid_info() marked one/two->oid with null_oid\n> \t * for a path whose oid is not available. Disable early\n> \t * return optimization for them.\n> \t */\n> \tif (oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n> \t\tsame_contents = 0; /* could be different */\n> \telse if (oideq(&one->oid, &two->oid))\n> \t\tsame_contents = 1; /* definitely the same */\n> \telse\n> \t\tsame_contents = 0; /* definitely different */\n\nThis is the direction I was getting at in my earlier emails, except that\nI imagined that first conditional could be checking:\n\n if (!one->oid_valid || !two->oid_valid)\n\nbut I was surprised to see that diff_fill_oid_info() does not set\noid_valid. Is that a bug?\n\nI also imagined that we'd have to determine right then whether the\ncontents are actually different or not with a memcmp(), to avoid\nemitting a \"0 changes\" line, but we do handle that case within the\n\"!same_contents\" conditional. See the comment starting with \"Omit\ndiffstats...\" added recently by 1cf3d5db9b (diff: teach --stat to ignore\nuninteresting modifications, 2020-08-20).\n\n-Peff\n"},{"id":"406068","messageId":"20200921193120.GB2399334@coredump.intra.peff.net","threadId":"54257","inReplyTo":"f338b63f-fd89-095c-b036-8d548fd2470c@gmail.com","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-21T19:31:20Z","receivedAt":"2020-09-21T19:31:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 20, 2020 at 08:54:53AM -0400, Thomas Guyot wrote:\n\n> Considering the issue with MacOS I'm starting to think the best solution\n> is to not use any heuristic and read passed-in files directly. That\n> said, I don't think it makes much change either way (if I resurrect\n> Brian's patch is will probably end up being a hybrid between the two as\n> both read the pipe at the same place and my approach was simpler further\n> down).\n> \n> I'm not sure which way I prefer to start first - will you accept a patch\n> that reads passed in files as-is if I I start with this one?\n\nI think the ideal is:\n\n - implement a command-line option to read the content of paths on the\n command-line literally (i.e., reading from pipes, dereferencing\n symlinks, etc)\n\n - make sure we have the inverse option (which you should get for free\n in step 1 if you use parse_options)\n\n - flip the default to do literal reads\n\nWe'd sometimes wait several versions before that last step to give\npeople time to adjust scripts, etc. But in this case, I suspect it would\nbe OK to just flip it immediately. We don't consider \"git diff\" itself\npart of the stable plumbing, and the --no-index part of it I would\nconsider even less stable. And AFAICT most people consider the current\nbehavior a bug because it doesn't behave like other diff tools.\n\n-Peff\n"},{"id":"406069","messageId":"xmqq1riuga5m.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200921193120.GB2399334@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T20:14:13Z","receivedAt":"2020-09-21T20:14:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We'd sometimes wait several versions before that last step to give\n> people time to adjust scripts, etc. But in this case, I suspect it would\n> be OK to just flip it immediately. We don't consider \"git diff\" itself\n> part of the stable plumbing, and the --no-index part of it I would\n> consider even less stable. And AFAICT most people consider the current\n> behavior a bug because it doesn't behave like other diff tools.\n\nThe \"git diff\" proper gets no filenames from the command line and\nthe above strictly applies only to the no-index mode, with or\nwithout the explicit \"--no-index\" option.\n\nIt was a way to give Git niceties like colored diffs, renames,\netc. to non-Git managed two sets of paths, and the primary reason\nwhy we have it as a mode of \"git diff\" is because we chose not to\nbother with interacting with upstream maintainers of \"diff\". In an\nideal world, \"GNU diff\" and others would have learned things like\nrenames, word diffs, etc., instead of \"git diff\" adding \"--no-index\"\nmode.\n\nAnd that makes me agree that users expect \"git diff --no-index\" to\nbehave like other people's diff and that is more important than\nbehaving like \"git diff\" for that mode.\n"},{"id":"406078","messageId":"xmqqft7aer3a.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200921192630.GA2399334@coredump.intra.peff.net","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T21:51:21Z","receivedAt":"2020-09-21T21:51:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For diffstat, though, it seems like a waste of time; we don't care what\n> the object hash is. I.e., if we were to do this:\n>\n> diff --git a/diff.c b/diff.c\n> index 16eeaf6645..1934af29a5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4564,9 +4564,6 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n> \tif (o->prefix_length)\n> \t\tstrip_prefix(o->prefix_length, &name, &other);\n> \n> -\tdiff_fill_oid_info(p->one, o->repo->index);\n> -\tdiff_fill_oid_info(p->two, o->repo->index);\n> -\n> \tbuiltin_diffstat(name, other, p->one, p->two,\n> \t\t\t diffstat, o, p);\n> }\n>\n> then everything seems to work fine _except_ a \"git diff --stat\n> --no-index\", exactly because it hits this \"same_contents\" check we've\n> been discussing. And once that is fixed properly (to handle any case\n> where we have no oid, not just when the stdin flag is set), then perhaps\n> it is worth doing.\n\n> Or perhaps not. Even if we have to memcmp sometimes in\n> builtin_diffstat(), it would be faster than computing the individual\n> hashes. But it may not be measurably so, and it would be no difference\n> for the common case of filespecs for which we do know the oid for free.\n> I also suspect we'd need to be a little smarter about combined formats\n> (e.g., \"--stat --patch\" might as well compute the oid as early as\n> possible, since we'll need it eventually for the patch; but we'd hit the\n> call in builtin_diffstat() before the one in run_diff()).\n>\n>> But there are paths for which oid cannot be computed without\n>> destroying their contents. Such paths are marked by the function\n>> with null_oid.\n>\n> I'm not clear how computing the oid destroys the contents. We have them\n> in an in-memory buffer at this point, don't we? So we _could_ generate\n> an oid even for stdin, like this:\n\nYes, yes yes. That is the \"best\" (which later retracted) approach I\nsuggested in the same message, but it would end up filling a\nreal-looking object name for working tree side of diff-files, which\nhas a far larger consequence we need to think about and consumes\nmore brain cycles than warranted here, I would think.\n\n>> Summarizing the above, I think the second best fix is this (which\n>> means that the posted patch is the third best):\n>> \n>> \t/*\n>> \t * diff_fill_oid_info() marked one/two->oid with null_oid\n>> \t * for a path whose oid is not available. Disable early\n>> \t * return optimization for them.\n>> \t */\n>> \tif (oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))\n>> \t\tsame_contents = 0; /* could be different */\n>> \telse if (oideq(&one->oid, &two->oid))\n>> \t\tsame_contents = 1; /* definitely the same */\n>> \telse\n>> \t\tsame_contents = 0; /* definitely different */\n>\n> This is the direction I was getting at in my earlier emails, except that\n> I imagined that first conditional could be checking:\n>\n> if (!one->oid_valid || !two->oid_valid)\n>\n> but I was surprised to see that diff_fill_oid_info() does not set\n> oid_valid. Is that a bug?\n\nI do not think so. oid_valid refers to the state during the\ncollection phase (those who called diff_addremove() etc.) and\nupdating it in diff_fill_oid_info() would lose information. Maybe\nnobody looks at the bit at this late in the processing chain these\ndays, in which case we can start flipping the bit there, but I\noffhand do not know what consequences such a change would trigger.\n\n> I also imagined that we'd have to determine right then whether the\n> contents are actually different or not with a memcmp(), to avoid\n> emitting a \"0 changes\" line, but we do handle that case within the\n> \"!same_contents\" conditional. See the comment starting with \"Omit\n> diffstats...\" added recently by 1cf3d5db9b (diff: teach --stat to ignore\n> uninteresting modifications, 2020-08-20).\n\nYes, we are essentially on the same page---same_contents bit is\nmerely an optimization to decide cheaply when we do not have to do\nxdl, but the codepath that does the xdl must be prepared to deal\nwith the \"we thought they are different, but after all they turn out\nto be equivalent\" case. Therefore false positive to declare two\ndifferent things as same cannot be tolerated, but false negative to\ndeclare two things that are the same as !same_contents is fine.\n"},{"id":"406089","messageId":"20200921222021.GA3533110@coredump.intra.peff.net","threadId":"54257","inReplyTo":"xmqqft7aer3a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-21T22:20:21Z","receivedAt":"2020-09-21T22:20:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 21, 2020 at 02:51:21PM -0700, Junio C Hamano wrote:\n\n> > This is the direction I was getting at in my earlier emails, except that\n> > I imagined that first conditional could be checking:\n> >\n> > if (!one->oid_valid || !two->oid_valid)\n> >\n> > but I was surprised to see that diff_fill_oid_info() does not set\n> > oid_valid. Is that a bug?\n> \n> I do not think so. oid_valid refers to the state during the\n> collection phase (those who called diff_addremove() etc.) and\n> updating it in diff_fill_oid_info() would lose information. Maybe\n> nobody looks at the bit at this late in the processing chain these\n> days, in which case we can start flipping the bit there, but I\n> offhand do not know what consequences such a change would trigger.\n\nWe use the flag to determine whether we need to compute the oid from\nscratch. So I would think the current code causes us to compute the oid\nmultiple times in many cases. For example, with this patch:\n\ndiff --git a/diff.c b/diff.c\nindex ee8e8189e9..8363abab5b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4424,6 +4424,8 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is\n \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n \t\t\tif (index_path(istate, &one->oid, one->path, &st, 0))\n \t\t\t\tdie(\"cannot hash %s\", one->path);\n+\t\t\twarning(\"computed oid of %s as %s\",\n+\t\t\t\tone->path, oid_to_hex(&one->oid));\n \t\t}\n \t}\n \telse\n\nI get (because diff.c is dirty in my working tree due to the patch):\n\n $ ./git diff --stat -p\n warning: computed oid of diff.c as 8363abab5b51479ac8cc9fb1c96b39fb90041f88\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n \n warning: computed oid of diff.c as 8363abab5b51479ac8cc9fb1c96b39fb90041f88\n diff --git a/diff.c b/diff.c\n index ee8e8189e9..8363abab5b 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -4424,6 +4424,8 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is\n \t\t\t\tdie_errno(\"stat '%s'\", one->path);\n \t\t\tif (index_path(istate, &one->oid, one->path, &st, 0))\n \t\t\t\tdie(\"cannot hash %s\", one->path);\n +\t\t\twarning(\"computed oid of %s as %s\",\n +\t\t\t\tone->path, oid_to_hex(&one->oid));\n \t\t}\n \t}\n \telse\n\neven though we already know the oid in the second call, so it's wasted\nwork. I agree that other code could be depending on oid_valid in a weird\nway, but IMHO that code is probably wrong to do so. But it may not be\nworth digging into, if nobody has complained about the waste.\n\n> > I also imagined that we'd have to determine right then whether the\n> > contents are actually different or not with a memcmp(), to avoid\n> > emitting a \"0 changes\" line, but we do handle that case within the\n> > \"!same_contents\" conditional. See the comment starting with \"Omit\n> > diffstats...\" added recently by 1cf3d5db9b (diff: teach --stat to ignore\n> > uninteresting modifications, 2020-08-20).\n> \n> Yes, we are essentially on the same page---same_contents bit is\n> merely an optimization to decide cheaply when we do not have to do\n> xdl, but the codepath that does the xdl must be prepared to deal\n> with the \"we thought they are different, but after all they turn out\n> to be equivalent\" case. Therefore false positive to declare two\n> different things as same cannot be tolerated, but false negative to\n> declare two things that are the same as !same_contents is fine.\n\nI thought it may matter on \"maint\", where we do not have 1cf3d5db9b.\nI.e., I expected:\n\n echo foo >a\n echo foo >b\n git diff --no-index --stat a b\n\nmight switch from no output to having a line like:\n\n a => b | 0\n\nBut we don't even get to builtin_diffstat() there. We throw out the pair\nin diffcore_skip_stat_unmatch(). Likewise, if you get past that with\nsomething like a mode change:\n\n chmod +x b\n git diff --no-index --stat a b\n\nthen that does generate the \"0\" stat line. But it does so both before\nand after the proposed change. The same thing happens in no-index mode:\n\n git init\n echo foo >file\n git add .\n git commit -am no-bit\n chmod +x file\n git commit -am exec-bit\n git show --stat\n\nwill give you:\n\n file | 0\n\nI'm not sure if that's the desired behavior or not, but at any rate\nfixing this builtin_diffstat() conditional won't change it either way. :)\n\n-Peff\n"},{"id":"406098","messageId":"xmqqv9g6dad6.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"20200921222021.GA3533110@coredump.intra.peff.net","subject":"Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T22:37:57Z","receivedAt":"2020-09-21T22:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> .... I agree that other code could be depending on oid_valid in a weird\n> way, but IMHO that code is probably wrong to do so. But it may not be\n> worth digging into, if nobody has complained about the waste.\n\nYup, that was my feeling when I wrote the message you are responding\nto.\n"},{"id":"406214","messageId":"nycvar.QRO.7.76.6.2009231702160.5061@tvgsbejvaqbjf.bet","threadId":"54257","inReplyTo":"20200918172747.GD183026@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T15:05:16Z","receivedAt":"2020-09-23T19:07:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 18 Sep 2020, Jeff King wrote:\n\n> I also wonder whether range-diff ought to be using that flag\n> [`oid_valid`] instead of is_stdin.\n\nFrom `diffcore.h`:\n\n unsigned oid_valid : 1; /* if true, use oid and trust mode;\n * if false, use the name and read from\n * the filesystem.\n */\n\nThat description leads me to believe that `oid_valid` cannot be used here:\nwe do _not_ want to read any data from the file system in `range-diff.c`'s\n`get_filespec()` function; Instead, we want to use the data provided via\nthe function parameter `p`.\n\nCiao,\nDscho\n"},{"id":"406215","messageId":"nycvar.QRO.7.76.6.2009231709340.5061@tvgsbejvaqbjf.bet","threadId":"54257","inReplyTo":"xmqq363fm02a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T19:16:20Z","receivedAt":"2020-09-23T19:16:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Sep 2020, Junio C Hamano wrote:\n\n> Thomas Guyot-Sionnest writes:\n>\n> >> > - same_contents = oideq(&one->oid, &two->oid);\n> >> > + if (one->is_stdin && two->is_stdin)\n> >> > + same_contents = !strcmp(one->data, two->data);\n> >> > + else\n> >> > + same_contents = oideq(&one->oid, &two->oid);\n> >>\n> >> ...should this actually be checking the oid_valid flag in each filespec?\n> >> That would presumably cover the is_stdin case, too. I also wonder\n> >> whether range-diff ought to be using that flag instead of is_stdin.\n> >\n> > I considered that, but IIRC when run under a debugger oid_valid was\n> > set to 0 - it seemed to be used for something different that i'm not\n> > familiar with, maybe it's an indication the object is in git datastore\n> > (whereas with --no-index outside files will only be hashed for\n> > comparison).\n>\n> If it says !oid_valid, I think you are getting what you do want.\n\nI suspect the same.\n\n> The contents from the outside world, be it what was read from the\n> standard input or a pipe, a regular file that is not up-to-date with\n> the index, may not have a usable oid computed for it, and oid_valid\n> being false signals you that you need byte-for-byte comparison. As\n> suggested by Peff in another message, you can take that signal and\n> compare the size and then the contents with memcmp() to see if they\n> are the same.\n\nTo complete the information: `struct diff_filespec`'s first attribute is\n`oid`, the object ID of the data. If it is left uninitialized (as is the\ncase in `range-diff`'s case), `oid_valid` has to be 0 to prevent it from\nbeing used.\n\nI believe that that is exactly the reason why we want this:\n\n-\tsame_contents = oideq(&one->oid, &two->oid);\n+\tsame_contents = one->oid_valid && two->oid_valid ?\n\t\toideq(&one->oid, &two->oid) : !strcmp(one->data, two->data);\n\nCiao,\nDscho\n"},{"id":"406216","messageId":"xmqqk0wki9fh.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"nycvar.QRO.7.76.6.2009231709340.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-23T19:23:46Z","receivedAt":"2020-09-23T19:23:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I believe that that is exactly the reason why we want this:\n>\n> -\tsame_contents = oideq(&one->oid, &two->oid);\n> +\tsame_contents = one->oid_valid && two->oid_valid ?\n> \t\toideq(&one->oid, &two->oid) : !strcmp(one->data, two->data);\n\nNot quite. The other side should either be\n\n\tone->size == two->size && !memcmp(...)\n\nor just left to false, as the downstream code must be prepared for\nsame_contents being false even when one and two turns out to be\nnot-byte-for-byte-same but equivalent anyway.\n"},{"id":"406221","messageId":"nycvar.QRO.7.76.6.2009232244000.5061@tvgsbejvaqbjf.bet","threadId":"54257","inReplyTo":"xmqqk0wki9fh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T20:44:26Z","receivedAt":"2020-09-23T20:44:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 23 Sep 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I believe that that is exactly the reason why we want this:\n> >\n> > -\tsame_contents = oideq(&one->oid, &two->oid);\n> > +\tsame_contents = one->oid_valid && two->oid_valid ?\n> > \t\toideq(&one->oid, &two->oid) : !strcmp(one->data, two->data);\n>\n> Not quite. The other side should either be\n>\n> \tone->size == two->size && !memcmp(...)\n\nRight!\n\nThank you for correcting my mistake,\nDscho\n\n>\n> or just left to false, as the downstream code must be prepared for\n> same_contents being false even when one and two turns out to be\n> not-byte-for-byte-same but equivalent anyway.\n"},{"id":"406231","messageId":"1d0a60c3-d15e-bcbb-f042-2f8ae06f0de1@gmail.com","threadId":"54257","inReplyTo":"nycvar.QRO.7.76.6.2009232244000.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-24T04:49:05Z","receivedAt":"2020-09-24T04:49:12Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi,\n\nOn 2020-09-23 16:44, Johannes Schindelin wrote:\n> Hi Junio,\n> \n> On Wed, 23 Sep 2020, Junio C Hamano wrote:\n> \n>> Johannes Schindelin writes:\n>>\n>>> I believe that that is exactly the reason why we want this:\n>>>\n>>> -\tsame_contents = oideq(&one->oid, &two->oid);\n>>> +\tsame_contents = one->oid_valid && two->oid_valid ?\n>>> \t\toideq(&one->oid, &two->oid) : !strcmp(one->data, two->data);\n>>\n>> Not quite. The other side should either be\n>>\n>> \tone->size == two->size && !memcmp(...)\n\nThanks for all the feedback, that was enlightening. Although I have been\nsilent the past few days I watched this thread with interest.\n\n\nSo as Junio pointed out this is merely an optimization - the range-diff\ntest that I corrected also showed two 0-line diffs and I realized\nthere's a block further down that should explicitly removes them, under\n\n else if (!same_contents) {\n\nWe can even remove same_contents entirely and everything work just fine\nafter adjusting the range-diff test - the logic is correct and\nunderlying functions already DTRT.\n\n\nMy next patch simplifies the test down to:\n\n same_contents = one->oid_valid && two->oid_valid &&\n oideq(&one->oid, &two->oid);\n\nMy understanding is that oid_valid will never be true for a modified\n(even mode change) or out of tree file so it's a valid assumption.\n\nI'll also rename that variable to \"same_oid\" - the current name is\nmisleading both ways (true doesn't means there will be diffs, false\ndoesn't mean contents differs).\n\nRegards,\n\nThomas\n"},{"id":"406232","messageId":"20200924052406.11349-1-tguyot@gmail.com","threadId":"54257","inReplyTo":"1d0a60c3-d15e-bcbb-f042-2f8ae06f0de1@gmail.com","subject":"[PATCH v3] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-24T05:24:07Z","receivedAt":"2020-09-24T05:26:08Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Only skip diffstats when both oids are valid and identical. This check\nwas causing both false-positives (files included in diffstats with no\nactual changes (0 lines modified) and false-negatives (showing 0 lines\nmodified in stats when files had actually changed).\n\nAlso renamed same_contents to same_file to avoid confusion.\n\nSigned-off-by: Thomas Guyot-Sionnest \n---\nInterdiff:\n diff --git a/diff.c b/diff.c\n index 2e47bf824e..77e0bd772e 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -3663,7 +3663,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n {\n \tmmfile_t mf1, mf2;\n \tstruct diffstat_file *data;\n -\tint same_contents;\n +\tint same_file;\n \tint complete_rewrite = 0;\n \n \tif (!DIFF_PAIR_UNMERGED(p)) {\n @@ -3681,19 +3681,14 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n -\t/* What is_stdin really means is that the file's content is only\n -\t * in the filespec's buffer and its oid is zero. We can't compare\n -\t * oid's if both are null and we can just diff the buffers */\n -\tif (one->is_stdin && two->is_stdin)\n -\t\tsame_contents = (one->size == two->size ?\n -\t\t\t!memcmp(one->data, two->data, one->size) : 0);\n -\telse\n -\t\tsame_contents = oideq(&one->oid, &two->oid);\n +\t/* saves some reads if true, not a guarantee of diff outcome */\n +\tsame_file = one->oid_valid && two->oid_valid &&\n +\t\toideq(&one->oid, &two->oid);\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\n \t\tdata->is_binary = 1;\n -\t\tif (same_contents) {\n +\t\tif (same_file) {\n \t\t\tdata->added = 0;\n \t\t\tdata->deleted = 0;\n \t\t} else {\n @@ -3709,7 +3704,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n \n -\telse if (!same_contents) {\n +\telse if (!same_file) {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n @@ -3734,7 +3729,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\tdiffstat->files[diffstat->nr - 1];\n \t\t\t/*\n \t\t\t * Omit diffstats of modified files where nothing changed.\n -\t\t\t * Even if !same_contents, this might be the case due to\n +\t\t\t * Even if !same_file, this might be the case due to\n \t\t\t * ignoring whitespace changes, etc.\n \t\t\t *\n \t\t\t * But note that we special-case additions, deletions,\n diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n index 4715e75b68..6eb344be03 100755\n --- a/t/t3206-range-diff.sh\n +++ b/t/t3206-range-diff.sh\n @@ -252,11 +252,7 @@ test_expect_success 'changed commit with --stat diff option' '\n \tgit range-diff --no-color --stat topic...changed >actual &&\n \tcat >expect <<-EOF &&\n \t1: $(test_oid t1) = 1: $(test_oid c1) s/5/A/\n -\t a => b | 0\n -\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t2: $(test_oid t2) = 2: $(test_oid c2) s/4/A/\n -\t a => b | 0\n -\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n \t a => b | 2 +-\n \t 1 file changed, 1 insertion(+), 1 deletion(-)\n\n diff.c | 12 +++++++-----\n t/t3206-range-diff.sh | 12 ++++--------\n 2 files changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ee8e8189e9..77e0bd772e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3663,7 +3663,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n {\n \tmmfile_t mf1, mf2;\n \tstruct diffstat_file *data;\n-\tint same_contents;\n+\tint same_file;\n \tint complete_rewrite = 0;\n \n \tif (!DIFF_PAIR_UNMERGED(p)) {\n@@ -3681,12 +3681,14 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n-\tsame_contents = oideq(&one->oid, &two->oid);\n+\t/* saves some reads if true, not a guarantee of diff outcome */\n+\tsame_file = one->oid_valid && two->oid_valid &&\n+\t\toideq(&one->oid, &two->oid);\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\n \t\tdata->is_binary = 1;\n-\t\tif (same_contents) {\n+\t\tif (same_file) {\n \t\t\tdata->added = 0;\n \t\t\tdata->deleted = 0;\n \t\t} else {\n@@ -3702,7 +3704,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n \n-\telse if (!same_contents) {\n+\telse if (!same_file) {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n@@ -3727,7 +3729,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\tdiffstat->files[diffstat->nr - 1];\n \t\t\t/*\n \t\t\t * Omit diffstats of modified files where nothing changed.\n-\t\t\t * Even if !same_contents, this might be the case due to\n+\t\t\t * Even if !same_file, this might be the case due to\n \t\t\t * ignoring whitespace changes, etc.\n \t\t\t *\n \t\t\t * But note that we special-case additions, deletions,\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e024cff65c..6eb344be03 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -252,17 +252,13 @@ test_expect_success 'changed commit with --stat diff option' '\n \tgit range-diff --no-color --stat topic...changed >actual &&\n \tcat >expect <<-EOF &&\n \t1: $(test_oid t1) = 1: $(test_oid c1) s/5/A/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t2: $(test_oid t2) = 2: $(test_oid c2) s/4/A/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \t4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \tEOF\n \ttest_cmp expect actual\n '\n-- \n2.20.1\n\n"},{"id":"406237","messageId":"xmqq4knnisn9.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"1d0a60c3-d15e-bcbb-f042-2f8ae06f0de1@gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T06:40:58Z","receivedAt":"2020-09-24T06:41:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Guyot writes:\n\n> My next patch simplifies the test down to:\n>\n> same_contents = one->oid_valid && two->oid_valid &&\n> oideq(&one->oid, &two->oid);\n>\n> My understanding is that oid_valid will never be true for a modified\n> (even mode change) or out of tree file so it's a valid assumption.\n>\n> I'll also rename that variable to \"same_oid\" - the current name is\n> misleading both ways (true doesn't means there will be diffs, false\n> doesn't mean contents differs).\n\nIt is not \"both ways\", I think. The idea is that when this variable\nis true, we know with certainty that these two are the same, but\neven when the variable is false, they still can be the same. So\ntrue does mean there will not be diff. False indeed is fuzzy.\n\nAnd as long as one side gives a 100% correct answer cheaply, we can\nuse it as an optimization (and 'true' being that side in this case).\n\nI have a mild suspicion that the name same_anything conveys a wrong\nimpression, no matter what word you use for . It does not\ncapture that we are saying the \"true\" side has no false positive.\n\nAnd that is why I alluded to \"may_differ\" earlier (with opposite\npolarity). The flow would become:\n\n may_differ = !one->oid_valid || !two->oid_valid || !oideq(...);\n\n if (binary) {\n if (!may_differ) {\n added = deleted = 0;\n ...\n } else {\n ... count added and deleted ...\n }\n } else if (rewrite) {\n\t...\n } else if (may_differ) {\n\t... use xdl ...\n }\n\nand it would become quite straight-forward to follow. When there is\nno chance that they may be different, we short-cut and otherwise we\ncompute without cheating. Only when they can be different, we do\nthe expensive xdl thing.\n\nThanks.\n\n\n\n"},{"id":"406241","messageId":"34484667-1085-c60b-9438-591faed41ddc@gmail.com","threadId":"54257","inReplyTo":"xmqq4knnisn9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-24T07:13:53Z","receivedAt":"2020-09-24T07:13:58Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Hi Junio,\n\nOn 2020-09-24 02:40, Junio C Hamano wrote:\n> Thomas Guyot writes:\n> \n> It is not \"both ways\", I think. The idea is that when this variable\n> is true, we know with certainty that these two are the same, but\n> even when the variable is false, they still can be the same. So\n> true does mean there will not be diff. False indeed is fuzzy.\n\nI meant to say the old behavior \"lied\" in both directions.\n\n> And as long as one side gives a 100% correct answer cheaply, we can\n> use it as an optimization (and 'true' being that side in this case).\n> \n> I have a mild suspicion that the name same_anything conveys a wrong\n> impression, no matter what word you use for . It does not\n> capture that we are saying the \"true\" side has no false positive.\n> \n> And that is why I alluded to \"may_differ\" earlier (with opposite\n> polarity). The flow would become:\n> \n> may_differ = !one->oid_valid || !two->oid_valid || !oideq(...);\n> \n> if (binary) {\n> if (!may_differ) {\n> added = deleted = 0;\n> ...\n> } else {\n> ... count added and deleted ...\n> }\n> } else if (rewrite) {\n> \t...\n> } else if (may_differ) {\n> \t... use xdl ...\n> }\n> \n> and it would become quite straight-forward to follow. When there is\n> no chance that they may be different, we short-cut and otherwise we\n> compute without cheating. Only when they can be different, we do\n> the expensive xdl thing.\n\nI toyed a bit on the binary side... I never sent my 2nd reply as I still\nneeded to dig up; testing with diff_filespec_is_binary() { return 1; } I\nwould get (as expected) the same false-positive modified binary files I\nused to get in range-diff test.\n\nWhat I didn't get is applying the same logic\n(free_diffstat_file(file);diffstat->nr--;) didn't have any effect. I'll\nhave to find what differs here to make binary files how up regardless.\n\nRegards,\n\nThomas\n"},{"id":"406244","messageId":"20200924074140.31153-1-tguyot@gmail.com","threadId":"54257","inReplyTo":"20200924052406.11349-1-tguyot@gmail.com","subject":"[PATCH v4] diff: Fix modified lines stats with --stat and --numstat","fromName":"Thomas Guyot-Sionnest","fromEmail":"tguyot@gmail.com","sentAt":"2020-09-24T07:41:41Z","receivedAt":"2020-09-24T07:42:03Z","isPatch":true,"sender":{"key":"tguyot@gmail.com","avatar":"https://avatars.githubusercontent.com/u/403890?v=4"},"body":"Only skip diffstats when both oids are valid and identical. This check\nwas causing both false-positives (files included in diffstats with no\nactual changes (0 lines modified) and false-negatives (showing 0 lines\nmodified in stats when files had actually changed).\n\nAlso replaced same_contents with may_differ to avoid confusion.\n\nSigned-off-by: Thomas Guyot-Sionnest \n---\nInterdiff:\n diff --git a/diff.c b/diff.c\n index 77e0bd772e..2bb2f8f57e 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -3663,7 +3663,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n {\n \tmmfile_t mf1, mf2;\n \tstruct diffstat_file *data;\n -\tint same_file;\n +\tint may_differ;\n \tint complete_rewrite = 0;\n \n \tif (!DIFF_PAIR_UNMERGED(p)) {\n @@ -3682,13 +3682,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t}\n \n \t/* saves some reads if true, not a guarantee of diff outcome */\n -\tsame_file = one->oid_valid && two->oid_valid &&\n -\t\toideq(&one->oid, &two->oid);\n +\tmay_differ = !(one->oid_valid && two->oid_valid &&\n +\t\t\toideq(&one->oid, &two->oid));\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\n \t\tdata->is_binary = 1;\n -\t\tif (same_file) {\n +\t\tif (!may_differ) {\n \t\t\tdata->added = 0;\n \t\t\tdata->deleted = 0;\n \t\t} else {\n @@ -3704,7 +3704,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n \n -\telse if (!same_file) {\n +\telse if (may_differ) {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n @@ -3729,7 +3729,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\tdiffstat->files[diffstat->nr - 1];\n \t\t\t/*\n \t\t\t * Omit diffstats of modified files where nothing changed.\n -\t\t\t * Even if !same_file, this might be the case due to\n +\t\t\t * Even if may_differ, this might be the case due to\n \t\t\t * ignoring whitespace changes, etc.\n \t\t\t *\n \t\t\t * But note that we special-case additions, deletions,\n\n diff.c | 12 +++++++-----\n t/t3206-range-diff.sh | 12 ++++--------\n 2 files changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ee8e8189e9..2bb2f8f57e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3663,7 +3663,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n {\n \tmmfile_t mf1, mf2;\n \tstruct diffstat_file *data;\n-\tint same_contents;\n+\tint may_differ;\n \tint complete_rewrite = 0;\n \n \tif (!DIFF_PAIR_UNMERGED(p)) {\n@@ -3681,12 +3681,14 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\treturn;\n \t}\n \n-\tsame_contents = oideq(&one->oid, &two->oid);\n+\t/* saves some reads if true, not a guarantee of diff outcome */\n+\tmay_differ = !(one->oid_valid && two->oid_valid &&\n+\t\t\toideq(&one->oid, &two->oid));\n \n \tif (diff_filespec_is_binary(o->repo, one) ||\n \t diff_filespec_is_binary(o->repo, two)) {\n \t\tdata->is_binary = 1;\n-\t\tif (same_contents) {\n+\t\tif (!may_differ) {\n \t\t\tdata->added = 0;\n \t\t\tdata->deleted = 0;\n \t\t} else {\n@@ -3702,7 +3704,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n \n-\telse if (!same_contents) {\n+\telse if (may_differ) {\n \t\t/* Crazy xdl interfaces.. */\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n@@ -3727,7 +3729,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\t\t\tdiffstat->files[diffstat->nr - 1];\n \t\t\t/*\n \t\t\t * Omit diffstats of modified files where nothing changed.\n-\t\t\t * Even if !same_contents, this might be the case due to\n+\t\t\t * Even if may_differ, this might be the case due to\n \t\t\t * ignoring whitespace changes, etc.\n \t\t\t *\n \t\t\t * But note that we special-case additions, deletions,\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e024cff65c..6eb344be03 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -252,17 +252,13 @@ test_expect_success 'changed commit with --stat diff option' '\n \tgit range-diff --no-color --stat topic...changed >actual &&\n \tcat >expect <<-EOF &&\n \t1: $(test_oid t1) = 1: $(test_oid c1) s/5/A/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t2: $(test_oid t2) = 2: $(test_oid c2) s/4/A/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n \t3: $(test_oid t3) ! 3: $(test_oid c3) s/11/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \t4: $(test_oid t4) ! 4: $(test_oid c4) s/12/B/\n-\t a => b | 0\n-\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t a => b | 2 +-\n+\t 1 file changed, 1 insertion(+), 1 deletion(-)\n \tEOF\n \ttest_cmp expect actual\n '\n-- \n2.20.1\n\n"},{"id":"406273","messageId":"xmqq7dsjgki8.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"34484667-1085-c60b-9438-591faed41ddc@gmail.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T17:19:43Z","receivedAt":"2020-09-24T17:19:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Guyot writes:\n\n> Hi Junio,\n>\n> On 2020-09-24 02:40, Junio C Hamano wrote:\n>> Thomas Guyot writes:\n>> \n>> It is not \"both ways\", I think. The idea is that when this variable\n>> is true, we know with certainty that these two are the same, but\n>> even when the variable is false, they still can be the same. So\n>> true does mean there will not be diff. False indeed is fuzzy.\n>\n> I meant to say the old behavior \"lied\" in both directions.\n\nIt depends on the perspective ;-)\n\nThe old one didn't expect/realize fill_oid_info() can leave the oid\nfield to \"unknown\". It was OK because is_stdin happens to be the\nonly such case [*1*] and we never saw both one->oid and two->oid\nbeing the null_oid at the same time, so it wasn't an issue that\ntheir validity weren't checked there. As long as one side was\nvalid, when the comparison said they were equal, they indeed were\nequal. So in that sense, the true side did not lie.\n\nIf you add new case where the oid of both sides can legitimately be\nnull_oid, that will of course break the code. I think that is the\nreason why we are having this discussion to prepare for such a\nfuture (that happens in 2/2???).\n\n\n[Footnote]\n\n*1* The missing side of addition and deletion will also get null_oid,\nbut we don't compare that with stdin in such a case.\n"},{"id":"406275","messageId":"xmqqy2kzf51s.fsf@gitster.c.googlers.com","threadId":"54257","inReplyTo":"xmqq7dsjgki8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T17:38:55Z","receivedAt":"2020-09-24T17:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If you add new case where the oid of both sides can legitimately be\n> null_oid, that will of course break the code.\n\nAhh, I forgot that we already had such an iffy caller that broke the\ncode. I should have re-read the test part of the patch.\n\nThanks.\n"}]}