Bug report - Can create worktrees from bare repo / such worktrees can fool is_bare_repository()

> Hi folks! See the following bug report. Let me know if anything is unclear -- in
> all honesty, I neglectfully `git worktree remove --force`'d the first one I
> wrote...
> 
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
> 
>     ~$ git clone --bare https://github.com/git/git.git
>     ---clip---
> 
>     ~/gitbare$ git config --list --show-origin
>     file:config     core.repositoryformatversion=1
>     file:config     core.filemode=false
>     file:config     core.bare=true
>     file:config     core.ignorecase=true
>     file:config     remote.origin.url=https://github.com/git/git.git
> 
>     ~/gitbare$ git worktree add --no-checkout ../next
>     Preparing worktree (checking out 'next')
> 
>     ~/gitbare$ git config --list --show-origin
>     file:config     core.repositoryformatversion=1
>     file:config     core.filemode=false
>     file:config     core.bare=true
>     file:config     core.ignorecase=true
>     file:config     remote.origin.url=https://github.com/git/git.git
> 
>     ~/gitbare$ cd ../next/
> 
>     ~/next$ git config --list --show-origin
>     file:../gitbare/config    core.repositoryformatversion=1
>     file:../gitbare/config    core.filemode=false
>     file:../gitbare/config    core.bare=true
>     file:../gitbare/config    core.ignorecase=true
>     file:../gitbare/config    remote.origin.url=https://github.com/git/git.git
> 
>     ~/next$ git rev-parse --is-bare-repository
>     false
> 
>     ~/next$ git config extensions.worktreeconfig true
>     ~/next$ git rev-parse --is-bare-repository
>     true
> 
>     ~/next$ git config --unset extensions.worktreeconfig
>     ~/next$ git rev-parse --is-bare-repository
>     false
> 
> I actually found this situation (and narrowed it to the above) by trying to
> perform a sparse-checkout in the worktree.  It appears sparse-checkout by
> default uses a worktree-specific config (which does make sense).
> 
> What did you expect to happen? (Expected behavior)
> 
>     I expected one of the following to happen:
> 
>     1. I should have been blocked from creating a worktree from a bare
>     repository.
> 
>     2. is_bare_repository() shouldn't be fooled by this situation,
>     assuming it's valid.
> 
>     All things being equal, I would more expect to have been blocked
>     from creating a worktree from a bare repository.  Personally, this
>     bare repo + worktree setup doesn't not align with my experience so
>     far with how bare repos are normally used (i.e., as a convenience
>     for centralized remotes that will never be doing a checkout).
> 
> What happened instead? (Actual behavior)
> 
>     is_bare_repository() is fooled and I'm prevented from performing
>     any operation that requires a worktree (like performing a sparse
>     checkout).
> 
> What's different between what you expected and what actually happened?
> 
>     is_bare_repository() is fooled into thinking the worktree is not a
>     worktree / I'm able to create a worktree from a bare repo.
> 
> Anything else you want to add:
> 
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
> 
> 
> [System Info]
> git version:
> git version 2.34.1
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> uname: Linux 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43
> UTC 2020 x86_64 compiler info: gnuc: 9.3 libc info: glibc: 2.31 $SHELL (typically,
> interactive shell): /bin/bash
> 
> 
> [Enabled Hooks]
> not run from a git repository - no hooks to show

