Re: [PATCH] merge-file: warn for implicit 'myers' algorithm

[Junio C Hamano <gitster@pobox.com>]

"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> The current default diff algorithm for the merge-file command is
> 'myers', despite the default for the 'ort' strategy being 'histogram'.

It is unclear only from the above description if `ort` should match
by adopting `myers` as its default, or vice versa.  I'll go into
more detail why I think this whole thing is done in a wrong order
later.

> Since 2.44.0 it is possible to specify a different diff algorithm via
> the --diff-algorithm option.

"2.44.0" -> "4f7fd79e (merge-file: add --diff-algorithm option,
2023-11-20)".  Thanks for that patch, by the way.

> As a preparation for changing the default
> to 'histogram', we warn the user about the different behaviour this
> may cause.

There is a huge leap in logic here.  Nobody proposed to change the
default and we had no such discussion here.  That needs to happen
before anybody can warn users that "the default will change".

Once everybody agrees that such a change is a good idea, we'd need
to devise transition plan, and one of the tasks _might_ be to add
this warning to the code, among other things we may do.  The whole
process has to be designed carefully.  Having this step as the first
one is way too suboptimal and hurts the users (e.g. "what if we
decide that using histogram as the default is not what we want to do
in the end?").

> +static int explicit_diff_algorithm = 0;

We shouldn't (and we generally do not) initialize a variable
explicitly to "0".  Just let being in the .bss section take care of
it instead.

This one is flipped by diff_algorithm_cb(), which is a callback
function that deals with the command line option "--diff-algorithm",
so calling the variable to remember the fact that the option was
used with "explicit" in its name is very much appropriate.

Now on to the real problem I have with this patch.

What do you want the end-user experience for those who saw and
understood this warning message to be?  Especially for ones who do
not really _care_ what the default algorithm used is, and would
rather tell us "I'll let Git developers to choose the best algorithm
for us, do not bother me with what exact choice you made---I'll
happily use the built-in default")?

Whether they have their favorite algorithm or they are willing to go
with the built-in default, they will keep getting shown by this
message and there is no obvious way to easily squelch the message.

If we do not count "Every time you run this command, you can give
the --diff-algorithm=myers option from the command line to squelch
it" as a usable piece of advice, that is.

Stepping back a bit.

It is curious that, even though we read the merge.conflictstyle
configuration variable by calling into xdiff-interface.c, we do not
seem to pay attention to the diff.algorithm configuration variable.
Shouldn't we teach the command to do so first, as a follow-up to
your 4f7fd79e (merge-file: add --diff-algorithm option, 2023-11-20)?
If `ort` does not pay attention to it (I do not know if it does),
then perhaps that can also be fixed in the same "preparatory"
series.  It would allow us to have a way to consistently and
uniformly configure the diff algorithm to employ regardless of the
caller of the diff machinery (and if we wanted to go fancy, we could
even introduce "diff.ort.algorithm" and "diff.merge-file.algorithm"
that overrides "diff.algorithm" that in turn overrides whatever the
built-in default is).

Such a change would prepare the codebase to allow users to say "I'll
adopt the new default that will come in Git 3.0 before it happens by
setting diff.algorithm to histogram" or "I'll set diff.algorithm to
default to express that I'll go with the flow and let Git developers
to decide".  With such a preparatory change made, you can build an
equivalent of this step, but you make sure that you pay attention to
both the command line (i.e. your explicit_diff_algorithm) and also
the configuration as ways for users to express that "I've read the
warning.  Please do not repeat it."

So, in short, I do not like this patch because of two reasons:

 - It shouldn't be the first message that begins the topic of
   flipping the default diff algorithm for merge-file.

 - Even if we arrived at a consensus to migrate the default to
   'histogram' (and I do not think we even started discussing), the
   "warning" mechanism that has no easy way to squelch is not an
   acceptable component in the migration plan.