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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ivan

On 11/02/2025 18:11, Ivan Shapovalov wrote:
> 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.

Thanks for the explanation. So this is about copying a branch and then 
rebasing the copy without updating the original. A while ago there was a 
discussion[1] about excluding branches that match HEAD from 
"--update-refs". Maybe we should revisit that with a view to adding a 
config setting that excludes copies of the current branch from 
"--update-refs".

Maintaining multiple versions of the same branch sounds like a lot of 
work - whats the advantage over merging a single branch into each release?

[1] 
https://lore.kernel.org/git/adb7f680-5bfa-6fa5-6d8a-61323fee7f53@haller-berlin.de/

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

I think having the example in the commit message would help - I feel 
like I've now got a clear idea of the problem you are facing whereas I 
didn't understand what the issue was just from the commit message.

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

You can write your alias as

    alias.myRebase = -c rebase.updaterefs=interactive rebase ...

instead. It is not quite as convenient but it means we don't have to add 
complexity to the command line interface that is only useful for aliases 
(I can't think of a use for "--update-refs=interactive" outside of an 
alias definition).

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

It would be even simpler if we didn't change the command line interface ;)

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

Sorry that wasn't very clear. Yes that is what I was trying to say.

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

As this is a hint in a error message I don't think we need to 
exhaustively list all the possible synonyms git accepts for "true" and 
"false"

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

We could add a new boolean variable which is initalized here and use 
that instead in the code below. Of the code below could just call 
should_update_refs() to convert the enum to a boolean.

Best Wishes

Phillip