| Message ID | 20230225180325.796624-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase: add a config option for --rebase-merges | expand |
Hi Alex! It was great to see you at Review Club today. If you'd like the notes, they are available at https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w though reviewers will send their feedback to the mailing list anyway, so don't feel compelled to address feedback that isn't on the mailing list :) Alex Henrie <alexhenrie24@gmail.com> writes: > Changes from v4: > > [...] > > Suggestions not incorporated: > > [...] > I would have appreciated a sentence or two explaining the high level changes in this series, since that would save reviewers from having to read the whole series to get an idea of the overall intent. For this series, I think the subject line captures the intent pretty well, though even simple descriptions like "Add a config option. Also clean up some problems with the --rebase-merges flag." can be useful framing for reviewers. I appreciate the summary of changes [not] incorporated, I found it useful for getting an idea of what was originally discussed. On the whole, I think the config option will be quite welcome, and the series is mostly mergeable, but might benefit from some documentation tweaking.
On Wed, Mar 1, 2023 at 4:14 PM Glen Choo <chooglen@google.com> wrote: > > Hi Alex! It was great to see you at Review Club today. If you'd like the > notes, they are available at > > https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w > > though reviewers will send their feedback to the mailing list anyway, so > don't feel compelled to address feedback that isn't on the mailing list > :) Hi Glenn, Thanks for hosting the call today. I was impressed by the commitment to having discussion and making decisions on the public mailing list, "as though the call never happened". It was a pleasant change from other open source projects I have contributed to where decisions are sometimes made behind closed doors with little to no opportunity to ask questions. Thanks again for making me think a little harder about my patches; I expect to send a v6 by the end of the week. -Alex
On Wed, Mar 1, 2023 at 10:02 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > Hi Glenn, Typo: Glen. No matter how much I proofread, I seem to always notice typos right after I send the email...
Changes from v4: - deprecate --rebase-merges="" rather than removing it outright - follow the established convention for what "" and NULL mean as booleans - add tailored error message for conflicting options and tests for it - rename parse_opt_merges to parse_opt_rebase_merges to avoid confusion with parse_opt_merge - similarly, rename parse_merges_value to parse_rebase_merges_value - move null check from parse_rebase_merges_value to parse_opt_rebase_merges - remove tests that check whether the config system itself works Suggestions not incorporated: - remove the callback function - make --rebase-merge without an argument override rebase.merges=rebase-cousins - make rebase.merge accept only a subset of the possible boolean values, or change the meanings of some of those values - make --rebase-merge="" and rebase.merge="" do different things without warning - remove tests that verify that the command line option properly overrides the config option Thanks to Johannes, Phillip, and Junio for your help making these patches better. If you feel strongly about one of the unincorporated suggestions, let's continue the discussion and try to figure out how to make it happen. Alex Henrie (3): rebase: add documentation and test for --no-rebase-merges rebase: deprecate --rebase-merges="" rebase: add a config option for --rebase-merges Documentation/config/rebase.txt | 10 ++++ Documentation/git-rebase.txt | 5 +- builtin/rebase.c | 75 ++++++++++++++++++------- t/t3422-rebase-incompatible-options.sh | 12 ++++ t/t3430-rebase-merges.sh | 78 ++++++++++++++++++++++++++ 5 files changed, 160 insertions(+), 20 deletions(-) Range-diff against v4: 1: e6d44a194c = 1: 76e38ef9f8 rebase: add documentation and test for --no-rebase-merges 2: 393b43c4e1 ! 2: c6099e6dee rebase: stop accepting --rebase-merges="" @@ Metadata Author: Alex Henrie <alexhenrie24@gmail.com> ## Commit message ## - rebase: stop accepting --rebase-merges="" + rebase: deprecate --rebase-merges="" The unusual syntax --rebase-merges="" (that is, --rebase-merges with an empty string argument) has been an undocumented synonym of - --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid + --rebase-merges=no-rebase-cousins. Deprecate that syntax to avoid confusion when a rebase.merges config option is introduced, where - rebase.merges="" will be equivalent to not passing --rebase-merges. + rebase.merges="" will be equivalent to --no-rebase-merges. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> @@ builtin/rebase.c: 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, @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) - imply_merge(&options, "--exec"); if (rebase_merges) { -- if (!*rebase_merges) + if (!*rebase_merges) - ; /* default mode; do nothing */ -- else if (!strcmp("rebase-cousins", rebase_merges)) -+ if (!strcmp("rebase-cousins", rebase_merges)) ++ warning(_("--rebase-merges with an empty string " ++ "argument is deprecated and will stop " ++ "working in a future version of Git. Use " ++ "--rebase-merges=no-rebase-cousins " ++ "instead.")); + else if (!strcmp("rebase-cousins", rebase_merges)) options.rebase_cousins = 1; else if (strcmp("no-rebase-cousins", rebase_merges)) - die(_("Unknown mode: %s"), rebase_merges); ## t/t3430-rebase-merges.sh ## @@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless asked for' ' EOF ' -+test_expect_success '--rebase-merges="" is invalid syntax' ' -+ echo "fatal: Unknown mode: " >expect && -+ test_must_fail git rebase --rebase-merges="" HEAD^ 2>actual && -+ test_cmp expect actual ++test_expect_success '--rebase-merges="" is deprecated' ' ++ git rebase --rebase-merges="" HEAD^ 2>actual && ++ grep deprecated actual +' + test_expect_success 'refs/rewritten/* is worktree-local' ' 3: b1b6fbfa86 ! 3: 95cba9588c rebase: add a config option for --rebase-merges @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below. have `<upstream>` as direct ancestor will keep their original branch point, ## builtin/rebase.c ## +@@ builtin/rebase.c: struct rebase_options { + int fork_point; + int update_refs; + int config_autosquash; ++ int config_rebase_merges; + int config_update_refs; + }; + +@@ builtin/rebase.c: struct rebase_options { + .allow_empty_message = 1, \ + .autosquash = -1, \ + .config_autosquash = -1, \ ++ .rebase_merges = -1, \ ++ .config_rebase_merges = -1, \ + .update_refs = -1, \ + .config_update_refs = -1, \ + } @@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts) return status ? -1 : 0; } -+static void parse_merges_value(struct rebase_options *options, const char *value) ++static void parse_rebase_merges_value(struct rebase_options *options, const char *value) +{ -+ if (value) { -+ if (!strcmp("no-rebase-cousins", value)) -+ options->rebase_cousins = 0; -+ else if (!strcmp("rebase-cousins", value)) -+ options->rebase_cousins = 1; -+ else -+ die(_("Unknown mode: %s"), value); -+ } -+ -+ options->rebase_merges = 1; ++ if (!strcmp("no-rebase-cousins", value)) ++ options->rebase_cousins = 0; ++ else if (!strcmp("rebase-cousins", value)) ++ options->rebase_cousins = 1; ++ else ++ die(_("Unknown rebase-merges mode: %s"), value); +} + static int rebase_config(const char *var, const char *value, void *data) @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v return 0; } -+ if (!strcmp(var, "rebase.merges") && value && *value) { -+ opts->rebase_merges = git_parse_maybe_bool(value); -+ if (opts->rebase_merges < 0) -+ parse_merges_value(opts, value); ++ if (!strcmp(var, "rebase.merges")) { ++ opts->config_rebase_merges = git_parse_maybe_bool(value); ++ if (opts->config_rebase_merges < 0) { ++ opts->config_rebase_merges = 1; ++ parse_rebase_merges_value(opts, value); ++ } + return 0; + } + - if (!strcmp(var, "rebase.backend")) { - return git_config_string(&opts->default_backend, var, value); - } + if (!strcmp(var, "rebase.updaterefs")) { + opts->config_update_refs = git_config_bool(var, value); + return 0; @@ builtin/rebase.c: static int parse_opt_empty(const struct option *opt, const char *arg, int unset) return 0; } -+static int parse_opt_merges(const struct option *opt, const char *arg, int unset) ++static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset) +{ + struct rebase_options *options = opt->value; + -+ if (unset) -+ options->rebase_merges = 0; -+ else -+ parse_merges_value(options, arg); ++ options->rebase_merges = !unset; ++ ++ if (arg) { ++ if (!*arg) { ++ warning(_("--rebase-merges with an empty string " ++ "argument is deprecated and will stop " ++ "working in a future version of Git. Use " ++ "--rebase-merges=no-rebase-cousins " ++ "instead.")); ++ arg = "no-rebase-cousins"; ++ } ++ parse_rebase_merges_value(options, arg); ++ } + + return 0; +} @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix + OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"), N_("try to rebase merges instead of skipping them"), - PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"}, -+ PARSE_OPT_OPTARG, parse_opt_merges), ++ PARSE_OPT_OPTARG, parse_opt_rebase_merges), OPT_BOOL(0, "fork-point", &options.fork_point, N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix imply_merge(&options, "--exec"); - if (rebase_merges) { -- if (!strcmp("rebase-cousins", rebase_merges)) +- if (!*rebase_merges) +- warning(_("--rebase-merges with an empty string " +- "argument is deprecated and will stop " +- "working in a future version of Git. Use " +- "--rebase-merges=no-rebase-cousins " +- "instead.")); +- else if (!strcmp("rebase-cousins", rebase_merges)) - options.rebase_cousins = 1; - else if (strcmp("no-rebase-cousins", rebase_merges)) - die(_("Unknown mode: %s"), rebase_merges); - options.rebase_merges = 1; -+ if (options.rebase_merges) - imply_merge(&options, "--rebase-merges"); +- imply_merge(&options, "--rebase-merges"); - } - +- if (options.type == REBASE_APPLY) { if (ignore_whitespace) + strvec_push(&options.git_am_opts, +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + break; + + if (i >= 0 || options.type == REBASE_APPLY) { +- if (is_merge(&options)) +- die(_("apply options and merge options " +- "cannot be used together")); +- else if (options.autosquash == -1 && options.config_autosquash == 1) ++ if (options.autosquash == -1 && options.config_autosquash == 1) + die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash")); ++ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1) ++ die(_("apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges")); + else if (options.update_refs == -1 && options.config_update_refs == 1) + die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs")); ++ else if (is_merge(&options)) ++ die(_("apply options and merge options " ++ "cannot be used together")); + else + options.type = REBASE_APPLY; + } +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + options.update_refs = (options.update_refs >= 0) ? options.update_refs : + ((options.config_update_refs >= 0) ? options.config_update_refs : 0); + ++ if (options.rebase_merges == 1) ++ imply_merge(&options, "--rebase-merges"); ++ options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges : ++ ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0); ++ + if (options.autosquash == 1) + imply_merge(&options, "--autosquash"); + options.autosquash = (options.autosquash >= 0) ? options.autosquash : + + ## t/t3422-rebase-incompatible-options.sh ## +@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () { + grep -e --no-autosquash err + " + ++ test_expect_success "$opt incompatible with rebase.merges" " ++ git checkout B^0 && ++ test_must_fail git -c rebase.merges=true rebase $opt A 2>err && ++ grep -e --no-rebase-merges err ++ " ++ + test_expect_success "$opt incompatible with rebase.updateRefs" " + git checkout B^0 && + test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err && +@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () { + git -c rebase.autosquash=true rebase --no-autosquash $opt A + " + ++ test_expect_success "$opt okay with overridden rebase.merges" " ++ test_when_finished \"git reset --hard B^0\" && ++ git checkout B^0 && ++ git -c rebase.merges=true rebase --no-rebase-merges $opt A ++ " ++ + test_expect_success "$opt okay with overridden rebase.updateRefs" " + test_when_finished \"git reset --hard B^0\" && + git checkout B^0 && ## t/t3430-rebase-merges.sh ## -@@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is invalid syntax' ' - test_cmp expect actual +@@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated' ' + grep deprecated actual ' -+test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' ' -+ test_config rebase.merges "" && -+ git checkout -b config-merges-blank E && -+ git rebase C && -+ test_cmp_graph C.. <<-\EOF -+ * B -+ * D -+ o C -+ EOF -+' -+ +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' + test_config rebase.merges rebase-cousins && + git checkout -b config-rebase-cousins main && @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is invalid syn + o H + EOF +' -+ -+test_expect_success 'local rebase.merges=false overrides global rebase.merges=true' ' -+ test_config_global rebase.merges true && -+ test_config rebase.merges false && -+ git checkout -b override-global-config E && -+ git rebase C && -+ test_cmp_graph C.. <<-\EOF -+ * B -+ * D -+ o C -+ EOF -+' -+ -+test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' ' -+ test_config_global rebase.merges no-rebase-cousins && -+ test_config rebase.merges "" && -+ git checkout -b no-override-global-config E && -+ before="$(git rev-parse --verify HEAD)" && -+ test_tick && -+ git rebase C && -+ test_cmp_rev HEAD $before -+' + test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt &&