Re: [PATCH 01/24] init-db: remove unnecessary global variable & document existing bug

From: Elijah Newren <newren@gmail.com>
Date: May 12, 2023, 01:21 UTC
Message-ID: <CABPp-BFT_aAmMcD7HKVuaZR3kkGND1mBF3R8OvisHimYWiX48g@mail.gmail.com>
In-Reply-To: <kl6lsfc2zjje.fsf@chooglen-macbookpro.roam.corp.google.com>

On Thu, May 11, 2023 at 1:43 PM Glen Choo <chooglen@google.com> wrote:

Show 21 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > This commit was prompted by a desire to move the functions which
> > builtin/init-db.c and builtin/clone.c share out of the former file and
> > into setup.c.  One issue that made it difficult was the
> > init_is_bare_repository global variable.
> >
> > init_is_bare_repository is actually not very useful.  It merely stores
> > the return value from is_bare_repository() and only for the duration of
> > a few additional function calls before its value is checked, and none of
> > those functions do anything that could change is_bare_repository()'s
> > return value.  So, we can simply dispense with the global by replacing
> > it with is_bare_repository().
>
> I think the purpose of init_is_bare_repository is something different.
> But based off my different understanding, I can't reproduce any
> different behavior. I don't know if I'm just confused or not, but I'll
> leave some breadcrumbs here to check my understanding.

So, while my patch does not change behavior under any circumstances, my explanation was slightly wrong. More on that below...

Show 17 quoted lines

> Reordering the hunks for clarity,
>
> > @@ -422,8 +436,6 @@ int init_db(const char *git_dir, const char *real_git_dir,
> >
> >       safe_create_dir(git_dir, 0);
> >
> > -     init_is_bare_repository = is_bare_repository();
> > -
> >       /* Check to see if the repository version is right.
> >        * Note that a newly created repository does not have
> >        * config file, so this will not fail.  What we are catching
>
> Here, init_db() caches the value of is_bare_repository(), which itself
> reads the value of is_bare_repository_cfg, which can be modified by when
> we read "core.bare" via git_config(git_default_config) or similar
> (basically, any config callback that uses git_default_config). It is
> also modified in other places though, like setup.c.

Close. is_bare_repository() not only reads the value of is_bare_repository_cfg, but it ANDs the result with !get_git_work_tree(). This turns out to be important.

> IIUC, we haven't actually parsed "core.bare" at this point. The
> git_config() call just above this calls "plaform_core_config", which is
> either "mingw_core_config" (doesn't read "core.bare") or
> noop_core_config (noop).

I believe this is correct.

Show 41 quoted lines

> > @@ -231,9 +230,24 @@ static int create_default_files(const char *template_path,
> >        * We must make sure command-line options continue to override any
> >        * values we might have just re-read from the config.
> >        */
> > -     is_bare_repository_cfg = init_is_bare_repository || !work_tree;
> >       if (init_shared_repository != -1)
> >               set_shared_repository(init_shared_repository);
> > +     /*
> > +      * TODO: heed core.bare from config file in templates if no
> > +      *       command-line override given
> > +      *
> > +      * Unfortunately, this location in the code is far too late to
> > +      * allow this to happen; both builtin/init.db and
> > +      * builtin/clone.c setup the new repository and call
> > +      * set_git_work_tree() before this point.  (Note that both do
> > +      * that repository setup before calling init_db(), which in
> > +      * turn calls create_default_files().)  Fixing it would
> > +      * require too much refactoring, and no one seems to have
> > +      * wanted this behavior in 15+ years, so we'll continue
> > +      * ignoring the config for now and just override
> > +      * is_bare_repository_cfg unconditionally.
> > +      */
> > +     is_bare_repository_cfg = is_bare_repository() || !work_tree;
> >
> >       /*
> >        * We would have created the above under user's umask -- under
>
> Now, we're in the midst of the re-init. Expanding the context a little,
> we see:
>
>         git_config(git_default_config, NULL);
>
>         /*
>          * We must make sure command-line options continue to override any
>          * values we might have just re-read from the config.
>          */
>         is_bare_repository_cfg = init_is_bare_repository || !work_tree;
>
> So now we've read the new config of the re-inited repo, which might have
> "core.bare" set to a value other than what "git init-db [--bare]"
> started with

Correct.

> so we want to _intentionally_ ignore it.

That's what the code does, but not what it should do. If neither `--bare` nor `--no-bare` is given on the command line, we ought to pay attention to the config setting.

The fact that we do ignore the config regardless of command line flags is the bug that this patch documents.

> We do this by
> reading out the cached value, _not_ by calling is_bare_repository()
> again.

Almost, but you missed part of the line you are commenting on. We read out the cached value *and* OR it with !work_tree. The distinction may look small, but it turns out to be critical.

> So it seems to me like this patch changes the intent.

Yeah, looks like it does.

But it doesn't change the result.  Note the critical thing here is we
changed from computing:
   init_is_bare_repository || !work_tree;
to
   is_bare_repository() || !work_tree;

Further,
   init_is_bare_repository = [cached value of] is_bare_repository_cfg
&& [cached value of] !get_git_work_tree()
   is_bare_repository() = [current value of] is_bare_repository_cfg &&
[current value of] !get_git_work_tree()

However, nothing calls repo_init() or set_git_work_tree() between any
of these calls, so
   [current value of] !get_git_work_tree() == [cached value of]
!get_git_work_tree() == !work_tree

So, both before and after what we are computing simplifies to
   <something> && !work_tree || !work_tree
which further simplifies, regardless of whether <something> is true or
false, down to
   !work_tree

So whether we are using a cached value of is_bare_repository_cfg or a
current value of is_bare_repository_cfg is irrelevant.  In fact, from
the analysis above, we can simplify this line to just
   is_bare_repository_cfg = !work_tree;
completely ignoring both is_bare_repository_cfg and
is_bare_repository(), and it won't change the behavior.  I just did
it; it passes all tests.

> Where I struggling with is how to make this behave badly. The lines
> above seem to be defensive in nature - we never use
> is_bare_repository_cfg past this point, but we want to guard against
> unintentional behavior in the future.

Not quite true; there is another call to is_bare_repository() in the same function about 60 lines later, which does pay attention to is_bare_repository_cfg.

[...]

> If I'm right (which I'm not sure about), we might need to keep
> init_is_bare_repository around _somewhere_. Not a global, but maybe
> as a param.

This makes sense, despite the other bugs present. I'll make this change, as well as split this patch into two as Calvin suggested, and update the description to correct my errors and explain the bug better.