git add --patch bug with split+edit?

git add --patch bug with split+edit?

[Hannu Koivisto]

Greetings,

If I have a hunk that adds three lines, I can edit the hunk and
remove the last line but I can't split it in two, stage the first
part, edit the second part and remove the last line.  An example:

mkdir gittest
cd gittest
git init
echo "baz\nbaz" > baz
git add baz
git commit -m baz baz
rm baz
echo "sur\nbaz\nbaz\njee\njee" > baz
git add --patch

Now say 's RET y RET e RET' and remove the second "+jee" line using
your editor.  The output for me looks like this:

--8<-----------------------------------------------------------------
diff --git a/baz b/baz
index 1f55335..48a5f83 100644
--- a/baz
+++ b/baz
@@ -1,2 +1,5 @@
+sur
 baz
 baz
+jee
+jee
Stage this hunk [y/n/a/d/s/e/?]? s
Split into 2 hunks.
@@ -1,2 +1,3 @@
+sur
 baz
 baz
Stage this hunk [y/n/a/d/j/J/e/?]? y
@@ -1,2 +2,4 @@
 baz
 baz
+jee
+jee
Stage this hunk [y/n/a/d/K/e/?]? e
Waiting for Emacs...
error: patch failed: baz:1
error: baz: patch does not apply
Your edited hunk does not apply. Edit again (saying "no" discards!) [y/n]?
--8<-----------------------------------------------------------------

What I also didn't expect is that if I answer 'n' to that last
question, I get...

@@ -1,2 +1,3 @@
+sur
 baz
 baz
Stage this hunk [y/n/a/d/j/J/e/?]?

...which is the first part of the splitted hunk that I already
staged.  If I answer 'd', git status and git diff indicate that
"+sur" is nevertheless staged.

Now, if instead of splitting the hunk and editing it, I edit the
entire...

@@ -1,2 +1,5 @@
+sur
 baz
 baz
+jee
+jee

...hunk and remove the last "+jee" line, I get no error.

I'm using git 1.6.1 on Linux.

-- 
Hannu

Re: git add --patch bug with split+edit?

[Jeff King]

On Sat, Jan 17, 2009 at 03:37:43AM +0200, Hannu Koivisto wrote:

> echo "sur\nbaz\nbaz\njee\njee" > baz
> git add --patch
> 
> Now say 's RET y RET e RET' and remove the second "+jee" line using
> your editor.  The output for me looks like this:
> [...]
> error: patch failed: baz:1
> error: baz: patch does not apply
> Your edited hunk does not apply. Edit again (saying "no" discards!) [y/n]?

Actually, you do not even need to change the patch at all for this to
fail. The hunk that you edit looks like this:

@@ -1,2 +2,4 @@
 baz
 baz
+jee
+jee

which doesn't make sense. I think the hunk header should actually be:

  @@ -1,2 +1,4 @@

But I don't think that is the problem, since git-apply should be
recounting the hunk information (and in a simple test, it doesn't fix
it).

Hm. OK, I see. The "does this diff apply" check feeds _both_ parts of
the split patch to git-apply. But of course the second part will never
correctly apply, because its context overlaps with the first part, but
doesn't take it into account.

Doing the check with _just_ the edited patch would work. But that
doesn't take into account that your edited patch will potentially fail
to apply in the long run, depending on whether or not you accept the
other half of the split patch. And we can't know that yet, because the
user may not have told us (they could have skipped the first half, and
then come back to it later after the edit step).

So in general, I think splitting and editing the same hunk is inherently
dangerous and is going to lead to these sorts of problems. And because
editing provides a superset of the functionality, I think you should
just edit and either allow the first part of the hunk to be applied or
not depending on your preference.

But maybe there is some better way of resolving the conflict. I don't
see one, but I'm tired and didn't think too hard on it. :)

-Peff