On Tue, Jan 28, 2025 at 4:45 PM Olga Pilipenco
[off-list ref] wrote:

quoted

On Jan 19, 2025, at 3:30 PM, Eric Sunshine [off-list ref] wrote:
On Thu, Jan 16, 2025 at 4:35 PM Olga Pilipenco via GitGitGadget
[off-list ref] 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.)

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+    /*
+    * 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.)