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

[Ivan Shapovalov <intelfx@intelfx.name>]

[-- Attachment #1: Type: text/plain, Size: 8096 bytes --]

On 2025-02-12 at 14:26 +0000, Phillip Wood wrote:
> 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".

This idea stops working once you have a bunch of interdependent feature
branches (consider two branches work/myfeatureA and work/myfeatureB,
with the latter based on the former, with each having two versions as
described above, and then you rebase work/myfeatureB-v2 from v1 onto v2
and expect to update work/myfeatureA-v2 but not work/myfeatureA-v1).
Excluding branches that match HEAD is a very narrow workaround that
only fixes one particular instance of one particular workflow.

I don't understand the opposition, really — in my understanding, an
ability to restrict update-refs to interactive runs is a significantly
useful mechanism that does not impose any particular policy. It answers
the question of "I want git to _suggest_ updating refs by default, but
only if I have a chance to confirm/reject each particular update".

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

Different people, different workflows.

-- 
Ivan Shapovalov / intelfx /

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 862 bytes --]