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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html