git stash show -v

Hi all,

I've been using jch's branch and I discovered a regression in ps/stash-in-c.

Here's a test-case on git.git:

echo '/**/' >>abspath.c
git stash
git stash show -v
# I am expecting the diff to show up here

I am expecting the diff to show up but in reality, I get no output. However, if I compile the latest master, the diff does show up.

Note that I can get the patch to show up using "git stash show -p" so it's not really a showstopper. However, my understanding is that this was supposed to be a one-to-one (bug included!) port.

Subject: [PATCH] stash: setup default diff output format if necessary

In the scripted 'git stash show' when no arguments are passed, we just pass '--stat' to 'git diff'. When any argument is passed to 'stash show', we no longer pass '--stat' to 'git diff', and pass whatever flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when we a user uses 'git stash show -v', they would be shown the diff, because that's the default behaviour of 'git diff', but not actually directly triggered by passing the '-v'.

In the C version of 'git stash show', we try to emulate that behaviour using the internal diff API. However we forgot to set up the default output format, in case it wasn't set by any of the flags that were passed through.

So 'git stash show -v' in the builtin
version of stash would be completely silent, while it would show the diff before.

Fix this by setting up the default output format for 'git diff'.

Reported-by Denton Liu <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---
 builtin/stash.c  |  4 ++++
 t/t3903-stash.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

builtin/stash.c +0 -0

index 51df092633..012662ce68 100644

builtin/stash.c +0 -0

 	rev.diffopt.flags.recursive = 1;
 	setup_diff_pager(&rev.diffopt);

t/t3903-stash.sh +18 -0

index 97cc71fbaf..e0a50ab267 100755
@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show -v shows diff' '
+	git reset --hard &&
+	echo foo >>file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show -v ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&

Quoted 12 lines · Thomas Gummerer

Subject: [PATCH] stash: setup default diff output format if necessary

In the scripted 'git stash show' when no arguments are passed, we just pass '--stat' to 'git diff'. When any argument is passed to 'stash show', we no longer pass '--stat' to 'git diff', and pass whatever flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when we a user uses 'git stash show -v', they would be shown the diff, because that's the default behaviour of 'git diff', but not actually directly triggered by passing the '-v'.

Quoted 4 lines

In the C version of 'git stash show', we try to emulate that behaviour using the internal diff API. However we forgot to set up the default output format, in case it wasn't set by any of the flags that were passed through.

Well explained. It might have avoided such a bug if the code did not manually stuff the diffopt.* structure fields (instead, e.g. prepare an array of strings like {"diff", "--stat", NULL} and let the option parser diff_opt_parse() to do its job), but that is lamenting over water under the bridge.

Quoted 3 lines

So 'git stash show -v' in the builtin
version of stash would be completely silent, while it would show the diff before.

That sounds reasonable. Thanks for a quick diagnosis and a fix.

Quoted 18 lines

Fix this by setting up the default output format for 'git diff'.

Reported-by Denton Liu <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---
 builtin/stash.c  |  4 ++++
 t/t3903-stash.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

builtin/stash.c +1 -0

index 51df092633..012662ce68 100644
@@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 		free_stash_info(&info);
 		usage_with_options(git_stash_show_usage, options);
 	}
+	if (!rev.diffopt.output_format) {

builtin/stash.c static int show_stash(int argc, const char **argv, const char *prefix) · line 767

Quoted 3 lines

builtin/stash.c +3 -0

+		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+		diff_setup_done(&rev.diffopt);
+	}

Hmph. Does this result in setup_done() called twice? As it would indicate another bug in the original code if setup_done() was never called, I am assuming that another setup_done() call in the same codeflow is already there.

Quoted 31 lines

builtin/stash.c +0 -0

 	rev.diffopt.flags.recursive = 1;
 	setup_diff_pager(&rev.diffopt);

t/t3903-stash.sh +18 -0

index 97cc71fbaf..e0a50ab267 100755
@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show -v shows diff' '
+	git reset --hard &&
+	echo foo >>file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show -v ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&

Subject: [PATCH] stash: setup default diff output format if necessary

In the scripted 'git stash show' when no arguments are passed, we just pass '--stat' to 'git diff'. When any argument is passed to 'stash show', we no longer pass '--stat' to 'git diff', and pass whatever flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when we a user uses 'git stash show -v', they would be shown the diff, because that's the default behaviour of 'git diff', but not actually directly triggered by passing the '-v'.

From a quick search I couldn't find where 'git diff' actually parses the '-v' argument, but I wonder if we should actually disallow it, in case we want to use it for something else in the future? It's not documented anywhere in the docs either.

Quoted 4 lines · Thomas Gummerer

From a quick search I couldn't find where 'git diff' actually parses the '-v' argument, but I wonder if we should actually disallow it, in case we want to use it for something else in the future? It's not documented anywhere in the docs either.

