revert: Don't implicitly stomp pending sequencer operation

Protect the user from forgetting about a pending sequencer operation
by immediately erroring out when an existing cherry-pick or revert
operation is in progress like:

  $ git cherry-pick foo
  ... conflict ...
  $ git cherry-pick moo
  error: .git/sequencer already exists
  hint: A cherry-pick or revert is in progress
  hint: Use --reset to forget about it
  fatal: cherry-pick failed

A naive version of this would break the following established ways of
working:

  $ git cherry-pick foo
  ... conflict ...
  $ git reset --hard  # I actually meant "moo" when I said "foo"
  $ git cherry-pick moo

  $ git cherry-pick foo
  ... conflict ...
  $ git commit # commit the resolution
  $ git cherry-pick moo # New operation

However, the previous patches "reset: Make reset remove the sequencer
state" and "revert: Remove sequencer state when no commits are
pending" make sure that this does not happen.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/revert.c b/builtin/revert.c
index 000806c7c42b282a0a6c37af8772e01ae9308a5b..19b6739104a6a11bf85b6d3c5015b4481fbd6020 100644 (file)
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -66,6 +66,15 @@ struct replay_opts {
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+static void fatal(const char *advice, ...)
+{
+       va_list params;
+
+       va_start(params, advice);
+       vreportf("fatal: ", advice, params);
+       va_end(params);
+}
+
 static const char *action_name(const struct replay_opts *opts)
 {
        return opts->action == REVERT ? "revert" : "cherry-pick";
@@ -673,13 +682,15 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,
                next = commit_list_append(commit, next);
 }
 
-static void create_seq_dir(void)
+static int create_seq_dir(void)
 {
        const char *seq_dir = git_path(SEQ_DIR);
 
-       if (!(file_exists(seq_dir) && is_directory(seq_dir))
-               && mkdir(seq_dir, 0777) < 0)
+       if (file_exists(seq_dir))
+               return error(_("%s already exists."), seq_dir);
+       else if (mkdir(seq_dir, 0777) < 0)
                die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+       return 0;
 }
 
 static void save_head(const char *head)
@@ -801,9 +812,18 @@ static int pick_revisions(struct replay_opts *opts)
                remove_sequencer_state(1);
                return 0;
        } else {
-               /* Start a new cherry-pick/ revert sequence */
+               /*
+                * Start a new cherry-pick/ revert sequence; but
+                * first, make sure that an existing one isn't in
+                * progress
+                */
+
                walk_revs_populate_todo(&todo_list, opts);
-               create_seq_dir();
+               if (create_seq_dir() < 0) {
+                       fatal(_("A cherry-pick or revert is in progress."));
+                       advise(_("Use --reset to forget about it"));
+                       exit(128);
+               }
                if (get_sha1("HEAD", sha1)) {
                        if (opts->action == REVERT)
                                die(_("Can't revert as initial commit"));

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index a4140865788cd4da0c81626416af420d482fe670..566a15ed8a6261d02fa186368d85d3bc0249d5c9 100755 (executable)
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -106,4 +106,13 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
        test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick base..anotherpick &&
+       test-chmtime -v +0 .git/sequencer >expect &&
+       test_must_fail git cherry-pick unrelatedpick &&
+       test-chmtime -v +0 .git/sequencer >actual &&
+       test_cmp expect actual
+'
+
 test_done