>
> On December 18, 2021 11:47 AM, Sean Allred wrote:
> > Hi folks! See the following bug report. Let me know if anything is unclear -- in
> > all honesty, I neglectfully `git worktree remove --force`'d the first one I
> > wrote...
> >
> > Thank you for filling out a Git bug report!
> > Please answer the following questions to help us understand your issue.
> >
> > What did you do before the bug happened? (Steps to reproduce your issue)
> >
> >     ~$ git clone --bare https://github.com/git/git.git
> >     ---clip---
> >
> >     ~/gitbare$ git config --list --show-origin
> >     file:config     core.repositoryformatversion=1
> >     file:config     core.filemode=false
> >     file:config     core.bare=true
> >     file:config     core.ignorecase=true
> >     file:config     remote.origin.url=https://github.com/git/git.git
> >
> >     ~/gitbare$ git worktree add --no-checkout ../next
> >     Preparing worktree (checking out 'next')
> >
> >     ~/gitbare$ git config --list --show-origin
> >     file:config     core.repositoryformatversion=1
> >     file:config     core.filemode=false
> >     file:config     core.bare=true
> >     file:config     core.ignorecase=true
> >     file:config     remote.origin.url=https://github.com/git/git.git
> >
> >     ~/gitbare$ cd ../next/
> >
> >     ~/next$ git config --list --show-origin
> >     file:../gitbare/config    core.repositoryformatversion=1
> >     file:../gitbare/config    core.filemode=false
> >     file:../gitbare/config    core.bare=true
> >     file:../gitbare/config    core.ignorecase=true
> >     file:../gitbare/config    remote.origin.url=https://github.com/git/git.git
> >
> >     ~/next$ git rev-parse --is-bare-repository
> >     false
> >
> >     ~/next$ git config extensions.worktreeconfig true
> >     ~/next$ git rev-parse --is-bare-repository
> >     true
> >
> >     ~/next$ git config --unset extensions.worktreeconfig
> >     ~/next$ git rev-parse --is-bare-repository
> >     false
> >
> > I actually found this situation (and narrowed it to the above) by trying to
> > perform a sparse-checkout in the worktree.  It appears sparse-checkout by
> > default uses a worktree-specific config (which does make sense).
> >
> > What did you expect to happen? (Expected behavior)
> >
> >     I expected one of the following to happen:
> >
> >     1. I should have been blocked from creating a worktree from a bare
> >     repository.
> >
> >     2. is_bare_repository() shouldn't be fooled by this situation,
> >     assuming it's valid.
> >
> >     All things being equal, I would more expect to have been blocked
> >     from creating a worktree from a bare repository.  Personally, this
> >     bare repo + worktree setup doesn't not align with my experience so
> >     far with how bare repos are normally used (i.e., as a convenience
> >     for centralized remotes that will never be doing a checkout).
> >
> > What happened instead? (Actual behavior)
> >
> >     is_bare_repository() is fooled and I'm prevented from performing
> >     any operation that requires a worktree (like performing a sparse
> >     checkout).
> >
> > What's different between what you expected and what actually happened?
> >
> >     is_bare_repository() is fooled into thinking the worktree is not a
> >     worktree / I'm able to create a worktree from a bare repo.
> >
> > Anything else you want to add:
> >
> > Please review the rest of the bug report below.
> > You can delete any lines you don't wish to share.
> >
> >
> > [System Info]
> > git version:
> > git version 2.34.1
> > cpu: x86_64
> > no commit associated with this build
> > sizeof-long: 8
> > sizeof-size_t: 8
> > shell-path: /bin/sh
> > uname: Linux 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43
> > UTC 2020 x86_64 compiler info: gnuc: 9.3 libc info: glibc: 2.31 $SHELL (typically,
> > interactive shell): /bin/bash
> >
> >
> > [Enabled Hooks]
> > not run from a git repository - no hooks to show
>
> My thoughts:
>
> 1. I think it is legitimate to create a worktree from a bare repository. The worktree is using its own working directory/index and does not require anything from the bare repo.
> 2. You ran is_bare_repository from next, which was in your worktree - not a bare repo, so that answer actually makes sense.
>
> I'm not sure whether this is an expected use case but it does make sense to be one. If you typically work in worktrees and write scripts under that assumption, not having to worry about being in the non-worktree part of a clone makes sense. So creating a worktree off a bare repo is not a bad thing, assuming everything else is correct.
>
> Just my $0.02,
> -Randall
>

