From: "Rubén Justo" <rjusto@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Victoria Dye" <vdye@github.com>
Cc: Git List <git@vger.kernel.org>, Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH 2/2] branch: clear target branch configuration before copying or renaming
Date: Sun, 20 Nov 2022 15:55:51 +0100 [thread overview]
Message-ID: <660dc134-121c-11b1-f722-2b4946495c40@gmail.com> (raw)
In-Reply-To: <221118.86r0y0f5ff.gmgdl@evledraar.gmail.com>
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.
prev parent reply other threads:[~2022-11-20 14:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-17 1:33 [PATCH 0/2] branch: fix some malfunctions in -m/-c Rubén Justo
2022-11-17 1:36 ` [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op Rubén Justo
2022-11-17 22:18 ` Victoria Dye
2022-11-20 8:10 ` Rubén Justo
2022-11-18 3:58 ` Ævar Arnfjörð Bjarmason
2022-11-20 9:34 ` Rubén Justo
2022-11-17 1:44 ` [PATCH 2/2] branch: clear target branch configuration before copying or renaming Rubén Justo
2022-11-18 2:10 ` Victoria Dye
2022-11-20 9:20 ` Rubén Justo
2022-11-20 22:10 ` Victoria Dye
2022-11-21 23:13 ` Rubén Justo
2022-11-18 4:51 ` Ævar Arnfjörð Bjarmason
2022-11-18 16:36 ` Victoria Dye
2022-11-18 18:12 ` Ævar Arnfjörð Bjarmason
2022-11-20 14:55 ` Rubén Justo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=660dc134-121c-11b1-f722-2b4946495c40@gmail.com \
--to=rjusto@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).