| Message ID | xmqqbk5i3ncw.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c22d41d641711879c57299244ae13b6c4a215fee |
| Headers | show |
| Series | Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hints | expand |
Hello Junio, On 2024-05-06 18:40, Junio C Hamano wrote: > So what's the conclusion of this discussion (have we reached one > yet)? > > to me it sounds like everybody is on board to raise the default used > for format-patch. In the old thread I said "I use something > unreasonably high like 9999", but my "unreasonably high" number > these days is 999, which was used in the earlier weatherbaloon > patch. The old thread ended with an academic "we know different > defaults are appropriate, but what is the right number then?" but > anything higher (like 999 or even 9999) is better than the default > for range-diff which is 60, I would think. > > Here is the patch, this time with a bit of documentation stolen from > the old weather-balloon by Ævar. After going through all the linked discussions, I think that introducing the new default value for the creation factor in "git format-patch" is the way to go. However, I still have one remaining concern. See, the creation factor is described in the documentation as some fudge factor, specified as a percentage. Without going through the actual source code in detail, a question that pops up in my mind is why do we need to use 999 or 9999 as the new default value? Shouldn't 99 or 100 be good enough instead, if it's a percentage? I'd assume that the same question might be asked by other Git users. Admittedly, I haven't produced numerous range diffs, but for what I did, 90 worked very well as the creation factor value. I even created some aliases that specify 90 as the creation factor value. > ----- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] format-patch: run range-diff with larger > creation-factor > > We see too often that a range-diff added to format-patch output > shows too many "unmatched" patches. This is because the default > value for creation-factor is set to a relatively low value. > > It may be justified for other uses (like you have a yet-to-be-sent > new iteration of your series, and compare it against the 'seen' > branch that has an older iteration, probably with the '--left-only' > option, to pick out only your patches while ignoring the others) of > "range-diff" command, but when the command is run as part of the > format-patch, the user _knows_ and expects that the patches in the > old and the new iterations roughly correspond to each other, so we > can and should use a much higher default. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/git-format-patch.txt | 5 +++++ > builtin/log.c | 2 +- > range-diff.h | 6 ++++++ > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-format-patch.txt > b/Documentation/git-format-patch.txt > index 728bb3821c..b72f87b114 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -346,6 +346,11 @@ 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 999 (the linkgit:git-range-diff[1] uses 60), as the use > +case is to show comparison with an older iteration of the same > +topic and the tool should find more correspondence between the two > +sets of patches. > > --notes[=<ref>]:: > --no-notes:: > diff --git a/builtin/log.c b/builtin/log.c > index c0a8bb95e9..73608ffef9 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -2274,7 +2274,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 = CREATION_FACTOR_FOR_THE_SAME_SERIES; > else if (!rdiff_prev) > die(_("the option '%s' requires '%s'"), "--creation-factor", > "--range-diff"); > > diff --git a/range-diff.h b/range-diff.h > index 04ffe217be..2f69f6a434 100644 > --- a/range-diff.h > +++ b/range-diff.h > @@ -6,6 +6,12 @@ > > #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 > > +/* > + * A much higher value than the default, when we KNOW we are comparing > + * the same series (e.g., used when format-patch calls range-diff). > + */ > +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999 > + > struct range_diff_options { > int creation_factor; > unsigned dual_color:1;
Dragan Simic <dsimic@manjaro.org> writes: > See, the creation factor is described in the documentation as some fudge > factor, specified as a percentage. Without going through the actual > source code in detail, a question that pops up in my mind is why do we > need to use 999 or 9999 as the new default value? Shouldn't 99 or 100 > be good enough instead, if it's a percentage? I'd assume that the same > question might be asked by other Git users. It is very much deliberate to choose a value beyond 100. Setting the value to such a large value was designed to force somebody to ask that question: "what does the value mean?" [*], and in a sense it already has achieved one half of its objective ;-). We never had an satisfactory update to the range-diff documentation when we discussed this parameter in the past. It was stated that the unit to express the value for creation-factor was called "percent", but it seems that nobody had a good explanation that can be given to a layperson what that percent means (e.g., what is it relative to? what it means to have that value at 100% as opposed to say 50%?). Somebody should come up with an easy-to-understand explanation suitable for users after reading and digesting the overly technical "algorithm" section of the manual, and forcing ourselves to do a good job at it is the other half of its objective. [Footnote] * Having more than 100 as a value that is measured in "percent" is not unusual, by the way. You can zoom into a picture at magnification level of 400%, for example.
On 2024-05-07 02:21, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> See, the creation factor is described in the documentation as some >> fudge >> factor, specified as a percentage. Without going through the actual >> source code in detail, a question that pops up in my mind is why do we >> need to use 999 or 9999 as the new default value? Shouldn't 99 or 100 >> be good enough instead, if it's a percentage? I'd assume that the >> same >> question might be asked by other Git users. > > It is very much deliberate to choose a value beyond 100. Setting > the value to such a large value was designed to force somebody to > ask that question: "what does the value mean?" [*], and in a sense > it already has achieved one half of its objective ;-). Ah, I see, it's also meant to provoke users' curiosity a bit. :) > We never had an satisfactory update to the range-diff documentation > when we discussed this parameter in the past. It was stated that > the unit to express the value for creation-factor was called > "percent", but it seems that nobody had a good explanation that can > be given to a layperson what that percent means (e.g., what is it > relative to? what it means to have that value at 100% as opposed to > say 50%?). Somebody should come up with an easy-to-understand > explanation suitable for users after reading and digesting the > overly technical "algorithm" section of the manual, and forcing > ourselves to do a good job at it is the other half of its objective. Yes, the whole percentage thing is a bit unclear. I'm going through the ALGORITHM section of the git-range-diff(1) man page, which could also use some wording and formatting improvements. I'll see to send a patch that improves it a bit later. > [Footnote] > > * Having more than 100 as a value that is measured in "percent" is > not unusual, by the way. You can zoom into a picture at > magnification level of 400%, for example. Of course. For example, I use 120% as the default scaling in my web browser, to compensate for the rather high value of the screen DPI.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 728bb3821c..b72f87b114 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -346,6 +346,11 @@ 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 999 (the linkgit:git-range-diff[1] uses 60), as the use +case is to show comparison with an older iteration of the same +topic and the tool should find more correspondence between the two +sets of patches. --notes[=<ref>]:: --no-notes:: diff --git a/builtin/log.c b/builtin/log.c index c0a8bb95e9..73608ffef9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -2274,7 +2274,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 = CREATION_FACTOR_FOR_THE_SAME_SERIES; else if (!rdiff_prev) die(_("the option '%s' requires '%s'"), "--creation-factor", "--range-diff"); diff --git a/range-diff.h b/range-diff.h index 04ffe217be..2f69f6a434 100644 --- a/range-diff.h +++ b/range-diff.h @@ -6,6 +6,12 @@ #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +/* + * A much higher value than the default, when we KNOW we are comparing + * the same series (e.g., used when format-patch calls range-diff). + */ +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999 + struct range_diff_options { int creation_factor; unsigned dual_color:1;