> On Sat, Dec 18, 2021 at 11:47 AM <rsbecker@nexbridge.com> wrote:
> >
> > On December 18, 2021 11:47 AM, Sean Allred wrote:
> > > Hi folks! See the following bug report. Let me know if anything is
> > > unclear -- in all honesty, I neglectfully `git worktree remove
> > > --force`'d the first one I wrote...
> > >
> > > Thank you for filling out a Git bug report!
> > > Please answer the following questions to help us understand your issue.
> > >
> > > What did you do before the bug happened? (Steps to reproduce your
> > > issue)
> > >
> > >     ~$ git clone --bare https://github.com/git/git.git
> > >     ---clip---
> > >
> > >     ~/gitbare$ git config --list --show-origin
> > >     file:config     core.repositoryformatversion=1
> > >     file:config     core.filemode=false
> > >     file:config     core.bare=true
> > >     file:config     core.ignorecase=true
> > >     file:config     remote.origin.url=https://github.com/git/git.git
> > >
> > >     ~/gitbare$ git worktree add --no-checkout ../next
> > >     Preparing worktree (checking out 'next')
> > >
> > >     ~/gitbare$ git config --list --show-origin
> > >     file:config     core.repositoryformatversion=1
> > >     file:config     core.filemode=false
> > >     file:config     core.bare=true
> > >     file:config     core.ignorecase=true
> > >     file:config     remote.origin.url=https://github.com/git/git.git
> > >
> > >     ~/gitbare$ cd ../next/
> > >
> > >     ~/next$ git config --list --show-origin
> > >     file:../gitbare/config    core.repositoryformatversion=1
> > >     file:../gitbare/config    core.filemode=false
> > >     file:../gitbare/config    core.bare=true
> > >     file:../gitbare/config    core.ignorecase=true
> > >     file:../gitbare/config    remote.origin.url=https://github.com/git/git.git
> > >
> > >     ~/next$ git rev-parse --is-bare-repository
> > >     false
> > >
> > >     ~/next$ git config extensions.worktreeconfig true
> > >     ~/next$ git rev-parse --is-bare-repository
> > >     true
> > >
> > >     ~/next$ git config --unset extensions.worktreeconfig
> > >     ~/next$ git rev-parse --is-bare-repository
> > >     false
> > >
> > > I actually found this situation (and narrowed it to the above) by
> > > trying to perform a sparse-checkout in the worktree.  It appears
> > > sparse-checkout by default uses a worktree-specific config (which does
> make sense).
> > >
> > > What did you expect to happen? (Expected behavior)
> > >
> > >     I expected one of the following to happen:
> > >
> > >     1. I should have been blocked from creating a worktree from a bare
> > >     repository.
> > >
> > >     2. is_bare_repository() shouldn't be fooled by this situation,
> > >     assuming it's valid.
> > >
> > >     All things being equal, I would more expect to have been blocked
> > >     from creating a worktree from a bare repository.  Personally, this
> > >     bare repo + worktree setup doesn't not align with my experience so
> > >     far with how bare repos are normally used (i.e., as a convenience
> > >     for centralized remotes that will never be doing a checkout).
> > >
> > > What happened instead? (Actual behavior)
> > >
> > >     is_bare_repository() is fooled and I'm prevented from performing
> > >     any operation that requires a worktree (like performing a sparse
> > >     checkout).
> > >
> > > What's different between what you expected and what actually
> happened?
> > >
> > >     is_bare_repository() is fooled into thinking the worktree is not a
> > >     worktree / I'm able to create a worktree from a bare repo.
> > >
> > > Anything else you want to add:
> > >
> > > Please review the rest of the bug report below.
> > > You can delete any lines you don't wish to share.
> > >
> > >
> > > [System Info]
> > > git version:
> > > git version 2.34.1
> > > cpu: x86_64
> > > no commit associated with this build
> > > sizeof-long: 8
> > > sizeof-size_t: 8
> > > shell-path: /bin/sh
> > > uname: Linux 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28
> > > 23:40:43 UTC 2020 x86_64 compiler info: gnuc: 9.3 libc info: glibc:
> > > 2.31 $SHELL (typically, interactive shell): /bin/bash
> > >
> > >
> > > [Enabled Hooks]
> > > not run from a git repository - no hooks to show
> >
> > My thoughts:
> >
> > 1. I think it is legitimate to create a worktree from a bare repository. The
> worktree is using its own working directory/index and does not require
> anything from the bare repo.
> > 2. You ran is_bare_repository from next, which was in your worktree - not
> a bare repo, so that answer actually makes sense.

> I'm not sure I follow. I did run is_bare_repository from the next-worktree,
> but the return value was evidently dependent on the value of
> extensions.worktreeconfig. When true, is_bare_repository returned true --
> even within the next-worktree. Unless I'm missing something fairly
> fundamental here...

> > I'm not sure whether this is an expected use case but it does make sense
> > to be one. If you typically work in worktrees and write scripts under that
> > assumption, not having to worry about being in the non-worktree part of a
> > clone makes sense. So creating a worktree off a bare repo is not a bad thing,
> > assuming everything else is correct.

> What did you do before the bug happened? (Steps to reproduce your issue)
> 
>      ~$ git clone --bare https://github.com/git/git.git
>      ~/gitbare$ git config --list --show-origin
>      file:config     core.bare=true
>      ~/gitbare$ git worktree add --no-checkout ../next
>      Preparing worktree (checking out 'next')
>      ~/gitbare$ git config --list --show-origin
>      file:config     core.bare=true
>      ~/gitbare$ cd ../next/
>      ~/next$ git config --list --show-origin
>      file:../gitbare/config    core.bare=true
>      ~/next$ git rev-parse --is-bare-repository
>      false
>      ~/next$ git config extensions.worktreeconfig true
>      ~/next$ git rev-parse --is-bare-repository
>      true
>      ~/next$ git config --unset extensions.worktreeconfig
>      ~/next$ git rev-parse --is-bare-repository
>      false

