From: Derrick Stolee Date: Wed, 07 May 2025 01:10:06 GMT Subject: Re: [PATCH 05/17] odb: get rid of `the_repository` in `find_odb()` Message-ID: In-Reply-To: <20250506-pks-object-store-wo-the-repository-v1-5-c05b82e7b126@pks.im> On 5/6/25 7:09 AM, Patrick Steinhardt wrote: > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 0a184d39720..28550d3bcc9 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -66,7 +66,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix, > struct repository *repo UNUSED) > { > struct commit_graph *graph = NULL; > - struct odb_backend *odb = NULL; > + struct odb_backend *backend = NULL; nit: this looks like a misplaced variable rename that should be in an earlier patch. > @@ -101,9 +101,9 @@ static int graph_verify(int argc, const char **argv, const char *prefix, > if (opts.progress) > flags |= COMMIT_GRAPH_WRITE_PROGRESS; > > - odb = find_odb(the_repository, opts.obj_dir); > - graph_name = get_commit_graph_filename(odb); > - chain_name = get_commit_graph_chain_filename(odb); > + backend = odb_find_backend(the_repository->objects, opts.obj_dir); > + graph_name = get_commit_graph_filename(backend); > + chain_name = get_commit_graph_chain_filename(backend); I like how your use of specific backends here cleaans up a lot of garbage around the --object-dir parameter of the commit-graph builtin. This is a real structural improvement that is made possible by this refactoring. > @@ -221,7 +221,7 @@ static int graph_write(int argc, const char **argv, const char *prefix, > struct string_list pack_indexes = STRING_LIST_INIT_DUP; > struct strbuf buf = STRBUF_INIT; > struct oidset commits = OIDSET_INIT; > - struct odb_backend *odb = NULL; > + struct odb_backend *backend = NULL; Here's another of those delayed renames. > -struct odb_backend *find_odb(struct repository *r, const char *obj_dir) > +struct odb_backend *odb_find_backend(struct object_database *odb, const char *obj_dir) I was looking at this implementation and wondering why it wasn't renamed earlier, but upon closer inspection I agree that the rename is worthwhile _and_ the method shouldn't have been changed until now. > { > - struct odb_backend *odb; > + struct odb_backend *backend; > char *obj_dir_real = real_pathdup(obj_dir, 1); > struct strbuf odb_path_real = STRBUF_INIT; > > - prepare_alt_odb(r); > - for (odb = r->objects->backends; odb; odb = odb->next) { > - strbuf_realpath(&odb_path_real, odb->path, 1); > + prepare_alt_odb(odb->repo); This does make me wonder if we should be able to prepare the alternates for a given odb without using an owning repo. Yes, we'd need to assign the repo parent when creating new odbs, but the info/alternates file is in the object dir. Perhaps this is waiting for me in a later patch... Thanks, -Stolee