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

Ramkumar Ramachandra [off-list ref] writes:

Junio C Hamano wrote:

quoted

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

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

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).

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.

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.

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

Yes.  Or a previous "git am" run lost "$dotest/last" by a bug and
then the user asked to "am --abort".  Either case, the best you can
do is probably to blow away .git/rebase-apply directory.

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?