{"thread":{"id":"61725","subject":"[PATCH] merge-file: warn for implicit 'myers' algorithm","startedAt":"2024-07-03T14:21:12Z","lastAt":"2024-07-06T18:06:18Z","messageCount":9,"participants":["Antonin Delpeuch via GitGitGadget","Junio C Hamano","Antonin Delpeuch","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498030","messageId":"pull.1741.git.git.1720016469254.gitgitgadget@gmail.com","threadId":"61725","inReplyTo":null,"subject":"[PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Antonin Delpeuch via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-03T14:21:09Z","receivedAt":"2024-07-03T14:21:12Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"From: Antonin Delpeuch \n\nThe current default diff algorithm for the merge-file command is\n'myers', despite the default for the 'ort' strategy being 'histogram'.\nSince 2.44.0 it is possible to specify a different diff algorithm via\nthe --diff-algorithm option. As a preparation for changing the default\nto 'histogram', we warn the user about the different behaviour this\nmay cause.\n\nSigned-off-by: Antonin Delpeuch \n---\n merge-file: warn for implicit 'myers' algorithm\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1741%2Fwetneb%2Fexplicit_diff_algorithm-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1741/wetneb/explicit_diff_algorithm-v1\nPull-Request: https://github.com/git/git/pull/1741\n\n builtin/merge-file.c | 8 ++++++++\n t/t6403-merge-file.sh | 5 +++--\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 1f987334a31..dce2676415e 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -29,6 +29,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int explicit_diff_algorithm = 0;\n+\n static int set_diff_algorithm(xpparam_t *xpp,\n \t\t\t const char *alg)\n {\n@@ -50,6 +52,8 @@ static int diff_algorithm_cb(const struct option *opt,\n \t\treturn error(_(\"option diff-algorithm accepts \\\"myers\\\", \"\n \t\t\t \"\\\"minimal\\\", \\\"patience\\\" and \\\"histogram\\\"\"));\n \n+\texplicit_diff_algorithm = 1;\n+\n \treturn 0;\n }\n \n@@ -103,6 +107,10 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\t\treturn error_errno(\"failed to redirect stderr to /dev/null\");\n \t}\n \n+\tif (!explicit_diff_algorithm) {\n+\t\twarning(_(\"--diff-algorithm not provided, defaulting to \\\"myers\\\". This default will change to \\\"histogram\\\" in a future version.\"));\n+\t}\n+\n \tif (object_id)\n \t\tsetup_git_directory();\n \ndiff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh\nindex fb872c5a113..9d0045be955 100755\n--- a/t/t6403-merge-file.sh\n+++ b/t/t6403-merge-file.sh\n@@ -540,8 +540,9 @@ test_expect_success 'merging C files with \"myers\" diff algorithm creates some sp\n \t}\n \tEOF\n \n-\ttest_must_fail git merge-file -p --diff3 --diff-algorithm myers ours.c base.c theirs.c >myers_output.c &&\n-\ttest_cmp expect.c myers_output.c\n+\ttest_must_fail git merge-file -p --diff3 ours.c base.c theirs.c >myers_output.c 2> err &&\n+\ttest_cmp expect.c myers_output.c &&\n+\tgrep \"diff-algorithm not provided\" err\n '\n \n test_expect_success 'merging C files with \"histogram\" diff algorithm avoids some spurious conflicts' '\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498049","messageId":"xmqqmsmycriv.fsf@gitster.g","threadId":"61725","inReplyTo":"pull.1741.git.git.1720016469254.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T17:30:16Z","receivedAt":"2024-07-03T17:30:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Antonin Delpeuch via GitGitGadget\" writes:\n\n> From: Antonin Delpeuch \n>\n> The current default diff algorithm for the merge-file command is\n> 'myers', despite the default for the 'ort' strategy being 'histogram'.\n\nIt is unclear only from the above description if `ort` should match\nby adopting `myers` as its default, or vice versa. I'll go into\nmore detail why I think this whole thing is done in a wrong order\nlater.\n\n> Since 2.44.0 it is possible to specify a different diff algorithm via\n> the --diff-algorithm option.\n\n\"2.44.0\" -> \"4f7fd79e (merge-file: add --diff-algorithm option,\n2023-11-20)\". Thanks for that patch, by the way.\n\n> As a preparation for changing the default\n> to 'histogram', we warn the user about the different behaviour this\n> may cause.\n\nThere is a huge leap in logic here. Nobody proposed to change the\ndefault and we had no such discussion here. That needs to happen\nbefore anybody can warn users that \"the default will change\".\n\nOnce everybody agrees that such a change is a good idea, we'd need\nto devise transition plan, and one of the tasks _might_ be to add\nthis warning to the code, among other things we may do. The whole\nprocess has to be designed carefully. Having this step as the first\none is way too suboptimal and hurts the users (e.g. \"what if we\ndecide that using histogram as the default is not what we want to do\nin the end?\").\n\n> +static int explicit_diff_algorithm = 0;\n\nWe shouldn't (and we generally do not) initialize a variable\nexplicitly to \"0\". Just let being in the .bss section take care of\nit instead.\n\nThis one is flipped by diff_algorithm_cb(), which is a callback\nfunction that deals with the command line option \"--diff-algorithm\",\nso calling the variable to remember the fact that the option was\nused with \"explicit\" in its name is very much appropriate.\n\nNow on to the real problem I have with this patch.\n\nWhat do you want the end-user experience for those who saw and\nunderstood this warning message to be? Especially for ones who do\nnot really _care_ what the default algorithm used is, and would\nrather tell us \"I'll let Git developers to choose the best algorithm\nfor us, do not bother me with what exact choice you made---I'll\nhappily use the built-in default\")?\n\nWhether they have their favorite algorithm or they are willing to go\nwith the built-in default, they will keep getting shown by this\nmessage and there is no obvious way to easily squelch the message.\n\nIf we do not count \"Every time you run this command, you can give\nthe --diff-algorithm=myers option from the command line to squelch\nit\" as a usable piece of advice, that is.\n\nStepping back a bit.\n\nIt is curious that, even though we read the merge.conflictstyle\nconfiguration variable by calling into xdiff-interface.c, we do not\nseem to pay attention to the diff.algorithm configuration variable.\nShouldn't we teach the command to do so first, as a follow-up to\nyour 4f7fd79e (merge-file: add --diff-algorithm option, 2023-11-20)?\nIf `ort` does not pay attention to it (I do not know if it does),\nthen perhaps that can also be fixed in the same \"preparatory\"\nseries. It would allow us to have a way to consistently and\nuniformly configure the diff algorithm to employ regardless of the\ncaller of the diff machinery (and if we wanted to go fancy, we could\neven introduce \"diff.ort.algorithm\" and \"diff.merge-file.algorithm\"\nthat overrides \"diff.algorithm\" that in turn overrides whatever the\nbuilt-in default is).\n\nSuch a change would prepare the codebase to allow users to say \"I'll\nadopt the new default that will come in Git 3.0 before it happens by\nsetting diff.algorithm to histogram\" or \"I'll set diff.algorithm to\ndefault to express that I'll go with the flow and let Git developers\nto decide\". With such a preparatory change made, you can build an\nequivalent of this step, but you make sure that you pay attention to\nboth the command line (i.e. your explicit_diff_algorithm) and also\nthe configuration as ways for users to express that \"I've read the\nwarning. Please do not repeat it.\"\n\nSo, in short, I do not like this patch because of two reasons:\n\n - It shouldn't be the first message that begins the topic of\n flipping the default diff algorithm for merge-file.\n\n - Even if we arrived at a consensus to migrate the default to\n 'histogram' (and I do not think we even started discussing), the\n \"warning\" mechanism that has no easy way to squelch is not an\n acceptable component in the migration plan.\n\n"},{"id":"498053","messageId":"dd1f768f-a137-428c-8a60-c5e875b66592@delpeuch.eu","threadId":"61725","inReplyTo":"xmqqmsmycriv.fsf@gitster.g","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-07-03T18:28:15Z","receivedAt":"2024-07-03T18:53:07Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"Hi Junio,\n\nI'm really sorry, I thought the switch of default and migration plan had already been agreed on in our discussion of my earlier patch.\nSpecifically, you wrote (https://lore.kernel.org/git/xmqq7cmdpbhq.fsf@gitster.g):\n\n First allow to configure the\n custom algorithm from the command line option (and optionally via a\n configuration variable) and ship it in a release, start giving a\n warning if the using script did not specify the configuration or the\n command line option and used the current default and ship it in the\n next release, wait for a few releases and then finally flip the\n default, or something like that.\n\nSo I thought it would be helpful to follow-up with a patch that implements the approach you outlined.\nBut I totally understand that it might be worth discussing this more.\nActually, I do agree with your assessment that this warning is not great UX.\n\nI think relying on `diff.algorithm` is a natural idea, but it might also be confusing for users.\nAt least to me, the name `diff.algorithm` suggests that it's the algorithm used for \"git diff\",\nbut I might not realize that it also influences how my merges are done.\nIt's probably common to want different algorithms for those situations as they require different speed and accuracy trade-offs.\n\nIn any case, I'm happy to withdraw this patch. Would it be helpful if I start a new thread on the mailing list, independently from this patch, to discuss if and how the default should be switched?\n\nBest,\nAntonin\n\n"},{"id":"498057","messageId":"xmqqr0ca9qkj.fsf@gitster.g","threadId":"61725","inReplyTo":"dd1f768f-a137-428c-8a60-c5e875b66592@delpeuch.eu","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T20:19:08Z","receivedAt":"2024-07-03T20:19:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonin Delpeuch writes:\n\n> I'm really sorry, I thought the switch of default and migration\n> plan had already been agreed on in our discussion of my earlier\n> patch.\n\nAhh, OK.\n\nSo we did some time ago floated the idea. I do not remember how\nwidely accepted the proposal was, though. Having a such reference\nand an explicit mention of what we have and not have yet reached\nconsensus on (either in cover letter or after the three-dash line)\nwould have been very much helpful.\n\n> I think relying on `diff.algorithm` is a natural idea, but it\n> might also be confusing for users. At least to me, the name\n> `diff.algorithm` suggests that it's the algorithm used for \"git\n> diff\", but I might not realize that it also influences how my\n> merges are done. It's probably common to want different\n> algorithms for those situations as they require different speed\n> and accuracy trade-offs.\n\nI never thought that we should get rid of one-off command line\noption. After all, we started with command line option to support\nsuch one-off tweaks in the earlier 4f7fd79e (merge-file: add\n--diff-algorithm option, 2023-11-20) for that exact reason.\n\nThe need to have one-off capability is orthogonal to the need to\nallow users to choose their own default via the configuration. We\nwant to have both, given that some commands other than \"git diff\"\nalready honor the `diff.THING` configuration variables. Doesn't\n\"git log -p\" already pay attention to \"diff.algorithm\" among other\n\"diff.THING\" variables?\n\nA possible downside I had envisioned was that depending on the\napplication (i.e. \"diff\" that produces a patch vs an internal\nimplementation detail of \"merge-file\") the users may want to choose\nthe value of \"diff.THING\" differently. But then we can use the\n\"'diff.THING' is used as the default, but 'diff.frotz.THING', when\ndefined, overrides the choice inside the 'git frotz' program as a\nmore specific configuration\" pattern.\n\nIn any case, honoring things like\n\n [diff] algorithm = default\n [diff \"merge-file\"] algorithm = default\n\nin the configuration file might be a reasonable way out to prepare\nthat users will have a way to squelch the warning messages.\n\nThanks.\n"},{"id":"498110","messageId":"CABPp-BEspjHqNXSAwptgxP059qOFU6MzwAd23-893Nw99ft_Ew@mail.gmail.com","threadId":"61725","inReplyTo":"xmqqr0ca9qkj.fsf@gitster.g","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-07-05T17:39:46Z","receivedAt":"2024-07-05T17:39:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 3, 2024 at 1:24 PM Junio C Hamano wrote:\n>\n> Antonin Delpeuch writes:\n>\n> > I'm really sorry, I thought the switch of default and migration\n> > plan had already been agreed on in our discussion of my earlier\n> > patch.\n>\n> Ahh, OK.\n>\n> So we did some time ago floated the idea. I do not remember how\n> widely accepted the proposal was, though. Having a such reference\n> and an explicit mention of what we have and not have yet reached\n> consensus on (either in cover letter or after the three-dash line)\n> would have been very much helpful.\n\nThere's been a few discussions, the other most recent one I remember\nwas the thread over at\nhttps://lore.kernel.org/git/Y+zzh80fybq8Tn66@coredump.intra.peff.net/.\n(And beyond the git community, there's\nhttps://lkml.org/lkml/2023/5/7/206 in the kernel community, and if\nothers know of discussions in other large developer communities I'd be\ninterested in links.)\n\nThe previous discussions felt to me like we were moving towards\nconsensus, but while I found that encouraging since I think histogram\nwould eventually be a better default, I did not make any actual\nproposals and try to push further towards consensus because there are\na couple known issues that I think should be fixed before we consider\nflipping the default. I have some work-in-progress that was put on\nthe backburner a few years ago that I would like to pick up again, and\nif successful, investigate how much that helps general cases in a\nformat that can help people make educated decisions, and then again\nfloat the idea of changing the default. If consensus is reached, then\nwe'd change the default across the board -- diff/log/merge-file/etc.\nrather than just the somewhat rarely used merge-file. At least,\nthat's my current plan in this area; if others think I should\ninvestigate things in a different order or would like to see\nadditional steps planned into this journey, please do let me know.\n"},{"id":"498121","messageId":"1fd4be07-47ba-4db4-b8aa-860c23b5dd39@delpeuch.eu","threadId":"61725","inReplyTo":"CABPp-BEspjHqNXSAwptgxP059qOFU6MzwAd23-893Nw99ft_Ew@mail.gmail.com","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-07-05T20:14:04Z","receivedAt":"2024-07-05T20:19:31Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"Thanks both.\n\nIt sounds like there is a strong case for at least making \"merge-file\"\nhonour the `diff.algorithm` config variable, possibly with more\nfine-grained config variables as suggested by Junio. I'll try to make a\npatch for that.\n\nElijah, if any of your work on improving the histogram diff can be\nshared (for instance by writing down a description of the issues you'd\nlike to address), I would be interested to have a look and perhaps help\nout if doable. But as far as I can tell there seems to be a lot of fans\nof the existing algorithm already, and such heuristics can never be\nperfect, so hopefully we can make the switch in a not too distant future.\n\nBest,\n\nAntonin\n\n"},{"id":"498142","messageId":"xmqqed873vgn.fsf@gitster.g","threadId":"61725","inReplyTo":"CABPp-BEspjHqNXSAwptgxP059qOFU6MzwAd23-893Nw99ft_Ew@mail.gmail.com","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-06T06:06:48Z","receivedAt":"2024-07-06T06:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> The previous discussions felt to me like we were moving towards\n> consensus, but while I found that encouraging since I think histogram\n> would eventually be a better default, I did not make any actual\n> proposals and try to push further towards consensus because there are\n> a couple known issues that I think should be fixed before we consider\n> flipping the default.\n\nOK.\n\n> I have some work-in-progress that was put on\n> the backburner a few years ago that I would like to pick up again, and\n> if successful, investigate how much that helps general cases in a\n> format that can help people make educated decisions, and then again\n> float the idea of changing the default.\n\nSounds like a good plan.\n\n> If consensus is reached, then\n> we'd change the default across the board -- diff/log/merge-file/etc.\n> rather than just the somewhat rarely used merge-file. \n\nEverybody uses diff.c::diff_algorithm somehow, will be affected by\nthe diff.algorithm configuration variable, and everybody should\nhonor \"--diff-algorithm=\" command line option to override?\n\nIt sounds like a great plan. If ort for some reason performs better\nwith histogram while format-patch works better with minimal, we\ncould extend the configuration system to make diff..algorithm\noverride the more generic diff.algorithm and do similarly silly\nthings. We may want to also teach the diff plumbing commands to\nignore diff.algorithm for better reproducibility but we may need to\nwait for Git 3.0 to do that.\n\n> At least,\n> that's my current plan in this area; if others think I should\n> investigate things in a different order or would like to see\n> additional steps planned into this journey, please do let me know.\n\nThanks.\n"},{"id":"498164","messageId":"7bc2ff20-98b8-45d7-95b8-e1b09bdeda07@delpeuch.eu","threadId":"61725","inReplyTo":"xmqqed873vgn.fsf@gitster.g","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Antonin Delpeuch","fromEmail":"antonin@delpeuch.eu","sentAt":"2024-07-06T13:30:45Z","receivedAt":"2024-07-06T13:30:57Z","isPatch":true,"sender":{"key":"antonin@delpeuch.eu","avatar":"https://avatars.githubusercontent.com/u/309908?v=4"},"body":"On 06/07/2024 08:06, Junio C Hamano wrote:\n> Everybody uses diff.c::diff_algorithm somehow, will be affected by\n> the diff.algorithm configuration variable, and everybody should\n> honor \"--diff-algorithm=\" command line option to override?\n\nI have looked into writing a patch to implement this, but it looks like\nthe situation is quite messy. There are already half a dozen of commands\nwhich claim to honour the diff.algorithm configuration variable, but\nignore it entirely.\n\nIn the documentation of the recursive merge strategy (for instance in\n\"man git-merge\"), it is claimed that \"recursive defaults to the\ndiff.algorithm config setting\". As far as I can tell, both from my\nreading of the code and my interactive testing, this is wrong. This\naffects the \"merge\", \"rebase\" and \"pull\" commands, which all three\nmention this configuration variable in their man page without respecting\nit. Ouch!\n\nI have looked for all commands which mention diff.algorithm in their man\npage and checked whether they indeed respect it. The \"diff-index\",\n\"diff-tree\" and \"diff-files\" commands also make this erroneous claim.\nThe --diff-algorithm CLI option (as well as the --histogram and\nsiblings) are respected, but not the diff.algorithm config variable.\nThose inconsistencies seem to be caused by the inclusion of the\n`diff-options.txt` file in the man pages, which leads their man pages to\ndocumenting a bunch of config variables which are in fact ignored. From\na user perspective, I would say that those commands should indeed honour\ndiff.algorithm, so I would be tempted to fix the code rather than the\ndocumentation. That being said, the man pages of those commands also\nmention other options which are in fact ignored, such as\n\"diff.relative\". I think it's likely that for some of those variables,\nthe contrary is desirable: remove them from the docs because they indeed\nshouldn't be relied on by the command (because it does not make sense\nfor that particular command). I don't have (yet) a good enough\nunderstanding of those commands and those options to judge this\nimmediately, but it looks like there is some cleaning up to do.\n\nIn diff.c, the code that is responsible for reading diff.* configuration\nvariables for commands like \"log\", \"diff\" or \"diff-index\" divides diff.*\nvariables into two categories: the ones that are \"basic\" (parsed by\n\"git_diff_basic_config\") and the ones only relevant to the \"ui\" (parsed\nby the \"git_diff_ui_config\" function). Surprisingly to me, the\n\"diff.algorithm\" variable belongs to the \"ui\" category, and is therefore\nskipped by commands such as \"diff-index\". It seems natural to me to move\ndiff.algorithm to the \"basic\" section. However, that alone will not fix\nthe problem (in my opinion much more serious) that the recursive merge\nstrategy ignores the variable. To fix this, the parsing of this variable\ncould either be added to \"merge_recursive_config\" (in\nmerge-recursive.c), or in fact directly to \"git_xmerge_config\" (in\nxdiff-interface.c), which would then not only fix the recursive merge\nstrategy, but also a range of other commands such as \"rerere\" or\n\"merge-file\". Given that I started looking into this with a specific\ninterest in \"merge-file\", I would obviously be tempted to fix this bug\ndirectly at the xmerge level, and I think it would indeed make sense for\nother affected commands (surely \"rerere\" should benefit from using merge\noptions that are consistent with the ones used for \"git merge\" or \"git\nrebase\", no?), but I can imagine that it's too bold a step and could\nhave unwanted consequences I am not aware of - especially since Junio\nrecommended to add support for diff.algorithm at the diff.c level. What\ndo you think?\n\nIn any case, I would of course make sure the \"ort\" strategy continues to\nignore diff.algorithm for now, given its current default value.\n\nIf you want to try this out for yourself, I have set up a test\nrepository at https://git.kanthaus.online/antonin/testrepo. It has two\nbranches \"master\" and \"theirs\", which you can try to merge/rebase, for\ninstance \"git merge -s recursive theirs\" while being on master and\nhaving \"diff.algorithm\" set to \"histogram\". The merge scenario is\ndesigned to fail with a conflict if the myers algorithm is used, and\nsucceed if histogram is used. You should be able to see that \"git merge\n-s recursive theirs\" will fail but \"git merge -s recursive\n-Xdiff-algorithm=histogram theirs\" should work.\n\nBest,\n\nAntonin\n\n"},{"id":"498170","messageId":"xmqqzfqupf8u.fsf@gitster.g","threadId":"61725","inReplyTo":"7bc2ff20-98b8-45d7-95b8-e1b09bdeda07@delpeuch.eu","subject":"Re: [PATCH] merge-file: warn for implicit 'myers' algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-06T18:06:09Z","receivedAt":"2024-07-06T18:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antonin Delpeuch writes:\n\n> In the documentation of the recursive merge strategy (for instance in\n> \"man git-merge\"), it is claimed that \"recursive defaults to the\n> diff.algorithm config setting\". As far as I can tell, both from my\n> reading of the code and my interactive testing, this is wrong. This\n> affects the \"merge\", \"rebase\" and \"pull\" commands, which all three\n> mention this configuration variable in their man page without respecting\n> it. Ouch!\n\nThanks for digging.\n\n> I have looked for all commands which mention diff.algorithm in their man\n> page and checked whether they indeed respect it. The \"diff-index\",\n> \"diff-tree\" and \"diff-files\" commands also make this erroneous claim.\n\nIt is not erroneous if we say that these 3 diff plumbing commands\nignore the configuration variable. They should ignore end-user\nconfiguration for reproducibility.\n\nSee also my earlier response to Elijah \non a related topic.\n\n> The --diff-algorithm CLI option (as well as the --histogram and\n> siblings) are respected, but not the diff.algorithm config variable.\n\nThen they are behaving exactly as designed, which is good. We still\nneed to correct their documentation, though.\n\n> Those inconsistencies seem to be caused by the inclusion of the\n> `diff-options.txt` file in the man pages, which leads their man pages to\n> documenting a bunch of config variables which are in fact ignored.\n\nThat is quite understandable mistake ;-) \"git merge\" and other\nend-user facing commands should be taught to pay attention to both\nthe command line option and the configuration variable. The\nplumbing commands should pay attention to the command line only.\n\n> In any case, I would of course make sure the \"ort\" strategy continues to\n> ignore diff.algorithm for now, given its current default value.\n\nIt may make the effort easy to follow if you do this step-wise:\n\n (1) start with \"all Porcelain commands pay attention to the same\n diff.algorithm variable. The plumbing commands ignore\n diff.algorithm. All commands, either Porcelain or plumbing,\n may have different default when unconfigured (like ort does)\".\n\n (2) then add \"each Porcelain command pays attention to\n diff..algorithm if defined, otherwise diff.algorithm is\n used as a fallback default. There is no diff..algorithm\n for plumbing commands---they are designed not to be affected by\n the configuration variables\".\n\n (3) optionally, doing \"all Porcelain commands, when not configured,\n will use the same default (ort is no longer special---everybody\n falls back to algorithm X)\" may be desiable for consistency and\n simplicity, but it would probably want further discussion can\n be left outside of the topic (e.g. right now the best candidate\n for X may be histogram, but is it suitable for all commands?\n should this extend to plumbing, making diff-index for example\n to use X as the default not myers when the command line does\n not specify --diff-algorithm?)\n\nThanks.\n\n"}]}