Re: [PATCH 01/27] repository: introduce raw object store field

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

> The raw object store field will contain any objects needed for
> access to objects in a given repository.
>
> This patch introduces the raw object store and populates it with the
> `objectdir`, which used to be part of the repository struct.
>
> As the struct gains members, we'll also populate the function to clear
> the memory for these members.
>
> In a later we'll introduce a struct object_parser, that will complement
> the object parsing in a repository struct: The raw object parser is the
> layer that will provide access to raw object content, while the higher
> level object parser code will parse raw objects and keeps track of
> parenthood and other object relationships using 'struct object'.
> For now only add the lower level to the repository struct.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

Heh, I suppose that sign-off makes me not a great candidate for
reviewing this.  It's been long enough since I looked at the patch
that I feel okay trying anyway.

[...]
> --- /dev/null
> +++ b/object-store.h
> @@ -0,0 +1,15 @@
> +#ifndef OBJECT_STORE_H
> +#define OBJECT_STORE_H
> +
> +struct raw_object_store {
> +	/*
> +	 * Path to the repository's object store.
> +	 * Cannot be NULL after initialization.
> +	 */
> +	char *objectdir;
> +};
> +#define RAW_OBJECT_STORE_INIT { NULL }

Who owns and is responsible for freeing objectdir?

> +
> +void raw_object_store_clear(struct raw_object_store *o);

Oh, that answers that.

It could be worth a note in the doc comment anyway, but I don't mind
not having one.

[...]
> +
> +void raw_object_store_clear(struct raw_object_store *o)
> +{
> +	free(o->objectdir);
> +}

Should this use FREE_AND_NULL?

[...]
> --- a/repository.c
> +++ b/repository.c
[...]
> @@ -42,8 +49,8 @@ static void repo_setup_env(struct repository *repo)
>  						    !repo->ignore_env);
>  	free(repo->commondir);
>  	repo->commondir = strbuf_detach(&sb, NULL);
> -	free(repo->objectdir);
> -	repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
> +	free(repo->objects.objectdir);

Should this call raw_object_store_clear instead of calling free
directly?

> +	repo->objects.objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
>  					    "objects", !repo->ignore_env);

Long line.  One way to break it would be

	repo->objects.objectdir =
		git_path_from_env(DB_ENVIRONMENT, repo->commondir,
				  "objects", !repo->ignore_env);

>  	free(repo->graft_file);
>  	repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
> @@ -209,12 +216,14 @@ void repo_clear(struct repository *repo)
>  {
>  	FREE_AND_NULL(repo->gitdir);
>  	FREE_AND_NULL(repo->commondir);
> -	FREE_AND_NULL(repo->objectdir);
>  	FREE_AND_NULL(repo->graft_file);
>  	FREE_AND_NULL(repo->index_file);
>  	FREE_AND_NULL(repo->worktree);
>  	FREE_AND_NULL(repo->submodule_prefix);
>  
> +	raw_object_store_clear(&repo->objects);
> +	memset(&repo->objects, 0, sizeof(repo->objects));

If raw_object_store_clear uses FREE_AND_NULL, this memset won't be necessary.

[...]
> --- a/repository.h
> +++ b/repository.h
> @@ -1,6 +1,8 @@
>  #ifndef REPOSITORY_H
>  #define REPOSITORY_H
>  
> +#include "object-store.h"
> +
>  struct config_set;
>  struct index_state;
>  struct submodule_cache;
> @@ -21,10 +23,9 @@ struct repository {
>  	char *commondir;
>  
>  	/*
> -	 * Path to the repository's object store.
> -	 * Cannot be NULL after initialization.
> +	 * Holds any information related to the object store.
>  	 */

This comment doesn't make it clear to me what the field represents.
Can it be replaced with some of the description from the commit
message?

> -	char *objectdir;
> +	struct raw_object_store objects;
>  

Thanks,
Jonathan