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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2013, 18:40 UTC
Message-ID: <7vmwqtua5r.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0mUnNTfJQuGQAoMtnsOXvXU+z+W6D_MMuio1sq4vkucjA@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 11 quoted lines

> 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

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

Show 11 quoted lines

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

Show 7 quoted lines

> 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?