{"thread":{"id":"50749","subject":"[WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","startedAt":"2019-03-15T16:09:44Z","lastAt":"2019-03-21T11:22:22Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371611","messageId":"87y35g9l18.fsf@evledraar.gmail.com","threadId":"50749","inReplyTo":null,"subject":"[WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T16:09:39Z","receivedAt":"2019-03-15T16:09:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I just submittted\nhttps://public-inbox.org/git/20190315155959.12390-1-avarab@gmail.com/\nand for 8/8 had to tweak the creation factor to 80% due to having added\na large comment. Maybe something like the below makes more sense for\nformat-patch?\n\nAlso, the \"Algorithm\" section of git-range-diff describes how we'll try\nto find a percentage similarity in the *diff*, but in this case I'm\nfairly sure that e.g. a creation factor of 50 would do if it also\nconsidered the commit message.\n\nMaybe I'm just wrong and it does that already, but assuming I'm right in\nmy reading and it doesn't, was that ever considered? That should result\nin fewer false \"not the same patch\" positives, but maybe if patches are\nsplit up it'll screw things up in other ways.\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 1af85d404f5..67a4881a20f 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -261,6 +261,10 @@ material (this may change in the future).\n \tbetween the previous and current series of patches by adjusting the\n \tcreation/deletion cost fudge factor. See linkgit:git-range-diff[1])\n \tfor details.\n++\n+Defaults to 90, whereas the linkgit:git-range-diff[1] default is\n+60. It's assumed that you're submitting a new patch series & that we\n+should try harder than normal to find similarities.\n\n --notes[=<ref>]::\n \tAppend the notes (see linkgit:git-notes[1]) for the commit\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ab859f59041..ff340130826 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1843,7 +1843,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t}\n\n \tif (creation_factor < 0)\n-\t\tcreation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n+\t\tcreation_factor = RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT;\n \telse if (!rdiff_prev)\n \t\tdie(_(\"--creation-factor requires --range-diff\"));\n\ndiff --git a/range-diff.h b/range-diff.h\nindex 08a50b6e98f..634112396d3 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -4,6 +4,7 @@\n #include \"diff.h\"\n\n #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n+#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90\n\n /*\n  * Compare series of commmits in RANGE1 and RANGE2, and emit to the\n"},{"id":"371621","messageId":"CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com","threadId":"50749","inReplyTo":"87y35g9l18.fsf@evledraar.gmail.com","subject":"Re: [WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-15T19:00:56Z","receivedAt":"2019-03-15T19:01:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> @@ -261,6 +261,10 @@ material (this may change in the future).\n> +Defaults to 90, whereas the linkgit:git-range-diff[1] default is\n> +60. It's assumed that you're submitting a new patch series & that we\n> +should try harder than normal to find similarities.\n\nMy understanding was that the primary use-case of git-range-diff\nitself (which existed before the --range-diff option was added to\ngit-format-patch) was to generate a \"range diff\" for a cover letter of\na re-rolled series. So, I'm confused about why this tweaks the default\nvalue of one command but not the other.\n\n> diff --git a/range-diff.h b/range-diff.h\n> @@ -4,6 +4,7 @@\n>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n> +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90\n\nThe point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the\nfirst place was to ensure that the default creation-factor didn't get\nout-of-sync between git-range-diff and git-format-patch., Thus,\nintroducing this sort of inconsistency between the two would likely\nlead to confusion on the part of users. After all, --range-diff was\nadded to git-format-patch merely as a convenience over having to run\ngit-range-diff separately and copy/pasting its output into a cover\nletter generated by git-format-patch. If the two programs default to\ndifferent values, then that \"convenience equality\" breaks down.\n\nSo, I'm fairly negative on this change. However, that doesn't mean I\nwould oppose tweaking the value shared between the two programs (and\nalso the default used by GitGitGadget, if it specifies it manually),\nthough I defer to Dscho in that regard.\n"},{"id":"371623","messageId":"87tvg49c5u.fsf@evledraar.gmail.com","threadId":"50749","inReplyTo":"CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com","subject":"Re: [WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-15T19:21:17Z","receivedAt":"2019-03-15T19:21:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 15 2019, Eric Sunshine wrote:\n\n> On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n>> @@ -261,6 +261,10 @@ material (this may change in the future).\n>> +Defaults to 90, whereas the linkgit:git-range-diff[1] default is\n>> +60. It's assumed that you're submitting a new patch series & that we\n>> +should try harder than normal to find similarities.\n>\n> My understanding was that the primary use-case of git-range-diff\n> itself (which existed before the --range-diff option was added to\n> git-format-patch) was to generate a \"range diff\" for a cover letter of\n> a re-rolled series. So, I'm confused about why this tweaks the default\n> value of one command but not the other.\n>\n>> diff --git a/range-diff.h b/range-diff.h\n>> @@ -4,6 +4,7 @@\n>>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n>> +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90\n>\n> The point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the\n> first place was to ensure that the default creation-factor didn't get\n> out-of-sync between git-range-diff and git-format-patch., Thus,\n> introducing this sort of inconsistency between the two would likely\n> lead to confusion on the part of users. After all, --range-diff was\n> added to git-format-patch merely as a convenience over having to run\n> git-range-diff separately and copy/pasting its output into a cover\n> letter generated by git-format-patch. If the two programs default to\n> different values, then that \"convenience equality\" breaks down.\n>\n> So, I'm fairly negative on this change. However, that doesn't mean I\n> would oppose tweaking the value shared between the two programs (and\n> also the default used by GitGitGadget, if it specifies it manually),\n> though I defer to Dscho in that regard.\n\nI think that was the intention initially, but at least I'm now using\nrange-diff as a general comparison tool of different non-ff-branches,\ne.g. the force-pushes to \"pu\".\n\nI'd expect the tool in general to be used like that, whereas with\nformat-patch it's safe to say we're dealing with a re-roll of the same\nthing.\n\nHence the hypothesis that for format-patch we can be more aggressive\nabout finding similarities.\n"},{"id":"371772","messageId":"xmqqzhps4uyq.fsf@gitster-ct.c.googlers.com","threadId":"50749","inReplyTo":"CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com","subject":"Re: [WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T05:23:09Z","receivedAt":"2019-03-18T05:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> So, I'm fairly negative on this change. However, that doesn't mean I\n> would oppose tweaking the value shared between the two programs (and\n> also the default used by GitGitGadget, if it specifies it manually),\n> though I defer to Dscho in that regard.\n\nI do not think of a good reason why the command should behave\ndifferently only when run from inside format-patch, and if we were\nchanging anything, perhaps raising it a bit for all may make sense.\nI have yet to see range-diff getting confused and matching wrong\npatches but often seee it being too conservative to match two\niterations of the same patch after a moderate amount of update.\n\nI find myself passing \"--creation-factor 99\" or some absurdly\nlooking value when accepting a rerolled series.  The most recent was\nthe updated 'clean-up merge message with scissors' from Denton\n[*1*].\n\n\n[Footnote]\n\n*1* <6716d28a5187c44c1d90f5ce840c44441f62352c.1552275703.git.liu.denton@gmail.com>\nand <08426189b5a29b376581eb0172e52222ab22387a.1552817044.git.liu.denton@gmail.com>\ndo not line up without a high creation factor.\n"},{"id":"372089","messageId":"nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet","threadId":"50749","inReplyTo":"87tvg49c5u.fsf@evledraar.gmail.com","subject":"Re: [WIP PATCH/RFC] Use a higher range-diff --creation-factor for format-patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-21T11:22:02Z","receivedAt":"2019-03-21T11:22:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar and Eric,\n\nOn Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Fri, Mar 15 2019, Eric Sunshine wrote:\n>\n> > On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> >> @@ -261,6 +261,10 @@ material (this may change in the future).\n> >> +Defaults to 90, whereas the linkgit:git-range-diff[1] default is\n> >> +60. It's assumed that you're submitting a new patch series & that we\n> >> +should try harder than normal to find similarities.\n> >\n> > My understanding was that the primary use-case of git-range-diff\n> > itself (which existed before the --range-diff option was added to\n> > git-format-patch) was to generate a \"range diff\" for a cover letter of\n> > a re-rolled series. So, I'm confused about why this tweaks the default\n> > value of one command but not the other.\n> >\n> >> diff --git a/range-diff.h b/range-diff.h\n> >> @@ -4,6 +4,7 @@\n> >>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n> >> +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90\n> >\n> > The point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the\n> > first place was to ensure that the default creation-factor didn't get\n> > out-of-sync between git-range-diff and git-format-patch., Thus,\n> > introducing this sort of inconsistency between the two would likely\n> > lead to confusion on the part of users. After all, --range-diff was\n> > added to git-format-patch merely as a convenience over having to run\n> > git-range-diff separately and copy/pasting its output into a cover\n> > letter generated by git-format-patch. If the two programs default to\n> > different values, then that \"convenience equality\" breaks down.\n> >\n> > So, I'm fairly negative on this change. However, that doesn't mean I\n> > would oppose tweaking the value shared between the two programs (and\n> > also the default used by GitGitGadget, if it specifies it manually),\n> > though I defer to Dscho in that regard.\n\nGitGitGadget does not specify the creation factor manually.\n\n> I think that was the intention initially, but at least I'm now using\n> range-diff as a general comparison tool of different non-ff-branches,\n> e.g. the force-pushes to \"pu\".\n\nInteresting ;-)\n\n> I'd expect the tool in general to be used like that, whereas with\n> format-patch it's safe to say we're dealing with a re-roll of the same\n> thing.\n>\n> Hence the hypothesis that for format-patch we can be more aggressive\n> about finding similarities.\n\nI do agree that `format-patch`'s `--range-diff` is certainly exclusively\nused for comparing different iterations of the same patch series. As such,\nI do agree with Ævar that it makes sense to have a *different* default for\nthe creation factor.\n\nHaving said that, I did notice while porting `tbdiff` to C that it would\nbe a neat idea to put more weight behind the differences of the commit\nmessage, and maybe even use a *different* measure on the commit message,\ntoo. Personally, I would try to use a variation of the word diff\n(maybe something that reflects my experience that it is common to change\nthe case in the oneline, to add a sentence here and there, or to delete a\nsentence, but not so much to replace entire sentences), to account for the\nfact that the commit message rarely changes substantially between\niterations.\n\nSo I guess there is a lot of room for improvement here: code (read: diff)\nchanges simply are not the same as commit message changes.\n\nCiao,\nDscho\n"}]}