threads / discuss / 60215

Regression in git diff with stdin, with -R

Subject: Regression in git diff with stdin, with -R

## tl;dr

6 messages between Sep 9, 2023 and Sep 11, 2023.

replies: 5people: 5as markdown or json

Martin Storsjö· Sep 9, 2023, 20:42 UTC · lore

Hi,

Since 1e3f26542a6ecd3006c2c0d5ccc0bae4a700f7e5, "diff --no-index: support reading from named pipes", one usecase about diffing with stdin has broken.

I see that this patch was preceded by adding some extra tests around diffing with stdin - but one case seem to have been missed.

"git diff --no-index - regularfile" still works fine as it did before, also "git diff --no-index regularfile -" also still works. (I.e. stdin can either be the first or second file argument - both work.)

However if using the -R option to reverse the diff direction, i.e. "git diff --no-index -R - regularfile" or "git diff --no-index -R regularfile -", I'm now getting the following error:

     fatal: stat '-': No such file or directory

// Martin

René Scharfe· Sep 9, 2023, 22:12 UTC · re: Martin Storsjö · lore

[PATCH] diff --no-index: fix -R with stdin

When -R is given, queue_diff() swaps the mode and name variables of the two files to produce a reverse diff. 1e3f26542a (diff --no-index: support reading from named pipes, 2023-07-05) added variables that indicate whether files are special, i.e named pipes or - for stdin. These new variables were not swapped, though, which broke the handling of stdin with with -R. Swap them like the other metadata variables.

Reported-by: Martin Storsjö <martin@martin.st>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Great bug report, thank you!

 diff-no-index.c          |  1 +
 t/t4053-diff-no-index.sh | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/diff-no-index.c b/diff-no-index.c
index 8aead3e332..e7041b89e3 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -232,6 +232,7 @@ static int queue_diff(struct diff_options *o,
 		if (o->flags.reverse_diff) {
 			SWAP(mode1, mode2);
 			SWAP(name1, name2);
+			SWAP(special1, special2);
 		}

 		d1 = noindex_filespec(name1, mode1, special1);
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 6781cc9078..5f059f65fc 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -224,6 +224,25 @@ test_expect_success "diff --no-index treats '-' as stdin" '
 	test_must_be_empty actual
 '

+test_expect_success "diff --no-index -R treats '-' as stdin" '
+	cat >expect <<-EOF &&
+	diff --git b/a/1 a/-
+	index $(git hash-object --stdin <a/1)..$ZERO_OID 100644
+	--- b/a/1
+	+++ a/-
+	@@ -1 +1 @@
+	-1
+	+x
+	EOF
+
+	test_write_lines x | test_expect_code 1 \
+		git -c core.abbrev=no diff --no-index -R -- - a/1 >actual &&
+	test_cmp expect actual &&
+
+	test_write_lines 1 | git diff --no-index -R -- 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 </dev/null 2>err &&
 	grep "fatal: cannot compare stdin to a directory" err
--
2.42.0

Phillip Wood· Sep 10, 2023, 10:00 UTC · re: René Scharfe · lore

Re: [PATCH] diff --no-index: fix -R with stdin

On 09/09/2023 23:12, René Scharfe wrote:

Show 11 quoted lines

> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff.  1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R.  Swap them like the other metadata variables.
> 
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!

Yes thank you Martin for reporting this and thank you to René for fixing it - I saw the report just before I went to bed and it was already fixed by the time I got up! The patch looks good and thanks for adding a test.

Best Wishes

Phillip

Show 48 quoted lines

>   diff-no-index.c          |  1 +
>   t/t4053-diff-no-index.sh | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8aead3e332..e7041b89e3 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -232,6 +232,7 @@ static int queue_diff(struct diff_options *o,
>   		if (o->flags.reverse_diff) {
>   			SWAP(mode1, mode2);
>   			SWAP(name1, name2);
> +			SWAP(special1, special2);
>   		}
> 
>   		d1 = noindex_filespec(name1, mode1, special1);
> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index 6781cc9078..5f059f65fc 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -224,6 +224,25 @@ test_expect_success "diff --no-index treats '-' as stdin" '
>   	test_must_be_empty actual
>   '
> 
> +test_expect_success "diff --no-index -R treats '-' as stdin" '
> +	cat >expect <<-EOF &&
> +	diff --git b/a/1 a/-
> +	index $(git hash-object --stdin <a/1)..$ZERO_OID 100644
> +	--- b/a/1
> +	+++ a/-
> +	@@ -1 +1 @@
> +	-1
> +	+x
> +	EOF
> +
> +	test_write_lines x | test_expect_code 1 \
> +		git -c core.abbrev=no diff --no-index -R -- - a/1 >actual &&
> +	test_cmp expect actual &&
> +
> +	test_write_lines 1 | git diff --no-index -R -- 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 </dev/null 2>err &&
>   	grep "fatal: cannot compare stdin to a directory" err
> --
> 2.42.0

Taylor Blau· Sep 10, 2023, 18:41 UTC · re: René Scharfe · lore

Re: [PATCH] diff --no-index: fix -R with stdin

On Sun, Sep 10, 2023 at 12:12:52AM +0200, René Scharfe wrote:

Show 11 quoted lines

> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff.  1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R.  Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!

Great patch ;-). LGTM!

Thanks, Taylor

Martin Storsjö· Sep 11, 2023, 19:20 UTC · re: René Scharfe · lore

Re: [PATCH] diff --no-index: fix -R with stdin

On Sun, 10 Sep 2023, René Scharfe wrote:

Show 11 quoted lines

> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff.  1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R.  Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!

Thanks for the extremely swift response and fix!

// Martin

Junio C Hamano· Sep 11, 2023, 19:04 UTC · re: René Scharfe · lore

Re: [PATCH] diff --no-index: fix -R with stdin

René Scharfe <l.s.r@web.de> writes:

Show 11 quoted lines

> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff.  1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R.  Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!

Looking good.  I wish all our bugs are this easy and obvious ;-)

Show 48 quoted lines

>  diff-no-index.c          |  1 +
>  t/t4053-diff-no-index.sh | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8aead3e332..e7041b89e3 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -232,6 +232,7 @@ static int queue_diff(struct diff_options *o,
>  		if (o->flags.reverse_diff) {
>  			SWAP(mode1, mode2);
>  			SWAP(name1, name2);
> +			SWAP(special1, special2);
>  		}
>
>  		d1 = noindex_filespec(name1, mode1, special1);
> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index 6781cc9078..5f059f65fc 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -224,6 +224,25 @@ test_expect_success "diff --no-index treats '-' as stdin" '
>  	test_must_be_empty actual
>  '
>
> +test_expect_success "diff --no-index -R treats '-' as stdin" '
> +	cat >expect <<-EOF &&
> +	diff --git b/a/1 a/-
> +	index $(git hash-object --stdin <a/1)..$ZERO_OID 100644
> +	--- b/a/1
> +	+++ a/-
> +	@@ -1 +1 @@
> +	-1
> +	+x
> +	EOF
> +
> +	test_write_lines x | test_expect_code 1 \
> +		git -c core.abbrev=no diff --no-index -R -- - a/1 >actual &&
> +	test_cmp expect actual &&
> +
> +	test_write_lines 1 | git diff --no-index -R -- 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 </dev/null 2>err &&
>  	grep "fatal: cannot compare stdin to a directory" err
> --
> 2.42.0