Ramkumar Ramachandra <artagnon@xxxxxxxxx> 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,
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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html