| Message ID | 20250210191650.316329-1-intelfx@intelfx.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rebase: add `--update-refs=interactive` | expand |
On Mon, Feb 10, 2025 at 2:17 PM Ivan Shapovalov <intelfx@intelfx.name> wrote: > > In rebase-heavy workflows involving multiple interdependent feature > branches, typing out `--update-refs` quickly becomes tiring, which > can be mitigated with setting the `rebase.updateRefs` git-config option > to perform update-refs by default. > > However, the utility of `rebase.updateRefs` is somewhat limited because > you rarely want it in a non-interactive rebase (as it does not give you > the chance to review the update-refs candidates, likely leading to > updating refs that you didn't want updated -- I made quite an amount > of mess by setting this option and subsequently forgetting about it). > > Try to find a middle ground by introducing a third value, > `--update-refs=interactive` (and `rebase.updateRefs=interactive`) > which means `--update-refs` when starting an interactive rebase and > `--no-update-refs` otherwise. This option is primarily intended to be > used in the gitconfig, but is also accepted on the command line > for completeness. > > Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name> > --- > Documentation/config/rebase.txt | 7 +++- > Documentation/git-rebase.txt | 8 +++- > builtin/rebase.c | 72 +++++++++++++++++++++++++++++---- > 3 files changed, 77 insertions(+), 10 deletions(-) > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > index c6187ab28b..d8bbaba69a 100644 > --- a/Documentation/config/rebase.txt > +++ b/Documentation/config/rebase.txt > @@ -24,7 +24,12 @@ rebase.autoStash:: > Defaults to false. > > rebase.updateRefs:: > - If set to true enable `--update-refs` option by default. > + If set to true, enable the `--update-refs` option of > + linkgit:git-rebase[1] by default. When set to 'interactive', > + only enable `--update-refs` by default for interactive mode > + (equivalent to `--update-refs=interactive`). > + This option can be overridden by specifying any form of > + `--update-refs` on the command line. > > rebase.missingCommitsCheck:: > If set to "warn", git rebase -i will print a warning if some > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index b18cdbc023..ae6939588d 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -647,12 +647,18 @@ rebase --continue` is invoked. Currently, you cannot pass > > --update-refs:: > --no-update-refs:: > +--update-refs=interactive:: Based on `git grep -e '--.*\[=' Documentation/git-*.txt`, I think this should be more like --update-refs[=interactive]:: --no-update-refs:: But maybe that unintentionally suggests that `=interactive` is the default? > Automatically force-update any branches that point to commits that > are being rebased. Any branches that are checked out in a worktree > are not updated in this way. > + > +If `--update-refs=interactive` is specified, the behavior is equivalent to > +`--update-refs` if the rebase is interactive and `--no-update-refs` otherwise. > +(This is mainly useful as a configuration setting, although it might also be > +of use in aliases.) > ++ > If the configuration variable `rebase.updateRefs` is set, then this option > -can be used to override and disable this setting. > +can be used to override or disable the configuration. > + > See also INCOMPATIBLE OPTIONS below. > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 6c9eaf3788..57b456599b 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -129,10 +129,17 @@ struct rebase_options { > int reschedule_failed_exec; > int reapply_cherry_picks; > int fork_point; > - int update_refs; > + // UPDATE_REFS_{UNKNOWN,NO,ALWAYS} numeric values must never > + // change as post-option-parsing code works with {,config_}update_refs > + // as if they were ints > + enum { > + UPDATE_REFS_UNKNOWN = -1, > + UPDATE_REFS_NO = 0, > + UPDATE_REFS_ALWAYS = 1, > + UPDATE_REFS_INTERACTIVE, > + } update_refs, config_update_refs; > int config_autosquash; > int config_rebase_merges; > - int config_update_refs; > }; > > #define REBASE_OPTIONS_INIT { \ > @@ -150,8 +157,8 @@ struct rebase_options { > .autosquash = -1, \ > .rebase_merges = -1, \ > .config_rebase_merges = -1, \ > - .update_refs = -1, \ > - .config_update_refs = -1, \ > + .update_refs = UPDATE_REFS_UNKNOWN, \ > + .config_update_refs = UPDATE_REFS_UNKNOWN, \ > .strategy_opts = STRING_LIST_INIT_NODUP,\ > } > > @@ -412,6 +419,18 @@ static void imply_merge(struct rebase_options *opts, const char *option) > } > } > > +static int coerce_update_refs(const struct rebase_options *opts, int update_refs) > +{ > + /* coerce "=interactive" into "no" rather than "not set" when not interactive > + * this way, `git -c rebase.updateRefs=yes rebase --update-refs=interactive [without -i]` > + * will not inherit the "yes" from the config */ > + if (update_refs == UPDATE_REFS_INTERACTIVE) > + return (opts->flags & REBASE_INTERACTIVE_EXPLICIT) > + ? UPDATE_REFS_ALWAYS > + : UPDATE_REFS_NO; > + return update_refs; > +} > + > /* Returns the filename prefixed by the state_dir */ > static const char *state_dir_path(const char *filename, struct rebase_options *opts) > { > @@ -779,6 +798,17 @@ static void parse_rebase_merges_value(struct rebase_options *options, const char > die(_("Unknown rebase-merges mode: %s"), value); > } > > +static int parse_update_refs_value(const char *value, const char *desc) > +{ > + int v = git_parse_maybe_bool(value); > + if (v >= 0) > + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; > + else if (!strcmp("interactive", value)) > + return UPDATE_REFS_INTERACTIVE; > + > + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); > +} > + > static int rebase_config(const char *var, const char *value, > const struct config_context *ctx, void *data) > { > @@ -821,7 +851,8 @@ static int rebase_config(const char *var, const char *value, > } > > if (!strcmp(var, "rebase.updaterefs")) { > - opts->config_update_refs = git_config_bool(var, value); > + opts->config_update_refs = parse_update_refs_value(value, > + "rebase.updateRefs"); > return 0; > } > > @@ -1042,6 +1073,19 @@ static int parse_opt_rebase_merges(const struct option *opt, const char *arg, in > return 0; > } > > +static int parse_opt_update_refs(const struct option *opt, const char *arg, int unset) > +{ > + struct rebase_options *options = opt->value; > + > + if (arg) > + options->update_refs = parse_update_refs_value(arg, > + "--update-refs"); > + else > + options->update_refs = unset ? UPDATE_REFS_NO : UPDATE_REFS_ALWAYS; > + > + return 0; > +} > + > static void NORETURN error_on_missing_default_upstream(void) > { > struct branch *current_branch = branch_get(NULL); > @@ -1187,9 +1231,11 @@ int cmd_rebase(int argc, > OPT_BOOL(0, "autosquash", &options.autosquash, > N_("move commits that begin with " > "squash!/fixup! under -i")), > - OPT_BOOL(0, "update-refs", &options.update_refs, > - N_("update branches that point to commits " > - "that are being rebased")), > + OPT_CALLBACK_F(0, "update-refs", &options, > + N_("(bool|interactive)"), > + N_("update branches that point to commits " > + "that are being rebased"), > + PARSE_OPT_OPTARG, parse_opt_update_refs), > { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"), > N_("GPG-sign commits"), > PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > @@ -1528,6 +1574,16 @@ int cmd_rebase(int argc, > if (isatty(2) && options.flags & REBASE_NO_QUIET) > strbuf_addstr(&options.git_format_patch_opt, " --progress"); > > + /* coerce --update-refs=interactive into yes or no. > + * we do it here because there's just too much code below that handles > + * {,config_}update_refs in one way or another and modifying it to > + * account for the new state would be too invasive. > + * all further code uses {,config_}update_refs as a tristate. */ > + options.update_refs = > + coerce_update_refs(&options, options.update_refs); > + options.config_update_refs = > + coerce_update_refs(&options, options.config_update_refs); > + > if (options.git_am_opts.nr || options.type == REBASE_APPLY) { > /* all am options except -q are compatible only with --apply */ > for (i = options.git_am_opts.nr - 1; i >= 0; i--) > -- > 2.48.1.5.g9188e14f140 > > Should we add a test for this?
On 2025-02-10 at 15:22 -0500, D. Ben Knoble wrote: > On Mon, Feb 10, 2025 at 2:17 PM Ivan Shapovalov <intelfx@intelfx.name> wrote: > > > > In rebase-heavy workflows involving multiple interdependent feature > > branches, typing out `--update-refs` quickly becomes tiring, which > > can be mitigated with setting the `rebase.updateRefs` git-config option > > to perform update-refs by default. > > > > However, the utility of `rebase.updateRefs` is somewhat limited because > > you rarely want it in a non-interactive rebase (as it does not give you > > the chance to review the update-refs candidates, likely leading to > > updating refs that you didn't want updated -- I made quite an amount > > of mess by setting this option and subsequently forgetting about it). > > > > Try to find a middle ground by introducing a third value, > > `--update-refs=interactive` (and `rebase.updateRefs=interactive`) > > which means `--update-refs` when starting an interactive rebase and > > `--no-update-refs` otherwise. This option is primarily intended to be > > used in the gitconfig, but is also accepted on the command line > > for completeness. > > > > Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name> > > --- > > Documentation/config/rebase.txt | 7 +++- > > Documentation/git-rebase.txt | 8 +++- > > builtin/rebase.c | 72 +++++++++++++++++++++++++++++---- > > 3 files changed, 77 insertions(+), 10 deletions(-) > > > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > > index c6187ab28b..d8bbaba69a 100644 > > --- a/Documentation/config/rebase.txt > > +++ b/Documentation/config/rebase.txt > > @@ -24,7 +24,12 @@ rebase.autoStash:: > > Defaults to false. > > > > rebase.updateRefs:: > > - If set to true enable `--update-refs` option by default. > > + If set to true, enable the `--update-refs` option of > > + linkgit:git-rebase[1] by default. When set to 'interactive', > > + only enable `--update-refs` by default for interactive mode > > + (equivalent to `--update-refs=interactive`). > > + This option can be overridden by specifying any form of > > + `--update-refs` on the command line. > > > > rebase.missingCommitsCheck:: > > If set to "warn", git rebase -i will print a warning if some > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > > index b18cdbc023..ae6939588d 100644 > > --- a/Documentation/git-rebase.txt > > +++ b/Documentation/git-rebase.txt > > @@ -647,12 +647,18 @@ rebase --continue` is invoked. Currently, you cannot pass > > > > --update-refs:: > > --no-update-refs:: > > +--update-refs=interactive:: > > Based on `git grep -e '--.*\[=' Documentation/git-*.txt`, I think this > should be more like > > --update-refs[=interactive]:: > --no-update-refs:: > > But maybe that unintentionally suggests that `=interactive` is the default? Perhaps --update-refs[=(yes|no|interactive)] then? Or is that too verbose? Anyway, I don't have a preference, I'll just do what I'm told here. > > > Automatically force-update any branches that point to commits that > > are being rebased. Any branches that are checked out in a worktree > > are not updated in this way. > > + > > +If `--update-refs=interactive` is specified, the behavior is equivalent to > > +`--update-refs` if the rebase is interactive and `--no-update-refs` otherwise. > > +(This is mainly useful as a configuration setting, although it might also be > > +of use in aliases.) > > ++ > > If the configuration variable `rebase.updateRefs` is set, then this option > > -can be used to override and disable this setting. > > +can be used to override or disable the configuration. > > + > > See also INCOMPATIBLE OPTIONS below. > > > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > index 6c9eaf3788..57b456599b 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -129,10 +129,17 @@ struct rebase_options { > > int reschedule_failed_exec; > > int reapply_cherry_picks; > > int fork_point; > > - int update_refs; > > + // UPDATE_REFS_{UNKNOWN,NO,ALWAYS} numeric values must never > > + // change as post-option-parsing code works with {,config_}update_refs > > + // as if they were ints > > + enum { > > + UPDATE_REFS_UNKNOWN = -1, > > + UPDATE_REFS_NO = 0, > > + UPDATE_REFS_ALWAYS = 1, > > + UPDATE_REFS_INTERACTIVE, > > + } update_refs, config_update_refs; > > int config_autosquash; > > int config_rebase_merges; > > - int config_update_refs; > > }; > > > > #define REBASE_OPTIONS_INIT { \ > > @@ -150,8 +157,8 @@ struct rebase_options { > > .autosquash = -1, \ > > .rebase_merges = -1, \ > > .config_rebase_merges = -1, \ > > - .update_refs = -1, \ > > - .config_update_refs = -1, \ > > + .update_refs = UPDATE_REFS_UNKNOWN, \ > > + .config_update_refs = UPDATE_REFS_UNKNOWN, \ > > .strategy_opts = STRING_LIST_INIT_NODUP,\ > > } > > > > @@ -412,6 +419,18 @@ static void imply_merge(struct rebase_options *opts, const char *option) > > } > > } > > > > +static int coerce_update_refs(const struct rebase_options *opts, int update_refs) > > +{ > > + /* coerce "=interactive" into "no" rather than "not set" when not interactive > > + * this way, `git -c rebase.updateRefs=yes rebase --update-refs=interactive [without -i]` > > + * will not inherit the "yes" from the config */ > > + if (update_refs == UPDATE_REFS_INTERACTIVE) > > + return (opts->flags & REBASE_INTERACTIVE_EXPLICIT) > > + ? UPDATE_REFS_ALWAYS > > + : UPDATE_REFS_NO; > > + return update_refs; > > +} > > + > > /* Returns the filename prefixed by the state_dir */ > > static const char *state_dir_path(const char *filename, struct rebase_options *opts) > > { > > @@ -779,6 +798,17 @@ static void parse_rebase_merges_value(struct rebase_options *options, const char > > die(_("Unknown rebase-merges mode: %s"), value); > > } > > > > +static int parse_update_refs_value(const char *value, const char *desc) > > +{ > > + int v = git_parse_maybe_bool(value); > > + if (v >= 0) > > + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; > > + else if (!strcmp("interactive", value)) > > + return UPDATE_REFS_INTERACTIVE; > > + > > + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); > > +} > > + > > static int rebase_config(const char *var, const char *value, > > const struct config_context *ctx, void *data) > > { > > @@ -821,7 +851,8 @@ static int rebase_config(const char *var, const char *value, > > } > > > > if (!strcmp(var, "rebase.updaterefs")) { > > - opts->config_update_refs = git_config_bool(var, value); > > + opts->config_update_refs = parse_update_refs_value(value, > > + "rebase.updateRefs"); > > return 0; > > } > > > > @@ -1042,6 +1073,19 @@ static int parse_opt_rebase_merges(const struct option *opt, const char *arg, in > > return 0; > > } > > > > +static int parse_opt_update_refs(const struct option *opt, const char *arg, int unset) > > +{ > > + struct rebase_options *options = opt->value; > > + > > + if (arg) > > + options->update_refs = parse_update_refs_value(arg, > > + "--update-refs"); > > + else > > + options->update_refs = unset ? UPDATE_REFS_NO : UPDATE_REFS_ALWAYS; > > + > > + return 0; > > +} > > + > > static void NORETURN error_on_missing_default_upstream(void) > > { > > struct branch *current_branch = branch_get(NULL); > > @@ -1187,9 +1231,11 @@ int cmd_rebase(int argc, > > OPT_BOOL(0, "autosquash", &options.autosquash, > > N_("move commits that begin with " > > "squash!/fixup! under -i")), > > - OPT_BOOL(0, "update-refs", &options.update_refs, > > - N_("update branches that point to commits " > > - "that are being rebased")), > > + OPT_CALLBACK_F(0, "update-refs", &options, > > + N_("(bool|interactive)"), > > + N_("update branches that point to commits " > > + "that are being rebased"), > > + PARSE_OPT_OPTARG, parse_opt_update_refs), > > { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"), > > N_("GPG-sign commits"), > > PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > > @@ -1528,6 +1574,16 @@ int cmd_rebase(int argc, > > if (isatty(2) && options.flags & REBASE_NO_QUIET) > > strbuf_addstr(&options.git_format_patch_opt, " --progress"); > > > > + /* coerce --update-refs=interactive into yes or no. > > + * we do it here because there's just too much code below that handles > > + * {,config_}update_refs in one way or another and modifying it to > > + * account for the new state would be too invasive. > > + * all further code uses {,config_}update_refs as a tristate. */ > > + options.update_refs = > > + coerce_update_refs(&options, options.update_refs); > > + options.config_update_refs = > > + coerce_update_refs(&options, options.config_update_refs); > > + > > if (options.git_am_opts.nr || options.type == REBASE_APPLY) { > > /* all am options except -q are compatible only with --apply */ > > for (i = options.git_am_opts.nr - 1; i >= 0; i--) > > -- > > 2.48.1.5.g9188e14f140 > > > > > > Should we add a test for this? > Any suggestions what exactly I should test here? I don't have much experience testing interactive CLI tools, so I'd appreciate some hints.
Hi Ivan On 10/02/2025 19:16, Ivan Shapovalov wrote: > In rebase-heavy workflows involving multiple interdependent feature > branches, typing out `--update-refs` quickly becomes tiring, which > can be mitigated with setting the `rebase.updateRefs` git-config option > to perform update-refs by default. > > However, the utility of `rebase.updateRefs` is somewhat limited because > you rarely want it in a non-interactive rebase (as it does not give you > the chance to review the update-refs candidates, likely leading to > updating refs that you didn't want updated -- I made quite an amount > of mess by setting this option and subsequently forgetting about it). I'm a bit surprised by this - I'd have thought there is more scope for messing things up by making a mistake when editing the todo list that for the non-interactive case. Are you able to explain a in a bit more detail the problem you have been experiencing please? > Try to find a middle ground by introducing a third value, > `--update-refs=interactive` (and `rebase.updateRefs=interactive`) > which means `--update-refs` when starting an interactive rebase and > `--no-update-refs` otherwise. This option is primarily intended to be > used in the gitconfig, but is also accepted on the command line > for completeness. I'm not convinced allowing "--update-refs=interactive" on the commandline improves the usability - why wouldn't I just say "--update-refs" if I want to update all the branches or "--no-update-refs" if I don't? I also think supporting --update-refs=(true|false) is verbose and unnecessary as the user can already specify their intent with the existing option. > rebase.updateRefs:: > - If set to true enable `--update-refs` option by default. > + If set to true, enable the `--update-refs` option of > + linkgit:git-rebase[1] by default. When set to 'interactive', Our existing documentation is inconsistent in how it formats config values. rebase.backend uses "apply", rebase.rebaseMerges uses `rebase-cousins` which I think matches other commands and is therefore what we should use here and rebase.missingCommitCheck uses a mixture with "warn" and `drop`. > + only enable `--update-refs` by default for interactive mode > + (equivalent to `--update-refs=interactive`). > + This option can be overridden by specifying any form of > + `--update-refs` on the command line. > @@ -129,10 +129,17 @@ struct rebase_options { > int reschedule_failed_exec; > int reapply_cherry_picks; > int fork_point; > - int update_refs; > + // UPDATE_REFS_{UNKNOWN,NO,ALWAYS} numeric values must never > + // change as post-option-parsing code works with {,config_}update_refs > + // as if they were ints This feels a bit fragile - why can't we update the code to use the enum? Also note that comments should be formatted as /* single line comment */ or /* * multi-line * comment */ > + enum { > + UPDATE_REFS_UNKNOWN = -1, > + UPDATE_REFS_NO = 0, > + UPDATE_REFS_ALWAYS = 1, > + UPDATE_REFS_INTERACTIVE, > + } update_refs, config_update_refs; I don't think we want to change the type of `update_refs` as I'm not convinced we want to change the commandline option. > +static int coerce_update_refs(const struct rebase_options *opts, int update_refs) I'd be tempted to call this "should_update_refs(...)" > +{ > + /* coerce "=interactive" into "no" rather than "not set" when not interactive > + * this way, `git -c rebase.updateRefs=yes rebase --update-refs=interactive [without -i]` > + * will not inherit the "yes" from the config */ Style - see above > + if (update_refs == UPDATE_REFS_INTERACTIVE) > + return (opts->flags & REBASE_INTERACTIVE_EXPLICIT) > + ? UPDATE_REFS_ALWAYS > + : UPDATE_REFS_NO; > + return update_refs; > +} > [...] > +static int parse_update_refs_value(const char *value, const char *desc) > +{ > + int v = git_parse_maybe_bool(value); Style: there should be a blank line after the variable declarations at the start of a function. > + if (v >= 0) > + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; > + else if (!strcmp("interactive", value)) > + return UPDATE_REFS_INTERACTIVE; > + > + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); I think we normally say "invalid" or "unknown" rather than "bad" in our error messages. It'd be clearer just to list the possible values as there are only three of them. > + /* coerce --update-refs=interactive into yes or no. > + * we do it here because there's just too much code below that handles > + * {,config_}update_refs in one way or another and modifying it to > + * account for the new state would be too invasive. > + * all further code uses {,config_}update_refs as a tristate. */ I think we need to find a cleaner way of handling this. There are only two mentions of options.config_update_refs below this point - is it really so difficult for those to use the enum? Given a bit more detail I could be convinced that the config option is useful but I don't think we should be changing the commandline option. Best Wishes Phillip
Ivan Shapovalov <intelfx@intelfx.name> writes: >> > --update-refs:: >> > --no-update-refs:: >> > +--update-refs=interactive:: >> >> Based on `git grep -e '--.*\[=' Documentation/git-*.txt`, I think this >> should be more like >> >> --update-refs[=interactive]:: >> --no-update-refs:: >> >> But maybe that unintentionally suggests that `=interactive` is the default? > > Perhaps --update-refs[=(yes|no|interactive)] then? Or is that too > verbose? If `--update-refs` does take values that the git_parse_maybe_bool() helper parses as a Boolean value, I do not think the above is verbose at all. Rather, it is a disservice to the users if the documentation does not mention yes/no in such a case. I'd say listing other Boolean synonyms like yes/true/on/no/false/off is too verbose, though ;-). > Anyway, I don't have a preference, I'll just do what I'm told That is not quite in line with how we'd like to operate. It is your itch. Others may give suggestions to help you polish it, but ultimately, we would not want to accept a patch that the author does not agree with. Thanks.
On 2025-02-11 at 08:50 -0800, Junio C Hamano wrote: > Ivan Shapovalov <intelfx@intelfx.name> writes: > > > > > --update-refs:: > > > > --no-update-refs:: > > > > +--update-refs=interactive:: > > > > > > Based on `git grep -e '--.*\[=' Documentation/git-*.txt`, I think this > > > should be more like > > > > > > --update-refs[=interactive]:: > > > --no-update-refs:: > > > > > > But maybe that unintentionally suggests that `=interactive` is the default? > > > > Perhaps --update-refs[=(yes|no|interactive)] then? Or is that too > > verbose? > > If `--update-refs` does take values that the git_parse_maybe_bool() > helper parses as a Boolean value, I do not think the above is > verbose at all. Rather, it is a disservice to the users if the > documentation does not mention yes/no in such a case. I'd say > listing other Boolean synonyms like yes/true/on/no/false/off is > too verbose, though ;-). > > > Anyway, I don't have a preference, I'll just do what I'm told > > That is not quite in line with how we'd like to operate. > > It is your itch. Others may give suggestions to help you polish it, > but ultimately, we would not want to accept a patch that the author > does not agree with. Of course, I care about the patch and the feature; what I wanted to say is that I do not care (comparatively) about the formatting of the help text: I couldn't figure it out on my own, so whatever you tell me is the proper way of formatting it, I'll do.
On 2025-02-11 at 14:36 +0000, Phillip Wood wrote: > Hi Ivan > > On 10/02/2025 19:16, Ivan Shapovalov wrote: > > In rebase-heavy workflows involving multiple interdependent feature > > branches, typing out `--update-refs` quickly becomes tiring, which > > can be mitigated with setting the `rebase.updateRefs` git-config option > > to perform update-refs by default. > > > > However, the utility of `rebase.updateRefs` is somewhat limited because > > you rarely want it in a non-interactive rebase (as it does not give you > > the chance to review the update-refs candidates, likely leading to > > updating refs that you didn't want updated -- I made quite an amount > > of mess by setting this option and subsequently forgetting about it). > > I'm a bit surprised by this - I'd have thought there is more scope for > messing things up by making a mistake when editing the todo list that > for the non-interactive case. Are you able to explain a in a bit more > detail the problem you have been experiencing please? I often find myself managing multiple interdependent downstream patch branches, rebasing them en masse from release to release. Eventually, I found myself typing `git rebase -i --update-refs` more often than not, so I just stuck it into the config as `rebase.updateRefs=true`. However, sometimes I also maintain those patch branches for multiple releases. Consider a (hypothetical) situation: - tag v1 - tag v2 - branch work/myfeature-v1 that is based on tag v1 Now, I want to rebase myfeature onto v2, so I do this: $ git checkout work/myfeature-v1 $ git checkout -b work/myfeature-v2 $ git rebase --onto v2 v1 work/myfeature-v2 With `rebase.updateRefs=true`, this ends up silently updating _both_ work/myfeature-v2 and work/myfeature-v1. With this in mind, I wrote this patch such that update-refs only happens for interactive rebases, when I have the chance to inspect the todo list and prune unwanted update-refs items. Does this make sense? I made an attempt to explain this motivation in the commit message, so if this does make sense but the commit message doesn't, please tell me how to improve/expand the latter. > > > Try to find a middle ground by introducing a third value, > > `--update-refs=interactive` (and `rebase.updateRefs=interactive`) > > which means `--update-refs` when starting an interactive rebase and > > `--no-update-refs` otherwise. This option is primarily intended to be > > used in the gitconfig, but is also accepted on the command line > > for completeness. > > I'm not convinced allowing "--update-refs=interactive" on the > commandline improves the usability - why wouldn't I just say > "--update-refs" if I want to update all the branches or > "--no-update-refs" if I don't? I also think supporting > --update-refs=(true|false) is verbose and unnecessary as the user can > already specify their intent with the existing option. I make heavy use of aliases for various workflows, which invoke one another (making use of the ability to override earlier command-line options with the latter ones), and the ability to spell out `alias.myRebase = rebase ... --update-refs=interactive ...` was useful. Re: specifying `=(true|false)`, the intention was to avoid unnecessary divergence, both in UX and code (and reuse the parser to simplify said code). If you think it will be harmful, I'll remove that. > > > rebase.updateRefs:: > > - If set to true enable `--update-refs` option by default. > > + If set to true, enable the `--update-refs` option of > > + linkgit:git-rebase[1] by default. When set to 'interactive', > > Our existing documentation is inconsistent in how it formats config > values. rebase.backend uses "apply", rebase.rebaseMerges uses > `rebase-cousins` which I think matches other commands and is therefore > what we should use here and rebase.missingCommitCheck uses a mixture > with "warn" and `drop`. Apologies, I'm not sure I understood what exactly you were suggesting here. Did you mean to suggest wrapping "interactive" in backticks instead of single quotes? > > > + only enable `--update-refs` by default for interactive mode > > + (equivalent to `--update-refs=interactive`). > > + This option can be overridden by specifying any form of > > + `--update-refs` on the command line. > > > @@ -129,10 +129,17 @@ struct rebase_options { > > int reschedule_failed_exec; > > int reapply_cherry_picks; > > int fork_point; > > - int update_refs; > > + // UPDATE_REFS_{UNKNOWN,NO,ALWAYS} numeric values must never > > + // change as post-option-parsing code works with {,config_}update_refs > > + // as if they were ints > > This feels a bit fragile - why can't we update the code to use the enum? It'd just be a lot of code to update, incl. stylistically (especially the implicit truthy/falsy coercions or `>= 0`). I opted to make the change as non-invasive as possible. > Also note that comments should be formatted as > > /* single line comment */ > > or > > /* > * multi-line > * comment > */ OK > > > + enum { > > + UPDATE_REFS_UNKNOWN = -1, > > + UPDATE_REFS_NO = 0, > > + UPDATE_REFS_ALWAYS = 1, > > + UPDATE_REFS_INTERACTIVE, > > + } update_refs, config_update_refs; > > I don't think we want to change the type of `update_refs` as I'm not > convinced we want to change the commandline option. > > > +static int coerce_update_refs(const struct rebase_options *opts, int update_refs) > > I'd be tempted to call this "should_update_refs(...)" OK > > > +{ > > + /* coerce "=interactive" into "no" rather than "not set" when not interactive > > + * this way, `git -c rebase.updateRefs=yes rebase --update-refs=interactive [without -i]` > > + * will not inherit the "yes" from the config */ > > Style - see above OK > > > + if (update_refs == UPDATE_REFS_INTERACTIVE) > > + return (opts->flags & REBASE_INTERACTIVE_EXPLICIT) > > + ? UPDATE_REFS_ALWAYS > > + : UPDATE_REFS_NO; > > + return update_refs; > > +} > > [...] > > +static int parse_update_refs_value(const char *value, const char *desc) > > +{ > > + int v = git_parse_maybe_bool(value); > > Style: there should be a blank line after the variable declarations at > the start of a function. OK > > > + if (v >= 0) > > + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; > > + else if (!strcmp("interactive", value)) > > + return UPDATE_REFS_INTERACTIVE; > > + > > + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); > > I think we normally say "invalid" or "unknown" rather than "bad" in our > error messages. It'd be clearer just to list the possible values as > there are only three of them. It's not just three (see other review from Junio), otherwise OK > > > + /* coerce --update-refs=interactive into yes or no. > > + * we do it here because there's just too much code below that handles > > + * {,config_}update_refs in one way or another and modifying it to > > + * account for the new state would be too invasive. > > + * all further code uses {,config_}update_refs as a tristate. */ > > I think we need to find a cleaner way of handling this. There are only > two mentions of options.config_update_refs below this point - is it > really so difficult for those to use the enum? See above; I opted to make this change as non-invasive as possible and keep the complex argument validation logic (lines 1599, 1606-1609) intact because I'm not even sure I understand it right. Besides, even if I convert those uses to use enumerators, I still wouldn't want to deal with non-tristate values beyond this point.
On Tue, Feb 11, 2025 at 6:33 AM Ivan Shapovalov <intelfx@intelfx.name> wrote: > > On 2025-02-10 at 15:22 -0500, D. Ben Knoble wrote: > > > > Based on `git grep -e '--.*\[=' Documentation/git-*.txt`, I think this > > should be more like > > > > --update-refs[=interactive]:: > > --no-update-refs:: > > > > But maybe that unintentionally suggests that `=interactive` is the default? > > Perhaps --update-refs[=(yes|no|interactive)] then? Or is that too > verbose? Anyway, I don't have a preference, I'll just do what I'm told > here. I don't have a strong opinion, and I think this is being discussed elsewhere in this thread. > > Should we add a test for this? > > > > Any suggestions what exactly I should test here? I don't have much > experience testing interactive CLI tools, so I'd appreciate some hints. > > -- > Ivan Shapovalov / intelfx / Give t/README a glance; t3404 is probably a good place to start given "git grep update-refs t."
On Tue, Feb 11, 2025 at 2:28 PM D. Ben Knoble <ben.knoble@gmail.com> wrote: > > On Tue, Feb 11, 2025 at 6:33 AM Ivan Shapovalov <intelfx@intelfx.name> wrote: > > > > On 2025-02-10 at 15:22 -0500, D. Ben Knoble wrote: > > > Should we add a test for this? > > > > > > > Any suggestions what exactly I should test here? I don't have much > > experience testing interactive CLI tools, so I'd appreciate some hints. > > > > -- > > Ivan Shapovalov / intelfx / > > Give t/README a glance; t3404 is probably a good place to start given > "git grep update-refs t." But I really meant to write—let's hash out the other details before worrying about a test ;)
Hi Ivan On 11/02/2025 18:11, Ivan Shapovalov wrote: > On 2025-02-11 at 14:36 +0000, Phillip Wood wrote: >> On 10/02/2025 19:16, Ivan Shapovalov wrote: >> >> I'm a bit surprised by this - I'd have thought there is more scope for >> messing things up by making a mistake when editing the todo list that >> for the non-interactive case. Are you able to explain a in a bit more >> detail the problem you have been experiencing please? > > I often find myself managing multiple interdependent downstream patch > branches, rebasing them en masse from release to release. Eventually, > I found myself typing `git rebase -i --update-refs` more often than > not, so I just stuck it into the config as `rebase.updateRefs=true`. > > However, sometimes I also maintain those patch branches for multiple > releases. Consider a (hypothetical) situation: > > - tag v1 > - tag v2 > - branch work/myfeature-v1 that is based on tag v1 > > Now, I want to rebase myfeature onto v2, so I do this: > > $ git checkout work/myfeature-v1 > $ git checkout -b work/myfeature-v2 > $ git rebase --onto v2 v1 work/myfeature-v2 > > With `rebase.updateRefs=true`, this ends up silently updating _both_ > work/myfeature-v2 and work/myfeature-v1. Thanks for the explanation. So this is about copying a branch and then rebasing the copy without updating the original. A while ago there was a discussion[1] about excluding branches that match HEAD from "--update-refs". Maybe we should revisit that with a view to adding a config setting that excludes copies of the current branch from "--update-refs". Maintaining multiple versions of the same branch sounds like a lot of work - whats the advantage over merging a single branch into each release? [1] https://lore.kernel.org/git/adb7f680-5bfa-6fa5-6d8a-61323fee7f53@haller-berlin.de/ > With this in mind, I wrote this patch such that update-refs only > happens for interactive rebases, when I have the chance to inspect the > todo list and prune unwanted update-refs items. > > Does this make sense? I made an attempt to explain this motivation in > the commit message, so if this does make sense but the commit message > doesn't, please tell me how to improve/expand the latter. I think having the example in the commit message would help - I feel like I've now got a clear idea of the problem you are facing whereas I didn't understand what the issue was just from the commit message. >>> Try to find a middle ground by introducing a third value, >>> `--update-refs=interactive` (and `rebase.updateRefs=interactive`) >>> which means `--update-refs` when starting an interactive rebase and >>> `--no-update-refs` otherwise. This option is primarily intended to be >>> used in the gitconfig, but is also accepted on the command line >>> for completeness. >> >> I'm not convinced allowing "--update-refs=interactive" on the >> commandline improves the usability - why wouldn't I just say >> "--update-refs" if I want to update all the branches or >> "--no-update-refs" if I don't? I also think supporting >> --update-refs=(true|false) is verbose and unnecessary as the user can >> already specify their intent with the existing option. > > I make heavy use of aliases for various workflows, which invoke one > another (making use of the ability to override earlier command-line > options with the latter ones), and the ability to spell out > `alias.myRebase = rebase ... --update-refs=interactive ...` was useful. You can write your alias as alias.myRebase = -c rebase.updaterefs=interactive rebase ... instead. It is not quite as convenient but it means we don't have to add complexity to the command line interface that is only useful for aliases (I can't think of a use for "--update-refs=interactive" outside of an alias definition). > Re: specifying `=(true|false)`, the intention was to avoid unnecessary > divergence, both in UX and code (and reuse the parser to simplify said > code). If you think it will be harmful, I'll remove that. It would be even simpler if we didn't change the command line interface ;) >>> rebase.updateRefs:: >>> - If set to true enable `--update-refs` option by default. >>> + If set to true, enable the `--update-refs` option of >>> + linkgit:git-rebase[1] by default. When set to 'interactive', >> >> Our existing documentation is inconsistent in how it formats config >> values. rebase.backend uses "apply", rebase.rebaseMerges uses >> `rebase-cousins` which I think matches other commands and is therefore >> what we should use here and rebase.missingCommitCheck uses a mixture >> with "warn" and `drop`. > > Apologies, I'm not sure I understood what exactly you were suggesting > here. Did you mean to suggest wrapping "interactive" in backticks > instead of single quotes? Sorry that wasn't very clear. Yes that is what I was trying to say. >>> + if (v >= 0) >>> + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; >>> + else if (!strcmp("interactive", value)) >>> + return UPDATE_REFS_INTERACTIVE; >>> + >>> + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); >> >> I think we normally say "invalid" or "unknown" rather than "bad" in our >> error messages. It'd be clearer just to list the possible values as >> there are only three of them. > > It's not just three (see other review from Junio), otherwise OK As this is a hint in a error message I don't think we need to exhaustively list all the possible synonyms git accepts for "true" and "false" >>> + /* coerce --update-refs=interactive into yes or no. >>> + * we do it here because there's just too much code below that handles >>> + * {,config_}update_refs in one way or another and modifying it to >>> + * account for the new state would be too invasive. >>> + * all further code uses {,config_}update_refs as a tristate. */ >> >> I think we need to find a cleaner way of handling this. There are only >> two mentions of options.config_update_refs below this point - is it >> really so difficult for those to use the enum? > > See above; I opted to make this change as non-invasive as possible and > keep the complex argument validation logic (lines 1599, 1606-1609) > intact because I'm not even sure I understand it right. > > Besides, even if I convert those uses to use enumerators, I still > wouldn't want to deal with non-tristate values beyond this point. We could add a new boolean variable which is initalized here and use that instead in the code below. Of the code below could just call should_update_refs() to convert the enum to a boolean. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Maintaining multiple versions of the same branch sounds like a lot of > work - whats the advantage over merging a single branch into each > release? Making a single branch that would merge to each release track cleanly, with preparing and maintaining semantic fixes necessary for each track, is probably equally a lot of work, if not more. I try to do that for this project only because I am a perfectionist for these things (and do so for fun), but I can understand if many others (a pragmatist in me included) consider it not worth the effort. After all, it stops mattering once the branch finally gets merged.
On 2025-02-12 at 14:26 +0000, Phillip Wood wrote: > Hi Ivan > > On 11/02/2025 18:11, Ivan Shapovalov wrote: > > On 2025-02-11 at 14:36 +0000, Phillip Wood wrote: > > > On 10/02/2025 19:16, Ivan Shapovalov wrote: > > > > > > I'm a bit surprised by this - I'd have thought there is more scope for > > > messing things up by making a mistake when editing the todo list that > > > for the non-interactive case. Are you able to explain a in a bit more > > > detail the problem you have been experiencing please? > > > > I often find myself managing multiple interdependent downstream patch > > branches, rebasing them en masse from release to release. Eventually, > > I found myself typing `git rebase -i --update-refs` more often than > > not, so I just stuck it into the config as `rebase.updateRefs=true`. > > > > However, sometimes I also maintain those patch branches for multiple > > releases. Consider a (hypothetical) situation: > > > > - tag v1 > > - tag v2 > > - branch work/myfeature-v1 that is based on tag v1 > > > > Now, I want to rebase myfeature onto v2, so I do this: > > > > $ git checkout work/myfeature-v1 > > $ git checkout -b work/myfeature-v2 > > $ git rebase --onto v2 v1 work/myfeature-v2 > > > > With `rebase.updateRefs=true`, this ends up silently updating _both_ > > work/myfeature-v2 and work/myfeature-v1. > > Thanks for the explanation. So this is about copying a branch and then > rebasing the copy without updating the original. A while ago there was a > discussion[1] about excluding branches that match HEAD from > "--update-refs". Maybe we should revisit that with a view to adding a > config setting that excludes copies of the current branch from > "--update-refs". This idea stops working once you have a bunch of interdependent feature branches (consider two branches work/myfeatureA and work/myfeatureB, with the latter based on the former, with each having two versions as described above, and then you rebase work/myfeatureB-v2 from v1 onto v2 and expect to update work/myfeatureA-v2 but not work/myfeatureA-v1). Excluding branches that match HEAD is a very narrow workaround that only fixes one particular instance of one particular workflow. I don't understand the opposition, really — in my understanding, an ability to restrict update-refs to interactive runs is a significantly useful mechanism that does not impose any particular policy. It answers the question of "I want git to _suggest_ updating refs by default, but only if I have a chance to confirm/reject each particular update". > > Maintaining multiple versions of the same branch sounds like a lot of > work - whats the advantage over merging a single branch into each release? Different people, different workflows.
Hi Ivan On 12/02/2025 17:18, Ivan Shapovalov wrote: > On 2025-02-12 at 14:26 +0000, Phillip Wood wrote: >> >> Thanks for the explanation. So this is about copying a branch and then >> rebasing the copy without updating the original. A while ago there was a >> discussion[1] about excluding branches that match HEAD from >> "--update-refs". Maybe we should revisit that with a view to adding a >> config setting that excludes copies of the current branch from >> "--update-refs". > > This idea stops working once you have a bunch of interdependent feature > branches (consider two branches work/myfeatureA and work/myfeatureB, > with the latter based on the former, with each having two versions as > described above, and then you rebase work/myfeatureB-v2 from v1 onto v2 > and expect to update work/myfeatureA-v2 but not work/myfeatureA-v1). > Excluding branches that match HEAD is a very narrow workaround that > only fixes one particular instance of one particular workflow. Good point > I don't understand the opposition, really — in my understanding, an > ability to restrict update-refs to interactive runs is a significantly > useful mechanism that does not impose any particular policy. It answers > the question of "I want git to _suggest_ updating refs by default, but > only if I have a chance to confirm/reject each particular update". I'm not opposed, I'm just trying to understand the problem and see if there are synergies with other issues people have brought to the list in the past. You've convinced me that supporting "rebase.updateRefs=interactive" is worthwhile but I do not think we want to change the commandline interface. I'd much rather reserve the optional argument to support filtering in the future so that git rebase --update-refs='*-v2' --update-refs=^not-me-v2 would update all the branches ending in "-v2" except "not-me-v2". We'd want configure any default patterns separately to whether "--update-refs" was enabled by default which means we can add "rebase .updateRefs=interactive" without boxing ourselves into a corner. >> Maintaining multiple versions of the same branch sounds like a lot of >> work - whats the advantage over merging a single branch into each release? > > Different people, different workflows. Fair enough, from what Junio said it may actually be less work anyway. Best Wishes Phillip
On 12/02/2025 16:58, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Maintaining multiple versions of the same branch sounds like a lot of >> work - whats the advantage over merging a single branch into each >> release? > > Making a single branch that would merge to each release track > cleanly, with preparing and maintaining semantic fixes necessary for > each track, is probably equally a lot of work, if not more. I try > to do that for this project only because I am a perfectionist for > these things (and do so for fun), but I can understand if many > others (a pragmatist in me included) consider it not worth the > effort. After all, it stops mattering once the branch finally gets > merged. Thanks for that insight, I'd assumed the benefits of having a single source of truth for each branch outweighed the costs but it seems that is not necessarily the case. Best Wishes Phillip
On 2025-02-13 at 09:43 +0000, phillip.wood123@gmail.com wrote: > Hi Ivan > > On 12/02/2025 17:18, Ivan Shapovalov wrote: > > On 2025-02-12 at 14:26 +0000, Phillip Wood wrote: > > > > > > Thanks for the explanation. So this is about copying a branch and then > > > rebasing the copy without updating the original. A while ago there was a > > > discussion[1] about excluding branches that match HEAD from > > > "--update-refs". Maybe we should revisit that with a view to adding a > > > config setting that excludes copies of the current branch from > > > "--update-refs". > > > > This idea stops working once you have a bunch of interdependent feature > > branches (consider two branches work/myfeatureA and work/myfeatureB, > > with the latter based on the former, with each having two versions as > > described above, and then you rebase work/myfeatureB-v2 from v1 onto v2 > > and expect to update work/myfeatureA-v2 but not work/myfeatureA-v1). > > Excluding branches that match HEAD is a very narrow workaround that > > only fixes one particular instance of one particular workflow. > > Good point > > > I don't understand the opposition, really — in my understanding, an > > ability to restrict update-refs to interactive runs is a significantly > > useful mechanism that does not impose any particular policy. It answers > > the question of "I want git to _suggest_ updating refs by default, but > > only if I have a chance to confirm/reject each particular update". > > I'm not opposed, I'm just trying to understand the problem and see if > there are synergies with other issues people have brought to the list in > the past. You've convinced me that supporting > "rebase.updateRefs=interactive" is worthwhile but I do not think we want > to change the commandline interface. I'd much rather reserve the > optional argument to support filtering in the future so that > > git rebase --update-refs='*-v2' --update-refs=^not-me-v2 > > would update all the branches ending in "-v2" except "not-me-v2". We'd > want configure any default patterns separately to whether > "--update-refs" was enabled by default which means we can add "rebase > .updateRefs=interactive" without boxing ourselves into a corner. Makes sense, that's indeed a better use of the optional argument. Alright, I'll send a v2 with +stylistic changes and -CLI changes.
Hi Ivan On 13/02/2025 12:04, Ivan Shapovalov wrote: >> would update all the branches ending in "-v2" except "not-me-v2". We'd >> want configure any default patterns separately to whether >> "--update-refs" was enabled by default which means we can add "rebase >> .updateRefs=interactive" without boxing ourselves into a corner. > > Makes sense, that's indeed a better use of the optional argument. > Alright, I'll send a v2 with +stylistic changes and -CLI changes. That's great, glad we're in agreement Thanks Phillip
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index c6187ab28b..d8bbaba69a 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt @@ -24,7 +24,12 @@ rebase.autoStash:: Defaults to false. rebase.updateRefs:: - If set to true enable `--update-refs` option by default. + If set to true, enable the `--update-refs` option of + linkgit:git-rebase[1] by default. When set to 'interactive', + only enable `--update-refs` by default for interactive mode + (equivalent to `--update-refs=interactive`). + This option can be overridden by specifying any form of + `--update-refs` on the command line. rebase.missingCommitsCheck:: If set to "warn", git rebase -i will print a warning if some diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index b18cdbc023..ae6939588d 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -647,12 +647,18 @@ rebase --continue` is invoked. Currently, you cannot pass --update-refs:: --no-update-refs:: +--update-refs=interactive:: Automatically force-update any branches that point to commits that are being rebased. Any branches that are checked out in a worktree are not updated in this way. + +If `--update-refs=interactive` is specified, the behavior is equivalent to +`--update-refs` if the rebase is interactive and `--no-update-refs` otherwise. +(This is mainly useful as a configuration setting, although it might also be +of use in aliases.) ++ If the configuration variable `rebase.updateRefs` is set, then this option -can be used to override and disable this setting. +can be used to override or disable the configuration. + See also INCOMPATIBLE OPTIONS below. diff --git a/builtin/rebase.c b/builtin/rebase.c index 6c9eaf3788..57b456599b 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -129,10 +129,17 @@ struct rebase_options { int reschedule_failed_exec; int reapply_cherry_picks; int fork_point; - int update_refs; + // UPDATE_REFS_{UNKNOWN,NO,ALWAYS} numeric values must never + // change as post-option-parsing code works with {,config_}update_refs + // as if they were ints + enum { + UPDATE_REFS_UNKNOWN = -1, + UPDATE_REFS_NO = 0, + UPDATE_REFS_ALWAYS = 1, + UPDATE_REFS_INTERACTIVE, + } update_refs, config_update_refs; int config_autosquash; int config_rebase_merges; - int config_update_refs; }; #define REBASE_OPTIONS_INIT { \ @@ -150,8 +157,8 @@ struct rebase_options { .autosquash = -1, \ .rebase_merges = -1, \ .config_rebase_merges = -1, \ - .update_refs = -1, \ - .config_update_refs = -1, \ + .update_refs = UPDATE_REFS_UNKNOWN, \ + .config_update_refs = UPDATE_REFS_UNKNOWN, \ .strategy_opts = STRING_LIST_INIT_NODUP,\ } @@ -412,6 +419,18 @@ static void imply_merge(struct rebase_options *opts, const char *option) } } +static int coerce_update_refs(const struct rebase_options *opts, int update_refs) +{ + /* coerce "=interactive" into "no" rather than "not set" when not interactive + * this way, `git -c rebase.updateRefs=yes rebase --update-refs=interactive [without -i]` + * will not inherit the "yes" from the config */ + if (update_refs == UPDATE_REFS_INTERACTIVE) + return (opts->flags & REBASE_INTERACTIVE_EXPLICIT) + ? UPDATE_REFS_ALWAYS + : UPDATE_REFS_NO; + return update_refs; +} + /* Returns the filename prefixed by the state_dir */ static const char *state_dir_path(const char *filename, struct rebase_options *opts) { @@ -779,6 +798,17 @@ static void parse_rebase_merges_value(struct rebase_options *options, const char die(_("Unknown rebase-merges mode: %s"), value); } +static int parse_update_refs_value(const char *value, const char *desc) +{ + int v = git_parse_maybe_bool(value); + if (v >= 0) + return v ? UPDATE_REFS_ALWAYS : UPDATE_REFS_NO; + else if (!strcmp("interactive", value)) + return UPDATE_REFS_INTERACTIVE; + + die(_("bad %s value '%s'; valid values are boolean or \"interactive\""), desc, value); +} + static int rebase_config(const char *var, const char *value, const struct config_context *ctx, void *data) { @@ -821,7 +851,8 @@ static int rebase_config(const char *var, const char *value, } if (!strcmp(var, "rebase.updaterefs")) { - opts->config_update_refs = git_config_bool(var, value); + opts->config_update_refs = parse_update_refs_value(value, + "rebase.updateRefs"); return 0; } @@ -1042,6 +1073,19 @@ static int parse_opt_rebase_merges(const struct option *opt, const char *arg, in return 0; } +static int parse_opt_update_refs(const struct option *opt, const char *arg, int unset) +{ + struct rebase_options *options = opt->value; + + if (arg) + options->update_refs = parse_update_refs_value(arg, + "--update-refs"); + else + options->update_refs = unset ? UPDATE_REFS_NO : UPDATE_REFS_ALWAYS; + + return 0; +} + static void NORETURN error_on_missing_default_upstream(void) { struct branch *current_branch = branch_get(NULL); @@ -1187,9 +1231,11 @@ int cmd_rebase(int argc, OPT_BOOL(0, "autosquash", &options.autosquash, N_("move commits that begin with " "squash!/fixup! under -i")), - OPT_BOOL(0, "update-refs", &options.update_refs, - N_("update branches that point to commits " - "that are being rebased")), + OPT_CALLBACK_F(0, "update-refs", &options, + N_("(bool|interactive)"), + N_("update branches that point to commits " + "that are being rebased"), + PARSE_OPT_OPTARG, parse_opt_update_refs), { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"), N_("GPG-sign commits"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, @@ -1528,6 +1574,16 @@ int cmd_rebase(int argc, if (isatty(2) && options.flags & REBASE_NO_QUIET) strbuf_addstr(&options.git_format_patch_opt, " --progress"); + /* coerce --update-refs=interactive into yes or no. + * we do it here because there's just too much code below that handles + * {,config_}update_refs in one way or another and modifying it to + * account for the new state would be too invasive. + * all further code uses {,config_}update_refs as a tristate. */ + options.update_refs = + coerce_update_refs(&options, options.update_refs); + options.config_update_refs = + coerce_update_refs(&options, options.config_update_refs); + if (options.git_am_opts.nr || options.type == REBASE_APPLY) { /* all am options except -q are compatible only with --apply */ for (i = options.git_am_opts.nr - 1; i >= 0; i--)
In rebase-heavy workflows involving multiple interdependent feature branches, typing out `--update-refs` quickly becomes tiring, which can be mitigated with setting the `rebase.updateRefs` git-config option to perform update-refs by default. However, the utility of `rebase.updateRefs` is somewhat limited because you rarely want it in a non-interactive rebase (as it does not give you the chance to review the update-refs candidates, likely leading to updating refs that you didn't want updated -- I made quite an amount of mess by setting this option and subsequently forgetting about it). Try to find a middle ground by introducing a third value, `--update-refs=interactive` (and `rebase.updateRefs=interactive`) which means `--update-refs` when starting an interactive rebase and `--no-update-refs` otherwise. This option is primarily intended to be used in the gitconfig, but is also accepted on the command line for completeness. Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name> --- Documentation/config/rebase.txt | 7 +++- Documentation/git-rebase.txt | 8 +++- builtin/rebase.c | 72 +++++++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 10 deletions(-)