{"thread":{"id":"53549","subject":"[BUG] diff algorithm selection issue","startedAt":"2020-05-26T09:37:30Z","lastAt":"2020-05-26T18:33:22Z","messageCount":6,"participants":["ydirson@free.fr","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"398574","messageId":"540830445.1034524732.1590485817611.JavaMail.root@zimbra39-e7","threadId":"53549","inReplyTo":"408624876.1034463388.1590484997966.JavaMail.root@zimbra39-e7","subject":"[BUG] diff algorithm selection issue","fromName":"","fromEmail":"ydirson@free.fr","sentAt":"2020-05-26T09:36:57Z","receivedAt":"2020-05-26T09:37:30Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Hi all,\n\nWhen the config has diff.algorithm=patience set, \"git diff --minimal\" seems to\nbe ignored, and does not give the same output as \"git diff --diff-algorithm=minimal\",\nbut really the same as \"git diff --diff-algorithm=patience\".\n\nSee top commit on https://gitlab.com/ydirson/omaha/-/commits/bug/git-diff-minimal\nwhere patience diff manages to show a quite braindead diff.\n\nAlso, I found it curious that in the manpage the long (--diff-algorithm=minimal) and\nshort (--minimal) forms are described with a copy-paste of the text, and no mention\nof them being equivalent (or not, for that matter ;).\n"},{"id":"398583","messageId":"xmqqh7w2oexd.fsf@gitster.c.googlers.com","threadId":"53549","inReplyTo":"540830445.1034524732.1590485817611.JavaMail.root@zimbra39-e7","subject":"Re: [BUG] diff algorithm selection issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-26T16:10:22Z","receivedAt":"2020-05-26T16:10:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ydirson@free.fr writes:\n\n> When the config has diff.algorithm=patience set, \"git diff --minimal\" seems to\n> be ignored, and does not give the same output as \"git diff --diff-algorithm=minimal\",\n> but really the same as \"git diff --diff-algorithm=patience\".\n\nThanks for reporting.  The document on --diff-algorithm does make it\nsound as if the --diff-algorithm=minimal option must operate as if\nmyers algorithm is used with the minumalization tweak, but that is\nwrong from the point of view of the intent of the \"minimal\" option,\nwhich was meant to be a secondary option that tweaks the base\nalgorithm (be it myers or patience or any other new algorithm we\nmight introduce in the future) by allowing it to spend more cycles\nto come up with a smaller diff.\n\nAt least, it is what the \"--minimal\" option (not the value \"minimal\"\ngiven to the \"--diff-algorithm=<algo>\" option), and the underlying\nmechanism that supports the option, meant to do.\n\nBut the way the option is surfaced to the end-user facing UI (and\nthe documentation) with \"--diff-algorithm=minimal\", it does look\nlike \n\n\tgit -c diff.algorithm=patience cmd --diff-algorithm=minimal\n\tgit -c diff.algorithm=patience cmd --diff-algorithm=myers --minimal\n\nought to be equivalent.\n\nAlso, I suspect that\n\n\tgit -c diff.algorithm=patience cmd --diff-algorithm=myers\n\ndoes not do what we expect, either.\n\nI have not convinced myself that the attached is the best way to fix\nthe issue, but anyway, somebody seems to be OR'ing in diff_algorithm\nto xdl_opts field after we see --diff-algorithm=minimal and replaced\nXDF_DIFF_ALGORITHM_MASK bits in xdl_opts field in this function, so\nthe attached patch may defeat that code---the real bug is probably in\nthat code, but I haven't figured out where it is X-<.\n\n diff.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 863da896c0..a6dba45cd6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5026,6 +5026,10 @@ static int diff_opt_diff_algorithm(const struct option *opt,\n \t\treturn error(_(\"option diff-algorithm accepts \\\"myers\\\", \"\n \t\t\t       \"\\\"minimal\\\", \\\"patience\\\" and \\\"histogram\\\"\"));\n \n+\t/* we shouldn't have to do this... */\n+\tif (!(value & XDF_DIFF_ALGORITHM_MASK))\n+\t\tdiff_algorithm &= ~XDF_DIFF_ALGORITHM_MASK;\n+\n \t/* clear out previous settings */\n \tDIFF_XDL_CLR(options, NEED_MINIMAL);\n \toptions->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;\n"},{"id":"398584","messageId":"xmqqd06qoec9.fsf@gitster.c.googlers.com","threadId":"53549","inReplyTo":"xmqqh7w2oexd.fsf@gitster.c.googlers.com","subject":"Re: [BUG] diff algorithm selection issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-26T16:23:02Z","receivedAt":"2020-05-26T16:23:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> the issue, but anyway, somebody seems to be OR'ing in diff_algorithm\n> to xdl_opts field after we see --diff-algorithm=minimal and replaced\n> XDF_DIFF_ALGORITHM_MASK bits in xdl_opts field in this function, ...\n\nWell, I am not so sure about that diagnosis, actually.  I should\nprobably have taken a bit of caffeine before writing my e-mails.\n\nThere is no patch needed. --- I simply had misread your report.\n\n> When the config has diff.algorithm=patience set, \"git diff --minimal\" seems to\n> be ignored, and does not give the same output as \"git diff --diff-algorithm=minimal\",\n> but really the same as \"git diff --diff-algorithm=patience\".\n\nAs I wrote, that is absolutely the intended behaviour.  \n\nWhen patience and other algorithm learns how to trade cycles off\nwith output size, --minimal may make a difference, but unlike\n\"--diff-algorithm=minimal\" that forces Myers algorithm, the\n\"--minimal\" option should not change the underlying algorithm.\n\nThanks.\n"},{"id":"398585","messageId":"253380167.1036247407.1590512216359.JavaMail.root@zimbra39-e7","threadId":"53549","inReplyTo":"xmqqd06qoec9.fsf@gitster.c.googlers.com","subject":"Re: [BUG] diff algorithm selection issue","fromName":"","fromEmail":"ydirson@free.fr","sentAt":"2020-05-26T16:56:56Z","receivedAt":"2020-05-26T16:57:29Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Hi Junio,\n\n> > When the config has diff.algorithm=patience set, \"git diff\n> > --minimal\" seems to\n> > be ignored, and does not give the same output as \"git diff\n> > --diff-algorithm=minimal\",\n> > but really the same as \"git diff --diff-algorithm=patience\".\n> \n> As I wrote, that is absolutely the intended behaviour.\n> \n> When patience and other algorithm learns how to trade cycles off\n> with output size, --minimal may make a difference, but unlike\n> \"--diff-algorithm=minimal\" that forces Myers algorithm, the\n> \"--minimal\" option should not change the underlying algorithm.\n\nOK, so then the problem is just in the doc, where --diff-algorithm=minimal\nshould rather be documented as something like:\n\n The basic greedy \"myers\" diff algorithm, spending extra time to make\n sure the smallest possible diff is produced (equivalent to\n `--diff-algorithm=myers --minimal`).\n\nOr is it rather intended to be `--diff-algorithm=default --minimal`,\nwhatever the default may be in the future ?\n\n\nAs for the other flags, --patience and --histogram should probably\ndocumented as backward-compatibility aliases for --diff-algorithm=, right ?\n\n\nI'll send a formal patch if all of this sounds good.\n\n\nAnd as a last point, there would be the problem shown by patience diff\non the commit referenced in my original post - If patience is still\nconsidered for promotion to default some day, I guess we wouldn't want\nit to make such a bad choice.\n\nBest regards,\n-- \nYann\n"},{"id":"398589","messageId":"505382066.1036329918.1590513687177.JavaMail.root@zimbra39-e7","threadId":"53549","inReplyTo":"253380167.1036247407.1590512216359.JavaMail.root@zimbra39-e7","subject":"Re: [BUG] diff algorithm selection issue","fromName":"","fromEmail":"ydirson@free.fr","sentAt":"2020-05-26T17:21:27Z","receivedAt":"2020-05-26T17:21:30Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"\n> > > When the config has diff.algorithm=patience set, \"git diff\n> > > --minimal\" seems to\n> > > be ignored, and does not give the same output as \"git diff\n> > > --diff-algorithm=minimal\",\n> > > but really the same as \"git diff --diff-algorithm=patience\".\n> > \n> > As I wrote, that is absolutely the intended behaviour.\n> > \n> > When patience and other algorithm learns how to trade cycles off\n> > with output size, --minimal may make a difference, but unlike\n> > \"--diff-algorithm=minimal\" that forces Myers algorithm, the\n> > \"--minimal\" option should not change the underlying algorithm.\n> \n> OK, so then the problem is just in the doc, where\n> --diff-algorithm=minimal\n> should rather be documented as something like:\n> \n>  The basic greedy \"myers\" diff algorithm, spending extra time to make\n>  sure the smallest possible diff is produced (equivalent to\n>  `--diff-algorithm=myers --minimal`).\n> \n> Or is it rather intended to be `--diff-algorithm=default --minimal`,\n> whatever the default may be in the future ?\n\nNow that I think about it, do we really want \"minimal\" as a valid choice\nfor --diff-algorithm, if it's not a choice of algorithm ?\n\nIn that case, we may need a separate config option line diff.algorithm.minimal\nor maybe diff.algorithm.tuning ?\n\n> \n> \n> As for the other flags, --patience and --histogram should probably\n> documented as backward-compatibility aliases for --diff-algorithm=,\n> right ?\n> \n> \n> I'll send a formal patch if all of this sounds good.\n> \n> \n> And as a last point, there would be the problem shown by patience\n> diff\n> on the commit referenced in my original post - If patience is still\n> considered for promotion to default some day, I guess we wouldn't\n> want\n> it to make such a bad choice.\n> \n> Best regards,\n> --\n> Yann\n> \n"},{"id":"398590","messageId":"xmqq5zcio8b5.fsf@gitster.c.googlers.com","threadId":"53549","inReplyTo":"505382066.1036329918.1590513687177.JavaMail.root@zimbra39-e7","subject":"Re: [BUG] diff algorithm selection issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-26T18:33:18Z","receivedAt":"2020-05-26T18:33:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ydirson@free.fr writes:\n\n>> Or is it rather intended to be `--diff-algorithm=default --minimal`,\n>> whatever the default may be in the future ?\n>\n> Now that I think about it, do we really want \"minimal\" as a valid choice\n> for --diff-algorithm, if it's not a choice of algorithm ?\n\nI asked the same question to myself before writing my first response\nbut it is way too late to ask it now.  I do support the spirit of\n\"--minimal\", but not \"--diff-algorithm=minimal\" which may be an\nill-thought-out shorthand for \"--diff-algorithm=default --minimal\".\n\nIf you dig the list archive, I would imagine that you would find\nthat not an insignificant part of list participant back then thought\nit was a good idea ;-)\n\n\n"}]}