"Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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.
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.
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).
> @@ -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, so we want to _intentionally_ ignore it. We do this by
reading out the cached value, _not_ by calling is_bare_repository()
again. So it seems to me like this patch changes the intent.
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.
But I thought that logging this value would show a difference in
behavior, e.g.
diff --git a/builtin/init-db.c b/builtin/init-db.c
index ba6e0b20fa..da3579d46d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -229,6 +229,8 @@ static int create_default_files(const char *template_path,
* values we might have just re-read from the config.
*/
is_bare_repository_cfg = init_is_bare_repository || !work_tree;
+ printf_ln("is_bare_repository_cfg is %d", is_bare_repository_cfg);
+
if (init_shared_repository != -1)
set_shared_repository(init_shared_repository);
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1b6437ec07..e4b978992e 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -15,6 +15,14 @@ TEST_PASSES_SANITIZE_LEAK=true
# Remove a default ACL from the test dir if possible.
setfacl -k . 2>/dev/null
+test_expect_success 're-init respects core.bare' '
+ git init bare.git >actual &&
+ grep "is_bare_repository_cfg is 0" actual &&
+ git -C bare.git config core.bare true &&
+ git init bare.git >actual &&
+ grep "is_bare_repository_cfg is 1" actual
+'
+
# User must have read permissions to the repo -> failure on --shared=0400
test_expect_success 'shared = 0400 (faulty permission u-w)' '
test_when_finished "rm -rf sub" &&
But apparently this doesn't work at all. I can't get this to print
"is_bare_repository_cfg is 1" no matter what I do, before or after your
patch. Maybe there's more at play here (e.g. with setup.c), but I
haven't figured that out.
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.