> I actually found this situation (and narrowed it to the above) by
> trying to perform a sparse-checkout in the worktree.  It appears
> sparse-checkout by default uses a worktree-specific config (which
> does make sense).
> 
> What did you expect to happen? (Expected behavior)
> 
>      I expected one of the following to happen:
> 
>      1. I should have been blocked from creating a worktree from a bare
>      repository.
> 
>      2. is_bare_repository() shouldn't be fooled by this situation,
>      assuming it's valid.
> 
>      All things being equal, I would more expect to have been blocked
>      from creating a worktree from a bare repository.  Personally, this
>      bare repo + worktree setup doesn't not align with my experience so
>      far with how bare repos are normally used (i.e., as a convenience
>      for centralized remotes that will never be doing a checkout).

> What happened instead? (Actual behavior)
> 
>      is_bare_repository() is fooled and I'm prevented from performing
>      any operation that requires a worktree (like performing a sparse
>      checkout).
> 
> What's different between what you expected and what actually happened?
> 
>      is_bare_repository() is fooled into thinking the worktree is not a
>      worktree / I'm able to create a worktree from a bare repo.

> To: Eric Sunshine <sunshine@sunshineco.com>
> Cc: git@vger.kernel.org
> Subject: Re: Bug report - Can create worktrees from bare repo / such
> worktrees can fool is_bare_repository()
> 
> > However, you missed the step (discussed in [1]) in which it is your
> > responsibility to move the `core.bare=true` setting from
> > git.git/config to git.git/worktree.config manually after setting
> > `extensions.worktreeconfig=true`.
> 
> Ahh, that makes sense!  I did notice the `core.bare` setting being respected
> in source and figured this had a part to play (which is why I included git-config
> output).
> 
> I think then that I was overzealous in trying to MWE-ify the issue: as I noted, I
> found this issue when I was trying to perform a sparse-checkout within the
> worktree.  To memory (I don't have my work system at the moment and
> don't have its `history`), I think it went something like this:
> 
>     git worktree add --no-checkout ../next && cd ../next
>     git sparse-checkout init --cone # auto-created a worktree config
>     git sparse-checkout set t
> 
> I think either the git-sparse-checkout-set command (or the git-checkout I ran
> after) would fail complaining that I was not in a worktree.  Based on the
> above, it sounds like `init` is creating the worktree-specific config, but is not
> overriding `core.bare` in that config.  Would a patch to take this step this
> automatically be well-received?  I see two options for when to set
> `core.bare=false` in worktree-specific config:
> 
>   1. At git-worktree-add: This is probably the earliest time which
>      makes sense, but may be over-reach.  I'm not up-to-speed on how
>      worktree-specific configs are generally considered on this list.
>      If I were implementing a workaround, though, this is probably
>      where I'd make it.
> 
>   2. At git-sparse-checkout-init: This is where the problem begins to
>      have an effect, so this might also make sense.
> 
> I'm glad to learn about bare repositories + worktrees being a supported use-
> case :-)

> > what about the comparison code where is_bare_repository_cfg is
> > compared with 1 (it is a boolean and sometimes set to -1). This would
> > not generally pass a code review.
> 
> I'm sorry, I'm afraid I don't completely follow.  Wouldn't the most
> straightforward change be to simply follow the documented
> recommendation when we create the worktree config in `git sparse-
> checkout init`?  Specifically,
> 
>     @@ -374,6 +374,9 @@ static int set_config(enum sparse_checkout_mode
> mode)
>                            "core.sparseCheckoutCone",
>                            mode == MODE_CONE_PATTERNS ? "true" : NULL);
> 
>     +    if (is_bare_repository())
>     +      git_config_set_in_file_gently(config_path, "core.bare", "false");
>     +
>          if (mode == MODE_NO_PATTERNS)
>              set_sparse_index_config(the_repository, 0);
> 
> Are we saying the comparison within is_bare_repository() may not be
> appropriate in this case?

> On December 19, 2021 5:23 PM, Sean Allred wrote:
> > > what about the comparison code where is_bare_repository_cfg is
> > > compared with 1 (it is a boolean and sometimes set to -1). This would
> > > not generally pass a code review.
> >
> > I'm sorry, I'm afraid I don't completely follow.  Wouldn't the most
> > straightforward change be to simply follow the documented
> > recommendation when we create the worktree config in `git sparse-
> > checkout init`?  Specifically,
> >
> >     +    if (is_bare_repository())
> >     +      git_config_set_in_file_gently(config_path, "core.bare", "false");
> >     +
> >
> > Are we saying the comparison within is_bare_repository() may not be
> > appropriate in this case?
>
> I'm suggesting that:
>
>         worktree->is_bare = (is_bare_repository_cfg == 1) ||
>                 is_bare_repository();
>
> the == 1 comparison should not be done for boolean-style variables. It is an int, but initialized to -1. Unless -1 and 1 mean different things, but that is not really documented.

