{"thread":{"id":"49762","subject":"[PATCH] range-diff: add a --no-patch option to show a summary","startedAt":"2018-11-05T20:07:04Z","lastAt":"2018-11-14T15:37:10Z","messageCount":40,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Johannes Schindelin","Stephen & Linda Smith","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362525","messageId":"20181105200650.31177-1-avarab@gmail.com","threadId":"49762","inReplyTo":null,"subject":"[PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-05T20:06:50Z","receivedAt":"2018-11-05T20:07:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --no-patch option which shows which changes got removed, added\nor moved etc., without showing the diff associated with them.\n\nThis allows for using range-diff as a poor man's \"shortlog\" for\nforce-pushed branches to see what changed without getting into the\ndetails of what specifically. E.g. diffing the latest forced-push to\n\"pu\" gives us:\n\n $ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10\n -: ---------- > 1: b613de67c4 diff: differentiate error handling in parse_color_moved_ws\n 28: c731affab0 ! 2: 23c4bbe28e build: link with curl-defined linker flags\n -: ---------- > 3: 14f74d5907 git-worktree.txt: correct linkgit command name\n -: ---------- > 4: 29d51e214c sequencer.c: remove a stray semicolon\n -: ---------- > 5: b7845cebc0 tree-walk.c: fix overoptimistic inclusion in :(exclude) matching\n -: ---------- > 6: 1a550529b1 t/t7510-signed-commit.sh: Add %GP to custom format checks\n -: ---------- > 7: 1e690847d1 t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key\n 9: d13ecb7d81 ! 8: d8ad847421 Add a base implementation of SHA-256 support\n 10: 3f0382eef8 = 9: cdae1d391c sha256: add an SHA-256 implementation using libgcrypt\n 11: 2422fd4227 = 10: 7d81aa0857 hash: add an SHA-256 implementation using OpenSSL\n\nThat would print a total of 44 lines of output, but the full\nrange-diff output with --patch is 460 lines.\n\nI thought of implementing --stat too. It would be neat if passing\nDIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying\nimplementation details of how range-diff works, instead of a useful\ndiffstat. So I'll leave that to a future change. Such a feature should\nbe something like a textual summary of the --patch output itself,\ne.g.:\n\n N hunks, X insertions(+), Y deletions(-)\n\nThis change doesn't update git-format-patch with a --no-patch\noption. That can be added later similar to how format-patch first\nlearned --range-diff, and then --creation-factor in\n8631bf1cdd (\"format-patch: add --creation-factor tweak for\n--range-diff\", 2018-07-22). I don't see why anyone would want this for\nformat-patch, it pretty much defeats the point of range-diff.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-range-diff.txt | 4 +++\n builtin/log.c | 2 +-\n builtin/range-diff.c | 5 +++-\n log-tree.c | 2 +-\n range-diff.c | 6 ++++-\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 45 ++++++++++++++++++++++++++++++++\n 7 files changed, 61 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex f693930fdb..d0473cbcb1 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff markers\n \tSee the ``Algorithm`` section below for an explanation why this is\n \tneeded.\n \n+--no-patch::\n+\tDon't show the range-diff itself, only which patches are the\n+\tsame or were added or removed etc.\n+\n ::\n \tCompare the commits specified by the two ranges, where\n \t`` is considered an older version of ``.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd864..e063bcf2dd 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (rev->rdiff1) {\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2,\n-\t\t\t\trev->creation_factor, 1, &rev->diffopt);\n+\t\t\t\trev->creation_factor, 1, 1, &rev->diffopt);\n \t}\n }\n \ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex f01a0be851..70c2761751 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tint creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n \tstruct diff_options diffopt = { NULL };\n \tint simple_color = -1;\n+\tint patch = 1;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"creation-factor\", &creation_factor,\n \t\t\t N_(\"Percentage by which creation is weighted\")),\n \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n \t\t\t N_(\"use simple diff colors\")),\n+\t\tOPT_BOOL('p', \"patch\", &patch,\n+\t\t\t N_(\"show patch output\")),\n \t\tOPT_END()\n \t};\n \tint i, j, res = 0;\n@@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t}\n \n \tres = show_range_diff(range1.buf, range2.buf, creation_factor,\n-\t\t\t simple_color < 1, &diffopt);\n+\t\t\t simple_color < 1, patch, &diffopt);\n \n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\ndiff --git a/log-tree.c b/log-tree.c\nindex 7a83e99250..843e3ef83b 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)\n \t\tnext_commentary_block(opt, NULL);\n \t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n \t\tshow_range_diff(opt->rdiff1, opt->rdiff2,\n-\t\t\t\topt->creation_factor, 1, &opt->diffopt);\n+\t\t\t\topt->creation_factor, 1, 1, &opt->diffopt);\n \n \t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n \t}\ndiff --git a/range-diff.c b/range-diff.c\nindex bd8083f2d1..c1bfa593ce 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -436,6 +436,7 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)\n \n int show_range_diff(const char *range1, const char *range2,\n \t\t int creation_factor, int dual_color,\n+\t\t int patch,\n \t\t struct diff_options *diffopt)\n {\n \tint res = 0;\n@@ -453,7 +454,10 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\tif (patch)\n+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\telse\n+\t\t\topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/range-diff.h b/range-diff.h\nindex 190593f0c7..99bbc1cd9f 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -7,6 +7,7 @@\n \n int show_range_diff(const char *range1, const char *range2,\n \t\t int creation_factor, int dual_color,\n+\t\t int patch,\n \t\t struct diff_options *diffopt);\n \n #endif\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6aae364171..8f8be0c57f 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,17 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit --no-patch' '\n+\tgit range-diff --no-color --no-patch topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed commit with sm config' '\n \tgit range-diff --no-color --submodule=log topic...changed >actual &&\n \tcat >expected <<-EOF &&\n@@ -151,6 +162,17 @@ test_expect_success 'changed commit with sm config' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with sm config --no-patch' '\n+\tgit range-diff --no-color --no-patch --submodule=log topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'no commits on one side' '\n \tgit commit --amend -m \"new message\" &&\n \tgit range-diff master HEAD@{1} HEAD\n@@ -176,6 +198,17 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed message --no-patch' '\n+\tgit range-diff --no-color --no-patch topic...changed-message >actual &&\n+\tsed s/Z/\\ /g >expected <<-EOF &&\n+\t1: 4de457d = 1: f686024 s/5/A/\n+\t2: fccce22 ! 2: 4ab067d s/4/A/\n+\t3: 147e64e = 3: b9cb956 s/11/B/\n+\t4: a63e992 = 4: 8add5f1 s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'dual-coloring' '\n \tsed -e \"s|^:||\" >expect <<-\\EOF &&\n \t:1: a4b3333 = 1: f686024 s/5/A/\n@@ -215,6 +248,18 @@ test_expect_success 'dual-coloring' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'dual-coloring --no-patch' '\n+\tsed -e \"s|^:||\" >expect <<-\\EOF &&\n+\t:1: a4b3333 = 1: f686024 s/5/A/\n+\t:2: f51d370 ! 2: 4ab067d s/4/A/\n+\t:3: 0559556 ! 3: b9cb956 s/11/B/\n+\t:4: d966c5c ! 4: 8add5f1 s/12/B/\n+\tEOF\n+\tgit range-diff changed...changed-message --color --dual-color --no-patch >actual.raw &&\n+\ttest_decode_color >actual wrote:\n> Add a --no-patch option which shows which changes got removed, added\n> or moved etc., without showing the diff associated with them.\n\nThis option existed in the very first version[1] of range-diff (then\ncalled branch-diff) implemented by Dscho, although it was called\n--no-patches (with an \"es\"), which it inherited from tbdiff. I think\nsomeone (possibly me) pointed out that --no-patch (sans \"es\") would be\nmore consistent with existing Git options. I don't recall why Dscho\nremoved the option during the re-rolls, but the explanation may be in\nthat thread.\n\nI was also wondering if --summarize or --summary-only might be a\nbetter name, describing the behavior at a higher level, but since\nthere is precedent for --no-patch (or --no-patches in tbdiff), perhaps\nthe name is fine as is.\n\nThe patch itself looks okay.\n\n[1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/\n"},{"id":"362532","messageId":"87efbz6xys.fsf@evledraar.gmail.com","threadId":"49762","inReplyTo":"CAPig+cThS8959jW9+X7bJHy5RG9Uoj4=V8ahjf2zGetTNw03SA@mail.gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-05T21:00:59Z","receivedAt":"2018-11-05T21:01:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 05 2018, Eric Sunshine wrote:\n\n> On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n>> Add a --no-patch option which shows which changes got removed, added\n>> or moved etc., without showing the diff associated with them.\n>\n> This option existed in the very first version[1] of range-diff (then\n> called branch-diff) implemented by Dscho, although it was called\n> --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> more consistent with existing Git options. I don't recall why Dscho\n> removed the option during the re-rolls, but the explanation may be in\n> that thread.\n\nThanks for digging. Big thread, not going to re-read it now. I'd just\nlike to have this.\n\n> I was also wondering if --summarize or --summary-only might be a\n> better name, describing the behavior at a higher level, but since\n> there is precedent for --no-patch (or --no-patches in tbdiff), perhaps\n> the name is fine as is.\n\nI think we should aim to keep a 1=1 mapping between range-diff and\nlog/show options when possible, even though the output might have a\nslightly different flavor as my 4th paragraph discussing a potential\n--stat talks about.\n\nE.g. I can imagine that range-diff --no-patch --stat --summary would not\nshow the patch, but a stat as described there, plus e.g. a \"create\nmode...\" if applicable.\n\nThis change implements only a tiny fraction of that, but it would be\nvery neat if we supported more stuff, and showed it in range-diff-y way,\ne.g. some compact format showing:\n\n 1 file changed, 3->2 insertions(+), 10->9 deletions(-)\n create mode 100(6 -> 7)44 new-executable\n\n> The patch itself looks okay.\n>\n> [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/\n"},{"id":"362554","messageId":"xmqqva5aet72.fsf@gitster-ct.c.googlers.com","threadId":"49762","inReplyTo":"20181105200650.31177-1-avarab@gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-06T04:16:49Z","receivedAt":"2018-11-06T04:16:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This change doesn't update git-format-patch with a --no-patch\n> option. That can be added later similar to how format-patch first\n> learned --range-diff, and then --creation-factor in\n> 8631bf1cdd (\"format-patch: add --creation-factor tweak for\n> --range-diff\", 2018-07-22). I don't see why anyone would want this for\n> format-patch, it pretty much defeats the point of range-diff.\n\nI am OK not to have this option integrated to format-patch from day\none, but I do not think it is a good idea to hint that it should not\nbe done later.\n\nDoes it defeats the point of range-diff to omit the patch part in\nthe context of the cover letter? How?\n\nI think the output with this option is a good addition to the cover\nletter as an abbreviated form (as opposed to the full range-diff,\nwhose support was added earlier) that gives an overview.\n\nCalling this --[no-]patch might make it harder to integrate it to\nformat-patch later, though. I suspect that people would expect\n\"format-patch --no-patch ...\" to omit both the patch part of the\nrange-diff output *AND* the patch that should be applied to the\ncodebase (it of course would defeat the point of format-patch, so\ntoday's format-patch would not pay attention to --no-patch, of\ncourse). We need to be careful not to break that when it happens.\n\n"},{"id":"362557","messageId":"CAPig+cR85-7wMYCGGFoRT3jSQzQmda_84Ox1kF6roa5j-1XZ0Q@mail.gmail.com","threadId":"49762","inReplyTo":"xmqqva5aet72.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-06T05:15:40Z","receivedAt":"2018-11-06T05:16:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> > This change doesn't update git-format-patch with a --no-patch\n> > option. That can be added later similar to how format-patch first\n> > learned --range-diff, and then --creation-factor in\n> > 8631bf1cdd (\"format-patch: add --creation-factor tweak for\n> > --range-diff\", 2018-07-22). I don't see why anyone would want this for\n> > format-patch, it pretty much defeats the point of range-diff.\n>\n> Does it defeats the point of range-diff to omit the patch part in\n> the context of the cover letter? How?\n>\n> I think the output with this option is a good addition to the cover\n> letter as an abbreviated form (as opposed to the full range-diff,\n> whose support was added earlier) that gives an overview.\n\nI had the same response when reading the commit message but didn't\nvocalize it. I could see people wanting to suppress the 'patch' part\nof the embedded range-diff in a cover letter (though probably not as\ncommentary in a single-patch).\n\n> Calling this --[no-]patch might make it harder to integrate it to\n> format-patch later, though. I suspect that people would expect\n> \"format-patch --no-patch ...\" to omit both the patch part of the\n> range-diff output *AND* the patch that should be applied to the\n> codebase (it of course would defeat the point of format-patch, so\n> today's format-patch would not pay attention to --no-patch, of\n> course). We need to be careful not to break that when it happens.\n\nSame concern on my side, which is why I was thinking of other, less\nconfusing, names, such as --summarize or such, though even that is too\ngeneral against the full set of git-format-patch options. It could,\nperhaps be a separate option, say, \"git format-patch\n--range-changes=\" or something, which would embed the equivalent\nof \"git range-diff --no-patch ...\" in the cover letter.\n"},{"id":"362558","messageId":"xmqqin1aeojr.fsf@gitster-ct.c.googlers.com","threadId":"49762","inReplyTo":"CAPig+cR85-7wMYCGGFoRT3jSQzQmda_84Ox1kF6roa5j-1XZ0Q@mail.gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-06T05:57:12Z","receivedAt":"2018-11-06T05:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Calling this --[no-]patch might make it harder to integrate it to\n>> format-patch later, though. I suspect that people would expect\n>> \"format-patch --no-patch ...\" to omit both the patch part of the\n>> range-diff output *AND* the patch that should be applied to the\n>> codebase (it of course would defeat the point of format-patch, so\n>> today's format-patch would not pay attention to --no-patch, of\n>> course). We need to be careful not to break that when it happens.\n>\n> Same concern on my side, which is why I was thinking of other, less\n> confusing, names, such as --summarize or such, though even that is too\n> general against the full set of git-format-patch options. It could,\n> perhaps be a separate option, say, \"git format-patch\n> --range-changes=\" or something, which would embed the equivalent\n> of \"git range-diff --no-patch ...\" in the cover letter.\n\nI actually am perfectly fine with --no-patch. My \"concern\" was\nmerely that we should be careful to make sure that we do not stop\nproducing patches when we plug this patch to format-patch when\n\"format-patch --no-patch\" is given; rather, it should only suppress\nthe patch part of range-diff shown in the cover letter.\n"},{"id":"362564","messageId":"87d0ri7gbs.fsf@evledraar.gmail.com","threadId":"49762","inReplyTo":"CAPig+cR85-7wMYCGGFoRT3jSQzQmda_84Ox1kF6roa5j-1XZ0Q@mail.gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T08:36:39Z","receivedAt":"2018-11-06T08:36:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 06 2018, Eric Sunshine wrote:\n\n> On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>> > This change doesn't update git-format-patch with a --no-patch\n>> > option. That can be added later similar to how format-patch first\n>> > learned --range-diff, and then --creation-factor in\n>> > 8631bf1cdd (\"format-patch: add --creation-factor tweak for\n>> > --range-diff\", 2018-07-22). I don't see why anyone would want this for\n>> > format-patch, it pretty much defeats the point of range-diff.\n>>\n>> Does it defeats the point of range-diff to omit the patch part in\n>> the context of the cover letter? How?\n>>\n>> I think the output with this option is a good addition to the cover\n>> letter as an abbreviated form (as opposed to the full range-diff,\n>> whose support was added earlier) that gives an overview.\n>\n> I had the same response when reading the commit message but didn't\n> vocalize it. I could see people wanting to suppress the 'patch' part\n> of the embedded range-diff in a cover letter (though probably not as\n> commentary in a single-patch).\n>\n>> Calling this --[no-]patch might make it harder to integrate it to\n>> format-patch later, though. I suspect that people would expect\n>> \"format-patch --no-patch ...\" to omit both the patch part of the\n>> range-diff output *AND* the patch that should be applied to the\n>> codebase (it of course would defeat the point of format-patch, so\n>> today's format-patch would not pay attention to --no-patch, of\n>> course). We need to be careful not to break that when it happens.\n>\n> Same concern on my side, which is why I was thinking of other, less\n> confusing, names, such as --summarize or such, though even that is too\n> general against the full set of git-format-patch options. It could,\n> perhaps be a separate option, say, \"git format-patch\n> --range-changes=\" or something, which would embed the equivalent\n> of \"git range-diff --no-patch ...\" in the cover letter.\n\nMaybe this was discussed more when this range-diff format-patch\nintegration was submitted, I wasn't following that closely:\n\nLooking at this more carefully it seems like quite a design limitation\nthat we're conflating the options for format-patch itself and for the\nrange-diff invocation it makes.\n\nWouldn't it be better to make all these options\ne.g. --range-diff-creation-factor=*, --range-diff-no-patch,\n--range-diff-U1 etc. Now there's no way to say supply a different\n-U for the range-diff & the patches themselves which seems like a\nsemi-common use-case.\n\nDoing that seems to be a matter of teaching setup_revisions() to\naccumulate unknown options, then parsing those into their own diffopts\nwith the \"range-diff-\" prefix stripped and handing that data off to the\nrange-diff machinery, not the parsed options for range-diff itself as\nhappens now.\n"},{"id":"362568","messageId":"nycvar.QRO.7.76.6.1811061140560.45@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"87efbz6xys.fsf@evledraar.gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-06T10:43:36Z","receivedAt":"2018-11-06T10:43:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Nov 05 2018, Eric Sunshine wrote:\n>\n> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n> >> Add a --no-patch option which shows which changes got removed, added\n> >> or moved etc., without showing the diff associated with them.\n> >\n> > This option existed in the very first version[1] of range-diff (then\n> > called branch-diff) implemented by Dscho, although it was called\n> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> > more consistent with existing Git options. I don't recall why Dscho\n> > removed the option during the re-rolls, but the explanation may be in\n> > that thread.\n>\n> Thanks for digging. Big thread, not going to re-read it now. I'd just\n> like to have this.\n\nIn my hands, the well-documented `-s` option works (see e.g.\nhttps://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\nthat the `git-range-diff` manual does not talk about the diff-options.\n\nAnd for the record, for me, `git range-diff A...B --no-patch` *already*\nworks.\n\nCiao,\nDscho\n\n>\n> > I was also wondering if --summarize or --summary-only might be a\n> > better name, describing the behavior at a higher level, but since\n> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps\n> > the name is fine as is.\n>\n> I think we should aim to keep a 1=1 mapping between range-diff and\n> log/show options when possible, even though the output might have a\n> slightly different flavor as my 4th paragraph discussing a potential\n> --stat talks about.\n>\n> E.g. I can imagine that range-diff --no-patch --stat --summary would not\n> show the patch, but a stat as described there, plus e.g. a \"create\n> mode...\" if applicable.\n>\n> This change implements only a tiny fraction of that, but it would be\n> very neat if we supported more stuff, and showed it in range-diff-y way,\n> e.g. some compact format showing:\n>\n> 1 file changed, 3->2 insertions(+), 10->9 deletions(-)\n> create mode 100(6 -> 7)44 new-executable\n>\n> > The patch itself looks okay.\n> >\n> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/\n>\n"},{"id":"362582","messageId":"874lcu6vr4.fsf@evledraar.gmail.com","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811061140560.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T16:01:03Z","receivedAt":"2018-11-06T16:01:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 06 2018, Johannes Schindelin wrote:\n\n> Hi,\n>\n> On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Mon, Nov 05 2018, Eric Sunshine wrote:\n>>\n>> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n>> >> Add a --no-patch option which shows which changes got removed, added\n>> >> or moved etc., without showing the diff associated with them.\n>> >\n>> > This option existed in the very first version[1] of range-diff (then\n>> > called branch-diff) implemented by Dscho, although it was called\n>> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n>> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n>> > more consistent with existing Git options. I don't recall why Dscho\n>> > removed the option during the re-rolls, but the explanation may be in\n>> > that thread.\n>>\n>> Thanks for digging. Big thread, not going to re-read it now. I'd just\n>> like to have this.\n>\n> In my hands, the well-documented `-s` option works (see e.g.\n> https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\n> that the `git-range-diff` manual does not talk about the diff-options.\n>\n> And for the record, for me, `git range-diff A...B --no-patch` *already*\n> works.\n\nNeither of those works for me without my patch. E.g.\n\n ./git-range-diff -s 711aaa392f...a5ba8f2101\n ./git-range-diff --no-patch 711aaa392f...a5ba8f2101\n\nThis is on current next, 2.19.1.1182.g4ecb1133ce. What version are you\non?\n\n>>\n>> > I was also wondering if --summarize or --summary-only might be a\n>> > better name, describing the behavior at a higher level, but since\n>> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps\n>> > the name is fine as is.\n>>\n>> I think we should aim to keep a 1=1 mapping between range-diff and\n>> log/show options when possible, even though the output might have a\n>> slightly different flavor as my 4th paragraph discussing a potential\n>> --stat talks about.\n>>\n>> E.g. I can imagine that range-diff --no-patch --stat --summary would not\n>> show the patch, but a stat as described there, plus e.g. a \"create\n>> mode...\" if applicable.\n>>\n>> This change implements only a tiny fraction of that, but it would be\n>> very neat if we supported more stuff, and showed it in range-diff-y way,\n>> e.g. some compact format showing:\n>>\n>> 1 file changed, 3->2 insertions(+), 10->9 deletions(-)\n>> create mode 100(6 -> 7)44 new-executable\n>>\n>> > The patch itself looks okay.\n>> >\n>> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/\n>>\n"},{"id":"362588","messageId":"20181106162413.9785-1-avarab@gmail.com","threadId":"49762","inReplyTo":"20181105200650.31177-1-avarab@gmail.com","subject":"[PATCH v2] range-diff: add a --no-patch option to show a summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-06T16:24:13Z","receivedAt":"2018-11-06T16:24:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a --no-patch option which shows which changes got removed, added\nor moved etc., without showing the diff associated with them.\n\nThis allows for using range-diff as a poor man's \"shortlog\" for\nforce-pushed branches to see what changed without getting into the\ndetails of what specifically. E.g. diffing the latest forced-push to\n\"pu\" gives us:\n\n $ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10\n -: ---------- > 1: b613de67c4 diff: differentiate error handling in parse_color_moved_ws\n 28: c731affab0 ! 2: 23c4bbe28e build: link with curl-defined linker flags\n -: ---------- > 3: 14f74d5907 git-worktree.txt: correct linkgit command name\n -: ---------- > 4: 29d51e214c sequencer.c: remove a stray semicolon\n -: ---------- > 5: b7845cebc0 tree-walk.c: fix overoptimistic inclusion in :(exclude) matching\n -: ---------- > 6: 1a550529b1 t/t7510-signed-commit.sh: Add %GP to custom format checks\n -: ---------- > 7: 1e690847d1 t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key\n 9: d13ecb7d81 ! 8: d8ad847421 Add a base implementation of SHA-256 support\n 10: 3f0382eef8 = 9: cdae1d391c sha256: add an SHA-256 implementation using libgcrypt\n 11: 2422fd4227 = 10: 7d81aa0857 hash: add an SHA-256 implementation using OpenSSL\n\nThat would print a total of 44 lines of output, but the full\nrange-diff output with --patch is 460 lines.\n\nI thought of implementing --stat too. It would be neat if passing\nDIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying\nimplementation details of how range-diff works, instead of a useful\ndiffstat. So I'll leave that to a future change. Such a feature should\nbe something like a textual summary of the --patch output itself,\ne.g.:\n\n N hunks, X insertions(+), Y deletions(-)\n\nThis change doesn't update git-format-patch with a --no-patch\noption. That can be added later similar to how format-patch first\nlearned --range-diff, and then --creation-factor in\n8631bf1cdd (\"format-patch: add --creation-factor tweak for\n--range-diff\", 2018-07-22). See [1] and related E-Mails for a\ndiscussion about that.\n\n1. https://public-inbox.org/git/87d0ri7gbs.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\nRange-diff:\n1: 6e735e991c ! 1: fe4e251f26 range-diff: add a --no-patch option to show a summary\n @@ -38,8 +38,10 @@\n option. That can be added later similar to how format-patch first\n learned --range-diff, and then --creation-factor in\n 8631bf1cdd (\"format-patch: add --creation-factor tweak for\n - --range-diff\", 2018-07-22). I don't see why anyone would want this for\n - format-patch, it pretty much defeats the point of range-diff.\n + --range-diff\", 2018-07-22). See [1] and related E-Mails for a\n + discussion about that.\n +\n + 1. https://public-inbox.org/git/87d0ri7gbs.fsf@evledraar.gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ -50,9 +52,10 @@\n \tSee the ``Algorithm`` section below for an explanation why this is\n \tneeded.\n \n ++-s::\n +--no-patch::\n -+\tDon't show the range-diff itself, only which patches are the\n -+\tsame or were added or removed etc.\n ++\tSuppress diff output. Only shows how the range has changed at\n ++\ta commit-level.\n +\n ::\n \tCompare the commits specified by the two ranges, where\n @@ -78,14 +81,14 @@\n \tint creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n \tstruct diff_options diffopt = { NULL };\n \tint simple_color = -1;\n -+\tint patch = 1;\n ++\tint no_patch = 0;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"creation-factor\", &creation_factor,\n \t\t\t N_(\"Percentage by which creation is weighted\")),\n \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n \t\t\t N_(\"use simple diff colors\")),\n -+\t\tOPT_BOOL('p', \"patch\", &patch,\n -+\t\t\t N_(\"show patch output\")),\n ++\t\tOPT_BOOL_F('s', \"no-patch\", &no_patch,\n ++\t\t\t N_(\"show patch output\"), PARSE_OPT_NONEG),\n \t\tOPT_END()\n \t};\n \tint i, j, res = 0;\n @@ -94,7 +97,7 @@\n \n \tres = show_range_diff(range1.buf, range2.buf, creation_factor,\n -\t\t\t simple_color < 1, &diffopt);\n -+\t\t\t simple_color < 1, patch, &diffopt);\n ++\t\t\t simple_color < 1, !no_patch, &diffopt);\n \n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\n @@ -155,7 +158,14 @@\n \ttest_cmp expected actual\n '\n \n -+test_expect_success 'changed commit --no-patch' '\n ++test_expect_success 'changed commit -p & --patch' '\n ++\tgit range-diff --no-color -p topic...changed >actual &&\n ++\ttest_cmp expected actual &&\n ++\tgit range-diff --no-color --patch topic...changed >actual &&\n ++\ttest_cmp expected actual\n ++'\n ++\n ++test_expect_success 'changed commit -s & --no-patch' '\n +\tgit range-diff --no-color --no-patch topic...changed >actual &&\n +\tcat >expected <<-EOF &&\n +\t1: 4de457d = 1: a4b3333 s/5/A/\n @@ -163,6 +173,8 @@\n +\t3: 147e64e ! 3: 0559556 s/11/B/\n +\t4: a63e992 ! 4: d966c5c s/12/B/\n +\tEOF\n ++\ttest_cmp expected actual &&\n ++\tgit range-diff --no-color -s topic...changed >actual &&\n +\ttest_cmp expected actual\n +'\n +\n\n Documentation/git-range-diff.txt | 5 +++\n builtin/log.c | 2 +-\n builtin/range-diff.c | 5 ++-\n log-tree.c | 2 +-\n range-diff.c | 6 +++-\n range-diff.h | 1 +\n t/t3206-range-diff.sh | 54 ++++++++++++++++++++++++++++++++\n 7 files changed, 71 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex f693930fdb..6c1eb647a1 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -57,6 +57,11 @@ to revert to color all lines according to the outer diff markers\n \tSee the ``Algorithm`` section below for an explanation why this is\n \tneeded.\n \n+-s::\n+--no-patch::\n+\tSuppress diff output. Only shows how the range has changed at\n+\ta commit-level.\n+\n ::\n \tCompare the commits specified by the two ranges, where\n \t`` is considered an older version of ``.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd864..e063bcf2dd 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (rev->rdiff1) {\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2,\n-\t\t\t\trev->creation_factor, 1, &rev->diffopt);\n+\t\t\t\trev->creation_factor, 1, 1, &rev->diffopt);\n \t}\n }\n \ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex f01a0be851..05d1f6b6b6 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \tint creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n \tstruct diff_options diffopt = { NULL };\n \tint simple_color = -1;\n+\tint no_patch = 0;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"creation-factor\", &creation_factor,\n \t\t\t N_(\"Percentage by which creation is weighted\")),\n \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n \t\t\t N_(\"use simple diff colors\")),\n+\t\tOPT_BOOL_F('s', \"no-patch\", &no_patch,\n+\t\t\t N_(\"show patch output\"), PARSE_OPT_NONEG),\n \t\tOPT_END()\n \t};\n \tint i, j, res = 0;\n@@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \t}\n \n \tres = show_range_diff(range1.buf, range2.buf, creation_factor,\n-\t\t\t simple_color < 1, &diffopt);\n+\t\t\t simple_color < 1, !no_patch, &diffopt);\n \n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\ndiff --git a/log-tree.c b/log-tree.c\nindex 7a83e99250..843e3ef83b 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)\n \t\tnext_commentary_block(opt, NULL);\n \t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n \t\tshow_range_diff(opt->rdiff1, opt->rdiff2,\n-\t\t\t\topt->creation_factor, 1, &opt->diffopt);\n+\t\t\t\topt->creation_factor, 1, 1, &opt->diffopt);\n \n \t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n \t}\ndiff --git a/range-diff.c b/range-diff.c\nindex bd8083f2d1..c1bfa593ce 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -436,6 +436,7 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)\n \n int show_range_diff(const char *range1, const char *range2,\n \t\t int creation_factor, int dual_color,\n+\t\t int patch,\n \t\t struct diff_options *diffopt)\n {\n \tint res = 0;\n@@ -453,7 +454,10 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\tif (patch)\n+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\telse\n+\t\t\topts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/range-diff.h b/range-diff.h\nindex 190593f0c7..99bbc1cd9f 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -7,6 +7,7 @@\n \n int show_range_diff(const char *range1, const char *range2,\n \t\t int creation_factor, int dual_color,\n+\t\t int patch,\n \t\t struct diff_options *diffopt);\n \n #endif\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6aae364171..27e071650b 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,26 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit -p & --patch' '\n+\tgit range-diff --no-color -p topic...changed >actual &&\n+\ttest_cmp expected actual &&\n+\tgit range-diff --no-color --patch topic...changed >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'changed commit -s & --no-patch' '\n+\tgit range-diff --no-color --no-patch topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual &&\n+\tgit range-diff --no-color -s topic...changed >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed commit with sm config' '\n \tgit range-diff --no-color --submodule=log topic...changed >actual &&\n \tcat >expected <<-EOF &&\n@@ -151,6 +171,17 @@ test_expect_success 'changed commit with sm config' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with sm config --no-patch' '\n+\tgit range-diff --no-color --no-patch --submodule=log topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'no commits on one side' '\n \tgit commit --amend -m \"new message\" &&\n \tgit range-diff master HEAD@{1} HEAD\n@@ -176,6 +207,17 @@ test_expect_success 'changed message' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed message --no-patch' '\n+\tgit range-diff --no-color --no-patch topic...changed-message >actual &&\n+\tsed s/Z/\\ /g >expected <<-EOF &&\n+\t1: 4de457d = 1: f686024 s/5/A/\n+\t2: fccce22 ! 2: 4ab067d s/4/A/\n+\t3: 147e64e = 3: b9cb956 s/11/B/\n+\t4: a63e992 = 4: 8add5f1 s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'dual-coloring' '\n \tsed -e \"s|^:||\" >expect <<-\\EOF &&\n \t:1: a4b3333 = 1: f686024 s/5/A/\n@@ -215,6 +257,18 @@ test_expect_success 'dual-coloring' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'dual-coloring --no-patch' '\n+\tsed -e \"s|^:||\" >expect <<-\\EOF &&\n+\t:1: a4b3333 = 1: f686024 s/5/A/\n+\t:2: f51d370 ! 2: 4ab067d s/4/A/\n+\t:3: 0559556 ! 3: b9cb956 s/11/B/\n+\t:4: d966c5c ! 4: 8add5f1 s/12/B/\n+\tEOF\n+\tgit range-diff changed...changed-message --color --dual-color --no-patch >actual.raw &&\n+\ttest_decode_color >actual writes:\n\n> diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> index f01a0be851..05d1f6b6b6 100644\n> --- a/builtin/range-diff.c\n> +++ b/builtin/range-diff.c\n> @@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n> \tint creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n> \tstruct diff_options diffopt = { NULL };\n> \tint simple_color = -1;\n> +\tint no_patch = 0;\n> \tstruct option options[] = {\n> \t\tOPT_INTEGER(0, \"creation-factor\", &creation_factor,\n> \t\t\t N_(\"Percentage by which creation is weighted\")),\n> \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n> \t\t\t N_(\"use simple diff colors\")),\n> +\t\tOPT_BOOL_F('s', \"no-patch\", &no_patch,\n> +\t\t\t N_(\"show patch output\"), PARSE_OPT_NONEG),\n\nAs OPT_BOOL(\"patch\") natively takes \"--no-patch\" to flip the bool\noff, an int variable \"patch\" that is initialized to 1 would make it\nmore readable by avoiding double negation !no_patch like the one we\nsee below. I guess the reason behind the contortion you wanted to\ngive the synonym --silent to it?\n\n> @@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tres = show_range_diff(range1.buf, range2.buf, creation_factor,\n> -\t\t\t simple_color < 1, &diffopt);\n> +\t\t\t simple_color < 1, !no_patch, &diffopt);\n\n> \tstrbuf_release(&range1);\n> \tstrbuf_release(&range2);\n\n> @@ -7,6 +7,7 @@\n> \n> int show_range_diff(const char *range1, const char *range2,\n> \t\t int creation_factor, int dual_color,\n> +\t\t int patch,\n> \t\t struct diff_options *diffopt);\n\nOther than that small \"Huh?\", the code looks good to me.\n\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 6aae364171..27e071650b 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'changed commit -p & --patch' '\n> +\tgit range-diff --no-color -p topic...changed >actual &&\n> +\ttest_cmp expected actual &&\n> +\tgit range-diff --no-color --patch topic...changed >actual &&\n> +\ttest_cmp expected actual\n\nThis makes sure that -p and --patch produces the same output as the\ndefault case? I am not sure who in the parseopt API groks the\nsingle letter \"-p\" in this case offhand. Care to explain how?\n\nThe other side of the test to see -s/--no-patch we see below also\nmakes sense.\n\n> +test_expect_success 'changed commit -s & --no-patch' '\n> +...\n> +\tcat >expected <<-EOF &&\n\nQuote EOF to allow readers skim the contents without looking for and\nworrying about $substitutions in there, unless there are tons of\noffending code in the same script already in which case we should\nleave the clean-up outside this primary change.\n\n"},{"id":"362637","messageId":"nycvar.QRO.7.76.6.1811071130570.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"874lcu6vr4.fsf@evledraar.gmail.com","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-07T10:34:34Z","receivedAt":"2018-11-07T10:34:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Nov 06 2018, Johannes Schindelin wrote:\n> \n> > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> On Mon, Nov 05 2018, Eric Sunshine wrote:\n> >>\n> >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n> >> >> Add a --no-patch option which shows which changes got removed, added\n> >> >> or moved etc., without showing the diff associated with them.\n> >> >\n> >> > This option existed in the very first version[1] of range-diff (then\n> >> > called branch-diff) implemented by Dscho, although it was called\n> >> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> >> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> >> > more consistent with existing Git options. I don't recall why Dscho\n> >> > removed the option during the re-rolls, but the explanation may be in\n> >> > that thread.\n> >>\n> >> Thanks for digging. Big thread, not going to re-read it now. I'd just\n> >> like to have this.\n> >\n> > In my hands, the well-documented `-s` option works (see e.g.\n> > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\n> > that the `git-range-diff` manual does not talk about the diff-options.\n> >\n> > And for the record, for me, `git range-diff A...B --no-patch` *already*\n> > works.\n> \n> Neither of those works for me without my patch. E.g.\n> \n> ./git-range-diff -s 711aaa392f...a5ba8f2101\n> ./git-range-diff --no-patch 711aaa392f...a5ba8f2101\n>\n> This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you\n> on?\n\nI tried it with git version 2.19.0.windows.1.\n\nTo verify, I repeated this with `next` (git version\n2.19.1.1215.g8438c0b2453a):\n\n./git range-diff -s 711aaa392f...a5ba8f2101\nfatal: unrecognized argument: --output-indicator-new=>\nerror: could not parse log for 'a5ba8f2101..711aaa392f'\n\nWhich means that something broke rather dramatically between\nv2.19.0.windows.1 and 8438c0b2453a.\n\nCiao,\nDscho\n\n> \n> >>\n> >> > I was also wondering if --summarize or --summary-only might be a\n> >> > better name, describing the behavior at a higher level, but since\n> >> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps\n> >> > the name is fine as is.\n> >>\n> >> I think we should aim to keep a 1=1 mapping between range-diff and\n> >> log/show options when possible, even though the output might have a\n> >> slightly different flavor as my 4th paragraph discussing a potential\n> >> --stat talks about.\n> >>\n> >> E.g. I can imagine that range-diff --no-patch --stat --summary would not\n> >> show the patch, but a stat as described there, plus e.g. a \"create\n> >> mode...\" if applicable.\n> >>\n> >> This change implements only a tiny fraction of that, but it would be\n> >> very neat if we supported more stuff, and showed it in range-diff-y way,\n> >> e.g. some compact format showing:\n> >>\n> >> 1 file changed, 3->2 insertions(+), 10->9 deletions(-)\n> >> create mode 100(6 -> 7)44 new-executable\n> >>\n> >> > The patch itself looks okay.\n> >> >\n> >> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/\n> >>\n> "},{"id":"362638","messageId":"nycvar.QRO.7.76.6.1811071142100.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071130570.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-07T10:43:32Z","receivedAt":"2018-11-07T10:43:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 7 Nov 2018, Johannes Schindelin wrote:\n\n> On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> \n> > On Tue, Nov 06 2018, Johannes Schindelin wrote:\n> > \n> > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> > >\n> > >> On Mon, Nov 05 2018, Eric Sunshine wrote:\n> > >>\n> > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n> > >> >> Add a --no-patch option which shows which changes got removed, added\n> > >> >> or moved etc., without showing the diff associated with them.\n> > >> >\n> > >> > This option existed in the very first version[1] of range-diff (then\n> > >> > called branch-diff) implemented by Dscho, although it was called\n> > >> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> > >> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> > >> > more consistent with existing Git options. I don't recall why Dscho\n> > >> > removed the option during the re-rolls, but the explanation may be in\n> > >> > that thread.\n> > >>\n> > >> Thanks for digging. Big thread, not going to re-read it now. I'd just\n> > >> like to have this.\n> > >\n> > > In my hands, the well-documented `-s` option works (see e.g.\n> > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\n> > > that the `git-range-diff` manual does not talk about the diff-options.\n> > >\n> > > And for the record, for me, `git range-diff A...B --no-patch` *already*\n> > > works.\n> > \n> > Neither of those works for me without my patch. E.g.\n> > \n> > ./git-range-diff -s 711aaa392f...a5ba8f2101\n> > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101\n> >\n> > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you\n> > on?\n> \n> I tried it with git version 2.19.0.windows.1.\n> \n> To verify, I repeated this with `next` (git version\n> 2.19.1.1215.g8438c0b2453a):\n> \n> ./git range-diff -s 711aaa392f...a5ba8f2101\n> fatal: unrecognized argument: --output-indicator-new=>\n> error: could not parse log for 'a5ba8f2101..711aaa392f'\n> \n> Which means that something broke rather dramatically between\n> v2.19.0.windows.1 and 8438c0b2453a.\n\nNevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can\nreproduce your finding.\n\nCiao,\nDscho"},{"id":"362640","messageId":"nycvar.QRO.7.76.6.1811071144140.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071142100.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-07T10:55:52Z","receivedAt":"2018-11-07T10:56:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 7 Nov 2018, Johannes Schindelin wrote:\n\n> On Wed, 7 Nov 2018, Johannes Schindelin wrote:\n> \n> > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> > \n> > > On Tue, Nov 06 2018, Johannes Schindelin wrote:\n> > > \n> > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> > > >\n> > > >> On Mon, Nov 05 2018, Eric Sunshine wrote:\n> > > >>\n> > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n> > > >> >> Add a --no-patch option which shows which changes got removed, added\n> > > >> >> or moved etc., without showing the diff associated with them.\n> > > >> >\n> > > >> > This option existed in the very first version[1] of range-diff (then\n> > > >> > called branch-diff) implemented by Dscho, although it was called\n> > > >> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> > > >> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> > > >> > more consistent with existing Git options. I don't recall why Dscho\n> > > >> > removed the option during the re-rolls, but the explanation may be in\n> > > >> > that thread.\n> > > >>\n> > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just\n> > > >> like to have this.\n> > > >\n> > > > In my hands, the well-documented `-s` option works (see e.g.\n> > > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\n> > > > that the `git-range-diff` manual does not talk about the diff-options.\n> > > >\n> > > > And for the record, for me, `git range-diff A...B --no-patch` *already*\n> > > > works.\n> > > \n> > > Neither of those works for me without my patch. E.g.\n> > > \n> > > ./git-range-diff -s 711aaa392f...a5ba8f2101\n> > > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101\n> > >\n> > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you\n> > > on?\n> > \n> > I tried it with git version 2.19.0.windows.1.\n> > \n> > To verify, I repeated this with `next` (git version\n> > 2.19.1.1215.g8438c0b2453a):\n> > \n> > ./git range-diff -s 711aaa392f...a5ba8f2101\n> > fatal: unrecognized argument: --output-indicator-new=>\n> > error: could not parse log for 'a5ba8f2101..711aaa392f'\n> > \n> > Which means that something broke rather dramatically between\n> > v2.19.0.windows.1 and 8438c0b2453a.\n> \n> Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can\n> reproduce your finding.\n\nI've bisected this breakage down to 73a834e9e279 (range-diff: relieve\ncallers of low-level configuration burden, 2018-07-22). Eric, that's one\nof your commits.\n\nCiao,\nDscho"},{"id":"362641","messageId":"nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071144140.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-07T11:08:24Z","receivedAt":"2018-11-07T11:08:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Me again,\n\nOn Wed, 7 Nov 2018, Johannes Schindelin wrote:\n\n> On Wed, 7 Nov 2018, Johannes Schindelin wrote:\n> \n> > On Wed, 7 Nov 2018, Johannes Schindelin wrote:\n> > \n> > > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> > > \n> > > > On Tue, Nov 06 2018, Johannes Schindelin wrote:\n> > > > \n> > > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n> > > > >\n> > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote:\n> > > > >>\n> > > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason wrote:\n> > > > >> >> Add a --no-patch option which shows which changes got removed, added\n> > > > >> >> or moved etc., without showing the diff associated with them.\n> > > > >> >\n> > > > >> > This option existed in the very first version[1] of range-diff (then\n> > > > >> > called branch-diff) implemented by Dscho, although it was called\n> > > > >> > --no-patches (with an \"es\"), which it inherited from tbdiff. I think\n> > > > >> > someone (possibly me) pointed out that --no-patch (sans \"es\") would be\n> > > > >> > more consistent with existing Git options. I don't recall why Dscho\n> > > > >> > removed the option during the re-rolls, but the explanation may be in\n> > > > >> > that thread.\n> > > > >>\n> > > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just\n> > > > >> like to have this.\n> > > > >\n> > > > > In my hands, the well-documented `-s` option works (see e.g.\n> > > > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit\n> > > > > that the `git-range-diff` manual does not talk about the diff-options.\n> > > > >\n> > > > > And for the record, for me, `git range-diff A...B --no-patch` *already*\n> > > > > works.\n> > > > \n> > > > Neither of those works for me without my patch. E.g.\n> > > > \n> > > > ./git-range-diff -s 711aaa392f...a5ba8f2101\n> > > > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101\n> > > >\n> > > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you\n> > > > on?\n> > > \n> > > I tried it with git version 2.19.0.windows.1.\n> > > \n> > > To verify, I repeated this with `next` (git version\n> > > 2.19.1.1215.g8438c0b2453a):\n> > > \n> > > ./git range-diff -s 711aaa392f...a5ba8f2101\n> > > fatal: unrecognized argument: --output-indicator-new=>\n> > > error: could not parse log for 'a5ba8f2101..711aaa392f'\n> > > \n> > > Which means that something broke rather dramatically between\n> > > v2.19.0.windows.1 and 8438c0b2453a.\n> > \n> > Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can\n> > reproduce your finding.\n> \n> I've bisected this breakage down to 73a834e9e279 (range-diff: relieve\n> callers of low-level configuration burden, 2018-07-22). Eric, that's one\n> of your commits.\n\nOkay, so I would really like to point you to this particular paragraph in\nthe manual page of `git range-diff` (just below\nhttps://git-scm.com/docs/git-range-diff#git-range-diff-ltbasegtltrev1gtltrev2gt):\n\n\t`git range-diff` also accepts the regular diff options (see\n\tlinkgit:git-diff[1]), most notably the `--color=[]` and\n\t`--no-color` options. These options are used when generating the \"diff\n\tbetween patches\", i.e. to compare the author, commit message and diff of\n\tcorresponding old/new commits. There is currently no means to tweak the\n\tdiff options passed to `git log` when generating those patches.\n\nSo this was quite intentional, in particular with an eye on `--no-patch`.\nNote also the commit message of c8c5e43ac3f9 (range-diff: also show the\ndiff between patches, 2018-08-13):\n\n Note also: while tbdiff accepts the `--no-patches` option to suppress\n these diffs between patches, we prefer the `-s` (or `--no-patch`)\n option that is automatically supported via our use of diff_opt_parse().\n\nThis was very, very intentional, as you can see, and it was pretty broken\nby 73a834e. This here patch papers over that breakage, sadly I have too\nmuch on my plate as it is, so I cannot wrap it up in a nice commit (nor\nadd a regression test, but you did that, nor investigate what else is\nbroken) and therefore I would be indebted if you could take this in your\ncustody:\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 3dd2edda0176..014112ee401f 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -433,7 +433,8 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\tif (!opts.output_format)\n+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\n\nCiao,\nDscho"},{"id":"362642","messageId":"nycvar.QRO.7.76.6.1811071210250.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"xmqqsh0dd7ql.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] range-diff: add a --no-patch option to show a summary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-07T11:11:42Z","receivedAt":"2018-11-07T11:11:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 7 Nov 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c\n> > index f01a0be851..05d1f6b6b6 100644\n> > --- a/builtin/range-diff.c\n> > +++ b/builtin/range-diff.c\n> > @@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n> > \tint creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n> > \tstruct diff_options diffopt = { NULL };\n> > \tint simple_color = -1;\n> > +\tint no_patch = 0;\n> > \tstruct option options[] = {\n> > \t\tOPT_INTEGER(0, \"creation-factor\", &creation_factor,\n> > \t\t\t N_(\"Percentage by which creation is weighted\")),\n> > \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n> > \t\t\t N_(\"use simple diff colors\")),\n> > +\t\tOPT_BOOL_F('s', \"no-patch\", &no_patch,\n> > +\t\t\t N_(\"show patch output\"), PARSE_OPT_NONEG),\n> \n> As OPT_BOOL(\"patch\") natively takes \"--no-patch\" to flip the bool\n> off, an int variable \"patch\" that is initialized to 1 would make it\n> more readable by avoiding double negation !no_patch like the one we\n> see below. I guess the reason behind the contortion you wanted to\n> give the synonym --silent to it?\n\nIn light of my investigation that revealed that the original behavior\n(which is still documented in the manual page of range-diff) was broken,\nand I would much rather see that fixed than adding a workaround.\n\nI would be fine with my patch being combined with the update to the manual\npage and the regression test, as a v3.\n\nCiao,\nDscho\n\n> \n> > @@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n> > \t}\n> > \n> > \tres = show_range_diff(range1.buf, range2.buf, creation_factor,\n> > -\t\t\t simple_color < 1, &diffopt);\n> > +\t\t\t simple_color < 1, !no_patch, &diffopt);\n> \n> > \tstrbuf_release(&range1);\n> > \tstrbuf_release(&range2);\n> \n> > @@ -7,6 +7,7 @@\n> > \n> > int show_range_diff(const char *range1, const char *range2,\n> > \t\t int creation_factor, int dual_color,\n> > +\t\t int patch,\n> > \t\t struct diff_options *diffopt);\n> \n> Other than that small \"Huh?\", the code looks good to me.\n> \n> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> > index 6aae364171..27e071650b 100755\n> > --- a/t/t3206-range-diff.sh\n> > +++ b/t/t3206-range-diff.sh\n> > @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '\n> > \ttest_cmp expected actual\n> > '\n> > \n> > +test_expect_success 'changed commit -p & --patch' '\n> > +\tgit range-diff --no-color -p topic...changed >actual &&\n> > +\ttest_cmp expected actual &&\n> > +\tgit range-diff --no-color --patch topic...changed >actual &&\n> > +\ttest_cmp expected actual\n> \n> This makes sure that -p and --patch produces the same output as the\n> default case? I am not sure who in the parseopt API groks the\n> single letter \"-p\" in this case offhand. Care to explain how?\n> \n> The other side of the test to see -s/--no-patch we see below also\n> makes sense.\n> \n> > +test_expect_success 'changed commit -s & --no-patch' '\n> > +...\n> > +\tcat >expected <<-EOF &&\n> \n> Quote EOF to allow readers skim the contents without looking for and\n> worrying about $substitutions in there, unless there are tons of\n> offending code in the same script already in which case we should\n> leave the clean-up outside this primary change.\n> \n> "},{"id":"362648","messageId":"20181107122202.1813-1-avarab@gmail.com","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet","subject":"[PATCH v3 0/2] range-diff: doc + regression fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-07T12:22:00Z","receivedAt":"2018-11-07T12:22:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As Johannes notes this --no-patch option I wanted to add is something\nwe had already, but is it turns out it was broken.\n\nSo this is an entirely rewritten v3 (not bothering with the range-diff\nfor it) which a) documents the output stability of stuff like --stat\nand the like (there isn't any) b) fixes the regression & adds a test.\n\nI did try various other diff options and they all seem to work.\n\nÆvar Arnfjörð Bjarmason (2):\n range-diff doc: add a section about output stability\n range-diff: fix regression in passing along diff options\n\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 1 deletion(-)\n\n-- \n2.19.1.930.g4563a0d9d0\n\n"},{"id":"362649","messageId":"20181107122202.1813-2-avarab@gmail.com","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet","subject":"[PATCH v3 1/2] range-diff doc: add a section about output stability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-07T12:22:01Z","receivedAt":"2018-11-07T12:22:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The range-diff command is already advertised as porcelain, but let's\nmake it really clear that the output is completely subject to change,\nparticularly when it comes to diff options such as --stat. Right now\nthat option doesn't work, but fixing that is the subject of a later\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex f693930fdb..bbd07a9be8 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -78,6 +78,23 @@ between patches\", i.e. to compare the author, commit message and diff of\n corresponding old/new commits. There is currently no means to tweak the\n diff options passed to `git log` when generating those patches.\n \n+OUTPUT STABILITY\n+----------------\n+\n+The output of the `range-diff` command is subject to change. It is\n+intended to be human-readable porcelain output, not something that can\n+be used across versions of Git to get a textually stable `range-diff`\n+(as opposed to something like the `--stable` option to\n+linkgit:git-patch-id[1]). There's also no equivalent of\n+linkgit:git-apply[1] for `range-diff`, the output is not intended to\n+be machine-readable.\n+\n+This is particularly true when passing in diff options. Currently some\n+options like `--stat` can as an emergent effect produce output that's\n+quite useless in the context of `range-diff`. Future versions of\n+`range-diff` may learn to interpret such options in a manner specifc\n+to `range-diff` (e.g. for `--stat` summarizing how the diffstat\n+changed).\n \n CONFIGURATION\n -------------\n-- \n2.19.1.930.g4563a0d9d0\n\n"},{"id":"362650","messageId":"20181107122202.1813-3-avarab@gmail.com","threadId":"49762","inReplyTo":"nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet","subject":"[PATCH v3 2/2] range-diff: fix regression in passing along diff options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-07T12:22:02Z","receivedAt":"2018-11-07T12:22:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\nburden\", 2018-07-22) we broke passing down options like --no-patch,\n--stat etc. Fix that regression, and add a test for some of these\noptions being passed down.\n\nAs noted in a change leading up to this (\"range-diff doc: add a\nsection about output stability\", 2018-11-07) the output is not meant\nto be stable. So this regression test will likely need to be tweaked\nonce we get a \"proper\" --stat option.\n\nSee\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/\nfor a further explanation of the regression.\n\nThe quoting of \"EOF\" here mirrors that of an earlier test. Perhaps\nthat should be fixed, but let's leave that up to a later cleanup\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex bd8083f2d1..488844c0af 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\tif (!opts.output_format)\n+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6aae364171..e497c1358f 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,36 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with --no-patch diff option' '\n+\tgit range-diff --no-color --no-patch topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'changed commit with --stat diff option' '\n+\tgit range-diff --no-color --stat topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed commit with sm config' '\n \tgit range-diff --no-color --submodule=log topic...changed >actual &&\n \tcat >expected <<-EOF &&\n-- \n2.19.1.930.g4563a0d9d0\n\n"},{"id":"362655","messageId":"2504086.hWFjgDPvVf@thunderbird","threadId":"49762","inReplyTo":"20181107122202.1813-2-avarab@gmail.com","subject":"Re: [PATCH v3 1/2] range-diff doc: add a section about output stability","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-11-07T13:10:52Z","receivedAt":"2018-11-07T13:10:55Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Wednesday, November 7, 2018 5:22:01 AM MST Ævar Arnfjörð Bjarmason wrote:\n> +OUTPUT STABILITY\n> +----------------\n> +\n> +The output of the `range-diff` command is subject to change. It is\n> +intended to be human-readable porcelain output, not something that can\n> +be used across versions of Git to get a textually stable `range-diff`\n> +(as opposed to something like the `--stable` option to\n> +linkgit:git-patch-id[1]). There's also no equivalent of\n> +linkgit:git-apply[1] for `range-diff`, the output is not intended to\n> +be machine-readable.\n> +\n> +This is particularly true when passing in diff options. Currently some\n> +options like `--stat` can as an emergent effect produce output that's\n\n\"`--stat` can as an emergent\": I read that for times to decided it was correct \ngrammar. Should it be reworded to read better? Just a nit.\n\n> +quite useless in the context of `range-diff`. Future versions of\n> +`range-diff` may learn to interpret such options in a manner specifc\n> +to `range-diff` (e.g. for `--stat` summarizing how the diffstat\n> +changed).\n\n\n\n\n"},{"id":"362669","messageId":"CAN0heSqPGQOYYQOc=J4NRJbRC1yCd4NTUi3nh5Dt54eCHTib8g@mail.gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-2-avarab@gmail.com","subject":"Re: [PATCH v3 1/2] range-diff doc: add a section about output stability","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-11-07T19:06:42Z","receivedAt":"2018-11-07T19:06:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 7 Nov 2018 at 13:22, Ævar Arnfjörð Bjarmason wrote:\n> +\n> +This is particularly true when passing in diff options. Currently some\n> +options like `--stat` can as an emergent effect produce output that's\n> +quite useless in the context of `range-diff`. Future versions of\n> +`range-diff` may learn to interpret such options in a manner specifc\n\ns/specifc/specific/\n\n> +to `range-diff` (e.g. for `--stat` summarizing how the diffstat\n> +changed).\n"},{"id":"362677","messageId":"xmqqlg64bivd.fsf@gitster-ct.c.googlers.com","threadId":"49762","inReplyTo":"2504086.hWFjgDPvVf@thunderbird","subject":"Re: [PATCH v3 1/2] range-diff doc: add a section about output stability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-07T22:52:38Z","receivedAt":"2018-11-07T22:52:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen & Linda Smith writes:\n\n>> +This is particularly true when passing in diff options. Currently some\n>> +options like `--stat` can as an emergent effect produce output that's\n>\n> \"`--stat` can as an emergent\": I read that for times to decided it was correct \n> grammar. Should it be reworded to read better? Just a nit.\n\nA pair of comma around \"as an ... effect\" would make it a bit more\nreadable. It also took me four reads before I convinced myself that\nthe original wants to say \"Some options may just do whatever they\nhappen to do that result in pretty useless output\".\n\n>\n>> +quite useless in the context of `range-diff`. Future versions of\n>> +`range-diff` may learn to interpret such options in a manner specifc\n>> +to `range-diff` (e.g. for `--stat` summarizing how the diffstat\n>> +changed).\n"},{"id":"362745","messageId":"CAPig+cTeNcn_raJWfEK_C_du1v1KYQFTVmWA9B4DNHAXoFHw-w@mail.gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-3-avarab@gmail.com","subject":"Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-08T17:08:37Z","receivedAt":"2018-11-08T17:08:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason wrote:\n> In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\n> burden\", 2018-07-22) we broke passing down options like --no-patch,\n> --stat etc. Fix that regression, and add a test for some of these\n> options being passed down.\n\nThanks both (Ævar and Dscho) for cleaning up my mess, and sorry for\nnot responding sooner; I only just found time to read the discussion\nthread. One comment below...\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/range-diff.c b/range-diff.c\n> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n> memcpy(&opts, diffopt, sizeof(opts));\n> - opts.output_format = DIFF_FORMAT_PATCH;\n> + if (!opts.output_format)\n> + opts.output_format = DIFF_FORMAT_PATCH;\n\nLooking at diff.c:parse_diff_opt() and enable_patch_output(), rather\nthan introducing this new conditional, I'm thinking that a more\ncorrect fix would be:\n\n opts.output_format |= DIFF_FORMAT_PATCH;\n\n(note the '|=' operator). This would result in 'opts.output_format'\ncontaining (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did\nprior to 73a834e9e2 when --no-patch was specified.\n"},{"id":"362762","messageId":"87k1ln5hcx.fsf@evledraar.gmail.com","threadId":"49762","inReplyTo":"CAPig+cTeNcn_raJWfEK_C_du1v1KYQFTVmWA9B4DNHAXoFHw-w@mail.gmail.com","subject":"Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-08T22:34:06Z","receivedAt":"2018-11-08T22:34:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 08 2018, Eric Sunshine wrote:\n\n> On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason wrote:\n>> In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\n>> burden\", 2018-07-22) we broke passing down options like --no-patch,\n>> --stat etc. Fix that regression, and add a test for some of these\n>> options being passed down.\n>\n> Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for\n> not responding sooner; I only just found time to read the discussion\n> thread. One comment below...\n>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> diff --git a/range-diff.c b/range-diff.c\n>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n>> memcpy(&opts, diffopt, sizeof(opts));\n>> - opts.output_format = DIFF_FORMAT_PATCH;\n>> + if (!opts.output_format)\n>> + opts.output_format = DIFF_FORMAT_PATCH;\n>\n> Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather\n> than introducing this new conditional, I'm thinking that a more\n> correct fix would be:\n>\n> opts.output_format |= DIFF_FORMAT_PATCH;\n>\n> (note the '|=' operator). This would result in 'opts.output_format'\n> containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did\n> prior to 73a834e9e2 when --no-patch was specified.\n\nMaybe I'm misunderstanding, but if you mean this on top:\n\n diff --git a/range-diff.c b/range-diff.c\n index 488844c0af..ea317f92f9 100644\n --- a/range-diff.c\n +++ b/range-diff.c\n @@ -453,8 +453,7 @@ int show_range_diff(const char *range1, const char *range2,\n struct strbuf indent = STRBUF_INIT;\n\n memcpy(&opts, diffopt, sizeof(opts));\n - if (!opts.output_format)\n - opts.output_format = DIFF_FORMAT_PATCH;\n + opts.output_format |= DIFF_FORMAT_PATCH;\n opts.flags.suppress_diff_headers = 1;\n opts.flags.dual_color_diffed_diffs = dual_color;\n opts.output_prefix = output_prefix_cb;\n\nThen the --stat test I've added here fails, because unlike \"diff\" the\n\"--stat\" (and others) will implicitly \"--patch\" and you need\n\"--no-patch\" as well (again, unlike with \"diff\").\n\nRight now --stat is pretty useless, but it could be made to make sense,\nand at that point (and earlier) I think it would be confusing if\n\"range-diff\" had different semantics with no options v.s. one option\nlike \"--stat\" v.s. \"--stat -p\" compared to \"diff\".\n"},{"id":"362778","messageId":"CAPig+cSJ=T_iwPOaxT9T0XK2UYZfvkE_Vx5D__ZLct+z1-p5cg@mail.gmail.com","threadId":"49762","inReplyTo":"87k1ln5hcx.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-09T06:46:17Z","receivedAt":"2018-11-09T06:46:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Nov 08 2018, Eric Sunshine wrote:\n> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather\n> > than introducing this new conditional, I'm thinking that a more\n> > correct fix would be:\n> >\n> > opts.output_format |= DIFF_FORMAT_PATCH;\n> >\n> > (note the '|=' operator). This would result in 'opts.output_format'\n> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did\n> > prior to 73a834e9e2 when --no-patch was specified.\n>\n> Maybe I'm misunderstanding, but if you mean this on top:\n>\n> - if (!opts.output_format)\n> - opts.output_format = DIFF_FORMAT_PATCH;\n> + opts.output_format |= DIFF_FORMAT_PATCH;\n\nThat is indeed what I mean.\n\n> Then the --stat test I've added here fails, because unlike \"diff\" the\n> \"--stat\" (and others) will implicitly \"--patch\" and you need\n> \"--no-patch\" as well (again, unlike with \"diff\").\n\nThis new --stat test also fails with Dscho's original git-range-diff\nimplementation, even before 73a834e9e2 regressed the --no-patch case.\nThe commit message seems to sell this patch as a pure regression-fix,\nso it feels wrong for it to be conflating a regression fix\n(--no-patch) with preparation for potential future improvements to\nother options (--stat, etc.).\n\nI could see this as a two-patch series in which patch 1/2 fixes the\nregression (with, say, \"|=\"), and patch 2/2 generalizes setting\n'opts.output_format' for the future. Alternately, if left as a single\npatch, perhaps the commit message could be fleshed out to better\nexplain that it is doing more than merely fixing a regression (since\nit wasn't at all obvious to me, even after digging into the code\nearlier to come up with \"|=\", or now when trying to understand your\nresponse).\n\n> Right now --stat is pretty useless, but it could be made to make sense,\n> and at that point (and earlier) I think it would be confusing if\n> \"range-diff\" had different semantics with no options v.s. one option\n> like \"--stat\" v.s. \"--stat -p\" compared to \"diff\".\n\nPerhaps this sort of rationale, along with some explanatory examples\ncould be added to the commit message to help the reader more fully\nunderstand the situation.\n\nThanks for working on this.\n"},{"id":"362779","messageId":"87in1666t9.fsf@evledraar.gmail.com","threadId":"49762","inReplyTo":"CAPig+cSJ=T_iwPOaxT9T0XK2UYZfvkE_Vx5D__ZLct+z1-p5cg@mail.gmail.com","subject":"Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-09T07:36:34Z","receivedAt":"2018-11-09T07:36:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 09 2018, Eric Sunshine wrote:\n\n> On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Nov 08 2018, Eric Sunshine wrote:\n>> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather\n>> > than introducing this new conditional, I'm thinking that a more\n>> > correct fix would be:\n>> >\n>> > opts.output_format |= DIFF_FORMAT_PATCH;\n>> >\n>> > (note the '|=' operator). This would result in 'opts.output_format'\n>> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did\n>> > prior to 73a834e9e2 when --no-patch was specified.\n>>\n>> Maybe I'm misunderstanding, but if you mean this on top:\n>>\n>> - if (!opts.output_format)\n>> - opts.output_format = DIFF_FORMAT_PATCH;\n>> + opts.output_format |= DIFF_FORMAT_PATCH;\n>\n> That is indeed what I mean.\n\n*Nod*\n\n>> Then the --stat test I've added here fails, because unlike \"diff\" the\n>> \"--stat\" (and others) will implicitly \"--patch\" and you need\n>> \"--no-patch\" as well (again, unlike with \"diff\").\n>\n> This new --stat test also fails with Dscho's original git-range-diff\n> implementation, even before 73a834e9e2 regressed the --no-patch case.\n> The commit message seems to sell this patch as a pure regression-fix,\n> so it feels wrong for it to be conflating a regression fix\n> (--no-patch) with preparation for potential future improvements to\n> other options (--stat, etc.).\n>\n> I could see this as a two-patch series in which patch 1/2 fixes the\n> regression (with, say, \"|=\"), and patch 2/2 generalizes setting\n> 'opts.output_format' for the future. Alternately, if left as a single\n> patch, perhaps the commit message could be fleshed out to better\n> explain that it is doing more than merely fixing a regression (since\n> it wasn't at all obvious to me, even after digging into the code\n> earlier to come up with \"|=\", or now when trying to understand your\n> response).\n\nYeah that makes sense. I did not see (but see now) that the --stat\nbehavior was different now v.s. before your 73a834e9e2.\n\n>> Right now --stat is pretty useless, but it could be made to make sense,\n>> and at that point (and earlier) I think it would be confusing if\n>> \"range-diff\" had different semantics with no options v.s. one option\n>> like \"--stat\" v.s. \"--stat -p\" compared to \"diff\".\n>\n> Perhaps this sort of rationale, along with some explanatory examples\n> could be added to the commit message to help the reader more fully\n> understand the situation.\n\n*Nod*\n"},{"id":"362809","messageId":"20181109101803.3038-1-avarab@gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-3-avarab@gmail.com","subject":"[PATCH v4 0/3] range-diff fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-09T10:18:00Z","receivedAt":"2018-11-09T10:18:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresses feedback on v3, especially Eric's suggestion to split out\nthe behavior change (which I was not aware of) into a 3/3. Diff with\nv3:\n\n1: 23295d7806 ! 1: 5399e57513 range-diff doc: add a section about output stability\n @@ -29,11 +29,11 @@\n +be machine-readable.\n +\n +This is particularly true when passing in diff options. Currently some\n -+options like `--stat` can as an emergent effect produce output that's\n -+quite useless in the context of `range-diff`. Future versions of\n -+`range-diff` may learn to interpret such options in a manner specifc\n -+to `range-diff` (e.g. for `--stat` summarizing how the diffstat\n -+changed).\n ++options like `--stat` can, as an emergent effect, produce output\n ++that's quite useless in the context of `range-diff`. Future versions\n ++of `range-diff` may learn to interpret such options in a manner\n ++specific to `range-diff` (e.g. for `--stat` producing human-readable\n ++output which summarizes how the diffstat changed).\n \n CONFIGURATION\n -------------\n2: b21bd273f5 ! 2: e56975df6c range-diff: fix regression in passing along diff options\n @@ -4,8 +4,10 @@\n \n In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\n burden\", 2018-07-22) we broke passing down options like --no-patch,\n - --stat etc. Fix that regression, and add a test for some of these\n - options being passed down.\n + --stat etc.\n +\n + Fix that regression, and add a test asserting the pre-73a834e9e2\n + behavior for some of these diff options.\n \n As noted in a change leading up to this (\"range-diff doc: add a\n section about output stability\", 2018-11-07) the output is not meant\n @@ -14,7 +16,9 @@\n \n See\n https://public-inbox.org/git/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/\n - for a further explanation of the regression.\n + for a further explanation of the regression. The fix here is not the\n + same as in Johannes's on-list patch, for reasons that'll be explained\n + in a follow-up commit.\n \n The quoting of \"EOF\" here mirrors that of an earlier test. Perhaps\n that should be fixed, but let's leave that up to a later cleanup\n @@ -30,8 +34,7 @@\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n -\t\topts.output_format = DIFF_FORMAT_PATCH;\n -+\t\tif (!opts.output_format)\n -+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n ++\t\topts.output_format |= DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\n @@ -55,20 +58,43 @@\n +'\n +\n +test_expect_success 'changed commit with --stat diff option' '\n ++\tfour_spaces=\" \" &&\n +\tgit range-diff --no-color --stat topic...changed >actual &&\n +\tcat >expected <<-EOF &&\n +\t1: 4de457d = 1: a4b3333 s/5/A/\n +\t a => b | 0\n +\t 1 file changed, 0 insertions(+), 0 deletions(-)\n ++\t$four_spaces\n +\t2: fccce22 = 2: f51d370 s/4/A/\n +\t a => b | 0\n +\t 1 file changed, 0 insertions(+), 0 deletions(-)\n ++\t$four_spaces\n +\t3: 147e64e ! 3: 0559556 s/11/B/\n +\t a => b | 0\n +\t 1 file changed, 0 insertions(+), 0 deletions(-)\n ++\t$four_spaces\n ++\t @@ -10,7 +10,7 @@\n ++\t 9\n ++\t 10\n ++\t -11\n ++\t -+B\n ++\t ++BB\n ++\t 12\n ++\t 13\n ++\t 14\n +\t4: a63e992 ! 4: d966c5c s/12/B/\n +\t a => b | 0\n +\t 1 file changed, 0 insertions(+), 0 deletions(-)\n ++\t$four_spaces\n ++\t @@ -8,7 +8,7 @@\n ++\t @@\n ++\t 9\n ++\t 10\n ++\t - B\n ++\t + BB\n ++\t -12\n ++\t +B\n ++\t 13\n +\tEOF\n +\ttest_cmp expected actual\n +'\n-: ---------- > 3: edfef733c7 range-diff: make diff option behavior (e.g. --stat) consistent\n\nÆvar Arnfjörð Bjarmason (3):\n range-diff doc: add a section about output stability\n range-diff: fix regression in passing along diff options\n range-diff: make diff option behavior (e.g. --stat) consistent\n\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 31 +++++++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 1 deletion(-)\n\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"362810","messageId":"20181109101803.3038-2-avarab@gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-3-avarab@gmail.com","subject":"[PATCH v4 1/3] range-diff doc: add a section about output stability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-09T10:18:01Z","receivedAt":"2018-11-09T10:18:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The range-diff command is already advertised as porcelain, but let's\nmake it really clear that the output is completely subject to change,\nparticularly when it comes to diff options such as --stat. Right now\nthat option doesn't work, but fixing that is the subject of a later\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex f693930fdb..8a6ea2c6c5 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -78,6 +78,23 @@ between patches\", i.e. to compare the author, commit message and diff of\n corresponding old/new commits. There is currently no means to tweak the\n diff options passed to `git log` when generating those patches.\n \n+OUTPUT STABILITY\n+----------------\n+\n+The output of the `range-diff` command is subject to change. It is\n+intended to be human-readable porcelain output, not something that can\n+be used across versions of Git to get a textually stable `range-diff`\n+(as opposed to something like the `--stable` option to\n+linkgit:git-patch-id[1]). There's also no equivalent of\n+linkgit:git-apply[1] for `range-diff`, the output is not intended to\n+be machine-readable.\n+\n+This is particularly true when passing in diff options. Currently some\n+options like `--stat` can, as an emergent effect, produce output\n+that's quite useless in the context of `range-diff`. Future versions\n+of `range-diff` may learn to interpret such options in a manner\n+specific to `range-diff` (e.g. for `--stat` producing human-readable\n+output which summarizes how the diffstat changed).\n \n CONFIGURATION\n -------------\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"362811","messageId":"20181109101803.3038-3-avarab@gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-3-avarab@gmail.com","subject":"[PATCH v4 2/3] range-diff: fix regression in passing along diff options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-09T10:18:02Z","receivedAt":"2018-11-09T10:18:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\nburden\", 2018-07-22) we broke passing down options like --no-patch,\n--stat etc.\n\nFix that regression, and add a test asserting the pre-73a834e9e2\nbehavior for some of these diff options.\n\nAs noted in a change leading up to this (\"range-diff doc: add a\nsection about output stability\", 2018-11-07) the output is not meant\nto be stable. So this regression test will likely need to be tweaked\nonce we get a \"proper\" --stat option.\n\nSee\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/\nfor a further explanation of the regression. The fix here is not the\nsame as in Johannes's on-list patch, for reasons that'll be explained\nin a follow-up commit.\n\nThe quoting of \"EOF\" here mirrors that of an earlier test. Perhaps\nthat should be fixed, but let's leave that up to a later cleanup\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n range-diff.c | 2 +-\n t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex bd8083f2d1..ea317f92f9 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -453,7 +453,7 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\topts.output_format |= DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6aae364171..ab44e085d5 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,59 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with --no-patch diff option' '\n+\tgit range-diff --no-color --no-patch topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'changed commit with --stat diff option' '\n+\tfour_spaces=\" \" &&\n+\tgit range-diff --no-color --stat topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t @@ -10,7 +10,7 @@\n+\t 9\n+\t 10\n+\t -11\n+\t -+B\n+\t ++BB\n+\t 12\n+\t 13\n+\t 14\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t @@ -8,7 +8,7 @@\n+\t @@\n+\t 9\n+\t 10\n+\t - B\n+\t + BB\n+\t -12\n+\t +B\n+\t 13\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed commit with sm config' '\n \tgit range-diff --no-color --submodule=log topic...changed >actual &&\n \tcat >expected <<-EOF &&\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"362812","messageId":"20181109101803.3038-4-avarab@gmail.com","threadId":"49762","inReplyTo":"20181107122202.1813-3-avarab@gmail.com","subject":"[PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-09T10:18:03Z","receivedAt":"2018-11-09T10:18:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the behavior when diff options (e.g. \"--stat\") are passed\nconsistent with how \"diff\" behaves.\n\nBefore 73a834e9e2 (\"range-diff: relieve callers of low-level\nconfiguration burden\", 2018-07-22) running range-diff with \"--stat\"\nwould produce stat output and the diff output, as opposed to how\n\"diff\" behaves where once \"--stat\" is specified \"--patch\" also needs\nto be provided to emit the patch output.\n\nAs noted in a previous change (\"range-diff doc: add a section about\noutput stability\", 2018-11-07) the \"--stat\" output with \"range-diff\"\nis useless at the moment.\n\nBut we should behave consistently with \"diff\" in anticipation of such\noutput being useful in the future, because it would make for confusing\nUI if two \"diff\" and \"range-diff\" behaved differently when it came to\nhow they interpret diff options.\n\nThe new behavior is also consistent with the existing documentation\nadded in ba931edd28 (\"range-diff: populate the man page\",\n2018-08-13). See \"[...]also accepts the regular diff options[...]\" in\ngit-range-diff(1).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 22 ----------------------\n 2 files changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex ea317f92f9..72bde281f3 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format |= DIFF_FORMAT_PATCH;\n+\t\tif (!opts.output_format)\n+\t\t\topts.output_format |= DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex ab44e085d5..9352f65280 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff option' '\n \t1: 4de457d = 1: a4b3333 s/5/A/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n \t2: fccce22 = 2: f51d370 s/4/A/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n \t3: 147e64e ! 3: 0559556 s/11/B/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n-\t @@ -10,7 +10,7 @@\n-\t 9\n-\t 10\n-\t -11\n-\t -+B\n-\t ++BB\n-\t 12\n-\t 13\n-\t 14\n \t4: a63e992 ! 4: d966c5c s/12/B/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n-\t @@ -8,7 +8,7 @@\n-\t @@\n-\t 9\n-\t 10\n-\t - B\n-\t + BB\n-\t -12\n-\t +B\n-\t 13\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"362827","messageId":"23017996.O9Ifb9KJqy@thunderbird","threadId":"49762","inReplyTo":"20181109101803.3038-4-avarab@gmail.com","subject":"Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Stephen & Linda Smith","fromEmail":"ischis2@cox.net","sentAt":"2018-11-09T13:25:59Z","receivedAt":"2018-11-09T13:26:03Z","isPatch":true,"sender":{"key":"ishchis2@gmail.com","avatar":null},"body":"On Friday, November 9, 2018 3:18:03 AM MST Ævar Arnfjörð Bjarmason wrote:\n> \n> But we should behave consistently with \"diff\" in anticipation of such\n> output being useful in the future, because it would make for confusing\n> UI if two \"diff\" and \"range-diff\" behaved differently when it came to\n 's/ two//'\n\n> how they interpret diff options.\n> \n\n\n"},{"id":"362832","messageId":"nycvar.QRO.7.76.6.1811091731550.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"20181109101803.3038-1-avarab@gmail.com","subject":"Re: [PATCH v4 0/3] range-diff fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-09T16:32:32Z","receivedAt":"2018-11-09T16:32:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Fri, 9 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> Addresses feedback on v3, especially Eric's suggestion to split out\n> the behavior change (which I was not aware of) into a 3/3.\n\nFor the record, I am fine with this iteration, too.\n\nCiao,\nDscho"},{"id":"362930","messageId":"CAPig+cTWgmApseaz4cZqUU4U1NJrcu3vq0FzVwHO6icT7nWcPw@mail.gmail.com","threadId":"49762","inReplyTo":"20181109101803.3038-4-avarab@gmail.com","subject":"Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-11T08:43:50Z","receivedAt":"2018-11-11T08:44:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason wrote:\n> Make the behavior when diff options (e.g. \"--stat\") are passed\n> consistent with how \"diff\" behaves.\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/range-diff.c b/range-diff.c\n> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n> - opts.output_format |= DIFF_FORMAT_PATCH;\n> + if (!opts.output_format)\n> + opts.output_format |= DIFF_FORMAT_PATCH;\n\nI think this can just be '=' now instead of '|=' (to avoid confusing\nthe reader, even if it's functionally equivalent).\n"},{"id":"362956","messageId":"xmqqk1lj3ry1.fsf@gitster-ct.c.googlers.com","threadId":"49762","inReplyTo":"CAPig+cTWgmApseaz4cZqUU4U1NJrcu3vq0FzVwHO6icT7nWcPw@mail.gmail.com","subject":"Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T03:17:26Z","receivedAt":"2018-11-12T03:17:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason wrote:\n>> Make the behavior when diff options (e.g. \"--stat\") are passed\n>> consistent with how \"diff\" behaves.\n>> [...]\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> diff --git a/range-diff.c b/range-diff.c\n>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,\n>> - opts.output_format |= DIFF_FORMAT_PATCH;\n>> + if (!opts.output_format)\n>> + opts.output_format |= DIFF_FORMAT_PATCH;\n>\n> I think this can just be '=' now instead of '|=' (to avoid confusing\n> the reader, even if it's functionally equivalent).\n\nHmph, could the condition in the future change to\n\n\t-\tif (!opts.output_format)\n\t+\tif (! (opts.output_format & DIFF_FORMAT_MASK))\n\t\t\topts.output_format |= DIFF_FORMAT_PATCH\n\nif we ever gain a new \"output_format\" bit that does not affect how\nwe show the diff in a major way, and that is on by default? If so,\nI think \"|=\" is more future-proof. Otherwise, \"=\" is indeed more\nclear way to spell the intention.\n\n\n\n\n\n\n\n\n\n"},{"id":"362957","messageId":"xmqqftw73r9a.fsf@gitster-ct.c.googlers.com","threadId":"49762","inReplyTo":"20181109101803.3038-4-avarab@gmail.com","subject":"Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-12T03:32:17Z","receivedAt":"2018-11-12T03:32:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index ab44e085d5..9352f65280 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff option' '\n> \t1: 4de457d = 1: a4b3333 s/5/A/\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> -\t$four_spaces\n\nThis removes all references to four_spaces=\" \" assigned at the very\nbeginning of this test piece, so that assignment should also go, no?\n\n> \t2: fccce22 = 2: f51d370 s/4/A/\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> -\t$four_spaces\n> \t3: 147e64e ! 3: 0559556 s/11/B/\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> -\t$four_spaces\n> -\t @@ -10,7 +10,7 @@\n> -\t 9\n> -\t 10\n> -\t -11\n> -\t -+B\n> -\t ++BB\n> -\t 12\n> -\t 13\n> -\t 14\n> \t4: a63e992 ! 4: d966c5c s/12/B/\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> -\t$four_spaces\n> -\t @@ -8,7 +8,7 @@\n> -\t @@\n> -\t 9\n> -\t 10\n> -\t - B\n> -\t + BB\n> -\t -12\n> -\t +B\n> -\t 13\n> \tEOF\n> \ttest_cmp expected actual\n> '\n"},{"id":"363194","messageId":"20181113185558.23438-1-avarab@gmail.com","threadId":"49762","inReplyTo":"xmqqftw73r9a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v5 0/3] range-diff fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-13T18:55:55Z","receivedAt":"2018-11-13T18:56:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Trivial updates since v4 addressing the feedback on that\niteration. Hopefully this is the last one, range-diff with the last\nversion:\n\n1: 5399e57513 = 1: f225173f43 range-diff doc: add a section about output stability\n2: e56975df6c = 2: 77804ac641 range-diff: fix regression in passing along diff options\n3: edfef733c7 ! 3: ed67dba073 range-diff: make diff option behavior (e.g. --stat) consistent\n @@ -17,8 +17,8 @@\n \n But we should behave consistently with \"diff\" in anticipation of such\n output being useful in the future, because it would make for confusing\n - UI if two \"diff\" and \"range-diff\" behaved differently when it came to\n - how they interpret diff options.\n + UI if \"diff\" and \"range-diff\" behaved differently when it came to how\n + they interpret diff options.\n \n The new behavior is also consistent with the existing documentation\n added in ba931edd28 (\"range-diff: populate the man page\",\n @@ -36,7 +36,7 @@\n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n -\t\topts.output_format |= DIFF_FORMAT_PATCH;\n +\t\tif (!opts.output_format)\n -+\t\t\topts.output_format |= DIFF_FORMAT_PATCH;\n ++\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\n @@ -45,6 +45,12 @@\n --- a/t/t3206-range-diff.sh\n +++ b/t/t3206-range-diff.sh\n @@\n + '\n + \n + test_expect_success 'changed commit with --stat diff option' '\n +-\tfour_spaces=\" \" &&\n + \tgit range-diff --no-color --stat topic...changed >actual &&\n + \tcat >expected <<-EOF &&\n \t1: 4de457d = 1: a4b3333 s/5/A/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n\nÆvar Arnfjörð Bjarmason (3):\n range-diff doc: add a section about output stability\n range-diff: fix regression in passing along diff options\n range-diff: make diff option behavior (e.g. --stat) consistent\n\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 1 deletion(-)\n\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363195","messageId":"20181113185558.23438-2-avarab@gmail.com","threadId":"49762","inReplyTo":"xmqqftw73r9a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v5 1/3] range-diff doc: add a section about output stability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-13T18:55:56Z","receivedAt":"2018-11-13T18:56:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The range-diff command is already advertised as porcelain, but let's\nmake it really clear that the output is completely subject to change,\nparticularly when it comes to diff options such as --stat. Right now\nthat option doesn't work, but fixing that is the subject of a later\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-range-diff.txt | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\nindex f693930fdb..8a6ea2c6c5 100644\n--- a/Documentation/git-range-diff.txt\n+++ b/Documentation/git-range-diff.txt\n@@ -78,6 +78,23 @@ between patches\", i.e. to compare the author, commit message and diff of\n corresponding old/new commits. There is currently no means to tweak the\n diff options passed to `git log` when generating those patches.\n \n+OUTPUT STABILITY\n+----------------\n+\n+The output of the `range-diff` command is subject to change. It is\n+intended to be human-readable porcelain output, not something that can\n+be used across versions of Git to get a textually stable `range-diff`\n+(as opposed to something like the `--stable` option to\n+linkgit:git-patch-id[1]). There's also no equivalent of\n+linkgit:git-apply[1] for `range-diff`, the output is not intended to\n+be machine-readable.\n+\n+This is particularly true when passing in diff options. Currently some\n+options like `--stat` can, as an emergent effect, produce output\n+that's quite useless in the context of `range-diff`. Future versions\n+of `range-diff` may learn to interpret such options in a manner\n+specific to `range-diff` (e.g. for `--stat` producing human-readable\n+output which summarizes how the diffstat changed).\n \n CONFIGURATION\n -------------\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363196","messageId":"20181113185558.23438-3-avarab@gmail.com","threadId":"49762","inReplyTo":"xmqqftw73r9a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v5 2/3] range-diff: fix regression in passing along diff options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-13T18:55:57Z","receivedAt":"2018-11-13T18:56:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 73a834e9e2 (\"range-diff: relieve callers of low-level configuration\nburden\", 2018-07-22) we broke passing down options like --no-patch,\n--stat etc.\n\nFix that regression, and add a test asserting the pre-73a834e9e2\nbehavior for some of these diff options.\n\nAs noted in a change leading up to this (\"range-diff doc: add a\nsection about output stability\", 2018-11-07) the output is not meant\nto be stable. So this regression test will likely need to be tweaked\nonce we get a \"proper\" --stat option.\n\nSee\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/\nfor a further explanation of the regression. The fix here is not the\nsame as in Johannes's on-list patch, for reasons that'll be explained\nin a follow-up commit.\n\nThe quoting of \"EOF\" here mirrors that of an earlier test. Perhaps\nthat should be fixed, but let's leave that up to a later cleanup\nchange.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n range-diff.c | 2 +-\n t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 3958720f00..ec937008d0 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -461,7 +461,7 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format = DIFF_FORMAT_PATCH;\n+\t\topts.output_format |= DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6aae364171..ab44e085d5 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,59 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with --no-patch diff option' '\n+\tgit range-diff --no-color --no-patch topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'changed commit with --stat diff option' '\n+\tfour_spaces=\" \" &&\n+\tgit range-diff --no-color --stat topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1: 4de457d = 1: a4b3333 s/5/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t2: fccce22 = 2: f51d370 s/4/A/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t3: 147e64e ! 3: 0559556 s/11/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t @@ -10,7 +10,7 @@\n+\t 9\n+\t 10\n+\t -11\n+\t -+B\n+\t ++BB\n+\t 12\n+\t 13\n+\t 14\n+\t4: a63e992 ! 4: d966c5c s/12/B/\n+\t a => b | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t$four_spaces\n+\t @@ -8,7 +8,7 @@\n+\t @@\n+\t 9\n+\t 10\n+\t - B\n+\t + BB\n+\t -12\n+\t +B\n+\t 13\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed commit with sm config' '\n \tgit range-diff --no-color --submodule=log topic...changed >actual &&\n \tcat >expected <<-EOF &&\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363197","messageId":"20181113185558.23438-4-avarab@gmail.com","threadId":"49762","inReplyTo":"xmqqftw73r9a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v5 3/3] range-diff: make diff option behavior (e.g. --stat) consistent","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-13T18:55:58Z","receivedAt":"2018-11-13T18:56:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make the behavior when diff options (e.g. \"--stat\") are passed\nconsistent with how \"diff\" behaves.\n\nBefore 73a834e9e2 (\"range-diff: relieve callers of low-level\nconfiguration burden\", 2018-07-22) running range-diff with \"--stat\"\nwould produce stat output and the diff output, as opposed to how\n\"diff\" behaves where once \"--stat\" is specified \"--patch\" also needs\nto be provided to emit the patch output.\n\nAs noted in a previous change (\"range-diff doc: add a section about\noutput stability\", 2018-11-07) the \"--stat\" output with \"range-diff\"\nis useless at the moment.\n\nBut we should behave consistently with \"diff\" in anticipation of such\noutput being useful in the future, because it would make for confusing\nUI if \"diff\" and \"range-diff\" behaved differently when it came to how\nthey interpret diff options.\n\nThe new behavior is also consistent with the existing documentation\nadded in ba931edd28 (\"range-diff: populate the man page\",\n2018-08-13). See \"[...]also accepts the regular diff options[...]\" in\ngit-range-diff(1).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n range-diff.c | 3 ++-\n t/t3206-range-diff.sh | 23 -----------------------\n 2 files changed, 2 insertions(+), 24 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex ec937008d0..767af8c5bb 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -461,7 +461,8 @@ int show_range_diff(const char *range1, const char *range2,\n \t\tstruct strbuf indent = STRBUF_INIT;\n \n \t\tmemcpy(&opts, diffopt, sizeof(opts));\n-\t\topts.output_format |= DIFF_FORMAT_PATCH;\n+\t\tif (!opts.output_format)\n+\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n \t\topts.flags.suppress_diff_headers = 1;\n \t\topts.flags.dual_color_diffed_diffs = dual_color;\n \t\topts.output_prefix = output_prefix_cb;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex ab44e085d5..e497c1358f 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -134,43 +134,20 @@ test_expect_success 'changed commit with --no-patch diff option' '\n '\n \n test_expect_success 'changed commit with --stat diff option' '\n-\tfour_spaces=\" \" &&\n \tgit range-diff --no-color --stat topic...changed >actual &&\n \tcat >expected <<-EOF &&\n \t1: 4de457d = 1: a4b3333 s/5/A/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n \t2: fccce22 = 2: f51d370 s/4/A/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n \t3: 147e64e ! 3: 0559556 s/11/B/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n-\t @@ -10,7 +10,7 @@\n-\t 9\n-\t 10\n-\t -11\n-\t -+B\n-\t ++BB\n-\t 12\n-\t 13\n-\t 14\n \t4: a63e992 ! 4: d966c5c s/12/B/\n \t a => b | 0\n \t 1 file changed, 0 insertions(+), 0 deletions(-)\n-\t$four_spaces\n-\t @@ -8,7 +8,7 @@\n-\t @@\n-\t 9\n-\t 10\n-\t - B\n-\t + BB\n-\t -12\n-\t +B\n-\t 13\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363374","messageId":"nycvar.QRO.7.76.6.1811141636350.39@tvgsbejvaqbjf.bet","threadId":"49762","inReplyTo":"20181113185558.23438-1-avarab@gmail.com","subject":"Re: [PATCH v5 0/3] range-diff fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T15:36:55Z","receivedAt":"2018-11-14T15:37:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 13 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> Trivial updates since v4 addressing the feedback on that\n> iteration. Hopefully this is the last one, range-diff with the last\n> version:\n\nThis range-diff looks good to me.\n\nThanks,\nDscho\n\n> \n> 1: 5399e57513 = 1: f225173f43 range-diff doc: add a section about output stability\n> 2: e56975df6c = 2: 77804ac641 range-diff: fix regression in passing along diff options\n> 3: edfef733c7 ! 3: ed67dba073 range-diff: make diff option behavior (e.g. --stat) consistent\n> @@ -17,8 +17,8 @@\n> \n> But we should behave consistently with \"diff\" in anticipation of such\n> output being useful in the future, because it would make for confusing\n> - UI if two \"diff\" and \"range-diff\" behaved differently when it came to\n> - how they interpret diff options.\n> + UI if \"diff\" and \"range-diff\" behaved differently when it came to how\n> + they interpret diff options.\n> \n> The new behavior is also consistent with the existing documentation\n> added in ba931edd28 (\"range-diff: populate the man page\",\n> @@ -36,7 +36,7 @@\n> \t\tmemcpy(&opts, diffopt, sizeof(opts));\n> -\t\topts.output_format |= DIFF_FORMAT_PATCH;\n> +\t\tif (!opts.output_format)\n> -+\t\t\topts.output_format |= DIFF_FORMAT_PATCH;\n> ++\t\t\topts.output_format = DIFF_FORMAT_PATCH;\n> \t\topts.flags.suppress_diff_headers = 1;\n> \t\topts.flags.dual_color_diffed_diffs = dual_color;\n> \t\topts.output_prefix = output_prefix_cb;\n> @@ -45,6 +45,12 @@\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@\n> + '\n> + \n> + test_expect_success 'changed commit with --stat diff option' '\n> +-\tfour_spaces=\" \" &&\n> + \tgit range-diff --no-color --stat topic...changed >actual &&\n> + \tcat >expected <<-EOF &&\n> \t1: 4de457d = 1: a4b3333 s/5/A/\n> \t a => b | 0\n> \t 1 file changed, 0 insertions(+), 0 deletions(-)\n> \n> Ævar Arnfjörð Bjarmason (3):\n> range-diff doc: add a section about output stability\n> range-diff: fix regression in passing along diff options\n> range-diff: make diff option behavior (e.g. --stat) consistent\n> \n> Documentation/git-range-diff.txt | 17 +++++++++++++++++\n> range-diff.c | 3 ++-\n> t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++\n> 3 files changed, 49 insertions(+), 1 deletion(-)\n> \n> -- \n> 2.19.1.1182.g4ecb1133ce\n> \n> "}]}