rebase -m: cleanup --strategy-option handling

When handling "--strategy-option" rebase collects the commands into a
struct string_list, then concatenates them into a string, prepending "--"
to each one before splitting the string and removing the "--" prefix.
This is an artifact of the scripted rebase and the need to support
"rebase --preserve-merges". Now that "--preserve-merges" no-longer
exists we can cleanup the way the argument is handled.

The tests for a bad strategy option are adjusted now that
parse_strategy_opts() is no-longer called when starting a rebase. The
fact that it only errors out when running "git rebase --continue" is a
mixed blessing but the next commit will fix the root cause of the
parsing problem so lets not worry about that here.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3bd215c7714a21c062dcf684707a84278fa728f3..511922c6fcba10b845f9d27f197819ca903ea91c 100644 (file)
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -117,7 +117,8 @@ struct rebase_options {
        struct string_list exec;
        int allow_empty_message;
        int rebase_merges, rebase_cousins;
-       char *strategy, *strategy_opts;
+       char *strategy;
+       struct string_list strategy_opts;
        struct strbuf git_format_patch_opt;
        int reschedule_failed_exec;
        int reapply_cherry_picks;
@@ -143,6 +144,7 @@ struct rebase_options {
                .config_autosquash = -1,                \
                .update_refs = -1,                      \
                .config_update_refs = -1,               \
+               .strategy_opts = STRING_LIST_INIT_NODUP,\
        }
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -176,8 +178,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
                replay.default_strategy = NULL;
        }
 
-       if (opts->strategy_opts)
-               parse_strategy_opts(&replay, opts->strategy_opts);
+       for (size_t i = 0; i < opts->strategy_opts.nr; i++)
+               strvec_push(&replay.xopts, opts->strategy_opts.items[i].string);
 
        if (opts->squash_onto) {
                oidcpy(&replay.squash_onto, opts->squash_onto);
@@ -1013,7 +1015,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
        int ignore_whitespace = 0;
        const char *gpg_sign = NULL;
        const char *rebase_merges = NULL;
-       struct string_list strategy_options = STRING_LIST_INIT_NODUP;
        struct object_id squash_onto;
        char *squash_onto_name = NULL;
        char *keep_base_onto_name = NULL;
@@ -1122,7 +1123,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                         N_("use 'merge-base --fork-point' to refine upstream")),
                OPT_STRING('s', "strategy", &options.strategy,
                           N_("strategy"), N_("use the given merge strategy")),
-               OPT_STRING_LIST('X', "strategy-option", &strategy_options,
+               OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts,
                                N_("option"),
                                N_("pass the argument through to the merge "
                                   "strategy")),
@@ -1436,23 +1437,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
        } else {
                /* REBASE_MERGE */
                if (ignore_whitespace) {
-                       string_list_append(&strategy_options,
+                       string_list_append(&options.strategy_opts,
                                           "ignore-space-change");
                }
        }
 
-       if (strategy_options.nr) {
-               int i;
-
-               if (!options.strategy)
-                       options.strategy = "ort";
-
-               strbuf_reset(&buf);
-               for (i = 0; i < strategy_options.nr; i++)
-                       strbuf_addf(&buf, " --%s",
-                                   strategy_options.items[i].string);
-               options.strategy_opts = xstrdup(buf.buf);
-       }
+       if (options.strategy_opts.nr && !options.strategy)
+               options.strategy = "ort";
 
        if (options.strategy) {
                options.strategy = xstrdup(options.strategy);
@@ -1827,10 +1818,9 @@ cleanup:
        free(options.gpg_sign_opt);
        string_list_clear(&options.exec, 0);
        free(options.strategy);
-       free(options.strategy_opts);
+       string_list_clear(&options.strategy_opts, 0);
        strbuf_release(&options.git_format_patch_opt);
        free(squash_onto_name);
        free(keep_base_onto_name);
-       string_list_clear(&strategy_options, 0);
        return !!ret;
 }

diff --git a/sequencer.c b/sequencer.c
index 50d469812a58b98893841d72cf96edf2c17a09eb..587a473d6e5594249397977e3a83721d2b2f2e8f 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -2913,7 +2913,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
        return 0;
 }
 
-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
+static void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
 {
        int i;
        int count;

diff --git a/sequencer.h b/sequencer.h
index 8a79d6b200e86d4b28ff8c6a1e5d91af60aca768..913a0f652d9ab356bc066f162fa1e5197f6900eb 100644 (file)
--- a/sequencer.h
+++ b/sequencer.h
@@ -252,7 +252,6 @@ int read_oneliner(struct strbuf *buf,
        const char *path, unsigned flags);
 int read_author_script(const char *path, char **name, char **email, char **date,
                       int allow_missing);
-void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
 int write_basic_state(struct replay_opts *opts, const char *head_name,
                      struct commit *onto, const struct object_id *orig_head);
 void sequencer_post_commit_cleanup(struct repository *r, int verbose);

diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index c3184c9ade95861d9e30164dee9c6ba81f3616f3..3adf42f47d1b2a7461a8a0935327a661ddbaa3dc 100755 (executable)
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -41,19 +41,25 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '
+       test_when_finished "test_might_fail git rebase --abort" &&
        cat >expect <<-\EOF &&
        fatal: could not split '\''--bad'\'': unclosed quote
        EOF
-       test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual &&
+       GIT_SEQUENCE_EDITOR="echo break >" \
+               git rebase -i -X"bad argument\"" side main &&
+       test_expect_code 128 git rebase --continue >out 2>actual &&
        test_must_be_empty out &&
        test_cmp expect actual
 '
 
 test_expect_success 'bad -X <strategy-option> arguments: bad escape' '
+       test_when_finished "test_might_fail git rebase --abort" &&
        cat >expect <<-\EOF &&
        fatal: could not split '\''--bad'\'': cmdline ends with \
        EOF
-       test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual &&
+       GIT_SEQUENCE_EDITOR="echo break >" \
+               git rebase -i -X"bad escape \\" side main &&
+       test_expect_code 128 git rebase --continue >out 2>actual &&
        test_must_be_empty out &&
        test_cmp expect actual
 '