{"thread":{"id":"66249","subject":"[PATCH 0/2] fix a leak in submodule error path","startedAt":"2026-09-02T05:51:26Z","lastAt":"2026-09-03T05:15:52Z","messageCount":9,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"551721","messageId":"20260902055117.GA41587@coredump.intra.peff.net","threadId":"66249","inReplyTo":null,"subject":"[PATCH 0/2] fix a leak in submodule error path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-02T05:51:17Z","receivedAt":"2026-09-02T05:51:26Z","isPatch":true,"body":"This fixes a small leak noticed by Coverity. I think it has been there\nfor a while, but nearby code movement caused it to be marked as \"new\".\n\n  [1/2]: repository: make repo_clear() idempotent\n  [2/2]: submodule--helper: free URL when repository setup fails\n\n builtin/submodule--helper.c             | 10 +++++++---\n repository.c                            |  3 ++-\n t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++\n 3 files changed, 26 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"551722","messageId":"20260902055526.GA41747@coredump.intra.peff.net","threadId":"66249","inReplyTo":"20260902055117.GA41587@coredump.intra.peff.net","subject":"[PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-02T05:55:26Z","receivedAt":"2026-09-02T05:55:28Z","isPatch":true,"body":"Calling repo_clear() twice in a row will segfault because the second\ncall will invoke parse_object_pool_clear() on a NULL pointer. This is\nnot usually a big deal, but we can make some error cleanup a little\nsimpler if callers do not need to worry about invoking it twice.\n\nWe can fix it by catching the NULL case. The rest of repo_clear()\nappears to be idempotent.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis helps in the next patch, but I think it's also just the path of\nleast surprise.\n\nArguably this should not be a pointer at all, but the pool code is\nweirdly asymmetric. It offers only \"new\" which allocates a struct, but\nonly \"clear\" to clean it up (but not deallocate). Might be worth fixing,\nbut out of scope for this series.\n\n repository.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/repository.c b/repository.c\nindex db4f9d006e..33ad8984bc 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -385,7 +385,8 @@ void repo_clear(struct repository *repo)\n \todb_free(repo->objects);\n \trepo->objects = NULL;\n \n-\tparsed_object_pool_clear(repo->parsed_objects);\n+\tif (repo->parsed_objects)\n+\t\tparsed_object_pool_clear(repo->parsed_objects);\n \tFREE_AND_NULL(repo->parsed_objects);\n \n \trepo_settings_clear(repo);\n-- \n2.55.0.1067.gf7fc94a55c\n\n"},{"id":"551723","messageId":"20260902055730.GB41747@coredump.intra.peff.net","threadId":"66249","inReplyTo":"20260902055117.GA41587@coredump.intra.peff.net","subject":"[PATCH 2/2] submodule--helper: free URL when repository setup fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-02T05:57:30Z","receivedAt":"2026-09-02T05:57:32Z","isPatch":true,"body":"If repo setup fails, we'll return an error without freeing the allocated\nurl string, leaking the memory. The test suite does trigger this error,\nbut never with the leak. We only allocate a url if submodule_from_path()\nreturned something, but our tests use other situations, like totally\nnonexistent submodules.\n\nWe can cover this case by asking about a submodule that exists but which\nhas not been initialized. The new test fails with SANITIZE=leak.\n\nThe smallest fix would just be a call to free(url), but I think it's a\nlittle nicer to set up a dedicated out-path for cleanup here. The\nprevious commit made it safe to call repo_clear() even if\nrepo_submodule_init() fails.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/submodule--helper.c             | 10 +++++++---\n t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e7cd3225fa..469e3dbcc9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -80,6 +80,7 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n \tstruct repository subrepo;\n \tconst char *remote_name = NULL;\n \tchar *url = NULL;\n+\tint ret = 0;\n \n \tsub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);\n \tif (sub && sub->url) {\n@@ -96,9 +97,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n \t}\n \n \tif (repo_submodule_init(&subrepo, the_repository, module_path,\n-\t\t\t\tnull_oid(the_hash_algo)) < 0)\n-\t\treturn die_message(_(\"could not get a repository handle for submodule '%s'\"),\n+\t\t\t\tnull_oid(the_hash_algo)) < 0) {\n+\t\tret = die_message(_(\"could not get a repository handle for submodule '%s'\"),\n \t\t\t\t   module_path);\n+\t\tgoto out;\n+\t}\n \n \t/* Look up by URL first */\n \tif (url)\n@@ -108,10 +111,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n \n \t*default_remote = xstrdup(remote_name);\n \n+out:\n \trepo_clear(&subrepo);\n \tfree(url);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int module_get_default_remote(int argc, const char **argv, const char *prefix,\ndiff --git a/t/t7426-submodule-get-default-remote.sh b/t/t7426-submodule-get-default-remote.sh\nindex b842af9a2d..0379c9f044 100755\n--- a/t/t7426-submodule-get-default-remote.sh\n+++ b/t/t7426-submodule-get-default-remote.sh\n@@ -60,6 +60,23 @@ test_expect_success 'get-default-remote fails with non-submodule path' '\n \t)\n '\n \n+test_expect_success 'get-default-remote fails with uninitialized submodule' '\n+\ttest_when_finished \"\n+\t\tgit -C super config -f .gitmodules --remove-section submodule.uninitialized &&\n+\t\tgit -C super update-index --force-remove uninitialized\n+\t\" &&\n+\t(\n+\t\tcd super &&\n+\t\tgit config -f .gitmodules submodule.uninitialized.path uninitialized &&\n+\t\tgit config -f .gitmodules submodule.uninitialized.url ../sub &&\n+\t\thead=$(git -C ../sub rev-parse HEAD) &&\n+\t\tgit update-index --add --cacheinfo 160000,$head,uninitialized &&\n+\t\ttest_must_fail git submodule--helper get-default-remote \\\n+\t\t\tuninitialized 2>err &&\n+\t\ttest_grep \"could not get a repository handle\" err\n+\t)\n+'\n+\n test_expect_success 'get-default-remote fails without path argument' '\n \t(\n \t\tcd super &&\n-- \n2.55.0.1067.gf7fc94a55c\n"},{"id":"551725","messageId":"20260902062940.GA47676@coredump.intra.peff.net","threadId":"66249","inReplyTo":"20260902055526.GA41747@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-02T06:29:40Z","receivedAt":"2026-09-02T06:29:41Z","isPatch":true,"body":"On Wed, Sep 02, 2026 at 01:55:27AM -0400, Jeff King wrote:\n\n> Arguably this should not be a pointer at all, but the pool code is\n> weirdly asymmetric. It offers only \"new\" which allocates a struct, but\n> only \"clear\" to clean it up (but not deallocate). Might be worth fixing,\n> but out of scope for this series.\n\nI took a quick stab at this, and it gets ugly. There is no mutual\nrecursion between the parsed_object_pool and repository struct\ndefinitions, but we do end up in a header include loop:\n\n  - repository.h would need object.h (to include the pool struct)\n\n  - object.h includes hash.h for object_id, etc\n\n  - hash.h (sometimes) includes repository.h so it can define\n    the_hash_algo when USE_THE_REPOSITORY_VARIABLE is defined\n\nWe could break the cycle if we had a separate the-repository.h which\nlooked like this:\n\n  struct repository;\n  extern struct repository *the_repository;\n\nand then included that from hash.h. But then callers which want to use\nthe_hash_algo would need to include repository.h themselves. It is\njust a macro looking at the_repository->hash_algo, so they need the\nactual repository definition. About 9 files need to start including\nrepository.h themselves to make it work. Though a few of them _ought_ to\nbe including it anyway; they are not using the_hash_algo at all, but\njust lucky that hash.h happens to bring repository.h when\nUSE_THE_REPOSITORY_VARIABLE is set.\n\nAn alternative would be to define the_hash_algo as its own pointer,\nlike:\n\ndiff --git a/hash.h b/hash.h\nindex cf94ad5700..9e21ac6480 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -269,8 +269,7 @@ enum get_oid_result {\n };\n \n #ifdef USE_THE_REPOSITORY_VARIABLE\n-# include \"repository.h\"\n-# define the_hash_algo the_repository->hash_algo\n+extern struct git_hash_algo *the_hash_algo;\n #endif\n \n /* A suitably aligned type for stack allocations of hash contexts. */\ndiff --git a/repository.c b/repository.c\nindex db4f9d006e..f70c4deecf 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -30,6 +30,7 @@ extern struct repository *the_repository;\n /* The main repository */\n static struct repository the_repo;\n struct repository *the_repository = &the_repo;\n+struct the_hash_algo = &the_repo->hash_algo;\n \n /*\n  * An escape hatch: if we hit a bug in the production code that fails\n\n\nThat makes the_hash_algo just work without most code caring about\nrepositories at all. But of course it reveals yet more spots which are\nrelying on hash.h mentioning the_repository. :-/\n\nI'm not sure how much it's worth untangling all of this, but probably\nnot enough just to remove pointer indirection from repo->parsed_objects.\n\n-Peff\n"},{"id":"551726","messageId":"20260902064907.GB47676@coredump.intra.peff.net","threadId":"66249","inReplyTo":"20260902062940.GA47676@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-02T06:49:07Z","receivedAt":"2026-09-02T06:49:13Z","isPatch":true,"body":"On Wed, Sep 02, 2026 at 02:29:40AM -0400, Jeff King wrote:\n\n> I'm not sure how much it's worth untangling all of this, but probably\n> not enough just to remove pointer indirection from repo->parsed_objects.\n\nBTW, another curiosity: parsed_objects contains a pointer back to the\nrepo that contains it! What could it possibly depend on in the repo\nitself?\n\nAs far as I can tell, the answer is nothing. We only ever access p->repo\nin order to get to p->repo->parsed_objects, which will always be the\nsame as our original \"p\". There are some internal functions within\nobject.c which could be simplified by passing around the\nparsed_object_pool directly.  But we also call lookup_commit() and a few\nother public functions, all of which take a repository struct. Even\nthough they only use it to look at the parsed_objects field!\n\nStructurally speaking these should be operating on a parsed_object_pool,\nsince that's all they need. But from the caller's point of view that is\njust an implementation detail, and it is easier to pass in the whole\nrepository.\n\nSo we'd probably need to provide functions that operate directly on the\npool like:\n\n  struct commit *lookup_commit_via_pool(struct parsed_object_pool *p,\n                                        const struct object_id *oid);\n\nand then maintain wrappers like:\n\n  struct commit *lookup_commit(struct repository *r,\n                               const struct object_id *oid)\n  {\n\treturn lookup_commit_via_pool(r->parsed_objects, oid);\n  }\n\nto avoid rewriting every caller with r->parsed_objects themselves.\n\nThe patch below illustrates the minimal change to drop the repo pointer\nfrom parsed_object_pool. I think it more accurately represents the\nactual dependencies of the data structures, but it's a fair bit of churn\nfor a minor amount of clarity. Probably not worth it.\n\n---\n alloc.c  |  9 ++++--\n alloc.h  |  1 +\n commit.c | 20 +++++++++----\n commit.h |  3 ++\n object.c | 55 ++++++++++++++++++++---------------\n object.h |  3 +-\n 6 files changed, 60 insertions(+), 31 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 533a045c2a..e7fb90734b 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -121,9 +121,14 @@ void init_commit_node(struct commit *c)\n \tc->index = alloc_commit_index();\n }\n \n-void *alloc_commit_node(struct repository *r)\n+void *alloc_commit_node_via_pool(struct parsed_object_pool *p)\n {\n-\tstruct commit *c = alloc_node(r->parsed_objects->commit_state, sizeof(struct commit));\n+\tstruct commit *c = alloc_node(p->commit_state, sizeof(struct commit));\n \tinit_commit_node(c);\n \treturn c;\n }\n+\n+void *alloc_commit_node(struct repository *r)\n+{\n+\treturn alloc_commit_node_via_pool(r->parsed_objects);\n+}\ndiff --git a/alloc.h b/alloc.h\nindex 87a47a9709..70c9c726c1 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -11,6 +11,7 @@ void *alloc_blob_node(struct repository *r);\n void *alloc_tree_node(struct repository *r);\n void init_commit_node(struct commit *c);\n void *alloc_commit_node(struct repository *r);\n+void *alloc_commit_node_via_pool(struct parsed_object_pool *p);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \ndiff --git a/commit.c b/commit.c\nindex ad26f0b40a..407e11f00b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -98,14 +98,19 @@ struct commit *lookup_commit_object(struct repository *r,\n \n }\n \n-struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+struct commit *lookup_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid)\n {\n-\tstruct object *obj = lookup_object(r, oid);\n+\tstruct object *obj = lookup_object_via_pool(p, oid);\n \tif (!obj)\n-\t\treturn create_object(r, oid, alloc_commit_node(r));\n+\t\treturn create_object_via_pool(p, oid, alloc_commit_node_via_pool(p));\n \treturn object_as_type(obj, OBJ_COMMIT, 0);\n }\n \n+struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_commit_via_pool(r->parsed_objects, oid);\n+}\n+\n struct commit *lookup_commit_reference_by_name(const char *name)\n {\n \treturn lookup_commit_reference_by_name_gently(name, 0);\n@@ -206,9 +211,9 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)\n \t\t       commit_graft_oid_access);\n }\n \n-void unparse_commit(struct repository *r, const struct object_id *oid)\n+void unparse_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid)\n {\n-\tstruct commit *c = lookup_commit(r, oid);\n+\tstruct commit *c = lookup_commit_via_pool(p, oid);\n \n \tif (!c->object.parsed)\n \t\treturn;\n@@ -217,6 +222,11 @@ void unparse_commit(struct repository *r, const struct object_id *oid)\n \tc->object.parsed = 0;\n }\n \n+void unparse_commit(struct repository *repo, const struct object_id *oid)\n+{\n+\tunparse_commit_via_pool(repo->parsed_objects, oid);\n+}\n+\n int register_commit_graft(struct repository *r, struct commit_graft *graft,\n \t\t\t  int ignore_dups)\n {\ndiff --git a/commit.h b/commit.h\nindex 1061ed791b..972db23558 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -76,6 +76,8 @@ struct commit *lookup_commit_object(struct repository *r, const struct object_id\n  * \"oid\" is not in the object cache.\n  */\n struct commit *lookup_commit(struct repository *r, const struct object_id *oid);\n+struct commit *lookup_commit_via_pool(struct parsed_object_pool *pool,\n+\t\t\t\t      const struct object_id *oid);\n struct commit *lookup_commit_reference(struct repository *r,\n \t\t\t\t       const struct object_id *oid);\n struct commit *lookup_commit_reference_gently(struct repository *r,\n@@ -104,6 +106,7 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n }\n \n void unparse_commit(struct repository *r, const struct object_id *oid);\n+void unparse_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid);\n \n static inline int repo_parse_commit_no_graph(struct repository *r,\n \t\t\t\t\t     struct commit *commit)\ndiff --git a/object.c b/object.c\nindex 97f7fc0e87..c6b0815240 100644\n--- a/object.c\n+++ b/object.c\n@@ -89,20 +89,20 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i\n  * Look up the record for the given sha1 in the hash map stored in\n  * obj_hash.  Return NULL if it was not found.\n  */\n-struct object *lookup_object(struct repository *r, const struct object_id *oid)\n+struct object *lookup_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid)\n {\n \tunsigned int i, first;\n \tstruct object *obj;\n \n-\tif (!r->parsed_objects->obj_hash)\n+\tif (!p->obj_hash)\n \t\treturn NULL;\n \n-\tfirst = i = hash_obj(oid, r->parsed_objects->obj_hash_size);\n-\twhile ((obj = r->parsed_objects->obj_hash[i]) != NULL) {\n+\tfirst = i = hash_obj(oid, p->obj_hash_size);\n+\twhile ((obj = p->obj_hash[i]) != NULL) {\n \t\tif (oideq(oid, &obj->oid))\n \t\t\tbreak;\n \t\ti++;\n-\t\tif (i == r->parsed_objects->obj_hash_size)\n+\t\tif (i == p->obj_hash_size)\n \t\t\ti = 0;\n \t}\n \tif (obj && i != first) {\n@@ -111,57 +111,66 @@ struct object *lookup_object(struct repository *r, const struct object_id *oid)\n \t\t * that we do not need to walk the hash table the next\n \t\t * time we look for it.\n \t\t */\n-\t\tSWAP(r->parsed_objects->obj_hash[i],\n-\t\t     r->parsed_objects->obj_hash[first]);\n+\t\tSWAP(p->obj_hash[i],\n+\t\t     p->obj_hash[first]);\n \t}\n \treturn obj;\n }\n \n+struct object *lookup_object(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_object_via_pool(r->parsed_objects, oid);\n+}\n+\n /*\n  * Increase the size of the hash map stored in obj_hash to the next\n  * power of 2 (but at least 32).  Copy the existing values to the new\n  * hash map.\n  */\n-static void grow_object_hash(struct repository *r)\n+static void grow_object_hash(struct parsed_object_pool *p)\n {\n \tint i;\n \t/*\n \t * Note that this size must always be power-of-2 to match hash_obj\n \t * above.\n \t */\n-\tint new_hash_size = r->parsed_objects->obj_hash_size < 32 ? 32 : 2 * r->parsed_objects->obj_hash_size;\n+\tint new_hash_size = p->obj_hash_size < 32 ? 32 : 2 * p->obj_hash_size;\n \tstruct object **new_hash;\n \n \tCALLOC_ARRAY(new_hash, new_hash_size);\n-\tfor (i = 0; i < r->parsed_objects->obj_hash_size; i++) {\n-\t\tstruct object *obj = r->parsed_objects->obj_hash[i];\n+\tfor (i = 0; i < p->obj_hash_size; i++) {\n+\t\tstruct object *obj = p->obj_hash[i];\n \n \t\tif (!obj)\n \t\t\tcontinue;\n \t\tinsert_obj_hash(obj, new_hash, new_hash_size);\n \t}\n-\tfree(r->parsed_objects->obj_hash);\n-\tr->parsed_objects->obj_hash = new_hash;\n-\tr->parsed_objects->obj_hash_size = new_hash_size;\n+\tfree(p->obj_hash);\n+\tp->obj_hash = new_hash;\n+\tp->obj_hash_size = new_hash_size;\n }\n \n-void *create_object(struct repository *r, const struct object_id *oid, void *o)\n+void *create_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid, void *o)\n {\n \tstruct object *obj = o;\n \n \tobj->parsed = 0;\n \tobj->flags = 0;\n \toidcpy(&obj->oid, oid);\n \n-\tif (r->parsed_objects->obj_hash_size - 1 <= r->parsed_objects->nr_objs * 2)\n-\t\tgrow_object_hash(r);\n+\tif (p->obj_hash_size - 1 <= p->nr_objs * 2)\n+\t\tgrow_object_hash(p);\n \n-\tinsert_obj_hash(obj, r->parsed_objects->obj_hash,\n-\t\t\tr->parsed_objects->obj_hash_size);\n-\tr->parsed_objects->nr_objs++;\n+\tinsert_obj_hash(obj, p->obj_hash, p->obj_hash_size);\n+\tp->nr_objs++;\n \treturn obj;\n }\n \n+void *create_object(struct repository *r, const struct object_id *oid, void *o)\n+{\n+\treturn create_object_via_pool(r->parsed_objects, oid, o);\n+}\n+\n void *object_as_type(struct object *obj, enum object_type type, int quiet)\n {\n \tif (obj->type == type)\n@@ -551,12 +560,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)\n \t}\n }\n \n-struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo\n+\t\t\t\t\t\t  UNUSED)\n {\n \tstruct parsed_object_pool *o = xmalloc(sizeof(*o));\n \tmemset(o, 0, sizeof(*o));\n \n-\to->repo = repo;\n \to->blob_state = alloc_state_alloc();\n \to->tree_state = alloc_state_alloc();\n \to->commit_state = alloc_state_alloc();\n@@ -573,7 +582,7 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)\n {\n \tfor (int i = 0; i < o->grafts_nr; i++) {\n-\t\tunparse_commit(o->repo, &o->grafts[i]->oid);\n+\t\tunparse_commit_via_pool(o, &o->grafts[i]->oid);\n \t\tfree(o->grafts[i]);\n \t}\n \to->grafts_nr = 0;\ndiff --git a/object.h b/object.h\nindex 8fb03ff90a..2bab336913 100644\n--- a/object.h\n+++ b/object.h\n@@ -7,7 +7,6 @@ struct buffer_slab;\n struct repository;\n \n struct parsed_object_pool {\n-\tstruct repository *repo;\n \tstruct object **obj_hash;\n \tint nr_objs, obj_hash_size;\n \n@@ -191,8 +190,10 @@ struct object *get_indexed_object(const struct repository *repo,\n  * by calling parse_object() on them.\n  */\n struct object *lookup_object(struct repository *r, const struct object_id *oid);\n+struct object *lookup_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid);\n \n void *create_object(struct repository *r, const struct object_id *oid, void *obj);\n+void *create_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid, void *obj);\n \n void *object_as_type(struct object *obj, enum object_type type, int quiet);\n \n"},{"id":"551736","messageId":"apfoNaZL8dg9OpbL@pks.im","threadId":"66249","inReplyTo":"20260902064907.GB47676@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-02T09:11:17Z","receivedAt":"2026-09-02T09:11:28Z","isPatch":true,"body":"On Wed, Sep 02, 2026 at 02:49:07AM -0400, Jeff King wrote:\n> On Wed, Sep 02, 2026 at 02:29:40AM -0400, Jeff King wrote:\n> \n> > I'm not sure how much it's worth untangling all of this, but probably\n> > not enough just to remove pointer indirection from repo->parsed_objects.\n> \n> BTW, another curiosity: parsed_objects contains a pointer back to the\n> repo that contains it! What could it possibly depend on in the repo\n> itself?\n> \n> As far as I can tell, the answer is nothing. We only ever access p->repo\n> in order to get to p->repo->parsed_objects, which will always be the\n> same as our original \"p\". There are some internal functions within\n> object.c which could be simplified by passing around the\n> parsed_object_pool directly.  But we also call lookup_commit() and a few\n> other public functions, all of which take a repository struct. Even\n> though they only use it to look at the parsed_objects field!\n> \n> Structurally speaking these should be operating on a parsed_object_pool,\n> since that's all they need. But from the caller's point of view that is\n> just an implementation detail, and it is easier to pass in the whole\n> repository.\n\nI was at one point wondering whether the parsed object pool should\nreally be an implementation detail of the object database -- parsing\nobjects should not have to depend on the repository, but it really\nshould only interact with the object database. I hacked together a\nseries, but it grew _huge_ because I was of course also trying to bubble\nup the changes into all subsystems that do parse objects rigth now. So I\ndiscarded that idea eventually.\n\nBut I think making the parsed object pool become more self-contained is\na step into the right direction.\n\n> So we'd probably need to provide functions that operate directly on the\n> pool like:\n> \n>   struct commit *lookup_commit_via_pool(struct parsed_object_pool *p,\n>                                         const struct object_id *oid);\n> \n> and then maintain wrappers like:\n> \n>   struct commit *lookup_commit(struct repository *r,\n>                                const struct object_id *oid)\n>   {\n> \treturn lookup_commit_via_pool(r->parsed_objects, oid);\n>   }\n> \n> to avoid rewriting every caller with r->parsed_objects themselves.\n\nI dunno. If we want to make this switch I'd say that we should go all or\nnothing. Otherwise, if we retain both interfaces, I don't really feel\nlike it gains us anything at all.\n\n> The patch below illustrates the minimal change to drop the repo pointer\n> from parsed_object_pool. I think it more accurately represents the\n> actual dependencies of the data structures, but it's a fair bit of churn\n> for a minor amount of clarity. Probably not worth it.\n\nI think there is value in it. While deglobalizing our state we tend to\njust pass the repository explicitly into the subsystems, which is a good\nstep. But I think it really should only be the first step, where the\nnext step would be to reduce the state we pass around. So ideally,\nsubsystems should really only receive as input what they actually need.\nThis would eventually ensure that our subsystems are more self-contained \nand that they can be used more flexibly.\n\nThanks!\n\nPatrick\n"},{"id":"551737","messageId":"apfoO5br4MMZv7nR@pks.im","threadId":"66249","inReplyTo":"20260902055730.GB41747@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] submodule--helper: free URL when repository setup fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-02T09:11:23Z","receivedAt":"2026-09-02T09:11:33Z","isPatch":true,"body":"On Wed, Sep 02, 2026 at 01:57:30AM -0400, Jeff King wrote:\n> If repo setup fails, we'll return an error without freeing the allocated\n> url string, leaking the memory. The test suite does trigger this error,\n> but never with the leak. We only allocate a url if submodule_from_path()\n> returned something, but our tests use other situations, like totally\n> nonexistent submodules.\n> \n> We can cover this case by asking about a submodule that exists but which\n> has not been initialized. The new test fails with SANITIZE=leak.\n> \n> The smallest fix would just be a call to free(url), but I think it's a\n> little nicer to set up a dedicated out-path for cleanup here. The\n> previous commit made it safe to call repo_clear() even if\n> repo_submodule_init() fails.\n\nAgreed.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/submodule--helper.c             | 10 +++++++---\n>  t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++\n>  2 files changed, 24 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index e7cd3225fa..469e3dbcc9 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -80,6 +80,7 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n>  \tstruct repository subrepo;\n>  \tconst char *remote_name = NULL;\n>  \tchar *url = NULL;\n> +\tint ret = 0;\n>  \n>  \tsub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);\n>  \tif (sub && sub->url) {\n\nNit, feel free to ignore: do we want to keep the value uninitialized\nand...\n\n> @@ -96,9 +97,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n>  \t}\n>  \n>  \tif (repo_submodule_init(&subrepo, the_repository, module_path,\n> -\t\t\t\tnull_oid(the_hash_algo)) < 0)\n> -\t\treturn die_message(_(\"could not get a repository handle for submodule '%s'\"),\n> +\t\t\t\tnull_oid(the_hash_algo)) < 0) {\n> +\t\tret = die_message(_(\"could not get a repository handle for submodule '%s'\"),\n>  \t\t\t\t   module_path);\n> +\t\tgoto out;\n> +\t}\n>  \n>  \t/* Look up by URL first */\n>  \tif (url)\n> @@ -108,10 +111,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n>  \n>  \t*default_remote = xstrdup(remote_name);\n>  \n\n... set it to 0 here? Many compilers would warn in case the value was\nuninitialized, which ensures that the return value is being explicitly\nset before every `goto out`.\n\n> +out:\n>  \trepo_clear(&subrepo);\n>  \tfree(url);\n>  \n> -\treturn 0;\n> +\treturn ret;\n>  }\n>  \n>  static int module_get_default_remote(int argc, const char **argv, const char *prefix,\n> diff --git a/t/t7426-submodule-get-default-remote.sh b/t/t7426-submodule-get-default-remote.sh\n> index b842af9a2d..0379c9f044 100755\n> --- a/t/t7426-submodule-get-default-remote.sh\n> +++ b/t/t7426-submodule-get-default-remote.sh\n> @@ -60,6 +60,23 @@ test_expect_success 'get-default-remote fails with non-submodule path' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'get-default-remote fails with uninitialized submodule' '\n> +\ttest_when_finished \"\n> +\t\tgit -C super config -f .gitmodules --remove-section submodule.uninitialized &&\n> +\t\tgit -C super update-index --force-remove uninitialized\n> +\t\" &&\n\nI was about to say we could use `test_config` instead, but you're of\ncourse not modifying the normal \".git/config\" file but \".gitmodules\".\n\n> +\t(\n> +\t\tcd super &&\n> +\t\tgit config -f .gitmodules submodule.uninitialized.path uninitialized &&\n> +\t\tgit config -f .gitmodules submodule.uninitialized.url ../sub &&\n> +\t\thead=$(git -C ../sub rev-parse HEAD) &&\n> +\t\tgit update-index --add --cacheinfo 160000,$head,uninitialized &&\n> +\t\ttest_must_fail git submodule--helper get-default-remote \\\n> +\t\t\tuninitialized 2>err &&\n> +\t\ttest_grep \"could not get a repository handle\" err\n> +\t)\n> +'\n\nThanks!\n\nPatrick\n"},{"id":"551787","messageId":"xmqqpkyvk45f.fsf@gitster.g","threadId":"66249","inReplyTo":"apfoNaZL8dg9OpbL@pks.im","subject":"Re: [PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-02T16:29:48Z","receivedAt":"2026-09-02T16:29:51Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I was at one point wondering whether the parsed object pool should\n> really be an implementation detail of the object database -- parsing\n> objects should not have to depend on the repository, ...\n\nWe need to be a bit careful here, as the above directly contradicts\nour earlier design choice to have things like hash algorithm as\nproperties of a repository instance.\n\n> This would eventually ensure that our subsystems are more self-contained \n> and that they can be used more flexibly.\n\nI do agree with this line of thinking, though.\n\nThanks.\n"},{"id":"551827","messageId":"apkCe9CKvYvM6Hy9@pks.im","threadId":"66249","inReplyTo":"xmqqpkyvk45f.fsf@gitster.g","subject":"Re: [PATCH 1/2] repository: make repo_clear() idempotent","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-03T05:15:39Z","receivedAt":"2026-09-03T05:15:52Z","isPatch":true,"body":"On Wed, Sep 02, 2026 at 09:29:48AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > I was at one point wondering whether the parsed object pool should\n> > really be an implementation detail of the object database -- parsing\n> > objects should not have to depend on the repository, ...\n> \n> We need to be a bit careful here, as the above directly contradicts\n> our earlier design choice to have things like hash algorithm as\n> properties of a repository instance.\n\nYeah, true. The question though is whether it really makes sense to\nalways propagate down the full repository, or whether we should instead\npropagate only what matters. We do of course require the hash algorithm\n(and potentially the compatibility hash algorithm) in these subsystems,\nbut propagating these two pieces of information might be the saner\napproach compared to always making the full repository available.\n\nIn any case, I eventually discarded the above idea anyway.\n\nPatrick\n"}]}