Ramkumar Ramachandra <artag...@gmail.com> writes:

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

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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to