| Message ID | 20190616082040.9440-1-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Teach cherry-pick/revert to skip commits | expand |
On 06/16, Rohit Ashiwal wrote: > Yet another iteration of my patch. We have changed the series a little bit. We > now have a commit that rename `reset_for_rollback` to `reset_merge`. A lot of > nit-picks were handled in this revision. Thanks for your work! I allowed myself to nitpick a bit more at this stage :) One other thing I wanted to point out here is range-diff, which can be helpful to include for the benefit of reviewers that saw this series before. See 'man git-range-diff' or the --range-diff flag in 'git format-patch' for more info on how it works. It helps seeing at a glance what changed between versions of a series. For the benefit of other reviewers that might find it helpful, here's one I generated between v3 and v4 of the series:: 1: 8f29142755 ! 1: 99279e617c sequencer: add advice for revert @@ -25,8 +25,8 @@ - error(_("a cherry-pick or revert is already in progress")); - advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); + enum replay_action action; -+ const char *in_progress_advice; + const char *in_progress_error = NULL; ++ const char *in_progress_advice = NULL; + + if (!sequencer_get_last_command(r, &action)) { + switch (action) { @@ -41,7 +41,7 @@ + _("try \"git cherry-pick (--continue | --abort | --quit)\""); + break; + default: -+ BUG(_("the control must not reach here")); ++ BUG(_("unexpected action in create_seq_dir")); + } + } + if (in_progress_error) { -: ---------- > 2: c64aabf2d2 sequencer: rename reset_for_rollback to reset_merge 2: 3bc8678df4 ! 3: 8b483815ca cherry-pick/revert: add --skip option @@ -27,7 +27,7 @@ -'git cherry-pick' --continue -'git cherry-pick' --quit -'git cherry-pick' --abort -+'git cherry-pick' --continue | --skip | --abort | --quit ++'git cherry-pick' (--continue | --skip | --abort | --quit) DESCRIPTION ----------- @@ -42,7 +42,7 @@ -'git revert' --continue -'git revert' --quit -'git revert' --abort -+'git revert' --continue | --skip | --abort | --quit ++'git revert' (--continue | --skip | --abort | --quit) DESCRIPTION ----------- @@ -97,10 +97,11 @@ +++ b/sequencer.c @@ - static int reset_for_rollback(const struct object_id *oid) + static int reset_merge(const struct object_id *oid) { - const char *argv[4]; /* reset --merge <arg> + NULL */ -+ struct argv_array argv = ARGV_ARRAY_INIT; /* reset --merge <arg> + NULL */ ++ int ret; ++ struct argv_array argv = ARGV_ARRAY_INIT; - argv[0] = "reset"; - argv[1] = "--merge"; @@ -112,34 +113,29 @@ + if (!is_null_oid(oid)) + argv_array_push(&argv, oid_to_hex(oid)); + -+ return run_command_v_opt(argv.argv, RUN_GIT_CMD); ++ ret = run_command_v_opt(argv.argv, RUN_GIT_CMD); ++ argv_array_clear(&argv); ++ ++ return ret; } --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); - } + static int rollback_single_pick(struct repository *r) @@ - * 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()); + 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; @@ return -1; } @@ -149,13 +145,35 @@ + enum replay_action action = -1; + sequencer_get_last_command(r, &action); + ++ /* ++ * opts->action tells us which subcommand requested to skip ++ * the commit. ++ */ + switch (opts->action) { + case REPLAY_REVERT: ++ /* ++ * If .git/REVERT_HEAD exists then we are sure that we are in ++ * the middle of a revert and we allow to skip the commit. ++ */ + if (!file_exists(git_path_revert_head(r))) { ++ /* ++ * Check if the last instruction executed was related to ++ * revert. If so, we are sure that a revert is in progress. ++ * ++ * NB: single commit revert is also counted in this ++ * definition of "progress" (and was dealt with in the ++ * previous check). ++ */ + if (action == REPLAY_REVERT) { ++ /* ++ * Check if the user has moved the HEAD, i.e., ++ * already committed. In this case, we would like ++ * to advise instead of skipping. ++ */ + if (!rollback_is_safe()) + goto give_advice; + else ++ /* skip commit :) */ + break; + } + return error(_("no revert in progress")); @@ -173,10 +191,10 @@ + } + break; + default: -+ BUG("the control must not reach here"); ++ BUG("unexpected action in sequencer_skip"); + } + -+ if (rollback_single_pick(r, 1)) ++ if (skip_single_pick()) + return error(_("failed to skip the commit")); + if (!is_directory(git_path_seq_dir())) + return 0; @@ -289,7 +307,7 @@ + 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_i18ncmp expect advice +' + +test_expect_success 'allow skipping commit but not abort for a new history' ' @@ -303,7 +321,7 @@ + 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_i18ncmp expect advice +' + +test_expect_success 'allow skipping stopped cherry-pick because of untracked file modifications' ' 3: a3cd17540b ! 4: 98618e08f4 cherry-pick/revert: advise using --skip @@ -42,8 +42,8 @@ +++ b/sequencer.c @@ enum replay_action action; - const char *in_progress_advice; const char *in_progress_error = NULL; + const char *in_progress_advice = NULL; + unsigned int advise_skip = file_exists(git_path_revert_head(r)) || + file_exists(git_path_cherry_pick_head(r)); @@ -62,7 +62,7 @@ + _("try \"git cherry-pick (--continue | %s--abort | --quit)\""); break; default: - BUG(_("the control must not reach here")); + BUG(_("unexpected action in create_seq_dir")); @@ } if (in_progress_error) { @@ -80,7 +80,7 @@ --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ - test_cmp expect advice + test_i18ncmp expect advice ' +test_expect_success 'selectively advise --skip while launching another sequence' ' @@ -92,7 +92,7 @@ + EOF + test_must_fail git cherry-pick picked..yetanotherpick && + test_must_fail git cherry-pick picked..yetanotherpick 2>advice && -+ test_cmp expect advice && ++ test_i18ncmp expect advice && + cat >expect <<-EOF && + error: cherry-pick is already in progress + hint: try "git cherry-pick (--continue | --abort | --quit)" @@ -100,7 +100,7 @@ + EOF + git reset --merge && + test_must_fail git cherry-pick picked..yetanotherpick 2>advice && -+ test_cmp expect advice ++ test_i18ncmp expect advice +' + test_expect_success 'allow skipping commit but not abort for a new history' ' > Rohit Ashiwal (4): > sequencer: add advice for revert > sequencer: rename reset_for_rollback to reset_merge > cherry-pick/revert: add --skip option > cherry-pick/revert: advise using --skip > > Documentation/git-cherry-pick.txt | 4 +- > Documentation/git-revert.txt | 4 +- > Documentation/sequencer.txt | 4 + > builtin/commit.c | 13 +-- > builtin/revert.c | 5 ++ > sequencer.c | 139 ++++++++++++++++++++++++++---- > sequencer.h | 1 + > t/t3510-cherry-pick-sequence.sh | 122 ++++++++++++++++++++++++++ > 8 files changed, 266 insertions(+), 26 deletions(-) > > -- > 2.21.0 >