Bug: 'git am --abort' can silently reset the wrong branch

Bug: 'git am --abort' can silently reset the wrong branch

[Eduardo Habkost]

Hi,

I've ben bitten by this multiple times, while maintaining a multi-branch
repository. 'git am --abort' can drop the whole history of a branch, if you
switch branches before running it. Below are the steps to reproduce the
problem:

[foo/branch1]$ git --version
git version 1.6.3.rc4.29.g8146

I have two branches, with distinct changes:

[foo/branch1]$ git diff branch1 branch2
diff --git a/file.txt b/file.txt
index e8a7b03..a11bf50 100644
--- a/file.txt
+++ b/file.txt
@@ -1,2 +1,2 @@
 initial content
-branch1 content, version 1
+branch2 content. lots of changes here. version 12345.
[foo/branch1]$ git status
# On branch branch1
nothing to commit (working directory clean)
[foo/branch1]$


I receive a patch and try to apply it to branch1:

[foo/branch1]$ git am /tmp/patch.patch
Applying: patch to be applied
fatal: corrupt patch at line 13
Patch failed at 0001 patch to be applied
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
[foo/branch1]$

(Seeing that solving this conflict will take some time. I go out to grab some
coffee).

(When back to work, somebody asks me to take a look on patch2.patch, that
applies to branch2).

[foo/branch1]$ git checkout branch2
Switched to branch 'branch2'
[foo/branch2]$ git am /tmp/patch2.patch
previous rebase directory /tmp/foo/.git/rebase-apply still exists but mbox given.
[foo/branch2]$


(Oh, I see. There was a pending 'git am' operation. Let's just cancel it, I can
start it again later anyway).

Here is where "git am" silently does the wrong thing:

[foo/branch2]$ git am --abort
[foo/branch2]$ cat file.txt
initial content
branch1 content, version 1
[foo/branch2]$

(Hey! where are the changes from branch2?)

[foo/branch2]$ git diff branch1 branch2
[foo/branch2]$

