On Thu, Jan 30, 2025 at 2:09 AM Olga Pilipenco <olga.pilipenco@xxxxxxxxxxx> wrot
> On Jan 29, 2025, at 6:41 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > 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.)
>
> Indeed, no need to pass git-dir anymore. There is actually no need
> to pass `the_repository` because it’s global. I like how this
> simplified things and made code clearer.
The reason I suggested passing in a `struct repository *` is that the
project is slowly moving away from the `the_repository` global, so
making this new function accept a `struct repository *` as its sole
argument means less work later on.
> > 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).
>
> I trust your judgement and knowledge of the code and really like the
> reasons presented. I’ll change this function to only check for
> worktree config. However I’d like to give it a good name where it’s
> clear we only check worktree config. It’s a bit challenging to make
> it short-ish and not to include multiple “worktree” words in the
> name. Before I submit a new release, maybe I have time to quickly
> align on the name. What do you think about this one:
>
> is_main_worktree_bare_in_worktree_config
>
> (It will check if bare=true in the main worktree’s worktree.config)
>
> Naming is harder than the code itself :)
It's a historic "accident" that when worktree support was designed,
the idea of linking worktrees to a bare repository was not considered.
Support for using worktrees with a bare repository was added later.
However, by that time, the term "main worktree" was already well
established, with the very unfortunate result that even when there is
no actual "main worktree" but only a bare repository with "linked
worktrees" hanging off it, the repository itself is usually referred
to as the "bare main worktree", which is an obvious misnomer; the
repository is just a repository (i.e. the object database and other
meta-information) and there is no actual main worktree.
Given the very real potential for confusion when employing the "bare
main worktree" misnomer, I suspect that we won't be able to come up
with a good name which easily conveys the function's purpose. Since
this is an internal helper (hence, there is slightly less pressure to
come up with a perfect name) rather than public API, this might be one
of those cases in which it makes more sense to choose a concise name
and then explain the function's purpose with a short comment block.
Perhaps something like this would be most helpful to future readers of
the code:
/*
* When in a secondary worktree, and when extensions.worktreeConfig
* is true, only $commondir/config and $commondir/worktrees/<id>/
* config.worktree are consulted, hence any core.bare=true setting in
* $commondir/config.worktree gets overlooked. Thus, check it manually
* to determine if the repository is bare.
*/
static int is_repo_bare(struct repository *r) {...}
> Thank you for this thorough explanation. I’ll drop the comment
> completely. Less code to read. (To be honest I’m not a big fan of
> comments and definitely don’t want to introduce confusing comments
> :)
Understood. Self-explanatory code is preferred. That said, a comment
such as the one proposed above can really help readers not intimately
familiar with this otherwise nonobvious behavior, thus may be
justified.
> Fun fact: my email app kept changing “worktree” to “corktree”. Maybe
> another git feature?
Your mailer obviously suffers from Gitophobia or is perhaps a nemophilist.
(By the way, when replying, please use the normal ">", "> >", "> > >"
markers to signify quoted portions of earlier messages in the thread
rather than using only indentation. The reason I make this request is
that when I replied to your message, my mailer stripped away all
indentation from your message, leaving all earlier quoted portions
flush with the left margin, which made it very difficult to figure out
which quotes came from which authors from which earlier messages, and
I ended up having to reinsert the "> >" markers manually to restore
structure to my reply.)
