| Message ID | 20190613040504.32317-3-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index 754b16ce0c..955880ab88 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -10,9 +10,7 @@ SYNOPSIS > [verse] > 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] > [-S[<keyid>]] <commit>... > -'git cherry-pick' --continue > -'git cherry-pick' --quit > -'git cherry-pick' --abort > +'git cherry-pick' --continue | --skip | --abort | --quit Is this correct, or do we need to enclose these choices inside (), i.e. 'git cherry-pick' ( --continue | --skip | --abort | --quit ) ? > -static int rollback_single_pick(struct repository *r) > +static int rollback_single_pick(struct repository *r, unsigned int is_skip) > { > struct object_id head_oid; > > if (!file_exists(git_path_cherry_pick_head(r)) && > - !file_exists(git_path_revert_head(r))) > + !file_exists(git_path_revert_head(r)) && !is_skip) > return error(_("no cherry-pick or revert in progress")); > if (read_ref_full("HEAD", 0, &head_oid, NULL)) > return error(_("cannot resolve HEAD")); > - if (is_null_oid(&head_oid)) > + if (is_null_oid(&head_oid) && !is_skip) > return error(_("cannot abort from a branch yet to be born")); > return reset_for_rollback(&head_oid); > } It is unclear *why* the function (and more importantly, its callers) would want to omit two sanity checks when is_skip is in effect. Before this patch introduced such conditional behaviour, the name was descriptive enough for this single-purpose function that is a file-local helper, but it is no longer a case. The function needs a bit of commentary before it. When &&-chaining error checks that are optional, check the condition that makes the error checks optional first, i.e. if (!is_skip && !file_exists(...) && !file_exists(...)) return error(...); The same comment applies to the "do not barf by checking is-null-oid under is-skip mode, as that is a sign that we are on an unborn branch and reset-for-rollback knows how to handle it now". It may even be a good idea to group the checks that are guarded by the condition for readability, i.e. if (!is_skip && (!file_exists(...) && !file_exists(...))) return error(...); > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > +{ > + enum replay_action action = -1; > + sequencer_get_last_command(r, &action); > + > + switch (opts->action) { > + case REPLAY_REVERT: > + if (!file_exists(git_path_revert_head(r))) { > + if (action == REPLAY_REVERT) { > + if (!rollback_is_safe()) > + goto give_advice; > + else > + break; > + } > + return error(_("no revert in progress")); > + } This part probably deserves a bit of in-code comment. The Git subcommand (i.e. opts->action) tells us that we are asked to "git revert --skip". When REVERT_HEAD is not there, we look at the last command of the sequencer state and make sure it is 'revert'; all other cases we barf. That much we can read from the code. But what are "all other cases"? Do we cover a single-revert case (i.e. "git revert <commit>" got conflict and the user is saying "git revert --skip")? Was the user in the middle of "git rebase -i" and the last command before we gave the control back was 'pick'? > + break; > + case REPLAY_PICK: > + if (!file_exists(git_path_cherry_pick_head(r))) { > + if (action == REPLAY_PICK) { > + if (!rollback_is_safe()) > + goto give_advice; > + else > + break; > + } > + return error(_("no cherry-pick in progress")); > + } > + break; > + default: > + BUG("the control must not reach here"); > + } > + > + if (rollback_single_pick(r, 1)) > + return error(_("failed to skip the commit")); And this takes us back to the previous comment. By passing '1' here, this caller is asking the callee to omit certain sanity check the original version of the callee used to do. What makes it an appropriate thing to do so here? "Because we reach at this point under such and such condition, we would never have CHERRY_PICK_HEAD or REVERT_HEAD---we do not want it to barf" is a good answer (and no, do not merely give answer to me in your response---write the answer as in-code comment to help future readers of the code). "Because when we come here, sometimes the XXX_HEAD must exist but some other times XXX_HEAD may not exist, so insisting that either exists would make the function fail" is *NOT* a good answer, on the other hand. Somebody must still check that the necessary file exists when it must exist. Thanks.
Hi Rohit This is definitely moving in the right direction. On 13/06/2019 05:05, Rohit Ashiwal wrote: > git am or rebase have a --skip flag to skip the current commit if the > user wishes to do so. During a cherry-pick or revert a user could > likewise skip a commit, but needs to use 'git reset' (or in the case > of conflicts 'git reset --merge'), followed by 'git (cherry-pick | > revert) --continue' to skip the commit. This is more annoying and > sometimes confusing on the users' part. Add a `--skip` option to make > skipping commits easier for the user and to make the commands more > consistent. > > In the next commit, we will change the advice messages and some tests > hence finishing the process of teaching revert and cherry-pick > "how to skip commits". > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > changes: > - Remove `reset_merge` from last revision[1] and modified `reset_for_rollback` > and signature of `rollback_single_pick` to handle if `--skip` flag > - Modify test which gave "ambiguous refname" warning[2] > > [1]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#m11a573a3b91dfd2fcfae40c6ecc2148f8c92e10c > [2]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#m586cb8930cfcb737e4f19662f0eef3716b1e9d35 > > Documentation/git-cherry-pick.txt | 4 +- > Documentation/git-revert.txt | 4 +- > Documentation/sequencer.txt | 4 ++ > builtin/revert.c | 5 ++ > sequencer.c | 67 +++++++++++++++++--- > sequencer.h | 1 + > t/t3510-cherry-pick-sequence.sh | 102 ++++++++++++++++++++++++++++++ > 7 files changed, 171 insertions(+), 16 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index 754b16ce0c..955880ab88 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -10,9 +10,7 @@ SYNOPSIS > [verse] > 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] > [-S[<keyid>]] <commit>... > -'git cherry-pick' --continue > -'git cherry-pick' --quit > -'git cherry-pick' --abort > +'git cherry-pick' --continue | --skip | --abort | --quit > > DESCRIPTION > ----------- > diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt > index 0c82ca5bc0..ffce98099c 100644 > --- a/Documentation/git-revert.txt > +++ b/Documentation/git-revert.txt > @@ -9,9 +9,7 @@ SYNOPSIS > -------- > [verse] > 'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>... > -'git revert' --continue > -'git revert' --quit > -'git revert' --abort > +'git revert' --continue | --skip | --abort | --quit > > DESCRIPTION > ----------- > diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt > index 5a57c4a407..3bceb56474 100644 > --- a/Documentation/sequencer.txt > +++ b/Documentation/sequencer.txt > @@ -3,6 +3,10 @@ > `.git/sequencer`. Can be used to continue after resolving > conflicts in a failed cherry-pick or revert. > > +--skip:: > + Skip the current commit and continue with the rest of the > + sequence. > + > --quit:: > Forget about the current operation in progress. Can be used > to clear the sequencer state after a failed cherry-pick or > diff --git a/builtin/revert.c b/builtin/revert.c > index d4dcedbdc6..5dc5891ea2 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -102,6 +102,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), > OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), > OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), > + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'), > OPT_CLEANUP(&cleanup_arg), > OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), > OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), > @@ -151,6 +152,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > this_operation = "--quit"; > else if (cmd == 'c') > this_operation = "--continue"; > + else if (cmd == 's') > + this_operation = "--skip"; > else { > assert(cmd == 'a'); > this_operation = "--abort"; > @@ -210,6 +213,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > return sequencer_continue(the_repository, opts); > if (cmd == 'a') > return sequencer_rollback(the_repository, opts); > + if (cmd == 's') > + return sequencer_skip(the_repository, opts); > return sequencer_pick_revisions(the_repository, opts); > } > > diff --git a/sequencer.c b/sequencer.c > index 918cb5d761..6b78a26920 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2733,25 +2733,26 @@ static int rollback_is_safe(void) > > static int reset_for_rollback(const struct object_id *oid) > { > - const char *argv[4]; /* reset --merge <arg> + NULL */ > + struct argv_array argv = ARGV_ARRAY_INIT; /* reset --merge <arg> + NULL */ > > - argv[0] = "reset"; > - argv[1] = "--merge"; > - argv[2] = oid_to_hex(oid); > - argv[3] = NULL; > - return run_command_v_opt(argv, RUN_GIT_CMD); > + argv_array_pushl(&argv, "reset", "--merge", NULL); > + > + if (!is_null_oid(oid)) > + argv_array_push(&argv, oid_to_hex(oid)); > + argv_array_pushl() copies the strings so you need to call argv_array_clear() when you're done with it. > + return run_command_v_opt(argv.argv, RUN_GIT_CMD);> } > > -static int rollback_single_pick(struct repository *r) > +static int rollback_single_pick(struct repository *r, unsigned int is_skip) > { > struct object_id head_oid; > > if (!file_exists(git_path_cherry_pick_head(r)) && > - !file_exists(git_path_revert_head(r))) > + !file_exists(git_path_revert_head(r)) && !is_skip) > return error(_("no cherry-pick or revert in progress")); > if (read_ref_full("HEAD", 0, &head_oid, NULL)) > return error(_("cannot resolve HEAD")); > - if (is_null_oid(&head_oid)) > + if (is_null_oid(&head_oid) && !is_skip) > return error(_("cannot abort from a branch yet to be born")); The changes here look good. One thing though - this still does not check if we're on the final pick or not. The more I think about it the more I think it shouldn't matter but I'm bothered about why abort refuses to run on the null oid. I've cc'd Jonathan in case he can remember why this code is here. I looked at the discussion [1] of the patch [2] that introduced this but that bit doesn't seem to have been discussed. [1] https://public-inbox.org/git/20111211195836.GA25482@elie.hsd1.il.comcast.net/T/#u [2] https://github.com/git/git/commit/539047c19ec040819b6f6af2d55714195b812abb > return reset_for_rollback(&head_oid); > } > @@ -2770,7 +2771,7 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) > * If CHERRY_PICK_HEAD or REVERT_HEAD indicates > * a single-cherry-pick in progress, abort that. > */ > - return rollback_single_pick(r); > + return rollback_single_pick(r, 0); > } > if (!f) > return error_errno(_("cannot open '%s'"), git_path_head_file()); > @@ -2805,6 +2806,52 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) > return -1; > } > > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > +{ > + enum replay_action action = -1; > + sequencer_get_last_command(r, &action); > + > + switch (opts->action) { > + case REPLAY_REVERT: > + if (!file_exists(git_path_revert_head(r))) { > + if (action == REPLAY_REVERT) { > + if (!rollback_is_safe()) > + goto give_advice; > + else > + break; > + } > + return error(_("no revert in progress")); > + } > + break; > + case REPLAY_PICK: > + if (!file_exists(git_path_cherry_pick_head(r))) { > + if (action == REPLAY_PICK) { > + if (!rollback_is_safe()) > + goto give_advice; > + else > + break; > + } > + return error(_("no cherry-pick in progress")); > + } > + break; > + default: > + BUG("the control must not reach here"); > + } > + > + if (rollback_single_pick(r, 1)) > + return error(_("failed to skip the commit")); > + if (!is_directory(git_path_seq_dir())) > + return 0; > + > + return sequencer_continue(r, opts); > + > +give_advice: > + advise(_("have you committed already?\n" > + "try \"git %s --continue\""), > + action == REPLAY_REVERT ? "revert" : "cherry-pick"); > + return error(_("there is nothing to skip")); > +} > + > static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) > { > struct lock_file todo_lock = LOCK_INIT; > diff --git a/sequencer.h b/sequencer.h > index 0c494b83d4..731b9853eb 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -129,6 +129,7 @@ int sequencer_pick_revisions(struct repository *repo, > struct replay_opts *opts); > int sequencer_continue(struct repository *repo, struct replay_opts *opts); > int sequencer_rollback(struct repository *repo, struct replay_opts *opts); > +int sequencer_skip(struct repository *repo, struct replay_opts *opts); > int sequencer_remove_state(struct replay_opts *opts); > > #define TODO_LIST_KEEP_EMPTY (1U << 0) > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > index 941d5026da..f85ef51cac 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -93,6 +93,108 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' ' > test_path_is_missing .git/sequencer > ' > > +test_expect_success 'cherry-pick --skip requires cherry-pick in progress' ' > + pristine_detach initial && > + test_must_fail git cherry-pick --skip > +' > + > +test_expect_success 'revert --skip requires revert in progress' ' > + pristine_detach initial && > + test_must_fail git revert --skip > +' > + > +test_expect_success 'cherry-pick --skip to skip commit' ' > + pristine_detach initial && > + test_must_fail git cherry-pick anotherpick && > + test_must_fail git revert --skip && > + git cherry-pick --skip && > + test_cmp_rev initial HEAD && > + test_path_is_missing .git/CHERRY_PICK_HEAD > +' > + > +test_expect_success 'revert --skip to skip commit' ' > + pristine_detach anotherpick && > + test_must_fail git revert anotherpick~1 && > + test_must_fail git cherry-pick --skip && > + git revert --skip && > + test_cmp_rev anotherpick HEAD > +' > + > +test_expect_success 'skip "empty" commit' ' > + pristine_detach picked && > + test_commit dummy foo d && > + test_must_fail git cherry-pick anotherpick && > + git cherry-pick --skip && > + test_cmp_rev dummy HEAD > +' > + > +test_expect_success 'skip a commit and check if rest of sequence is correct' ' > + pristine_detach initial && > + echo e >expect && > + cat >expect.log <<-EOF && > + OBJID > + :100644 100644 OBJID OBJID M foo > + OBJID > + :100644 100644 OBJID OBJID M foo > + OBJID > + :100644 100644 OBJID OBJID M unrelated > + OBJID > + :000000 100644 OBJID OBJID A foo > + :000000 100644 OBJID OBJID A unrelated > + EOF > + test_must_fail git cherry-pick base..yetanotherpick && > + test_must_fail git cherry-pick --skip && > + echo d >foo && > + git add foo && > + git cherry-pick --continue && > + { > + git rev-list HEAD | > + git diff-tree --root --stdin | > + sed "s/$OID_REGEX/OBJID/g" > + } >actual.log && > + test_cmp expect foo && > + test_cmp expect.log actual.log > +' > + > +test_expect_success 'check advice when we move HEAD by committing' ' > + pristine_detach initial && > + cat >expect <<-EOF && > + hint: have you committed already? > + hint: try "git cherry-pick --continue" > + error: there is nothing to skip > + fatal: cherry-pick failed > + EOF > + test_must_fail git cherry-pick base..yetanotherpick && > + echo c >foo && > + git commit -a && > + test_path_is_missing .git/CHERRY_PICK_HEAD && > + test_must_fail git cherry-pick --skip 2>advice && > + test_cmp expect advice When you compare a translated string you need to use test_i18ncmp to avoid test breakages when running under GET_TEXT_POISON. The CI suite shows this up [3]. There are instructions for running tests under travis in Documentation/SubmittingPatches.txt or Johannes can advise you about using his CI setup with pull requests on gitgitgadget (and maybe git.git?) Best Wishes Phillip [3] https://travis-ci.org/phillipwood/git/jobs/545251777 > +' > + > +test_expect_success 'allow skipping commit but not abort for a new history' ' > + pristine_detach initial && > + cat >expect <<-EOF && > + error: cannot abort from a branch yet to be born > + fatal: cherry-pick failed > + EOF > + git checkout --orphan new_disconnected && > + git reset --hard && > + test_must_fail git cherry-pick anotherpick && > + test_must_fail git cherry-pick --abort 2>advice && > + git cherry-pick --skip && > + test_cmp expect advice > +' > + > +test_expect_success 'allow skipping stopped cherry-pick because of untracked file modifications' ' > + pristine_detach initial && > + git rm --cached unrelated && > + git commit -m "untrack unrelated" && > + test_must_fail git cherry-pick initial base && > + test_path_is_missing .git/CHERRY_PICK_HEAD && > + git cherry-pick --skip > +' > + > test_expect_success '--quit does not complain when no cherry-pick is in progress' ' > pristine_detach initial && > git cherry-pick --quit >
Junio C Hamano <gitster@pobox.com> writes: >> -static int rollback_single_pick(struct repository *r) >> +static int rollback_single_pick(struct repository *r, unsigned int is_skip) >> { >> ... >> + if (is_null_oid(&head_oid) && !is_skip) >> return error(_("cannot abort from a branch yet to be born")); >> return reset_for_rollback(&head_oid); >> } > > It is unclear *why* the function (and more importantly, its callers) > would want to omit two sanity checks when is_skip is in effect. > ... >> + default: >> + BUG("the control must not reach here"); >> + } >> + >> + if (rollback_single_pick(r, 1)) >> + return error(_("failed to skip the commit")); > > And this takes us back to the previous comment. By passing '1' > here, this caller is asking the callee to omit certain sanity check > the original version of the callee used to do. What makes it an > appropriate thing to do so here? I think my earlier comments would lead to a wrong direction, i.e. to justify the change made to rollback_single_pick(), so let's step back a bit. Perhaps the change is unjustifiable and that is why I had trouble reading it and trying to make sense out of it. Is it possible that the new callsite that passes is_skip==1 should not be calling it (while castrating many parts of the callee) in the first place? Perhaps it is doing something _different_ from being called "rollback single pick" (or perhaps the name of the function is not specific enough to describe what its existing caller, i.e. the one that passes is_skip==0 after your patch, calls it for)? IOW, would it lead to a better code structure if you left the original rollback_single_pick() helper and its caller alone (perhaps rename it to make it clearer what it does), and *add* a new helper around the underlying reset_for_rollback() function and call it from here? Perhaps it is not rolling back but is skipping, so the new function needs to be called skip_single_pick() or something, and the existing one is named correctly and there is no need for even renaming?
Hi Junio On 2019-06-13 17:56 UTC Junio C Hamano <gitster@pobox.com> wrote: > > > +'git cherry-pick' --continue | --skip | --abort | --quit > > Is this correct, or do we need to enclose these choices inside (), > i.e. > > 'git cherry-pick' ( --continue | --skip | --abort | --quit ) > > ? Documentation of `git rebase` also lists these options without the '('s so, I thought to make it similar to that. > It is unclear *why* the function (and more importantly, its callers) > would want to omit two sanity checks when is_skip is in effect. > > Before this patch introduced such conditional behaviour, the name > was descriptive enough for this single-purpose function that is a > file-local helper, but it is no longer a case. The function needs a > bit of commentary before it. > > When &&-chaining error checks that are optional, check the condition > that makes the error checks optional first, i.e. > > if (!is_skip && > !file_exists(...) && !file_exists(...)) > return error(...); > > [...] > > no, do not merely give answer to me in your response---write the > answer as in-code comment to help future readers of the code). > > "Because when we come here, sometimes the XXX_HEAD must exist but > some other times XXX_HEAD may not exist, so insisting that either > exists would make the function fail" is *NOT* a good answer, on the > other hand. Somebody must still check that the necessary file > exists when it must exist. Yes, I should have added some comments. I'll add them in next revision. Thanks Rohit
Hi Junio On 2019-06-13 19:57 UTC Junio C Hamano <gitster@pobox.com> wrote: > > I think my earlier comments would lead to a wrong direction, i.e. to > justify the change made to rollback_single_pick(), so let's step > back a bit. Perhaps the change is unjustifiable and that is why I > had trouble reading it and trying to make sense out of it. > > Is it possible that the new callsite that passes is_skip==1 should > not be calling it (while castrating many parts of the callee) in the > first place? Perhaps it is doing something _different_ from being > called "rollback single pick" (or perhaps the name of the function > is not specific enough to describe what its existing caller, i.e. the > one that passes is_skip==0 after your patch, calls it for)? IOW, > would it lead to a better code structure if you left the original > rollback_single_pick() helper and its caller alone (perhaps rename > it to make it clearer what it does), and *add* a new helper around > the underlying reset_for_rollback() function and call it from here? That is why I added a new function, but I was not super sure if it was a good idea to begin with, since it only introduced small changes to `reset_for_rollback` (also Phillip[1] discouraged it so, I stopped thinking in that direction). > Perhaps it is not rolling back but is skipping, so the new function > needs to be called skip_single_pick() or something, and the existing > one is named correctly and there is no need for even renaming? We could have a wrapper function to `reset_for_rollback` and better rename it to `reset_merge` since we *are* resetting a merge which translates to rolling-back in reality? Thanks Rohit [1]: https://public-inbox.org/git/6d3c1c1e-6140-dd8c-c37f-8c625b04ddc9@gmail.com/
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: >> Is this correct, or do we need to enclose these choices inside (), >> i.e. >> >> 'git cherry-pick' ( --continue | --skip | --abort | --quit ) >> >> ? > > Documentation of `git rebase` also lists these options without the > '('s so, I thought to make it similar to that. As long as you picked the ones that correctly spell their SYNOPSIS, it is OK to model yours after them. Seeing how "git-am.txt", "git-cat-file.txt", "git-checkout.txt", ... "git-var.txt" etc. all use ( choice1 | choice2 | choice3 ) when one among the choices MUST be chosen, contrasting with [ choice1 | choice2 | choice3 ] when one can be chosen but not using any is also OK, I am not sure if the documentation for "git rebase" is done right and should be used as a model, though.
Hi Junio I am sorry, I missed to reply to these. On 2019-06-13 17:56 UTC Junio C Hamano <gitster@pobox.com> wrote: > > > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > > +{ > > + enum replay_action action = -1; > > + sequencer_get_last_command(r, &action); > > + > > + switch (opts->action) { > > + case REPLAY_REVERT: > > + if (!file_exists(git_path_revert_head(r))) { > > + if (action == REPLAY_REVERT) { > > + if (!rollback_is_safe()) > > + goto give_advice; > > + else > > + break; > > + } > > + return error(_("no revert in progress")); > > + } > > This part probably deserves a bit of in-code comment. > > The Git subcommand (i.e. opts->action) tells us that we are > asked to "git revert --skip". When REVERT_HEAD is not there, we > look at the last command of the sequencer state and make sure it > is 'revert'; all other cases we barf. > > That much we can read from the code. But what are "all other cases"? > Do we cover a single-revert case (i.e. "git revert <commit>" got > conflict and the user is saying "git revert --skip")? Yes, we actually cover the single-revert case. Let's say there were some conflicts then either .git/REVERT_HEAD will exist allowing `git revert --skip` it's way or the user has already dealt with it and committed his way in which case .git/REVERT_HEAD will be removed and reverting ends (as it should). > Was the user > in the middle of "git rebase -i" and the last command before we gave > the control back was 'pick'? rebase -i creates different directories so, we are sure that --skip does not skip in middle of a rebase. Am I right? Hope these answers your questions -- Rohit
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 754b16ce0c..955880ab88 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -10,9 +10,7 @@ SYNOPSIS [verse] 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] [-S[<keyid>]] <commit>... -'git cherry-pick' --continue -'git cherry-pick' --quit -'git cherry-pick' --abort +'git cherry-pick' --continue | --skip | --abort | --quit DESCRIPTION ----------- diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index 0c82ca5bc0..ffce98099c 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -9,9 +9,7 @@ SYNOPSIS -------- [verse] 'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>... -'git revert' --continue -'git revert' --quit -'git revert' --abort +'git revert' --continue | --skip | --abort | --quit DESCRIPTION ----------- diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt index 5a57c4a407..3bceb56474 100644 --- a/Documentation/sequencer.txt +++ b/Documentation/sequencer.txt @@ -3,6 +3,10 @@ `.git/sequencer`. Can be used to continue after resolving conflicts in a failed cherry-pick or revert. +--skip:: + Skip the current commit and continue with the rest of the + sequence. + --quit:: Forget about the current operation in progress. Can be used to clear the sequencer state after a failed cherry-pick or diff --git a/builtin/revert.c b/builtin/revert.c index d4dcedbdc6..5dc5891ea2 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -102,6 +102,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'), OPT_CLEANUP(&cleanup_arg), OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), @@ -151,6 +152,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) this_operation = "--quit"; else if (cmd == 'c') this_operation = "--continue"; + else if (cmd == 's') + this_operation = "--skip"; else { assert(cmd == 'a'); this_operation = "--abort"; @@ -210,6 +213,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) return sequencer_continue(the_repository, opts); if (cmd == 'a') return sequencer_rollback(the_repository, opts); + if (cmd == 's') + return sequencer_skip(the_repository, opts); return sequencer_pick_revisions(the_repository, opts); } diff --git a/sequencer.c b/sequencer.c index 918cb5d761..6b78a26920 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2733,25 +2733,26 @@ static int rollback_is_safe(void) static int reset_for_rollback(const struct object_id *oid) { - const char *argv[4]; /* reset --merge <arg> + NULL */ + struct argv_array argv = ARGV_ARRAY_INIT; /* reset --merge <arg> + NULL */ - argv[0] = "reset"; - argv[1] = "--merge"; - argv[2] = oid_to_hex(oid); - argv[3] = NULL; - return run_command_v_opt(argv, RUN_GIT_CMD); + argv_array_pushl(&argv, "reset", "--merge", NULL); + + if (!is_null_oid(oid)) + argv_array_push(&argv, oid_to_hex(oid)); + + return run_command_v_opt(argv.argv, RUN_GIT_CMD); } -static int rollback_single_pick(struct repository *r) +static int rollback_single_pick(struct repository *r, unsigned int is_skip) { struct object_id head_oid; if (!file_exists(git_path_cherry_pick_head(r)) && - !file_exists(git_path_revert_head(r))) + !file_exists(git_path_revert_head(r)) && !is_skip) return error(_("no cherry-pick or revert in progress")); if (read_ref_full("HEAD", 0, &head_oid, NULL)) return error(_("cannot resolve HEAD")); - if (is_null_oid(&head_oid)) + if (is_null_oid(&head_oid) && !is_skip) return error(_("cannot abort from a branch yet to be born")); return reset_for_rollback(&head_oid); } @@ -2770,7 +2771,7 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) * If CHERRY_PICK_HEAD or REVERT_HEAD indicates * a single-cherry-pick in progress, abort that. */ - return rollback_single_pick(r); + return rollback_single_pick(r, 0); } if (!f) return error_errno(_("cannot open '%s'"), git_path_head_file()); @@ -2805,6 +2806,52 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) return -1; } +int sequencer_skip(struct repository *r, struct replay_opts *opts) +{ + enum replay_action action = -1; + sequencer_get_last_command(r, &action); + + switch (opts->action) { + case REPLAY_REVERT: + if (!file_exists(git_path_revert_head(r))) { + if (action == REPLAY_REVERT) { + if (!rollback_is_safe()) + goto give_advice; + else + break; + } + return error(_("no revert in progress")); + } + break; + case REPLAY_PICK: + if (!file_exists(git_path_cherry_pick_head(r))) { + if (action == REPLAY_PICK) { + if (!rollback_is_safe()) + goto give_advice; + else + break; + } + return error(_("no cherry-pick in progress")); + } + break; + default: + BUG("the control must not reach here"); + } + + if (rollback_single_pick(r, 1)) + return error(_("failed to skip the commit")); + if (!is_directory(git_path_seq_dir())) + return 0; + + return sequencer_continue(r, opts); + +give_advice: + advise(_("have you committed already?\n" + "try \"git %s --continue\""), + action == REPLAY_REVERT ? "revert" : "cherry-pick"); + return error(_("there is nothing to skip")); +} + static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) { struct lock_file todo_lock = LOCK_INIT; diff --git a/sequencer.h b/sequencer.h index 0c494b83d4..731b9853eb 100644 --- a/sequencer.h +++ b/sequencer.h @@ -129,6 +129,7 @@ int sequencer_pick_revisions(struct repository *repo, struct replay_opts *opts); int sequencer_continue(struct repository *repo, struct replay_opts *opts); int sequencer_rollback(struct repository *repo, struct replay_opts *opts); +int sequencer_skip(struct repository *repo, struct replay_opts *opts); int sequencer_remove_state(struct replay_opts *opts); #define TODO_LIST_KEEP_EMPTY (1U << 0) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 941d5026da..f85ef51cac 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -93,6 +93,108 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' ' test_path_is_missing .git/sequencer ' +test_expect_success 'cherry-pick --skip requires cherry-pick in progress' ' + pristine_detach initial && + test_must_fail git cherry-pick --skip +' + +test_expect_success 'revert --skip requires revert in progress' ' + pristine_detach initial && + test_must_fail git revert --skip +' + +test_expect_success 'cherry-pick --skip to skip commit' ' + pristine_detach initial && + test_must_fail git cherry-pick anotherpick && + test_must_fail git revert --skip && + git cherry-pick --skip && + test_cmp_rev initial HEAD && + test_path_is_missing .git/CHERRY_PICK_HEAD +' + +test_expect_success 'revert --skip to skip commit' ' + pristine_detach anotherpick && + test_must_fail git revert anotherpick~1 && + test_must_fail git cherry-pick --skip && + git revert --skip && + test_cmp_rev anotherpick HEAD +' + +test_expect_success 'skip "empty" commit' ' + pristine_detach picked && + test_commit dummy foo d && + test_must_fail git cherry-pick anotherpick && + git cherry-pick --skip && + test_cmp_rev dummy HEAD +' + +test_expect_success 'skip a commit and check if rest of sequence is correct' ' + pristine_detach initial && + echo e >expect && + cat >expect.log <<-EOF && + OBJID + :100644 100644 OBJID OBJID M foo + OBJID + :100644 100644 OBJID OBJID M foo + OBJID + :100644 100644 OBJID OBJID M unrelated + OBJID + :000000 100644 OBJID OBJID A foo + :000000 100644 OBJID OBJID A unrelated + EOF + test_must_fail git cherry-pick base..yetanotherpick && + test_must_fail git cherry-pick --skip && + echo d >foo && + git add foo && + git cherry-pick --continue && + { + git rev-list HEAD | + git diff-tree --root --stdin | + sed "s/$OID_REGEX/OBJID/g" + } >actual.log && + test_cmp expect foo && + test_cmp expect.log actual.log +' + +test_expect_success 'check advice when we move HEAD by committing' ' + pristine_detach initial && + cat >expect <<-EOF && + hint: have you committed already? + hint: try "git cherry-pick --continue" + error: there is nothing to skip + fatal: cherry-pick failed + EOF + test_must_fail git cherry-pick base..yetanotherpick && + echo c >foo && + git commit -a && + test_path_is_missing .git/CHERRY_PICK_HEAD && + test_must_fail git cherry-pick --skip 2>advice && + test_cmp expect advice +' + +test_expect_success 'allow skipping commit but not abort for a new history' ' + pristine_detach initial && + cat >expect <<-EOF && + error: cannot abort from a branch yet to be born + fatal: cherry-pick failed + EOF + git checkout --orphan new_disconnected && + git reset --hard && + test_must_fail git cherry-pick anotherpick && + test_must_fail git cherry-pick --abort 2>advice && + git cherry-pick --skip && + test_cmp expect advice +' + +test_expect_success 'allow skipping stopped cherry-pick because of untracked file modifications' ' + pristine_detach initial && + git rm --cached unrelated && + git commit -m "untrack unrelated" && + test_must_fail git cherry-pick initial base && + test_path_is_missing .git/CHERRY_PICK_HEAD && + git cherry-pick --skip +' + test_expect_success '--quit does not complain when no cherry-pick is in progress' ' pristine_detach initial && git cherry-pick --quit
git am or rebase have a --skip flag to skip the current commit if the user wishes to do so. During a cherry-pick or revert a user could likewise skip a commit, but needs to use 'git reset' (or in the case of conflicts 'git reset --merge'), followed by 'git (cherry-pick | revert) --continue' to skip the commit. This is more annoying and sometimes confusing on the users' part. Add a `--skip` option to make skipping commits easier for the user and to make the commands more consistent. In the next commit, we will change the advice messages and some tests hence finishing the process of teaching revert and cherry-pick "how to skip commits". Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- changes: - Remove `reset_merge` from last revision[1] and modified `reset_for_rollback` and signature of `rollback_single_pick` to handle if `--skip` flag - Modify test which gave "ambiguous refname" warning[2] [1]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#m11a573a3b91dfd2fcfae40c6ecc2148f8c92e10c [2]: https://public-inbox.org/git/20190608191958.4593-1-rohit.ashiwal265@gmail.com/T/#m586cb8930cfcb737e4f19662f0eef3716b1e9d35 Documentation/git-cherry-pick.txt | 4 +- Documentation/git-revert.txt | 4 +- Documentation/sequencer.txt | 4 ++ builtin/revert.c | 5 ++ sequencer.c | 67 +++++++++++++++++--- sequencer.h | 1 + t/t3510-cherry-pick-sequence.sh | 102 ++++++++++++++++++++++++++++++ 7 files changed, 171 insertions(+), 16 deletions(-)