> On Sun, Dec 19, 2021 at 5:51 PM <rsbecker@nexbridge.com> wrote:
> >         worktree->is_bare = (is_bare_repository_cfg == 1) ||
> >                 is_bare_repository();
> >
> > the == 1 comparison should not be done for boolean-style variables. It is an int, but initialized to -1. Unless -1 and 1 mean different things, but that is not really documented.
>
> `is_bare_repository_cfg` is not exactly a boolean; it's a tristate,
> with -1 meaning "not yet determined". I didn't, at the time, closely
> follow the discussion[1] of the particular bit of code you're
> questioning, but the `== 1` was mentioned at least a couple times,
> once in review by Junio[2], and then in the extra patch commentary by
> "jtan" when he submitted v2[3]. Anyhow, if I'm following the original
> discussion correctly, then the usage, `== 1` (or the equivalent `> 0`)
> is probably correct, and that treating it as a simple boolean (where
> -1 is true, too) would be undesirable. (Of course, I haven't traced
> through the init code at all, so I don't even know if it can ever be
> -1 at this point.) Five existing consumers of this global variable use
> `== 1`, and only two use `> 0`, so this usage is at least reasonably
> consistent with other parts of the project.

> On Sun, Dec 19, 2021 at 5:51 PM <rsbecker@nexbridge.com> wrote:
> > On December 19, 2021 5:23 PM, Sean Allred wrote:
> > > > what about the comparison code where is_bare_repository_cfg is
> > > > compared with 1 (it is a boolean and sometimes set to -1). This
> > > > would not generally pass a code review.
> > >
> > > I'm sorry, I'm afraid I don't completely follow.  Wouldn't the most
> > > straightforward change be to simply follow the documented
> > > recommendation when we create the worktree config in `git sparse-
> > > checkout init`?  Specifically,
> > >
> > >     +    if (is_bare_repository())
> > >     +      git_config_set_in_file_gently(config_path, "core.bare", "false");
> > >     +
> > >
> > > Are we saying the comparison within is_bare_repository() may not be
> > > appropriate in this case?
> >
> > I'm suggesting that:
> >
> >         worktree->is_bare = (is_bare_repository_cfg == 1) ||
> >                 is_bare_repository();
> >
> > the == 1 comparison should not be done for boolean-style variables. It is an
> int, but initialized to -1. Unless -1 and 1 mean different things, but that is not
> really documented.
> 
> `is_bare_repository_cfg` is not exactly a boolean; it's a tristate, with -1
> meaning "not yet determined". I didn't, at the time, closely follow the
> discussion[1] of the particular bit of code you're questioning, but the `== 1`
> was mentioned at least a couple times, once in review by Junio[2], and then
> in the extra patch commentary by "jtan" when he submitted v2[3]. Anyhow,
> if I'm following the original discussion correctly, then the usage, `== 1` (or the
> equivalent `> 0`) is probably correct, and that treating it as a simple boolean
> (where
> -1 is true, too) would be undesirable. (Of course, I haven't traced through the
> init code at all, so I don't even know if it can ever be
> -1 at this point.) Five existing consumers of this global variable use `== 1`, and
> only two use `> 0`, so this usage is at least reasonably consistent with other
> parts of the project.
> 
> [1]: https://lore.kernel.org/git/20190419172128.130170-1-
> jonathantanmy@google.com/T/
> [2]: https://lore.kernel.org/git/xmqqo954gira.fsf@gitster-ct.c.googlers.com/
> [3]: https://lore.kernel.org/git/20190419172128.130170-1-
> jonathantanmy@google.com/

> On December 19, 2021 6:30 PM, Eric Sunshine wrote:
> > `is_bare_repository_cfg` is not exactly a boolean; it's a tristate, with -1
> > meaning "not yet determined". I didn't, at the time, closely follow the
> > discussion[1] of the particular bit of code you're questioning, but the `== 1`
> > was mentioned at least a couple times, once in review by Junio[2], and then
> > in the extra patch commentary by "jtan" when he submitted v2[3]. Anyhow,
> > if I'm following the original discussion correctly, then the usage, `== 1` (or the
> > equivalent `> 0`) is probably correct, and that treating it as a simple boolean
> > (where
> > -1 is true, too) would be undesirable. (Of course, I haven't traced through the
> > init code at all, so I don't even know if it can ever be
> > -1 at this point.) Five existing consumers of this global variable use `== 1`, and
> > only two use `> 0`, so this usage is at least reasonably consistent with other
> > parts of the project.
>
> Thanks for the clarification. It helps to understand the code. Could the variable type be changed to a new typedef like ConfigTriState instead of int to be clear about its semantics? Or perhaps an enum with -1, 0, 1 declared explicitly?

