{"thread":{"id":"34188","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","startedAt":"2013-06-18T06:31:46Z","lastAt":"2013-06-18T22:47:22Z","messageCount":5,"participants":["Jeff King","Thomas Rast","Mark Abraham"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221148","messageId":"20130618063144.GA6276@sigill.intra.peff.net","threadId":"34188","inReplyTo":"CAMNuMARruu+1=kny=g5O1MoxCXuoT3BHdSEEPSqvyn2t2JsAYg@mail.gmail.com","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-18T06:31:46Z","receivedAt":"2013-06-18T06:31:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 15, 2013 at 04:01:04PM +0200, Mark Abraham wrote:\n\n> Controlled by new configuration option\n> \"color.word-diff-in-interactive-add\". There is no existing support for\n> \"git add\" to pass a command-line option like \"--word-diff=color\" to\n> git-add--interactive.perl, so a configuration option is the only\n> lightweight solution.\n> \n> With this feature, the added or deleted form of a hunk can be empty,\n> so made some necessary checks for $_ being defined.\n\nHmm. We can't apply a word-diff, so presumably your \"permit\" here is\njust for the display, replacing the colorized bits. And that looks like\nwhat your patch does.\n\nNote that the number of lines in your --word-diff=color hunk and the\nactual diff will not necessarily be the same.  What happens if I split a\nhunk with your patch?\n\n-Peff\n"},{"id":"221155","messageId":"87k3lrzxw7.fsf@hexa.v.cablecom.net","threadId":"34188","inReplyTo":"20130618063144.GA6276@sigill.intra.peff.net","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-18T07:22:16Z","receivedAt":"2013-06-18T07:22:16Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"[I don't seem to have received a copy of the original mail, so I can\nonly guess...]\n\nJeff King <peff@peff.net> writes:\n\n> On Sat, Jun 15, 2013 at 04:01:04PM +0200, Mark Abraham wrote:\n>\n>> Controlled by new configuration option\n>> \"color.word-diff-in-interactive-add\". There is no existing support for\n>> \"git add\" to pass a command-line option like \"--word-diff=color\" to\n>> git-add--interactive.perl, so a configuration option is the only\n>> lightweight solution.\n>> \n>> With this feature, the added or deleted form of a hunk can be empty,\n>> so made some necessary checks for $_ being defined.\n>\n> Hmm. We can't apply a word-diff, so presumably your \"permit\" here is\n> just for the display, replacing the colorized bits. And that looks like\n> what your patch does.\n>\n> Note that the number of lines in your --word-diff=color hunk and the\n> actual diff will not necessarily be the same.  What happens if I split a\n> hunk with your patch?\n\nIf it's actually what you hint at, there's another problem: the word\ndiff might not even have the same number of hunks.  For example, a\nlong-standing bug (or feature, depending on POV) of word-diff is that it\ndoes not take opportunities to completely drop hunks that did not make\nany word-level changes.\n\nConsider this:\n\n  diff --git a/foo b/foo\n  index 5eaddaa..089eeac 100644\n  --- a/foo\n  +++ b/foo\n  @@ -1,2 +1,2 @@\n  -foo bar                                                                                               \n  -baz                                                                                                   \n  +foo                                                                                                   \n  +bar baz\n\nIn word-diff mode that's\n\n  diff --git a/foo b/foo                                                                                 \n  index 5eaddaa..089eeac 100644                                                                          \n  --- a/foo                                                                                              \n  +++ b/foo                                                                                              \n  @@ -1,2 +1,2 @@                                                                                        \n  foo                                                                                                    \n  bar baz\n\nArguably word-diff should be fixed to drop the hunk.  But if 'add -p'\ndepends on exact hunk correspondence, it will break.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"221273","messageId":"20130618172300.GA3557@sigill.intra.peff.net","threadId":"34188","inReplyTo":"87k3lrzxw7.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-18T17:23:22Z","receivedAt":"2013-06-18T17:23:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2013 at 09:22:16AM +0200, Thomas Rast wrote:\n\n> [I don't seem to have received a copy of the original mail, so I can\n> only guess...]\n\nYes, the original doesn't seem to have made it to the list. Sorry, I\ndon't have a copy (I am in the habit of deleting direct mails that are\ncc'd to the list, as I keep a separate list archive).\n\nMark, did you happen to send an HTML mail? The list will silently\nreject such mail.\n\n> > Note that the number of lines in your --word-diff=color hunk and the\n> > actual diff will not necessarily be the same.  What happens if I split a\n> > hunk with your patch?\n> \n> If it's actually what you hint at, there's another problem: the word\n> diff might not even have the same number of hunks.  For example, a\n> long-standing bug (or feature, depending on POV) of word-diff is that it\n> does not take opportunities to completely drop hunks that did not make\n> any word-level changes.\n\nYeah, I didn't even think of that.\n\nIn general, I think one can assume 1-to-1 correspondence between whole\nregular diffs and whole word-diffs, but not below that (i.e., neither a\ncorrespondence between hunks nor between lines).\n\nWith something like contrib/diff-highlight, you get some decoration, and\ncan assume a 1-to-1 line correspondence.\n\nHowever, I think that when reviewing text (especially re-wrapped\nparagraphs) that word-diff can be much easier to read, _because_ it\nthrows away the line correspondence. To be correct, though, I think we\nwould have to simply show the whole word-diff for a file and say \"is\nthis OK?\". Which sort of defeats the purpose of \"add -p\" as a hunk\nselector (you could just as easily run \"git diff --color-words foo\" and\n\"git add foo\"). But it does save keystrokes if your workflow is to\nsimply \"add -p\" everything to double-check it before adding.\n\nSo I dunno. I could see myself using it, but I certainly wouldn't want a\nconfig variable that turns it on all the time (which is what the\noriginal patch did).\n\n-Peff\n"},{"id":"221305","messageId":"CAMNuMATdbhgydx5R0+OgSECqxs9tYgwqEjMuXq-0LV8fNaRWZw@mail.gmail.com","threadId":"34188","inReplyTo":"20130618172300.GA3557@sigill.intra.peff.net","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","fromName":"Mark Abraham","fromEmail":"mark.j.abraham@gmail.com","sentAt":"2013-06-18T21:12:59Z","receivedAt":"2013-06-18T21:12:59Z","isPatch":true,"sender":{"key":"mark.j.abraham@gmail.com","avatar":null},"body":"On Tue, Jun 18, 2013 at 7:23 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 18, 2013 at 09:22:16AM +0200, Thomas Rast wrote:\n>\n>> [I don't seem to have received a copy of the original mail, so I can\n>> only guess...]\n>\n> Yes, the original doesn't seem to have made it to the list. Sorry, I\n> don't have a copy (I am in the habit of deleting direct mails that are\n> cc'd to the list, as I keep a separate list archive).\n>\n> Mark, did you happen to send an HTML mail? The list will silently\n> reject such mail.\n\nYes, that was probably it. I tried to find a gmail configuration, but\nI now discover it is done per-email, not globally. Apologies. I have\nforwarded the original to Thomas, but based on current feedback, it\nseems not worth re-sending the original mail to the list. See below.\n\n>> > Note that the number of lines in your --word-diff=color hunk and the\n>> > actual diff will not necessarily be the same.  What happens if I split a\n>> > hunk with your patch?\n>>\n>> If it's actually what you hint at, there's another problem: the word\n>> diff might not even have the same number of hunks.  For example, a\n>> long-standing bug (or feature, depending on POV) of word-diff is that it\n>> does not take opportunities to completely drop hunks that did not make\n>> any word-level changes.\n>\n> Yeah, I didn't even think of that.\n>\n> In general, I think one can assume 1-to-1 correspondence between whole\n> regular diffs and whole word-diffs, but not below that (i.e., neither a\n> correspondence between hunks nor between lines).\n>\n> With something like contrib/diff-highlight, you get some decoration, and\n> can assume a 1-to-1 line correspondence.\n\nMy choice of \"permit\" in the description was not best. My\nimplementation showed a word-based diff, but preserved the existing\nmechanism for actually applying the hunk. I understand the way\ncolorization in git-add--interactive.perl works right now is to\ncolorize one version to display and use another - I think I preserved\nthat. I intended to permit the user to choose to show a word-based\ndiff of a patch during interactive add.\n\n> However, I think that when reviewing text (especially re-wrapped\n> paragraphs) that word-diff can be much easier to read, _because_ it\n> throws away the line correspondence. To be correct, though, I think we\n> would have to simply show the whole word-diff for a file and say \"is\n> this OK?\". Which sort of defeats the purpose of \"add -p\" as a hunk\n> selector (you could just as easily run \"git diff --color-words foo\" and\n\nHmm, I will have to re-consider the implications on that kind of\nworkflow. Thanks!\n\n> \"git add foo\"). But it does save keystrokes if your workflow is to\n> simply \"add -p\" everything to double-check it before adding.\n\nYes, that was what I was aiming to make easier.\n\n> So I dunno. I could see myself using it, but I certainly wouldn't want a\n> config variable that turns it on all the time (which is what the\n> original patch did).\n\nGood point. What I think I really want is \"git add\n--interactive=color\" (with or without --patch) to permit the user to\nchoose to see the (colorized) word-based diff when they want one. I\nnow see that the config file approach in my proposed patch doesn't go\nclose enough to that to be worth considering further.\n\nI think a proper implementation of the above command would have to\n* add something to builtin_add_options in builtin/add.c,\n* set a new static variable in add.c, and\n* extend the calling logic for interactive_add() and/or\nrun_add_interactive() accordingly,\nso that the perl script can get the user's choice on the command line\nand not from a config file. And only respond when colorization is\nconfigured.\n\nDoes --patch=color, --interactive=color or adding new option\n--color-words make more sense?\n\nI'll have a think about that and get back to you guys.\n\nThanks!\n\nMark\n\n> -Peff\n"},{"id":"221309","messageId":"20130618224720.GB14234@sigill.intra.peff.net","threadId":"34188","inReplyTo":"CAMNuMATdbhgydx5R0+OgSECqxs9tYgwqEjMuXq-0LV8fNaRWZw@mail.gmail.com","subject":"Re: [PATCH] git-add--interactive.perl: Permit word-based diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-18T22:47:22Z","receivedAt":"2013-06-18T22:47:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 18, 2013 at 11:12:59PM +0200, Mark Abraham wrote:\n\n> > In general, I think one can assume 1-to-1 correspondence between whole\n> > regular diffs and whole word-diffs, but not below that (i.e., neither a\n> > correspondence between hunks nor between lines).\n> [...]\n> My choice of \"permit\" in the description was not best. My\n> implementation showed a word-based diff, but preserved the existing\n> mechanism for actually applying the hunk. I understand the way\n> colorization in git-add--interactive.perl works right now is to\n> colorize one version to display and use another - I think I preserved\n> that. I intended to permit the user to choose to show a word-based\n> diff of a patch during interactive add.\n\nRight. My point is that the colorized version always lines up with the\n\"real\" to-apply version line-for-line. But the word diff version does\nnot.\n\nIf we assume that they line up hunk for hunk (that is, --color-words\ncomes up with the same number of hunks, but the contents of each hunk\nmay be different), then swapping \"colorized line diff\" for \"word diff\"\nin the presentation version (i.e., your patch) is fine. I think this is\nthe case with the current --word-diff, so the only question would be one\nof not wanting to paint ourselves into a corner with implementation\ndetails. I think that is OK, as if we later wanted to change --word-diff\nto coalesce hunks, we could continue to support a\n\"--hunk-based-word-diff\" mode for this type of operation that cares\nabout matching the normal diff hunks.\n\nIf we assume that the presentation version lines up line for line within\neach hunk, then it is safe to do hunk-level operations like splitting.\nThat works with the current colorized diffs, but not with word-diff. I\nthink you can construct a case where doing a hunk-split from the\nselection menu causes us to display a word-diff that is not the\nequivalent of what would be applied if it is selected.\n\nFor example:\n\n\t# make a file and add it to the index\n\tcat >file <<-\\EOF\n\ta\n\tc\n\td\n\te\n\tf\n\tEOF\n\tgit add file\n\n\t# now fix the missing letter, tweak the wrapping, and add some new\n\t# content\n\tcat >file <<\\EOF\n\ta b c d e\n\tf\n\tg h i j k\n\tEOF\n\nThe regular diff has 7 lines:\n\n\t$ git diff\n\tdiff --git a/file b/file\n\tindex dffa0a4..3a7aeb4 100644\n\t--- a/file\n\t+++ b/file\n\t@@ -1,5 +1,3 @@ f\n\t-a\n\t-c\n\t-d\n\t-e\n\t+a b c d e\n\t f\n\t+g h i j k\n\nBut the word diff has only 3:\n\n\t$ git diff --word-diff\n\tdiff --git a/file b/file\n\tindex dffa0a4..3a7aeb4 100644\n\t--- a/file\n\t+++ b/file\n\t@@ -1,5 +1,3 @@ f\n\ta {+b+} c d e\n\tf\n\t{+g h i j k+}\n\nIf I run \"git add -p\" with your patch and choose \"s\" to split the hunk,\nI will still be shown:\n\n\tStage this hunk [y,n,q,a,d,/,s,e,?]? s\n\tSplit into 2 hunks.\n\t@@ -1,5 +1,2 @@\n\ta b c d e\n\tf\n\tg h i j k\n\tStage this hunk [y,n,q,a,d,/,j,J,g,e,?]?\n\nIf I say \"yes\" here, I get shown the next hunk, which has no lines at\nall.  Let's imagine I say \"no\". What gets applied? Only the first change\n(rewrapping and adding \"b\"). But not the addition of \"g\" through \"k\",\neven though they were shown in the presentation of the hunk that I said\n\"yes\" to.\n\nSo I think if we want to do a word-diff for the presentation, it would\nprobably make sense to shut off line-level manipulation like\nhunk-splitting.\n\n> Good point. What I think I really want is \"git add\n> --interactive=color\" (with or without --patch) to permit the user to\n> choose to see the (colorized) word-based diff when they want one. I\n> now see that the config file approach in my proposed patch doesn't go\n> close enough to that to be worth considering further.\n\nI wonder if it should go even a step below there: always generate the\nnormal diff for presentation, and then have an interactive key to show\nthe --word-diff of it.\n\nI find I often do not know until I get to a very ugly documentation diff\nwith paragraph rewrapping that I wanted word-diff (and I would _not_\nwant it for my code hunks, just for the documentation hunk).\n\nAnd as a bonus, it is easier to implement, since the logic is all within\nthe hunk-selection menu.\n\n-Peff\n"}]}