Re: [PATCH 01/27] repository: introduce raw object store field
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 22, 2018, 06:26 UTC
- Message-ID
- <20180222062635.GC133592@aiede.svl.corp.google.com>
- In-Reply-To
- <20180221015430.96054-2-sbeller@google.com>
Hi,
Stefan Beller wrote:
Show 19 quoted lines
> 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.
[...]
Show 14 quoted lines
> --- /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.
[...]
Show 5 quoted lines
> +
> +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
[...]
Show 7 quoted lines
> @@ -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);
Show 14 quoted lines
> 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.
[...]
Show 19 quoted lines
> --- 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