It's a bit interesting, actually. git-diff uses setup_revisions() to parse its arguments, which picks up any diff options, as well as parsing the revs and pathspecs.

...

- we may want to teach the "diff" porcelain not to accept useless revision options. I suspect it may be a bit tricky, just because of the way the code takes advantage of setup_revisions.

Quoted 4 lines · Thomas Gummerer

From a quick search I couldn't find where 'git diff' actually parses the '-v' argument, but I wonder if we should actually disallow it, in case we want to use it for something else in the future? It's not documented anywhere in the docs either.

It's a bit interesting, actually. git-diff uses setup_revisions() to parse its arguments, which picks up any diff options, as well as parsing the revs and pathspecs.

But it also means we accept any random revision options. So nonsense like:

  git diff --ancestry-path HEAD^ HEAD

is accepted, even though nobody ever looks at the flags set by parsing that option.

And "-v" is mostly in the same boat. It works more or less like --pretty (try rev-list with and without it), and does nothing for an endpoint diff. What's a little interesting, though, is that it was originally added as a diff-tree option in the very early days, via cee99d2257 (diff-tree: add "verbose header" mode, 2005-05-06). And the reason there is that "diff-tree --stdin" filled a "log"-like role; it didn't traverse the commits itself, but it was responsible for showing them.

Most of that is historical curiosity, but I think the takeaways are:

- we probably should use a less bizarre option to demonstrate this bug (Junio suggested --patience, which makes perfect sense to me)

- we may want to teach the "diff" porcelain not to accept useless revision options. I suspect it may be a bit tricky, just because of the way the code takes advantage of setup_revisions. It would also be nice if "diff-tree" in non-stdin mode could do the same, but I suspect that is even trickier (we do not even know whether we are in --stdin mode or not until we've fed the options to setup_revisions). So I'd guess this is not really worth the effort it would take.

- "-v" is a real thing; we should consider either documenting it or deprecating it.

In the scripted 'git stash show' when no arguments are passed, we just pass '--stat' to 'git diff'. When any argument is passed to 'stash show', we no longer pass '--stat' to 'git diff', and pass whatever flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when we a user uses

'git stash show --patience', they would be shown the diff as expected, using the patience algorithm. '--patience' in this case only changes the diff algorithm, but does not cause 'git diff' to show the diff by itself. The diff is shown because that's the default behaviour of 'git diff'.

In the C version of 'git stash show', we try to emulate that behaviour using the internal diff API. However we forgot to set up the default output format, in case it wasn't set by any of the flags that were passed through. So 'git stash show --patience' in the builtin version of stash would be completely silent, while it would show the diff in the scripted version.

The same thing would happen for other flags that only affect the way a patch is displayed, rather than switching to a different output format than the default one.

Fix this by setting up the default output format for 'git diff'.

Reported-by Denton Liu <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---

Thanks Peff and Junio for your comments on the previous round.

Compared to v1, this uses the --patience flags for the tests now, and mentions only the --patience flag in the commit message. While the original report was about -v, I do agree that --patience is more relevant here.

I think this also deserves some explanation of what didn't change, especially after what I said in [*1*]. We're still not using the 'diff_opt_parse()' option parser, as it doesn't understand '-v' for example. 'setup_revisions()' understands that, but 'diff_opt_parse()' doesn't, so we'd still have a change in behaviour at least there. After discovering that I gave up on that approach.

The other thing that was pointed out is the 'diff_setup_done()' call here. 'diff_setup_done()' is already called inside of 'setup_revisions()', so we don't need to do it again, unless we change the output format, which is what we are doing here. In fact this is the same way it's implemented in 'builtin/diff.c'.

*1*: <[email protected]>

 builtin/stash.c  |  4 ++++
 t/t3903-stash.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

builtin/stash.c +4 -0

index 51df092633..012662ce68 100644
@@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 		free_stash_info(&info);
 		usage_with_options(git_stash_show_usage, options);
 	}
+	if (!rev.diffopt.output_format) {
+		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+		diff_setup_done(&rev.diffopt);
+	}
 
 	rev.diffopt.flags.recursive = 1;
 	setup_diff_pager(&rev.diffopt);

t/t3903-stash.sh +18 -0

index 97cc71fbaf..83926ab55b 100755
@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show -v shows diff' '
+	git reset --hard &&
+	echo foo >>file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show --patience ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&

Signature · 1 line

2.21.0.226.g764ec437b0.dirty

In the scripted 'git stash show' when no arguments are passed, we just pass '--stat' to 'git diff'. When any argument is passed to 'stash show', we no longer pass '--stat' to 'git diff', and pass whatever flags are passed directly through to 'git diff'.

By default 'git diff' shows the patch output. So when we a user uses 'git stash show --patience', they would be shown the diff as expected, using the patience algorithm. '--patience' in this case only changes the diff algorithm, but does not cause 'git diff' to show the diff by itself. The diff is shown because that's the default behaviour of 'git diff'.

