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

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

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

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

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