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
>
