Re: [PATCH 05/17] odb: get rid of `the_repository` in `find_odb()`
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 7, 2025, 01:10 UTC
- Message-ID
- <d731678d-bda2-474d-81e8-39da85bd9038@gmail.com>
- 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:
Show 10 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.
Show 10 quoted lines
> {
> - 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