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

[Duy Nguyen]

On Tue, Feb 27, 2018 at 2:28 AM, Stefan Beller  wrote:
> On Mon, Feb 26, 2018 at 1:30 AM, Duy Nguyen  wrote:
>> On Fri, Feb 23, 2018 at 04:47:28PM -0800, Stefan Beller wrote:
>>>  /* The main repository */
>>>  static struct repository the_repo = {
>>> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
>>> _algos[GIT_HASH_SHA1], 0, 0
>>> + NULL, NULL,
>>> + RAW_OBJECT_STORE_INIT,
>>> + NULL, NULL, NULL,
>>> + NULL, NULL, NULL,
>>> + _index,
>>> + _algos[GIT_HASH_SHA1],
>>> + 0, 0
>>>  };
>>>  struct repository *the_repository = _repo;
>>
>> I wonder if we should do something like this. It makes the_repo
>> initialization easier to read and it helps unify the init code with
>> submodule.
>>
>> No don't reroll. If you think it's a good idea, you can do something
>> like this in the next series instead.
>>
>> -- 8< --
>> diff --git a/check-racy.c b/check-racy.c
>> index 24b6542352..47cbb4eb6d 100644
>> --- a/check-racy.c
>> +++ b/check-racy.c
>
> totally offtopic: Do we want to move this file into t/helper?

No wonder both Jeff and I missed this program (he didn't convert it to
use cmd_main, and I didn't move it to t/helper). This git-check-racy
is added in 42f774063d (Add check program "git-check-racy" -
2006-08-15) and is not part of the default build. You need to manually
update Makefile first to build it.

Right now it's broken (multiple definition of 'main'). If you add
init_the_repository() or something similar, just leave this file
untouched. Maybe I'll fix it later, separately. Or perhaps I'll move
this functionality to git-update-index if this is still worth keeping.
-- 
Duy

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

[Stefan Beller]

On Mon, Feb 26, 2018 at 1:30 AM, Duy Nguyen  wrote:
> On Fri, Feb 23, 2018 at 04:47:28PM -0800, Stefan Beller wrote:
>>  /* The main repository */
>>  static struct repository the_repo = {
>> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
>> _algos[GIT_HASH_SHA1], 0, 0
>> + NULL, NULL,
>> + RAW_OBJECT_STORE_INIT,
>> + NULL, NULL, NULL,
>> + NULL, NULL, NULL,
>> + _index,
>> + _algos[GIT_HASH_SHA1],
>> + 0, 0
>>  };
>>  struct repository *the_repository = _repo;
>
> I wonder if we should do something like this. It makes the_repo
> initialization easier to read and it helps unify the init code with
> submodule.
>
> No don't reroll. If you think it's a good idea, you can do something
> like this in the next series instead.
>
> -- 8< --
> diff --git a/check-racy.c b/check-racy.c
> index 24b6542352..47cbb4eb6d 100644
> --- a/check-racy.c
> +++ b/check-racy.c

totally offtopic: Do we want to move this file into t/helper?

> --- a/common-main.c
> +++ b/common-main.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "exec_cmd.h"
>  #include "attr.h"
> +#include "repository.h"
>
>  /*
>   * Many parts of Git have subprograms communicate via pipe, expect the
> @@ -32,6 +33,8 @@ int main(int argc, const char **argv)
>  */
> sanitize_stdfds();
>
> +   init_the_repository();
> +

So this is the real deal.

> -#define RAW_OBJECT_STORE_INIT { NULL }
>
> +void raw_object_store_init(struct raw_object_store *o);
>  void raw_object_store_clear(struct raw_object_store *o);
>
>  #endif /* OBJECT_STORE_H */
> diff --git a/object.c b/object.c
> index 11d904c033..8a4d01dd5f 100644
> --- a/object.c
> +++ b/object.c
> @@ -446,6 +446,11 @@ void clear_commit_marks_all(unsigned int flags)
> }
>  }
>
> +void raw_object_store_init(struct raw_object_store *o)
> +{
> +   memset(o, 0, sizeof(*o));
> +}

We'll have to adapt this for the list that we use for packed_git_mru,
but that should be no problem.

