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@xxxxxxxxxx>
> Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
> ---
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
