Re: [PATCH v3 3/6] setup: handle ODB-related environment variables in `odb_new()`

Patrick Steinhardt <[email protected]> writes:

> When initializing a repository's object database we have to respect the
> GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES environment
> variables, which can be set by the user to override the default location
> of where we write objects to and read objects from.
>
> This is handled in `apply_repository_format()`, which is fine. But in a
> subsequent commit we'll have to defer constructing the object database
> to a later point in some cases, and that will require a second site
> where we call `odb_new()`. And of course, that second site would have to
> handle those environment variables, as well.
>
> It would be somewhat awkward to duplicate the logic though. But there's
> a better alternative: instead of handling this logic in "setup.c", we
> can easily handle environment variables in `odb_new()` itself. This
> ensures that object database creation is neatly self-contained, and we
> don't have to duplicate any of the logic.
>
> Another benefit is that in a future patch series we plan to move
> handling of alternates into the backends themselves [1], and that will
> require us to also handle those environment variables in the "files"
> backend itself. So moving the logic into the ODB level already gets us
> one step closer to that goal.
>
> Refactor the logic accordingly.

I like this!

>
> [1]: https://lore.kernel.org/git/[email protected]/
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb.c                         | 20 ++++++++++++--------
>  odb.h                         | 17 +++++++++++++++--
>  setup.c                       | 11 ++++-------
>  t/unit-tests/u-odb-inmemory.c |  2 +-
>  4 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/odb.c b/odb.c
> index cf6e7938c0..b463afa072 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -1004,26 +1004,30 @@ int odb_write_object_stream(struct object_database *odb,
>  }
>  
>  struct object_database *odb_new(struct repository *repo,
> -				const char *primary_source,
> -				const char *secondary_sources)
> +				enum odb_new_flags flags)
>  {
> -	struct object_database *o = xmalloc(sizeof(*o));
> -	char *to_free = NULL;
> +	char *primary_source = NULL, *secondary_sources = NULL;
> +	struct object_database *o;
>  
> -	memset(o, 0, sizeof(*o));
> +	CALLOC_ARRAY(o, 1);
>  	o->repo = repo;
>  	pthread_mutex_init(&o->replace_mutex, NULL);
>  	string_list_init_dup(&o->submodule_source_paths);
>  
> +	if (flags & ODB_NEW_HONOR_ENV) {
> +		primary_source = xstrdup_or_null(getenv(DB_ENVIRONMENT));
> +		secondary_sources = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
> +	}
>  	if (!primary_source)
> -		primary_source = to_free = xstrfmt("%s/objects", repo->commondir);
> +		primary_source = xstrfmt("%s/objects", repo->commondir);
> +
>  	o->sources = odb_source_new(o, primary_source, true);
>  	o->sources_tail = &o->sources->next;
>  	o->alternate_db = xstrdup_or_null(secondary_sources);

I'd say this xstrdup_or_null() is not needed no more, and so is the
free() of that variable below.

>  	o->inmemory_objects = &odb_source_inmemory_new(o)->base;
>  
> -	free(to_free);
> -
> +	free(secondary_sources);
> +	free(primary_source);
>  	return o;
>  }

-- 
Cheers,
Toon