Re: [PATCH 05/17] odb: get rid of `the_repository` in `find_odb()`

[Derrick Stolee <stolee@gmail.com>]

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