Ouch.   :(


When that happens, I need to resort to git reflog, and manually git-reset
branch2 to point to the original commit.

I don't know what would be the best approach to avoid this issue. I tested a
similar scenario using 'git rebase' and 'git rebase --abort' switched back to
branch1 before resetting the branch state, instead of resetting branch2. Maybe
'git am' could do the same.

-- 
Eduardo

Re: Bug: 'git am --abort' can silently reset the wrong branch

[Jeff King]

On Wed, May 06, 2009 at 04:19:46PM -0300, Eduardo Habkost wrote:

> I've ben bitten by this multiple times, while maintaining a multi-branch
> repository. 'git am --abort' can drop the whole history of a branch, if you
> switch branches before running it. Below are the steps to reproduce the
> problem:

Yeah, it would be nice if there was a safety valve here to cause "git am
--abort" to realize that you had switched branches out from under it and
not bother with resetting HEAD (but it should still blow away
.git/rebase-apply).

Below is a patch which accomplishes that. However, I'm not sure it is
the right direction. Switching branches and clobbering some other branch
with --abort is just _one_ thing you can do to screw yourself. You could
also have been doing useful work on the _same_ branch, and that would
get clobbered by --abort.  However, I'm not sure if we have a good way
of telling the difference between "work which I did to try to get these
patches to apply, but which should be thrown away when I abort" and
"work which I did because I forgot I had an active git-am".

So maybe just picking the changed-branch situation is the best we can
do. I'd be interested to hear comments from others.

This patch works for git-am; we would also need to do something similar
for rebase.

-- >8 --
am: try not to clobber alternate branches on --abort

A user in the middle of a failed git-am may forget that they
are there and begin doing other work. They may later realize
that the "am" session is still active (when trying to rebase
or apply another patch), at which point they are tempted to
"git am --abort". However, this resets their HEAD back to
ORIG_HEAD, clobbering any work they have done in the
meantime.

This patch detects the situation where the branch has
changed and skips the reset step (which assumes the user has
the tree in a state they like now).

---
diff --git a/git-am.sh b/git-am.sh
index 6d1848b..4d5f408 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -244,10 +244,13 @@ then
 			exec git rebase --abort
 		fi
 		git rerere clear
-		test -f "$dotest/dirtyindex" || {
+		if ! test -f "$dotest/dirtyindex" &&
+		     test "`git symbolic-ref -q HEAD`" = \
+		          "`cat "$dotest/start-ref"`"
+		then
 			git read-tree --reset -u HEAD ORIG_HEAD
 			git reset ORIG_HEAD
-		}
+		fi
 		rm -fr "$dotest"
 		exit ;;
 	esac
@@ -304,6 +307,7 @@ else
 			git update-ref -d ORIG_HEAD >/dev/null 2>&1
 		fi
 	fi
+	git symbolic-ref -q HEAD >"$dotest/start-ref"
 fi
 
 case "$resolved" in
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 2b912d7..8e62e76 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -62,4 +62,19 @@ do
 
 done
 
+test_expect_success 'am --abort does not clobber changed branch' '
+	rm -f otherfile-* &&
+	git checkout -b other &&
+	echo content >unrelated &&
+	git add unrelated &&
+	git commit -m other &&
+	git rev-parse other >expect &&
+	git checkout master &&
+	test_must_fail git am 000[1245]-*.patch &&
+	git checkout other &&
+	git am --abort &&
+	git rev-parse other >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: Bug: 'git am --abort' can silently reset the wrong branch

[Sverre Rabbelier]

Heya,

On Fri, May 8, 2009 at 10:28, Jeff King <peff@peff.net> wrote:
> So maybe just picking the changed-branch situation is the best we can
> do. I'd be interested to hear comments from others.

FWIW, I usually use 'git am' on accident (when trying to type somthing
else :P), and I would prefer if 'git am --abort' would just clean up
it's own mess, but leave my working dir alone. If I want to obliterate
my data I can 'git reset --hard' myself.

-- 
Cheers,

Sverre Rabbelier

Re: Bug: 'git am --abort' can silently reset the wrong branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Switching branches and clobbering some other branch
> with --abort is just _one_ thing you can do to screw yourself. You could
> also have been doing useful work on the _same_ branch, and that would
> get clobbered by --abort.  However, I'm not sure if we have a good way
> of telling the difference between "work which I did to try to get these
> patches to apply, but which should be thrown away when I abort" and
> "work which I did because I forgot I had an active git-am".

I think I've said this already, but honestly speaking, I think --abort
should not do --reset at all, but just remove the $dotest directory.  Or
perhaps introduce a --clear option to do so.

At least your patch is an improvement.

What I sometimes see to my users happen is to try applying to the oldest
integration branch the patch (the users think) ought to apply, see it fail
to apply, switch to a bit newer branch and run "am" again (trusting that
it will pick up the material from $dotest), repeat the above and then give
up with "git am --abort".  I do not think anybody can offhand explain to
which branch and to what state the command takes the user back to in such
a situation without looking at what the code actually does X-<; even
though I think it should take the user back to the original branch, I do
not think that is what the code does.

Re: Bug: 'git am --abort' can silently reset the wrong branch

[Jeff King]

On Fri, May 08, 2009 at 02:01:16AM -0700, Junio C Hamano wrote:

> > Switching branches and clobbering some other branch
> > with --abort is just _one_ thing you can do to screw yourself. You could
> > also have been doing useful work on the _same_ branch, and that would
> > get clobbered by --abort.  However, I'm not sure if we have a good way
> > of telling the difference between "work which I did to try to get these
> > patches to apply, but which should be thrown away when I abort" and
> > "work which I did because I forgot I had an active git-am".
> 
> I think I've said this already, but honestly speaking, I think --abort
> should not do --reset at all, but just remove the $dotest directory.  Or
> perhaps introduce a --clear option to do so.

I assumed that people actually liked the current "reset" behavior, so I
didn't want to propose getting rid of it.  Personally, I hate it. So I
would be very happy to see it ripped out entirely, and then that neatly
solves the problem (i.e., it now errs on the side of not throwing away
work).

> What I sometimes see to my users happen is to try applying to the oldest
> integration branch the patch (the users think) ought to apply, see it fail
> to apply, switch to a bit newer branch and run "am" again (trusting that
> it will pick up the material from $dotest), repeat the above and then give
> up with "git am --abort".  I do not think anybody can offhand explain to
> which branch and to what state the command takes the user back to in such
> a situation without looking at what the code actually does X-<; even
> though I think it should take the user back to the original branch, I do
> not think that is what the code does.

No, the current code clobbers whatever is in the current HEAD with
ORIG_HEAD. So not only might you set another random branch back to the
originally am'd branch, but if you did a pull in between you can pick up
some random commit.

-Peff

[RFC PATCH] am: do not do any reset on --abort

[Jeff King]

We really have no idea what state the tree is in at this
point, and whether the user might have done useful work on
top of it. So let's err on the side of keeping the user's
data intact.

The downside is that if they do have cruft to get rid of, or
want to pretend as if earlier parts of the series that were
applied did not exist, they must manually "git reset --hard"
now.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a followup to:

  http://thread.gmane.org/gmane.comp.version-control.git/118373

 git-am.sh |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 578780b..a7e24cf 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -240,10 +240,6 @@ then
 			exec git rebase --abort
 		fi
 		git rerere clear
-		test -f "$dotest/dirtyindex" || {
-			git read-tree --reset -u HEAD ORIG_HEAD
-			git reset ORIG_HEAD
-		}
 		rm -fr "$dotest"
 		exit ;;
 	esac
-- 
1.6.3.1.250.g01b8b.dirty

Re: [RFC PATCH] am: do not do any reset on --abort

[Johannes Schindelin]

Hi,

On Mon, 25 May 2009, Jeff King wrote:

> We really have no idea what state the tree is in at this
> point, and whether the user might have done useful work on
> top of it. So let's err on the side of keeping the user's
> data intact.
> 
> The downside is that if they do have cruft to get rid of, or
> want to pretend as if earlier parts of the series that were
> applied did not exist, they must manually "git reset --hard"
> now.
> 
> Signed-off-by: Jeff King <peff@peff.net>

Hmm.  I think I would revert that patch after merging git.git right away.

Can you at least check for a dirty tree and reset --hard if it is clean?  
In the other case, you could still say "you seem to have modifications, 
bla bla bla"...

Ciao,
Dscho

Re: [RFC PATCH] am: do not do any reset on --abort

[Jeff King]

On Mon, May 25, 2009 at 01:49:18PM +0200, Johannes Schindelin wrote:

> > We really have no idea what state the tree is in at this
> > point, and whether the user might have done useful work on
> > top of it. So let's err on the side of keeping the user's
> > data intact.
> > 
> > The downside is that if they do have cruft to get rid of, or
> > want to pretend as if earlier parts of the series that were
> > applied did not exist, they must manually "git reset --hard"
> > now.
> 
> Hmm.  I think I would revert that patch after merging git.git right away.

You know, you can just say you don't like it. ;)

> Can you at least check for a dirty tree and reset --hard if it is clean?

No, that would defeat the purpose. The problem is that we have no idea
what has happened since the initial "git am". The user may have made
commits they want to keep, and we don't want to reset those away. They
may even have pulled, which means ORIG_HEAD can no longer be trusted for
a reset.

> In the other case, you could still say "you seem to have modifications, 
> bla bla bla"...

I think you raise a good point here. If we do decide to stop doing the
reset automatically, it is probably better not to simply stop doing it
(as my patch does), but to downgrade it to a warning ("You probably want
to reset, etc...").

-Peff

Re: [RFC PATCH] am: do not do any reset on --abort

[Johannes Sixt]

Jeff King schrieb:
> On Mon, May 25, 2009 at 01:49:18PM +0200, Johannes Schindelin wrote:
> 
>>> We really have no idea what state the tree is in at this
>>> point, and whether the user might have done useful work on
>>> top of it. So let's err on the side of keeping the user's
>>> data intact.
>>>
>>> The downside is that if they do have cruft to get rid of, or
>>> want to pretend as if earlier parts of the series that were
>>> applied did not exist, they must manually "git reset --hard"
>>> now.
>> Hmm.  I think I would revert that patch after merging git.git right away.
> 
> You know, you can just say you don't like it. ;)
> 
>> Can you at least check for a dirty tree and reset --hard if it is clean?
> 
> No, that would defeat the purpose. The problem is that we have no idea
> what has happened since the initial "git am". The user may have made
> commits they want to keep, and we don't want to reset those away. They
> may even have pulled, which means ORIG_HEAD can no longer be trusted for
> a reset.

I wonder why we have this problem (and do something about it) with git-am,
but not with git-rebase. Is it perhaps that the usual case were people
were bitten by the old behavior is:

   $ git am mbox
   .... stops with conflicts
   .... oops, wrong branch
   $ git checkout other-branch
   $ git am mbox
   error: git am is already in progress
   $ git am --abort
   OUTCH! other-branch was reset!

rebase is not used in this manner, and even though it does reset --hard,
it doesn't hurt (that often).

-- Hannes

Re: [RFC PATCH] am: do not do any reset on --abort

[Avery Pennarun]

On Mon, May 25, 2009 at 8:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>   $ git am --abort
>   OUTCH! other-branch was reset!

If *that's* your problem, then presumably you could avoid it just by
checking whether the right branch corresponds to HEAD before doing a
reset.

Have fun,

Avery

Re: [RFC PATCH] am: do not do any reset on --abort

[Jeff King]

On Mon, May 25, 2009 at 11:54:36AM -0400, Avery Pennarun wrote:

> On Mon, May 25, 2009 at 8:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >   $ git am --abort
> >   OUTCH! other-branch was reset!
> 
> If *that's* your problem, then presumably you could avoid it just by
> checking whether the right branch corresponds to HEAD before doing a
> reset.

But that only covers one problem. How about you forgot that you had a
failed am in progress, waited hours or days, made some commits on the
same branch, tried to am a series, got the "in progress" message and
then did an "--abort"?

And yes, I have done that.

In both examples, an alternative method for dealing with this is to try
to alert the user that we are in the middle of an am when doing
potentially suspicious things (like switching branches or making commits
outside of "git am --resolved"). I don't know how well that would work
in practice.

-Peff

Re: [RFC PATCH] am: do not do any reset on --abort

[Jeff King]

On Mon, May 25, 2009 at 02:17:48PM +0200, Johannes Sixt wrote:

> I wonder why we have this problem (and do something about it) with git-am,
> but not with git-rebase. Is it perhaps that the usual case were people
> were bitten by the old behavior is:

I don't know. I had assumed a safety valve we put in git-am might need
to be matched in rebase. But I don't recall whether I have screwed
myself in the same way with rebase. Perhaps because rebase happens on a
detached HEAD, I tend to notice sooner that something is not right.

-Peff

Re: [RFC PATCH] am: do not do any reset on --abort

[Avery Pennarun]

On Mon, May 25, 2009 at 12:02 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 25, 2009 at 02:17:48PM +0200, Johannes Sixt wrote:
>
>> I wonder why we have this problem (and do something about it) with git-am,
>> but not with git-rebase. Is it perhaps that the usual case were people
>> were bitten by the old behavior is:
>
> I don't know. I had assumed a safety valve we put in git-am might need
> to be matched in rebase. But I don't recall whether I have screwed
> myself in the same way with rebase. Perhaps because rebase happens on a
> detached HEAD, I tend to notice sooner that something is not right.

Ah, maybe that's the difference.  rebase seems to detach the HEAD,
then do a bunch of stuff, then reset the original branch only when
it's done.  So aborting doesn't reset any branches at all, it just
checks out the original branch.

Thus one option would be to try to make am more like rebase: detach
the HEAD before it starts, and reattach it only on success.  At least
then you only have one set of UI problems to fix.

(Of course, I've gotten myself into trouble anyway by checking new
stuff in on the detached HEAD and later aborting a rebase.  I have
quite a love-hate relationship with detached HEADs.)

Maybe the best solution here isn't to prevent people from shooting
themselves in the foot, but instead to help them recover afterwards.
I've noticed most people don't know about 'git reflog'; they often
seem astonished when I tell them about it.  reflog + not blowing away
dirty repositories would mean that you're always safe.

Have fun,

Avery

Re: [RFC PATCH] am: do not do any reset on --abort

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We really have no idea what state the tree is in at this
> point, and whether the user might have done useful work on
> top of it. So let's err on the side of keeping the user's
> data intact.
>
> The downside is that if they do have cruft to get rid of, or
> want to pretend as if earlier parts of the series that were
> applied did not exist, they must manually "git reset --hard"
> now.

I do not see it as a major downside, but not telling them that they may
want to when we avoid doing it ourselves might be.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is a followup to:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/118373
>
>  git-am.sh |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 578780b..a7e24cf 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -240,10 +240,6 @@ then
>  			exec git rebase --abort
>  		fi
>  		git rerere clear
> -		test -f "$dotest/dirtyindex" || {
> -			git read-tree --reset -u HEAD ORIG_HEAD
> -			git reset ORIG_HEAD
> -		}
>  		rm -fr "$dotest"
>  		exit ;;
>  	esac
> -- 
> 1.6.3.1.250.g01b8b.dirty