{"thread":{"id":"65305","subject":"[Question] check_repository_format_gently() is not side-effect-free","startedAt":"2026-03-19T17:45:32Z","lastAt":"2026-03-20T16:26:29Z","messageCount":6,"participants":["Tian Yuchen","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"539412","messageId":"c0bb931a-3ee6-416b-8ceb-9fab013a621e@malon.dev","threadId":"65305","inReplyTo":null,"subject":"[Question] check_repository_format_gently() is not side-effect-free","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-19T17:45:25Z","receivedAt":"2026-03-19T17:45:32Z","isPatch":false,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Git community,\n\nIn setup.c:\n\n> static const char *setup_explicit_git_dir(const char *gitdirenv,\n> \t\t\t\t\t  struct strbuf *cwd,\n> \t\t\t\t\t  struct repository_format *repo_fmt,\n> \t\t\t\t\t  int *nongit_ok)\n> {\n> \tconst char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);\n> \tconst char *worktree;\n> \tchar *gitfile;\n> \tint offset;\n> \n> \tif (PATH_MAX - 40 < strlen(gitdirenv))\n> \t\tdie(_(\"'$%s' too big\"), GIT_DIR_ENVIRONMENT);\n> \n> \tgitfile = (char*)read_gitfile(gitdirenv);\n> \tif (gitfile) {\n> \t\tgitfile = xstrdup(gitfile);\n> \t\tgitdirenv = gitfile;\n> \t}\n> \n> \tif (!is_git_directory(gitdirenv)) {\n> \t\tif (nongit_ok) {\n> \t\t\t*nongit_ok = 1;\n> \t\t\tfree(gitfile);\n> \t\t\treturn NULL;\n> \t\t}\n> \t\tdie(_(\"not a git repository: '%s'\"), gitdirenv);\n> \t}\n> \n\n...\n\n> \tif (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {\n> \t\tfree(gitfile);\n> \t\treturn NULL;\n> \t}\n\nI've noticed that this function behaves rather strangely. If I am \ncorrect, The primary purpose of this function is to verify whether the \nGit binary understands the current repository format.\n\nThe implementation is as follows:\n\n> static int check_repository_format_gently(const char *gitdir, struct repository_format *candidate, int *nongit_ok)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstruct strbuf err = STRBUF_INIT;\n> \tint has_common;\n> \n> \thas_common = get_common_dir(&sb, gitdir);\n> \tstrbuf_addstr(&sb, \"/config\");\n> \tread_repository_format(candidate, sb.buf);\n> \tstrbuf_release(&sb);\n> \n> \t/*\n> \t * For historical use of check_repository_format() in git-init,\n> \t * we treat a missing config as a silent \"ok\", even when nongit_ok\n> \t * is unset.\n> \t */\n> \tif (candidate->version < 0)\n> \t\treturn 0;\n> \n> \tif (verify_repository_format(candidate, &err) < 0) {\n> \t\tif (nongit_ok) {\n> \t\t\twarning(\"%s\", err.buf);\n> \t\t\tstrbuf_release(&err);\n> \t\t\t*nongit_ok = -1;\n> \t\t\treturn -1;\n> \t\t}\n> \t\tdie(\"%s\", err.buf);\n> \t}\n> \n> \tthe_repository->repository_format_precious_objects = candidate->precious_objects;\n> \n> \tstring_list_clear(&candidate->unknown_extensions, 0);\n> \tstring_list_clear(&candidate->v1_only_extensions, 0);\n> \n> \tif (candidate->worktree_config) {\n> \t\t/*\n> \t\t * pick up core.bare and core.worktree from per-worktree\n> \t\t * config if present\n> \t\t */\n> \t\tstrbuf_addf(&sb, \"%s/config.worktree\", gitdir);\n> \t\tgit_config_from_file(read_worktree_config, sb.buf, candidate);\n> \t\tstrbuf_release(&sb);\n> \t\thas_common = 0;\n> \t}\n\n...\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\t\tinside_work_tree = -1;\n> \t\t}\n> \t\tif (candidate->work_tree) {\n> \t\t\tfree(git_work_tree_cfg);\n> \t\t\tgit_work_tree_cfg = xstrdup(candidate->work_tree);\n> \t\t\tinside_work_tree = -1;\n> \t\t}\n> \t}\n> \n> \treturn 0;\n> }\n\nObviously it applies the parsed results to the global environment. Since \nthis function starts with 'check_' and ends with '_gently,' it's hard \nnot to think of it as a side-effect-free diagnostic function.\n\ngit grep \"^[a-z_]* check_[a-z_]*(\" -- \"*.c\"\n\nUsing the command above, we can see that there are other functions in \nthe Git source code that start with 'check_' but are not entirely \nside-effect-free. However, I believe that the two variables \n(is_bare_repository_cfg, git_work_tree_cfg) this function modifies are \nmuch more crucial: For example, if I want to handle both a bare \nrepository and a normal repository within a single process, wouldn't \nproblems arise soon?\n\nI would love to hear if there are any historical reasons preventing us \nfrom stripping these global side-effects out of this function.\n\nRegards,\n\nYuchen\n"},{"id":"539413","messageId":"xmqqfr5vlmlu.fsf@gitster.g","threadId":"65305","inReplyTo":"c0bb931a-3ee6-416b-8ceb-9fab013a621e@malon.dev","subject":"Re: [Question] check_repository_format_gently() is not side-effect-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-19T18:07:09Z","receivedAt":"2026-03-19T18:07:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen <cat@malon.dev> writes:\n\nThe verb \"check\" does not imply side-effect-free.  By checking, each\nof these functions tries to achieve something, and the way the\nresult of their work is conveyed back to the caller may not\nnecessarily be only by their return values.\n\nThe adverb \"gently\" in this codebase typically means \"the variant\nwithout gently signals problems by dying.  Instead of dying, return\nto the caller with error code, so that the caller can decide to\ndie\".\n"},{"id":"539448","messageId":"00d622d4-cfb8-41ff-b2df-5fb58a492a75@malon.dev","threadId":"65305","inReplyTo":"xmqqfr5vlmlu.fsf@gitster.g","subject":"Re: [Question] check_repository_format_gently() is not side-effect-free","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-20T00:24:09Z","receivedAt":"2026-03-20T00:24:29Z","isPatch":false,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"On 3/20/26 02:07, Junio C Hamano wrote:\n> The verb \"check\" does not imply side-effect-free.  By checking, each\n> of these functions tries to achieve something, and the way the\n> result of their work is conveyed back to the caller may not\n> necessarily be only by their return values.\n> \n> The adverb \"gently\" in this codebase typically means \"the variant\n> without gently signals problems by dying.  Instead of dying, return\n> to the caller with error code, so that the caller can decide to\n> die\".\n\nAh, I see. I guess I took it too literally. Thank you for clarification!\n\nSetting the semantics aside, the problem remains: I still think the \nsetup method here isn't quite right. It creates a bottleneck for \neventually handling multiple repositories in the same process without \ndata races.\n\nDo you think this is worth a patch?\n\nThanks,\n\nYuchen\n"},{"id":"539475","messageId":"abzkC9uLwZz_nmgv@pks.im","threadId":"65305","inReplyTo":"00d622d4-cfb8-41ff-b2df-5fb58a492a75@malon.dev","subject":"Re: [Question] check_repository_format_gently() is not side-effect-free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-20T06:07:07Z","receivedAt":"2026-03-20T06:07:13Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 20, 2026 at 08:24:09AM +0800, Tian Yuchen wrote:\n> On 3/20/26 02:07, Junio C Hamano wrote:\n> > The verb \"check\" does not imply side-effect-free.  By checking, each\n> > of these functions tries to achieve something, and the way the\n> > result of their work is conveyed back to the caller may not\n> > necessarily be only by their return values.\n> > \n> > The adverb \"gently\" in this codebase typically means \"the variant\n> > without gently signals problems by dying.  Instead of dying, return\n> > to the caller with error code, so that the caller can decide to\n> > die\".\n> \n> Ah, I see. I guess I took it too literally. Thank you for clarification!\n> \n> Setting the semantics aside, the problem remains: I still think the setup\n> method here isn't quite right. It creates a bottleneck for eventually\n> handling multiple repositories in the same process without data races.\n> \n> Do you think this is worth a patch?\n\nYes, I think that the whole of \"setup.c\" is something we will want to\nrefactor eventually so that it does not modify global state anymore. So\nit's not only `check_repository_format_gently()`, but also lots of other\nfunctionality in that file. The motivation is not only being able to set\nup multiple repositories, but also making the code overall easier to\nunderstand.\n\nThat being said, I'll give a small warning that it's probably\nnon-trivial to refactor this subystem :)\n\nThanks!\n\nPatrick\n"},{"id":"539560","messageId":"xmqqpl4yfq9o.fsf@gitster.g","threadId":"65305","inReplyTo":"abzkC9uLwZz_nmgv@pks.im","subject":"Re: [Question] check_repository_format_gently() is not side-effect-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-20T15:56:51Z","receivedAt":"2026-03-20T15:56:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> ... The motivation is not only being able to set\n> up multiple repositories, but also making the code overall easier to\n> understand.\n>\n> That being said, I'll give a small warning that it's probably\n> non-trivial to refactor this subystem :)\n\nWell said, and I agree 100%; thanks.\n"},{"id":"539563","messageId":"4d2001cc-ab9e-4595-88a4-fc650518ab3c@malon.dev","threadId":"65305","inReplyTo":"abzkC9uLwZz_nmgv@pks.im","subject":"Re: [Question] check_repository_format_gently() is not side-effect-free","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-20T16:26:21Z","receivedAt":"2026-03-20T16:26:29Z","isPatch":false,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Patrick,\n\nOn 3/20/26 14:07, Patrick Steinhardt wrote:\n\n> Yes, I think that the whole of \"setup.c\" is something we will want to\n> refactor eventually so that it does not modify global state anymore. So\n> it's not only `check_repository_format_gently()`, but also lots of other\n> functionality in that file. The motivation is not only being able to set\n> up multiple repositories, but also making the code overall easier to\n> understand.\n> \n> That being said, I'll give a small warning that it's probably\n> non-trivial to refactor this subystem 🙂\n\nThanks for the reply!\n\nIt’s true — setup.c seems utterly baffling to me. I thought I understood \nit before, but the more closely I look at it, the more I realize there \nare details everywhere that require careful attention.\n\nI won't drop a break-the-world patch out of nowhere. I'll keep learning \nuntil I'm able to do so ;)\n\nThanks,\n\nYuchen\n"}]}