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. -- Ivan Shapovalov / intelfx / > > 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 >
Attachment:
signature.asc
Description: This is a digitally signed message part