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

Ramkumar Ramachandra [off-list ref] writes:

The following bug has been observed since rr/rebase-autostash:

  $ git am  # no input file
  ^C
  $ git am --abort
  Resolve operation not in progress, we are not resuming.

This happens because the following test fails:

  test -d "$dotest" && test -f "$dotest/last" && test -f "$dotest/next"

and am precludes the possibility of a stray $dotest directory
existing (when $dotest/{last,next} are not present).

Why did the original code sequence that read:

	if test -d "$dotest"
        then
		... handle skip, resolved, abort, because
                ... these can be run ONLY when we know we have
                ... started an "am" session.
		... catch new "git am mbox" invocation and error
                ... out, because that should not be allowed when
                ... we know we have started an "am" session.

had to change its guarding condition to

	if test -d "$dotest" && test -f "$dotest/last" && test -f "$dotest/next"

in the first place?  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.

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?