| Message ID | pull.1829.v3.git.1738346881907.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] worktree: detect from secondary worktree if main worktree is bare | expand |
"Olga Pilipenco via GitGitGadget" <gitgitgadget@gmail.com> writes: > +/* > +* 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_main_worktree_bare(struct repository *repo) > +{ > + int bare = 0; > + struct config_set cs = {0}; > + char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo)); > + > + git_configset_init(&cs); > + git_configset_add_file(&cs, worktree_config); > + git_configset_get_bool(&cs, "core.bare", &bare); > + > + git_configset_clear(&cs); > + free(worktree_config); > + return bare; > +} That is nicely described. > /** > * get the main worktree > */ > @@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head) > CALLOC_ARRAY(worktree, 1); > worktree->repo = the_repository; > worktree->path = strbuf_detach(&worktree_path, NULL); > - /* > - * NEEDSWORK: If this function is called from a secondary worktree and > - * config.worktree is present, is_bare_repository_cfg will reflect the > - * contents of config.worktree, not the contents of the main worktree. > - * This means that worktree->is_bare may be set to 0 even if the main > - * worktree is configured to be bare. > - */ > - worktree->is_bare = (is_bare_repository_cfg == 1) || > - is_bare_repository(); > worktree->is_current = is_current_worktree(worktree); > + worktree->is_bare = (is_bare_repository_cfg == 1) || > + is_bare_repository() || > + (!worktree->is_current && is_main_worktree_bare(the_repository)); Is "this worktree does not have is_current bit set" equivalent to "this worktree is the main one, so is_main_worktree_bare() needs to be consulted"? That linkage between "the is_current bit unset" and "is the main worktree" is not obvious to me. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "Olga Pilipenco via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> +/* >> +* 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_main_worktree_bare(struct repository *repo) >> +{ >> + int bare = 0; >> + struct config_set cs = {0}; >> + char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo)); >> + >> + git_configset_init(&cs); >> + git_configset_add_file(&cs, worktree_config); >> + git_configset_get_bool(&cs, "core.bare", &bare); >> + >> + git_configset_clear(&cs); >> + free(worktree_config); >> + return bare; >> +} > > That is nicely described. > >> /** >> * get the main worktree >> */ >> @@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head) >> CALLOC_ARRAY(worktree, 1); >> worktree->repo = the_repository; >> worktree->path = strbuf_detach(&worktree_path, NULL); >> - /* >> - * NEEDSWORK: If this function is called from a secondary worktree and >> - * config.worktree is present, is_bare_repository_cfg will reflect the >> - * contents of config.worktree, not the contents of the main worktree. >> - * This means that worktree->is_bare may be set to 0 even if the main >> - * worktree is configured to be bare. >> - */ >> - worktree->is_bare = (is_bare_repository_cfg == 1) || >> - is_bare_repository(); >> worktree->is_current = is_current_worktree(worktree); >> + worktree->is_bare = (is_bare_repository_cfg == 1) || >> + is_bare_repository() || >> + (!worktree->is_current && is_main_worktree_bare(the_repository)); > > Is "this worktree does not have is_current bit set" equivalent to > "this worktree is the main one, so is_main_worktree_bare() needs to > be consulted"? That linkage between "the is_current bit unset" and > "is the main worktree" is not obvious to me. Does the thinking behind it go like this? We grabbed the "main" worktree object and stored it in worktree; it is either our current worktree (in which case is_current is true), or it is not (in which case, is_current is false). We know that the old logic failed when asking the "is it bare" question from a secondary worktree. !worktree->is_current tells us that we _are_ asking the question from a secondary worktree, so we need to make the extra call to check config.worktree file as well in that case. Perhaps the logic is clear to those who diagnosed the problem, wrote the patch, and reviewed it, in which case there is no reason to reroll. Perhaps it was just me to whom it was not obvious that the purpose of "is_current" check was not about "are we looking at the main worktree" but was about "if we are not in the main worktree, we need this extra check". Thanks.
On Fri, Jan 31, 2025 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > "Olga Pilipenco via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > >> +/* > >> +* 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_main_worktree_bare(struct repository *repo) > >> +{ > >> + int bare = 0; > >> + struct config_set cs = {0}; > >> + char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo)); > >> + > >> + git_configset_init(&cs); > >> + git_configset_add_file(&cs, worktree_config); > >> + git_configset_get_bool(&cs, "core.bare", &bare); > >> + > >> + git_configset_clear(&cs); > >> + free(worktree_config); > >> + return bare; > >> +} > > > > That is nicely described. > > > >> /** > >> * get the main worktree > >> */ > >> @@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head) > >> CALLOC_ARRAY(worktree, 1); > >> worktree->repo = the_repository; > >> worktree->path = strbuf_detach(&worktree_path, NULL); > >> - /* > >> - * NEEDSWORK: If this function is called from a secondary worktree and > >> - * config.worktree is present, is_bare_repository_cfg will reflect the > >> - * contents of config.worktree, not the contents of the main worktree. > >> - * This means that worktree->is_bare may be set to 0 even if the main > >> - * worktree is configured to be bare. > >> - */ > >> - worktree->is_bare = (is_bare_repository_cfg == 1) || > >> - is_bare_repository(); > >> worktree->is_current = is_current_worktree(worktree); > >> + worktree->is_bare = (is_bare_repository_cfg == 1) || > >> + is_bare_repository() || > >> + (!worktree->is_current && is_main_worktree_bare(the_repository)); > > > > Is "this worktree does not have is_current bit set" equivalent to > > "this worktree is the main one, so is_main_worktree_bare() needs to > > be consulted"? That linkage between "the is_current bit unset" and > > "is the main worktree" is not obvious to me. > > Does the thinking behind it go like this? > > We grabbed the "main" worktree object and stored it in worktree; > it is either our current worktree (in which case is_current is > true), or it is not (in which case, is_current is false). We > know that the old logic failed when asking the "is it bare" > question from a secondary worktree. !worktree->is_current tells > us that we _are_ asking the question from a secondary worktree, > so we need to make the extra call to check config.worktree file > as well in that case. > > Perhaps the logic is clear to those who diagnosed the problem, wrote > the patch, and reviewed it, in which case there is no reason to > reroll. Perhaps it was just me to whom it was not obvious that > the purpose of "is_current" check was not about "are we looking at > the main worktree" but was about "if we are not in the main worktree, > we need this extra check". > > Thanks. You did a great job figuring it out and I agree it's confusing at first, but we tried our best to make it less confusing. `is_current` check is actually not necessary there, but having it there saves extra unnecessary calculations, also describes & fixes the exact scenario that didn't work (not being able to see main worktree as bare from a secondary worktree). Thanks for looking into that.
Olga Pilipenco <olga.pilipenco@shopify.com> writes: >> Perhaps the logic is clear to those who diagnosed the problem, wrote >> the patch, and reviewed it, in which case there is no reason to >> reroll. Perhaps it was just me to whom it was not obvious that >> the purpose of "is_current" check was not about "are we looking at >> the main worktree" but was about "if we are not in the main worktree, >> we need this extra check". >> >> Thanks. > > You did a great job figuring it out and I agree it's confusing at > first, but we tried our best to make it less confusing. > `is_current` check is actually not necessary there, but having it there saves > extra unnecessary calculations, also describes & fixes the exact scenario > that didn't work (not being able to see main worktree as bare from a > secondary worktree). If I had to do a great job there, then the code does deserve to be explained a bit better for later developers who wonder why it is written in the way it is, perhaps we a single-liner comment? Thanks.
On Fri, Jan 31, 2025 at 1:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > Olga Pilipenco <olga.pilipenco@shopify.com> writes: > > >> Perhaps the logic is clear to those who diagnosed the problem, wrote > >> the patch, and reviewed it, in which case there is no reason to > >> reroll. Perhaps it was just me to whom it was not obvious that > >> the purpose of "is_current" check was not about "are we looking at > >> the main worktree" but was about "if we are not in the main worktree, > >> we need this extra check". > >> > >> Thanks. > > > > You did a great job figuring it out and I agree it's confusing at > > first, but we tried our best to make it less confusing. > > `is_current` check is actually not necessary there, but having it there saves > > extra unnecessary calculations, also describes & fixes the exact scenario > > that didn't work (not being able to see main worktree as bare from a > > secondary worktree). > > If I had to do a great job there, then the code does deserve to be > explained a bit better for later developers who wonder why it is > written in the way it is, perhaps we a single-liner comment? I have 2 versions for comment: 1. Since is_main_worktree_bare explains quite well what it does we can have a shorter explanation of `!worktree->is_current` part, something like: /* Additional checks are needed if main worktree is not current (checking from secondary worktree) */ (!worktree->is_current && is_main_worktree_bare(the_repository)); 2. Or a bit longer inline explanation that partially repeats the explanation of is_main_worktree_bare + adds explanation about efficiency: /* * When in a secondary worktree we have to also verify if the main worktree * is bare in $commondir/config.worktree. * This check is unnecessary if we're currently in the main worktree, * as prior checks already consulted all configs of the current worktree. */ (!worktree->is_current && is_main_worktree_bare(the_repository)); Let me know if any of these work. Thanks. > Thanks.
Olga Pilipenco <olga.pilipenco@shopify.com> writes: > I have 2 versions for comment: > > 1. Since is_main_worktree_bare explains quite well what it does we can have > a shorter explanation of `!worktree->is_current` part, something like: > > /* Additional checks are needed if main worktree is not current > (checking from secondary worktree) */ > (!worktree->is_current && is_main_worktree_bare(the_repository)); For somebody who thought about the issue themselves (like me, before writing the message you are responding to), this shorter form would suffice. I'd rephrase it more like so /* When a secondary worktree, an extra check is needed */ for brevity, though. > 2. Or a bit longer inline explanation that partially repeats the > explanation of is_main_worktree_bare > + adds explanation about efficiency: > /* > * When in a secondary worktree we have to also verify if the main worktree > * is bare in $commondir/config.worktree. > * This check is unnecessary if we're currently in the main worktree, > * as prior checks already consulted all configs of the current worktree. > */ > (!worktree->is_current && is_main_worktree_bare(the_repository)); And this more extended version would have helped me by not having to ask Is "this worktree does not have is_current bit set" equivalent to "this worktree is the main one, so is_main_worktree_bare() needs to be consulted"? That linkage between "the is_current bit unset" and "is the main worktree" is not obvious to me. in the first place. In short, both should work, and I personally find that the latter may be a bit more helpful to readers. THanks.
On Tue, Feb 4, 2025 at 12:43 PM Junio C Hamano <gitster@pobox.com> wrote: > > Olga Pilipenco <olga.pilipenco@shopify.com> writes: > > > I have 2 versions for comment: > > > > 1. Since is_main_worktree_bare explains quite well what it does we can have > > a shorter explanation of `!worktree->is_current` part, something like: > > > > /* Additional checks are needed if main worktree is not current > > (checking from secondary worktree) */ > > (!worktree->is_current && is_main_worktree_bare(the_repository)); > > For somebody who thought about the issue themselves (like me, before > writing the message you are responding to), this shorter form would > suffice. I'd rephrase it more like so > > /* When a secondary worktree, an extra check is needed */ > > for brevity, though. > > > > 2. Or a bit longer inline explanation that partially repeats the > > explanation of is_main_worktree_bare > > + adds explanation about efficiency: > > /* > > * When in a secondary worktree we have to also verify if the main worktree > > * is bare in $commondir/config.worktree. > > * This check is unnecessary if we're currently in the main worktree, > > * as prior checks already consulted all configs of the current worktree. > > */ > > (!worktree->is_current && is_main_worktree_bare(the_repository)); > > And this more extended version would have helped me by not having to > ask > > Is "this worktree does not have is_current bit set" equivalent > to "this worktree is the main one, so is_main_worktree_bare() > needs to be consulted"? That linkage between "the is_current > bit unset" and "is the main worktree" is not obvious to me. > > in the first place. > > In short, both should work, and I personally find that the latter > may be a bit more helpful to readers. > > THanks. Perfect, I'll add the latter one then. Thank you!
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index a3a21c54cf6..f3e720dc10d 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -410,6 +410,20 @@ test_expect_success 'bare main worktree has HEAD at branch deleted by secondary git -C secondary branch -D main ' +test_expect_success 'secondary worktrees recognize core.bare=true in main config.worktree' ' + test_when_finished "rm -rf bare_repo non_bare_repo secondary_worktree" && + git init -b main non_bare_repo && + test_commit -C non_bare_repo x && + + git clone --bare non_bare_repo bare_repo && + git -C bare_repo config extensions.worktreeConfig true && + git -C bare_repo config unset core.bare && + git -C bare_repo config --worktree core.bare true && + + git -C bare_repo worktree add ../secondary_worktree && + git -C secondary_worktree checkout main +' + test_expect_success 'git branch --list -v with --abbrev' ' test_when_finished "git branch -D t" && git branch t && diff --git a/worktree.c b/worktree.c index 248bbb39d43..6df4ccf97f7 100644 --- a/worktree.c +++ b/worktree.c @@ -65,6 +65,28 @@ static int is_current_worktree(struct worktree *wt) return is_current; } +/* +* 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_main_worktree_bare(struct repository *repo) +{ + int bare = 0; + struct config_set cs = {0}; + char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo)); + + git_configset_init(&cs); + git_configset_add_file(&cs, worktree_config); + git_configset_get_bool(&cs, "core.bare", &bare); + + git_configset_clear(&cs); + free(worktree_config); + return bare; +} + /** * get the main worktree */ @@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head) CALLOC_ARRAY(worktree, 1); worktree->repo = the_repository; worktree->path = strbuf_detach(&worktree_path, NULL); - /* - * NEEDSWORK: If this function is called from a secondary worktree and - * config.worktree is present, is_bare_repository_cfg will reflect the - * contents of config.worktree, not the contents of the main worktree. - * This means that worktree->is_bare may be set to 0 even if the main - * worktree is configured to be bare. - */ - worktree->is_bare = (is_bare_repository_cfg == 1) || - is_bare_repository(); worktree->is_current = is_current_worktree(worktree); + worktree->is_bare = (is_bare_repository_cfg == 1) || + is_bare_repository() || + (!worktree->is_current && is_main_worktree_bare(the_repository)); + if (!skip_reading_head) add_head_info(worktree); return worktree;