From 498198453dbb8ae46becbc83a1ef0979a0eb805d Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 5 Jul 2023 20:49:27 +0100 Subject: [PATCH 1/4] diff --no-index: refuse to compare stdin to a directory When the user runs git diff --no-index file directory we follow the behavior of POSIX diff and rewrite the arguments as git diff --no-index file directory/file Doing that when "file" is "-" (which means "read from stdin") does not make sense so we should error out if the user asks us to compare "-" to a directory. This matches the behavior of GNU diff and diff on *BSD. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- diff-no-index.c | 12 +++++++----- t/t4053-diff-no-index.sh | 5 +++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index 4296940f90..77462ac2a9 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -218,11 +218,13 @@ static void fixup_paths(const char **path, struct strbuf *replacement) { unsigned int isdir0, isdir1; - if (path[0] == file_from_standard_input || - path[1] == file_from_standard_input) - return; - isdir0 = is_directory(path[0]); - isdir1 = is_directory(path[1]); + isdir0 = path[0] != file_from_standard_input && is_directory(path[0]); + isdir1 = path[1] != file_from_standard_input && is_directory(path[1]); + + if ((path[0] == file_from_standard_input && isdir1) || + (isdir0 && path[1] == file_from_standard_input)) + die(_("cannot compare stdin to a directory")); + if (isdir0 == isdir1) return; if (isdir0) { diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 4e9fa0403d..5bfb282e98 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -205,4 +205,9 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik test_cmp expected actual ' +test_expect_success 'diff --no-index refuses to diff stdin and a directory' ' + test_must_fail git diff --no-index -- - a err && + grep "fatal: cannot compare stdin to a directory" err +' + test_done From 4e61e0f68053ff3fcca298ebd3080527e3565004 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 5 Jul 2023 20:49:28 +0100 Subject: [PATCH 2/4] diff --no-index: die on error reading stdin If there is an error when reading from stdin then "diff --no-index" prints an error message but continues to try and diff a file named "-" resulting in an error message that looks like error: error while reading from stdin: Invalid argument fatal: stat '-': No such file or directory assuming that no file named "-" exists. If such a file exists it prints the first error message and generates the diff from that file which is not what the user wanted. Instead just die() straight away if we cannot read from stdin. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- diff-no-index.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index 77462ac2a9..4470e0271d 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -60,20 +60,19 @@ static int get_mode(const char *path, int *mode) return 0; } -static int populate_from_stdin(struct diff_filespec *s) +static void populate_from_stdin(struct diff_filespec *s) { struct strbuf buf = STRBUF_INIT; size_t size = 0; if (strbuf_read(&buf, 0, 0) < 0) - return error_errno("error while reading from stdin"); + die_errno("error while reading from stdin"); s->should_munmap = 0; s->data = strbuf_detach(&buf, &size); s->size = size; s->should_free = 1; s->is_stdin = 1; - return 0; } static struct diff_filespec *noindex_filespec(const char *name, int mode) From df521462f01b61314c9c18a4b58a53db0cd7bf3c Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 5 Jul 2023 20:49:29 +0100 Subject: [PATCH 3/4] t4054: test diff --no-index with stdin "git diff --no-index" supports reading from stdin with the path "-". There is no test coverage for this so add a regression test before changing the code in the next commit. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- t/t4053-diff-no-index.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 5bfb282e98..4870443609 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -205,6 +205,25 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik test_cmp expected actual ' +test_expect_success "diff --no-index treats '-' as stdin" ' + cat >expect <<-EOF && + diff --git a/- b/a/1 + index $ZERO_OID..$(git hash-object --stdin actual && + test_cmp expect actual && + + test_write_lines 1 | git diff --no-index -- a/1 - >actual && + test_must_be_empty actual +' + test_expect_success 'diff --no-index refuses to diff stdin and a directory' ' test_must_fail git diff --no-index -- - a err && grep "fatal: cannot compare stdin to a directory" err From 1e3f26542a6ecd3006c2c0d5ccc0bae4a700f7e5 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 5 Jul 2023 20:49:30 +0100 Subject: [PATCH 4/4] diff --no-index: support reading from named pipes In some shells, such as bash and zsh, it's possible to use a command substitution to provide the output of a command as a file argument to another process, like so: diff -u <(printf "a\nb\n") <(printf "a\nc\n") However, this syntax does not produce useful results with "git diff --no-index". On macOS, the arguments to the command are named pipes under /dev/fd, and git diff doesn't know how to handle a named pipe. On Linux, the arguments are symlinks to pipes, so git diff "helpfully" diffs these symlinks, comparing their targets like "pipe:[1234]" and "pipe:[5678]". To address this "diff --no-index" is changed so that if a path given on the commandline is a named pipe or a symbolic link that resolves to a named pipe then we read the data to diff from that pipe. This is implemented by generalizing the code that already exists to handle reading from stdin when the user passes the path "-". If the user tries to compare a named pipe to a directory then we die as we do when trying to compare stdin to a directory. As process substitution is not support by POSIX this change is tested by using a pipe and a symbolic link to a pipe. Helped-by: Junio C Hamano Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- diff-no-index.c | 111 +++++++++++++++++++++++++++++---------- t/t4053-diff-no-index.sh | 40 ++++++++++++++ 2 files changed, 124 insertions(+), 27 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index 4470e0271d..4771cf02aa 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -41,41 +41,81 @@ static int read_directory_contents(const char *path, struct string_list *list) */ static const char file_from_standard_input[] = "-"; -static int get_mode(const char *path, int *mode) +/* + * For paths given on the command-line we treat "-" as stdin and named + * pipes and symbolic links to named pipes specially. + */ +enum special { + SPECIAL_NONE, + SPECIAL_STDIN, + SPECIAL_PIPE, +}; + +static int get_mode(const char *path, int *mode, enum special *special) { struct stat st; - if (!path || !strcmp(path, "/dev/null")) + if (!path || !strcmp(path, "/dev/null")) { *mode = 0; #ifdef GIT_WINDOWS_NATIVE - else if (!strcasecmp(path, "nul")) + } else if (!strcasecmp(path, "nul")) { *mode = 0; #endif - else if (path == file_from_standard_input) + } else if (path == file_from_standard_input) { *mode = create_ce_mode(0666); - else if (lstat(path, &st)) + *special = SPECIAL_STDIN; + } else if (lstat(path, &st)) { return error("Could not access '%s'", path); - else + } else { *mode = st.st_mode; + } + /* + * For paths on the command-line treat named pipes and symbolic + * links that resolve to a named pipe specially. + */ + if (special && + (S_ISFIFO(*mode) || + (S_ISLNK(*mode) && !stat(path, &st) && S_ISFIFO(st.st_mode)))) { + *mode = create_ce_mode(0666); + *special = SPECIAL_PIPE; + } + return 0; } -static void populate_from_stdin(struct diff_filespec *s) +static void populate_common(struct diff_filespec *s, struct strbuf *buf) { - struct strbuf buf = STRBUF_INIT; size_t size = 0; - if (strbuf_read(&buf, 0, 0) < 0) - die_errno("error while reading from stdin"); - s->should_munmap = 0; - s->data = strbuf_detach(&buf, &size); + s->data = strbuf_detach(buf, &size); s->size = size; s->should_free = 1; s->is_stdin = 1; } -static struct diff_filespec *noindex_filespec(const char *name, int mode) +static void populate_from_pipe(struct diff_filespec *s) +{ + struct strbuf buf = STRBUF_INIT; + int fd = xopen(s->path, O_RDONLY); + + if (strbuf_read(&buf, fd, 0) < 0) + die_errno("error while reading from '%s'", s->path); + close(fd); + populate_common(s, &buf); +} + +static void populate_from_stdin(struct diff_filespec *s) +{ + struct strbuf buf = STRBUF_INIT; + + if (strbuf_read(&buf, 0, 0) < 0) + die_errno("error while reading from stdin"); + populate_common(s, &buf); +} + +static struct diff_filespec *noindex_filespec(const char *name, int mode, + enum special special) { struct diff_filespec *s; @@ -83,17 +123,22 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode) name = "/dev/null"; s = alloc_filespec(name); fill_filespec(s, null_oid(), 0, mode); - if (name == file_from_standard_input) + if (special == SPECIAL_STDIN) populate_from_stdin(s); + else if (special == SPECIAL_PIPE) + populate_from_pipe(s); return s; } static int queue_diff(struct diff_options *o, - const char *name1, const char *name2) + const char *name1, const char *name2, int recursing) { int mode1 = 0, mode2 = 0; + enum special special1 = SPECIAL_NONE, special2 = SPECIAL_NONE; - if (get_mode(name1, &mode1) || get_mode(name2, &mode2)) + /* Paths can only be special if we're not recursing. */ + if (get_mode(name1, &mode1, recursing ? NULL : &special1) || + get_mode(name2, &mode2, recursing ? NULL : &special2)) return -1; if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) { @@ -101,14 +146,14 @@ static int queue_diff(struct diff_options *o, if (S_ISDIR(mode1)) { /* 2 is file that is created */ - d1 = noindex_filespec(NULL, 0); - d2 = noindex_filespec(name2, mode2); + d1 = noindex_filespec(NULL, 0, SPECIAL_NONE); + d2 = noindex_filespec(name2, mode2, special2); name2 = NULL; mode2 = 0; } else { /* 1 is file that is deleted */ - d1 = noindex_filespec(name1, mode1); - d2 = noindex_filespec(NULL, 0); + d1 = noindex_filespec(name1, mode1, special1); + d2 = noindex_filespec(NULL, 0, SPECIAL_NONE); name1 = NULL; mode1 = 0; } @@ -173,7 +218,7 @@ static int queue_diff(struct diff_options *o, n2 = buffer2.buf; } - ret = queue_diff(o, n1, n2); + ret = queue_diff(o, n1, n2, 1); } string_list_clear(&p1, 0); string_list_clear(&p2, 0); @@ -189,8 +234,8 @@ static int queue_diff(struct diff_options *o, SWAP(name1, name2); } - d1 = noindex_filespec(name1, mode1); - d2 = noindex_filespec(name2, mode2); + d1 = noindex_filespec(name1, mode1, special1); + d2 = noindex_filespec(name2, mode2, special2); diff_queue(&diff_queued_diff, d1, d2); return 0; } @@ -215,15 +260,27 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi */ static void fixup_paths(const char **path, struct strbuf *replacement) { - unsigned int isdir0, isdir1; + struct stat st; + unsigned int isdir0 = 0, isdir1 = 0; + unsigned int ispipe0 = 0, ispipe1 = 0; - isdir0 = path[0] != file_from_standard_input && is_directory(path[0]); - isdir1 = path[1] != file_from_standard_input && is_directory(path[1]); + if (path[0] != file_from_standard_input && !stat(path[0], &st)) { + isdir0 = S_ISDIR(st.st_mode); + ispipe0 = S_ISFIFO(st.st_mode); + } + + if (path[1] != file_from_standard_input && !stat(path[1], &st)) { + isdir1 = S_ISDIR(st.st_mode); + ispipe1 = S_ISFIFO(st.st_mode); + } if ((path[0] == file_from_standard_input && isdir1) || (isdir0 && path[1] == file_from_standard_input)) die(_("cannot compare stdin to a directory")); + if ((isdir0 && ispipe1) || (ispipe0 && isdir1)) + die(_("cannot compare a named pipe to a directory")); + if (isdir0 == isdir1) return; if (isdir0) { @@ -297,7 +354,7 @@ int diff_no_index(struct rev_info *revs, setup_diff_pager(&revs->diffopt); revs->diffopt.flags.exit_with_status = 1; - if (queue_diff(&revs->diffopt, paths[0], paths[1])) + if (queue_diff(&revs->diffopt, paths[0], paths[1], 0)) goto out; diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/"); diffcore_std(&revs->diffopt); diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 4870443609..a28b9ff243 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -229,4 +229,44 @@ test_expect_success 'diff --no-index refuses to diff stdin and a directory' ' grep "fatal: cannot compare stdin to a directory" err ' +test_expect_success PIPE 'diff --no-index refuses to diff a named pipe and a directory' ' + test_when_finished "rm -f pipe" && + mkfifo pipe && + { + (>pipe) & + } && + test_when_finished "kill $!" && + test_must_fail git diff --no-index -- pipe a 2>err && + grep "fatal: cannot compare a named pipe to a directory" err +' + +test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' ' + test_when_finished "rm -f old new new-link" && + mkfifo old && + mkfifo new && + ln -s new new-link && + { + (test_write_lines a b c >old) & + } && + test_when_finished "! kill $!" && + { + (test_write_lines a x c >new) & + } && + test_when_finished "! kill $!" && + + cat >expect <<-EOF && + diff --git a/old b/new-link + --- a/old + +++ b/new-link + @@ -1,3 +1,3 @@ + a + -b + +x + c + EOF + + test_expect_code 1 git diff --no-index old new-link >actual && + test_cmp expect actual +' + test_done