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

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

[Sean Allred]

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

Best,
-Sean

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

[rsbecker]

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

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

[Sean Allred]

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

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


-- 
-Sean

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

[rsbecker]

On December 18, 2021 2:01 PM: Sean Allred wrote:
> 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 agree that this interpretation *may* be incorrect. Worktreeconfig allows a configuration associated with worktrees but does not mean that there is one. It seems like worktreeconfig=true is causing git to check the worktree-specific configuration, finds out that you are in a worktree but there is in fact no worktree configuration specified, so the main repo is checked, which is bare, so reports true. When worktreeconfig=false, it looks like a quick decision is made that because you are in a worktree, obviously you are not bare (this may be an incorrect interpretation). I can somewhat see both sides of this. Perhaps some clarification on the interpretation is required.

It does seem like is_bare_repository_cfg is false in is_bare_repository, which seems to be wrong in context. However, there is a strange comparison in worktree.c that bothers me - is_bare_repository_cfg == 1 around line 67 which is a numeric comparison to a boolean. I think that may be incorrect. There is a NEEDSWORK comment in the code immediately before that line.

Hoping someone else can chime in on this.

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

The essential part of create a worktree from a bare repo still makes sense to me.

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

[Eric Sunshine]

On 12/18/21 11:46 AM, Sean Allred wrote:
> 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

As far as I can tell, this is working as designed and as documented[1]. 
`git rev-parse --is-bare-repository` is supposed to return "true" in the 
bare repository, and "false" in a worktree. 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`.

When `extensions.worktreeconfig` is false, --is-bare-repository special 
cases `core.bare` to return "false" in a worktree regardless of the 
setting in git.git/config. When `extensions.worktreeconfig` is true, 
however, that special-casing behavior is disabled, and 
--is-bare-repository returns whatever it pulls from the configuration. 
In this case, it returned the value of `core.bare` from git.git/config 
since you hadn't moved `core.bare=true` to git.git/worktree.config, 
which would be local only to the bare repository, and not seen by any 
worktrees.

[1]: https://git-scm.com/docs/git-worktree#_configuration_file

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

Regarding #1, creating worktrees from a bare repository is an 
explicitly-supported use-case, and we've heard several times on the 
mailing list from people who take advantage of the ability in automation 
situations (often in CI setups), as well as from people who use it as a 
part of their daily workflow (in which they don't consider any 
particular worktree to be "blessed").

Regarding #2, --is-bare-repository is not so much being fooled as it is 
being lied to by your configuration which wasn't updated to move 
`core.bare=true` to git.git/worktree.config. (Granted, it's painful that 
it's your responsibility to move the setting manually; automating would 
avoid this sort of confusion.)

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

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

[Sean Allred]

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

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

[rsbecker]

On December 19, 2021 3:47 PM, Sean Allred wrote:
> 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 :-)

Fair enough, but 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.
-Randall

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

[Sean Allred]

> 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?

---

Or maybe it makes more sense to call set_git_work_tree() somewhere
sensible such that get_git_work_tree() will return non-null.  Doing a
deeper dive into the code, that might be the *real* root cause
(assuming the_repository->worktree is NULL in this scenario; I have
not run git through a debugger).  Such a change could have
farther-reaching consequences, though.

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

[rsbecker]

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,
> 
>     @@ -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?

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.

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

[Eric Sunshine]

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/

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

[Eric Sunshine]

On Sun, Dec 19, 2021 at 6:30 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

Thinking on it a bit more and re-reading jtan's commit message[1], it
seems that it can be -1 at this point if `core.bare` is not set in
configuration, as indicated at the end of his commit message:

   In order to avoid that, also check core.bare when setting is_bare. If
   core.bare=1, trust it, and otherwise, use is_bare_repository().

It does make me wonder if the code should have been:

    if (is_bare_repository_cfg > 0)
        worktree->is_bare = 1;
    else if (is_bare_repository_cfg < 0)
        worktree->is_bare = is_bare_repository();

in order to respect `core.bare=0`, which the existing code doesn't
seem to do, but perhaps I'm misunderstanding the case he was trying to
solve related to submodules. Anyhow, I think that's all a tangent from
the original issue raised in this thread by Sean (though I could be
wrong about that too).

[1]: https://lore.kernel.org/git/20190419172128.130170-1-jonathantanmy@google.com/

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

[rsbecker]

On December 19, 2021 6:30 PM, Eric Sunshine wrote:
> 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/

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?
-Randall

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

[Eric Sunshine]

On Sun, Dec 19, 2021 at 6:54 PM <rsbecker@nexbridge.com> wrote:
> 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?

I'm not sure how much value an enum would add in this particular case
since this is a quite common idiom in this codebase. It might be
sufficient merely to add a comment to its declaration in cache.h
explaining its possible values.

What could be even more helpful is some documentation about the entire
setup sequence and how all the different "is bare" and "do we have a
worktree" computations interact since the general setup logic is quite
involved, and there are likely a number of pitfalls which are not
obvious by merely glancing at the code, but which people know about
who have dealt extensively with this code (such as Duy and probably
Peff).

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

[Eric Sunshine]

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.

What I could see, perhaps, is a new git-worktree subcommand which does
all the necessary bookkeeping required when switching between
worktree-specific configuration and the legacy configuration model.
For the sake of example, calling this fictitious command "manage",
then we might have "git worktree manage --enable-worktree-config"
which both sets `extensions.worktreeconfig=true` _and_ moves
`core.bare` and `core.worktree` from .git/config to
.git/worktree.config if those config keys exist. The fictitious "git
worktree manage --disable-worktree-config" would reverse the operation
by moving all of .git/worktree.config into .git/config and disabling
`extensions.worktreeconfig`. However, there's a question of what to do
with the worktree-specific worktree.config files when disabling
`extensions.worktreeconfig`. Should they become orphaned or should
they somehow be incorporated into .git/config. Or, perhaps, there
shouldn't even be a --disable-worktree-config switch since a human
probably needs to make decisions about merging the worktree-specific
worktree.config files; or maybe --disable-worktree-config should exist
but error out if it can't figure out how to automatically deal with
the worktree-specific worktree.config files. Anyhow, this is not a
simple solution to the immediate problem and needs a lot more thought.

Another possibility is coming up with a migration plan to eventually
make worktree-specific configuration the default, and eventually drop
support for the legacy configuration model. I don't know if Duy had
any such migration plan in mind when he implemented worktree-specific
configuration, but it seems a reasonable end goal. However, although
that would solve this problem in the long run (since `core.bare` would
be local to the repository and not bleed into worktrees), it doesn't
help in the short term since any migration plan is likely to be
years-long.

>   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've cc:'d a few sparse checkout area experts, though I may have
forgotten some.)

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

[Derrick Stolee]

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.

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

Right, 'init' will set the sparse-checkout information in your worktree
config and initialize it as needed, here:

static int set_config(enum sparse_checkout_mode mode)
{
	const char *config_path;

	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;
	}

	config_path = git_path("config.worktree");
	git_config_set_in_file_gently(config_path,
				      "core.sparseCheckout",
				      mode ? "true" : NULL);

	git_config_set_in_file_gently(config_path,
				      "core.sparseCheckoutCone",
				      mode == MODE_CONE_PATTERNS ? "true" : NULL);

	if (mode == MODE_NO_PATTERNS)
		set_sparse_index_config(the_repository, 0);

	return 0;
}

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.

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

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.

I'll take a stab at this today.

> (I've cc:'d a few sparse checkout area experts, though I may have
> forgotten some.)

Thank you for CC'ing me!
-Stolee

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

[Eric Sunshine]

`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:
> >>> 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 suppose one of Duy's motivations when adding worktree-specific
configuration -- and, importantly, configuration specific to the main
worktree -- was to get rid of the ugly special cases (such as
hard-coding overrides for `core.bare` and `core.worktree`) required by
the original (mis-)design. Importantly, those special-cases need to be
implemented by all third-party tools too, so it's not a winning
approach, whereas worktree-specific configuration gets by without
special cases and could easily be implemented by foreign tools. In the
long run, rather than re-introducing such overrides, a better solution
probably would be to make worktree-specific configuration the default,
but that's a decision for some other day.

