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>
Subject: Re: Bug report - Can create worktrees from bare repo / such worktrees can fool is_bare_repository()
Date: Sun, 19 Dec 2021 18:30:05 -0500 [thread overview]
Message-ID: <CAPig+cTwmBViAGELF69mkYNoz+S-YW5-dcYuNfGeF-wob-=v9Q@mail.gmail.com> (raw)
In-Reply-To: <004001d7f52a$f0367010$d0a35030$@nexbridge.com>
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/
next prev parent reply other threads:[~2021-12-19 23:30 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-18 16:46 Bug report - Can create worktrees from bare repo / such worktrees can fool is_bare_repository() Sean Allred
2021-12-18 17:47 ` rsbecker
2021-12-18 19:00 ` Sean Allred
2021-12-18 21:55 ` rsbecker
2021-12-19 20:16 ` Eric Sunshine
2021-12-19 20:46 ` Sean Allred
2021-12-19 21:32 ` rsbecker
2021-12-19 22:23 ` Sean Allred
2021-12-19 22:51 ` rsbecker
2021-12-19 23:30 ` Eric Sunshine [this message]
2021-12-19 23:45 ` Eric Sunshine
2021-12-19 23:54 ` rsbecker
2021-12-20 0:07 ` Eric Sunshine
2021-12-20 0:58 ` Eric Sunshine
2021-12-20 14:11 ` Derrick Stolee
2021-12-20 15:58 ` Eric Sunshine
2021-12-20 17:29 ` Derrick Stolee
2021-12-20 21:58 ` Eric Sunshine
2021-12-20 16:20 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cTwmBViAGELF69mkYNoz+S-YW5-dcYuNfGeF-wob-=v9Q@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=allred.sean@gmail.com \
--cc=git@vger.kernel.org \
--cc=rsbecker@nexbridge.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).