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

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

[Jeff King]

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

> Controlled by new configuration option
> "color.word-diff-in-interactive-add". There is no existing support for
> "git add" to pass a command-line option like "--word-diff=color" to
> git-add--interactive.perl, so a configuration option is the only
> lightweight solution.
> 
> With this feature, the added or deleted form of a hunk can be empty,
> so made some necessary checks for $_ being defined.

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

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

[Thomas Rast]

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

Jeff King <peff@peff.net> writes:

> On Sat, Jun 15, 2013 at 04:01:04PM +0200, Mark Abraham wrote:
>
>> Controlled by new configuration option
>> "color.word-diff-in-interactive-add". There is no existing support for
>> "git add" to pass a command-line option like "--word-diff=color" to
>> git-add--interactive.perl, so a configuration option is the only
>> lightweight solution.
>> 
>> With this feature, the added or deleted form of a hunk can be empty,
>> so made some necessary checks for $_ being defined.
>
> 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?

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.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Jeff King]

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

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

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.

> > 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?
> 
> 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.

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

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

[Mark Abraham]

On Tue, Jun 18, 2013 at 7:23 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 18, 2013 at 09:22:16AM +0200, Thomas Rast wrote:
>
>> [I don't seem to have received a copy of the original mail, so I can
>> only guess...]
>
> 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.

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.

>> > 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?
>>
>> 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.
>
> 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.

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.

> 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

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

> "git add foo"). But it does save keystrokes if your workflow is to
> simply "add -p" everything to double-check it before adding.

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

> 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).

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

> -Peff

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

[Jeff King]

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

> > 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).
> [...]
> 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.

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.

> 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 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