In the C version of 'git stash show', we try to emulate that behaviour using the internal diff API. However we forgot to set up the default output format, in case it wasn't set by any of the flags that were passed through. So 'git stash show --patience' in the builtin version of stash would be completely silent, while it would show the diff in the scripted version.

The same thing would happen for other flags that only affect the way a patch is displayed, rather than switching to a different output format than the default one.

Fix this by setting up the default output format for 'git diff'.

Reported-by Denton Liu <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---

Thanks Peff and Junio for your comments on the previous round.

Compared to v1, this uses the --patience flags for the tests now, and mentions only the --patience flag in the commit message. While the original report was about -v, I do agree that --patience is more relevant here.

I think this also deserves some explanation of what didn't change, especially after what I said in [*1*]. We're still not using the 'diff_opt_parse()' option parser, as it doesn't understand '-v' for example. 'setup_revisions()' understands that, but 'diff_opt_parse()' doesn't, so we'd still have a change in behaviour at least there. After discovering that I gave up on that approach.

The other thing that was pointed out is the 'diff_setup_done()' call here. 'diff_setup_done()' is already called inside of 'setup_revisions()', so we don't need to do it again, unless we change the output format, which is what we are doing here. In fact this is the same way it's implemented in 'builtin/diff.c'.

*1*: <[email protected]>

 builtin/stash.c  |  4 ++++
 t/t3903-stash.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

builtin/stash.c +4 -0

index 51df092633..012662ce68 100644
@@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 		free_stash_info(&info);
 		usage_with_options(git_stash_show_usage, options);
 	}
+	if (!rev.diffopt.output_format) {
+		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+		diff_setup_done(&rev.diffopt);
+	}
 
 	rev.diffopt.flags.recursive = 1;
 	setup_diff_pager(&rev.diffopt);

t/t3903-stash.sh +0 -0

index 97cc71fbaf..83926ab55b 100755

t/t3903-stash.sh +17 -0

+	git reset --hard &&
+	echo foo >>file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show --patience ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&

Signature · 1 line

2.21.0.226.g764ec437b0.dirty

Quoted 8 lines · Thomas Gummerer

t/t3903-stash.sh +0 -0

index 97cc71fbaf..83926ab55b 100755
@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 

t/t3903-stash.sh test_expect_success 'stash show -p - no stashes on stack, stash-like argument' ' · line 615

Quoted 1 line

t/t3903-stash.sh +1 -0

+test_expect_success 'stash show -v shows diff' '

s/-v/--patience/

Missed this in my last email, my bad!

Quoted 23 lines

t/t3903-stash.sh +17 -0

+	git reset --hard &&
+	echo foo >>file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	cat >expected <<-EOF &&
+	diff --git a/file b/file
+	index 7601807..71b52c4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 baz
+	+foo
+	EOF
+	git stash show --patience ${STASH_ID} >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'drop: fail early if specified stash is not a stash ref' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&

Signature · 1 line

2.21.0.226.g764ec437b0.dirty

I think this also deserves some explanation of what didn't change, especially after what I said in [*1*]. We're still not using the 'diff_opt_parse()' option parser, as it doesn't understand '-v' for example. 'setup_revisions()' understands that, but 'diff_opt_parse()' doesn't, so we'd still have a change in behaviour at least there. After discovering that I gave up on that approach.

The other thing that was pointed out is the 'diff_setup_done()' call here. 'diff_setup_done()' is already called inside of 'setup_revisions()', so we don't need to do it again, unless we change the output format, which is what we are doing here. In fact this is the same way it's implemented in 'builtin/diff.c'.

Quoted 6 lines

I think this also deserves some explanation of what didn't change, especially after what I said in [*1*]. We're still not using the 'diff_opt_parse()' option parser, as it doesn't understand '-v' for example. 'setup_revisions()' understands that, but 'diff_opt_parse()' doesn't, so we'd still have a change in behaviour at least there. After discovering that I gave up on that approach.

Yeah, I think this would get into the "why are we even looking at revision options" thing, which really is a separate topic. Let's do the minimal fix here.

There is a "set_default" callback that was added by 6c374008b1 (diff_opt: track whether flags have been set explicitly, 2013-05-10), but it looks like it was never actually used. I think the theory is that cases like this could do their tweaking in such a callback. But I think it makes sense to follow the lead of builtin/diff.c for the immediate fix, and we can look at using set_default as a separate topic.

Quoted 6 lines

There is a "set_default" callback that was added by 6c374008b1 (diff_opt: track whether flags have been set explicitly, 2013-05-10), but it looks like it was never actually used. I think the theory is that cases like this could do their tweaking in such a callback. But I think it makes sense to follow the lead of builtin/diff.c for the immediate fix, and we can look at using set_default as a separate topic.

I agree with the conclusion. I wouldn't be surprised if this is one of the things that was once used but left behind when the last caller disappeared, though.