Re: [PATCH v2 01/27] init-db: document existing bug with core.bare in template config
On Fri, May 12, 2023 at 2:34 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 29 quoted lines
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > - 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
> > + */
> > + is_bare_repository_cfg = init_is_bare_repository || !work_tree;
>
> This patch moves this line down a few lines, but that's fine because
> set_shared_repository() doesn't modify any of the relevant variables.
>
> > + /* TODO (continued):
> > + *
> > + * Unfortunately, the line above is equivalent to
> > + * is_bare_repository_cfg = !work_tree;
> > + * which ignores the config entirely even if no `--[no-]bare`
> > + * command line option was present.
> > + *
> > + * To see why, note that before this function, there was this call:
> > + * init_is_bare_repository = is_bare_repository()
>
> This is in init_db(), indeed.
>
> > + * expanding the right hande side:
>
> s/hande/hand/
Show 23 quoted lines
> > + * = is_bare_repository_cfg && !get_git_work_tree()
> > + * = is_bare_repository_cfg && !work_tree
> > + * note that the last simplification above is valid because nothing
> > + * calls repo_init() or set_git_work_tree() between any of the
> > + * relevant calls in the code,
>
> Yes, the only calls are check_repository_format() and
> validate_hash_algorithm() (as can be seen in init_db()) before
> get_git_work_tree() is called at the start of create_default_files().
>
> > diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> > index 1b6437ec079..c02fd64793b 100755
> > --- a/t/t1301-shared-repo.sh
> > +++ b/t/t1301-shared-repo.sh
> > @@ -52,6 +52,28 @@ test_expect_success 'shared=all' '
> > test 2 = $(git config core.sharedrepository)
> > '
> >
> > +test_expect_failure 'template can set core.bare' '
>
> I would have preferred a test_expect_success with the exact failing line
> documented and prepended with test_must_fail, but I can see why someone
> would prefer test_expect_failure.
Elijah Newren <newren@gmail.com>