diff mbox series

[v2,01/27] init-db: document existing bug with core.bare in template config

Message ID f7ee69e7e687e8cbdead070df644c5bca23d7578.1683875070.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Header cleanups (final splitting of cache.h, and some splitting of other headers) | expand

Commit Message

Elijah Newren May 12, 2023, 7:04 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

The comments in create_default_files() talks about reading config from
the config file in the specified `--templates` directory, which leads to
the question of whether core.bare could be set in such a config file and
thus whether the code is doing the right thing.  It turns out, that it
doesn't; it unconditionally ignores core.bare in the config file in any
--templates directory.  It is not clear to me that fixing it can be done
within this function; it seems to occur too late:
  * create_default_files() is called by init_db()
  * init_db() is called by both builtin/{clone.c,init-db.c}
  * both callers of init_db() call set_git_work_tree() before init_db()
and in order to actual affect whether a repository is bear, we'd need to
somewhere reset these values, not just the is_bare_repository_cfg
setting.

I do not want to open this can of worms at this time; I'm trying to
clean up some headers, for which I need to move some functions, for
which I need to clean up some globals, and that's far enough down the
rabbit hole.  So, simply document the issue with a careful TODO comment
and a few testcases.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/init-db.c        | 29 ++++++++++++++++++++++++++++-
 t/t1301-shared-repo.sh   | 22 ++++++++++++++++++++++
 t/t5606-clone-options.sh | 10 ++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)

Comments

Jonathan Tan May 12, 2023, 9:34 p.m. UTC | #1 
"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/

> +	 *                 = 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 May 16, 2023, 3:08 a.m. UTC | #2 
On Fri, May 12, 2023 at 2:34 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> "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/

Thanks; will fix.

> > +      *                 = 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.
diff mbox series

Patch

diff --git a/builtin/init-db.c b/builtin/init-db.c
index aef40361052..715e94befa0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -231,9 +231,36 @@  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
+	 */
+	is_bare_repository_cfg = init_is_bare_repository || !work_tree;
+	/* 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()
+	 * expanding the right hande side:
+	 *                 = 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, and thus the !get_git_work_tree()
+	 * calls will return the same result each time.  So, what we are
+	 * interested in computing is the right hand side of the line of
+	 * code just above this comment:
+	 *     init_is_bare_repository || !work_tree
+	 *        = is_bare_repository_cfg && !work_tree || !work_tree
+	 *        = !work_tree
+	 * because "A && !B || !B == !B" for all boolean values of A & B.
+	 */
 
 	/*
 	 * We would have created the above under user's umask -- under
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' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "rm -rf templates" &&
+	test_config core.bare true &&
+	umask 0022 &&
+	mkdir -p templates/ &&
+	cp .git/config templates/config &&
+	git init --template=templates subdir &&
+	test_path_exists subdir/HEAD
+'
+
+test_expect_success 'template can set core.bare but overridden by command line' '
+	test_when_finished "rm -rf subdir" &&
+	test_when_finished "rm -rf templates" &&
+	test_config core.bare true &&
+	umask 0022 &&
+	mkdir -p templates/ &&
+	cp .git/config templates/config &&
+	git init --no-bare --template=templates subdir &&
+	test_path_exists subdir/.git/HEAD
+'
+
 test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '
 	: > a1 &&
 	git add a1 &&
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 27f9f776389..5890319b97b 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -120,6 +120,16 @@  test_expect_success 'prefers -c config over --template config' '
 
 '
 
+test_expect_failure 'prefers --template config even for core.bare' '
+
+	template="$TRASH_DIRECTORY/template-with-bare-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" core.bare true &&
+	git clone "--template=$template" parent clone-bare-config &&
+	test "$(git -C clone-bare-config config --local core.bare)" = "true" &&
+	test_path_is_file clone-bare-config/HEAD
+'
+
 test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
 
 	test_config_global clone.defaultRemoteName from_config &&