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

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Randall S. Becker" <rsbecker@nexbridge.com>
Cc: Sean Allred <allred.sean@gmail.com>,
	Git List <git@vger.kernel.org>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: Bug report - Can create worktrees from bare repo / such worktrees can fool is_bare_repository()
Date: Sun, 19 Dec 2021 18:45:00 -0500	[thread overview]
Message-ID: <CAPig+cSUT3B1OQLatvfnbyRezWm68AzcdhmkiFhnJ3T5Hs=x_w@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cTwmBViAGELF69mkYNoz+S-YW5-dcYuNfGeF-wob-=v9Q@mail.gmail.com>

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/