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@xxxxxxxxxxxxxxxx/
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
