| Message ID | 20190608191958.4593-3-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
On 06/09, Rohit Ashiwal wrote: > git am or rebase advise the user to use `git (am | rebase) --skip` to > skip the commit. cherry-pick and revert also have this concept of > skipping commits but they advise the user to use `git reset` (or in > case of a patch which had conflicts, `git reset --merge`) which on the > user's part is annoying and sometimes confusing. Add a `--skip` option > to make these commands more consistent. Something I missed in the off-list review for this PR: I think this commit message is a bit heavy on the advice part, rather than on the real advantage the --skip flag brings, which is that it's less cumbersome for the user to skip a commit, and brings consistency to the commands. Maybe something like: 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 hence finishing > the process of teaching revert and cherry-pick "how to skip commits". > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > Documentation/git-cherry-pick.txt | 4 +- > Documentation/git-revert.txt | 4 +- > Documentation/sequencer.txt | 4 ++ > builtin/revert.c | 5 +++ > sequencer.c | 23 +++++++++++ > sequencer.h | 1 + > t/t3510-cherry-pick-sequence.sh | 63 +++++++++++++++++++++++++++++++ > 7 files changed, 98 insertions(+), 6 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 9c561a041b..f586e677d3 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) > return -1; > } > > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > +{ > + switch (opts->action) { > + case REPLAY_REVERT: > + if (!file_exists(git_path_revert_head(r))) > + return error(_("no revert in progress")); > + break; > + case REPLAY_PICK: > + if (!file_exists(git_path_cherry_pick_head(r))) > + return error(_("no cherry-pick in progress")); > + break; I see that in some cases (e.g 'rollback_single_pick'), we just check if either REVERT_HEAD or CHERRY_PICK_HEAD exist, but would allow the "wrong" command to be used, e.g. 'git revert --abort' when a cherry-pick is in progress. This helps avoiding that, good! > + default: > + BUG("the control must not reach here."); We don't support rebase in 'sequencer_skip' yet, makes sense as this patch is focused on cherry-pick and revert. There's nothing preventing supporting that in the future though. > + } > + > + if (rollback_single_pick(r)) > + return error(_("failed to skip the commit")); > + if (!is_directory(git_path_seq_dir())) > + return 0; If there's no sequencer directory, we don't need to continue the cherry-pick/revert, as there's nothing left to do. Otherwise we continue. Good. So this looks good to me. > + > + return sequencer_continue(r, opts); > +} > + > 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..48cc9f13ee 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -93,6 +93,69 @@ 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 '--quit does not complain when no cherry-pick is in progress' ' > pristine_detach initial && > git cherry-pick --quit > -- > 2.21.0 >
Hi Rohit --skip is definitely a useful addition to cherry-pick/revert On 08/06/2019 20:19, Rohit Ashiwal wrote: > git am or rebase advise the user to use `git (am | rebase) --skip` to > skip the commit. cherry-pick and revert also have this concept of > skipping commits but they advise the user to use `git reset` (or in > case of a patch which had conflicts, `git reset --merge`) which on the > user's part is annoying and sometimes confusing. Add a `--skip` option > to make these commands more consistent. > > In the next commit, we will change the advice messages hence finishing > the process of teaching revert and cherry-pick "how to skip commits". > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > Documentation/git-cherry-pick.txt | 4 +- > Documentation/git-revert.txt | 4 +- > Documentation/sequencer.txt | 4 ++ > builtin/revert.c | 5 +++ > sequencer.c | 23 +++++++++++ > sequencer.h | 1 + > t/t3510-cherry-pick-sequence.sh | 63 +++++++++++++++++++++++++++++++ > 7 files changed, 98 insertions(+), 6 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 9c561a041b..f586e677d3 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) > return -1; > } > > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > +{ > + switch (opts->action) { > + case REPLAY_REVERT: > + if (!file_exists(git_path_revert_head(r))) > + return error(_("no revert in progress")); This error message is slightly misleading. If the user has already committed a conflict resolution then REVERT_HEAD/CHERRY_PICK_HEAD will not exist but it is possible that a cherry-pick/revert is in progress if the user was picking a sequence of commits. It would be nice to give a different error message or maybe just a warning in that case. sequencer_get_last_command() should help with that. > + break; > + case REPLAY_PICK: > + if (!file_exists(git_path_cherry_pick_head(r))) > + return error(_("no cherry-pick in progress")); > + break; > + default: > + BUG("the control must not reach here."); > + } > + > + if (rollback_single_pick(r)) > + return error(_("failed to skip the commit")); If rollback_single_pick() sees that HEAD is the null oid then it gives the error "cannot abort from a branch yet to be born". We're not aborting and if we're picking a sequence of commits the skip ought succeed, but it won't at the moment. If we're picking a single commit then the skip should probably fail like abort but with an appropriate message. Admittedly that's all a bit of a corner case. > + if (!is_directory(git_path_seq_dir())) > + return 0; > + > + return sequencer_continue(r, opts); > +} > + > 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..48cc9f13ee 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -93,6 +93,69 @@ 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 && It would be good to check that the cherry-pick has progressed since --skip and didn't just fail without trying to pick another commit. Best Wishes Phillip > + 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 '--quit does not complain when no cherry-pick is in progress' ' > pristine_detach initial && > git cherry-pick --quit >
Hey Phillip On Sun, 9 Jun 2019 19:01:35 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Rohit > > --skip is definitely a useful addition to cherry-pick/revert > > On 08/06/2019 20:19, Rohit Ashiwal wrote: > > > > [...] > > @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) > > return -1; > > } > > > > +int sequencer_skip(struct repository *r, struct replay_opts *opts) > > +{ > > + switch (opts->action) { > > + case REPLAY_REVERT: > > + if (!file_exists(git_path_revert_head(r))) > > + return error(_("no revert in progress")); > > This error message is slightly misleading. If the user has already > committed a conflict resolution then REVERT_HEAD/CHERRY_PICK_HEAD will > not exist but it is possible that a cherry-pick/revert is in progress if > the user was picking a sequence of commits. It would be nice to give a > different error message or maybe just a warning in that case. > sequencer_get_last_command() should help with that. Yes, .git/{REVERT|CHERRY_PICK}_HEAD will not exist in this case, but in case of cherry-picking/reverting: 1. multiple commits: sequencer dir will exist which will throw out the error listed under `create_seq_dir` 2. single commit: Then ACTION is already over and there is nothing to do and the error is correct. > > + break; > > + case REPLAY_PICK: > > + if (!file_exists(git_path_cherry_pick_head(r))) > > + return error(_("no cherry-pick in progress")); > > + break; > > + default: > > + BUG("the control must not reach here."); > > + } > > + > > + if (rollback_single_pick(r)) > > + return error(_("failed to skip the commit")); > > If rollback_single_pick() sees that HEAD is the null oid then it gives > the error "cannot abort from a branch yet to be born". We're not > aborting and if we're picking a sequence of commits the skip ought > succeed, but it won't at the moment. If we're picking a single commit > then the skip should probably fail like abort but with an appropriate > message. Admittedly that's all a bit of a corner case. Yes, you are right here. We could actually modify the advice there to be more like _("cannot perform the specified action, the branch is yet to be born") and I think it should suffice this. What do you think? > > [...] > > +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 && > > It would be good to check that the cherry-pick has progressed since > --skip and didn't just fail without trying to pick another commit. The overall test tests that only, if cherry-pick --skip "failed" then we won't get 'e' inside of `foo` and `test_cmp expect foo` will also fail and if it skipped wrongly then expect.log will not match the actual.log and `test_cmp` will fail. Am I missing something here? Please tell if so. > Best Wishes > > Phillip Thanks Rohit
Hi Rohit On 10/06/2019 06:57, Rohit Ashiwal wrote: > Hey Phillip > > On Sun, 9 Jun 2019 19:01:35 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> Hi Rohit >> >> --skip is definitely a useful addition to cherry-pick/revert >> >> On 08/06/2019 20:19, Rohit Ashiwal wrote: >>> >>> [...] >>> @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) >>> return -1; >>> } >>> >>> +int sequencer_skip(struct repository *r, struct replay_opts *opts) >>> +{ >>> + switch (opts->action) { >>> + case REPLAY_REVERT: >>> + if (!file_exists(git_path_revert_head(r))) >>> + return error(_("no revert in progress")); >> >> This error message is slightly misleading. If the user has already >> committed a conflict resolution then REVERT_HEAD/CHERRY_PICK_HEAD will >> not exist but it is possible that a cherry-pick/revert is in progress if >> the user was picking a sequence of commits. It would be nice to give a >> different error message or maybe just a warning in that case. >> sequencer_get_last_command() should help with that. It's actually a bit more complicated as if the cherry-pick failed because it would have overwriten untracked files then CHERRY_PICK_HEAD will not exist but we want to be able to skip that pick. So it should not error out in that case either. (I think you may be able to use the abort safety file (see rollback_is_safe()) to distinguish the 'failed to pick case' from the 'user committed a conflict resolution' case.) > Yes, .git/{REVERT|CHERRY_PICK}_HEAD will not exist in this case, but > in case of cherry-picking/reverting: > > 1. multiple commits: > sequencer dir will exist which will throw out the error listed > under `create_seq_dir` I don't understand. Wont it will error out here? Why would we call create_seq_dir() for --skip? > 2. single commit: > Then ACTION is already over and there is nothing to do and the > error is correct. > >>> + break; >>> + case REPLAY_PICK: >>> + if (!file_exists(git_path_cherry_pick_head(r))) >>> + return error(_("no cherry-pick in progress")); >>> + break; >>> + default: >>> + BUG("the control must not reach here."); >>> + } >>> + >>> + if (rollback_single_pick(r)) >>> + return error(_("failed to skip the commit")); >> >> If rollback_single_pick() sees that HEAD is the null oid then it gives >> the error "cannot abort from a branch yet to be born". We're not >> aborting and if we're picking a sequence of commits the skip ought >> succeed, but it won't at the moment. If we're picking a single commit >> then the skip should probably fail like abort but with an appropriate >> message. Admittedly that's all a bit of a corner case. > > Yes, you are right here. We could actually modify the advice there > to be more like _("cannot perform the specified action, the branch > is yet to be born") and I think it should suffice this. What do you > think? I think it should allow the user to skip if there are more commits to pick . Just changing the error message does not fix that. > >>> [...] >>> +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 && >> >> It would be good to check that the cherry-pick has progressed since >> --skip and didn't just fail without trying to pick another commit. > > The overall test tests that only, if cherry-pick --skip "failed" then > we won't get 'e' inside of `foo` and `test_cmp expect foo` will also > fail and if it skipped wrongly then expect.log will not match the > actual.log and `test_cmp` will fail. Am I missing something here? > Please tell if so. You're right that the tests at the end would probably pick up a failure, but I'm concerned that there could be some obscure corner case we've not thought of so checking HEAD and the file contents here would be an additional safety measure. It also makes it easier for someone tracking down a test failure to see what happened. If they rely only on the test at the end they need to spend time to understand where the mismatched contents came from. Best Wishes Phillip > >> Best Wishes >> >> Phillip > > Thanks > Rohit >
Hi Phillip On 2019-06-10 10:40 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > > [...] > It's actually a bit more complicated as if the cherry-pick failed > because it would have overwriten untracked files then CHERRY_PICK_HEAD > will not exist but we want to be able to skip that pick. So it should > not error out in that case either. (I think you may be able to use the > abort safety file (see rollback_is_safe()) to distinguish the 'failed to > pick case' from the 'user committed a conflict resolution' case.) Oh! I was thinking about some other case. (spawing another cherry-pick, which is wrong since the topic is --skip). I'm sorry. >> Yes, .git/{REVERT|CHERRY_PICK}_HEAD will not exist in this case, but >> in case of cherry-picking/reverting: >> >> 1. multiple commits: >> sequencer dir will exist which will throw out the error listed >> under `create_seq_dir` > > I don't understand. Wont it will error out here? Why would we call > create_seq_dir() for --skip? No, you are correct. This won't skip commit in this case. I'll change it to do the required. >>> If rollback_single_pick() sees that HEAD is the null oid then it gives >>> the error "cannot abort from a branch yet to be born". We're not >>> aborting and if we're picking a sequence of commits the skip ought >>> succeed, but it won't at the moment. If we're picking a single commit >>> then the skip should probably fail like abort but with an appropriate >>> message. Admittedly that's all a bit of a corner case. >> >> Yes, you are right here. We could actually modify the advice there >> to be more like _("cannot perform the specified action, the branch >> is yet to be born") and I think it should suffice this. What do you >> think? > > I think it should allow the user to skip if there are more commits to > pick . Just changing the error message does not fix that. Right! I'll check what can be done here. >> The overall test tests that only, if cherry-pick --skip "failed" then >> we won't get 'e' inside of `foo` and `test_cmp expect foo` will also >> fail and if it skipped wrongly then expect.log will not match the >> actual.log and `test_cmp` will fail. Am I missing something here? >> Please tell if so. > > You're right that the tests at the end would probably pick up a failure, > but I'm concerned that there could be some obscure corner case we've not > thought of so checking HEAD and the file contents here would be an > additional safety measure. It also makes it easier for someone tracking > down a test failure to see what happened. If they rely only on the test > at the end they need to spend time to understand where the mismatched > contents came from. Yes, it is worth checking here if HEAD after cherry-picking is in the correct position, same for the file foo. I'll change this test too. Thanks for pointing out. Thanks for the review Rohit
Hi Rohit On 10/06/2019 14:43, Rohit Ashiwal wrote: > Hi Phillip > > On 2019-06-10 10:40 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> [...] >> It's actually a bit more complicated as if the cherry-pick failed >> because it would have overwriten untracked files then CHERRY_PICK_HEAD >> will not exist but we want to be able to skip that pick. So it should >> not error out in that case either. (I think you may be able to use the >> abort safety file (see rollback_is_safe()) to distinguish the 'failed to >> pick case' from the 'user committed a conflict resolution' case.) > > Oh! I was thinking about some other case. (spawing another cherry-pick, > which is wrong since the topic is --skip). I'm sorry. > >>> Yes, .git/{REVERT|CHERRY_PICK}_HEAD will not exist in this case, but >>> in case of cherry-picking/reverting: >>> >>> 1. multiple commits: >>> sequencer dir will exist which will throw out the error listed >>> under `create_seq_dir` >> >> I don't understand. Wont it will error out here? Why would we call >> create_seq_dir() for --skip? > > No, you are correct. This won't skip commit in this case. I'll change > it to do the required. Shout if you get stuck. I think distinguishing the "user committed a conflict resolution and so we don't want to skip" from the "the pick would have overwritten an untracked file so we do want to skip" cases should be possible by calling rollback_is_safe(). It would be good to have a test case for at least the "pick would have overwritten an untracked file case" as that would be easy to break in the future without noticing as it's a rare situation. > >>>> If rollback_single_pick() sees that HEAD is the null oid then it gives >>>> the error "cannot abort from a branch yet to be born". We're not >>>> aborting and if we're picking a sequence of commits the skip ought >>>> succeed, but it won't at the moment. If we're picking a single commit >>>> then the skip should probably fail like abort but with an appropriate >>>> message. Admittedly that's all a bit of a corner case. >>> >>> Yes, you are right here. We could actually modify the advice there >>> to be more like _("cannot perform the specified action, the branch >>> is yet to be born") and I think it should suffice this. What do you >>> think? >> >> I think it should allow the user to skip if there are more commits to >> pick . Just changing the error message does not fix that. > > Right! I'll check what can be done here. I think you can just pass a flag to rollback_single_pick() to tell it whether it is rolling back for --skip or --abort so it can do the right thing. Best Wishes Phillip > >>> The overall test tests that only, if cherry-pick --skip "failed" then >>> we won't get 'e' inside of `foo` and `test_cmp expect foo` will also >>> fail and if it skipped wrongly then expect.log will not match the >>> actual.log and `test_cmp` will fail. Am I missing something here? >>> Please tell if so. >> >> You're right that the tests at the end would probably pick up a failure, >> but I'm concerned that there could be some obscure corner case we've not >> thought of so checking HEAD and the file contents here would be an >> additional safety measure. It also makes it easier for someone tracking >> down a test failure to see what happened. If they rely only on the test >> at the end they need to spend time to understand where the mismatched >> contents came from. > > Yes, it is worth checking here if HEAD after cherry-picking is in the > correct position, same for the file foo. I'll change this test too. > Thanks for pointing out. > > Thanks for the review > 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 9c561a041b..f586e677d3 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts) return -1; } +int sequencer_skip(struct repository *r, struct replay_opts *opts) +{ + switch (opts->action) { + case REPLAY_REVERT: + if (!file_exists(git_path_revert_head(r))) + return error(_("no revert in progress")); + break; + case REPLAY_PICK: + if (!file_exists(git_path_cherry_pick_head(r))) + return error(_("no cherry-pick in progress")); + break; + default: + BUG("the control must not reach here."); + } + + if (rollback_single_pick(r)) + return error(_("failed to skip the commit")); + if (!is_directory(git_path_seq_dir())) + return 0; + + return sequencer_continue(r, opts); +} + 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..48cc9f13ee 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -93,6 +93,69 @@ 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 '--quit does not complain when no cherry-pick is in progress' ' pristine_detach initial && git cherry-pick --quit
git am or rebase advise the user to use `git (am | rebase) --skip` to skip the commit. cherry-pick and revert also have this concept of skipping commits but they advise the user to use `git reset` (or in case of a patch which had conflicts, `git reset --merge`) which on the user's part is annoying and sometimes confusing. Add a `--skip` option to make these commands more consistent. In the next commit, we will change the advice messages hence finishing the process of teaching revert and cherry-pick "how to skip commits". Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- Documentation/git-cherry-pick.txt | 4 +- Documentation/git-revert.txt | 4 +- Documentation/sequencer.txt | 4 ++ builtin/revert.c | 5 +++ sequencer.c | 23 +++++++++++ sequencer.h | 1 + t/t3510-cherry-pick-sequence.sh | 63 +++++++++++++++++++++++++++++++ 7 files changed, 98 insertions(+), 6 deletions(-)