> +
> +static void repo_pre_init(struct repository *repo)
> +{
> +   memset(repo, 0, sizeof(*repo));
> +   raw_object_store_init(>objects);
> +}
> +
> +void init_the_repository(void)
> +{
> +   the_repository = _repo;
> +   repo_pre_init(the_repository);
> +   the_repository->index = _index;
> +   repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +}
>
>  static char *git_path_from_env(const char *envvar, const char *git_dir,
>const char *path, int fromenv)
> @@ -138,7 +144,8 @@ static int read_and_verify_repository_format(struct 
> repository_format *format,
>  int repo_init(struct repository *repo, const char *gitdir, const char 
> *worktree)
>  {
> struct repository_format format;
> -   memset(repo, 0, sizeof(*repo));
> +
> +   repo_pre_init(repo);
>
> repo->ignore_env = 1;
>

I like the approach. Though as you say, let's have it in the next series.

Thanks for the thoughts and guidance,
Stefan

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

[Brandon Williams]

On 02/26, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> > diff --git a/common-main.c b/common-main.c
> > index 6a689007e7..a13ab981aa 100644
> > --- a/common-main.c
> > +++ b/common-main.c
> > @@ -1,6 +1,7 @@
> >  #include "cache.h"
> >  #include "exec_cmd.h"
> >  #include "attr.h"
> > +#include "repository.h"
> >  
> >  /*
> >   * Many parts of Git have subprograms communicate via pipe, expect the
> > @@ -32,6 +33,8 @@ int main(int argc, const char **argv)
> >  */
> > sanitize_stdfds();
> >  
> > +   init_the_repository();
> > +
> > git_setup_gettext();
> > ...
> > +void init_the_repository(void)
> > +{
> > +   the_repository = _repo;
> > +   repo_pre_init(the_repository);
> > +   the_repository->index = _index;
> > +   repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> > +}
> 
> I see what you did here, and I like it.

I thought this would be a good idea to do eventually but back when I
first introduced struct repository there wasn't enough to justify it
till now.  This definitely makes it much easier to read the
initialization and I prefer this over the initializer.  Thanks for
working on this :)

-- 
Brandon Williams

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

[Junio C Hamano]

Duy Nguyen  writes:

> diff --git a/common-main.c b/common-main.c
> index 6a689007e7..a13ab981aa 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "exec_cmd.h"
>  #include "attr.h"
> +#include "repository.h"
>  
>  /*
>   * Many parts of Git have subprograms communicate via pipe, expect the
> @@ -32,6 +33,8 @@ int main(int argc, const char **argv)
>*/
>   sanitize_stdfds();
>  
> + init_the_repository();
> +
>   git_setup_gettext();
> ...
> +void init_the_repository(void)
> +{
> + the_repository = _repo;
> + repo_pre_init(the_repository);
> + the_repository->index = _index;
> + repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
> +}

I see what you did here, and I like it.

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

[Duy Nguyen]

On Fri, Feb 23, 2018 at 04:47:28PM -0800, Stefan Beller wrote:
>  /* The main repository */
>  static struct repository the_repo = {
> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
> _algos[GIT_HASH_SHA1], 0, 0
> + NULL, NULL,
> + RAW_OBJECT_STORE_INIT,
> + NULL, NULL, NULL,
> + NULL, NULL, NULL,
> + _index,
> + _algos[GIT_HASH_SHA1],
> + 0, 0
>  };
>  struct repository *the_repository = _repo;

I wonder if we should do something like this. It makes the_repo
initialization easier to read and it helps unify the init code with
submodule.

No don't reroll. If you think it's a good idea, you can do something
like this in the next series instead.

-- 8< --
diff --git a/check-racy.c b/check-racy.c
index 24b6542352..47cbb4eb6d 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -1,10 +1,12 @@
 #include "cache.h"
+#include "repository.h"
 
 int main(int ac, char **av)
 {
int i;
int dirty, clean, racy;
 
+   init_the_repository();
dirty = clean = racy = 0;
read_cache();
for (i = 0; i < active_nr; i++) {
diff --git a/common-main.c b/common-main.c
index 6a689007e7..a13ab981aa 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "attr.h"
+#include "repository.h"
 
 /*
  * Many parts of Git have subprograms communicate via pipe, expect the
@@ -32,6 +33,8 @@ int main(int argc, const char **argv)
 */
sanitize_stdfds();
 
+   init_the_repository();
+
git_setup_gettext();
 
attr_start();
diff --git a/object-store.h b/object-store.h
index cf35760ceb..c3253ebc59 100644
--- a/object-store.h
+++ b/object-store.h
@@ -8,8 +8,8 @@ struct raw_object_store {
 */
char *objectdir;
 };
-#define RAW_OBJECT_STORE_INIT { NULL }
 
+void raw_object_store_init(struct raw_object_store *o);
 void raw_object_store_clear(struct raw_object_store *o);
 
 #endif /* OBJECT_STORE_H */
diff --git a/object.c b/object.c
index 11d904c033..8a4d01dd5f 100644
--- a/object.c
+++ b/object.c
@@ -446,6 +446,11 @@ void clear_commit_marks_all(unsigned int flags)
}
 }
 
+void raw_object_store_init(struct raw_object_store *o)
+{
+   memset(o, 0, sizeof(*o));
+}
+
 void raw_object_store_clear(struct raw_object_store *o)
 {
free(o->objectdir);
diff --git a/repository.c b/repository.c
index 2255ff657e..0ebcca8539 100644
--- a/repository.c
+++ b/repository.c
@@ -5,16 +5,22 @@
 #include "submodule-config.h"
 
 /* The main repository */
-static struct repository the_repo = {
-   NULL, NULL,
-   RAW_OBJECT_STORE_INIT,
-   NULL, NULL, NULL,
-   NULL, NULL, NULL,
-   _index,
-   _algos[GIT_HASH_SHA1],
-   0, 0
-};
-struct repository *the_repository = _repo;
+static struct repository the_repo;
+struct repository *the_repository;
+
+static void repo_pre_init(struct repository *repo)
+{
+   memset(repo, 0, sizeof(*repo));
+   raw_object_store_init(>objects);
+}
+
+void init_the_repository(void)
+{
+   the_repository = _repo;
+   repo_pre_init(the_repository);
+   the_repository->index = _index;
+   repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+}
 
 static char *git_path_from_env(const char *envvar, const char *git_dir,
   const char *path, int fromenv)
@@ -138,7 +144,8 @@ static int read_and_verify_repository_format(struct 
repository_format *format,
 int repo_init(struct repository *repo, const char *gitdir, const char 
*worktree)
 {
struct repository_format format;
-   memset(repo, 0, sizeof(*repo));
+
+   repo_pre_init(repo);
 
repo->ignore_env = 1;
 
diff --git a/repository.h b/repository.h
index 1f8bc7a7cf..da6a8f9af9 100644
--- a/repository.h
+++ b/repository.h
@@ -93,6 +93,7 @@ extern void repo_set_gitdir(struct repository *repo, const 
char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern void repo_set_hash_algo(struct repository *repo, int algo);
 extern int repo_init(struct repository *repo, const char *gitdir, const char 
*worktree);
+extern void init_the_repository(void);
 extern int repo_submodule_init(struct repository *submodule,
   struct repository *superproject,
   const char *path);
-- 8< --

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

[Stefan Beller]

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 
Signed-off-by: Jonathan Nieder 
---
 builtin/grep.c |  2 +-
 environment.c  |  5 +++--
 object-store.h | 15 +++
 object.c   |  5 +
 path.c |  2 +-
 repository.c   | 19 ++-
 repository.h   |  7 ---
 7 files changed, 43 insertions(+), 12 deletions(-)
 create mode 100644 object-store.h

diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d8c..0f0c195705f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -432,7 +432,7 @@ static int grep_submodule(struct grep_opt *opt, struct 
repository *superproject,
 * object.
 */
grep_read_lock();
-   add_to_alternates_memory(submodule.objectdir);
+   add_to_alternates_memory(submodule.objects.objectdir);
grep_read_unlock();
 
if (oid) {
diff --git a/environment.c b/environment.c
index de8431e01e7..ec10b062e68 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 #include "commit.h"
+#include "object-store.h"
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
@@ -244,9 +245,9 @@ const char *get_git_work_tree(void)
 
 char *get_object_directory(void)
 {
-   if (!the_repository->objectdir)
+   if (!the_repository->objects.objectdir)
BUG("git environment hasn't been setup");
-   return the_repository->objectdir;
+   return the_repository->objects.objectdir;
 }
 
 int odb_mkstemp(struct strbuf *template, const char *pattern)
diff --git a/object-store.h b/object-store.h
new file mode 100644
index 000..cf35760ceb6
--- /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 }
+
+void raw_object_store_clear(struct raw_object_store *o);
+
+#endif /* OBJECT_STORE_H */
diff --git a/object.c b/object.c
index 9e6f9ff20b0..a2acdee1405 100644
--- a/object.c
+++ b/object.c
@@ -445,3 +445,8 @@ void clear_commit_marks_all(unsigned int flags)
obj->flags &= ~flags;
}
 }
+
+void raw_object_store_clear(struct raw_object_store *o)
+{
+   FREE_AND_NULL(o->objectdir);
+}
diff --git a/path.c b/path.c
index da8b655730d..81a42d91155 100644
--- a/path.c
+++ b/path.c
@@ -382,7 +382,7 @@ static void adjust_git_path(const struct repository *repo,
strbuf_splice(buf, 0, buf->len,
  repo->index_file, strlen(repo->index_file));
else if (dir_prefix(base, "objects"))
-   replace_dir(buf, git_dir_len + 7, repo->objectdir);
+   replace_dir(buf, git_dir_len + 7, repo->objects.objectdir);
else if (git_hooks_path && dir_prefix(base, "hooks"))
replace_dir(buf, git_dir_len + 5, git_hooks_path);
else if (repo->different_commondir)
diff --git a/repository.c b/repository.c
index 4ffbe9bc94e..fb65e8072d5 100644
--- a/repository.c
+++ b/repository.c
@@ -1,11 +1,18 @@
 #include "cache.h"
 #include "repository.h"
+#include "object-store.h"
 #include "config.h"
 #include "submodule-config.h"
 
 /* The main repository */
 static struct repository the_repo = {
-   NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, _index, 
_algos[GIT_HASH_SHA1], 0, 0
+   NULL, NULL,
+   RAW_OBJECT_STORE_INIT,
+   NULL, NULL, NULL,
+   NULL, NULL, NULL,
+   _index,
+   _algos[GIT_HASH_SHA1],
+   0, 0
 };
 struct repository *the_repository = _repo;
 
@@ -42,9 +49,10 @@ static void repo_setup_env(struct repository *repo)
!repo->ignore_env);
free(repo->commondir);
repo->commondir = strbuf_detach(, NULL);
-   free(repo->objectdir);
-   repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
-   "objects", !repo->ignore_env);
+   raw_object_store_clear(>objects);
+   repo->objects.objectdir =
+   git_path_from_env(DB_ENVIRONMENT, repo->commondir,
+