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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 13, 2013, 19:07 UTC
Message-ID: <CALkWK0kO=KY4=Y7ex47LJ2F0oEfF+R9g3REO7rddDLL4c8CuEw@mail.gmail.com>
In-Reply-To: <7vmwqtua5r.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> Hmph, when did ORIG_HEAD set, and what commit does it point at?

By some unrelated previous operation (eg. pull, rebase, merge). The point is that at any point in "normal operation", ORIG_HEAD exists, and usually points to @~N, for some N. If I rm .git/ORIG_HEAD by hand, the am --abort obviously errors out:

  $ git am --abort
  fatal: Not a valid object name ORIG_HEAD
  fatal: ambiguous argument 'ORIG_HEAD': unknown revision or path not
in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

Show 12 quoted lines

> As "git am" reading from stdin, waiting, hasn't moved HEAD yet at
> all, I think two things need to happen to fix that:
>
>  (1) at around the call to check_patch_format and split_patches,
>      clear ORIG_HEAD (this may have to be done only !$rebasing,
>      though).
>
>  (2) safe_to_abort() should make sure ORIG_HEAD exists; otherwise it
>      is unsafe.
>
> But that is entirely an independent issue (I am going to agree with
> you in the end).

Exactly. It might be nice to fix those two things (are there any observed bugs?), but it is entirely orthogonal to our issue.

> That is a correct observation.  But it needed a bit of thinking to
> reach your conclusion that special casing this and handling --abort
> in a new different codepath is the right solution.

Yeah, the commit message is lacking.

Show 6 quoted lines

> How would "am --skip", "am --resolved", or "am anothermbox" behave
> in this "we already have $dotest because the user started one
> session but killed it" case, which used to be covered by -d $dotest
> alone but now flows to the other side of the if/else/fi codepath?
> Do they need a similar treatment, or would they naturally error out
> as they should?

am --skip and am --resolved will error out, saying "Resolve operation not in progress, we are not resuming". The message needs to be tweaked.

am anothermbox will work just fine, implicitly overwriting the existing $dotest directory. But yeah, we could explicitly remove the directory and allow the mkdir -p to re-create it.

I'll probably work on these follow-ups in the morning.  Thanks for poking.