From: Jonathan Tan <jonathantanmy@google.com>
Date: Fri, 12 May 2023 21:34:41 GMT
Subject: Re: [PATCH v2 01/27] init-db: document existing bug with core.bare in template config
Message-ID: <20230512213441.1127721-1-jonathantanmy@google.com>
In-Reply-To: <f7ee69e7e687e8cbdead070df644c5bca23d7578.1683875070.git.gitgitgadget@gmail.com>

"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.