On Tue, Jan 28, 2025 at 4:45 PM Olga Pilipenco
<olga.pilipenco@xxxxxxxxxxx> wrote:
> > On Jan 19, 2025, at 3:30 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > On Thu, Jan 16, 2025 at 4:35 PM Olga Pilipenco via GitGitGadget
> > <gitgitgadget@xxxxxxxxx> wrote:
> > I found that I had to dig around a bit to fully understand the problem
> > expressed by this commit message. Perhaps adding a bit more detail
> > would help? Here's my attempt at rewriting the above (also in a way
> > which is more idiomatic to this project):
> >
> > When extensions.worktreeConfig is true and the main worktree is
> > bare -- that is, its config.worktree file contains core.bare=true
> > -- commands run from secondary worktrees incorrectly see the main
> > worktree as not bare. As such, those commands incorrectly think
> > that the repository's default branch (typically "main" or
> > "master") is checked out in the bare repository even though it's
> > not. This makes it impossible, for instance, to checkout or delete
> > the default branch from a secondary worktree, among other
> > shortcomings.
> >
> > This problem occurs because, when extensions.worktreeConfig is
> > true, commands run in secondary worktrees only consult
> > $commondir/config and $commondir/worktrees/<id>/config.worktree,
> > thus they never see the main worktree's core.bare=true setting in
> > $commondir/config.worktree.
> >
> > Fix this problem by consulting the main worktree's config.worktree
> > file when checking whether it is bare. (This extra work is
> > performed only when running from a secondary worktree.)
>
> Wow, your explanation is so much better than mine.Thank you for
> “translating" it for the world :) I’m still trying to get used to
> the terminology used in this codebase. I’ll steal your description
> for sure (if you don’t mind).
You are more than welcome to use the proposed commit message rewrite.
(If you want to acknowledge assistance rendered, a Helped-by: trailer,
preceding your Signed-off-by:, is the way to do so. Or not. It's up to
you.)
> >> diff --git a/worktree.c b/worktree.c
> >> @@ -65,6 +65,28 @@ static int is_current_worktree(struct worktree *wt)
> >> +static int is_bare_git_dir(const char *git_dir)
> >
> > Nit: I wonder if a name such as is_main_worktree_bare() would clue
> > readers in a bit more?
>
> I was about to explain how I wanted this function to be more generic
> and handle all sorts of bare and non-bare cases - whether it’s the
> main worktree or not. However, after seeing your comments and after
> revisiting the code, I realized that generalization doesn’t really
> provide much benefit here. It is much clearer if we're explicit that
> the bare check in this case is only performed on the main
> worktree. I’ll update it in the next version.
I see. When reviewing, I was wondering why the git-dir was being
passed into the function. Your explanation above answers that
question. On that note, in addition to renaming the function as
suggested, for clarity, I would probably go a bit further and pass in
a `struct repository *` rather than passing in the git-dir itself,
just to make it clear that the function is checking main-worktree
bareness of the repository in question, as opposed to merely checking
bareness of any arbitrary directory. (At least, I would find the
intention more clear at-a-glance with that additional change applied.)
> >> + config_file = xstrfmt("%s/config", git_dir);
> >> + worktree_config_file = xstrfmt("%s/config.worktree", git_dir);
> >> +
> >> + git_configset_init(&cs);
> >> + git_configset_add_file(&cs, config_file);
> >> + git_configset_add_file(&cs, worktree_config_file);
> >
> > Genuine question: I haven't thought too deeply about it, but do we
> > gain anything by loading $commondir/config here -- which is shared by
> > the main worktree and all secondary worktrees -- considering that it
> > was already loaded and consulted by the earlier is-bare check before
> > this function was even called?
>
> This function determines if a worktree is bare or not. I want this
> logic to work even when it’s called from a different context and not
> rely on other is-bare checks (that are a bit confusing tbh).
Agreed about the is-bare checks -- and indeed the entire Git startup
sequence -- being difficult to digest, however...
One reason I asked the question was due to concern that future readers
of this code may very well wonder (as I did) why $commondir/config is
being loaded when doing so is (apparently) unnecessary in this
particular context. The question is especially pertinent given that
this is a private helper function with a single caller. A second
reason was that, over the years, a good deal of effort has been put
into optimizing Git's startup to avoid doing unnecessary work, and
this appears to be unnecessary since $commondir/config would already
have been consulted by earlier checks before this function gets called
(assuming I'm correctly understanding the code-flow).
Anyhow, we can probably punt on the question for the moment and leave
the code as you wrote it if you feel strongly about it or if you think
it is clearer this way for future readers.
> >> + /*
> >> + * NEEDSWORK: the_repository is not always main worktree's repository
> >> + */
> >> worktree->repo = the_repository;
> >> worktree->path = strbuf_detach(&worktree_path, NULL);
> >
> > I found this new NEEDSWORK comment rather confusing the first several
> > times I read the patch. It wasn't until I finally realized that the
> > reference to `the_repository` here is the same reference to
> > `the_repository` in the commit message -- which confused me, as well
> > -- that I understood what this was trying to say. The actual problem,
> > of course, is that the _configuration_ stored in `the_repository` is
> > the secondary worktree's configuration, not the main worktree's
> > configuration. Considering that this patch addresses that problem, I'd
> > probably just drop this new comment altogether (unless, perhaps, you
> > rewrite it to talk about the _configuration_ stored in
> > `the_repository`).
>
> This `the_repository` structure is soooo confusing, took me a while
> to figure out what it is! I would feel guilty not mentioning that
> under some circumstances `the_repository` assigned here could be not
> actual configuration of the worktree object. I don’t know if that
> will ever matter or not, but I find this assignment kinda “stinky”
> and want everyone to know about it. I don’t want to change this
> assignment in this patch because it didn’t bring any harm so far.
> I’ll try again to rephrase this comment, just to give a heads up in
> case someone experiences “weird” behaviour in this area (same way
> the previous NEEDSWORK comment gave me ideas why my workflow didn’t
> work and inspired me to try to fix it).
Likely, the confusion is an outcome of the natural evolution
(mutation) software undergoes. The development of linked worktrees and
the concept of a `repository` structure did not necessarily occur
concurrently. I suppose one could develop one of two views (if not
more) of the `repository`: (1) an in-memory representation of the
".git" directory or bare-repository "object database", including all
worktrees hanging off it, or (2) a single worktree's
view/representation of the repository, meaning paths, configuration,
"index" specific to that worktree.
In the present state of the code, the second view is the more accurate
one, so the existing `worktree->repo = the_repository` assignment does
make sense without any further commentary. My main concern with the
NEEDSWORK comment is that it implies that there is a problem with the
assignment, even though there isn't. While it may be true that the
entire `repository` idea needs to be rethought or clarified or
expanded, that's a global issue permeating the entire code-base, not
specific to this one spot, which is why it feels inappropriate to have
a NEEDSWORK comment here. So, I'm not, in general, opposed to a
comment explaining the the `worktree->repo` assignment for future
readers if you think that would be valuable, but I am concerned about
giving it a "NEEDSWORK" prefix, which feels misleading for this
particular piece of code.
> Thanks for the review. I’ll incorporate the changes in my next
> version and hopefully it will be good to go :tada: I hope I
> responded to all the comments, it’s a bit nerve-wrecking to
> contribute for the first time (so many rules and instructions!) :)
Understood, and it didn't help the nerve situation when your v1 was
apparently ignored. Rest assured, though, that your submission was
nicely done and fixes a real problem which ought to be addressed. (In
fact, I'm surprised it took this long for someone to tackle it. So,
thanks.)