> > However, you missed the step (discussed in [1]) in which it is your
> > responsibility to move the `core.bare=true` setting from
> > git.git/config to git.git/worktree.config manually after setting
> > `extensions.worktreeconfig=true`.
>
> Ahh, that makes sense!  I did notice the `core.bare` setting being
> respected in source and figured this had a part to play (which is why
> I included git-config output).
>
> I think then that I was overzealous in trying to MWE-ify the issue: as
> I noted, I found this issue when I was trying to perform a
> sparse-checkout within the worktree.  To memory (I don't have my work
> system at the moment and don't have its `history`), I think it went
> something like this:
>
>     git worktree add --no-checkout ../next && cd ../next
>     git sparse-checkout init --cone # auto-created a worktree config
>     git sparse-checkout set t

> I see two options for when to set `core.bare=false` in
> worktree-specific config:
>
>   1. At git-worktree-add: This is probably the earliest time which
>      makes sense, but may be over-reach.  I'm not up-to-speed on how
>      worktree-specific configs are generally considered on this list.
>      If I were implementing a workaround, though, this is probably
>      where I'd make it.

> On Sun, Dec 19, 2021 at 3:47 PM Sean Allred <allred.sean@gmail.com> wrote:
>>> However, you missed the step (discussed in [1]) in which it is your
>>> responsibility to move the `core.bare=true` setting from
>>> git.git/config to git.git/worktree.config manually after setting
>>> `extensions.worktreeconfig=true`.

>> Ahh, that makes sense!  I did notice the `core.bare` setting being
>> respected in source and figured this had a part to play (which is why
>> I included git-config output).
>>
>> I think then that I was overzealous in trying to MWE-ify the issue: as
>> I noted, I found this issue when I was trying to perform a
>> sparse-checkout within the worktree.  To memory (I don't have my work
>> system at the moment and don't have its `history`), I think it went
>> something like this:
>>
>>     git worktree add --no-checkout ../next && cd ../next
>>     git sparse-checkout init --cone # auto-created a worktree config
>>     git sparse-checkout set t
> 
> Thanks for this information. I haven't followed sparse-checkout mode
> at all and haven't used it, so I quickly scanned the man page for the
> worktree-relevant information, and indeed I was able to reproduce the
> problem using the recipe (with the prerequisite that the repository is
> bare) which you present here.
> 
>> I think either the git-sparse-checkout-set command (or the
>> git-checkout I ran after) would fail complaining that I was not in a
>> worktree.
> 
> It is indeed the `git sparse-checkout set` command which fails.

>> Based on the above, it sounds like `init` is creating the
>> worktree-specific config, but is not overriding `core.bare` in that
>> config.  Would a patch to take this step this automatically be
>> well-received?
> 
> This looks like a legitimate oversight, so some sort of patch to
> resolve it would likely be welcomed.
> 
>> I see two options for when to set `core.bare=false` in
>> worktree-specific config:
>>
>>   1. At git-worktree-add: This is probably the earliest time which
>>      makes sense, but may be over-reach.  I'm not up-to-speed on how
>>      worktree-specific configs are generally considered on this list.
>>      If I were implementing a workaround, though, this is probably
>>      where I'd make it.
> 
> My knee-jerk reaction is that applying a "fix" to `git worktree add`
> to assign `core.bare=false` in the worktree-specific config may be
> misguided, or at least feels too much like a band-aid. Although it's
> true that that would address the problem for worktrees created after
> `extensions.worktreeconfig=true` is set, it won't help
> already-existing worktrees. This reason alone makes me hesitant to
> endorse this approach.

>>   2. At git-sparse-checkout-init: This is where the problem begins to
>>      have an effect, so this might also make sense.
> 
> Yes, if I'm understanding everything correctly, this seems like the
> best and most localized place to address the problem at this time. The
> simple, but too minimal, approach would be for `git sparse-checkout
> init` to simply add `core.bare=false` to the worktree-specific config,
> however, this only addresses the immediate problem and still leaves
> things broken in general for any non-sparse worktrees.
> 
> So, the better and more correct approach, while still being localized
> to `git sparse-checkout init` is for it to respect the documented
> requirement and automatically move `core.bare` and `core.worktree`
> from .git/config to .git/worktree.config if those keys exist. That
> should leave everything in a nice Kosher state for all worktrees,
> existing and newly-created.

> On 12/19/2021 7:58 PM, Eric Sunshine wrote:
> > On Sun, Dec 19, 2021 at 3:47 PM Sean Allred <allred.sean@gmail.com> wrote:
> >>> However, you missed the step (discussed in [1]) in which it is your
> >>> responsibility to move the `core.bare=true` setting from
> >>> git.git/config to git.git/worktree.config manually after setting
> >>> `extensions.worktreeconfig=true`.
>
> Thanks for this context of added responsibility. It seems a bit strange
> to require this, since it doesn't make any sense to have a bare worktree
> (at least not to me, feel free to elaborate on the need of such a
> situation).
>
> I think the most defensive thing to do would be to always special case
> core.bare to false when in a worktree. But if there is a reason to allow
> bare worktrees, then that isn't feasible.

> >> I think either the git-sparse-checkout-set command (or the
> >> git-checkout I ran after) would fail complaining that I was not in a
> >> worktree.
> >
> > It is indeed the `git sparse-checkout set` command which fails.
>
> Right, 'init' will set the sparse-checkout information in your worktree
> config and initialize it as needed, here:
>
>         if (upgrade_repository_format(1) < 0)
>                 die(_("unable to upgrade repository format to enable worktreeConfig"));
>         if (git_config_set_gently("extensions.worktreeConfig", "true")) {
>                 error(_("failed to set extensions.worktreeConfig setting"));
>                 return 1;
>         }
>
> So, we are manually specifying "put this in the config.worktree file"
> and not going through some "initialize worktree config" helper. Such
> a helper would be useful to avoid this issue in the future.

> >> I see two options for when to set `core.bare=false` in
> >> worktree-specific config:
> >>
> >>   1. At git-worktree-add: This is probably the earliest time which
> >>      makes sense, but may be over-reach.  I'm not up-to-speed on how
> >>      worktree-specific configs are generally considered on this list.
> >>      If I were implementing a workaround, though, this is probably
> >>      where I'd make it.
> >
> > My knee-jerk reaction is that applying a "fix" to `git worktree add`
> > to assign `core.bare=false` in the worktree-specific config may be
> > misguided, or at least feels too much like a band-aid. Although it's
> > true that that would address the problem for worktrees created after
> > `extensions.worktreeconfig=true` is set, it won't help
> > already-existing worktrees. This reason alone makes me hesitant to
> > endorse this approach.
>
> Yeah, my concern is that it requires the extension and could cause
> some tools to stop working immediately. If we have the extension
> already, it might make sense to initialize the file then.

> >>   2. At git-sparse-checkout-init: This is where the problem begins to
> >>      have an effect, so this might also make sense.
> >
> > Yes, if I'm understanding everything correctly, this seems like the
> > best and most localized place to address the problem at this time. The
> > simple, but too minimal, approach would be for `git sparse-checkout
> > init` to simply add `core.bare=false` to the worktree-specific config,
> > however, this only addresses the immediate problem and still leaves
> > things broken in general for any non-sparse worktrees.
> >
> > So, the better and more correct approach, while still being localized
> > to `git sparse-checkout init` is for it to respect the documented
> > requirement and automatically move `core.bare` and `core.worktree`
> > from .git/config to .git/worktree.config if those keys exist. That
> > should leave everything in a nice Kosher state for all worktrees,
> > existing and newly-created.
>
> I agree that this is the only place that currently _writes_ to the
> worktree config. All other code paths that seem to care about the
> worktree config specifically only read from the config using a
> modified scope.
>
> My thought on the direction to go would be to extract some code
> from the set_config() in builtin/sparse-checkout.c into a config
> helper, say "ensure_worktree_config_exists()", that adds the
> extension, creates the file, and then adds core.bare=false.
>
> Even better, we could create a config helper that writes to the
> worktree config, and _that_ could ensure the config is set
> correctly before writing the requested value.

> `On Mon, Dec 20, 2021 at 9:11 AM Derrick Stolee <stolee@gmail.com> wrote:
>> On 12/19/2021 7:58 PM, Eric Sunshine wrote:
>>> On Sun, Dec 19, 2021 at 3:47 PM Sean Allred <allred.sean@gmail.com> wrote:
>>>> I think either the git-sparse-checkout-set command (or the
>>>> git-checkout I ran after) would fail complaining that I was not in a
>>>> worktree.
>>>
>>> It is indeed the `git sparse-checkout set` command which fails.
>>
>> Right, 'init' will set the sparse-checkout information in your worktree
>> config and initialize it as needed, here:
>>
>>         if (upgrade_repository_format(1) < 0)
>>                 die(_("unable to upgrade repository format to enable worktreeConfig"));
>>         if (git_config_set_gently("extensions.worktreeConfig", "true")) {
>>                 error(_("failed to set extensions.worktreeConfig setting"));
>>                 return 1;
>>         }
>>
>> So, we are manually specifying "put this in the config.worktree file"
>> and not going through some "initialize worktree config" helper. Such
>> a helper would be useful to avoid this issue in the future.
> 
> Yes, I was planning to suggest this in a follow-up message.
> Specifically, I think top-level worktree.[hc] (not builtin/worktree.c)
> should publish a function which enables worktree-specific
> configuration _and_ does all the necessary bookkeeping, such as moving
> `core.bare` and `core.worktree` from .git/config to
> .git/worktree.config. That way, not only can git-sparse-checkout take
> advantage of it, but so can any command which needs the functionality
> in the future, as well as the fictitious "git worktree manage" command
> I mentioned earlier if it ever materializes.

>>>> I see two options for when to set `core.bare=false` in
>>>> worktree-specific config:
>>>>
>>>>   1. At git-worktree-add: This is probably the earliest time which
>>>>      makes sense, but may be over-reach.  I'm not up-to-speed on how
>>>>      worktree-specific configs are generally considered on this list.
>>>>      If I were implementing a workaround, though, this is probably
>>>>      where I'd make it.
>>>
>>> My knee-jerk reaction is that applying a "fix" to `git worktree add`
>>> to assign `core.bare=false` in the worktree-specific config may be
>>> misguided, or at least feels too much like a band-aid. Although it's
>>> true that that would address the problem for worktrees created after
>>> `extensions.worktreeconfig=true` is set, it won't help
>>> already-existing worktrees. This reason alone makes me hesitant to
>>> endorse this approach.
>>
>> Yeah, my concern is that it requires the extension and could cause
>> some tools to stop working immediately. If we have the extension
>> already, it might make sense to initialize the file then.
> 
> I'm not following what you're saying. Initialize which file? When?

>>>>   2. At git-sparse-checkout-init: This is where the problem begins to
>>>>      have an effect, so this might also make sense.
>>>
>>> Yes, if I'm understanding everything correctly, this seems like the
>>> best and most localized place to address the problem at this time. The
>>> simple, but too minimal, approach would be for `git sparse-checkout
>>> init` to simply add `core.bare=false` to the worktree-specific config,
>>> however, this only addresses the immediate problem and still leaves
>>> things broken in general for any non-sparse worktrees.
>>>
>>> So, the better and more correct approach, while still being localized
>>> to `git sparse-checkout init` is for it to respect the documented
>>> requirement and automatically move `core.bare` and `core.worktree`
>>> from .git/config to .git/worktree.config if those keys exist. That
>>> should leave everything in a nice Kosher state for all worktrees,
>>> existing and newly-created.
>>
>> I agree that this is the only place that currently _writes_ to the
>> worktree config. All other code paths that seem to care about the
>> worktree config specifically only read from the config using a
>> modified scope.
>>
>> My thought on the direction to go would be to extract some code
>> from the set_config() in builtin/sparse-checkout.c into a config
>> helper, say "ensure_worktree_config_exists()", that adds the
>> extension, creates the file, and then adds core.bare=false.
>>
>> Even better, we could create a config helper that writes to the
>> worktree config, and _that_ could ensure the config is set
>> correctly before writing the requested value.
> 
> Please do not take the approach of setting `core.bare=false` in the
> worktree-specific config file. As I mentioned in the quoted text just
> above, that only resolves the problem for the worktree in question but
> leaves all other potentially worktrees broken (both existing worktrees
> and all new worktrees which are not being used for sparse checkout).
> The _only_ correct thing to do, as far as I can see, is for the new
> helper function to precisely implement the requirements as laid out by
> git-worktree.txt: specifically, enable
> `extensions.worktreeConfig=true` _and_ relocate `core.bare` and
> `core.worktree` from .git/config to .git/worktree.config.

> On 12/20/2021 10:58 AM, Eric Sunshine wrote:
> > `On Mon, Dec 20, 2021 at 9:11 AM Derrick Stolee <stolee@gmail.com> wrote:
> >> So, we are manually specifying "put this in the config.worktree file"
> >> and not going through some "initialize worktree config" helper. Such
> >> a helper would be useful to avoid this issue in the future.
> >
> > Yes, I was planning to suggest this in a follow-up message.
> > Specifically, I think top-level worktree.[hc] (not builtin/worktree.c)
> > should publish a function which enables worktree-specific
> > configuration _and_ does all the necessary bookkeeping, such as moving
> > `core.bare` and `core.worktree` from .git/config to
> > .git/worktree.config. That way, not only can git-sparse-checkout take
> > advantage of it, but so can any command which needs the functionality
> > in the future, as well as the fictitious "git worktree manage" command
> > I mentioned earlier if it ever materializes.
>
> Ah. I put my change in config.[hc], but let's discuss that in the
> patch series [1].