Message ID | 87y35g9l18.fsf@evledraar.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [WIP,PATCH/RFC] Use a higher range-diff --creation-factor for format-patch | expand |
On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > @@ -261,6 +261,10 @@ material (this may change in the future). > +Defaults to 90, whereas the linkgit:git-range-diff[1] default is > +60. It's assumed that you're submitting a new patch series & that we > +should try harder than normal to find similarities. My understanding was that the primary use-case of git-range-diff itself (which existed before the --range-diff option was added to git-format-patch) was to generate a "range diff" for a cover letter of a re-rolled series. So, I'm confused about why this tweaks the default value of one command but not the other. > diff --git a/range-diff.h b/range-diff.h > @@ -4,6 +4,7 @@ > #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 > +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90 The point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the first place was to ensure that the default creation-factor didn't get out-of-sync between git-range-diff and git-format-patch., Thus, introducing this sort of inconsistency between the two would likely lead to confusion on the part of users. After all, --range-diff was added to git-format-patch merely as a convenience over having to run git-range-diff separately and copy/pasting its output into a cover letter generated by git-format-patch. If the two programs default to different values, then that "convenience equality" breaks down. So, I'm fairly negative on this change. However, that doesn't mean I would oppose tweaking the value shared between the two programs (and also the default used by GitGitGadget, if it specifies it manually), though I defer to Dscho in that regard.
On Fri, Mar 15 2019, Eric Sunshine wrote: > On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt >> @@ -261,6 +261,10 @@ material (this may change in the future). >> +Defaults to 90, whereas the linkgit:git-range-diff[1] default is >> +60. It's assumed that you're submitting a new patch series & that we >> +should try harder than normal to find similarities. > > My understanding was that the primary use-case of git-range-diff > itself (which existed before the --range-diff option was added to > git-format-patch) was to generate a "range diff" for a cover letter of > a re-rolled series. So, I'm confused about why this tweaks the default > value of one command but not the other. > >> diff --git a/range-diff.h b/range-diff.h >> @@ -4,6 +4,7 @@ >> #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 >> +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90 > > The point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the > first place was to ensure that the default creation-factor didn't get > out-of-sync between git-range-diff and git-format-patch., Thus, > introducing this sort of inconsistency between the two would likely > lead to confusion on the part of users. After all, --range-diff was > added to git-format-patch merely as a convenience over having to run > git-range-diff separately and copy/pasting its output into a cover > letter generated by git-format-patch. If the two programs default to > different values, then that "convenience equality" breaks down. > > So, I'm fairly negative on this change. However, that doesn't mean I > would oppose tweaking the value shared between the two programs (and > also the default used by GitGitGadget, if it specifies it manually), > though I defer to Dscho in that regard. I think that was the intention initially, but at least I'm now using range-diff as a general comparison tool of different non-ff-branches, e.g. the force-pushes to "pu". I'd expect the tool in general to be used like that, whereas with format-patch it's safe to say we're dealing with a re-roll of the same thing. Hence the hypothesis that for format-patch we can be more aggressive about finding similarities.
Eric Sunshine <sunshine@sunshineco.com> writes: > So, I'm fairly negative on this change. However, that doesn't mean I > would oppose tweaking the value shared between the two programs (and > also the default used by GitGitGadget, if it specifies it manually), > though I defer to Dscho in that regard. I do not think of a good reason why the command should behave differently only when run from inside format-patch, and if we were changing anything, perhaps raising it a bit for all may make sense. I have yet to see range-diff getting confused and matching wrong patches but often seee it being too conservative to match two iterations of the same patch after a moderate amount of update. I find myself passing "--creation-factor 99" or some absurdly looking value when accepting a rerolled series. The most recent was the updated 'clean-up merge message with scissors' from Denton [*1*]. [Footnote] *1* <6716d28a5187c44c1d90f5ce840c44441f62352c.1552275703.git.liu.denton@gmail.com> and <08426189b5a29b376581eb0172e52222ab22387a.1552817044.git.liu.denton@gmail.com> do not line up without a high creation factor.
Hi Ævar and Eric, On Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Mar 15 2019, Eric Sunshine wrote: > > > On Fri, Mar 15, 2019 at 12:09 PM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > >> @@ -261,6 +261,10 @@ material (this may change in the future). > >> +Defaults to 90, whereas the linkgit:git-range-diff[1] default is > >> +60. It's assumed that you're submitting a new patch series & that we > >> +should try harder than normal to find similarities. > > > > My understanding was that the primary use-case of git-range-diff > > itself (which existed before the --range-diff option was added to > > git-format-patch) was to generate a "range diff" for a cover letter of > > a re-rolled series. So, I'm confused about why this tweaks the default > > value of one command but not the other. > > > >> diff --git a/range-diff.h b/range-diff.h > >> @@ -4,6 +4,7 @@ > >> #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 > >> +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90 > > > > The point of introducing RANGE_DIFF_CREATION_FACTOR_DEFAULT in the > > first place was to ensure that the default creation-factor didn't get > > out-of-sync between git-range-diff and git-format-patch., Thus, > > introducing this sort of inconsistency between the two would likely > > lead to confusion on the part of users. After all, --range-diff was > > added to git-format-patch merely as a convenience over having to run > > git-range-diff separately and copy/pasting its output into a cover > > letter generated by git-format-patch. If the two programs default to > > different values, then that "convenience equality" breaks down. > > > > So, I'm fairly negative on this change. However, that doesn't mean I > > would oppose tweaking the value shared between the two programs (and > > also the default used by GitGitGadget, if it specifies it manually), > > though I defer to Dscho in that regard. GitGitGadget does not specify the creation factor manually. > I think that was the intention initially, but at least I'm now using > range-diff as a general comparison tool of different non-ff-branches, > e.g. the force-pushes to "pu". Interesting ;-) > I'd expect the tool in general to be used like that, whereas with > format-patch it's safe to say we're dealing with a re-roll of the same > thing. > > Hence the hypothesis that for format-patch we can be more aggressive > about finding similarities. I do agree that `format-patch`'s `--range-diff` is certainly exclusively used for comparing different iterations of the same patch series. As such, I do agree with Ævar that it makes sense to have a *different* default for the creation factor. Having said that, I did notice while porting `tbdiff` to C that it would be a neat idea to put more weight behind the differences of the commit message, and maybe even use a *different* measure on the commit message, too. Personally, I would try to use a variation of the word diff (maybe something that reflects my experience that it is common to change the case in the oneline, to add a sentence here and there, or to delete a sentence, but not so much to replace entire sentences), to account for the fact that the commit message rarely changes substantially between iterations. So I guess there is a lot of room for improvement here: code (read: diff) changes simply are not the same as commit message changes. Ciao, Dscho
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 1af85d404f5..67a4881a20f 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -261,6 +261,10 @@ material (this may change in the future). between the previous and current series of patches by adjusting the creation/deletion cost fudge factor. See linkgit:git-range-diff[1]) for details. ++ +Defaults to 90, whereas the linkgit:git-range-diff[1] default is +60. It's assumed that you're submitting a new patch series & that we +should try harder than normal to find similarities. --notes[=<ref>]:: Append the notes (see linkgit:git-notes[1]) for the commit diff --git a/builtin/log.c b/builtin/log.c index ab859f59041..ff340130826 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1843,7 +1843,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) } if (creation_factor < 0) - creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT; + creation_factor = RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT; else if (!rdiff_prev) die(_("--creation-factor requires --range-diff")); diff --git a/range-diff.h b/range-diff.h index 08a50b6e98f..634112396d3 100644 --- a/range-diff.h +++ b/range-diff.h @@ -4,6 +4,7 @@ #include "diff.h" #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +#define RANGE_DIFF_CREATION_FACTOR_FORMAT_PATCH_DEFAULT 90 /* * Compare series of commmits in RANGE1 and RANGE2, and emit to the