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

[<rsbecker@nexbridge.com>]

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