{"thread":{"id":"38438","subject":"[PATCH] diff: make -M -C mean the same as -C -M","startedAt":"2015-01-23T02:07:15Z","lastAt":"2015-01-23T22:34:47Z","messageCount":4,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255161","messageId":"1421978835-9921-1-git-send-email-mh@glandium.org","threadId":"38438","inReplyTo":null,"subject":"[PATCH] diff: make -M -C mean the same as -C -M","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-23T02:07:15Z","receivedAt":"2015-01-23T02:07:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"While -C implies -M, it is quite common to see both on example command lines\nhere and there. The unintuitive thing is that if -M appears after -C, then\ncopy detection is turned off because of how the command line arguments are\nhandled.\n\nChange this so that when both -C and -M appear, whatever their order, copy\ndetection is on.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n\nInterestingly, I even found mentions of -C -M in this order for benchmarks,\non this very list (see 6555655.XSJ9EnW4BY@mako).\n\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1bd534..9081fd8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3670,7 +3670,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\t !strcmp(arg, \"--find-renames\")) {\n \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n \t\t\treturn error(\"invalid argument to -M: %s\", arg+2);\n-\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n+\t\tif (options->detect_rename != DIFF_DETECT_COPY)\n+\t\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n \t}\n \telse if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n \t\toptions->irreversible_delete = 1;\n-- \n2.2.2.2.g806f5e2.dirty\n"},{"id":"255205","messageId":"xmqqr3ulqr09.fsf@gitster.dls.corp.google.com","threadId":"38438","inReplyTo":"1421978835-9921-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH] diff: make -M -C mean the same as -C -M","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-23T18:41:10Z","receivedAt":"2015-01-23T18:41:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> While -C implies -M, it is quite common to see both on example command lines\n> here and there. The unintuitive thing is that if -M appears after -C, then\n> copy detection is turned off because of how the command line arguments are\n> handled.\n\nThis is deliberate, see below.\n\n> Change this so that when both -C and -M appear, whatever their order, copy\n> detection is on.\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>\n> Interestingly, I even found mentions of -C -M in this order for benchmarks,\n> on this very list (see 6555655.XSJ9EnW4BY@mako).\n\nAside from the general warning against taking everything you see on\nthis list as true, I think you are looking at an orange and talking\nabout an apple.  $gmane/244581 is about \"git blame\", and \"-M/-C\"\nover there mean completely different things.\n\nImagine you have a file a/b/c/d/e/f/g/h/, where each alphabet\nrepresents a line with more meaningful contents than a single-liner,\nand slash represents an end of line, and you have changed the\ncontents of the file to e/f/g/h/a/b/c/d/.\n\n\"blame -M\" is to tell the command to notice that you moved the first\nfour lines to the end (i.e. you did not do anything original and\njust moved lines).  Because this needs more processing to notice,\nthe feature is triggered by a \"-M\" option (you would see something\nlike \"e/f/g/h/ came from the original and then a/b/c/d/ are newly\nadded\" without the option).  \"-M\" in \"blame\" is about showing such\nmovement of lines within the same file [*1*].\n\nOn the other hand \"-C\" in \"blame\" is about noticing contents that\nwere copied from other files.\n\nIn the context of \"git blame\", \"-C\" and \"-M\" control orthogonal\nconcepts and it makes sense to use only one but not the other, or\nboth.\n\nBut \"-M\" given to \"git diff\" is not about such an orthogonal\nconcept.\n\nEven though its source candidates are narrower than that of \"-C\" in\nthat \"-M\" chooses only from the set of files that disappeared while\n\"-C\" also chooses from files that remain after the change, they are\nboth about \"which file did the whole thing come from?\", and it makes\nsense to have progression of \"-M\" < \"-C\" < \"-C -C\".  You can think\nof these as a short-hand for --rename-copy-level={0,1,2,3} option\n(where the level 0 -- do not do anything -- corresponds to giving no\n\"-M/-C\").  It can be argued both ways: either we take the maximum of\nthe values given to --rename-copy-level options (which corresponds\nto what your patch attempts to do), or the last one wins.  We happen\nto have implemented the latter long time ago and that is how\nexisting users expect things to work.\n\nSo, I dunno.  I am saying that the semantics the current code gives\nis *not* the only possibly correct one, and I am not saying that\nyour alternative interpretation is *wrong*, but I am not sure if it\nmakes sense to switch the semantics this late in the game.\n\n\n[Footnote]\n\n*1* \"diff\" cannot do anything magical about such a change.  It can\nonly say that you removed the first four lines from the original,\nand then added new four lines at the end (or it may say you added\nnew four lines at the beginning and removed four lines at the end).\n\nThere is no way for \"diff\" to say that these new four lines have any\nrelation to what you removed from the beginning of the file, with\nany combination of options.  This is inherent in its output format,\nwhich is limited to express only deletions and additions.\n\n>  diff.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index d1bd534..9081fd8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3670,7 +3670,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n>  \t\t !strcmp(arg, \"--find-renames\")) {\n>  \t\tif ((options->rename_score = diff_scoreopt_parse(arg)) == -1)\n>  \t\t\treturn error(\"invalid argument to -M: %s\", arg+2);\n> -\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n> +\t\tif (options->detect_rename != DIFF_DETECT_COPY)\n> +\t\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n>  \t}\n>  \telse if (!strcmp(arg, \"-D\") || !strcmp(arg, \"--irreversible-delete\")) {\n>  \t\toptions->irreversible_delete = 1;\n"},{"id":"255231","messageId":"20150123222659.GA22303@glandium.org","threadId":"38438","inReplyTo":"xmqqr3ulqr09.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: make -M -C mean the same as -C -M","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-01-23T22:26:59Z","receivedAt":"2015-01-23T22:26:59Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jan 23, 2015 at 10:41:10AM -0800, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > While -C implies -M, it is quite common to see both on example command lines\n> > here and there. The unintuitive thing is that if -M appears after -C, then\n> > copy detection is turned off because of how the command line arguments are\n> > handled.\n> \n> This is deliberate, see below.\n> \n> > Change this so that when both -C and -M appear, whatever their order, copy\n> > detection is on.\n> >\n> > Signed-off-by: Mike Hommey <mh@glandium.org>\n> > ---\n> >\n> > Interestingly, I even found mentions of -C -M in this order for benchmarks,\n> > on this very list (see 6555655.XSJ9EnW4BY@mako).\n> \n> Aside from the general warning against taking everything you see on\n> this list as true\n\nThe problem is not whether what is on the list is true or not, but that\npeople will use what they see.\n\n> I think you are looking at an orange and talking\n> about an apple.  $gmane/244581 is about \"git blame\", and \"-M/-C\"\n> over there mean completely different things.\n\nI thought blame was using the diff option parser, and I was wrong.\n\n(snip)\n> In the context of \"git blame\", \"-C\" and \"-M\" control orthogonal\n> concepts and it makes sense to use only one but not the other, or\n> both.\n\nIn the context of blame both -C and -M |= a flags set, so one doesn't\noverride the other. You can place them in any order, the result will be\nthe same. Incidentally, -C includes the flag -M sets, so -C -M is\nactually redundant. What -C and -M can be used for is set different\nscores (-C9 -M8).\n\n> But \"-M\" given to \"git diff\" is not about such an orthogonal\n> concept.\n> \n> Even though its source candidates are narrower than that of \"-C\" in\n> that \"-M\" chooses only from the set of files that disappeared while\n> \"-C\" also chooses from files that remain after the change, they are\n> both about \"which file did the whole thing come from?\", and it makes\n> sense to have progression of \"-M\" < \"-C\" < \"-C -C\".  You can think\n> of these as a short-hand for --rename-copy-level={0,1,2,3} option\n> (where the level 0 -- do not do anything -- corresponds to giving no\n> \"-M/-C\").  It can be argued both ways: either we take the maximum of\n> the values given to --rename-copy-level options (which corresponds\n> to what your patch attempts to do), or the last one wins.  We happen\n> to have implemented the latter long time ago and that is how\n> existing users expect things to work.\n\nAre they? I, for one, wasn't. It is easy to understand that --foo=1\n--foo=2 is going to take the last given --foo, but do people really\nexpect the last of -C -M to be used? Reading the code further, I can see\nthat it's even more confusing than that: -C -C wins over -M, wherever it\nhappens. So -C -C -M == -C -C ; -C -M == -M ; -M -C == -C.\n\nAt the very least, this should be spelled out in the documentation.\n\nMike\n"},{"id":"255233","messageId":"xmqqvbjxp1mg.fsf@gitster.dls.corp.google.com","threadId":"38438","inReplyTo":"20150123222659.GA22303@glandium.org","subject":"Re: [PATCH] diff: make -M -C mean the same as -C -M","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-23T22:34:47Z","receivedAt":"2015-01-23T22:34:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n>> In the context of \"git blame\", \"-C\" and \"-M\" control orthogonal\n>> concepts and it makes sense to use only one but not the other, or\n>> both.\n>\n> In the context of blame both -C and -M |= a flags set, so one doesn't\n> override the other. You can place them in any order, the result will be\n> the same. Incidentally, -C includes the flag -M sets, so -C -M is\n> actually redundant. What -C and -M can be used for is set different\n> scores (-C9 -M8).\n\nYes.  That is what I meant by \"orthogonal\" and \"makese sense to use\nonly one but not the other, or both\".  As they are not related with\neach other, it makes sense to mix them freely, unlike \"-C/-M\" given\nto diff.\n"}]}