{"thread":{"id":"50684","subject":"[PATCH v2] line-log: suppress diff output with \"-s\"","startedAt":"2019-03-07T19:45:18Z","lastAt":"2019-03-11T03:54:36Z","messageCount":6,"participants":["Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"370923","messageId":"20190307194514.GA29260@sigill.intra.peff.net","threadId":"50684","inReplyTo":null,"subject":"[PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-07T19:45:15Z","receivedAt":"2019-03-07T19:45:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When \"-L\" is in use, we ignore any diff output format that the user\nprovides to us, and just always print a patch (with extra context lines\ncovering the whole area of interest). It's not entirely clear what we\nshould do with all formats (e.g., should \"--stat\" show just the diffstat\nof the touched lines, or the stat for the whole file?).\n\nBut \"-s\" is pretty clear: the user probably wants to see just the\ncommits that touched those lines, without any diff at all. Let's at\nleast make that work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a repost from the thread at:\n\n  https://public-inbox.org/git/CAEkQehdFu5zM4AY3ihN0pn1aCNEomY0WV07pryfAB45JN-tDDA@mail.gmail.com/\n\n line-log.c          | 6 ++++--\n t/t4211-line-log.sh | 7 +++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 24e21731c4..59248e37cc 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1103,10 +1103,12 @@ static int process_all_files(struct line_log_data **range_out,\n \n int line_log_print(struct rev_info *rev, struct commit *commit)\n {\n-\tstruct line_log_data *range = lookup_line_range(rev, commit);\n \n \tshow_log(rev);\n-\tdump_diff_hacky(rev, range);\n+\tif (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {\n+\t\tstruct line_log_data *range = lookup_line_range(rev, commit);\n+\t\tdump_diff_hacky(rev, range);\n+\t}\n \treturn 1;\n }\n \ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex bd5fe4d148..c9f2036f68 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -115,4 +115,11 @@ test_expect_success 'range_set_union' '\n \tgit log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)\n '\n \n+test_expect_success '-s shows only line-log commits' '\n+\tgit log --format=\"commit %s\" -L1,24:b.c >expect.raw &&\n+\tgrep ^commit expect.raw >expect &&\n+\tgit log --format=\"commit %s\" -L1,24:b.c -s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.21.0.787.g929e938557\n"},{"id":"370990","messageId":"nycvar.QRO.7.76.6.1903081636350.41@tvgsbejvaqbjf.bet","threadId":"50684","inReplyTo":"20190307194514.GA29260@sigill.intra.peff.net","subject":"Re: [PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-08T15:38:44Z","receivedAt":"2019-03-08T15:39:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 7 Mar 2019, Jeff King wrote:\n\n> When \"-L\" is in use, we ignore any diff output format that the user\n> provides to us, and just always print a patch (with extra context lines\n> covering the whole area of interest). It's not entirely clear what we\n> should do with all formats (e.g., should \"--stat\" show just the diffstat\n> of the touched lines, or the stat for the whole file?).\n> \n> But \"-s\" is pretty clear: the user probably wants to see just the\n> commits that touched those lines, without any diff at all. Let's at\n> least make that work.\n\nAgree. The patch looks obviously good.\n\nThank you,\nDscho\n\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This is a repost from the thread at:\n> \n>   https://public-inbox.org/git/CAEkQehdFu5zM4AY3ihN0pn1aCNEomY0WV07pryfAB45JN-tDDA@mail.gmail.com/\n> \n>  line-log.c          | 6 ++++--\n>  t/t4211-line-log.sh | 7 +++++++\n>  2 files changed, 11 insertions(+), 2 deletions(-)\n> \n> diff --git a/line-log.c b/line-log.c\n> index 24e21731c4..59248e37cc 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -1103,10 +1103,12 @@ static int process_all_files(struct line_log_data **range_out,\n>  \n>  int line_log_print(struct rev_info *rev, struct commit *commit)\n>  {\n> -\tstruct line_log_data *range = lookup_line_range(rev, commit);\n>  \n>  \tshow_log(rev);\n> -\tdump_diff_hacky(rev, range);\n> +\tif (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {\n> +\t\tstruct line_log_data *range = lookup_line_range(rev, commit);\n> +\t\tdump_diff_hacky(rev, range);\n> +\t}\n>  \treturn 1;\n>  }\n>  \n> diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\n> index bd5fe4d148..c9f2036f68 100755\n> --- a/t/t4211-line-log.sh\n> +++ b/t/t4211-line-log.sh\n> @@ -115,4 +115,11 @@ test_expect_success 'range_set_union' '\n>  \tgit log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)\n>  '\n>  \n> +test_expect_success '-s shows only line-log commits' '\n> +\tgit log --format=\"commit %s\" -L1,24:b.c >expect.raw &&\n> +\tgrep ^commit expect.raw >expect &&\n> +\tgit log --format=\"commit %s\" -L1,24:b.c -s >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> -- \n> 2.21.0.787.g929e938557\n> \n"},{"id":"370997","messageId":"20190308162031.GA17326@sigill.intra.peff.net","threadId":"50684","inReplyTo":"nycvar.QRO.7.76.6.1903081636350.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-08T16:20:32Z","receivedAt":"2019-03-08T16:20:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:\n\n> On Thu, 7 Mar 2019, Jeff King wrote:\n> \n> > When \"-L\" is in use, we ignore any diff output format that the user\n> > provides to us, and just always print a patch (with extra context lines\n> > covering the whole area of interest). It's not entirely clear what we\n> > should do with all formats (e.g., should \"--stat\" show just the diffstat\n> > of the touched lines, or the stat for the whole file?).\n> > \n> > But \"-s\" is pretty clear: the user probably wants to see just the\n> > commits that touched those lines, without any diff at all. Let's at\n> > least make that work.\n> \n> Agree. The patch looks obviously good.\n\nThanks. This leaves the other formats as silently ignored. Do we want to\ndo something like this:\n\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..a1b4fe2aa6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (revs->first_parent_only && revs->bisect)\n \t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n \n+\tif (revs->line_level_traverse &&\n+\t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))\n+\t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\n+\n \tif (revs->expand_tabs_in_log < 0)\n \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n \n\n?\n\n-Peff\n"},{"id":"371005","messageId":"nycvar.QRO.7.76.6.1903081942340.41@tvgsbejvaqbjf.bet","threadId":"50684","inReplyTo":"20190308162031.GA17326@sigill.intra.peff.net","subject":"Re: [PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-08T18:44:22Z","receivedAt":"2019-03-08T18:44:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Mar 2019, Jeff King wrote:\n\n> On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:\n> \n> > On Thu, 7 Mar 2019, Jeff King wrote:\n> > \n> > > When \"-L\" is in use, we ignore any diff output format that the user\n> > > provides to us, and just always print a patch (with extra context lines\n> > > covering the whole area of interest). It's not entirely clear what we\n> > > should do with all formats (e.g., should \"--stat\" show just the diffstat\n> > > of the touched lines, or the stat for the whole file?).\n> > > \n> > > But \"-s\" is pretty clear: the user probably wants to see just the\n> > > commits that touched those lines, without any diff at all. Let's at\n> > > least make that work.\n> > \n> > Agree. The patch looks obviously good.\n> \n> Thanks. This leaves the other formats as silently ignored.\n\nI'd be fine with that... but...\n\n> Do we want to do something like this:\n> \n> diff --git a/revision.c b/revision.c\n> index eb8e51bc63..a1b4fe2aa6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \tif (revs->first_parent_only && revs->bisect)\n>  \t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n>  \n> +\tif (revs->line_level_traverse &&\n> +\t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))\n> +\t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\n> +\n>  \tif (revs->expand_tabs_in_log < 0)\n>  \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n\nSince you already have that patch, why not go wild and apply it, too? ;-)\n\nI guess you copy-edited the code from somewhere because you usually do\nleave spaces around the `|`... I don't care, though ;-)\n\nCiao,\nDscho\n"},{"id":"371094","messageId":"xmqqef7emb7i.fsf@gitster-ct.c.googlers.com","threadId":"50684","inReplyTo":"20190308162031.GA17326@sigill.intra.peff.net","subject":"Re: [PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T01:49:53Z","receivedAt":"2019-03-11T01:49:58Z","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> On Fri, Mar 08, 2019 at 04:38:44PM +0100, Johannes Schindelin wrote:\n>\n>> On Thu, 7 Mar 2019, Jeff King wrote:\n>> \n>> > When \"-L\" is in use, we ignore any diff output format that the user\n>> > provides to us, and just always print a patch (with extra context lines\n>> > covering the whole area of interest). It's not entirely clear what we\n>> > should do with all formats (e.g., should \"--stat\" show just the diffstat\n>> > of the touched lines, or the stat for the whole file?).\n>> > \n>> > But \"-s\" is pretty clear: the user probably wants to see just the\n>> > commits that touched those lines, without any diff at all. Let's at\n>> > least make that work.\n>> \n>> Agree. The patch looks obviously good.\n>\n> Thanks. This leaves the other formats as silently ignored. Do we want to\n> do something like this:\n\nIt probably would make sense to do this if only to avoid surprises.\n\n>\n> diff --git a/revision.c b/revision.c\n> index eb8e51bc63..a1b4fe2aa6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \tif (revs->first_parent_only && revs->bisect)\n>  \t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n>  \n> +\tif (revs->line_level_traverse &&\n> +\t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))\n> +\t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\n> +\n>  \tif (revs->expand_tabs_in_log < 0)\n>  \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n>  \n>\n> ?\n>\n> -Peff\n"},{"id":"371115","messageId":"20190311035432.GC7087@sigill.intra.peff.net","threadId":"50684","inReplyTo":"nycvar.QRO.7.76.6.1903081942340.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] line-log: suppress diff output with \"-s\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T03:54:33Z","receivedAt":"2019-03-11T03:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 08, 2019 at 07:44:22PM +0100, Johannes Schindelin wrote:\n\n> > Do we want to do something like this:\n> > \n> > diff --git a/revision.c b/revision.c\n> > index eb8e51bc63..a1b4fe2aa6 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> >  \tif (revs->first_parent_only && revs->bisect)\n> >  \t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n> >  \n> > +\tif (revs->line_level_traverse &&\n> > +\t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH|DIFF_FORMAT_NO_OUTPUT)))\n> > +\t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\n> > +\n> >  \tif (revs->expand_tabs_in_log < 0)\n> >  \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n> \n> Since you already have that patch, why not go wild and apply it, too? ;-)\n\nOK, then. Here it is as a real patch.\n\n> I guess you copy-edited the code from somewhere because you usually do\n> leave spaces around the `|`... I don't care, though ;-)\n\nHeh, nope, I wrote it by hand.\n\nI actually wondered if there was a better way to write this. One of the\nthings that bugs me about the patch is that this policy check is so far\nfrom actual line-log code, which means the two may drift apart over\ntime. We could put a LINE_LOG_SUPPORTED_FORMATS into line-log.h, but\nthat's also pretty far from the actual code. We could just make the\noutput logic in the line-log code something like:\n\n  if (NO_OUTPUT)\n\t...do no output...\n  else if (PATCH || DEFAULT)\n        ...show normal patch...\n  else\n\tdie(\"unsupported output format\");\n\nbut it's probably a little friendlier to diagnose the problem up front,\nnot after we did a bunch of computation.  And most of these other\n\"incompatible\" checks in setup_revisions() suffer from the same\nmaintainability problem. So instead, I went with the code above (with\nspaces!) and protected it with some tests.\n\n-- >8 --\nSubject: [PATCH] line-log: detect unsupported formats\n\nIf you use \"log -L\" with an output format like \"--raw\" or \"--stat\",\nwe'll silently ignore the format and just output the normal patch.\nLet's detect and complain about this, which at least tells the user\nwhat's going on.\n\nThe tests here aren't exhaustive over the set of all formats, but it\nshould at least let us know if somebody breaks the format-checking.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn top of the other patch, naturally.\n\n revision.c          |  4 ++++\n t/t4211-line-log.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..cb69a227d5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2689,6 +2689,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (revs->first_parent_only && revs->bisect)\n \t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n \n+\tif (revs->line_level_traverse &&\n+\t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))\n+\t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\n+\n \tif (revs->expand_tabs_in_log < 0)\n \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n \ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex c9f2036f68..1db7bd0f59 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -122,4 +122,14 @@ test_expect_success '-s shows only line-log commits' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '-p shows the default patch output' '\n+\tgit log -L1,24:b.c >expect &&\n+\tgit log -L1,24:b.c -p >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--raw is forbidden' '\n+\ttest_must_fail git log -L1,24:b.c --raw\n+'\n+\n test_done\n-- \n2.21.0.787.gbeacff058d\n\n"}]}