# [PATCH 0/7] setup: enforce repo passed to `create_repository()` has no state 32 messages from 2026-09-24 to 2026-09-28. Participants: Patrick Steinhardt, Kaartic Sivaraam, Karthik Nayak, Junio C Hamano. Thread: https://gitlist.dev/t/66382 ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` Hi, when creating a new repository via `create_repository()` we pass in a repository. This repository is acting as an in/out parameter: the caller expects that it will be fully configured after the call, but the function itself also uses some information from the passed-in repository to figure out how exactly we want to create it. This interface is quite confusing, as it's not obvious at all what configuration of the repository is relevant. We have thus over a couple of patch series reduced the use of the parameter as in/out parameter. So now, the only piece of info that is still being propagated via the repo is "core.sharedRepository". This patch series cleans up that last remaining part so that the repo becomes purely an out-parameter. To ensure that this is the case we also start to `repo_clear()` it as a first step. Besides simplifying the interface, the intent is also to go further into the direction of unifying repository initialization in a follow-up patch series. The series is built on top of 0f8e75abeb (Revert "Merge branch 'en/no-amend-during-conflicts'", 2026-09-23) with ps/odb-alternates-at-creation at d1019ac894 (odb/source: remove the ability to write alternates, 2026-09-10) merged into it. Thanks! Patrick --- Patrick Steinhardt (7): path: drop useless `safe_create_leading_directories_1()` path: introduce `safe_create_leading_directories_no_share_const()` builtin/init: refactor messy creation of leading directories builtin/init: move handling of "core.sharedRepository" into "setup.c" builtin/clone: don't apply "core.sharedRepository" to leading dirs repository: adapt `repo_clear()` to fully reset the repository setup: enforce that passed-in repo does not carry relevant state builtin/clone.c | 4 ++-- builtin/init-db.c | 15 ++------------- path.c | 13 ++++++------- path.h | 1 + repository.c | 37 ++++++++++++++++++------------------- repository.h | 2 +- setup.c | 6 ++++++ t/t1301-shared-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 78 insertions(+), 42 deletions(-) --- base-commit: 6b6fe25b12e5324f2fdaf8c73816b9d2207e9404 change-id: 20260916-pks-create-repository-stateless-f0ca03cca689 ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 1/7] path: drop useless `safe_create_leading_directories_1()` Message-ID: <20260924-pks-create-repository-stateless-v1-1-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` The function `safe_create_leading_directories_1()` is being called by both `safe_create_leading_directories()` and its `_no_share()` variant. It is ultimately the exact same as the former of these functions though and is thus quite useless. Drop the function and inline it into its callsites directly. Signed-off-by: Patrick Steinhardt --- path.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/path.c b/path.c index c3a709a928..69b06c9464 100644 --- a/path.c +++ b/path.c @@ -829,8 +829,8 @@ int safe_create_dir_in_gitdir(struct repository *repo, const char *path) return adjust_shared_perm(repo, path); } -static enum scld_error safe_create_leading_directories_1(struct repository *repo, - char *path) +enum scld_error safe_create_leading_directories(struct repository *repo, + char *path) { char *next_component = path + offset_1st_component(path); enum scld_error ret = SCLD_OK; @@ -884,15 +884,9 @@ static enum scld_error safe_create_leading_directories_1(struct repository *repo return ret; } -enum scld_error safe_create_leading_directories(struct repository *repo, - char *path) -{ - return safe_create_leading_directories_1(repo, path); -} - enum scld_error safe_create_leading_directories_no_share(char *path) { - return safe_create_leading_directories_1(NULL, path); + return safe_create_leading_directories(NULL, path); } enum scld_error safe_create_leading_directories_const(struct repository *repo, -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 2/7] path: introduce `safe_create_leading_directories_no_share_const()` Message-ID: <20260924-pks-create-repository-stateless-v1-2-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` The `safe_create_leading_directories()` family of functions modify the passed-in path so that we can obtain all the different segments of the path. This is done by overwriting path separators with a NUL byte for every component. While we ultimately restore the original string, the consequence is that the caller needs to pass a non-constant string. While it would be trivial to modify the function to not modify the path in-place anymore, the intent of this whole mechanism is to save an allocation. It's quite dubious whether this optimization really matters in the grand scheme of things, but here we are. In any case, we provide a `_const()` variant that handles the case where the caller only has a string constant. But we lack such a variant for the `safe_create_leading_directories_no_share()` function, and we're about to add a couple of callers that would need it. Add this helper function. Signed-off-by: Patrick Steinhardt --- path.c | 5 +++++ path.h | 1 + 2 files changed, 6 insertions(+) diff --git a/path.c b/path.c index 69b06c9464..f8f5a9dd28 100644 --- a/path.c +++ b/path.c @@ -889,6 +889,11 @@ enum scld_error safe_create_leading_directories_no_share(char *path) return safe_create_leading_directories(NULL, path); } +enum scld_error safe_create_leading_directories_no_share_const(const char *path) +{ + return safe_create_leading_directories_const(NULL, path); +} + enum scld_error safe_create_leading_directories_const(struct repository *repo, const char *path) { diff --git a/path.h b/path.h index 7e7408dd05..e2d62c4978 100644 --- a/path.h +++ b/path.h @@ -254,6 +254,7 @@ enum scld_error safe_create_leading_directories(struct repository *repo, char *p enum scld_error safe_create_leading_directories_const(struct repository *repo, const char *path); enum scld_error safe_create_leading_directories_no_share(char *path); +enum scld_error safe_create_leading_directories_no_share_const(const char *path); /* * Create a file, potentially creating its leading directories in case they -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 3/7] builtin/init: refactor messy creation of leading directories Message-ID: <20260924-pks-create-repository-stateless-v1-3-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` When creating a new repository via git-init(1) we potentially have to create any leading directories via `safe_create_leading_directories()`. This function optionally knows to handle "core.sharedRepository" to adjust the permissions of the created directories. The value of that setting is taken from the passed-in repository. When creating a new repository we don't want to honor it though, so we painstakingly: 1. Save the current value of that setting. 2. Set it to 0. 3. Create the directory with `safe_create_leading_directories()`. This has the effect that `adjust_shared_perm()` will exit early and not adjust permissions. 4. Restore the old value. This is extremely awkward, but it achieves the desired effect that we ignore the configuration. There's a significantly easier way to achieve this though: we can just call the `_no_share()` variant, whose entire purpose it is to ignore "core.sharedRepository". Refactor the code to use that variant accordingly. Signed-off-by: Patrick Steinhardt --- builtin/init-db.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index 5c22eae2f3..e45268f1ff 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -131,15 +131,7 @@ int cmd_init_db(int argc, retry: if (chdir(argv[0]) < 0) { if (!mkdir_tried) { - int saved; - /* - * At this point we haven't read any configuration, - * and we know shared_repository should always be 0; - * but just in case we play safe. - */ - saved = repo_settings_get_shared_repository(the_repository); - repo_settings_set_shared_repository(the_repository, 0); - switch (safe_create_leading_directories_const(the_repository, argv[0])) { + switch (safe_create_leading_directories_no_share_const(argv[0])) { case SCLD_OK: case SCLD_PERMS: break; @@ -150,7 +142,7 @@ int cmd_init_db(int argc, die_errno(_("cannot mkdir %s"), argv[0]); break; } - repo_settings_set_shared_repository(the_repository, saved); + if (mkdir(argv[0], 0777) < 0) die_errno(_("cannot mkdir %s"), argv[0]); mkdir_tried = 1; -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 4/7] builtin/init: move handling of "core.sharedRepository" into "setup.c" Message-ID: <20260924-pks-create-repository-stateless-v1-4-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` When initializing a new repository via git-init(1) we know to honor "core.sharedRepository" and adjust permissions of newly created files accordingly. The way we propagate that setting is quite awkward though, as we have to set it on the repository that we pass into `create_repository()` and pass it as a parameter. This is because there are two different scopes in play here: - We need to apply it to the repository so that creating the repository's directory uses the correct permissions. - We need to reapply it to the repository after we have created default files so that we know to override any configuration that we have read from the new repository's configuration. The effect of this though is that the repository works as an in-out parameter, which is quite awkward. Refactor the code so that the caller only needs to pass the value. Starting with this change, the passed-in repository can essentially be completely blank as it doesn't carry any state anymore that we'd care about in `create_repository()`. Note that this change in theory also impacts the other caller of `create_repository()` that exists in git-clone(1). But that caller already passes `-1` as a value for this parameter, and neither does that caller modify the repository it passes. So there shouldn't be any change in behaviour here. Signed-off-by: Patrick Steinhardt --- builtin/init-db.c | 3 --- setup.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index e45268f1ff..34215bbf18 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -171,9 +171,6 @@ int cmd_init_db(int argc, die(_("unknown ref storage format '%s'"), ref_format); } - if (init_shared_repository != -1) - repo_settings_set_shared_repository(the_repository, init_shared_repository); - /* * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR * without --bare. Catch the error early. diff --git a/setup.c b/setup.c index f335111d1e..0d0a4abbe6 100644 --- a/setup.c +++ b/setup.c @@ -2896,6 +2896,9 @@ void create_repository(struct repository *repo, */ repo_config(repo, git_default_core_config, NULL); + if (init_shared_repository != -1) + repo_settings_set_shared_repository(repo, init_shared_repository); + safe_create_dir(repo, git_dir, 0); if (!reinit_ok) -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 5/7] builtin/clone: don't apply "core.sharedRepository" to leading dirs Message-ID: <20260924-pks-create-repository-stateless-v1-5-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` When creating a repository via git-clone(1) we create leading directories with `safe_create_leading_directories()`. We have adapted git-init(1) in a preceding commit to instead use the variant of this function that doesn't honor "core.sharedRepository". In that subcommand it didn't have an effect though as we explicitly unset the value of that configuration anyway, so we never honored that config. In git-clone(1) it's a bit of a different thing though: while the repository isn't initialized at the point in time where we call the function, we didn't explicitly unset the value. Consequently we _do_ honor the configuration here, but when it's configured in global- or system-level scope. This divergence doesn't seem to be intentional -- I cannot think of any good reason why git-init(1) and git-clone(1) should have divergent behaviour here. Adapt git-clone(1) to work the same as git-init(1) by also using the `no_share()` variants to create leading directories. Add tests for both commands. Signed-off-by: Patrick Steinhardt --- builtin/clone.c | 4 ++-- t/t1301-shared-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index b14264c33a..e72f8aa325 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1133,7 +1133,7 @@ int cmd_clone(int argc, sigchain_push_common(remove_junk_on_signal); if (!option_bare) { - if (safe_create_leading_directories_const(the_repository, work_tree) < 0) + if (safe_create_leading_directories_no_share_const(work_tree) < 0) die_errno(_("could not create leading directories of '%s'"), work_tree); if (dest_exists) @@ -1153,7 +1153,7 @@ int cmd_clone(int argc, junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL; junk_git_dir = git_dir; } - if (safe_create_leading_directories_const(the_repository, git_dir) < 0) + if (safe_create_leading_directories_no_share_const(git_dir) < 0) die(_("could not create leading directories of '%s'"), git_dir); if (0 <= option_verbosity) { diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 0e0d07a1a1..3bc4bdb038 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -210,4 +210,46 @@ test_expect_success POSIXPERM 'template can set core.sharedrepository' ' test_cmp expect actual ' +test_expect_success POSIXPERM 'init does not apply core.sharedRepository to leading directories' ' + test_config_global core.sharedRepository 0666 && + umask 0077 && + test_when_finished "rm -rf dst" && + git init --bare dst/with/leading/dirs && + cat >expect <<-\EOF && + drwx------ + drwx------ + drwx------ + drwxrwxrwx + EOF + { + test_modebits dst && + test_modebits dst/with && + test_modebits dst/with/leading && + test_modebits dst/with/leading/dirs + } >actual && + test_cmp expect actual +' + +test_expect_success POSIXPERM 'clone does not apply core.sharedRepository to leading directories' ' + test_config_global core.sharedRepository 0666 && + umask 0077 && + test_when_finished "rm -rf source dst" && + git init source && + test_commit -C source initial && + git clone --bare source dst/with/leading/dirs && + cat >expect <<-\EOF && + drwx------ + drwx------ + drwx------ + drwxrwxrwx + EOF + { + test_modebits dst && + test_modebits dst/with && + test_modebits dst/with/leading && + test_modebits dst/with/leading/dirs + } >actual && + test_cmp expect actual +' + test_done -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 6/7] repository: adapt `repo_clear()` to fully reset the repository Message-ID: <20260924-pks-create-repository-stateless-v1-6-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` The function `repo_clear()` can be used to clear a repository's state. The way it's written though it's quite easy for it to accidentally leak some state because we don't make sure to clear the whole structure. Refactor the function to set the whole repository to all-zeroes to avoid any kind of leaking state. While at it, make it a bit more robust when called on an already-blank repository. Signed-off-by: Patrick Steinhardt --- repository.c | 37 ++++++++++++++++++------------------- repository.h | 2 +- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/repository.c b/repository.c index b857e1c580..e67ff00550 100644 --- a/repository.c +++ b/repository.c @@ -374,60 +374,57 @@ void repo_clear(struct repository *repo) struct hashmap_iter iter; struct strmap_entry *e; - FREE_AND_NULL(repo->gitdir); - FREE_AND_NULL(repo->commondir); - FREE_AND_NULL(repo->prefix); - FREE_AND_NULL(repo->graft_file); - FREE_AND_NULL(repo->index_file); - FREE_AND_NULL(repo->worktree); - FREE_AND_NULL(repo->submodule_prefix); - FREE_AND_NULL(repo->ref_storage_payload); + free(repo->gitdir); + free(repo->commondir); + free(repo->prefix); + free(repo->graft_file); + free(repo->index_file); + free(repo->worktree); + free(repo->submodule_prefix); + free(repo->ref_storage_payload); odb_free(repo->objects); - repo->objects = NULL; if (repo->parsed_objects) parsed_object_pool_clear(repo->parsed_objects); - FREE_AND_NULL(repo->parsed_objects); + free(repo->parsed_objects); repo_settings_clear(repo); repo_config_values_clear(&repo->config_values_private_); if (repo->config) { git_configset_clear(repo->config); - FREE_AND_NULL(repo->config); + free(repo->config); } - if (repo->submodule_cache) { + if (repo->submodule_cache) submodule_cache_free(repo->submodule_cache); - repo->submodule_cache = NULL; - } if (repo->index) { discard_index(repo->index); - FREE_AND_NULL(repo->index); + free(repo->index); } if (repo->hook_config_cache) { hook_cache_clear(repo->hook_config_cache); - FREE_AND_NULL(repo->hook_config_cache); + free(repo->hook_config_cache); } strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ string_list_clear(&repo->disabled_events, 0); if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); - FREE_AND_NULL(repo->promisor_remote_config); + free(repo->promisor_remote_config); } if (repo->remote_state) { remote_state_clear(repo->remote_state); - FREE_AND_NULL(repo->remote_state); + free(repo->remote_state); } if (repo->refs_private) { ref_store_release(repo->refs_private); - FREE_AND_NULL(repo->refs_private); + free(repo->refs_private); } strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) @@ -439,6 +436,8 @@ void repo_clear(struct repository *repo) strmap_clear(&repo->worktree_ref_stores, 1); repo_clear_path_cache(&repo->cached_paths); + + memset(repo, 0, sizeof(*repo)); } int repo_read_index(struct repository *repo) diff --git a/repository.h b/repository.h index 11f5c2ed10..2a348012e8 100644 --- a/repository.h +++ b/repository.h @@ -258,6 +258,7 @@ void repo_set_ref_storage_format(struct repository *repo, void initialize_repository(struct repository *repo); RESULT_MUST_BE_USED int repo_init(struct repository *r, const char *gitdir, const char *worktree); +void repo_clear(struct repository *repo); /* * Initialize the repository 'subrepo' as the submodule at the given path. If @@ -273,7 +274,6 @@ int repo_submodule_init(struct repository *subrepo, struct repository *superproject, const char *path, const struct object_id *treeish_name); -void repo_clear(struct repository *repo); /* * Populates the repository's index from its index_file, an index struct will -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-24 09:19 Subject: [PATCH 7/7] setup: enforce that passed-in repo does not carry relevant state Message-ID: <20260924-pks-create-repository-stateless-v1-7-11499557cf31@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` In the preceding patches we have refactored `create_repository()` so that the passed-in repository is not used anymore to propagate any kind of state. This was done so that the parameter doesn't act like an in-out parameter, but only as an out parameter that we initialize with the state of the newly created repository. We don't enforce though that the repository _cannot_ be used to propagate state anymore, which makes it quite easy for state to sneak in at a later point again. Ideally, we'd do that by having the function create a newly allocated repository instead of taking a repository as input. But unfortunately, that does not work because we end up calling `repo_config_values()` when we create the "files" ref database, and that function requires that the passed-in repository is `the_repository`. Instead, call `repo_clear()` at the beginning of the function, which gives us a clean slate. Signed-off-by: Patrick Steinhardt --- setup.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/setup.c b/setup.c index 0d0a4abbe6..fa39219d6a 100644 --- a/setup.c +++ b/setup.c @@ -2858,6 +2858,9 @@ void create_repository(struct repository *repo, struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; struct strbuf err = STRBUF_INIT; + repo_clear(repo); + initialize_repository(repo); + if (real_git_dir) { struct stat st; -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Kaartic Sivaraam, 2026-09-25 19:49 Subject: Re: [PATCH 2/7] path: introduce `safe_create_leading_directories_no_share_const()` Message-ID: <4770b19f-9a8d-4a4c-8cc6-745aa2868b94@gmail.com> In-Reply-To: <20260924-pks-create-repository-stateless-v1-2-11499557cf31@pks.im> ``` On 9/24/26 14:49, Patrick Steinhardt wrote: > > diff --git a/path.h b/path.h > index 7e7408dd05..e2d62c4978 100644 > --- a/path.h > +++ b/path.h > @@ -254,6 +254,7 @@ enum scld_error safe_create_leading_directories(struct repository *repo, char *p > enum scld_error safe_create_leading_directories_const(struct repository *repo, > const char *path); > enum scld_error safe_create_leading_directories_no_share(char *path); > +enum scld_error safe_create_leading_directories_no_share_const(const char *path); > nit: All other variants are mentioned in the documentation blurb just above the declarations. Would it also be worth mentioning this new one there? -- Sivaraam ``` ## Kaartic Sivaraam, 2026-09-25 20:21 Subject: Re: [PATCH 3/7] builtin/init: refactor messy creation of leading directories Message-ID: <63798ed5-3fea-4938-8b0b-910d5f1c7bf0@gmail.com> In-Reply-To: <20260924-pks-create-repository-stateless-v1-3-11499557cf31@pks.im> ``` On 9/24/26 14:49, Patrick Steinhardt wrote: > diff --git a/builtin/init-db.c b/builtin/init-db.c > index 5c22eae2f3..e45268f1ff 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -131,15 +131,7 @@ int cmd_init_db(int argc, > retry: > if (chdir(argv[0]) < 0) { > if (!mkdir_tried) { > - int saved; > - /* > - * At this point we haven't read any configuration, > - * and we know shared_repository should always be 0; > - * but just in case we play safe. > - */ > - saved = repo_settings_get_shared_repository(the_repository); > - repo_settings_set_shared_repository(the_repository, 0); > - switch (safe_create_leading_directories_const(the_repository, argv[0])) { > + switch (safe_create_leading_directories_no_share_const(argv[0])) { > case SCLD_OK: > case SCLD_PERMS: > break; > @@ -150,7 +142,7 @@ int cmd_init_db(int argc, > die_errno(_("cannot mkdir %s"), argv[0]); > break; > } > - repo_settings_set_shared_repository(the_repository, saved); > + Even though this patch does not aim to do so, we lost a bunch of 'the_repository' references with this change which is nice. The patch also looks good to me. -- Sivaraam ``` ## Kaartic Sivaraam, 2026-09-25 20:54 Subject: Re: [PATCH 4/7] builtin/init: move handling of "core.sharedRepository" into "setup.c" Message-ID: <4e26b51a-f811-4548-97e0-6570680efa13@gmail.com> In-Reply-To: <20260924-pks-create-repository-stateless-v1-4-11499557cf31@pks.im> ``` On 9/24/26 14:49, Patrick Steinhardt wrote: > > diff --git a/builtin/init-db.c b/builtin/init-db.c > index e45268f1ff..34215bbf18 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -171,9 +171,6 @@ int cmd_init_db(int argc, > die(_("unknown ref storage format '%s'"), ref_format); > } > > - if (init_shared_repository != -1) > - repo_settings_set_shared_repository(the_repository, init_shared_repository); > - > /* > * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR > * without --bare. Catch the error early. I was wondering if there'll be any function that between here and the create_repository call that may use the_repository. I could not find any from my reading of the code, though. So, this looks good to me. - Sivaraam ``` ## Kaartic Sivaraam, 2026-09-25 21:08 Subject: Re: [PATCH 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` On 9/24/26 14:49, Patrick Steinhardt wrote: > > when creating a new repository via `create_repository()` we pass in a > repository. This repository is acting as an in/out parameter: the caller > expects that it will be fully configured after the call, but the > function itself also uses some information from the passed-in repository > to figure out how exactly we want to create it. > > This interface is quite confusing, as it's not obvious at all what > configuration of the repository is relevant. We have thus over a couple > of patch series reduced the use of the parameter as in/out parameter. So > now, the only piece of info that is still being propagated via the repo > is "core.sharedRepository". > > This patch series cleans up that last remaining part so that the repo > becomes purely an out-parameter. To ensure that this is the case we also > start to `repo_clear()` it as a first step. > > Besides simplifying the interface, the intent is also to go further into > the direction of unifying repository initialization in a follow-up patch > series. > The patches seem to be well-split and the changes look good. It was a nice read. Overall, this series seems to look good to me. Thank you for making create_repository not rely on state from the repo given to it! -- Sivaraam ``` ## Patrick Steinhardt, 2026-09-28 07:15 Subject: Re: [PATCH 2/7] path: introduce `safe_create_leading_directories_no_share_const()` Message-ID: In-Reply-To: <4770b19f-9a8d-4a4c-8cc6-745aa2868b94@gmail.com> ``` On Sat, Sep 26, 2026 at 01:19:42AM +0530, Kaartic Sivaraam wrote: > On 9/24/26 14:49, Patrick Steinhardt wrote: > > > > diff --git a/path.h b/path.h > > index 7e7408dd05..e2d62c4978 100644 > > --- a/path.h > > +++ b/path.h > > @@ -254,6 +254,7 @@ enum scld_error safe_create_leading_directories(struct repository *repo, char *p > > enum scld_error safe_create_leading_directories_const(struct repository *repo, > > const char *path); > > enum scld_error safe_create_leading_directories_no_share(char *path); > > +enum scld_error safe_create_leading_directories_no_share_const(const char *path); > > > > nit: All other variants are mentioned in the documentation blurb just above > the declarations. Would it also be worth mentioning this new one there? That's fair. I find the comment to be somewhat unwieldy overall. How about this diff? diff --git a/path.h b/path.h index 7e7408dd05..922bd6e377 100644 --- a/path.h +++ b/path.h @@ -234,14 +234,11 @@ int safe_create_dir_in_gitdir(struct repository *repo, const char *path); * race, callers might want to try invoking the function again when it * returns SCLD_VANISHED. * - * safe_create_leading_directories() temporarily changes path while it - * is working but restores it before returning. - * safe_create_leading_directories_const() doesn't modify path, even - * temporarily. Both these variants adjust the permissions of the - * created directories to honor core.sharedRepository, so they are best - * suited for files inside the git dir. For working tree files, use - * safe_create_leading_directories_no_share() instead, as it ignores - * the core.sharedRepository setting. + * The default variants honor "core.sharedRepository" and temporarily modify + * `path`. Note that this configuration should be honored for all files in the + * git directory. The `no_share()` variants ignore "core.sharedRepository", + * and should be used for working tree files. The `const()` variants do not + * modify `path`. */ enum scld_error { SCLD_OK = 0, Patrick ``` ## Karthik Nayak, 2026-09-28 09:01 Subject: Re: [PATCH 1/7] path: drop useless `safe_create_leading_directories_1()` Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-1-11499557cf31@pks.im> ``` Patrick Steinhardt writes: > The function `safe_create_leading_directories_1()` is being called by > both `safe_create_leading_directories()` and its `_no_share()` variant. > It is ultimately the exact same as the former of these functions though > and is thus quite useless. > > Drop the function and inline it into its callsites directly. > Nice, always happy to see '_1()' functions go away or be renamed. [snip] ``` ## Karthik Nayak, 2026-09-28 09:13 Subject: Re: [PATCH 4/7] builtin/init: move handling of "core.sharedRepository" into "setup.c" Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-4-11499557cf31@pks.im> ``` Patrick Steinhardt writes: > When initializing a new repository via git-init(1) we know to honor > "core.sharedRepository" and adjust permissions of newly created files > accordingly. The way we propagate that setting is quite awkward though, > as we have to set it on the repository that we pass into > `create_repository()` and pass it as a parameter. This is because there > are two different scopes in play here: > > - We need to apply it to the repository so that creating the > repository's directory uses the correct permissions. > > - We need to reapply it to the repository after we have created > default files so that we know to override any configuration that we > have read from the new repository's configuration. > > The effect of this though is that the repository works as an in-out > parameter, which is quite awkward. > > Refactor the code so that the caller only needs to pass the value. > Starting with this change, the passed-in repository can essentially be > completely blank as it doesn't carry any state anymore that we'd care > about in `create_repository()`. > > Note that this change in theory also impacts the other caller of > `create_repository()` that exists in git-clone(1). But that caller > already passes `-1` as a value for this parameter, and neither does that > caller modify the repository it passes. So there shouldn't be any change > in behaviour here. > So this works, becaus we already pass in the `init_shared_repository` value to `create_repository()`. Which is currently used while creating the default files, now we also extend it to set the adequate permissions on the repository too. > Signed-off-by: Patrick Steinhardt > --- > builtin/init-db.c | 3 --- > setup.c | 3 +++ > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/builtin/init-db.c b/builtin/init-db.c > index e45268f1ff..34215bbf18 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -171,9 +171,6 @@ int cmd_init_db(int argc, > die(_("unknown ref storage format '%s'"), ref_format); > } > > - if (init_shared_repository != -1) > - repo_settings_set_shared_repository(the_repository, init_shared_repository); > - > /* > * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR > * without --bare. Catch the error early. > diff --git a/setup.c b/setup.c > index f335111d1e..0d0a4abbe6 100644 > --- a/setup.c > +++ b/setup.c > @@ -2896,6 +2896,9 @@ void create_repository(struct repository *repo, > */ > repo_config(repo, git_default_core_config, NULL); > > + if (init_shared_repository != -1) > + repo_settings_set_shared_repository(repo, init_shared_repository); > + > safe_create_dir(repo, git_dir, 0); > > if (!reinit_ok) > > -- > 2.56.0.rc2.329.gd58861e689.dirty With that context, this patch makes sense. ``` ## Karthik Nayak, 2026-09-28 09:18 Subject: Re: [PATCH 6/7] repository: adapt `repo_clear()` to fully reset the repository Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-6-11499557cf31@pks.im> ``` Patrick Steinhardt writes: > The function `repo_clear()` can be used to clear a repository's state. > The way it's written though it's quite easy for it to accidentally leak > some state because we don't make sure to clear the whole structure. > > Refactor the function to set the whole repository to all-zeroes to avoid > any kind of leaking state. While at it, make it a bit more robust when > called on an already-blank repository. > > Signed-off-by: Patrick Steinhardt > --- > repository.c | 37 ++++++++++++++++++------------------- > repository.h | 2 +- > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/repository.c b/repository.c > index b857e1c580..e67ff00550 100644 > --- a/repository.c > +++ b/repository.c > @@ -374,60 +374,57 @@ void repo_clear(struct repository *repo) > struct hashmap_iter iter; > struct strmap_entry *e; > > - FREE_AND_NULL(repo->gitdir); > - FREE_AND_NULL(repo->commondir); > - FREE_AND_NULL(repo->prefix); > - FREE_AND_NULL(repo->graft_file); > - FREE_AND_NULL(repo->index_file); > - FREE_AND_NULL(repo->worktree); > - FREE_AND_NULL(repo->submodule_prefix); > - FREE_AND_NULL(repo->ref_storage_payload); > + free(repo->gitdir); > + free(repo->commondir); > + free(repo->prefix); > + free(repo->graft_file); > + free(repo->index_file); > + free(repo->worktree); > + free(repo->submodule_prefix); > + free(repo->ref_storage_payload); > > odb_free(repo->objects); > - repo->objects = NULL; > > if (repo->parsed_objects) > parsed_object_pool_clear(repo->parsed_objects); > - FREE_AND_NULL(repo->parsed_objects); > + free(repo->parsed_objects); > > repo_settings_clear(repo); > repo_config_values_clear(&repo->config_values_private_); > > if (repo->config) { > git_configset_clear(repo->config); > - FREE_AND_NULL(repo->config); > + free(repo->config); > } > > - if (repo->submodule_cache) { > + if (repo->submodule_cache) > submodule_cache_free(repo->submodule_cache); > - repo->submodule_cache = NULL; > - } > > if (repo->index) { > discard_index(repo->index); > - FREE_AND_NULL(repo->index); > + free(repo->index); > } > > if (repo->hook_config_cache) { > hook_cache_clear(repo->hook_config_cache); > - FREE_AND_NULL(repo->hook_config_cache); > + free(repo->hook_config_cache); > } > strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ > string_list_clear(&repo->disabled_events, 0); > > if (repo->promisor_remote_config) { > promisor_remote_clear(repo->promisor_remote_config); > - FREE_AND_NULL(repo->promisor_remote_config); > + free(repo->promisor_remote_config); > } > > if (repo->remote_state) { > remote_state_clear(repo->remote_state); > - FREE_AND_NULL(repo->remote_state); > + free(repo->remote_state); > } > > if (repo->refs_private) { > ref_store_release(repo->refs_private); > - FREE_AND_NULL(repo->refs_private); > + free(repo->refs_private); > } > > strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) > @@ -439,6 +436,8 @@ void repo_clear(struct repository *repo) > strmap_clear(&repo->worktree_ref_stores, 1); > > repo_clear_path_cache(&repo->cached_paths); > + > + memset(repo, 0, sizeof(*repo)); The reason we swap `FREE_AND_NULL()` with `free()` is because we anyways set everything to 0. Okay. Or was this referring to the 'already blank' repository? Since FREE_AND_NULL() can already handle NULL values. > } > > int repo_read_index(struct repository *repo) > diff --git a/repository.h b/repository.h > index 11f5c2ed10..2a348012e8 100644 > --- a/repository.h > +++ b/repository.h > @@ -258,6 +258,7 @@ void repo_set_ref_storage_format(struct repository *repo, > void initialize_repository(struct repository *repo); > RESULT_MUST_BE_USED > int repo_init(struct repository *r, const char *gitdir, const char *worktree); > +void repo_clear(struct repository *repo); > > /* > * Initialize the repository 'subrepo' as the submodule at the given path. If > @@ -273,7 +274,6 @@ int repo_submodule_init(struct repository *subrepo, > struct repository *superproject, > const char *path, > const struct object_id *treeish_name); > -void repo_clear(struct repository *repo); > This is a purely cosmetic move to bring it closer to `repo_init()`, right? I think it makes sense. > /* > * Populates the repository's index from its index_file, an index struct will > > -- > 2.56.0.rc2.329.gd58861e689.dirty ``` ## Karthik Nayak, 2026-09-28 09:20 Subject: Re: [PATCH 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` Patrick Steinhardt writes: > Hi, > > when creating a new repository via `create_repository()` we pass in a > repository. This repository is acting as an in/out parameter: the caller > expects that it will be fully configured after the call, but the > function itself also uses some information from the passed-in repository > to figure out how exactly we want to create it. > > This interface is quite confusing, as it's not obvious at all what > configuration of the repository is relevant. We have thus over a couple > of patch series reduced the use of the parameter as in/out parameter. So > now, the only piece of info that is still being propagated via the repo > is "core.sharedRepository". > > This patch series cleans up that last remaining part so that the repo > becomes purely an out-parameter. To ensure that this is the case we also > start to `repo_clear()` it as a first step. > > Besides simplifying the interface, the intent is also to go further into > the direction of unifying repository initialization in a follow-up patch > series. > > The series is built on top of 0f8e75abeb (Revert "Merge branch > 'en/no-amend-during-conflicts'", 2026-09-23) with > ps/odb-alternates-at-creation at d1019ac894 (odb/source: remove the > ability to write alternates, 2026-09-10) merged into it. > > Thanks! > > Patrick > The series was a good read and I didn't see anything that needed changes. Thanks > --- > Patrick Steinhardt (7): > path: drop useless `safe_create_leading_directories_1()` > path: introduce `safe_create_leading_directories_no_share_const()` > builtin/init: refactor messy creation of leading directories > builtin/init: move handling of "core.sharedRepository" into "setup.c" > builtin/clone: don't apply "core.sharedRepository" to leading dirs > repository: adapt `repo_clear()` to fully reset the repository > setup: enforce that passed-in repo does not carry relevant state > > builtin/clone.c | 4 ++-- > builtin/init-db.c | 15 ++------------- > path.c | 13 ++++++------- > path.h | 1 + > repository.c | 37 ++++++++++++++++++------------------- > repository.h | 2 +- > setup.c | 6 ++++++ > t/t1301-shared-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ > 8 files changed, 78 insertions(+), 42 deletions(-) > > > --- > base-commit: 6b6fe25b12e5324f2fdaf8c73816b9d2207e9404 > change-id: 20260916-pks-create-repository-stateless-f0ca03cca689 ``` ## Kaartic Sivaraam, 2026-09-28 09:21 Subject: Re: [PATCH 2/7] path: introduce `safe_create_leading_directories_no_share_const()` Message-ID: <557ab7b2-a1b6-4064-997c-d0dc50126df6@gmail.com> In-Reply-To: ``` On 9/28/26 12:45, Patrick Steinhardt wrote: > On Sat, Sep 26, 2026 at 01:19:42AM +0530, Kaartic Sivaraam wrote: >> >> nit: All other variants are mentioned in the documentation blurb just above >> the declarations. Would it also be worth mentioning this new one there? > > That's fair. I find the comment to be somewhat unwieldy overall. How > about this diff? > > diff --git a/path.h b/path.h > index 7e7408dd05..922bd6e377 100644 > --- a/path.h > +++ b/path.h > @@ -234,14 +234,11 @@ int safe_create_dir_in_gitdir(struct repository *repo, const char *path); > * race, callers might want to try invoking the function again when it > * returns SCLD_VANISHED. > * > - * safe_create_leading_directories() temporarily changes path while it > - * is working but restores it before returning. > - * safe_create_leading_directories_const() doesn't modify path, even > - * temporarily. Both these variants adjust the permissions of the > - * created directories to honor core.sharedRepository, so they are best > - * suited for files inside the git dir. For working tree files, use > - * safe_create_leading_directories_no_share() instead, as it ignores > - * the core.sharedRepository setting. > + * The default variants honor "core.sharedRepository" and temporarily modify > + * `path`. Note that this configuration should be honored for all files in the > + * git directory. The `no_share()` variants ignore "core.sharedRepository", > + * and should be used for working tree files. The `const()` variants do not > + * modify `path`. > */ Reads much better to me. Thanks. -- Sivaraam ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> In-Reply-To: <20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im> ``` Hi, when creating a new repository via `create_repository()` we pass in a repository. This repository is acting as an in/out parameter: the caller expects that it will be fully configured after the call, but the function itself also uses some information from the passed-in repository to figure out how exactly we want to create it. This interface is quite confusing, as it's not obvious at all what configuration of the repository is relevant. We have thus over a couple of patch series reduced the use of the parameter as in/out parameter. So now, the only piece of info that is still being propagated via the repo is "core.sharedRepository". This patch series cleans up that last remaining part so that the repo becomes purely an out-parameter. To ensure that this is the case we also start to `repo_clear()` it as a first step. Besides simplifying the interface, the intent is also to go further into the direction of unifying repository initialization in a follow-up patch series. The series is built on top of 0f8e75abeb (Revert "Merge branch 'en/no-amend-during-conflicts'", 2026-09-23) with ps/odb-alternates-at-creation at d1019ac894 (odb/source: remove the ability to write alternates, 2026-09-10) merged into it. Changes in v2: - Adapt documentation of `safe_create_leading_directories()`. - Better explain change to fully clear repos. - Link to v1: https://patch.msgid.link/20260924-pks-create-repository-stateless-v1-0-11499557cf31@pks.im Thanks! Patrick --- Patrick Steinhardt (7): path: drop useless `safe_create_leading_directories_1()` path: introduce `safe_create_leading_directories_no_share_const()` builtin/init: refactor messy creation of leading directories builtin/init: move handling of "core.sharedRepository" into "setup.c" builtin/clone: don't apply "core.sharedRepository" to leading dirs repository: adapt `repo_clear()` to fully reset the repository setup: enforce that passed-in repo does not carry relevant state builtin/clone.c | 4 ++-- builtin/init-db.c | 15 ++------------- path.c | 13 ++++++------- path.h | 14 ++++++-------- repository.c | 37 ++++++++++++++++++------------------- repository.h | 2 +- setup.c | 6 ++++++ t/t1301-shared-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 83 insertions(+), 50 deletions(-) Range-diff versus v1: 1: 6ec41760de = 1: 0f9513d5c6 path: drop useless `safe_create_leading_directories_1()` 2: 85ac056b80 ! 2: 3294cdb6b6 path: introduce `safe_create_leading_directories_no_share_const()` @@ path.c: enum scld_error safe_create_leading_directories_no_share(char *path) { ## path.h ## +@@ path.h: int safe_create_dir_in_gitdir(struct repository *repo, const char *path); + * race, callers might want to try invoking the function again when it + * returns SCLD_VANISHED. + * +- * safe_create_leading_directories() temporarily changes path while it +- * is working but restores it before returning. +- * safe_create_leading_directories_const() doesn't modify path, even +- * temporarily. Both these variants adjust the permissions of the +- * created directories to honor core.sharedRepository, so they are best +- * suited for files inside the git dir. For working tree files, use +- * safe_create_leading_directories_no_share() instead, as it ignores +- * the core.sharedRepository setting. ++ * The default variants honor "core.sharedRepository" and temporarily modify ++ * `path`. Note that this configuration should be honored for all files in the ++ * git directory. The `no_share()` variants ignore "core.sharedRepository", ++ * and should be used for working tree files. The `const()` variants do not ++ * modify `path`. + */ + enum scld_error { + SCLD_OK = 0, @@ path.h: enum scld_error safe_create_leading_directories(struct repository *repo, char *p enum scld_error safe_create_leading_directories_const(struct repository *repo, const char *path); 3: 3a7c197f1b = 3: 8dd89f144a builtin/init: refactor messy creation of leading directories 4: c3ced666bd = 4: f37db1b17d builtin/init: move handling of "core.sharedRepository" into "setup.c" 5: 25918a4ff6 = 5: db76d32f2c builtin/clone: don't apply "core.sharedRepository" to leading dirs 6: a742852675 ! 6: 19388a188c repository: adapt `repo_clear()` to fully reset the repository @@ Commit message some state because we don't make sure to clear the whole structure. Refactor the function to set the whole repository to all-zeroes to avoid - any kind of leaking state. While at it, make it a bit more robust when - called on an already-blank repository. + any kind of leaking state. Replace calls of `FREE_AND_NULL()` to instead + use free(3p) to avoid zeroing out the data twice. Signed-off-by: Patrick Steinhardt 7: 760058e9c5 = 7: 0d4819f005 setup: enforce that passed-in repo does not carry relevant state --- base-commit: 6b6fe25b12e5324f2fdaf8c73816b9d2207e9404 change-id: 20260916-pks-create-repository-stateless-f0ca03cca689 ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 1/7] path: drop useless `safe_create_leading_directories_1()` Message-ID: <20260928-pks-create-repository-stateless-v2-1-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` The function `safe_create_leading_directories_1()` is being called by both `safe_create_leading_directories()` and its `_no_share()` variant. It is ultimately the exact same as the former of these functions though and is thus quite useless. Drop the function and inline it into its callsites directly. Signed-off-by: Patrick Steinhardt --- path.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/path.c b/path.c index c3a709a928..69b06c9464 100644 --- a/path.c +++ b/path.c @@ -829,8 +829,8 @@ int safe_create_dir_in_gitdir(struct repository *repo, const char *path) return adjust_shared_perm(repo, path); } -static enum scld_error safe_create_leading_directories_1(struct repository *repo, - char *path) +enum scld_error safe_create_leading_directories(struct repository *repo, + char *path) { char *next_component = path + offset_1st_component(path); enum scld_error ret = SCLD_OK; @@ -884,15 +884,9 @@ static enum scld_error safe_create_leading_directories_1(struct repository *repo return ret; } -enum scld_error safe_create_leading_directories(struct repository *repo, - char *path) -{ - return safe_create_leading_directories_1(repo, path); -} - enum scld_error safe_create_leading_directories_no_share(char *path) { - return safe_create_leading_directories_1(NULL, path); + return safe_create_leading_directories(NULL, path); } enum scld_error safe_create_leading_directories_const(struct repository *repo, -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 2/7] path: introduce `safe_create_leading_directories_no_share_const()` Message-ID: <20260928-pks-create-repository-stateless-v2-2-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` The `safe_create_leading_directories()` family of functions modify the passed-in path so that we can obtain all the different segments of the path. This is done by overwriting path separators with a NUL byte for every component. While we ultimately restore the original string, the consequence is that the caller needs to pass a non-constant string. While it would be trivial to modify the function to not modify the path in-place anymore, the intent of this whole mechanism is to save an allocation. It's quite dubious whether this optimization really matters in the grand scheme of things, but here we are. In any case, we provide a `_const()` variant that handles the case where the caller only has a string constant. But we lack such a variant for the `safe_create_leading_directories_no_share()` function, and we're about to add a couple of callers that would need it. Add this helper function. Signed-off-by: Patrick Steinhardt --- path.c | 5 +++++ path.h | 14 ++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/path.c b/path.c index 69b06c9464..f8f5a9dd28 100644 --- a/path.c +++ b/path.c @@ -889,6 +889,11 @@ enum scld_error safe_create_leading_directories_no_share(char *path) return safe_create_leading_directories(NULL, path); } +enum scld_error safe_create_leading_directories_no_share_const(const char *path) +{ + return safe_create_leading_directories_const(NULL, path); +} + enum scld_error safe_create_leading_directories_const(struct repository *repo, const char *path) { diff --git a/path.h b/path.h index 7e7408dd05..922bd6e377 100644 --- a/path.h +++ b/path.h @@ -234,14 +234,11 @@ int safe_create_dir_in_gitdir(struct repository *repo, const char *path); * race, callers might want to try invoking the function again when it * returns SCLD_VANISHED. * - * safe_create_leading_directories() temporarily changes path while it - * is working but restores it before returning. - * safe_create_leading_directories_const() doesn't modify path, even - * temporarily. Both these variants adjust the permissions of the - * created directories to honor core.sharedRepository, so they are best - * suited for files inside the git dir. For working tree files, use - * safe_create_leading_directories_no_share() instead, as it ignores - * the core.sharedRepository setting. + * The default variants honor "core.sharedRepository" and temporarily modify + * `path`. Note that this configuration should be honored for all files in the + * git directory. The `no_share()` variants ignore "core.sharedRepository", + * and should be used for working tree files. The `const()` variants do not + * modify `path`. */ enum scld_error { SCLD_OK = 0, @@ -254,6 +251,7 @@ enum scld_error safe_create_leading_directories(struct repository *repo, char *p enum scld_error safe_create_leading_directories_const(struct repository *repo, const char *path); enum scld_error safe_create_leading_directories_no_share(char *path); +enum scld_error safe_create_leading_directories_no_share_const(const char *path); /* * Create a file, potentially creating its leading directories in case they -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 3/7] builtin/init: refactor messy creation of leading directories Message-ID: <20260928-pks-create-repository-stateless-v2-3-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` When creating a new repository via git-init(1) we potentially have to create any leading directories via `safe_create_leading_directories()`. This function optionally knows to handle "core.sharedRepository" to adjust the permissions of the created directories. The value of that setting is taken from the passed-in repository. When creating a new repository we don't want to honor it though, so we painstakingly: 1. Save the current value of that setting. 2. Set it to 0. 3. Create the directory with `safe_create_leading_directories()`. This has the effect that `adjust_shared_perm()` will exit early and not adjust permissions. 4. Restore the old value. This is extremely awkward, but it achieves the desired effect that we ignore the configuration. There's a significantly easier way to achieve this though: we can just call the `_no_share()` variant, whose entire purpose it is to ignore "core.sharedRepository". Refactor the code to use that variant accordingly. Signed-off-by: Patrick Steinhardt --- builtin/init-db.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index 5c22eae2f3..e45268f1ff 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -131,15 +131,7 @@ int cmd_init_db(int argc, retry: if (chdir(argv[0]) < 0) { if (!mkdir_tried) { - int saved; - /* - * At this point we haven't read any configuration, - * and we know shared_repository should always be 0; - * but just in case we play safe. - */ - saved = repo_settings_get_shared_repository(the_repository); - repo_settings_set_shared_repository(the_repository, 0); - switch (safe_create_leading_directories_const(the_repository, argv[0])) { + switch (safe_create_leading_directories_no_share_const(argv[0])) { case SCLD_OK: case SCLD_PERMS: break; @@ -150,7 +142,7 @@ int cmd_init_db(int argc, die_errno(_("cannot mkdir %s"), argv[0]); break; } - repo_settings_set_shared_repository(the_repository, saved); + if (mkdir(argv[0], 0777) < 0) die_errno(_("cannot mkdir %s"), argv[0]); mkdir_tried = 1; -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 4/7] builtin/init: move handling of "core.sharedRepository" into "setup.c" Message-ID: <20260928-pks-create-repository-stateless-v2-4-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` When initializing a new repository via git-init(1) we know to honor "core.sharedRepository" and adjust permissions of newly created files accordingly. The way we propagate that setting is quite awkward though, as we have to set it on the repository that we pass into `create_repository()` and pass it as a parameter. This is because there are two different scopes in play here: - We need to apply it to the repository so that creating the repository's directory uses the correct permissions. - We need to reapply it to the repository after we have created default files so that we know to override any configuration that we have read from the new repository's configuration. The effect of this though is that the repository works as an in-out parameter, which is quite awkward. Refactor the code so that the caller only needs to pass the value. Starting with this change, the passed-in repository can essentially be completely blank as it doesn't carry any state anymore that we'd care about in `create_repository()`. Note that this change in theory also impacts the other caller of `create_repository()` that exists in git-clone(1). But that caller already passes `-1` as a value for this parameter, and neither does that caller modify the repository it passes. So there shouldn't be any change in behaviour here. Signed-off-by: Patrick Steinhardt --- builtin/init-db.c | 3 --- setup.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index e45268f1ff..34215bbf18 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -171,9 +171,6 @@ int cmd_init_db(int argc, die(_("unknown ref storage format '%s'"), ref_format); } - if (init_shared_repository != -1) - repo_settings_set_shared_repository(the_repository, init_shared_repository); - /* * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR * without --bare. Catch the error early. diff --git a/setup.c b/setup.c index f335111d1e..0d0a4abbe6 100644 --- a/setup.c +++ b/setup.c @@ -2896,6 +2896,9 @@ void create_repository(struct repository *repo, */ repo_config(repo, git_default_core_config, NULL); + if (init_shared_repository != -1) + repo_settings_set_shared_repository(repo, init_shared_repository); + safe_create_dir(repo, git_dir, 0); if (!reinit_ok) -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 5/7] builtin/clone: don't apply "core.sharedRepository" to leading dirs Message-ID: <20260928-pks-create-repository-stateless-v2-5-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` When creating a repository via git-clone(1) we create leading directories with `safe_create_leading_directories()`. We have adapted git-init(1) in a preceding commit to instead use the variant of this function that doesn't honor "core.sharedRepository". In that subcommand it didn't have an effect though as we explicitly unset the value of that configuration anyway, so we never honored that config. In git-clone(1) it's a bit of a different thing though: while the repository isn't initialized at the point in time where we call the function, we didn't explicitly unset the value. Consequently we _do_ honor the configuration here, but when it's configured in global- or system-level scope. This divergence doesn't seem to be intentional -- I cannot think of any good reason why git-init(1) and git-clone(1) should have divergent behaviour here. Adapt git-clone(1) to work the same as git-init(1) by also using the `no_share()` variants to create leading directories. Add tests for both commands. Signed-off-by: Patrick Steinhardt --- builtin/clone.c | 4 ++-- t/t1301-shared-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index b14264c33a..e72f8aa325 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1133,7 +1133,7 @@ int cmd_clone(int argc, sigchain_push_common(remove_junk_on_signal); if (!option_bare) { - if (safe_create_leading_directories_const(the_repository, work_tree) < 0) + if (safe_create_leading_directories_no_share_const(work_tree) < 0) die_errno(_("could not create leading directories of '%s'"), work_tree); if (dest_exists) @@ -1153,7 +1153,7 @@ int cmd_clone(int argc, junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL; junk_git_dir = git_dir; } - if (safe_create_leading_directories_const(the_repository, git_dir) < 0) + if (safe_create_leading_directories_no_share_const(git_dir) < 0) die(_("could not create leading directories of '%s'"), git_dir); if (0 <= option_verbosity) { diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 0e0d07a1a1..3bc4bdb038 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -210,4 +210,46 @@ test_expect_success POSIXPERM 'template can set core.sharedrepository' ' test_cmp expect actual ' +test_expect_success POSIXPERM 'init does not apply core.sharedRepository to leading directories' ' + test_config_global core.sharedRepository 0666 && + umask 0077 && + test_when_finished "rm -rf dst" && + git init --bare dst/with/leading/dirs && + cat >expect <<-\EOF && + drwx------ + drwx------ + drwx------ + drwxrwxrwx + EOF + { + test_modebits dst && + test_modebits dst/with && + test_modebits dst/with/leading && + test_modebits dst/with/leading/dirs + } >actual && + test_cmp expect actual +' + +test_expect_success POSIXPERM 'clone does not apply core.sharedRepository to leading directories' ' + test_config_global core.sharedRepository 0666 && + umask 0077 && + test_when_finished "rm -rf source dst" && + git init source && + test_commit -C source initial && + git clone --bare source dst/with/leading/dirs && + cat >expect <<-\EOF && + drwx------ + drwx------ + drwx------ + drwxrwxrwx + EOF + { + test_modebits dst && + test_modebits dst/with && + test_modebits dst/with/leading && + test_modebits dst/with/leading/dirs + } >actual && + test_cmp expect actual +' + test_done -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 6/7] repository: adapt `repo_clear()` to fully reset the repository Message-ID: <20260928-pks-create-repository-stateless-v2-6-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` The function `repo_clear()` can be used to clear a repository's state. The way it's written though it's quite easy for it to accidentally leak some state because we don't make sure to clear the whole structure. Refactor the function to set the whole repository to all-zeroes to avoid any kind of leaking state. Replace calls of `FREE_AND_NULL()` to instead use free(3p) to avoid zeroing out the data twice. Signed-off-by: Patrick Steinhardt --- repository.c | 37 ++++++++++++++++++------------------- repository.h | 2 +- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/repository.c b/repository.c index b857e1c580..e67ff00550 100644 --- a/repository.c +++ b/repository.c @@ -374,60 +374,57 @@ void repo_clear(struct repository *repo) struct hashmap_iter iter; struct strmap_entry *e; - FREE_AND_NULL(repo->gitdir); - FREE_AND_NULL(repo->commondir); - FREE_AND_NULL(repo->prefix); - FREE_AND_NULL(repo->graft_file); - FREE_AND_NULL(repo->index_file); - FREE_AND_NULL(repo->worktree); - FREE_AND_NULL(repo->submodule_prefix); - FREE_AND_NULL(repo->ref_storage_payload); + free(repo->gitdir); + free(repo->commondir); + free(repo->prefix); + free(repo->graft_file); + free(repo->index_file); + free(repo->worktree); + free(repo->submodule_prefix); + free(repo->ref_storage_payload); odb_free(repo->objects); - repo->objects = NULL; if (repo->parsed_objects) parsed_object_pool_clear(repo->parsed_objects); - FREE_AND_NULL(repo->parsed_objects); + free(repo->parsed_objects); repo_settings_clear(repo); repo_config_values_clear(&repo->config_values_private_); if (repo->config) { git_configset_clear(repo->config); - FREE_AND_NULL(repo->config); + free(repo->config); } - if (repo->submodule_cache) { + if (repo->submodule_cache) submodule_cache_free(repo->submodule_cache); - repo->submodule_cache = NULL; - } if (repo->index) { discard_index(repo->index); - FREE_AND_NULL(repo->index); + free(repo->index); } if (repo->hook_config_cache) { hook_cache_clear(repo->hook_config_cache); - FREE_AND_NULL(repo->hook_config_cache); + free(repo->hook_config_cache); } strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */ string_list_clear(&repo->disabled_events, 0); if (repo->promisor_remote_config) { promisor_remote_clear(repo->promisor_remote_config); - FREE_AND_NULL(repo->promisor_remote_config); + free(repo->promisor_remote_config); } if (repo->remote_state) { remote_state_clear(repo->remote_state); - FREE_AND_NULL(repo->remote_state); + free(repo->remote_state); } if (repo->refs_private) { ref_store_release(repo->refs_private); - FREE_AND_NULL(repo->refs_private); + free(repo->refs_private); } strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) @@ -439,6 +436,8 @@ void repo_clear(struct repository *repo) strmap_clear(&repo->worktree_ref_stores, 1); repo_clear_path_cache(&repo->cached_paths); + + memset(repo, 0, sizeof(*repo)); } int repo_read_index(struct repository *repo) diff --git a/repository.h b/repository.h index 11f5c2ed10..2a348012e8 100644 --- a/repository.h +++ b/repository.h @@ -258,6 +258,7 @@ void repo_set_ref_storage_format(struct repository *repo, void initialize_repository(struct repository *repo); RESULT_MUST_BE_USED int repo_init(struct repository *r, const char *gitdir, const char *worktree); +void repo_clear(struct repository *repo); /* * Initialize the repository 'subrepo' as the submodule at the given path. If @@ -273,7 +274,6 @@ int repo_submodule_init(struct repository *subrepo, struct repository *superproject, const char *path, const struct object_id *treeish_name); -void repo_clear(struct repository *repo); /* * Populates the repository's index from its index_file, an index struct will -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:51 Subject: [PATCH v2 7/7] setup: enforce that passed-in repo does not carry relevant state Message-ID: <20260928-pks-create-repository-stateless-v2-7-a03612f703fa@pks.im> In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` In the preceding patches we have refactored `create_repository()` so that the passed-in repository is not used anymore to propagate any kind of state. This was done so that the parameter doesn't act like an in-out parameter, but only as an out parameter that we initialize with the state of the newly created repository. We don't enforce though that the repository _cannot_ be used to propagate state anymore, which makes it quite easy for state to sneak in at a later point again. Ideally, we'd do that by having the function create a newly allocated repository instead of taking a repository as input. But unfortunately, that does not work because we end up calling `repo_config_values()` when we create the "files" ref database, and that function requires that the passed-in repository is `the_repository`. Instead, call `repo_clear()` at the beginning of the function, which gives us a clean slate. Signed-off-by: Patrick Steinhardt --- setup.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/setup.c b/setup.c index 0d0a4abbe6..fa39219d6a 100644 --- a/setup.c +++ b/setup.c @@ -2858,6 +2858,9 @@ void create_repository(struct repository *repo, struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; struct strbuf err = STRBUF_INIT; + repo_clear(repo); + initialize_repository(repo); + if (real_git_dir) { struct stat st; -- 2.56.0.rc2.329.gd58861e689.dirty ``` ## Patrick Steinhardt, 2026-09-28 09:52 Subject: Re: [PATCH 6/7] repository: adapt `repo_clear()` to fully reset the repository Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 09:18:52AM +0000, Karthik Nayak wrote: > Patrick Steinhardt writes: > > diff --git a/repository.c b/repository.c > > index b857e1c580..e67ff00550 100644 > > --- a/repository.c > > +++ b/repository.c > > @@ -439,6 +436,8 @@ void repo_clear(struct repository *repo) > > strmap_clear(&repo->worktree_ref_stores, 1); > > > > repo_clear_path_cache(&repo->cached_paths); > > + > > + memset(repo, 0, sizeof(*repo)); > > The reason we swap `FREE_AND_NULL()` with `free()` is because we anyways > set everything to 0. Okay. > > Or was this referring to the 'already blank' repository? Since > FREE_AND_NULL() can already handle NULL values. Yeah, the only reason I swap to plain free(3p) calls is because it's redundant now with the final call to memset(3p). I think the part about already-blank repositories is not accurate anymore, but it used to be at one point. Let me reword it. > > diff --git a/repository.h b/repository.h > > index 11f5c2ed10..2a348012e8 100644 > > --- a/repository.h > > +++ b/repository.h > > @@ -258,6 +258,7 @@ void repo_set_ref_storage_format(struct repository *repo, > > void initialize_repository(struct repository *repo); > > RESULT_MUST_BE_USED > > int repo_init(struct repository *r, const char *gitdir, const char *worktree); > > +void repo_clear(struct repository *repo); > > > > /* > > * Initialize the repository 'subrepo' as the submodule at the given path. If > > @@ -273,7 +274,6 @@ int repo_submodule_init(struct repository *subrepo, > > struct repository *superproject, > > const char *path, > > const struct object_id *treeish_name); > > -void repo_clear(struct repository *repo); > > > > This is a purely cosmetic move to bring it closer to `repo_init()`, > right? I think it makes sense. Yes, it is. Patrick ``` ## Kaartic Sivaraam, 2026-09-28 12:15 Subject: Re: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: <20260928-pks-create-repository-stateless-v2-0-a03612f703fa@pks.im> ``` On 9/28/26 15:21, Patrick Steinhardt wrote: > > [... snip ...] > > 3: 3a7c197f1b = 3: 8dd89f144a builtin/init: refactor messy creation of leading directories > 4: c3ced666bd = 4: f37db1b17d builtin/init: move handling of "core.sharedRepository" into "setup.c" > 5: 25918a4ff6 = 5: db76d32f2c builtin/clone: don't apply "core.sharedRepository" to leading dirs > 6: a742852675 ! 6: 19388a188c repository: adapt `repo_clear()` to fully reset the repository > @@ Commit message > some state because we don't make sure to clear the whole structure. > > Refactor the function to set the whole repository to all-zeroes to avoid > - any kind of leaking state. While at it, make it a bit more robust when > - called on an already-blank repository. > + any kind of leaking state. Replace calls of `FREE_AND_NULL()` to instead > + use free(3p) to avoid zeroing out the data twice. > s/free(3p)/free/ Rest of the inter-diff looks neat. -- Sivaraam ``` ## Kaartic Sivaraam, 2026-09-28 12:19 Subject: Re: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: <886d145f-ac38-4079-8a96-f09904fc3b10@gmail.com> In-Reply-To: ``` On 9/28/26 17:45, Kaartic Sivaraam wrote: > On 9/28/26 15:21, Patrick Steinhardt wrote: >> >> [... snip ...] > > >> 3:  3a7c197f1b = 3:  8dd89f144a builtin/init: refactor messy creation >> of leading directories >> 4:  c3ced666bd = 4:  f37db1b17d builtin/init: move handling of >> "core.sharedRepository" into "setup.c" >> 5:  25918a4ff6 = 5:  db76d32f2c builtin/clone: don't apply >> "core.sharedRepository" to leading dirs >> 6:  a742852675 ! 6:  19388a188c repository: adapt `repo_clear()` to >> fully reset the repository >>      @@ Commit message >>           some state because we don't make sure to clear the whole >> structure. >>           Refactor the function to set the whole repository to all- >> zeroes to avoid >>      -    any kind of leaking state. While at it, make it a bit more >> robust when >>      -    called on an already-blank repository. >>      +    any kind of leaking state. Replace calls of >> `FREE_AND_NULL()` to instead >>      +    use free(3p) to avoid zeroing out the data twice. >> > > s/free(3p)/free/ Oops. I meant s/free(3p)/free(3)/ > > Rest of the inter-diff looks neat. > -- Sivaraam ``` ## Patrick Steinhardt, 2026-09-28 12:48 Subject: Re: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: ``` On Mon, Sep 28, 2026 at 05:45:00PM +0530, Kaartic Sivaraam wrote: > On 9/28/26 15:21, Patrick Steinhardt wrote: > > > > [... snip ...] > > > > 3: 3a7c197f1b = 3: 8dd89f144a builtin/init: refactor messy creation of leading directories > > 4: c3ced666bd = 4: f37db1b17d builtin/init: move handling of "core.sharedRepository" into "setup.c" > > 5: 25918a4ff6 = 5: db76d32f2c builtin/clone: don't apply "core.sharedRepository" to leading dirs > > 6: a742852675 ! 6: 19388a188c repository: adapt `repo_clear()` to fully reset the repository > > @@ Commit message > > some state because we don't make sure to clear the whole structure. > > Refactor the function to set the whole repository to all-zeroes to avoid > > - any kind of leaking state. While at it, make it a bit more robust when > > - called on an already-blank repository. > > + any kind of leaking state. Replace calls of `FREE_AND_NULL()` to instead > > + use free(3p) to avoid zeroing out the data twice. > > > > s/free(3p)/free/ > Rest of the inter-diff looks neat. The "(3p)" is intentional, as we use that to refer to man pages. In this case, it's free as specified in the POSIX programmer's manual. Thanks! Patrick ``` ## Patrick Steinhardt, 2026-09-28 12:48 Subject: Re: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: <886d145f-ac38-4079-8a96-f09904fc3b10@gmail.com> ``` On Mon, Sep 28, 2026 at 05:49:20PM +0530, Kaartic Sivaraam wrote: > On 9/28/26 17:45, Kaartic Sivaraam wrote: > > On 9/28/26 15:21, Patrick Steinhardt wrote: > > > > > > [... snip ...] > > > > > > 3:  3a7c197f1b = 3:  8dd89f144a builtin/init: refactor messy > > > creation of leading directories > > > 4:  c3ced666bd = 4:  f37db1b17d builtin/init: move handling of > > > "core.sharedRepository" into "setup.c" > > > 5:  25918a4ff6 = 5:  db76d32f2c builtin/clone: don't apply > > > "core.sharedRepository" to leading dirs > > > 6:  a742852675 ! 6:  19388a188c repository: adapt `repo_clear()` to > > > fully reset the repository > > >      @@ Commit message > > >           some state because we don't make sure to clear the whole > > > structure. > > >           Refactor the function to set the whole repository to all- > > > zeroes to avoid > > >      -    any kind of leaking state. While at it, make it a bit more > > > robust when > > >      -    called on an already-blank repository. > > >      +    any kind of leaking state. Replace calls of > > > `FREE_AND_NULL()` to instead > > >      +    use free(3p) to avoid zeroing out the data twice. > > > > > > > s/free(3p)/free/ > > Oops. I meant s/free(3p)/free(3)/ Ah. 3p is correct though and refers to the POSIX man pages. Patrick ``` ## Junio C Hamano, 2026-09-28 15:59 Subject: Re: [PATCH v2 0/7] setup: enforce repo passed to `create_repository()` has no state Message-ID: In-Reply-To: ``` Patrick Steinhardt writes: >> Oops. I meant s/free(3p)/free(3)/ > > Ah. 3p is correct though and refers to the POSIX man pages. If used in a context where you really care about posix specified behaviour and are interferred by differences among generic C library's free() implementations, free(3p) may be the right way to spell it out concisely. Everywhere else, like in this patch where you do not care about the distinction, the extra 'p' is merely a noise, I would have to say. If you are writing a wrapper that _depends_ on your platform free() being strictly posix compliant, then you might write something like #ifdef WE_HAVE_POSIX_FREE #define safe_free(x) free(x) #else static void safe_free(void *x) { ... } #endif and your commit log message may say "We use free(3p) where available, but otherwise emulate it via platform free() with some safety knob". ```