| Message ID | 20230221055805.210951-3-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] rebase: document the --no-rebase-merges option | expand |
Hi Alex On 21/02/2023 05:58, Alex Henrie wrote: > 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 > confusion when a rebase.merges config option is introduced, where > rebase.merges="" will be equivalent to not passing --rebase-merges. I think this is sensible in the context of adding a config option. It is a backwards incompatible change though, lets hope no one was relying on it. Is there a particular reason you decided to redo the option parsing rather than just calling parse_merges_value() from the existing "if (rebase_merges)" block? I don't think it really matters, I'm just curious. Best Wishes Phillip > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > builtin/rebase.c | 42 +++++++++++++++++++++++++++------------- > t/t3430-rebase-merges.sh | 6 ++++++ > 2 files changed, 35 insertions(+), 13 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 6635f10d52..0a8366f30f 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -771,6 +771,20 @@ 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) > +{ > + 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; > +} > + > static int rebase_config(const char *var, const char *value, void *data) > { > struct rebase_options *opts = data; > @@ -980,6 +994,18 @@ 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) > +{ > + struct rebase_options *options = opt->value; > + > + if (unset) > + options->rebase_merges = 0; > + else > + parse_merges_value(options, arg); > + > + return 0; > +} > + > static void NORETURN error_on_missing_default_upstream(void) > { > struct branch *current_branch = branch_get(NULL); > @@ -1035,7 +1061,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > struct object_id branch_base; > 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; > @@ -1137,10 +1162,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > &options.allow_empty_message, > N_("allow rebasing commits with empty messages"), > PARSE_OPT_HIDDEN), > - {OPTION_STRING, 'r', "rebase-merges", &rebase_merges, > - N_("mode"), > + OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"), > N_("try to rebase merges instead of skipping them"), > - PARSE_OPT_OPTARG, NULL, (intptr_t)""}, > + PARSE_OPT_OPTARG, parse_opt_merges), > OPT_BOOL(0, "fork-point", &options.fork_point, > N_("use 'merge-base --fork-point' to refine upstream")), > OPT_STRING('s', "strategy", &options.strategy, > @@ -1436,16 +1460,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.exec.nr) > imply_merge(&options, "--exec"); > > - if (rebase_merges) { > - if (!*rebase_merges) > - ; /* default mode; do nothing */ > - 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"); > - } > > if (options.type == REBASE_APPLY) { > if (ignore_whitespace) > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index e0d910c229..b8ba323dbc 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -293,6 +293,12 @@ test_expect_success 'do not rebase cousins unless asked for' ' > EOF > ' > > +test_expect_success '--rebase-merges="" is invalid syntax' ' > + echo "fatal: Unknown mode: " >expect && > + ! git rebase --rebase-merges="" HEAD^ 2>actual && > + test_cmp expect actual > +' > + > test_expect_success 'refs/rewritten/* is worktree-local' ' > git worktree add wt && > cat >wt/script-from-scratch <<-\EOF &&
On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 21/02/2023 05:58, Alex Henrie wrote: > > 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 > > confusion when a rebase.merges config option is introduced, where > > rebase.merges="" will be equivalent to not passing --rebase-merges. > > I think this is sensible in the context of adding a config option. It is > a backwards incompatible change though, lets hope no one was relying on > it. Since the syntax is bizarre and undocumented, I doubt anyone is relying on it for anything serious, if anyone uses it at all. > Is there a particular reason you decided to redo the option parsing > rather than just calling parse_merges_value() from the existing "if > (rebase_merges)" block? I don't think it really matters, I'm just curious. Without a parse_opt_merges callback, how could we know whether the user passed --no-rebase-merges as opposed to passing nothing at all? const char *rebase_merges would be NULL in either case. It's an important distinction to make because --no-rebase-merges overrides rebase.merges but the absence of a command-line argument does not. All the same, your comment made me realize that it would probably make more sense to simply change the default value of --rebase-cousins from "" to "no-rebase-cousins" in this patch and then add the parse_opt_merges callback in the next patch when it is actually needed. -Alex
On Tue, Feb 21, 2023 at 6:38 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > All the same, your comment made me realize that it would probably make > more sense to simply change the default value of --rebase-cousins from > "" to "no-rebase-cousins" in this patch I meant --rebase-merges, sorry. --rebase-cousins obviously isn't a thing. -Alex
Hi Alex On 22/02/2023 01:38, Alex Henrie wrote: > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> On 21/02/2023 05:58, Alex Henrie wrote: >>> 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 >>> confusion when a rebase.merges config option is introduced, where >>> rebase.merges="" will be equivalent to not passing --rebase-merges. >> >> I think this is sensible in the context of adding a config option. It is >> a backwards incompatible change though, lets hope no one was relying on >> it. > > Since the syntax is bizarre and undocumented, I doubt anyone is > relying on it for anything serious, if anyone uses it at all. > >> Is there a particular reason you decided to redo the option parsing >> rather than just calling parse_merges_value() from the existing "if >> (rebase_merges)" block? I don't think it really matters, I'm just curious. > > Without a parse_opt_merges callback, how could we know whether the > user passed --no-rebase-merges as opposed to passing nothing at all? > const char *rebase_merges would be NULL in either case. It's an > important distinction to make because --no-rebase-merges overrides > rebase.merges but the absence of a command-line argument does not. The usual way we handle that is to set the value of rebase_merges from the config before calling parse_options(). However your solution is fine. > All the same, your comment made me realize that it would probably make > more sense to simply change the default value of --rebase-cousins from > "" to "no-rebase-cousins" in this patch and then add the > parse_opt_merges callback in the next patch when it is actually > needed. Sounds good Phillip > -Alex
Phillip Wood <phillip.wood123@gmail.com> writes: > The usual way we handle that is to set the value of rebase_merges from > the config before calling parse_options(). However your solution is > fine. Is it? If it is not too much to ask, I'd prefer to have code that does not surprise people, and "the usual way" you mentioned is what readers around here expect to see. I didn't check and think about the patch in quetion, and especially the existing code that the patch needs to touch, too deeply, so if it is too convoluted already that it would be a lot of work to make it work in "the usual way", it may be OK, but otherwise, the solution may not be fine. Thanks.
On Wed, Feb 22, 2023 at 3:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 22/02/2023 01:38, Alex Henrie wrote: > > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> > >> Is there a particular reason you decided to redo the option parsing > >> rather than just calling parse_merges_value() from the existing "if > >> (rebase_merges)" block? I don't think it really matters, I'm just curious. > > > > Without a parse_opt_merges callback, how could we know whether the > > user passed --no-rebase-merges as opposed to passing nothing at all? > > const char *rebase_merges would be NULL in either case. It's an > > important distinction to make because --no-rebase-merges overrides > > rebase.merges but the absence of a command-line argument does not. > > The usual way we handle that is to set the value of rebase_merges from > the config before calling parse_options(). However your solution is fine. On Wed, Feb 22, 2023 at 4:08 PM Junio C Hamano <gitster@pobox.com> wrote: > > Is it? If it is not too much to ask, I'd prefer to have code that > does not surprise people, and "the usual way" you mentioned is what > readers around here expect to see. I didn't check and think about > the patch in quetion, and especially the existing code that the > patch needs to touch, too deeply, so if it is too convoluted already > that it would be a lot of work to make it work in "the usual way", > it may be OK, but otherwise, the solution may not be fine. There was a const char *rebase_merges in cmd_rebase and an int rebase_merges in struct rebase_options. I deleted const char *rebase_merges, leaving only int rebase_merges. int rebase_merges is set from rebase_config before it is set from parse_options. Do you want me to add back const char *rebase_merges? If so, where should it be declared so that it can be set from both rebase_config and parse_options? -Alex
diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..0a8366f30f 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -771,6 +771,20 @@ 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) +{ + 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; +} + static int rebase_config(const char *var, const char *value, void *data) { struct rebase_options *opts = data; @@ -980,6 +994,18 @@ 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) +{ + struct rebase_options *options = opt->value; + + if (unset) + options->rebase_merges = 0; + else + parse_merges_value(options, arg); + + return 0; +} + static void NORETURN error_on_missing_default_upstream(void) { struct branch *current_branch = branch_get(NULL); @@ -1035,7 +1061,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) struct object_id branch_base; 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; @@ -1137,10 +1162,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) &options.allow_empty_message, N_("allow rebasing commits with empty messages"), PARSE_OPT_HIDDEN), - {OPTION_STRING, 'r', "rebase-merges", &rebase_merges, - N_("mode"), + OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"), N_("try to rebase merges instead of skipping them"), - PARSE_OPT_OPTARG, NULL, (intptr_t)""}, + PARSE_OPT_OPTARG, parse_opt_merges), OPT_BOOL(0, "fork-point", &options.fork_point, N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, @@ -1436,16 +1460,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.exec.nr) imply_merge(&options, "--exec"); - if (rebase_merges) { - if (!*rebase_merges) - ; /* default mode; do nothing */ - 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"); - } if (options.type == REBASE_APPLY) { if (ignore_whitespace) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index e0d910c229..b8ba323dbc 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -293,6 +293,12 @@ test_expect_success 'do not rebase cousins unless asked for' ' EOF ' +test_expect_success '--rebase-merges="" is invalid syntax' ' + echo "fatal: Unknown mode: " >expect && + ! git rebase --rebase-merges="" HEAD^ 2>actual && + test_cmp expect actual +' + test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt && cat >wt/script-from-scratch <<-\EOF &&
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 confusion when a rebase.merges config option is introduced, where rebase.merges="" will be equivalent to not passing --rebase-merges. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- builtin/rebase.c | 42 +++++++++++++++++++++++++++------------- t/t3430-rebase-merges.sh | 6 ++++++ 2 files changed, 35 insertions(+), 13 deletions(-)