"Antonin Delpeuch via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Antonin Delpeuch <antonin@xxxxxxxxxxx> > > 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.
