patchRe: [PATCH] git-add--interactive.perl: Permit word-based diff

On Sat, Jun 15, 2013 at 04:01:04PM +0200, Mark Abraham wrote:

Hmm. We can't apply a word-diff, so presumably your "permit" here is just for the display, replacing the colorized bits. And that looks like what your patch does.

Note that the number of lines in your --word-diff=color hunk and the actual diff will not necessarily be the same. What happens if I split a hunk with your patch?

-Peff

[I don't seem to have received a copy of the original mail, so I can only guess...]

Jeff King <peff@peff.net> writes:

If it's actually what you hint at, there's another problem: the word diff might not even have the same number of hunks. For example, a long-standing bug (or feature, depending on POV) of word-diff is that it does not take opportunities to completely drop hunks that did not make any word-level changes.

Consider this:

  diff --git a/foo b/foo
  index 5eaddaa..089eeac 100644
  --- a/foo
  +++ b/foo
  @@ -1,2 +1,2 @@
  -foo bar                                                                                               
  -baz                                                                                                   
  +foo                                                                                                   
  +bar baz

In word-diff mode that's

  diff --git a/foo b/foo                                                                                 
  index 5eaddaa..089eeac 100644                                                                          
  --- a/foo                                                                                              
  +++ b/foo                                                                                              
  @@ -1,2 +1,2 @@                                                                                        
  foo                                                                                                    
  bar baz

Arguably word-diff should be fixed to drop the hunk. But if 'add -p' depends on exact hunk correspondence, it will break.

On Tue, Jun 18, 2013 at 09:22:16AM +0200, Thomas Rast wrote:

Yes, the original doesn't seem to have made it to the list. Sorry, I don't have a copy (I am in the habit of deleting direct mails that are cc'd to the list, as I keep a separate list archive).

Mark, did you happen to send an HTML mail? The list will silently reject such mail.

Yeah, I didn't even think of that.

In general, I think one can assume 1-to-1 correspondence between whole regular diffs and whole word-diffs, but not below that (i.e., neither a correspondence between hunks nor between lines).

With something like contrib/diff-highlight, you get some decoration, and can assume a 1-to-1 line correspondence.

However, I think that when reviewing text (especially re-wrapped paragraphs) that word-diff can be much easier to read, _because_ it throws away the line correspondence. To be correct, though, I think we would have to simply show the whole word-diff for a file and say "is this OK?". Which sort of defeats the purpose of "add -p" as a hunk selector (you could just as easily run "git diff --color-words foo" and "git add foo"). But it does save keystrokes if your workflow is to simply "add -p" everything to double-check it before adding.

So I dunno. I could see myself using it, but I certainly wouldn't want a config variable that turns it on all the time (which is what the original patch did).

-Peff

On Tue, Jun 18, 2013 at 7:23 PM, Jeff King <peff@peff.net> wrote:

Yes, that was probably it. I tried to find a gmail configuration, but I now discover it is done per-email, not globally. Apologies. I have forwarded the original to Thomas, but based on current feedback, it seems not worth re-sending the original mail to the list. See below.

My choice of "permit" in the description was not best. My implementation showed a word-based diff, but preserved the existing mechanism for actually applying the hunk. I understand the way colorization in git-add--interactive.perl works right now is to colorize one version to display and use another - I think I preserved that. I intended to permit the user to choose to show a word-based diff of a patch during interactive add.

Hmm, I will have to re-consider the implications on that kind of workflow. Thanks!

Yes, that was what I was aiming to make easier.

Good point. What I think I really want is "git add --interactive=color" (with or without --patch) to permit the user to choose to see the (colorized) word-based diff when they want one. I now see that the config file approach in my proposed patch doesn't go close enough to that to be worth considering further.

I think a proper implementation of the above command would have to
* add something to builtin_add_options in builtin/add.c,
* set a new static variable in add.c, and
* extend the calling logic for interactive_add() and/or
run_add_interactive() accordingly,
so that the perl script can get the user's choice on the command line
and not from a config file. And only respond when colorization is
configured.

Does --patch=color, --interactive=color or adding new option --color-words make more sense?

I'll have a think about that and get back to you guys.

Thanks!

Mark

On Tue, Jun 18, 2013 at 11:12:59PM +0200, Mark Abraham wrote:

Right. My point is that the colorized version always lines up with the "real" to-apply version line-for-line. But the word diff version does not.

If we assume that they line up hunk for hunk (that is, --color-words comes up with the same number of hunks, but the contents of each hunk may be different), then swapping "colorized line diff" for "word diff" in the presentation version (i.e., your patch) is fine. I think this is the case with the current --word-diff, so the only question would be one of not wanting to paint ourselves into a corner with implementation details. I think that is OK, as if we later wanted to change --word-diff to coalesce hunks, we could continue to support a "--hunk-based-word-diff" mode for this type of operation that cares about matching the normal diff hunks.

If we assume that the presentation version lines up line for line within each hunk, then it is safe to do hunk-level operations like splitting. That works with the current colorized diffs, but not with word-diff. I think you can construct a case where doing a hunk-split from the selection menu causes us to display a word-diff that is not the equivalent of what would be applied if it is selected.

For example:

	# make a file and add it to the index
	cat >file <<-\EOF
	a
	c
	d
	e
	f
	EOF
	git add file

	# now fix the missing letter, tweak the wrapping, and add some new
	# content
	cat >file <<\EOF
	a b c d e
	f
	g h i j k
	EOF

The regular diff has 7 lines:

	$ git diff
	diff --git a/file b/file
	index dffa0a4..3a7aeb4 100644
	--- a/file
	+++ b/file
	@@ -1,5 +1,3 @@ f
	-a
	-c
	-d
	-e
	+a b c d e
	 f
	+g h i j k

But the word diff has only 3:

	$ git diff --word-diff
	diff --git a/file b/file
	index dffa0a4..3a7aeb4 100644
	--- a/file
	+++ b/file
	@@ -1,5 +1,3 @@ f
	a {+b+} c d e
	f
	{+g h i j k+}

If I run "git add -p" with your patch and choose "s" to split the hunk, I will still be shown:

	Stage this hunk [y,n,q,a,d,/,s,e,?]? s
	Split into 2 hunks.
	@@ -1,5 +1,2 @@
	a b c d e
	f
	g h i j k
	Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]?

If I say "yes" here, I get shown the next hunk, which has no lines at all. Let's imagine I say "no". What gets applied? Only the first change (rewrapping and adding "b"). But not the addition of "g" through "k", even though they were shown in the presentation of the hunk that I said "yes" to.

So I think if we want to do a word-diff for the presentation, it would probably make sense to shut off line-level manipulation like hunk-splitting.

I wonder if it should go even a step below there: always generate the normal diff for presentation, and then have an interactive key to show the --word-diff of it.

I find I often do not know until I get to a very ugly documentation diff with paragraph rewrapping that I wanted word-diff (and I would _not_ want it for my code hunks, just for the documentation hunk).

And as a bonus, it is easier to implement, since the logic is all within the hunk-selection menu.

-Peff