{"thread":{"id":"50781","subject":"[REGRESSION ps/stash-in-c] git stash show -v","startedAt":"2019-03-19T19:05:09Z","lastAt":"2019-03-28T20:45:52Z","messageCount":15,"participants":["Denton Liu","Thomas Gummerer","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"371929","messageId":"20190319190503.GA10066@dev-l","threadId":"50781","inReplyTo":null,"subject":"[REGRESSION ps/stash-in-c] git stash show -v","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-19T19:05:03Z","receivedAt":"2019-03-19T19:05:09Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi all,\n\nI've been using jch's branch and I discovered a regression in\nps/stash-in-c.\n\nHere's a test-case on git.git:\n\n\techo '/**/' >>abspath.c\n\tgit stash\n\tgit stash show -v\n\t# I am expecting the diff to show up here\n\nI am expecting the diff to show up but in reality, I get no output.\nHowever, if I compile the latest master, the diff does show up.\n\nNote that I can get the patch to show up using \"git stash show -p\" so\nit's not really a showstopper. However, my understanding is that this\nwas supposed to be a one-to-one (bug included!) port.\n\nPlease let me know if we're keeping this behaviour so I can retrain my\nfingers.\n\nThanks,\n\nDenton\n"},{"id":"371934","messageId":"20190319231826.GB32487@hank.intra.tgummerer.com","threadId":"50781","inReplyTo":"20190319190503.GA10066@dev-l","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-19T23:18:26Z","receivedAt":"2019-03-19T23:18:36Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/19, Denton Liu wrote:\n> Hi all,\n> \n> I've been using jch's branch and I discovered a regression in\n> ps/stash-in-c.\n> \n> Here's a test-case on git.git:\n> \n> \techo '/**/' >>abspath.c\n> \tgit stash\n> \tgit stash show -v\n> \t# I am expecting the diff to show up here\n> \n> I am expecting the diff to show up but in reality, I get no output.\n> However, if I compile the latest master, the diff does show up.\n> \n> Note that I can get the patch to show up using \"git stash show -p\" so\n> it's not really a showstopper. However, my understanding is that this\n> was supposed to be a one-to-one (bug included!) port.\n\nYes, I think this is indeed a regression, even if it worked more or\nless accidentally.  '-v' would just be passed through to 'git diff'\nwhich wouldn't do anything with it.  It would show the patch because\nthat's the default behaviour of 'git diff'.\n\nFrom a quick search I couldn't find where 'git diff' actually parses\nthe '-v' argument, but I wonder if we should actually disallow it, in\ncase we want to use it for something else in the future?  It's not\ndocumented anywhere in the docs either.\n\nAnyway a patch to fix this is below, as we should have some\ndeprecation plan if we wanted to get rid of the '-v', and not just\nsilently change the behaviour.\n\n--- >8 ---\nSubject: [PATCH] stash: setup default diff output format if necessary\n\nIn the scripted 'git stash show' when no arguments are passed, we just\npass '--stat' to 'git diff'.  When any argument is passed to 'stash\nshow', we no longer pass '--stat' to 'git diff', and pass whatever\nflags are passed directly through to 'git diff'.\n\nBy default 'git diff' shows the patch output.  So when we a user uses\n'git stash show -v', they would be shown the diff, because that's the\ndefault behaviour of 'git diff', but not actually directly triggered\nby passing the '-v'.\n\nIn the C version of 'git stash show', we try to emulate that\nbehaviour using the internal diff API.  However we forgot to set up\nthe default output format, in case it wasn't set by any of the flags\nthat were passed through.  So 'git stash show -v' in the builtin\nversion of stash would be completely silent, while it would show the\ndiff before.\n\nFix this by setting up the default output format for 'git diff'.\n\nReported-by: Denton Liu <liu.denton@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/stash.c  |  4 ++++\n t/t3903-stash.sh | 18 ++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 51df092633..012662ce68 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n \t\tfree_stash_info(&info);\n \t\tusage_with_options(git_stash_show_usage, options);\n \t}\n+\tif (!rev.diffopt.output_format) {\n+\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\t\tdiff_setup_done(&rev.diffopt);\n+\t}\n \n \trev.diffopt.flags.recursive = 1;\n \tsetup_diff_pager(&rev.diffopt);\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 97cc71fbaf..e0a50ab267 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'stash show -v shows diff' '\n+\tgit reset --hard &&\n+\techo foo >>file &&\n+\tSTASH_ID=$(git stash create) &&\n+\tgit reset --hard &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/file b/file\n+\tindex 7601807..71b52c4 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1,2 @@\n+\t baz\n+\t+foo\n+\tEOF\n+\tgit stash show -v ${STASH_ID} >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n-- \n2.21.0.157.g0e94f7aa73.dirty\n\n"},{"id":"371941","messageId":"xmqqwokuwe9w.fsf@gitster-ct.c.googlers.com","threadId":"50781","inReplyTo":"20190319231826.GB32487@hank.intra.tgummerer.com","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-20T01:00:43Z","receivedAt":"2019-03-20T01:00:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Subject: [PATCH] stash: setup default diff output format if necessary\n>\n> In the scripted 'git stash show' when no arguments are passed, we just\n> pass '--stat' to 'git diff'.  When any argument is passed to 'stash\n> show', we no longer pass '--stat' to 'git diff', and pass whatever\n> flags are passed directly through to 'git diff'.\n>\n> By default 'git diff' shows the patch output.  So when we a user uses\n> 'git stash show -v', they would be shown the diff, because that's the\n> default behaviour of 'git diff', but not actually directly triggered\n> by passing the '-v'.\n>\n> In the C version of 'git stash show', we try to emulate that\n> behaviour using the internal diff API.  However we forgot to set up\n> the default output format, in case it wasn't set by any of the flags\n> that were passed through.\n\nWell explained.  It might have avoided such a bug if the code did\nnot manually stuff the diffopt.* structure fields (instead, e.g.\nprepare an array of strings like {\"diff\", \"--stat\", NULL} and let\nthe option parser diff_opt_parse() to do its job), but that is\nlamenting over water under the bridge.\n\n> So 'git stash show -v' in the builtin\n> version of stash would be completely silent, while it would show the\n> diff before.\n\nThat sounds reasonable.  Thanks for a quick diagnosis and a fix.\n\n> Fix this by setting up the default output format for 'git diff'.\n>\n> Reported-by: Denton Liu <liu.denton@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  builtin/stash.c  |  4 ++++\n>  t/t3903-stash.sh | 18 ++++++++++++++++++\n>  2 files changed, 22 insertions(+)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 51df092633..012662ce68 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n>  \t\tfree_stash_info(&info);\n>  \t\tusage_with_options(git_stash_show_usage, options);\n>  \t}\n> +\tif (!rev.diffopt.output_format) {\n> +\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n> +\t\tdiff_setup_done(&rev.diffopt);\n> +\t}\n\nHmph.  Does this result in setup_done() called twice?  As it would\nindicate another bug in the original code if setup_done() was never\ncalled, I am assuming that another setup_done() call in the same\ncodeflow is already there.\n\n>  \trev.diffopt.flags.recursive = 1;\n>  \tsetup_diff_pager(&rev.diffopt);\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 97cc71fbaf..e0a50ab267 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'stash show -v shows diff' '\n> +\tgit reset --hard &&\n> +\techo foo >>file &&\n> +\tSTASH_ID=$(git stash create) &&\n> +\tgit reset --hard &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/file b/file\n> +\tindex 7601807..71b52c4 100644\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -1 +1,2 @@\n> +\t baz\n> +\t+foo\n> +\tEOF\n> +\tgit stash show -v ${STASH_ID} >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n>  \tgit stash clear &&\n>  \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n"},{"id":"371942","messageId":"xmqqsgviwe2s.fsf@gitster-ct.c.googlers.com","threadId":"50781","inReplyTo":"20190319231826.GB32487@hank.intra.tgummerer.com","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-20T01:04:59Z","receivedAt":"2019-03-20T01:05:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Subject: [PATCH] stash: setup default diff output format if necessary\n>\n> In the scripted 'git stash show' when no arguments are passed, we just\n> pass '--stat' to 'git diff'.  When any argument is passed to 'stash\n> show', we no longer pass '--stat' to 'git diff', and pass whatever\n> flags are passed directly through to 'git diff'.\n>\n> By default 'git diff' shows the patch output.  So when we a user uses\n> 'git stash show -v', they would be shown the diff, because that's the\n> default behaviour of 'git diff', but not actually directly triggered\n> by passing the '-v'.\n\nA more interesting use case would be not with an otherwise useless\n\"-v\" but a real option that does have impact to how \"diff\" works,\ne.g. \"--patience\".\n\n"},{"id":"371952","messageId":"20190320050449.GA6401@sigill.intra.peff.net","threadId":"50781","inReplyTo":"20190319231826.GB32487@hank.intra.tgummerer.com","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-20T05:04:50Z","receivedAt":"2019-03-20T05:04:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2019 at 11:18:26PM +0000, Thomas Gummerer wrote:\n\n> From a quick search I couldn't find where 'git diff' actually parses\n> the '-v' argument, but I wonder if we should actually disallow it, in\n> case we want to use it for something else in the future?  It's not\n> documented anywhere in the docs either.\n\nIt's a bit interesting, actually. git-diff uses setup_revisions() to\nparse its arguments, which picks up any diff options, as well as parsing\nthe revs and pathspecs.\n\nBut it also means we accept any random revision options. So nonsense\nlike:\n\n  git diff --ancestry-path HEAD^ HEAD\n\nis accepted, even though nobody ever looks at the flags set by parsing\nthat option.\n\nAnd \"-v\" is mostly in the same boat. It works more or less like --pretty\n(try rev-list with and without it), and does nothing for an endpoint\ndiff. What's a little interesting, though, is that it was originally\nadded as a diff-tree option in the very early days, via cee99d2257\n(diff-tree: add \"verbose header\" mode, 2005-05-06). And the reason there\nis that \"diff-tree --stdin\" filled a \"log\"-like role; it didn't traverse\nthe commits itself, but it was responsible for showing them.\n\nMost of that is historical curiosity, but I think the takeaways are:\n\n  - we probably should use a less bizarre option to demonstrate this bug\n    (Junio suggested --patience, which makes perfect sense to me)\n\n  - we may want to teach the \"diff\" porcelain not to accept useless\n    revision options. I suspect it may be a bit tricky, just because of\n    the way the code takes advantage of setup_revisions.  It would also\n    be nice if \"diff-tree\" in non-stdin mode could do the same, but I\n    suspect that is even trickier (we do not even know whether we are in\n    --stdin mode or not until we've fed the options to setup_revisions).\n    So I'd guess this is not really worth the effort it would take.\n\n  - \"-v\" is a real thing; we should consider either documenting it or\n    deprecating it.\n\n-Peff\n"},{"id":"371971","messageId":"CACsJy8BMdjR9cS4qDVaNXtW-U0cQt5e3au2y5PsW7eKQdd3xJA@mail.gmail.com","threadId":"50781","inReplyTo":"20190320050449.GA6401@sigill.intra.peff.net","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-20T09:30:13Z","receivedAt":"2019-03-20T09:30:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 20, 2019 at 12:06 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Mar 19, 2019 at 11:18:26PM +0000, Thomas Gummerer wrote:\n>\n> > From a quick search I couldn't find where 'git diff' actually parses\n> > the '-v' argument, but I wonder if we should actually disallow it, in\n> > case we want to use it for something else in the future?  It's not\n> > documented anywhere in the docs either.\n>\n> It's a bit interesting, actually. git-diff uses setup_revisions() to\n> parse its arguments, which picks up any diff options, as well as parsing\n> the revs and pathspecs.\n>\n> ...\n>\n>   - we may want to teach the \"diff\" porcelain not to accept useless\n>     revision options. I suspect it may be a bit tricky, just because of\n>     the way the code takes advantage of setup_revisions.\n\nThis is actually in my TODO list. Once the parseopt conversion for\nrevision.c and diff.c is done, \"git diff --help\" would be _very_ long\nbecause of this exact problem. But then once the conversion is over, I\nthink removing unused options is quite simple (by manipulating 'struct\noption[]' array before calling parse_options).\n--\nDuy\n"},{"id":"372025","messageId":"20190320214504.GC32487@hank.intra.tgummerer.com","threadId":"50781","inReplyTo":"xmqqwokuwe9w.fsf@gitster-ct.c.googlers.com","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-20T21:45:04Z","receivedAt":"2019-03-20T21:45:11Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> > Subject: [PATCH] stash: setup default diff output format if necessary\n> >\n> > In the scripted 'git stash show' when no arguments are passed, we just\n> > pass '--stat' to 'git diff'.  When any argument is passed to 'stash\n> > show', we no longer pass '--stat' to 'git diff', and pass whatever\n> > flags are passed directly through to 'git diff'.\n> >\n> > By default 'git diff' shows the patch output.  So when we a user uses\n> > 'git stash show -v', they would be shown the diff, because that's the\n> > default behaviour of 'git diff', but not actually directly triggered\n> > by passing the '-v'.\n> >\n> > In the C version of 'git stash show', we try to emulate that\n> > behaviour using the internal diff API.  However we forgot to set up\n> > the default output format, in case it wasn't set by any of the flags\n> > that were passed through.\n> \n> Well explained.  It might have avoided such a bug if the code did\n> not manually stuff the diffopt.* structure fields (instead, e.g.\n> prepare an array of strings like {\"diff\", \"--stat\", NULL} and let\n> the option parser diff_opt_parse() to do its job), but that is\n> lamenting over water under the bridge.\n\nI actually think this is still a valid point.  I'm not too familiar\nwith the diff API, which is why I missed this during my review in the\nfirst place, but I feel like using 'diff_opt_parse()' is still the\nright choice here.  Both to fix this bug and to avoid potential\nfurther ones.\n\nLet me give implementing using that a try, I have a feeling like that\nmight turn out to be better code.\n\n> > So 'git stash show -v' in the builtin\n> > version of stash would be completely silent, while it would show the\n> > diff before.\n> \n> That sounds reasonable.  Thanks for a quick diagnosis and a fix.\n> \n> > Fix this by setting up the default output format for 'git diff'.\n> >\n> > Reported-by: Denton Liu <liu.denton@gmail.com>\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> > ---\n> >  builtin/stash.c  |  4 ++++\n> >  t/t3903-stash.sh | 18 ++++++++++++++++++\n> >  2 files changed, 22 insertions(+)\n> >\n> > diff --git a/builtin/stash.c b/builtin/stash.c\n> > index 51df092633..012662ce68 100644\n> > --- a/builtin/stash.c\n> > +++ b/builtin/stash.c\n> > @@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n> >  \t\tfree_stash_info(&info);\n> >  \t\tusage_with_options(git_stash_show_usage, options);\n> >  \t}\n> > +\tif (!rev.diffopt.output_format) {\n> > +\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n> > +\t\tdiff_setup_done(&rev.diffopt);\n> > +\t}\n> \n> Hmph.  Does this result in setup_done() called twice?  As it would\n> indicate another bug in the original code if setup_done() was never\n> called, I am assuming that another setup_done() call in the same\n> codeflow is already there.\n\nNo, it is not called anywhere, will fix that as well in v2.\n\n> >  \trev.diffopt.flags.recursive = 1;\n> >  \tsetup_diff_pager(&rev.diffopt);\n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 97cc71fbaf..e0a50ab267 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n> >  \ttest_cmp expected actual\n> >  '\n> >  \n> > +test_expect_success 'stash show -v shows diff' '\n> > +\tgit reset --hard &&\n> > +\techo foo >>file &&\n> > +\tSTASH_ID=$(git stash create) &&\n> > +\tgit reset --hard &&\n> > +\tcat >expected <<-EOF &&\n> > +\tdiff --git a/file b/file\n> > +\tindex 7601807..71b52c4 100644\n> > +\t--- a/file\n> > +\t+++ b/file\n> > +\t@@ -1 +1,2 @@\n> > +\t baz\n> > +\t+foo\n> > +\tEOF\n> > +\tgit stash show -v ${STASH_ID} >actual &&\n> > +\ttest_cmp expected actual\n> > +'\n> > +\n> >  test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n> >  \tgit stash clear &&\n> >  \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n"},{"id":"372026","messageId":"20190320215916.GD32487@hank.intra.tgummerer.com","threadId":"50781","inReplyTo":"20190320050449.GA6401@sigill.intra.peff.net","subject":"Re: [REGRESSION ps/stash-in-c] git stash show -v","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-20T21:59:16Z","receivedAt":"2019-03-20T21:59:21Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Jeff King wrote:\n> On Tue, Mar 19, 2019 at 11:18:26PM +0000, Thomas Gummerer wrote:\n> \n> > From a quick search I couldn't find where 'git diff' actually parses\n> > the '-v' argument, but I wonder if we should actually disallow it, in\n> > case we want to use it for something else in the future?  It's not\n> > documented anywhere in the docs either.\n> \n> It's a bit interesting, actually. git-diff uses setup_revisions() to\n> parse its arguments, which picks up any diff options, as well as parsing\n> the revs and pathspecs.\n> \n> But it also means we accept any random revision options. So nonsense\n> like:\n> \n>   git diff --ancestry-path HEAD^ HEAD\n> \n> is accepted, even though nobody ever looks at the flags set by parsing\n> that option.\n> \n> And \"-v\" is mostly in the same boat. It works more or less like --pretty\n> (try rev-list with and without it), and does nothing for an endpoint\n> diff. What's a little interesting, though, is that it was originally\n> added as a diff-tree option in the very early days, via cee99d2257\n> (diff-tree: add \"verbose header\" mode, 2005-05-06). And the reason there\n> is that \"diff-tree --stdin\" filled a \"log\"-like role; it didn't traverse\n> the commits itself, but it was responsible for showing them.\n\nThanks for the explanation!\n\n> Most of that is historical curiosity, but I think the takeaways are:\n> \n>   - we probably should use a less bizarre option to demonstrate this bug\n>     (Junio suggested --patience, which makes perfect sense to me)\n\nYep, that should make the test a bit easier to understand.  Will do\nthat in v2.\n\n>   - we may want to teach the \"diff\" porcelain not to accept useless\n>     revision options. I suspect it may be a bit tricky, just because of\n>     the way the code takes advantage of setup_revisions.  It would also\n>     be nice if \"diff-tree\" in non-stdin mode could do the same, but I\n>     suspect that is even trickier (we do not even know whether we are in\n>     --stdin mode or not until we've fed the options to setup_revisions).\n>     So I'd guess this is not really worth the effort it would take.\n> \n>   - \"-v\" is a real thing; we should consider either documenting it or\n>     deprecating it.\n\nMakes sense, I don't have a strong opinion on which way we should go\nhere, but I'll leave that separately from the bug fix either way.\n"},{"id":"372031","messageId":"20190320224955.GE32487@hank.intra.tgummerer.com","threadId":"50781","inReplyTo":"20190319231826.GB32487@hank.intra.tgummerer.com","subject":"[PATCH v2] stash: setup default diff output format if necessary","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-20T22:49:55Z","receivedAt":"2019-03-20T22:50:01Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In the scripted 'git stash show' when no arguments are passed, we just\npass '--stat' to 'git diff'.  When any argument is passed to 'stash\nshow', we no longer pass '--stat' to 'git diff', and pass whatever\nflags are passed directly through to 'git diff'.\n\nBy default 'git diff' shows the patch output.  So when we a user uses\n'git stash show --patience', they would be shown the diff as expected,\nusing the patience algorithm.  '--patience' in this case only changes\nthe diff algorithm, but does not cause 'git diff' to show the diff by\nitself.  The diff is shown because that's the default behaviour of\n'git diff'.\n\nIn the C version of 'git stash show', we try to emulate that behaviour\nusing the internal diff API.  However we forgot to set up the default\noutput format, in case it wasn't set by any of the flags that were\npassed through.  So 'git stash show --patience' in the builtin version\nof stash would be completely silent, while it would show the diff in\nthe scripted version.\n\nThe same thing would happen for other flags that only affect the way a\npatch is displayed, rather than switching to a different output format\nthan the default one.\n\nFix this by setting up the default output format for 'git diff'.\n\nReported-by: Denton Liu <liu.denton@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nThanks Peff and Junio for your comments on the previous round.\n\nCompared to v1, this uses the --patience flags for the tests now, and\nmentions only the --patience flag in the commit message.  While the\noriginal report was about -v, I do agree that --patience is more\nrelevant here.\n\nI think this also deserves some explanation of what didn't change,\nespecially after what I said in [*1*].  We're still not using the\n'diff_opt_parse()' option parser, as it doesn't understand '-v' for\nexample.  'setup_revisions()' understands that, but 'diff_opt_parse()'\ndoesn't, so we'd still have a change in behaviour at least there.\nAfter discovering that I gave up on that approach.\n\nThe other thing that was pointed out is the 'diff_setup_done()' call\nhere.  'diff_setup_done()' is already called inside of\n'setup_revisions()', so we don't need to do it again, unless we change\nthe output format, which is what we are doing here.  In fact this is\nthe same way it's implemented in 'builtin/diff.c'.\n\n*1*: <20190320214504.GC32487@hank.intra.tgummerer.com>\n\n builtin/stash.c  |  4 ++++\n t/t3903-stash.sh | 18 ++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 51df092633..012662ce68 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n \t\tfree_stash_info(&info);\n \t\tusage_with_options(git_stash_show_usage, options);\n \t}\n+\tif (!rev.diffopt.output_format) {\n+\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\t\tdiff_setup_done(&rev.diffopt);\n+\t}\n \n \trev.diffopt.flags.recursive = 1;\n \tsetup_diff_pager(&rev.diffopt);\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 97cc71fbaf..83926ab55b 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'stash show -v shows diff' '\n+\tgit reset --hard &&\n+\techo foo >>file &&\n+\tSTASH_ID=$(git stash create) &&\n+\tgit reset --hard &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/file b/file\n+\tindex 7601807..71b52c4 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1,2 @@\n+\t baz\n+\t+foo\n+\tEOF\n+\tgit stash show --patience ${STASH_ID} >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n-- \n2.21.0.226.g764ec437b0.dirty\n\n"},{"id":"372034","messageId":"20190320230448.GA28130@dev-l","threadId":"50781","inReplyTo":"20190320224955.GE32487@hank.intra.tgummerer.com","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-20T23:04:48Z","receivedAt":"2019-03-20T23:04:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Thomas,\n\nThanks for the quick fix!\n\nOn Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote:\n> In the scripted 'git stash show' when no arguments are passed, we just\n> pass '--stat' to 'git diff'.  When any argument is passed to 'stash\n> show', we no longer pass '--stat' to 'git diff', and pass whatever\n> flags are passed directly through to 'git diff'.\n> \n> By default 'git diff' shows the patch output.  So when we a user uses\n\ns/we a/a/\n\nLooks good otherwise (but I don't know the diff API very well either).\n\nThanks!\n\n> 'git stash show --patience', they would be shown the diff as expected,\n> using the patience algorithm.  '--patience' in this case only changes\n> the diff algorithm, but does not cause 'git diff' to show the diff by\n> itself.  The diff is shown because that's the default behaviour of\n> 'git diff'.\n> \n> In the C version of 'git stash show', we try to emulate that behaviour\n> using the internal diff API.  However we forgot to set up the default\n> output format, in case it wasn't set by any of the flags that were\n> passed through.  So 'git stash show --patience' in the builtin version\n> of stash would be completely silent, while it would show the diff in\n> the scripted version.\n> \n> The same thing would happen for other flags that only affect the way a\n> patch is displayed, rather than switching to a different output format\n> than the default one.\n> \n> Fix this by setting up the default output format for 'git diff'.\n> \n> Reported-by: Denton Liu <liu.denton@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n> \n> Thanks Peff and Junio for your comments on the previous round.\n> \n> Compared to v1, this uses the --patience flags for the tests now, and\n> mentions only the --patience flag in the commit message.  While the\n> original report was about -v, I do agree that --patience is more\n> relevant here.\n> \n> I think this also deserves some explanation of what didn't change,\n> especially after what I said in [*1*].  We're still not using the\n> 'diff_opt_parse()' option parser, as it doesn't understand '-v' for\n> example.  'setup_revisions()' understands that, but 'diff_opt_parse()'\n> doesn't, so we'd still have a change in behaviour at least there.\n> After discovering that I gave up on that approach.\n> \n> The other thing that was pointed out is the 'diff_setup_done()' call\n> here.  'diff_setup_done()' is already called inside of\n> 'setup_revisions()', so we don't need to do it again, unless we change\n> the output format, which is what we are doing here.  In fact this is\n> the same way it's implemented in 'builtin/diff.c'.\n> \n> *1*: <20190320214504.GC32487@hank.intra.tgummerer.com>\n> \n>  builtin/stash.c  |  4 ++++\n>  t/t3903-stash.sh | 18 ++++++++++++++++++\n>  2 files changed, 22 insertions(+)\n> \n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 51df092633..012662ce68 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n>  \t\tfree_stash_info(&info);\n>  \t\tusage_with_options(git_stash_show_usage, options);\n>  \t}\n> +\tif (!rev.diffopt.output_format) {\n> +\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n> +\t\tdiff_setup_done(&rev.diffopt);\n> +\t}\n>  \n>  \trev.diffopt.flags.recursive = 1;\n>  \tsetup_diff_pager(&rev.diffopt);\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 97cc71fbaf..83926ab55b 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'stash show -v shows diff' '\n> +\tgit reset --hard &&\n> +\techo foo >>file &&\n> +\tSTASH_ID=$(git stash create) &&\n> +\tgit reset --hard &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/file b/file\n> +\tindex 7601807..71b52c4 100644\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -1 +1,2 @@\n> +\t baz\n> +\t+foo\n> +\tEOF\n> +\tgit stash show --patience ${STASH_ID} >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n>  \tgit stash clear &&\n>  \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n> -- \n> 2.21.0.226.g764ec437b0.dirty\n> \n"},{"id":"372037","messageId":"20190320230942.GA28455@dev-l","threadId":"50781","inReplyTo":"20190320224955.GE32487@hank.intra.tgummerer.com","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-20T23:09:42Z","receivedAt":"2019-03-20T23:09:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote:\n> In the scripted 'git stash show' when no arguments are passed, we just\n> pass '--stat' to 'git diff'.  When any argument is passed to 'stash\n> show', we no longer pass '--stat' to 'git diff', and pass whatever\n> flags are passed directly through to 'git diff'.\n> \n> By default 'git diff' shows the patch output.  So when we a user uses\n> 'git stash show --patience', they would be shown the diff as expected,\n> using the patience algorithm.  '--patience' in this case only changes\n> the diff algorithm, but does not cause 'git diff' to show the diff by\n> itself.  The diff is shown because that's the default behaviour of\n> 'git diff'.\n> \n> In the C version of 'git stash show', we try to emulate that behaviour\n> using the internal diff API.  However we forgot to set up the default\n> output format, in case it wasn't set by any of the flags that were\n> passed through.  So 'git stash show --patience' in the builtin version\n> of stash would be completely silent, while it would show the diff in\n> the scripted version.\n> \n> The same thing would happen for other flags that only affect the way a\n> patch is displayed, rather than switching to a different output format\n> than the default one.\n> \n> Fix this by setting up the default output format for 'git diff'.\n> \n> Reported-by: Denton Liu <liu.denton@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n> \n> Thanks Peff and Junio for your comments on the previous round.\n> \n> Compared to v1, this uses the --patience flags for the tests now, and\n> mentions only the --patience flag in the commit message.  While the\n> original report was about -v, I do agree that --patience is more\n> relevant here.\n> \n> I think this also deserves some explanation of what didn't change,\n> especially after what I said in [*1*].  We're still not using the\n> 'diff_opt_parse()' option parser, as it doesn't understand '-v' for\n> example.  'setup_revisions()' understands that, but 'diff_opt_parse()'\n> doesn't, so we'd still have a change in behaviour at least there.\n> After discovering that I gave up on that approach.\n> \n> The other thing that was pointed out is the 'diff_setup_done()' call\n> here.  'diff_setup_done()' is already called inside of\n> 'setup_revisions()', so we don't need to do it again, unless we change\n> the output format, which is what we are doing here.  In fact this is\n> the same way it's implemented in 'builtin/diff.c'.\n> \n> *1*: <20190320214504.GC32487@hank.intra.tgummerer.com>\n> \n>  builtin/stash.c  |  4 ++++\n>  t/t3903-stash.sh | 18 ++++++++++++++++++\n>  2 files changed, 22 insertions(+)\n> \n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 51df092633..012662ce68 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -761,6 +761,10 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n>  \t\tfree_stash_info(&info);\n>  \t\tusage_with_options(git_stash_show_usage, options);\n>  \t}\n> +\tif (!rev.diffopt.output_format) {\n> +\t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n> +\t\tdiff_setup_done(&rev.diffopt);\n> +\t}\n>  \n>  \trev.diffopt.flags.recursive = 1;\n>  \tsetup_diff_pager(&rev.diffopt);\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 97cc71fbaf..83926ab55b 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'stash show -v shows diff' '\n\ns/-v/--patience/\n\nMissed this in my last email, my bad!\n\n> +\tgit reset --hard &&\n> +\techo foo >>file &&\n> +\tSTASH_ID=$(git stash create) &&\n> +\tgit reset --hard &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/file b/file\n> +\tindex 7601807..71b52c4 100644\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -1 +1,2 @@\n> +\t baz\n> +\t+foo\n> +\tEOF\n> +\tgit stash show --patience ${STASH_ID} >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n>  \tgit stash clear &&\n>  \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n> -- \n> 2.21.0.226.g764ec437b0.dirty\n> \n"},{"id":"372086","messageId":"20190321095103.GB2894@sigill.intra.peff.net","threadId":"50781","inReplyTo":"20190320224955.GE32487@hank.intra.tgummerer.com","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-21T09:51:03Z","receivedAt":"2019-03-21T09:51:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote:\n\n> I think this also deserves some explanation of what didn't change,\n> especially after what I said in [*1*].  We're still not using the\n> 'diff_opt_parse()' option parser, as it doesn't understand '-v' for\n> example.  'setup_revisions()' understands that, but 'diff_opt_parse()'\n> doesn't, so we'd still have a change in behaviour at least there.\n> After discovering that I gave up on that approach.\n\nYeah, I think this would get into the \"why are we even looking at\nrevision options\" thing, which really is a separate topic. Let's do the\nminimal fix here.\n\n> The other thing that was pointed out is the 'diff_setup_done()' call\n> here.  'diff_setup_done()' is already called inside of\n> 'setup_revisions()', so we don't need to do it again, unless we change\n> the output format, which is what we are doing here.  In fact this is\n> the same way it's implemented in 'builtin/diff.c'.\n\nThat makes me wonder if any part of diff_setup_done() could be surprised\nat being called twice. I guess not, if nobody has reported a bug in\ngit-diff. :)\n\nThere is a \"set_default\" callback that was added by 6c374008b1\n(diff_opt: track whether flags have been set explicitly, 2013-05-10),\nbut it looks like it was never actually used. I think the theory is that\ncases like this could do their tweaking in such a callback. But I think\nit makes sense to follow the lead of builtin/diff.c for the immediate\nfix, and we can look at using set_default as a separate topic.\n\n-Peff\n"},{"id":"372195","messageId":"xmqqtvfvr3oi.fsf@gitster-ct.c.googlers.com","threadId":"50781","inReplyTo":"20190321095103.GB2894@sigill.intra.peff.net","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-22T03:25:17Z","receivedAt":"2019-03-22T03:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I think this also deserves some explanation of what didn't change,\n>> especially after what I said in [*1*].  We're still not using the\n>> 'diff_opt_parse()' option parser, as it doesn't understand '-v' for\n>> example.  'setup_revisions()' understands that, but 'diff_opt_parse()'\n>> doesn't, so we'd still have a change in behaviour at least there.\n>> After discovering that I gave up on that approach.\n>\n> Yeah, I think this would get into the \"why are we even looking at\n> revision options\" thing, which really is a separate topic. Let's do the\n> minimal fix here.\n\nI tend to agree.  Limiting ourselves to diff options would be a good\ndirection going forward in the longer term, but let's fix the regression\nso that we can push the base topic to 'master' before we need to\nknow the shape of the next release.\n\n> There is a \"set_default\" callback that was added by 6c374008b1\n> (diff_opt: track whether flags have been set explicitly, 2013-05-10),\n> but it looks like it was never actually used. I think the theory is that\n> cases like this could do their tweaking in such a callback. But I think\n> it makes sense to follow the lead of builtin/diff.c for the immediate\n> fix, and we can look at using set_default as a separate topic.\n\nI agree with the conclusion.  I wouldn't be surprised if this is one\nof the things that was once used but left behind when the last\ncaller disappeared, though.\n\n\n"},{"id":"372198","messageId":"20190322034837.GA20091@sigill.intra.peff.net","threadId":"50781","inReplyTo":"xmqqtvfvr3oi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-22T03:48:37Z","receivedAt":"2019-03-22T03:48:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 22, 2019 at 12:25:17PM +0900, Junio C Hamano wrote:\n\n> > There is a \"set_default\" callback that was added by 6c374008b1\n> > (diff_opt: track whether flags have been set explicitly, 2013-05-10),\n> > but it looks like it was never actually used. I think the theory is that\n> > cases like this could do their tweaking in such a callback. But I think\n> > it makes sense to follow the lead of builtin/diff.c for the immediate\n> > fix, and we can look at using set_default as a separate topic.\n> \n> I agree with the conclusion.  I wouldn't be surprised if this is one\n> of the things that was once used but left behind when the last\n> caller disappeared, though.\n\nI wondered, too, but `git log --pickaxe-regex -S'set_default[^_]'` seems\nto think it was purely aspirational. :)\n\nThat calls for a little extra caution when starting to use it (because\nit has not really been tested), but from a quick look I cannot see how\nit would go wrong.\n\n-Peff\n"},{"id":"372646","messageId":"20190328204546.GG32487@hank.intra.tgummerer.com","threadId":"50781","inReplyTo":"20190320230942.GA28455@dev-l","subject":"Re: [PATCH v2] stash: setup default diff output format if necessary","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-28T20:45:46Z","receivedAt":"2019-03-28T20:45:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Denton Liu wrote:\n> On Wed, Mar 20, 2019 at 10:49:55PM +0000, Thomas Gummerer wrote:\n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 97cc71fbaf..83926ab55b 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -612,6 +612,24 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n> >  \ttest_cmp expected actual\n> >  '\n> >  \n> > +test_expect_success 'stash show -v shows diff' '\n> \n> s/-v/--patience/\n> \n> Missed this in my last email, my bad!\n\nI see Junio already picked this and the fix for the commit message up\nalready.  Thanks both!\n\n> > +\tgit reset --hard &&\n> > +\techo foo >>file &&\n> > +\tSTASH_ID=$(git stash create) &&\n> > +\tgit reset --hard &&\n> > +\tcat >expected <<-EOF &&\n> > +\tdiff --git a/file b/file\n> > +\tindex 7601807..71b52c4 100644\n> > +\t--- a/file\n> > +\t+++ b/file\n> > +\t@@ -1 +1,2 @@\n> > +\t baz\n> > +\t+foo\n> > +\tEOF\n> > +\tgit stash show --patience ${STASH_ID} >actual &&\n> > +\ttest_cmp expected actual\n> > +'\n> > +\n> >  test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n> >  \tgit stash clear &&\n> >  \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n> > -- \n> > 2.21.0.226.g764ec437b0.dirty\n> > \n"}]}