> >> 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?

Anyhow, this does bring up a good point and it makes me wonder if
git-sparse-checkout (or whatever helper function is implemented)
should warn the user that setting this extension could break foreign
tools and that the repository format is being upgraded.

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

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

[Derrick Stolee]

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

Ah. I put my change in config.[hc], but let's discuss that in the
patch series [1].

[1] https://lore.kernel.org/git/pull.1101.git.1640015844.gitgitgadget@gmail.com

>>>> 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?

I mean when initializing the config.worktree file.

> Anyhow, this does bring up a good point and it makes me wonder if
> git-sparse-checkout (or whatever helper function is implemented)
> should warn the user that setting this extension could break foreign
> tools and that the repository format is being upgraded.

Yes, this was a concern when doing this change. I at least have not
seen anyone complain about it. This is not critical to the sparse-checkout
feature, but is currently how the sparse-checkout builtin works, so is
part of the paved path for most users getting started.

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

Sorry I either misread or missed the part about moving core.bare over
to core.worktree.

Putting this in the helper that writes to config.worktree is the best
location, because we don't want to force it during 'git worktree add'
since that would cause compatibility issues for tools that don't
understand extensions.worktreeConfig.

Thanks for diligence in communicating what I missed.

-Stolee

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

[Eric Sunshine]

On Mon, Dec 20, 2021 at 12:29 PM Derrick Stolee <stolee@gmail.com> wrote:
> 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].

My concern and sole reason for bringing it up is that this new
function (which should be generally useful) should not end up in
builtin/sparse-checkout.c. I had suggested worktree.c because its
functionality is closely related to worktrees, however, since config.c
has intimate knowledge of the location of worktree configuration, that
also is a reasonable home for the new function. Either location should
be fine; I don't feel strongly either way and don't think it needs a
lot (or any) discussion.

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

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

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

Stepping back a bit, those who want to have two new worktrees
attached to a single bare repository justify the wish by saying that
neither of these two new worktrees should be the primary thing that
they can lose to make the other inoperable, and having a dedicated
"shared object and ref store" repository makes it more symmetric and
safer by making it obvious which one is the precious thing to keep.

Wanting to create two new "bare repository lookalike" attached to a
single bare repository might be defensible the same way.

Not that the current "git worktree" has readily-available features
to create such a layout.  If people who have worked on "worktree"
did not see the possibility of needing such a layout, it is
understandable that such features wouldn't have been designed yet.

Also not that I think that such a layout necessarily makes sense.