{"thread":{"id":"49721","subject":"Using --word-diff breaks --color-moved","startedAt":"2018-10-31T02:06:02Z","lastAt":"2018-11-02T20:46:17Z","messageCount":7,"participants":["james harvey","Junio C Hamano","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"362049","messageId":"CA+X5Wn76N34oBhRZvXKOwP0L_pF=LYbT6ugTgtPYSvnHg=MZVw@mail.gmail.com","threadId":"49721","inReplyTo":null,"subject":"Using --word-diff breaks --color-moved","fromName":"james harvey","fromEmail":"jamespharvey20@gmail.com","sentAt":"2018-10-31T02:05:48Z","receivedAt":"2018-10-31T02:06:02Z","isPatch":false,"sender":{"key":"jamespharvey20@gmail.com","avatar":null},"body":"If you use both \"--word-diff\" and \"--color-moved\", regardless of the\norder of arguments, \"--word-diff\" takes precedence and \"--color-moved\"\nisn't allowed to do anything.\n\nI think \"--color-moved\" should have precedence over \"--word-diff\".  I\ncannot think of a scenario where a user would supply both options, and\nactually want \"--word-diff\" to take precedence.  If I'm not thinking\nof a scenario where this wouldn't be desired, perhaps whichever is\nfirst as an argument could take precedence.\n\n(The same behavior happens if 4+ lines are moved and\n\"--color-moved{default=zebra}\" is used, but below\n\"--color-moved=plain\" is used to be a smaller testcase.)\n\nGiven the following setup:\n\n$ cat << EOF > file\n> a\n> b\n> c\n> EOF\n$ git add file\n$ git commit -m \"Added file.\"\n$ cat << EOF > file\n> b\n> c\n> a\n> EOF\n\nYou can have moved lines colorization:\n$ git diff --color-moved=plain\n...\n[oldMovedColor]-a\nb\nc\n[newMovedColor]+a\n\nYou can diff based on words:\n$ git diff --word-diff\n...\n[oldColor][-a-]\nb\nc\n[newColor][+a+}\n\nBut, you cannot diff based on words, and have moved lines colorization:\n$ git diff --color-moved=plain --word-diff\n$ git diff --word-diff\n...\n[oldColor][-a-]\nb\nc\n[newColor][+a+}\n"},{"id":"362061","messageId":"xmqqlg6eyc3u.fsf@gitster-ct.c.googlers.com","threadId":"49721","inReplyTo":"CA+X5Wn76N34oBhRZvXKOwP0L_pF=LYbT6ugTgtPYSvnHg=MZVw@mail.gmail.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T04:27:33Z","receivedAt":"2018-10-31T04:27:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"james harvey <jamespharvey20@gmail.com> writes:\n\n> If you use both \"--word-diff\" and \"--color-moved\", regardless of the\n> order of arguments, \"--word-diff\" takes precedence and \"--color-moved\"\n> isn't allowed to do anything.\n>\n> I think \"--color-moved\" should have precedence over \"--word-diff\".  I\n> cannot think of a scenario where a user would supply both options, and\n> actually want \"--word-diff\" to take precedence.\n\nI am not sure if I follow.  If these two cannot work well together,\nthen we should just reject the request as asking for incompatible\ncombination of options while we are parsing the command line\narguments, rather than arguing which one should trump the other\none---that would simply lead to \"in my opinion, word-diff is more\nimportant\" vs \"in mine, color-moved is more important\", no?\n\n"},{"id":"362081","messageId":"CA+X5Wn6vGLiQr8O8UVkJnFpVq+OVzOjtxhtqwtZEsyh0RrSz9w@mail.gmail.com","threadId":"49721","inReplyTo":"xmqqlg6eyc3u.fsf@gitster-ct.c.googlers.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"james harvey","fromEmail":"jamespharvey20@gmail.com","sentAt":"2018-10-31T07:07:43Z","receivedAt":"2018-10-31T07:07:57Z","isPatch":false,"sender":{"key":"jamespharvey20@gmail.com","avatar":null},"body":"On Wed, Oct 31, 2018 at 12:27 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> james harvey <jamespharvey20@gmail.com> writes:\n>\n> > If you use both \"--word-diff\" and \"--color-moved\", regardless of the\n> > order of arguments, \"--word-diff\" takes precedence and \"--color-moved\"\n> > isn't allowed to do anything.\n> >\n> > I think \"--color-moved\" should have precedence over \"--word-diff\".  I\n> > cannot think of a scenario where a user would supply both options, and\n> > actually want \"--word-diff\" to take precedence.\n>\n> I am not sure if I follow.  If these two cannot work well together,\n> then we should just reject the request as asking for incompatible\n> combination of options while we are parsing the command line\n> arguments, rather than arguing which one should trump the other\n> one---that would simply lead to \"in my opinion, word-diff is more\n> important\" vs \"in mine, color-moved is more important\", no?\n\nI should have been more clear in my original message.  I don't mean\nthat if \"--color-moved\" is given, that the argument \"--word-diff\"\nshould be completely ignored as if it weren't given as an option.\n\nI'm not too concerned about my reduced test case scenario.  I'm\nconcerned about a larger diff, where there's some areas that got\nmoved, some lines that got deleted, some added, and some lines with\njust a word or two changed.\n\nIn those larger scenarios, WITHOUT using BOTH \"--color-moved\" and\n\"--word-diff\", and INSTEAD just using \"git diff --color-moved\", a\ntypical full line(s) diff occurs for changed areas that weren't moved,\nas if it were given as a hidden/default option.  It's analyzing each\ndiffering area to see if it's going to show each of those differing\nareas as a move or a full line(s) diff.  Here, \"--color-moved\" takes\nprecedence (in the way I'm trying to use the word) over the typical\nfull line(s) diff.\n\nI could be wrong, but I don't see why \"--color-moved\" can't operate\nthe same way, with \"--word-diff\" taking the place of the typical full\nline(s) diff.  So, if it would be technically accurate to show\nsomething that was moved using either method, that it would show moved\nareas as a move rather than as word-diffs.  This would leave areas not\nmoved to be word-diffed.\n\nI think these options can co-exist.  I could be wrong, but I'm betting\nthe code for \"--color-moved\" was only written with the typical full\nline(s) diff in mind, and wasn't written with \"--word-diff\" in mind.\n"},{"id":"362117","messageId":"CAGZ79kZ6LxRevLy2mZd1Ag=oO_NtDdmRSuadswR_n=RGpO=rGQ@mail.gmail.com","threadId":"49721","inReplyTo":"CA+X5Wn76N34oBhRZvXKOwP0L_pF=LYbT6ugTgtPYSvnHg=MZVw@mail.gmail.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-31T17:41:52Z","receivedAt":"2018-10-31T17:42:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 30, 2018 at 7:06 PM james harvey <jamespharvey20@gmail.com> wrote:\n>\n> If you use both \"--word-diff\" and \"--color-moved\", regardless of the\n> order of arguments, \"--word-diff\" takes precedence and \"--color-moved\"\n> isn't allowed to do anything.\n\nThe order of arguments doesn't matter here, as these just set internal\nflags at parse time, which determine what later stages do.\n\nGit uses the xdiff library internally for producing diffs[1].\nTo produce a diff, we have to feed two \"streams of symbols\"\nto the library which then figures out the diff.\nUsually a symbol is a whole line. Once we have the diff\nwe need to make it look nice again (i.e. put file names,\ncontext markers and lines around the diff), which happens\nin diff.c.\n\nBut when --word-diff is given, each line is broken up\ninto words and those are used as symbols for the finding\nthe diff[2]. See the function fn_out_consume() [3],\nfor example 'ecbdata->diff_words' is set on '--word-diff'.\n\nWhen it is not set we fall down to the switch case that\nwill call emit_{add, del, context}_line(), which in turn\nemits the lines.\nThe --color-moved step is performed after all diffing\n(and nicing up) is done already and solely works on\nthe add/del lines. The word diff is piecing together lines\nfor output, which are completely ignored for move\ndetection.\n\n[1] see the xdiff/ dir in your copy of git. We have some\n    substantial changes compared to unmaintained upstream\n    http://www.xmailserver.org/xdiff-lib.html\n    http://www.xmailserver.org/xdiff.html\n\n[2] https://github.com/git/git/blob/master/diff.c#L1872\n\n[3] https://github.com/git/git/blob/master/diff.c#L2259\n\n> I think \"--color-moved\" should have precedence over \"--word-diff\".\n\nI agree for precedence as in \"work well together\". Now we'd need\nto figure out what that means. In its current form, the move\ndetection can detect moved lines across diff hunks or file\nboundaries.\n\nShould that also be the case for word diffing?\nI think word diffing is mostly used for free text, which has different\nproperties compared to code, that the color-moved was originally\nintended for.\n\nFor example in code we often have few characters on a line\nsuch as \"<TAB> }\" which is found often in gits code base.\nWe added some heuristics that lines showing up often with\nfew characters would not be detected on their own as a moved\nblock [4]. I would expect we'd have to figure out a similar heuristic\nfor word diffing, if we go down that route.\n\nBut that is a detail; we'd first have to figure out how to make the\nwords work with the move detection.\n\n[4] https://github.com/git/git/commit/f0b8fb6e591b50b72b921f2c4cf120ebd284f510\n\n\n>   I\n> cannot think of a scenario where a user would supply both options, and\n> actually want \"--word-diff\" to take precedence.  If I'm not thinking\n> of a scenario where this wouldn't be desired, perhaps whichever is\n> first as an argument could take precedence.\n\nword diffing and move detection are completely orthogonal at the moment.\nInstead of option order, I'd rather introduce a new option that tells us\nhow to resolve some corner case. Or in the short term we might just\nwant to raise an error?\n\n> (The same behavior happens if 4+ lines are moved and\n> \"--color-moved{default=zebra}\" is used, but below\n> \"--color-moved=plain\" is used to be a smaller testcase.)\n>\n> [...]\n\nThis sounds like you are asking for two things:\n(1) make color-moved work with words (somehow)\n(2) allow the user to fine tune the heuristics for a block,\n    such that default=zebra would still work.\n"},{"id":"362118","messageId":"CAGZ79kY+QVN4P2NhuOfDdXH1yHZfOjuA-H6FXS91jundhgo_2A@mail.gmail.com","threadId":"49721","inReplyTo":"CA+X5Wn6vGLiQr8O8UVkJnFpVq+OVzOjtxhtqwtZEsyh0RrSz9w@mail.gmail.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-31T17:43:38Z","receivedAt":"2018-10-31T17:43:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 31, 2018 at 12:07 AM james harvey <jamespharvey20@gmail.com> wrote:\n\n> I think these options can co-exist.  I could be wrong, but I'm betting\n> the code for \"--color-moved\" was only written with the typical full\n> line(s) diff in mind, and wasn't written with \"--word-diff\" in mind.\n\nI think it was brought up, but neglected at the time.\n"},{"id":"362222","messageId":"CA+X5Wn6nP5sZh8Z2dcTZKpPBWG43nSUATcWx0fzp2dNTJ3UzQQ@mail.gmail.com","threadId":"49721","inReplyTo":"CAGZ79kZ6LxRevLy2mZd1Ag=oO_NtDdmRSuadswR_n=RGpO=rGQ@mail.gmail.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"james harvey","fromEmail":"jamespharvey20@gmail.com","sentAt":"2018-11-02T01:18:58Z","receivedAt":"2018-11-02T01:19:12Z","isPatch":false,"sender":{"key":"jamespharvey20@gmail.com","avatar":null},"body":"On Wed, Oct 31, 2018 at 1:42 PM Stefan Beller <sbeller@google.com> wrote:\n>\n> On Tue, Oct 30, 2018 at 7:06 PM james harvey <jamespharvey20@gmail.com> wrote:\n> > I think \"--color-moved\" should have precedence over \"--word-diff\".\n>\n> I agree for precedence as in \"work well together\". Now we'd need\n> to figure out what that means. In its current form, the move\n> detection can detect moved lines across diff hunks or file\n> boundaries.\n>\n> Should that also be the case for word diffing?\n> I think word diffing is mostly used for free text, which has different\n> properties compared to code, that the color-moved was originally\n> intended for.\n\nThat's how I think of it too.  I think I'd be fine if word diffing\nstayed not being able to be detected with moved lines across diff\nhunks or file boundaries.\n\n> >   I\n> > cannot think of a scenario where a user would supply both options, and\n> > actually want \"--word-diff\" to take precedence.  If I'm not thinking\n> > of a scenario where this wouldn't be desired, perhaps whichever is\n> > first as an argument could take precedence.\n>\n> word diffing and move detection are completely orthogonal at the moment.\n> Instead of option order, I'd rather introduce a new option that tells us\n> how to resolve some corner case. Or in the short term we might just\n> want to raise an error?\n\nI'm fine with option order not mattering, as it does now.  Was\nassuming it didn't matter now, but mentioned trying it in case it\nworked that way.  And, mentioned it as an alternative in case it\nturned out the two could conflict in some corner case.  I think\ndefaulting to resolving one way or the other with an optional option\nto go the other way makes sense.\n\n> > (The same behavior happens if 4+ lines are moved and\n> > \"--color-moved{default=zebra}\" is used, but below\n> > \"--color-moved=plain\" is used to be a smaller testcase.)\n> >\n> > [...]\n>\n> This sounds like you are asking for two things:\n> (1) make color-moved work with words (somehow)\n> (2) allow the user to fine tune the heuristics for a block,\n>     such that default=zebra would still work.\n\nI was asking for #1.  #2 might be a good idea, but I just tried using\n\"--color-moved\" for the first time the other day, so haven't used it\nenough to get that far.  If they worked together, I'm not sure yet if\nI'd be using plain or zebra.  I mentioned \"4+ lines\" because I can\nremember something said zebra only worked with more than 3 lines.  Not\nsure where that was.  I thought it was the manpage, but I'm not seeing\nthat in there now.\n"},{"id":"362314","messageId":"CAGZ79kasAsUNbNKXtexZDiZtQtFXP5EODLaquwvf7BN_JSiEPA@mail.gmail.com","threadId":"49721","inReplyTo":"CA+X5Wn6nP5sZh8Z2dcTZKpPBWG43nSUATcWx0fzp2dNTJ3UzQQ@mail.gmail.com","subject":"Re: Using --word-diff breaks --color-moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-02T20:46:02Z","receivedAt":"2018-11-02T20:46:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 1, 2018 at 6:19 PM james harvey <jamespharvey20@gmail.com> wrote:\n\n> > This sounds like you are asking for two things:\n> > (1) make color-moved work with words (somehow)\n> > (2) allow the user to fine tune the heuristics for a block,\n> >     such that default=zebra would still work.\n>\n> I was asking for #1.\n\nI currently have no time to look into that,\nbut you're welcome to do so. :-)\nI'd be happy to review the patches!\n\nThanks,\nStefan\n"}]}