| Message ID | 20190320224955.GE32487@hank.intra.tgummerer.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] stash: setup default diff output format if necessary | expand |
Hi Thomas, Thanks for the quick fix! On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote: > 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 s/we a/a/ Looks good otherwise (but I don't know the diff API very well either). Thanks! > '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 <liu.denton@gmail.com> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > > 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*: <20190320214504.GC32487@hank.intra.tgummerer.com> > > builtin/stash.c | 4 ++++ > t/t3903-stash.sh | 18 ++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/builtin/stash.c b/builtin/stash.c > index 51df092633..012662ce68 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -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); > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 97cc71fbaf..83926ab55b 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -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" && > -- > 2.21.0.226.g764ec437b0.dirty >
On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote: > 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 <liu.denton@gmail.com> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > > 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*: <20190320214504.GC32487@hank.intra.tgummerer.com> > > builtin/stash.c | 4 ++++ > t/t3903-stash.sh | 18 ++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/builtin/stash.c b/builtin/stash.c > index 51df092633..012662ce68 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -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); > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 97cc71fbaf..83926ab55b 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -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' ' s/-v/--patience/ Missed this in my last email, my bad! > + 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" && > -- > 2.21.0.226.g764ec437b0.dirty >
On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote: > 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. > 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'. That makes me wonder if any part of diff_setup_done() could be surprised at being called twice. I guess not, if nobody has reported a bug in git-diff. :) 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. -Peff
Jeff King <peff@peff.net> writes: >> 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. I tend to agree. Limiting ourselves to diff options would be a good direction going forward in the longer term, but let's fix the regression so that we can push the base topic to 'master' before we need to know the shape of the next release. > 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.
On Fri, Mar 22, 2019 at 12:25:17PM +0900, Junio C Hamano wrote: > > 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. I wondered, too, but `git log --pickaxe-regex -S'set_default[^_]'` seems to think it was purely aspirational. :) That calls for a little extra caution when starting to use it (because it has not really been tested), but from a quick look I cannot see how it would go wrong. -Peff
On 03/20, Denton Liu wrote: > On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote: > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > > index 97cc71fbaf..83926ab55b 100755 > > --- a/t/t3903-stash.sh > > +++ b/t/t3903-stash.sh > > @@ -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' ' > > s/-v/--patience/ > > Missed this in my last email, my bad! I see Junio already picked this and the fix for the commit message up already. Thanks both! > > + 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" && > > -- > > 2.21.0.226.g764ec437b0.dirty > >
diff --git a/builtin/stash.c b/builtin/stash.c index 51df092633..012662ce68 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -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); diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 97cc71fbaf..83926ab55b 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -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" &&
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 <liu.denton@gmail.com> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- 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*: <20190320214504.GC32487@hank.intra.tgummerer.com> builtin/stash.c | 4 ++++ t/t3903-stash.sh | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+)