{"thread":{"id":"54630","subject":"git format-patch --range-diff bug?","startedAt":"2020-11-12T17:43:59Z","lastAt":"2020-11-12T20:06:24Z","messageCount":5,"participants":["SZEDER Gábor","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409776","messageId":"20201112174353.GD4270@szeder.dev","threadId":"54630","inReplyTo":null,"subject":"git format-patch --range-diff bug?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-11-12T17:43:53Z","receivedAt":"2020-11-12T17:43:59Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"I've just run into an unexpected looking range-diff generated by 'git\nformat-patch -1 --range-diff=...'. While the 'git range-diff' command\ncorrectly pairs up the old and new versions of the commit, 'git\nformat-patch --range-diff' somehow doesn't and reports it in a weird\nway.\n\nThe test below demonstrates the issue, though it doesn't fail but only\nprints the outputs of those commands:\n\n --- >8 ---\n\n#!/bin/sh\n\ntest_description='test'\n\n. ./test-lib.sh\n\ntest_expect_success 'test' '\n\ttest_tick &&\n\tprintf \"%s\\n\" 0 1 2 3 4 5 6 7 8 9 >file &&\n\tgit add file &&\n\tgit commit -m initial &&\n\n\tgit checkout -b v1 &&\n\techo foo >>file &&\n\tgit commit -m change file &&\n\n\tgit checkout -b v2 master &&\n\techo bar >>file &&\n\tgit commit -m change file &&\n\n\t: The output of the range-diff command is what I would expect to be included in the generated patch &&\n\tgit range-diff v1...v2 &&\n\n\t: But that is not what we get embedded in the generated patch &&\n\tgit format-patch -1 --range-diff=v1...v2 v2 &&\n\tsed -n -e \"/^Range-diff:$/,/^$/ p\" 0001-change.patch &&\n\n\t: Adding the range-diff to the cover letter doesnt seem to help &&\n\tgit format-patch -1 --range-diff=v1...v2 --cover-letter v2 &&\n\tsed -n -e \"/^Range-diff:$/,$ p\" 0000-cover-letter.patch\n'\ntest_done\n\n --- 8< ---\n\nAnd here is the relevant part of the output:\n\n+ : The output of the range-diff command is what I would expect to be\nincluded in the generated patch\n+ git range-diff v1...v2\n1: f8c2d0b ! 1: 83c34a8 change\n @@ -9,4 +9,4 @@\n 7\n 8\n 9\n -+foo\n ++bar\n+ : But that is not what we get embedded in the generated patch\n+ git format-patch -1 --range-diff=v1...v2 v2\n0001-change.patch\n+ sed -n -e /^Range-diff:$/,/^$/ p 0001-change.patch\nRange-diff:\n1: 83c34a8 = 1: 83c34a8 change\n2: f8c2d0b < -: ------- change\n\n+ : Adding the range-diff to the cover letter doesnt seem to help\n+ git format-patch -1 --range-diff=v1...v2 --cover-letter v2\n0000-cover-letter.patch\n0001-change.patch\n+ sed -n -e /^Range-diff:$/,$ p 0000-cover-letter.patch\nRange-diff:\n1: 83c34a8 = 1: 83c34a8 change\n2: f8c2d0b < -: ------- change\n-- \n2.18.0.584.g40ce41604d\n\n"},{"id":"409778","messageId":"xmqqk0uqjw4l.fsf@gitster.c.googlers.com","threadId":"54630","inReplyTo":"20201112174353.GD4270@szeder.dev","subject":"Re: git format-patch --range-diff bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-12T17:57:46Z","receivedAt":"2020-11-12T17:57:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> \t: The output of the range-diff command is what I would expect to be included in the generated patch &&\n> \tgit range-diff v1...v2 &&\n>\n> \t: But that is not what we get embedded in the generated patch &&\n> \tgit format-patch -1 --range-diff=v1...v2 v2 &&\n\nThe other day I did something similar and ended up with\n\n\tgit format-patch --range-diff=v1 v1..v2\n\nWould it help not to use the three-dot form? From my reading of\n\"git format-patch --range-diff=\" description, it only\nneeds to give a single range (i.e. previous side of series of\ncommits) as the other range to be compared are by definition the\npatches you are producing, while v1...v2 syntax is to give two\nranges with one option. So...\n"},{"id":"409782","messageId":"CAPig+cT1zDT1iqRqO-4U8Rwq7p=MFm5Bkn990AVbxMHqp=knmg@mail.gmail.com","threadId":"54630","inReplyTo":"xmqqk0uqjw4l.fsf@gitster.c.googlers.com","subject":"Re: git format-patch --range-diff bug?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-12T18:32:42Z","receivedAt":"2020-11-12T18:32:56Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 12, 2020 at 12:57 PM Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> > git range-diff v1...v2 &&\n> > git format-patch -1 --range-diff=v1...v2 v2 &&\n>\n> The other day I did something similar and ended up with\n>\n> git format-patch --range-diff=v1 v1..v2\n>\n> Would it help not to use the three-dot form? From my reading of\n> \"git format-patch --range-diff=\" description, it only\n> needs to give a single range (i.e. previous side of series of\n> commits) as the other range to be compared are by definition the\n> patches you are producing, while v1...v2 syntax is to give two\n> ranges with one option. So...\n\nIndeed. It's not clear (at least to me) how git-format-patch's\n--range-diff option should interpret a 3-dot range, thus the\nimplementation explicitly supports only a single revision or a 2-dot\nrange, and the documentation reflects this.\n\nIt turns out, as you discovered, that the implementation is a bit\ndeficient in that it doesn't outright reject a 3-dot range, and\ninstead gets tripped up by:\n\n int prev_is_range = !!strstr(prev, \"..\");\n\nwhich accidentally also matches a 3-dot range, with the result that\nthe 3-dot range gets passed into the lower-level range-diff machinery\nwhich then forwards the 3-dot range to git-log. The git-range-diff\ncommand, on the other hand, interprets a 3-dot range manually so such\na range doesn't make it down to the lower-level range-diff machinery.\n\nI haven't fully thought it through yet, but at this point, the best\n\"fix\" likely would be for `git format-patch --range-diff` to error out\nwhen it sees a 3-dot range. (Unless there is some other intuitive\ninterpretation of a 3-dot range which escapes me.)\n"},{"id":"409787","messageId":"xmqqblg2jt95.fsf@gitster.c.googlers.com","threadId":"54630","inReplyTo":"CAPig+cT1zDT1iqRqO-4U8Rwq7p=MFm5Bkn990AVbxMHqp=knmg@mail.gmail.com","subject":"Re: git format-patch --range-diff bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-12T18:59:50Z","receivedAt":"2020-11-12T19:00:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I haven't fully thought it through yet, but at this point, the best\n> \"fix\" likely would be for `git format-patch --range-diff` to error out\n> when it sees a 3-dot range. (Unless there is some other intuitive\n> interpretation of a 3-dot range which escapes me.)\n\nAnd possibly rename the option. Giving \"--range-diff=\" is not\nan instruction to run \"git range-diff \", so it is clear that\nthe option is misnamed.\n\nIt probably should have been \"--[no-]range-diff\" boolean that\ncontrols if we add the range-diff from the previous, whose default\nmay be affected by the user of the \"-v$n\" option, plus another\noption that gives where to find the \"previous series\", whose\npresence probably trigger \"--range-diff\" implicitly, or something\nlike that.\n\nAnd the option whose value we are having problem with is exactly\nthat \"--previous-series=\" option.\n"},{"id":"409790","messageId":"CAPig+cTeiLDFQD6HDhr-or4F+2POfsMkQ2f0H5ipvi7DCkmLRw@mail.gmail.com","threadId":"54630","inReplyTo":"xmqqblg2jt95.fsf@gitster.c.googlers.com","subject":"Re: git format-patch --range-diff bug?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-12T20:06:08Z","receivedAt":"2020-11-12T20:06:24Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 12, 2020 at 2:00 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > [...] the best \"fix\" likely would be for `git format-patch --range-diff` to\n> > error out when it sees a 3-dot range.\n>\n> And possibly rename the option. Giving \"--range-diff=\" is not\n> an instruction to run \"git range-diff \", so it is clear that\n> the option is misnamed.\n>\n> It probably should have been \"--[no-]range-diff\" boolean that\n> controls if we add the range-diff from the previous, whose default\n> may be affected by the user of the \"-v$n\" option, plus another\n> option that gives where to find the \"previous series\", whose\n> presence probably trigger \"--range-diff\" implicitly, or something\n> like that.\n>\n> And the option whose value we are having problem with is exactly\n> that \"--previous-series=\" option.\n\nYou may be right that, due to the name similarity, some people\nmisinterpret --range-diff as providing one-to-one parity with the `git\nrange-diff` command, but that was never the intention (as was\ndiscussed during review). Instead, the intention all along was only to\nmake it easy to embed range-diffs in `git format-patch` output with a\nsimple and concise invocation, and only for common cases. For\ninstance, it is quite common for v2 to share a common base with v1, in\nwhich case `git format-patch -v2 --range-diff=v1 ` would be\nsufficient (when v2 is the checked-out branch). Anything more complex\ncan be achieved by utilizing `git range-diff` directly; for instance:\n`git range-diff {complexity} >>0000-cover-letter.patch`\n\nAs discussed during review, I had considered more complex invocation\npossibilities for `git format-patch` but ended up rejecting them and\nopting instead for simplicity since `git range-diff` itself is a\nsuitable escape hatch for accomplishing complex cases not handled by\nthe intentionally simplistic --range-diff option. Thus, I'm hesitant\nto go in a direction which adds more complexity to the `git\nformat-patch` invocation, thus making it more difficult to use\nrange-diff with `git format-patch`.\n"}]}