On 18-nov-2022 19:12:10, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Nov 18 2022, Victoria Dye wrote:
>
> > Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Thu, Nov 17 2022, Rubén Justo wrote:
> >>> Whenever we copy or move (forced or not) we must make sure that there is
> >>> no residual configuration that will be, probably erroneously, inherited
> >>> by the new branch. To avoid confusions, clear any branch configuration
> >>> before setting the configuration from the copied or moved branch.
> >>
> >> So, whatever tea leaves we read into the history, or whether this was a
> >> good or bad design in the first place, I think we should probably lean
> >> towards not having this be a bug fix, but a new feature. Both modes are
> >> clearly easy to support.
> >>
> >> And then document it in terms of some new switch being the equivalent to
> >> --remove-section followed by a rename, the existing thing a rename etc.
Mmm... the way I see this is as an unexpected _and unintentional_
result, not sure if I am happy describing this is as a bug.
Maybe we can see this as "completing the functionality of moving and
copying a branch", a missing "NEEDSWORK". Though I think the message
describing this patch as a problem is sensible because of the
motivations and for future reference.
I might agree with a new option like "--mix-configuration" to add this
"feature" (and document it). But I don't find sensible to consolidate
this unexpected and unintentional result, imho, and making the
expectable an option.
> But my spidey sense is a bit tickled by the proposed change discussing
> the existing behavior as a pure bug, when it seems to me that it's
> approximately what you'd get if it was implemented in terms of "config
> --rename-section", v.s. the proposed new behavior of (as I understand
> it) a "config --remove-section" followed by "config --rename-section".
Explaining this as something you'd get modifying directly the
configuration, might not be a good idea. A user can break the config,
but "git branch" should not.
> I think a conservative approach would be to focus narrowly on the
> tracking config, if that's the thing that's at issue. I.e. we read that
> ourselves, know that it's broken if it's transmuted in certain ways etc.
>
> But the proposed patch suggests that we extend that fix to anything we
> might find in that config namespace.
Yes, as the expectation (again, imho) is that the resultant branch gets
its original configuration not a mix of the original and the remanent.
> >>> @@ -583,12 +583,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >>>
> >>> strbuf_release(&logmsg);
> >>>
> >>> - strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> >>> - strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> >>> - if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >>> - die(_("Branch is renamed, but update of config-file failed"));
> >>> - if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> >>> - die(_("Branch is copied, but update of config-file failed"));
> >>> + if (strcmp(interpreted_oldname, interpreted_newname)) {
> >>> + strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> >>> + strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> >>> +
> >>> + delete_branch_config(interpreted_newname);
> >>> +
> >>> + if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >>> + die(_("Branch is renamed, but update of config-file failed"));
> >>> + if (copy && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> >>> + die(_("Branch is copied, but update of config-file failed"));
> >>
> >> Aside from any question of a hypothetical "should", your implementation
> >> is running head-first into a major caveat in our config API.
> >>
> >> Which is that we don't have transactions or rollbacks, and we don't even
> >> carry a lock forward for all of these.
> >>
> >> So, there's crappy edge cases in the old implementation as you've found,
> >> but at least it mostly failed-safe.
> >>
> >> But here we'll delete_branch_config(), then release the lock, and then
> >> try to rename the new branch to that location, which might fail.
> >>
> >> So, we'll be left with no config for the thing we tried to clobber, nor
> >> the new config.
> >
> > This is a good point, so to add to it: I think a fairly unobtrusive solution
> > would be to move the config deletion after the rename is 100% complete.
The update of the ref is already done when we operate on the
configuration. I think that _even_ if the
git_config_{rename/copy}_section fails, the delete_branch_config() is
sensible.
> Also, whatever this is supposed to do, shouldn't we make it consistent
> with other "git branch" modes. E.g.:
>
> git branch -D foo
> git -P config --show-origin --get-regexp '^branch\.foo\.'
> git config branch.foo.rebase true
> git branch foo -t origin/seen
> git -P config --show-origin --get-regexp '^branch\.foo\.'
>
> Will emit:
>
> Deleted branch foo (was ...).
> branch 'foo' set up to track 'origin/seen'.
> file:.git/config branch.foo.rebase true
> file:.git/config branch.foo.remote origin
> file:.git/config branch.foo.merge refs/heads/seen
I'm not sure. I did notice that, and I can think of ill but legit uses
there: a user might be setting the configuration and then creating the
reference. It is tangential here, but we can issue a warning like:
$ git branch -t foo
$ git branch -t bar
$ git branch -C foo bar
$ git branch --set-upstream-to foo bar
warning: branch.bar.remote has multiple values
warning: branch.bar.merge has multiple values
branch 'bar' set up to track 'foo'.
I have different unsent patches to address this too, but I'm not sure
what we want there, and how.
> The upthread commit message doesn't discuss it, but even multiple
> "merge" values are a valid state of affairs. E.g.:
>
> $ git branch seen -t origin/seen
> branch 'seen' set up to track 'origin/seen'.
> $ git branch next -t origin/next
> branch 'next' set up to track 'origin/next'.
> $ git branch -C seen next
> $ git config --get-regexp 'branch\.next'
> branch.next.remote origin
> branch.next.merge refs/heads/seen
> branch.next.remote origin
> branch.next.merge refs/heads/next
>
> Now, if you do:
>
> git checkout next
>
> and:
> git pull --no-rebase
>
> You'll get e.g.:
>
> Merge branches 'next' and 'seen' of github.com:git/git into next
I think this is unexpected and unintentional.
> And again, don't take any of that as an a-priori argument against
> changing it, I'm still undecided. I'd just prefer that we know what it
> is we're changing.
Thank you for your analysis.
