Re: [PATCH] am: handle stray $dotest directory case

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 13, 2013, 17:33 UTC
Message-ID: <CALkWK0mUnNTfJQuGQAoMtnsOXvXU+z+W6D_MMuio1sq4vkucjA@mail.gmail.com>
In-Reply-To: <7v61xivsxh.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> Perhaps _that_ guarding condition is what needs
> to be fixed, by reverting it back to just "does $dotest exist?"
>
> Adding a single case workaround smells like a band-aid to me.

Like I pointed out earlier, the original codepath is not equipped to handle this case. A "normal" git am --abort runs:

  git read-tree --reset -u HEAD ORIG_HEAD
  git reset ORIG_HEAD

blowing away the top commit in the scenario you outlined.

This happens because that codepath incorrectly believes that an am is "in progress". What this means is that it believes that some of the am code actually got executed in the previous run, setting ORIG_HEAD etc. In your scenario

  % git am
  ^C

nothing is executed, and a stray directory is left behind.

If anything, I think the check for $dotest/{next,last} has made the code more robust by correctly verifying that some code did get executed, and that an am is indeed in progress. The bug you have outlined is equivalent to:

  % mkdir .git/rebase-apply
  % git am --abort

Therefore, the fix is to treat it as exactly that: a stray directory that needs to be cleaned up in the codepath that treats it as a "fresh run"; not going through the "normal" am --abort logic and blowing away the top commit.

Makes sense?