Re: [PATCH] git-add--interactive.perl: Permit word-based diff
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-add--interactive.perl: Permit word-based diff
[I don't seem to have received a copy of the original mail, so I can
only guess...]
Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-add--interactive.perl: Permit word-based diff
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-add--interactive.perl: Permit word-based diff
On Tue, Jun 18, 2013 at 7:23 PM, Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-add--interactive.perl: Permit word-based diff
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
