From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 11 Feb 2025 14:36:03 GMT
Subject: Re: [PATCH] rebase: add `--update-refs=interactive`
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:
> 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