Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 3, 2024, 19:26 UTC
Message-ID: <CAPig+cRZEQjwhnMYvQ_0BjvXaR=3x=LQeJHzFOxC+ZPyOy9yJQ@mail.gmail.com>
In-Reply-To: <xmqqjzka7p2t.fsf_-_@gitster.g>

On Fri, May 3, 2024 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 32 quoted lines

> Dragan Simic <dsimic@manjaro.org> writes:
> > Just a small suggestion...  Perhaps the creation factor needs adjusting
> > for the range diff to actually be produced. [...]
>
> I see this happen to too many series I see on the list.  There are
> cases when the user knows that they are comparing an old and a new
> iterations of the same series, e.g. running it from format-patch.
> We probably should use a much higher creation factor than default to
> run range-diff in such a context.
>
> IOW, this shouldn't have to be done by individual users, but by the
> tool.
>
> Perhaps something along this line may not be a bad idea.
>
> ----- >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>

Ævar had posted[1] pretty much the exact same patch a few years ago. At the time, I had trouble understanding why `git range-diff` and `git format-patch --range-dif` would need separate default creation factors[2], and I still have trouble understanding why they should be different. Aren't both commands addressing the same use-case of comparing one version of a series against a subsequent version? In your response[3], you seemed to agree with that observation and suggested instead simply increasing the default creation factor for both commands (which sounds reasonable to me).

[1]: https://lore.kernel.org/git/87y35g9l18.fsf@evledraar.gmail.com/ [2]: https://lore.kernel.org/git/CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com/ [3]: https://lore.kernel.org/git/xmqqzhps4uyq.fsf@gitster-ct.c.googlers.com/

Show 23 quoted lines

> ---
> diff --git c/builtin/log.c w/builtin/log.c
> index 4da7399905..7a019476c3 100644
> --- c/builtin/log.c
> +++ w/builtin/log.c
> @@ -2294,7 +2294,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 c/range-diff.h w/range-diff.h
> index 04ffe217be..2f69f6a434 100644
> --- c/range-diff.h
> +++ w/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