| Message ID | 20190618170650.22721-5-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: > +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")); > +} Two comments. The places touched by patch 1/5 emitted the error followed by an advice message; this new one breaks the pattern by giving the "hint:" first and then error. Be consistent by swapping these two (and return -1, as "error() that returns -1" will no longer be the last thing executed in this function. This one, and the in_progress_advice emitted from the patch 1/5, are both bad in that they make calls to advise() without guarding it with an advice.* configuration variable. This does not allow the user to say say "I've learned this part of Git enough; do not tell me verbosely." Pick a random global variable that is defined near the top of advice.c, and learn how they are set (to true by default, allowing configuration to flip it off) and how they are used in order to prevent a call to advise() getting made. Then mimick that to guard these calls to advise(). Thanks.
Hi Junio On Wed, 19 Jun 2019 20:40:50 -0700 Junio C Hamano <gitster@pobox.com> wrote: > > Two comments. > > The places touched by patch 1/5 emitted the error followed by an > advice message; this new one breaks the pattern by giving the "hint:" > first and then error. Be consistent by swapping these two (and > return -1, as "error() that returns -1" will no longer be the last > thing executed in this function. Yes, it also makes sense otherwise, but this will be more consistent. > This one, and the in_progress_advice emitted from the patch 1/5, are > both bad in that they make calls to advise() without guarding it > with an advice.* configuration variable. This does not allow the > user to say say "I've learned this part of Git enough; do not tell > me verbosely." Oh~ I was missing this point from the start. I'll make proper changes to my patch. > Pick a random global variable that is defined near the top of > advice.c, and learn how they are set (to true by default, allowing > configuration to flip it off) and how they are used in order to > prevent a call to advise() getting made. Then mimick that to guard > these calls to advise(). Ok, I'll look into it and change accordingly. > Thanks. Thanks Rohit
On 20/06/2019 04:40, Junio C Hamano wrote: > Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > >> +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")); >> +} > > Two comments. > > The places touched by patch 1/5 emitted the error followed by an > advice message; this new one breaks the pattern by giving the "hint:" > first and then error. Be consistent by swapping these two (and > return -1, as "error() that returns -1" will no longer be the last > thing executed in this function. > > This one, and the in_progress_advice emitted from the patch 1/5, are > both bad in that they make calls to advise() without guarding it > with an advice.* configuration variable. I'm not sure we have one for cherry-pick/revert/rebase. At the moment they print advice advice for a failed pick unconditionally (the caller of `print_advice()` sets `show_hint` based on the result of the merge rather than user preference) it would be nice to fix that. Maybe that should be checking advice.resolveConflict though. I'm also not sure if that is really within the scope of this patch series. Best Wishes Phillip > This does not allow the > user to say say "I've learned this part of Git enough; do not tell > me verbosely." > > Pick a random global variable that is defined near the top of > advice.c, and learn how they are set (to true by default, allowing > configuration to flip it off) and how they are used in order to > prevent a call to advise() getting made. Then mimick that to guard > these calls to advise(). > > Thanks. >
Hi Rohit On 18/06/2019 18:06, 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 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 | 69 ++++++++++++++++++++ > sequencer.h | 1 + > t/t3510-cherry-pick-sequence.sh | 102 ++++++++++++++++++++++++++++++ > 7 files changed, 183 insertions(+), 6 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index 754b16ce0c..83ce51aedf 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..665e065ee3 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 6762a5f485..5720cd1c85 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2761,6 +2761,15 @@ static int rollback_single_pick(struct repository *r) > return reset_merge(&head_oid); > } > > +static int skip_single_pick(void) > +{ > + struct object_id head; > + > + if (read_ref_full("HEAD", 0, &head, NULL)) > + return error(_("cannot resolve HEAD")); > + return reset_merge(&head); > +} > + > int sequencer_rollback(struct repository *r, struct replay_opts *opts) > { > FILE *f; > @@ -2810,6 +2819,66 @@ 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); > + > + /* > + * Check whether the subcommand requested to skip the commit is actually > + * in progress and that it's safe to skip the commit. > + * > + * opts->action tells us which subcommand requested to skip the commit. > + * If the corresponding .git/<ACTION>_HEAD exists, we know that the > + * action is in progress and we can skip the commit. > + * > + * Otherwise we check that the last instruction was related to the > + * particular subcommand we're trying to execute and barf if that's not > + * the case. > + * > + * Finally we check that the rollback is "safe", i.e., has the HEAD > + * moved? In this case, it doesn't make sense to "reset the merge" and > + * "skip the commit" as the user already handled this by committing. But > + * we'd not want to barf here, instead give advice on how to proceed. We > + * only need to check that when .git/<ACTION>_HEAD doesn't exist because > + * it gets removed when the user commits, so if it still exists we're > + * sure the user can't have committed before. > + */ Thanks for updating the comment, I think it is easier to follow now (Thanks to Thomas for suggesting it as well) > + switch (opts->action) { > + case REPLAY_REVERT: > + if (!file_exists(git_path_revert_head(r))) { > + if (action != REPLAY_REVERT) > + return error(_("no revert in progress")); > + if (!rollback_is_safe()) > + goto give_advice; > + } > + break; > + case REPLAY_PICK: > + if (!file_exists(git_path_cherry_pick_head(r))) { > + if (action != REPLAY_PICK) > + return error(_("no cherry-pick in progress")); > + if (!rollback_is_safe()) > + goto give_advice; > + } > + break; > + default: > + BUG("unexpected action in sequencer_skip"); > + } > + > + if (skip_single_pick()) > + 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..dc0ac8343c 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_i18ncmp 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_i18ncmp 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 If you change this to --continue rather than --skip the test also passes! I think we could fix this by checking if HEAD has changed if CHERRY_PICK_HEAD/REVERT_HEAD is missing and not dropping the last command in the todo list in that case when we continue. Best Wishes Phillip > +' > + > test_expect_success '--quit does not complain when no cherry-pick is in progress' ' > pristine_detach initial && > git cherry-pick --quit >
Hi Phillip On 2019-06-20 10:02 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > > > +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 > > If you change this to --continue rather than --skip the test also > passes! I think we could fix this by checking if HEAD has changed if > CHERRY_PICK_HEAD/REVERT_HEAD is missing and not dropping the last > command in the todo list in that case when we continue. I don't think I fully understood this. At this point --skip is essentially --continue. How is checking unmoved HEAD and unchanged todo uniquely related to --skip flag (or for that matter any _flag_)? Thanks Rohit
Hi Rohit On 20/06/2019 11:34, Rohit Ashiwal wrote: > Hi Phillip > > On 2019-06-20 10:02 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >> >>> +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 >> >> If you change this to --continue rather than --skip the test also >> passes! I think we could fix this by checking if HEAD has changed if >> CHERRY_PICK_HEAD/REVERT_HEAD is missing and not dropping the last >> command in the todo list in that case when we continue. > > I don't think I fully understood this. At this point --skip is essentially > --continue. How is checking unmoved HEAD and unchanged todo uniquely related > to --skip flag (or for that matter any _flag_)? My point is that --continue should reschedule the failed pick and try to pick it again - it should not silently skip a failed pick and --skip should skip it. Best Wishes Phillip > > Thanks > Rohit >
Phillip Wood <phillip.wood123@gmail.com> writes: >> This one, and the in_progress_advice emitted from the patch 1/5, are >> both bad in that they make calls to advise() without guarding it >> with an advice.* configuration variable. > > I'm not sure we have one for cherry-pick/revert/rebase. At the moment > they print advice advice for a failed pick unconditionally... Yes, 1/5 does not introduce a new problem; it just makes it worse by allowing the misdesign survive another update. The one introduced by 4/5 is genuinely new. > ... Maybe that > should be checking advice.resolveConflict though. I think that is a sensible one, rather than inventing a new knob.
Hi Phillip On 2019-06-20 11:42 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Rohit > > On 20/06/2019 11:34, Rohit Ashiwal wrote: >> Hi Phillip >> >> On 2019-06-20 10:02 UTC Phillip Wood <phillip.wood123@gmail.com> wrote: >>> >>>> +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 >>> >>> If you change this to --continue rather than --skip the test also >>> passes! I think we could fix this by checking if HEAD has changed if >>> CHERRY_PICK_HEAD/REVERT_HEAD is missing and not dropping the last >>> command in the todo list in that case when we continue. >> >> I don't think I fully understood this. At this point --skip is essentially >> --continue. How is checking unmoved HEAD and unchanged todo uniquely related >> to --skip flag (or for that matter any _flag_)? > > My point is that --continue should reschedule the failed pick and try to > pick it again - it should not silently skip a failed pick and --skip > should skip it. So, this is a flaw in the --continue, I guess? Fixing that is beyond the scope of this patch. May be we can launch another series in which we fix this and decouple skip and continue? Thanks Rohit
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 754b16ce0c..83ce51aedf 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..665e065ee3 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 6762a5f485..5720cd1c85 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2761,6 +2761,15 @@ static int rollback_single_pick(struct repository *r) return reset_merge(&head_oid); } +static int skip_single_pick(void) +{ + struct object_id head; + + if (read_ref_full("HEAD", 0, &head, NULL)) + return error(_("cannot resolve HEAD")); + return reset_merge(&head); +} + int sequencer_rollback(struct repository *r, struct replay_opts *opts) { FILE *f; @@ -2810,6 +2819,66 @@ 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); + + /* + * Check whether the subcommand requested to skip the commit is actually + * in progress and that it's safe to skip the commit. + * + * opts->action tells us which subcommand requested to skip the commit. + * If the corresponding .git/<ACTION>_HEAD exists, we know that the + * action is in progress and we can skip the commit. + * + * Otherwise we check that the last instruction was related to the + * particular subcommand we're trying to execute and barf if that's not + * the case. + * + * Finally we check that the rollback is "safe", i.e., has the HEAD + * moved? In this case, it doesn't make sense to "reset the merge" and + * "skip the commit" as the user already handled this by committing. But + * we'd not want to barf here, instead give advice on how to proceed. We + * only need to check that when .git/<ACTION>_HEAD doesn't exist because + * it gets removed when the user commits, so if it still exists we're + * sure the user can't have committed before. + */ + switch (opts->action) { + case REPLAY_REVERT: + if (!file_exists(git_path_revert_head(r))) { + if (action != REPLAY_REVERT) + return error(_("no revert in progress")); + if (!rollback_is_safe()) + goto give_advice; + } + break; + case REPLAY_PICK: + if (!file_exists(git_path_cherry_pick_head(r))) { + if (action != REPLAY_PICK) + return error(_("no cherry-pick in progress")); + if (!rollback_is_safe()) + goto give_advice; + } + break; + default: + BUG("unexpected action in sequencer_skip"); + } + + if (skip_single_pick()) + 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..dc0ac8343c 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_i18ncmp 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_i18ncmp 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 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 | 69 ++++++++++++++++++++ sequencer.h | 1 + t/t3510-cherry-pick-sequence.sh | 102 ++++++++++++++++++++++++++++++ 7 files changed, 183 insertions(+), 6 deletions(-)