{"thread":{"id":"62461","subject":"[PATCH 0/3] Remove is_bare_repository_cfg global state","startedAt":"2024-11-06T20:48:06Z","lastAt":"2024-12-11T23:09:48Z","messageCount":11,"participants":["John Cai via GitGitGadget","Junio C Hamano","shejialuo"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"506756","messageId":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","threadId":"62461","inReplyTo":null,"subject":"[PATCH 0/3] Remove is_bare_repository_cfg global state","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-06T20:47:59Z","receivedAt":"2024-11-06T20:48:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This patch series removes the global state introduced by the\nis_bare_repository_cfg variable by moving it into the repository struct.\nMost of the refactor is done by patch 1. Patch 2 initializes the member in\nplaces that left it unInitialized, while patch 3 adds a safety measure by\nBUG()ing when the variable has not been properly initialized.\n\nJohn Cai (3):\n  git: remove is_bare_repository_cfg global variable\n  setup: initialize is_bare_cfg\n  repository: BUG when is_bare_cfg is not initialized\n\n attr.c                        |  4 ++--\n builtin/bisect.c              |  2 +-\n builtin/blame.c               |  2 +-\n builtin/check-attr.c          |  2 +-\n builtin/clone.c               |  4 ++--\n builtin/gc.c                  |  2 +-\n builtin/init-db.c             | 14 +++++++-------\n builtin/repack.c              |  2 +-\n builtin/reset.c               |  2 +-\n builtin/rev-parse.c           |  2 +-\n builtin/submodule--helper.c   |  2 +-\n config.c                      |  2 +-\n dir.c                         |  2 +-\n environment.c                 |  7 -------\n environment.h                 |  3 +--\n git.c                         |  2 +-\n mailmap.c                     |  4 ++--\n refs/files-backend.c          |  2 +-\n refs/reftable-backend.c       |  2 +-\n repository.c                  | 23 +++++++++++++++++++----\n repository.h                  | 12 +++++++++++-\n scalar.c                      |  2 +-\n setup.c                       | 19 +++++++++++++------\n submodule.c                   |  2 +-\n t/helper/test-partial-clone.c |  2 +-\n t/helper/test-repository.c    |  4 ++--\n transport.c                   |  4 ++--\n worktree.c                    |  4 ++--\n 28 files changed, 79 insertions(+), 55 deletions(-)\n\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1826%2Fjohn-cai%2Fjc%2Fremove_is_bare_global-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1826/john-cai/jc/remove_is_bare_global-v1\nPull-Request: https://github.com/git/git/pull/1826\n-- \ngitgitgadget\n"},{"id":"506757","messageId":"3d341a9ae4ef1d2776734fa82a45913f91e6083c.1730926082.git.gitgitgadget@gmail.com","threadId":"62461","inReplyTo":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","subject":"[PATCH 1/3] git: remove is_bare_repository_cfg global variable","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-06T20:48:00Z","receivedAt":"2024-11-06T20:48:07Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nThe is_bare_repository_cfg global variable is used for storing a bare\nrepository setting, either through the config, an env var, or the\ncommandline. This variable is global, and hence introduces global state\neverywhere it is used.\n\nIn order to reduce global state, add a member to the repository struct\nto keep track of the setting there. For now, the_repository is what's\nused to set the member, which still represents global state. However,\nthere is a parallel effort to replace calls to the_repository with a\nrepository struct that is passed into builtins, see [1]. Hence, this\nchange will help the overall effort in reducing global state.\n\n1. 9b1cb5070f (builtin: add a repository parameter for builtin\n   functions, Fri Sep 13 21:16:14 2024 +0000)\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n attr.c                        |  4 ++--\n builtin/bisect.c              |  2 +-\n builtin/blame.c               |  2 +-\n builtin/check-attr.c          |  2 +-\n builtin/clone.c               |  4 ++--\n builtin/gc.c                  |  2 +-\n builtin/init-db.c             | 14 +++++++-------\n builtin/repack.c              |  2 +-\n builtin/reset.c               |  2 +-\n builtin/rev-parse.c           |  2 +-\n builtin/submodule--helper.c   |  2 +-\n config.c                      |  2 +-\n dir.c                         |  2 +-\n environment.c                 |  7 -------\n environment.h                 |  3 +--\n git.c                         |  2 +-\n mailmap.c                     |  4 ++--\n refs/files-backend.c          |  2 +-\n refs/reftable-backend.c       |  2 +-\n repository.c                  | 21 +++++++++++++++++----\n repository.h                  | 12 +++++++++++-\n scalar.c                      |  2 +-\n setup.c                       | 12 ++++++------\n submodule.c                   |  2 +-\n t/helper/test-partial-clone.c |  2 +-\n t/helper/test-repository.c    |  4 ++--\n transport.c                   |  4 ++--\n worktree.c                    |  4 ++--\n 28 files changed, 70 insertions(+), 55 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex c605d2c1703..053cd59af26 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -716,7 +716,7 @@ static enum git_attr_direction direction;\n \n void git_attr_set_direction(enum git_attr_direction new_direction)\n {\n-\tif (is_bare_repository() && new_direction != GIT_ATTR_INDEX)\n+\tif (repo_is_bare(the_repository) && new_direction != GIT_ATTR_INDEX)\n \t\tBUG(\"non-INDEX attr direction in a bare repo\");\n \n \tif (new_direction != direction)\n@@ -883,7 +883,7 @@ static struct attr_stack *read_attr(struct index_state *istate,\n \t\tres = read_attr_from_index(istate, path, flags);\n \t} else if (tree_oid) {\n \t\tres = read_attr_from_blob(istate, tree_oid, path, flags);\n-\t} else if (!is_bare_repository()) {\n+\t} else if (!repo_is_bare(the_repository)) {\n \t\tif (direction == GIT_ATTR_CHECKOUT) {\n \t\t\tres = read_attr_from_index(istate, path, flags);\n \t\t\tif (!res)\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 21d17a6c1a8..b794a84528f 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -705,7 +705,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \tstruct object_id oid;\n \tconst char *head;\n \n-\tif (is_bare_repository())\n+\tif (repo_is_bare(the_repository))\n \t\tno_checkout = 1;\n \n \t/*\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e407a22da3b..5365c3d4594 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1092,7 +1092,7 @@ parse_done:\n \n \trevs.disable_stdin = 1;\n \tsetup_revisions(argc, argv, &revs, NULL);\n-\tif (!revs.pending.nr && is_bare_repository()) {\n+\tif (!revs.pending.nr && repo_is_bare(the_repository)) {\n \t\tstruct commit *head_commit;\n \t\tstruct object_id head_oid;\n \ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 7cf275b8937..37baf64f949 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -116,7 +116,7 @@ int cmd_check_attr(int argc,\n \tstruct object_id initialized_oid;\n \tint cnt, i, doubledash, filei;\n \n-\tif (!is_bare_repository())\n+\tif (!repo_is_bare(the_repository))\n \t\tsetup_work_tree();\n \n \tgit_config(git_default_config, NULL);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 59fcb317a68..80b594c6011 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1415,7 +1415,7 @@ int cmd_clone(int argc,\n \t\trepo_clear(the_repository);\n \n \t\t/* At this point, we need the_repository to match the cloned repo. */\n-\t\tif (repo_init(the_repository, git_dir, work_tree))\n+\t\tif (repo_init(the_repository, git_dir, work_tree, -1))\n \t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n \t\telse if (fetch_bundle_uri(the_repository, bundle_uri, &has_heuristic))\n \t\t\twarning(_(\"failed to fetch objects from bundle URI '%s'\"),\n@@ -1446,7 +1446,7 @@ int cmd_clone(int argc,\n \t\t\trepo_clear(the_repository);\n \n \t\t\t/* At this point, we need the_repository to match the cloned repo. */\n-\t\t\tif (repo_init(the_repository, git_dir, work_tree))\n+\t\t\tif (repo_init(the_repository, git_dir, work_tree, -1))\n \t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n \t\t\telse if (fetch_bundle_list(the_repository,\n \t\t\t\t\t\t   transport->bundles))\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex d52735354c9..e43219e1c17 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -712,7 +712,7 @@ struct repository *repo UNUSED)\n \t\tdie(_(\"failed to parse gc.logExpiry value %s\"), cfg.gc_log_expire);\n \n \tif (cfg.pack_refs < 0)\n-\t\tcfg.pack_refs = !is_bare_repository();\n+\t\tcfg.pack_refs = !repo_is_bare(the_repository);\n \n \targc = parse_options(argc, argv, prefix, builtin_gc_options,\n \t\t\t     builtin_gc_usage, 0);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 7e00d57d654..901bf30b508 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -89,7 +89,7 @@ int cmd_init_db(int argc,\n \tconst struct option init_db_options[] = {\n \t\tOPT_STRING(0, \"template\", &template_dir, N_(\"template-directory\"),\n \t\t\t\tN_(\"directory from which templates will be used\")),\n-\t\tOPT_SET_INT(0, \"bare\", &is_bare_repository_cfg,\n+\t\tOPT_SET_INT(0, \"bare\", &the_repository->is_bare_cfg,\n \t\t\t\tN_(\"create a bare repository\"), 1),\n \t\t{ OPTION_CALLBACK, 0, \"shared\", &init_shared_repository,\n \t\t\tN_(\"permissions\"),\n@@ -109,7 +109,7 @@ int cmd_init_db(int argc,\n \n \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n \n-\tif (real_git_dir && is_bare_repository_cfg == 1)\n+\tif (real_git_dir && the_repository->is_bare_cfg == 1)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--separate-git-dir\", \"--bare\");\n \n \tif (real_git_dir && !is_absolute_path(real_git_dir))\n@@ -155,7 +155,7 @@ int cmd_init_db(int argc,\n \t} else if (0 < argc) {\n \t\tusage(init_db_usage[0]);\n \t}\n-\tif (is_bare_repository_cfg == 1) {\n+\tif (the_repository->is_bare_cfg == 1) {\n \t\tchar *cwd = xgetcwd();\n \t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);\n \t\tfree(cwd);\n@@ -182,7 +182,7 @@ int cmd_init_db(int argc,\n \t */\n \tgit_dir = xstrdup_or_null(getenv(GIT_DIR_ENVIRONMENT));\n \twork_tree = xstrdup_or_null(getenv(GIT_WORK_TREE_ENVIRONMENT));\n-\tif ((!git_dir || is_bare_repository_cfg == 1) && work_tree)\n+\tif ((!git_dir || the_repository->is_bare_cfg == 1) && work_tree)\n \t\tdie(_(\"%s (or --work-tree=<directory>) not allowed without \"\n \t\t\t  \"specifying %s (or --git-dir=<directory>)\"),\n \t\t    GIT_WORK_TREE_ENVIRONMENT,\n@@ -218,10 +218,10 @@ int cmd_init_db(int argc,\n \t\tstrbuf_release(&sb);\n \t}\n \n-\tif (is_bare_repository_cfg < 0)\n-\t\tis_bare_repository_cfg = guess_repository_type(git_dir);\n+\tif (the_repository->is_bare_cfg < 0)\n+\t\tthe_repository->is_bare_cfg = guess_repository_type(git_dir);\n \n-\tif (!is_bare_repository_cfg) {\n+\tif (!the_repository->is_bare_cfg) {\n \t\tconst char *git_dir_parent = strrchr(git_dir, '/');\n \t\tif (git_dir_parent) {\n \t\t\tchar *rel = xstrndup(git_dir, git_dir_parent - git_dir);\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d6bb37e84ae..45621f70c5b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1266,7 +1266,7 @@ int cmd_repack(int argc,\n \n \tif (write_bitmaps < 0) {\n \t\tif (!write_midx &&\n-\t\t    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n+\t\t    (!(pack_everything & ALL_INTO_ONE) || !repo_is_bare(the_repository)))\n \t\t\twrite_bitmaps = 0;\n \t}\n \tif (pack_kept_objects < 0)\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7154f88826d..dccd5d95dae 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -448,7 +448,7 @@ int cmd_reset(int argc,\n \tif (reset_type != SOFT && (reset_type != MIXED || repo_get_work_tree(the_repository)))\n \t\tsetup_work_tree();\n \n-\tif (reset_type == MIXED && is_bare_repository())\n+\tif (reset_type == MIXED && repo_is_bare(the_repository))\n \t\tdie(_(\"%s reset is not allowed in a bare repository\"),\n \t\t    _(reset_type_names[reset_type]));\n \ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 8401b4d7ab6..281b557483e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -1063,7 +1063,7 @@ int cmd_rev_parse(int argc,\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-bare-repository\")) {\n-\t\t\t\tprintf(\"%s\\n\", is_bare_repository() ? \"true\"\n+\t\t\t\tprintf(\"%s\\n\", repo_is_bare(the_repository) ? \"true\"\n \t\t\t\t\t\t: \"false\");\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b6b5f1ebde7..7bff99bf08f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1591,7 +1591,7 @@ static int add_possible_reference_from_superproject(\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tstrbuf_add(&sb, odb->path, len);\n \n-\t\tif (repo_init(&alternate, sb.buf, NULL) < 0)\n+\t\tif (repo_init(&alternate, sb.buf, NULL, the_repository->is_bare_cfg) < 0)\n \t\t\tdie(_(\"could not get a repository handle for gitdir '%s'\"),\n \t\t\t    sb.buf);\n \ndiff --git a/config.c b/config.c\nindex a11bb85da30..c1b14c89947 100644\n--- a/config.c\n+++ b/config.c\n@@ -1441,7 +1441,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.bare\")) {\n-\t\tis_bare_repository_cfg = git_config_bool(var, value);\n+\t\tthe_repository->is_bare_cfg = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \ndiff --git a/dir.c b/dir.c\nindex e3ddd5b5296..c995668e54c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -4008,7 +4008,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,\n \tconst struct submodule *sub;\n \n \t/* If the submodule has no working tree, we can ignore it. */\n-\tif (repo_init(&subrepo, sub_gitdir, sub_worktree))\n+\tif (repo_init(&subrepo, sub_gitdir, sub_worktree, the_repository->is_bare_cfg))\n \t\treturn;\n \n \tif (repo_read_index(&subrepo) < 0)\ndiff --git a/environment.c b/environment.c\nindex a2ce9980818..9af20d5e34e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -34,7 +34,6 @@ int has_symlinks = 1;\n int minimum_abbrev = 4, default_abbrev = -1;\n int ignore_case;\n int assume_unchanged;\n-int is_bare_repository_cfg = -1; /* unspecified */\n int warn_on_object_refname_ambiguity = 1;\n int repository_format_precious_objects;\n char *git_commit_encoding;\n@@ -146,12 +145,6 @@ const char *getenv_safe(struct strvec *argv, const char *name)\n \treturn argv->v[argv->nr - 1];\n }\n \n-int is_bare_repository(void)\n-{\n-\t/* if core.bare is not 'false', let's see if there is a work tree */\n-\treturn is_bare_repository_cfg && !repo_get_work_tree(the_repository);\n-}\n-\n int have_git_dir(void)\n {\n \treturn startup_info->have_repository\ndiff --git a/environment.h b/environment.h\nindex 923e12661e1..23f29a4df05 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -144,8 +144,7 @@ void set_shared_repository(int value);\n int get_shared_repository(void);\n void reset_shared_repository(void);\n \n-extern int is_bare_repository_cfg;\n-int is_bare_repository(void);\n+int is_bare_repository(struct repository *repo);\n extern char *git_work_tree_cfg;\n \n /* Environment bits from configuration mechanism */\ndiff --git a/git.c b/git.c\nindex c2c1b8e22c2..c8ed29b2295 100644\n--- a/git.c\n+++ b/git.c\n@@ -251,7 +251,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n \t\t\tchar *cwd = xgetcwd();\n-\t\t\tis_bare_repository_cfg = 1;\n+\t\t\tthe_repository->is_bare_cfg = 1;\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, cwd, 0);\n \t\t\tfree(cwd);\n \t\t\tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\ndiff --git a/mailmap.c b/mailmap.c\nindex 9f9fa3199a8..65fdd853a8e 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -216,10 +216,10 @@ int read_mailmap(struct string_list *map)\n \tmap->strdup_strings = 1;\n \tmap->cmp = namemap_cmp;\n \n-\tif (!git_mailmap_blob && is_bare_repository())\n+\tif (!git_mailmap_blob && repo_is_bare(the_repository))\n \t\tgit_mailmap_blob = xstrdup(\"HEAD:.mailmap\");\n \n-\tif (!startup_info->have_repository || !is_bare_repository())\n+\tif (!startup_info->have_repository || !repo_is_bare(the_repository))\n \t\terr |= read_mailmap_file(map, \".mailmap\",\n \t\t\t\t\t startup_info->have_repository ?\n \t\t\t\t\t MAILMAP_NOFOLLOW : 0);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0824c0b8a94..7310dc0b332 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1779,7 +1779,7 @@ static int log_ref_setup(struct files_ref_store *refs,\n \tchar *logfile;\n \n \tif (log_refs_cfg == LOG_REFS_UNSET)\n-\t\tlog_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n+\t\tlog_refs_cfg = repo_is_bare(refs->base.repo) ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n \tfiles_reflog_path(refs, &logfile_sb, refname);\n \tlogfile = strbuf_detach(&logfile_sb, NULL);\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 38eb14d591e..f3b875726cb 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -163,7 +163,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname\n {\n \tenum log_refs_config log_refs_cfg = refs->log_all_ref_updates;\n \tif (log_refs_cfg == LOG_REFS_UNSET)\n-\t\tlog_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n+\t\tlog_refs_cfg = repo_is_bare(refs->base.repo) ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n \tswitch (log_refs_cfg) {\n \tcase LOG_REFS_NONE:\ndiff --git a/repository.c b/repository.c\nindex f988b8ae68a..96608058b61 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -25,7 +25,9 @@\n extern struct repository *the_repository;\n \n /* The main repository */\n-static struct repository the_repo;\n+static struct repository the_repo = {\n+\t.is_bare_cfg = -1,\n+};\n struct repository *the_repository = &the_repo;\n \n /*\n@@ -263,10 +265,13 @@ static int read_and_verify_repository_format(struct repository_format *format,\n /*\n  * Initialize 'repo' based on the provided 'gitdir'.\n  * Return 0 upon success and a non-zero value upon failure.\n+ * is_bare can be passed to indicate whether or not the repository should be\n+ * treated as bare when repo_init() is used to initiate a secondary repository.\n  */\n int repo_init(struct repository *repo,\n \t      const char *gitdir,\n-\t      const char *worktree)\n+\t      const char *worktree,\n+\t      int is_bare)\n {\n \tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \tmemset(repo, 0, sizeof(*repo));\n@@ -283,6 +288,8 @@ int repo_init(struct repository *repo,\n \trepo_set_compat_hash_algo(repo, format.compat_hash_algo);\n \trepo_set_ref_storage_format(repo, format.ref_storage_format);\n \trepo->repository_format_worktree_config = format.worktree_config;\n+\tif (is_bare > 0)\n+\t\trepo->is_bare_cfg = is_bare;\n \n \t/* take ownership of format.partial_clone */\n \trepo->repository_format_partial_clone = format.partial_clone;\n@@ -314,7 +321,7 @@ int repo_submodule_init(struct repository *subrepo,\n \tstrbuf_repo_worktree_path(&gitdir, superproject, \"%s/.git\", path);\n \tstrbuf_repo_worktree_path(&worktree, superproject, \"%s\", path);\n \n-\tif (repo_init(subrepo, gitdir.buf, worktree.buf)) {\n+\tif (repo_init(subrepo, gitdir.buf, worktree.buf, superproject->is_bare_cfg)) {\n \t\t/*\n \t\t * If initialization fails then it may be due to the submodule\n \t\t * not being populated in the superproject's worktree.  Instead\n@@ -332,7 +339,7 @@ int repo_submodule_init(struct repository *subrepo,\n \t\tstrbuf_reset(&gitdir);\n \t\tsubmodule_name_to_gitdir(&gitdir, superproject, sub->name);\n \n-\t\tif (repo_init(subrepo, gitdir.buf, NULL)) {\n+\t\tif (repo_init(subrepo, gitdir.buf, NULL, superproject->is_bare_cfg)) {\n \t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n@@ -453,3 +460,9 @@ int repo_hold_locked_index(struct repository *repo,\n \t\tBUG(\"the repo hasn't been setup\");\n \treturn hold_lock_file_for_update(lf, repo->index_file, flags);\n }\n+\n+int repo_is_bare(struct repository *repo)\n+{\n+\t/* if core.bare is not 'false', let's see if there is a work tree */\n+\treturn repo->is_bare_cfg && !repo_get_work_tree(repo);\n+}\ndiff --git a/repository.h b/repository.h\nindex 24a66a496a6..c243653492b 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -153,6 +153,14 @@ struct repository {\n \n \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n \tunsigned different_commondir:1;\n+\n+\t/*\n+\t * Indicates if the repository is set to be treated as a bare repository,\n+\t * through a command line argument, configuration, or environment\n+\t * variable.\n+\t * -1 means unspecified, 0 indicates non-bare, and 1 indicates bare.\n+\t */\n+\tint is_bare_cfg;\n };\n \n #ifdef USE_THE_REPOSITORY_VARIABLE\n@@ -188,7 +196,7 @@ void repo_set_ref_storage_format(struct repository *repo,\n \t\t\t\t enum ref_storage_format format);\n void initialize_repository(struct repository *repo);\n RESULT_MUST_BE_USED\n-int repo_init(struct repository *r, const char *gitdir, const char *worktree);\n+int repo_init(struct repository *r, const char *gitdir, const char *worktree, int is_bare);\n \n /*\n  * Initialize the repository 'subrepo' as the submodule at the given path. If\n@@ -232,4 +240,6 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n  */\n int upgrade_repository_format(int target_version);\n \n+int repo_is_bare(struct repository *repo);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/scalar.c b/scalar.c\nindex ac0cb579d3f..c2ec1f3e745 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -722,7 +722,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\tgit_config_clear();\n \n-\t\tif (repo_init(&r, gitdir.buf, commondir.buf))\n+\t\tif (repo_init(&r, gitdir.buf, commondir.buf, the_repository->is_bare_cfg))\n \t\t\tgoto loop_end;\n \n \t\told_repo = the_repository;\ndiff --git a/setup.c b/setup.c\nindex 7b648de0279..6bc4aef3a8b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -766,8 +766,8 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \n \tif (!has_common) {\n \t\tif (candidate->is_bare != -1) {\n-\t\t\tis_bare_repository_cfg = candidate->is_bare;\n-\t\t\tif (is_bare_repository_cfg == 1)\n+\t\t\tthe_repository->is_bare_cfg = candidate->is_bare;\n+\t\t\tif (the_repository->is_bare_cfg == 1)\n \t\t\t\tinside_work_tree = -1;\n \t\t}\n \t\tif (candidate->work_tree) {\n@@ -1030,7 +1030,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */\n \tif (work_tree_env)\n \t\tset_git_work_tree(work_tree_env);\n-\telse if (is_bare_repository_cfg > 0) {\n+\telse if (the_repository->is_bare_cfg > 0) {\n \t\tif (git_work_tree_cfg) {\n \t\t\t/* #22.2, #30 */\n \t\t\twarning(\"core.bare and core.worktree do not make sense\");\n@@ -1116,7 +1116,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \t}\n \n \t/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */\n-\tif (is_bare_repository_cfg > 0) {\n+\tif (the_repository->is_bare_cfg > 0) {\n \t\tset_git_dir(gitdir, (offset != cwd->len));\n \t\tif (chdir(cwd->buf))\n \t\t\tdie_errno(_(\"cannot come back to cwd\"));\n@@ -2323,7 +2323,7 @@ static int create_default_files(const char *template_path,\n \tif (init_shared_repository != -1)\n \t\tset_shared_repository(init_shared_repository);\n \n-\tis_bare_repository_cfg = !work_tree;\n+\tthe_repository->is_bare_cfg = !work_tree;\n \n \t/*\n \t * We would have created the above under user's umask -- under\n@@ -2349,7 +2349,7 @@ static int create_default_files(const char *template_path,\n \t}\n \tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n \n-\tif (is_bare_repository())\n+\tif (repo_is_bare(the_repository))\n \t\tgit_config_set(\"core.bare\", \"true\");\n \telse {\n \t\tgit_config_set(\"core.bare\", \"false\");\ndiff --git a/submodule.c b/submodule.c\nindex 74d5766f07c..059b9d32dcb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -535,7 +535,7 @@ static struct repository *open_submodule(const char *path)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct repository *out = xmalloc(sizeof(*out));\n \n-\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n+\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL, -1)) {\n \t\tstrbuf_release(&sb);\n \t\tfree(out);\n \t\treturn NULL;\ndiff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c\nindex a1af9710c31..66d9390dae1 100644\n--- a/t/helper/test-partial-clone.c\n+++ b/t/helper/test-partial-clone.c\n@@ -19,7 +19,7 @@ static void object_info(const char *gitdir, const char *oid_hex)\n \tstruct object_info oi = {.sizep = &size};\n \tconst char *p;\n \n-\tif (repo_init(&r, gitdir, NULL))\n+\tif (repo_init(&r, gitdir, NULL, -1))\n \t\tdie(\"could not init repo\");\n \tif (parse_oid_hex_algop(oid_hex, &oid, &p, r.hash_algo))\n \t\tdie(\"could not parse oid\");\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex 63c37de33d2..90d58190c37 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -21,7 +21,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \n \trepo_clear(the_repository);\n \n-\tif (repo_init(&r, gitdir, worktree))\n+\tif (repo_init(&r, gitdir, worktree, -1))\n \t\tdie(\"Couldn't init repo\");\n \n \trepo_set_hash_algo(the_repository, hash_algo_by_ptr(r.hash_algo));\n@@ -51,7 +51,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \n \trepo_clear(the_repository);\n \n-\tif (repo_init(&r, gitdir, worktree))\n+\tif (repo_init(&r, gitdir, worktree, -1))\n \t\tdie(\"Couldn't init repo\");\n \n \trepo_set_hash_algo(the_repository, hash_algo_by_ptr(r.hash_algo));\ndiff --git a/transport.c b/transport.c\nindex 47fda6a7732..d72b8380846 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1428,7 +1428,7 @@ int transport_push(struct repository *r,\n \n \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t    !is_bare_repository()) {\n+\t    !repo_is_bare(r)) {\n \t\tstruct ref *ref = remote_refs;\n \t\tstruct oid_array commits = OID_ARRAY_INIT;\n \n@@ -1455,7 +1455,7 @@ int transport_push(struct repository *r,\n \tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n \t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n \t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n-\t      !pretend)) && !is_bare_repository()) {\n+\t      !pretend)) && !repo_is_bare(r)) {\n \t\tstruct ref *ref = remote_refs;\n \t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n \t\tstruct oid_array commits = OID_ARRAY_INIT;\ndiff --git a/worktree.c b/worktree.c\nindex 77ff484d3ec..c9d5b228959 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -85,8 +85,8 @@ static struct worktree *get_main_worktree(int skip_reading_head)\n \t * This means that worktree->is_bare may be set to 0 even if the main\n \t * worktree is configured to be bare.\n \t */\n-\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n-\t\tis_bare_repository();\n+\n+\tworktree->is_bare = the_repository->is_bare_cfg == 1;\n \tworktree->is_current = is_current_worktree(worktree);\n \tif (!skip_reading_head)\n \t\tadd_head_info(worktree);\n-- \ngitgitgadget\n\n"},{"id":"506758","messageId":"19c97feb06ef2f01f89b462678fe304b58fcba37.1730926082.git.gitgitgadget@gmail.com","threadId":"62461","inReplyTo":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","subject":"[PATCH 2/3] setup: initialize is_bare_cfg","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-06T20:48:01Z","receivedAt":"2024-11-06T20:48:08Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <jcai@gitlab.com>\n\nA subsequent commit will BUG() when the is_bare_cfg member is\nuninitialized. Since there are still some codepaths that initializing the\nis_bare_cfg variable, initialize them.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n setup.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 6bc4aef3a8b..5680976c598 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -741,6 +741,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \n \tif (verify_repository_format(candidate, &err) < 0) {\n \t\tif (nongit_ok) {\n+\t\t\tthe_repository->is_bare_cfg = 1;\n \t\t\twarning(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t\t*nongit_ok = -1;\n@@ -1017,6 +1018,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\tif (nongit_ok) {\n \t\t\t*nongit_ok = 1;\n \t\t\tfree(gitfile);\n+\t\t\tthe_repository->is_bare_cfg = 0;\n \t\t\treturn NULL;\n \t\t}\n \t\tdie(_(\"not a git repository: '%s'\"), gitdirenv);\n@@ -1069,6 +1071,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \n \t/* set_git_work_tree() must have been called by now */\n \tworktree = repo_get_work_tree(the_repository);\n+\tthe_repository->is_bare_cfg = 0;\n \n \t/* both repo_get_work_tree() and cwd are already normalized */\n \tif (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */\n@@ -1125,6 +1128,9 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \n \t/* #0, #1, #5, #8, #9, #12, #13 */\n \tset_git_work_tree(\".\");\n+\n+\tif (the_repository->is_bare_cfg < 0)\n+\t\tthe_repository->is_bare_cfg = 0;\n \tif (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))\n \t\tset_git_dir(gitdir, 0);\n \tinside_git_dir = 0;\n@@ -1767,6 +1773,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n \t\t\t    DEFAULT_GIT_DIR_ENVIRONMENT);\n \t\t*nongit_ok = 1;\n+\t\tthe_repository->is_bare_cfg = 1;\n \t\tbreak;\n \tcase GIT_DIR_HIT_MOUNT_POINT:\n \t\tif (!nongit_ok)\n-- \ngitgitgadget\n\n"},{"id":"506759","messageId":"749ba7f52086f2b444a9e073c431d7d4f4c7204e.1730926082.git.gitgitgadget@gmail.com","threadId":"62461","inReplyTo":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","subject":"[PATCH 3/3] repository: BUG when is_bare_cfg is not initialized","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-06T20:48:02Z","receivedAt":"2024-11-06T20:48:09Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <jcai@gitlab.com>\n\nThe is_bare_cfg member of the repository struct should be properly\ninitiated when setting up a repository. BUG when repo_is_bare() sees\nthat the flag has not been set.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n repository.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/repository.c b/repository.c\nindex 96608058b61..cd1d59ea1b9 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -464,5 +464,7 @@ int repo_hold_locked_index(struct repository *repo,\n int repo_is_bare(struct repository *repo)\n {\n \t/* if core.bare is not 'false', let's see if there is a work tree */\n+\tif (repo->is_bare_cfg < 0 )\n+\t\tBUG(\"is_bare_cfg unspecified\");\n \treturn repo->is_bare_cfg && !repo_get_work_tree(repo);\n }\n-- \ngitgitgadget\n"},{"id":"506780","messageId":"xmqqv7wzsijc.fsf@gitster.g","threadId":"62461","inReplyTo":"3d341a9ae4ef1d2776734fa82a45913f91e6083c.1730926082.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-07T05:46:15Z","receivedAt":"2024-11-07T05:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> The is_bare_repository_cfg global variable is used for storing a bare\n> repository setting, either through the config, an env var, or the\n> commandline.\n\nI found it curious that the above enumeration does not include the\ncase where we go through the repository discovery process and find\nthat we are in a bare repository.  Looking at the original\nimplementation of is_bare_repository() call, we do check if the\ndirectory structure does have a working tree when these three\nsources you listed above say \"we are bare\" or \"we do not know yet\".\n\nSo it would be be helpful if we made these two points\n\n - The above enumeration is not meant to be exhausitive.\n\n - The answer to anybody who asks \"is this repository bare?\" is more\n   subtle than just reading the variable.\n\nclear to readers.\n\n> This variable is global, and hence introduces global state\n> everywhere it is used.\n>\n> In order to reduce global state, add a member to the repository struct\n> to keep track of the setting there. For now, the_repository is what's\n> used to set the member, which still represents global state. However,\n> there is a parallel effort to replace calls to the_repository with a\n> repository struct that is passed into builtins, see [1]. Hence, this\n> change will help the overall effort in reducing global state.\n\nOK.\n\n> diff --git a/attr.c b/attr.c\n> index c605d2c1703..053cd59af26 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -716,7 +716,7 @@ static enum git_attr_direction direction;\n>  \n>  void git_attr_set_direction(enum git_attr_direction new_direction)\n>  {\n> -\tif (is_bare_repository() && new_direction != GIT_ATTR_INDEX)\n> +\tif (repo_is_bare(the_repository) && new_direction != GIT_ATTR_INDEX)\n>  \t\tBUG(\"non-INDEX attr direction in a bare repo\");\n\nSo everybody called is_bare_repository() which implicitly relied on\nthe global variable now calls repo_is_bare() on the_repository,\nwhere the new member in the struct serves the purpose of the old\nglobal variable.\n\nThis replacement to repo_is_bare(the_repository) from\nis_bare_repository() is a recurring pattern in this patch, so I'll\nremove them from my quoting.\n\nI've used coccinelle to apply this semantic patchlet\n\n    - is_bare_repository()\n    + repo_is_bare(the_repository)\n\nand then compared the result with applying this patch to see what\nelse this patch contains, so I can comment on them.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 59fcb317a68..80b594c6011 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1415,7 +1415,7 @@ int cmd_clone(int argc,\n>  \t\trepo_clear(the_repository);\n>  \n>  \t\t/* At this point, we need the_repository to match the cloned repo. */\n> -\t\tif (repo_init(the_repository, git_dir, work_tree))\n> +\t\tif (repo_init(the_repository, git_dir, work_tree, -1))\n>  \t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n>  \t\telse if (fetch_bundle_uri(the_repository, bundle_uri, &has_heuristic))\n>  \t\t\twarning(_(\"failed to fetch objects from bundle URI '%s'\"),\n> @@ -1446,7 +1446,7 @@ int cmd_clone(int argc,\n>  \t\t\trepo_clear(the_repository);\n>  \n>  \t\t\t/* At this point, we need the_repository to match the cloned repo. */\n> -\t\t\tif (repo_init(the_repository, git_dir, work_tree))\n> +\t\t\tif (repo_init(the_repository, git_dir, work_tree, -1))\n>  \t\t\t\twarning(_(\"failed to initialize the repo, skipping bundle URI\"));\n>  \t\t\telse if (fetch_bundle_list(the_repository,\n>  \t\t\t\t\t\t   transport->bundles))\n\nOK, so our repo_init() now takes one extra parameter.  We'll see what\nthe new parameter means when we look at the changes to repository.c.\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 7e00d57d654..901bf30b508 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -89,7 +89,7 @@ int cmd_init_db(int argc,\n>  \tconst struct option init_db_options[] = {\n>  \t\tOPT_STRING(0, \"template\", &template_dir, N_(\"template-directory\"),\n>  \t\t\t\tN_(\"directory from which templates will be used\")),\n> -\t\tOPT_SET_INT(0, \"bare\", &is_bare_repository_cfg,\n> +\t\tOPT_SET_INT(0, \"bare\", &the_repository->is_bare_cfg,\n>  \t\t\t\tN_(\"create a bare repository\"), 1),\n>  \t\t{ OPTION_CALLBACK, 0, \"shared\", &init_shared_repository,\n>  \t\t\tN_(\"permissions\"),\n\nAs you said, this depends on the fact that the_repository is a\npointer pointing at the static singleton variable the_repo at the\ncompile time, so while it is already safe to take the address of\nthe_repository->is_bare_cfg to prepare the array of options here, we\nhaven't really solved the \"we shouldn't be using this global\nvariable\" yet.  But we can go one step at a time.\n\nThe remaining hunks in the file now all access the \"global variable\"\nvia the_repository pointer, but the fact remains that the address of\nthe thing being accessed is determined at the compile time, so it is\njust like accessing a global variable.\n\nWhich is naturally expected.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index b6b5f1ebde7..7bff99bf08f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1591,7 +1591,7 @@ static int add_possible_reference_from_superproject(\n>  \t\tstruct strbuf err = STRBUF_INIT;\n>  \t\tstrbuf_add(&sb, odb->path, len);\n>  \n> -\t\tif (repo_init(&alternate, sb.buf, NULL) < 0)\n> +\t\tif (repo_init(&alternate, sb.buf, NULL, the_repository->is_bare_cfg) < 0)\n\nOK.  I do not recall what the original repo_init() did, but I\npresume that it initialized the new one depending on what the global\nvariable said.  We now propagate the setting from the superproject\ndown to the submodule, which amounts to the same thing but probably\nis better as the \"inheritance\" is more explicitly visible here?\n\n> diff --git a/config.c b/config.c\n> index a11bb85da30..c1b14c89947 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1441,7 +1441,7 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t}\n>  \n>  \tif (!strcmp(var, \"core.bare\")) {\n> -\t\tis_bare_repository_cfg = git_config_bool(var, value);\n> +\t\tthe_repository->is_bare_cfg = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n\nOK.  This is the same as what init-db did.\n\n> diff --git a/dir.c b/dir.c\n> index e3ddd5b5296..c995668e54c 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -4008,7 +4008,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,\n>  \tconst struct submodule *sub;\n>  \n>  \t/* If the submodule has no working tree, we can ignore it. */\n> -\tif (repo_init(&subrepo, sub_gitdir, sub_worktree))\n> +\tif (repo_init(&subrepo, sub_gitdir, sub_worktree, the_repository->is_bare_cfg))\n>  \t\treturn;\n\nSame logic for submodule inheriting from the superproject?\n\n> diff --git a/environment.c b/environment.c\n> index a2ce9980818..9af20d5e34e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -34,7 +34,6 @@ int has_symlinks = 1;\n>  int minimum_abbrev = 4, default_abbrev = -1;\n>  int ignore_case;\n>  int assume_unchanged;\n> -int is_bare_repository_cfg = -1; /* unspecified */\n\nThis is now gone.  We'll see a corresponding change in repository.c\nwhere the_repo instance is initialized, I presume.\n\n> @@ -146,12 +145,6 @@ const char *getenv_safe(struct strvec *argv, const char *name)\n>  \treturn argv->v[argv->nr - 1];\n>  }\n>  \n> -int is_bare_repository(void)\n> -{\n> -\t/* if core.bare is not 'false', let's see if there is a work tree */\n> -\treturn is_bare_repository_cfg && !repo_get_work_tree(the_repository);\n> -}\n\nThis is now gone, and we'll see a corresponding change in\nrepository.c, I presume.\n\nIt is somewhat curious that in a repository where core.bare says\ntrue, we countermand it if we cannot figure out where its working\ntree is and say \"core.bare is lying; we are in a bare repository\".\n\nThe curiousity is not the fault of this patch, of course.  The\nupdated code in repository.c would hopefully have the same\ncuriousity (or we'd be looking at an unintended behaviour change,\nif it didn't).\n\n> diff --git a/environment.h b/environment.h\n> index 923e12661e1..23f29a4df05 100644\n> --- a/environment.h\n> +++ b/environment.h\n> @@ -144,8 +144,7 @@ void set_shared_repository(int value);\n>  int get_shared_repository(void);\n>  void reset_shared_repository(void);\n>  \n> -extern int is_bare_repository_cfg;\n> -int is_bare_repository(void);\n> +int is_bare_repository(struct repository *repo);\n\nCurious.  I somehow thought is_bare_repository() will be gone, and\neverybody is supposed to call repo_is_bare(the_repository), instead.\n\nWhat makes a caller pick one over the other?\n\n\n> diff --git a/git.c b/git.c\n> index c2c1b8e22c2..c8ed29b2295 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -251,7 +251,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n>  \t\t\t\t*envchanged = 1;\n>  \t\t} else if (!strcmp(cmd, \"--bare\")) {\n>  \t\t\tchar *cwd = xgetcwd();\n> -\t\t\tis_bare_repository_cfg = 1;\n> +\t\t\tthe_repository->is_bare_cfg = 1;\n\nOK.  The same as how init-db and config now access the new member of\nthe global singleton the_repository instead of the global variable.\n\n> diff --git a/repository.c b/repository.c\n> index f988b8ae68a..96608058b61 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -25,7 +25,9 @@\n>  extern struct repository *the_repository;\n>  \n>  /* The main repository */\n> -static struct repository the_repo;\n> +static struct repository the_repo = {\n> +\t.is_bare_cfg = -1,\n> +};\n\nOK, this is just as expected by reading the patch so far.\n\n> @@ -263,10 +265,13 @@ static int read_and_verify_repository_format(struct repository_format *format,\n>  /*\n>   * Initialize 'repo' based on the provided 'gitdir'.\n>   * Return 0 upon success and a non-zero value upon failure.\n> + * is_bare can be passed to indicate whether or not the repository should be\n> + * treated as bare when repo_init() is used to initiate a secondary repository.\n\n\"initiate\" -> \"initialize\" perhaps?\n\n>  int repo_init(struct repository *repo,\n>  \t      const char *gitdir,\n> -\t      const char *worktree)\n> +\t      const char *worktree,\n> +\t      int is_bare)\n>  {\n>  \tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n>  \tmemset(repo, 0, sizeof(*repo));\n> @@ -283,6 +288,8 @@ int repo_init(struct repository *repo,\n>  \trepo_set_compat_hash_algo(repo, format.compat_hash_algo);\n>  \trepo_set_ref_storage_format(repo, format.ref_storage_format);\n>  \trepo->repository_format_worktree_config = format.worktree_config;\n> +\tif (is_bare > 0)\n> +\t\trepo->is_bare_cfg = is_bare;\n\nWhen repo_init() is called with anything other than &the_repo, who\ninitializes repo->is_bare_cfg?  If the answer is \"nobody\", shouldn't\nthis function be doing something like\n\n\trepo->is_bare_cfg = (0 <= is_bare) ? is_bare : -1;\n\nwhich actually amounts to an unconditional\n\n\trepo->is_bare_cfg = is_bare;\n\nas is_bare can only take one of (-1, 0, 1).\n\nPerhaps I am missing some subtleties in the original construction\nyou wrote?  You leave repo->is_bare_cfg unset when is_bare parameter\nexplicitly says \"false\", which I suspect might be related to the\nsource of confusion I am having.\n\n> +int repo_is_bare(struct repository *repo)\n> +{\n> +\t/* if core.bare is not 'false', let's see if there is a work tree */\n> +\treturn repo->is_bare_cfg && !repo_get_work_tree(repo);\n> +}\n\nThe curiosity we saw in the original implementation of\nis_bare_repository() above is faithfully reproduced, which is good.\n\n> diff --git a/repository.h b/repository.h\n> index 24a66a496a6..c243653492b 100644\n> --- a/repository.h\n> +++ b/repository.h\n> @@ -153,6 +153,14 @@ struct repository {\n>  \n>  \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n>  \tunsigned different_commondir:1;\n> +\n> +\t/*\n> +\t * Indicates if the repository is set to be treated as a bare repository,\n> +\t * through a command line argument, configuration, or environment\n> +\t * variable.\n> +\t * -1 means unspecified, 0 indicates non-bare, and 1 indicates bare.\n> +\t */\n> +\tint is_bare_cfg;\n>  };\n\nI am very happy with the above phrasing.  The member tells us what\nthe repo is \"set to be treated as\", which implies that the code does\na bit more on top of that setting.\n\n> diff --git a/scalar.c b/scalar.c\n> index ac0cb579d3f..c2ec1f3e745 100644\n> --- a/scalar.c\n> +++ b/scalar.c\n> @@ -722,7 +722,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n>  \n>  \t\tgit_config_clear();\n>  \n> -\t\tif (repo_init(&r, gitdir.buf, commondir.buf))\n> +\t\tif (repo_init(&r, gitdir.buf, commondir.buf, the_repository->is_bare_cfg))\n>  \t\t\tgoto loop_end;\n\nGiven this caller, if is_bare_cfg of the current state says \"I am\nset to be treated as a non-bare repository\" by having 0, shouldn't\nrepo_init() copy it to the repository at r?  IOW, this yells at me\nsaying that repo_init() patch we saw earlier is somewhat buggy.\n\n> diff --git a/setup.c b/setup.c\n> index 7b648de0279..6bc4aef3a8b 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -766,8 +766,8 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n>  \n>  \tif (!has_common) {\n>  \t\tif (candidate->is_bare != -1) {\n> -\t\t\tis_bare_repository_cfg = candidate->is_bare;\n> -\t\t\tif (is_bare_repository_cfg == 1)\n> +\t\t\tthe_repository->is_bare_cfg = candidate->is_bare;\n> +\t\t\tif (the_repository->is_bare_cfg == 1)\n>  \t\t\t\tinside_work_tree = -1;\n\nOK, this is as expected.\n\nAll other hunks to this file, except for the last one, follow the\nsame pattern as init-db and config to access the member of the\nsingleton struct instead of global variable.  And the last one is to\ncall repo_is_bare(the_repository) instead of is_bare_repository().\n\nMakes sense.\n\n> diff --git a/transport.c b/transport.c\n> index 47fda6a7732..d72b8380846 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1428,7 +1428,7 @@ int transport_push(struct repository *r,\n>  \n>  \tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n>  \t\t      TRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n> -\t    !is_bare_repository()) {\n> +\t    !repo_is_bare(r)) {\n>  \t\tstruct ref *ref = remote_refs;\n>  \t\tstruct oid_array commits = OID_ARRAY_INIT;\n>  \n> @@ -1455,7 +1455,7 @@ int transport_push(struct repository *r,\n>  \tif (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||\n>  \t     ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n>  \t\t\tTRANSPORT_RECURSE_SUBMODULES_ONLY)) &&\n> -\t      !pretend)) && !is_bare_repository()) {\n> +\t      !pretend)) && !repo_is_bare(r)) {\n>  \t\tstruct ref *ref = remote_refs;\n>  \t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n>  \t\tstruct oid_array commits = OID_ARRAY_INIT;\n\nOK, these are better than the mechanical \"singleton the_repository\nis the new home for the global\".  It makes it even more important\nfor us to answer \"Who initializes the repository r?  Is is_bare_cfg\ninitialized to -1 just like the_repo.is_bare_cfg is?  Is repo_init()\ndoing the right thing to update it by doing only when it is set to 1\nbut ignoring -1 and 0 as incoming parameter?\" questions, though.\n\n> diff --git a/worktree.c b/worktree.c\n> index 77ff484d3ec..c9d5b228959 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -85,8 +85,8 @@ static struct worktree *get_main_worktree(int skip_reading_head)\n>  \t * This means that worktree->is_bare may be set to 0 even if the main\n>  \t * worktree is configured to be bare.\n>  \t */\n> -\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n> -\t\tis_bare_repository();\n> +\n> +\tworktree->is_bare = the_repository->is_bare_cfg == 1;\n\nIf this changes the behaviour subtly without explaining, it needs to\nbe justified, I suspect.\n\nWe used to pay attention to what is_bare_repository() says, which is\na bit more than \"set to bbe treated as\" with config.  We no longer\ndo so, and also unconfigured case is always treated as non-bare.\n\n"},{"id":"506781","messageId":"xmqq8qtvsgpu.fsf@gitster.g","threadId":"62461","inReplyTo":"19c97feb06ef2f01f89b462678fe304b58fcba37.1730926082.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] setup: initialize is_bare_cfg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-07T06:25:33Z","receivedAt":"2024-11-07T06:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: John Cai <jcai@gitlab.com>\n>\n> A subsequent commit will BUG() when the is_bare_cfg member is\n> uninitialized. Since there are still some codepaths that initializing the\n> is_bare_cfg variable, initialize them.\n>\n> Signed-off-by: John Cai <johncai86@gmail.com>\n> ---\n>  setup.c | 7 +++++++\n>  1 file changed, 7 insertions(+)\n\nI am not sure about the wisdom of this step (and the next one).\nBefore this step, it used to be that the global variable (or\nthe_repository->is_bare_cfg) can be inspected to see if there is an\nexplicit \"set to be treated as\", or nobody told us if the repository\nought to be bare (or not).  With this and the next step, that is no\nlonger possible, yet we still do the \"core.bare says it is either\ntrue or unconfigured, so we ask repo_get_work_tree() and it returns\nNULL, so it is bare\", which feels awfully inconsistent.  Especially\nthe change from the next patch\n\n> diff --git a/repository.c b/repository.c\n> index 96608058b61..cd1d59ea1b9 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -464,5 +464,7 @@ int repo_hold_locked_index(struct repository *repo,\n>  int repo_is_bare(struct repository *repo)\n>  {\n>  \t/* if core.bare is not 'false', let's see if there is a work tree */\n> +\tif (repo->is_bare_cfg < 0 )\n> +\t\tBUG(\"is_bare_cfg unspecified\");\n>  \treturn repo->is_bare_cfg && !repo_get_work_tree(repo);\n>  }\n\nthe returned value does not make much sense anymore.  One half of it\nused to be \"if not configured, we can ask if there is worktree and\nthe lack of one by definition means we are bare\", which made perfect\nsense, but now what remains is \"the configuration says it is, but\nwhen we ask if there is a worktree, there is, so it is not bare\nafter all\", which is somewhat dubious.\n\nAnd if the goal of steps 2 & 3 is to redefine what is_bare_cfg means\nand make it \"this is the only thing we need to check if the\nrepository is bare\" (which by itself is not a bad thing), shouldn't\nthe checking of worktree be done where the code assigns true to the\nrepo->is_bare_cfg, no?\n\n> diff --git a/setup.c b/setup.c\n> index 6bc4aef3a8b..5680976c598 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -741,6 +741,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n>  \n>  \tif (verify_repository_format(candidate, &err) < 0) {\n>  \t\tif (nongit_ok) {\n> +\t\t\tthe_repository->is_bare_cfg = 1;\n\nIt is unclear how we can be certain that we are looking at a bare\nrepository in this case.  We do not even understand the repository\nformat, GIT_DIR we were given to decide which file called \"config\"\nmay not even be a repository.  We are losing a bit of information\n(i.e. nobody has told us if we ought to treat the repository as a\nbare one, or a non-bare one\") by overriding the value here.\n\n> @@ -1017,6 +1018,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n>  \t\tif (nongit_ok) {\n>  \t\t\t*nongit_ok = 1;\n>  \t\t\tfree(gitfile);\n> +\t\t\tthe_repository->is_bare_cfg = 0;\n>  \t\t\treturn NULL;\n\nDitto.\n\n> @@ -1069,6 +1071,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n>  \n>  \t/* set_git_work_tree() must have been called by now */\n>  \tworktree = repo_get_work_tree(the_repository);\n> +\tthe_repository->is_bare_cfg = 0;\n\nWhat if worktree is NULL?  Wouldn't it be more meaningful to say\nis_bare_cfg is true in such a case?\n\n> @@ -1125,6 +1128,9 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n>  \n>  \t/* #0, #1, #5, #8, #9, #12, #13 */\n>  \tset_git_work_tree(\".\");\n> +\n> +\tif (the_repository->is_bare_cfg < 0)\n> +\t\tthe_repository->is_bare_cfg = 0;\n\nOK.  We did discovery, is_bare_cfg did not say true (it would have\nreturned before we got here if is_bare_cfg were set to true).  We\ndecided to treat the current directory as the top of the working tree,\nso by definition, we are not treating the repository as bare.\n\nBut this makes me wonder what should happen\nthe_repository->is_bare_cfg is already set to true.  Shouldn't that\nbe a BUG()?\n\n> @@ -1767,6 +1773,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\t\tdie(_(\"not a git repository (or any of the parent directories): %s\"),\n>  \t\t\t    DEFAULT_GIT_DIR_ENVIRONMENT);\n>  \t\t*nongit_ok = 1;\n> +\t\tthe_repository->is_bare_cfg = 1;\n\nThis is not bare nor non-bare---simply we did not find any usable\ngit repository, and we lose the single bit of information \"nobody\ntold us to treat the repository as bare or non-bare\".\n\nNot that the loss of information is a huge deal.  But having to make\nan arbitrary choice like the above (and similar ones in previous\nhunks where we didn't have any repository to begin with) is an\nindication that the entire \"is_bare_cfg must mean if our repository\nis bare or non-bare\" premise patch 3/3 wants to enforce may be\nmisguided, I am afraid.\n\n>  \t\tbreak;\n>  \tcase GIT_DIR_HIT_MOUNT_POINT:\n>  \t\tif (!nongit_ok)\n"},{"id":"506804","messageId":"ZyzlBZnL-K3S7Env@ArchLinux","threadId":"62461","inReplyTo":"xmqqv7wzsijc.fsf@gitster.g","subject":"Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-11-07T16:04:21Z","receivedAt":"2024-11-07T16:03:58Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Nov 07, 2024 at 02:46:15PM +0900, Junio C Hamano wrote:\n\n[snip]\n\n> >  int repo_init(struct repository *repo,\n> >  \t      const char *gitdir,\n> > -\t      const char *worktree)\n> > +\t      const char *worktree,\n> > +\t      int is_bare)\n> >  {\n> >  \tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n> >  \tmemset(repo, 0, sizeof(*repo));\n> > @@ -283,6 +288,8 @@ int repo_init(struct repository *repo,\n> >  \trepo_set_compat_hash_algo(repo, format.compat_hash_algo);\n> >  \trepo_set_ref_storage_format(repo, format.ref_storage_format);\n> >  \trepo->repository_format_worktree_config = format.worktree_config;\n> > +\tif (is_bare > 0)\n> > +\t\trepo->is_bare_cfg = is_bare;\n> \n> When repo_init() is called with anything other than &the_repo, who\n> initializes repo->is_bare_cfg?\n\nI also want to ask this question. Actually, I feel quite strange about\nwhy we need to add a new parameter `is_bare` to `repo_init` function.\n\nFor this call:\n\n    repo_init(the_repository, git_dir, work_tree, -1);\n\nWe add a new field \"is_bare_cfg\" to the \"struct repository\". So, at now,\n`the_repository` variable should contain the information about whether\nthe repo is bare(1), is not bare(0) or unknown(-1). However, in this\ncall, we pass \"-1\" to the parameter `is_bare` for \"repo_init\" function.\n\nWhen I first look at this code, I have thought that we will set\n\"repo->is_bare_cfg = -1\" to indicate that we cannot tell whether the\nrepo is bare or not. But it just sets the \"repo->is_bare_cfg = is_bare\"\nif `bare > 0`. Junio has already commented on this.\n\nThis raises a question: why we need to set up `is_bare_cfg` in the\n`repo_init` function? I guess this is because we need to set up other\n\"struct repository\" parameter like the following:\n\n    if (repo_init(&alternate, sb.buf, NULL, the_repository->is_bare_cfg) < 0)\n\nAnd I think it's better for us to use the following way.\n\n    alternate->is_bare_cfg = the_repository->is_bare_cfg;\n    if (repo_init(&alternate, sb.buf, NULL))\n\nAnd we may create a function called `repo_copy_settings` to set up the\ncommon setting inherited from an existing repo:\n\n    repo_copy_settings(alternate, the_repository);\n    if (repo_init(&alternate, sb.buf, NULL))\n\nI agree that we could put `is_bare_cfg` to \"struct repository *\". But I\ndon't agree with the idea that we need to pass `is_bare` to `repo_init`.\nI think we should know whether the repo is bare or not before calling\n`repo_init`. And from my understanding, this is what we are doing now.\n\nAlso, I think we may add a enum type instead of using (-1, 0, 1).\n(However, this is not the main point of this patch).\n\nThanks,\nJialuo\n"},{"id":"506818","messageId":"xmqqjzdeqzzk.fsf@gitster.g","threadId":"62461","inReplyTo":"ZyzlBZnL-K3S7Env@ArchLinux","subject":"Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-08T01:24:31Z","receivedAt":"2024-11-08T01:24:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n> I also want to ask this question. Actually, I feel quite strange about\n> why we need to add a new parameter `is_bare` to `repo_init` function.\n>\n> For this call:\n>\n>     repo_init(the_repository, git_dir, work_tree, -1);\n>\n> We add a new field \"is_bare_cfg\" to the \"struct repository\". So, at now,\n> `the_repository` variable should contain the information about whether\n> the repo is bare(1), is not bare(0) or unknown(-1). However, in this\n> call, we pass \"-1\" to the parameter `is_bare` for \"repo_init\" function.\n\nIsn't this merely trying to be faithful to the original to avoid\nunintended behaviour change?  We initialize the global variable\nis_bare_repository_cfg to unspecified(-1) in the original, and\nfor a rewrite to move the global to a member in the singleton\ninstance of the_repo, it would need to be able to do the same.\n\nAnd for callers of repo_init() that prepares _another_ in-core\nrepository instance, which is different from the_repository, because\nthe original has a process-wide singleton global variable, copying\nthe value from the_repository->is_bare to a newly initialized one\nwould hopefully give us the most faithful rewrite to avoid\nunintended behaviour change.\n\nAt least, that is how I understood why the patch does it this way.\nAs you noticed, too, there are ...\n\n> When I first look at this code, I have thought that we will set\n> \"repo->is_bare_cfg = -1\" to indicate that we cannot tell whether the\n> repo is bare or not. But it just sets the \"repo->is_bare_cfg = is_bare\"\n> if `bare > 0`. Junio has already commented on this.\n\n... places in the updated code that makes it unclear what the\nis_bare member really means.  The corresponding global variable used\nto be \"this is what we were told by config or env or command line\",\nbut it is unclear, with conditional assignments like the above, what\nit means in the updated code.\n\nThanks.\n"},{"id":"507396","messageId":"ZziLlXln3NLPaHc-@ArchLinux","threadId":"62461","inReplyTo":"xmqqjzdeqzzk.fsf@gitster.g","subject":"Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-11-16T12:09:57Z","receivedAt":"2024-11-16T12:09:54Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Nov 08, 2024 at 10:24:31AM +0900, Junio C Hamano wrote:\n> shejialuo <shejialuo@gmail.com> writes:\n> \n> > I also want to ask this question. Actually, I feel quite strange about\n> > why we need to add a new parameter `is_bare` to `repo_init` function.\n> >\n> > For this call:\n> >\n> >     repo_init(the_repository, git_dir, work_tree, -1);\n> >\n> > We add a new field \"is_bare_cfg\" to the \"struct repository\". So, at now,\n> > `the_repository` variable should contain the information about whether\n> > the repo is bare(1), is not bare(0) or unknown(-1). However, in this\n> > call, we pass \"-1\" to the parameter `is_bare` for \"repo_init\" function.\n> \n> Isn't this merely trying to be faithful to the original to avoid\n> unintended behaviour change?  We initialize the global variable\n> is_bare_repository_cfg to unspecified(-1) in the original, and\n> for a rewrite to move the global to a member in the singleton\n> instance of the_repo, it would need to be able to do the same.\n> \n> And for callers of repo_init() that prepares _another_ in-core\n> repository instance, which is different from the_repository, because\n> the original has a process-wide singleton global variable, copying\n> the value from the_repository->is_bare to a newly initialized one\n> would hopefully give us the most faithful rewrite to avoid\n> unintended behaviour change.\n> \n\nYes, I agree that this is the most faithful way to make sure the\nconsistency when we want to create a new `repo` instead of letting the\ncaller do this itself.\n\nSo, I think what I feel strange is that we need to do this assignment.\nBecause we make a global variable not global by incorporating this into\n\"struct repository *\", we have to maintain this state whenever we create\na new \"repo\".\n\nIt lets me think whether we should place \"is_bare_cfg\" into \"struct\nrepository\" in the first place. I will explain why in the later\ncomments.\n\n> At least, that is how I understood why the patch does it this way.\n> As you noticed, too, there are ...\n> \n> > When I first look at this code, I have thought that we will set\n> > \"repo->is_bare_cfg = -1\" to indicate that we cannot tell whether the\n> > repo is bare or not. But it just sets the \"repo->is_bare_cfg = is_bare\"\n> > if `bare > 0`. Junio has already commented on this.\n> \n> ... places in the updated code that makes it unclear what the\n> is_bare member really means.  The corresponding global variable used\n> to be \"this is what we were told by config or env or command line\",\n> but it is unclear, with conditional assignments like the above, what\n> it means in the updated code.\n> \n\nYes, John has changed the corresponding code paths by setting the global\nvariable \"the_repository->is_bare_cfg\". So, we will refactor this later.\n\nIn the previous days, Kousik wanted to make \"builtin/mailinfo\" not to\nreply on \"the_repository\". I have commented in\n\n    https://lore.kernel.org/git/Zw6SsUyZ0oA0XqMK@ArchLinux/\n\nIn this thread, I do not agree that we should not incorporate the global\nvariables in \"git_commit_encoding\" and \"git_log_output_encoding\" in\n\"environment.c\" into \"struct repository *\" because we could use these\ntwo configs outside of the repo.\n\nSo, I don't think it's a good idea to put into \"is_bare_cfg\" into\n\"struct repository\". Put it further more, we should not put the global\nvariables in \"struct repository\" structure for the following reasons:\n\n  1. These variables are used across the whole lifecycle. Not just only\n     related to the repository. Some variables could be used outside of\n     the repo.\n  2. Currently, the config functions which set up these variables don't\n     have parameters to access the \"struct repository *\". Of course, we\n     could add the parameter, but as 1 shows, some variables could be\n     used outside of the repo. We may need many efforts for such\n     situation.\n  3. We need to maintain the consistency if we create a new \"struct\n     repository\", because we will make global variables not global.\n\nSo, in my perspective, we may just create a new structure called \"struct\nenv\" to incorporate all these variables in \"environment.c\" just like\nwhat we have done for \"struct repository *\". But we also introduced\nanother overhead, we may pass this structure to every function when\nsetting up.\n\n> Thanks.\n\nThanks,\nJialuo\n"},{"id":"508157","messageId":"xmqqy116xvr3.fsf@gitster.g","threadId":"62461","inReplyTo":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Remove is_bare_repository_cfg global state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T08:08:32Z","receivedAt":"2024-11-26T08:08:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This patch series removes the global state introduced by the\n> is_bare_repository_cfg variable by moving it into the repository struct.\n> Most of the refactor is done by patch 1. Patch 2 initializes the member in\n> places that left it unInitialized, while patch 3 adds a safety measure by\n> BUG()ing when the variable has not been properly initialized.\n\nI think these patches go in the right direction in general, but the\ntopic hasn't seen much activity for a few weeks since they received\nreview messages.  Is a new revision being worked on, or is the topic\nbeing backburnered?\n\nThanks.\n"},{"id":"509005","messageId":"xmqqzfl1hl52.fsf@gitster.g","threadId":"62461","inReplyTo":"pull.1826.git.git.1730926082.gitgitgadget@gmail.com","subject":"(RFH Windows breakage) Re: [PATCH 0/3] Remove is_bare_repository_cfg global state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-11T23:09:45Z","receivedAt":"2024-12-11T23:09:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This patch series removes the global state introduced by the\n> is_bare_repository_cfg variable by moving it into the repository struct.\n> Most of the refactor is done by patch 1. Patch 2 initializes the member in\n> places that left it unInitialized, while patch 3 adds a safety measure by\n> BUG()ing when the variable has not been properly initialized.\n>\n> John Cai (3):\n>   git: remove is_bare_repository_cfg global variable\n>   setup: initialize is_bare_cfg\n>   repository: BUG when is_bare_cfg is not initialized\n\nWe've been seeing a job \"win test (5)\" fail on 'seen' for a while,\nand I happened to have rebuilt 'seen' without this topic (first by\naccident) and the job started passing.\n\nThe topic coming from GGG, I'd assume that it byitself will pass the\ntests (including Windows ones), so I suspect it is some interaction\nwith other topics in 'seen'.\n\nAs I do not have Windows environment to test and dig into any\nproblem, often pushing 'seen' with suspect topic(s) removed is the\nonly way for me to isolate which topic might be causing a problem,\nand after doing so, I'll have to leave it up to the author of the\ntopic to dig further with help from others.\n\n(failing) https://github.com/git/git/actions/runs/12279217687/job/34263221584\n(passing) https://github.com/git/git/actions/runs/12286174648/job/34286039276\n\nThe difference between these is that the former (failing) one has\nthis topic with three patches merged at the tip of 'seen', and the\nlatter (passing) one is the result of tentatively dropping this\ntopic from the CI run.\n\nThanks.\n"}]}