Here is another round of my patch, hopefully I have addressed all the changes.
- I have introduced a separate commit which changes `reset_merge` to use
`argv_array` instead of a manual `char *argv`. This will avoid specifying
array size and index and make code easier to read and extend
- Removed _() from BUG() messages
- Rearrange comments under `sequencer_skip`. I have not changed switch-case to
if-else since the former looked better to me
Thanks
Rohit Ashiwal (5):
sequencer: add advice for revert
sequencer: rename reset_for_rollback to reset_merge
sequencer: use argv_array in 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 | 131 ++++++++++++++++++++++++++----
sequencer.h | 1 +
t/t3510-cherry-pick-sequence.sh | 122 ++++++++++++++++++++++++++++
8 files changed, 258 insertions(+), 26 deletions(-)
Range-diff:
1: 59fa13c4d8 ! 1: 67c212090d sequencer: add advice for revert
@@ -41,7 +41,7 @@
+ _("try \"git cherry-pick (--continue | --abort |
--quit)\"");
+ break;
+ default:
-+ BUG(_("unexpected action in create_seq_dir"));
++ BUG("unexpected action in create_seq_dir");
+ }
+ }
+ if (in_progress_error) {
2: dea4582591 = 2: 300d6f64f0 sequencer: rename reset_for_rollback to
reset_merge
-: ---------- > 3: edc35f6a4c sequencer: use argv_array in reset_merge
3: 29686d828f ! 4: 825486c22d cherry-pick/revert: add --skip option
@@ -11,9 +11,8 @@
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".
+ 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 <[email protected]>
@@ -96,31 +95,6 @@
--- a/sequencer.c
+++ b/sequencer.c
@@
-
- static int reset_merge(const struct object_id *oid)
- {
-- const char *argv[4]; /* reset --merge <arg> + NULL */
-+ int ret;
-+ struct argv_array argv = ARGV_ARRAY_INIT;
-
-- 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));
-+
-+ ret = run_command_v_opt(argv.argv, RUN_GIT_CMD);
-+ argv_array_clear(&argv);
-+
-+ return ret;
- }
-
- static int rollback_single_pick(struct repository *r)
-@@
return reset_merge(&head_oid);
}
@@ -146,48 +120,40 @@
+ sequencer_get_last_command(r, &action);
+
+ /*
-+ * opts->action tells us which subcommand requested to skip
-+ * the commit.
++ * 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 .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"));
++ 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) {
-+ if (!rollback_is_safe())
-+ goto give_advice;
-+ else
-+ break;
-+ }
-+ return error(_("no cherry-pick in progress"));
++ if (action != REPLAY_PICK)
++ return error(_("no cherry-pick in progress"));
++ if (!rollback_is_safe())
++ goto give_advice;
+ }
+ break;
+ default:
4: 941e73b654 ! 5: 63dbc11ab1 cherry-pick/revert: advise using --skip
@@ -62,7 +62,7 @@
+ _("try \"git cherry-pick (--continue | %s--abort |
--quit)\"");
break;
default:
- BUG(_("unexpected action in create_seq_dir"));
+ BUG("unexpected action in create_seq_dir");
@@
}
if (in_progress_error) {
--
2.21.0
