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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 11, 2025, 14:36 UTC
Message-ID: <1279671f-4063-4347-b153-9f6ff079bd77@gmail.com>
In-Reply-To: <20250210191650.316329-1-intelfx@intelfx.name>

Hi Ivan

On 10/02/2025 19:16, Ivan Shapovalov wrote:

Show 10 quoted lines

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

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

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

Show 8 quoted lines

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

Show 6 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(...)"

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

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

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

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

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