Re: [PATCH] am: support --quit
Duy Nguyenwrites: >> The internal implementation detail of am_abort() is leaking out >> here, by saying "rerere-clear" is the only special thing other than >> recovering the HEAD and working tree state when abort happens. It >> makes readers wonder if am_rerere_clear() should become part of >> am_destroy(). I dunno. > > I think the original design is am_destroy takes care of > $GIT_DIR/rebase-apply and nothing else. --abort has to clean up things > outside (index, HEAD, rerere) while a successful operation should not > leave anything else to clean up (except rebase-apply dir). Yes, and that is why I think the code would have been nicer to understand if the update to add 'reset' had turned the existing am_abort() into a helper that is one level higher in the abstraction (perhaps even by renaming the function) that the caller can tell which part to clear (i.e. e.g. am_finish(, AM_CLEAR_ALL) vs am_finish(, AM_CLEAR_STEP_ONLY)). Stepping back even further, perhaps the call made to am_destroy() in the normal exit case at the end of am_run() could have been using the same helper.
Re: [PATCH] am: support --quit
On Thu, Feb 15, 2018 at 2:26 AM, Junio C Hamanowrote: > Nguyễn Thái Ngọc Duy writes: > >> Among the "in progress" commands, only git-am and git-merge do not >> support --quit. Support --quit in git-am too. > > That's a strange way to phrase it, when the number of commands that > know and do not know it are about the same. Arghh!! I thought I deleted that "git merge" part. That command is not really in the same group as revert/rebase/cherry-pick/am. It only has --continue, aborting is via "git reset"... probably because it's older than most. >> @@ -2359,6 +2363,10 @@ int cmd_am(int argc, const char **argv, const char >> *prefix) >> case RESUME_ABORT: >> am_abort(); >> break; >> + case RESUME_QUIT: >> + am_rerere_clear(); >> + am_destroy(); >> + break; > > The internal implementation detail of am_abort() is leaking out > here, by saying "rerere-clear" is the only special thing other than > recovering the HEAD and working tree state when abort happens. It > makes readers wonder if am_rerere_clear() should become part of > am_destroy(). I dunno. I think the original design is am_destroy takes care of $GIT_DIR/rebase-apply and nothing else. --abort has to clean up things outside (index, HEAD, rerere) while a successful operation should not leave anything else to clean up (except rebase-apply dir). -- Duy
Re: [PATCH] am: support --quit
Nguyễn Thái Ngọc Duywrites: > Among the "in progress" commands, only git-am and git-merge do not > support --quit. Support --quit in git-am too. That's a strange way to phrase it, when the number of commands that know and do not know it are about the same. > --abort:: > Restore the original branch and abort the patching operation. > > +--quit:: > + Abort the patching operation but keep HEAD and the index > + untouched. OK, I see from the documentation of rebase that the above pair is consistent with how --abort/--quit are done over there. > @@ -2359,6 +2363,10 @@ int cmd_am(int argc, const char **argv, const char > *prefix) > case RESUME_ABORT: > am_abort(); > break; > + case RESUME_QUIT: > + am_rerere_clear(); > + am_destroy(); > + break; The internal implementation detail of am_abort() is leaking out here, by saying "rerere-clear" is the only special thing other than recovering the HEAD and working tree state when abort happens. It makes readers wonder if am_rerere_clear() should become part of am_destroy(). I dunno.