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.
[PATCH] am: support --quit
Among the "in progress" commands, only git-am and git-merge do not support --quit. Support --quit in git-am too. Signed-off-by: Nguyễn Thái Ngọc Duy--- Documentation/git-am.txt | 6 +- builtin/am.c | 12 ++-- contrib/completion/git-completion.bash | 2 +- t/t4150-am.sh | 12 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 12879e4029..460662e4b9 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -16,7 +16,7 @@ SYNOPSIS [--exclude=] [--include=] [--reject] [-q | --quiet] [--[no-]scissors] [-S[]] [--patch-format=] [( | )...] -'git am' (--continue | --skip | --abort) +'git am' (--continue | --skip | --abort | --quit) DESCRIPTION --- @@ -167,6 +167,10 @@ default. You can use `--no-utf8` to override this. --abort:: Restore the original branch and abort the patching operation. +--quit:: + Abort the patching operation but keep HEAD and the index + untouched. + DISCUSSION -- diff --git a/builtin/am.c b/builtin/am.c index 5bdd2d7578..793c1e2765 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2149,7 +2149,8 @@ enum resume_mode { RESUME_APPLY, RESUME_RESOLVED, RESUME_SKIP, - RESUME_ABORT + RESUME_ABORT, + RESUME_QUIT }; static int git_am_config(const char *k, const char *v, void *cb) @@ -2249,6 +2250,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_CMDMODE(0, "abort", , N_("restore the original branch and abort the patching operation."), RESUME_ABORT), + OPT_CMDMODE(0, "quit", , + N_("abort the patching operation but keep HEAD where it is."), + RESUME_QUIT), OPT_BOOL(0, "committer-date-is-author-date", _date_is_author_date, N_("lie about committer date")), @@ -2317,7 +2321,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) * stray directories. */ if (file_exists(state.dir) && !state.rebasing) { - if (resume == RESUME_ABORT) { + if (resume == RESUME_ABORT || resume == RESUME_QUIT) { am_destroy(); am_state_release(); return 0; @@ -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; default: die("BUG: invalid resume value"); } diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 88813e9124..c7d5c7af29 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1077,7 +1077,7 @@ _git_am () { __git_find_repo_path if [ -d "$__git_repo_path"/rebase-apply ]; then - __gitcomp "--skip --continue --resolved --abort" + __gitcomp "--skip --continue --resolved --abort --quit" return fi case "$cur" in diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 73b67b4280..512c754e02 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -1045,4 +1045,16 @@ test_expect_success 'am works with multi-line in-body headers' ' git cat-file commit HEAD | grep "^$LONG$" ' +test_expect_success 'am --quit keeps HEAD where it is' ' + mkdir .git/rebase-apply && + >.git/rebase-apply/last && + >.git/rebase-apply/next && + git rev-parse HEAD^ >.git/ORIG_HEAD && + git rev-parse HEAD >expected && + git am --quit && + test_path_is_missing .git/rebase-apply && + git rev-parse HEAD >actual && + test_cmp expected actual +' + test_done -- 2.16.1.435.g8f24da2e1a