Re: [PATCH] am: handle stray $dotest directory case
Ramkumar Ramachandra artag...@gmail.com 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. Fix the bug by checking for a stray $dotest directory explicitly and removing it on --abort. Reported-by: Junio C Hamano gits...@pobox.com Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- git-am.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/git-am.sh b/git-am.sh index 1cf3d1d..f46a123 100755 --- a/git-am.sh +++ b/git-am.sh @@ -506,6 +506,11 @@ then esac rm -f $dotest/dirtyindex else + # Possible stray $dotest directory + if test -d $dotest test t = $abort; then + clean_abort + fi + # Make sure we are not given --skip, --resolved, nor --abort test $skip$resolved$abort = || die $(gettext Resolve operation not in progress, we are not resuming.) -- 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
Re: [PATCH] am: handle stray $dotest directory case
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? -- 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
Re: [PATCH] am: handle stray $dotest directory case
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, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] am: handle stray $dotest directory case
Junio C Hamano wrote: Hmph, when did ORIG_HEAD set, and what commit does it point at? By some unrelated previous operation (eg. pull, rebase, merge). The point is that at any point in normal operation, ORIG_HEAD exists, and usually points to @~N, for some N. If I rm .git/ORIG_HEAD by hand, the am --abort obviously errors out: $ git am --abort fatal: Not a valid object name ORIG_HEAD fatal: ambiguous argument 'ORIG_HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git command [revision...] -- [file...]' 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). Exactly. It might be nice to fix those two things (are there any observed bugs?), but it is entirely orthogonal to our issue. 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. Yeah, the commit message is lacking. 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? am --skip and am --resolved will error out, saying Resolve operation not in progress, we are not resuming. The message needs to be tweaked. am anothermbox will work just fine, implicitly overwriting the existing $dotest directory. But yeah, we could explicitly remove the directory and allow the mkdir -p to re-create it. I'll probably work on these follow-ups in the morning. Thanks for poking. -- 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
Re: [PATCH] am: handle stray $dotest directory case
Ramkumar Ramachandra artag...@gmail.com writes: But that is entirely an independent issue (I am going to agree with you in the end). Exactly. It might be nice to fix those two things (are there any observed bugs?), but it is entirely orthogonal to our issue. OK. 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. Yeah, the commit message is lacking. OK. I'll queue this on 'pu', not in 'next' so that it can be rerolled. I'll probably work on these follow-ups in the morning. Thanks for poking. Thanks. -- 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