Re: [PATCH] rebase: add `--update-refs=interactive`

From: Ivan Shapovalov <intelfx@intelfx.name>
Date: Feb 11, 2025, 18:11 UTC
Message-ID: <f689c263ead8104ec42f63f1e9ed10350a27ae1d.camel@intelfx.name>
In-Reply-To: <1279671f-4063-4347-b153-9f6ff079bd77@gmail.com>

On 2025-02-11 at 14:36 +0000, Phillip Wood wrote:

Show 18 quoted lines

> 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.

Show 14 quoted lines

> 
> > 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.

Show 11 quoted lines

> 
> >   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?

Show 16 quoted lines

> 
> > +	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.

Show 10 quoted lines

> Also note that comments should be formatted as
> 
> /* single line comment */
> 
> or
> 
> /*
>   * multi-line
>   * comment
>   */

OK

Show 14 quoted lines

> 
> > +	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

Show 7 quoted lines

> 
> > +{
> > +	/* 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

Show 14 quoted lines

> 
> > +	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

Show 11 quoted lines

> 
> > +	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

Show 10 quoted lines

> 
> > +	/* 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
>