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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2025-02-11 at 14:36 +0000, Phillip Wood wrote:
> 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?

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.

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.

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

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

It'd just be a lot of code to update, incl. stylistically (especially
the implicit truthy/falsy coercions or `>= 0`). I opted to make the
change as non-invasive as possible.

> Also note that comments should be formatted as
> 
> /* single line comment */
> 
> or
> 
> /*
>   * multi-line
>   * comment
>   */

OK

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

OK

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

OK

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

OK

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

-- 
Ivan Shapovalov / intelfx /

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

Attachment: signature.asc
Description: This is a digitally signed message part


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux