Object store refactoring: commit graph

Signed-off-by Jonathan Tan <[email protected]>

Both this and the previous patch look good to me; you seem to have better (stricter) checking for missing includes/forward declarations, am I missing a compile option? (I have DEVELOPER=1 in config.mak)

Quoted 4 lines

Both this and the previous patch look good to me; you seem to have better (stricter) checking for missing includes/forward declarations, am I missing a compile option? (I have DEVELOPER=1 in config.mak)

Thanks. No I don't - I discovered that these were missing as I was including these header files in other files.

Instead of storing commit graphs in static variables, store them in struct object_store. There are no changes to the signatures of existing functions - they all still only support the_repository, and support for other instances of struct repository will be added in a subsequent commit.

Signed-off-by Jonathan Tan <[email protected]>

---
 commit-graph.c | 34 ++++++++++++++++++----------------
 object-store.h |  3 +++
 object.c       |  5 +++++
 3 files changed, 26 insertions(+), 16 deletions(-)

...

commit-graph.c +3 -1

@@ -293,7 +290,9 @@ int parse_commit_in_graph(struct commit *item)
 		return 1;
 
 	prepare_commit_graph();
-	if (commit_graph) {
+	if (the_repository->objects->commit_graph) {
+		struct commit_graph *commit_graph =
+			the_repository->objects->commit_graph;
 		uint32_t pos;
 		int found;
 		if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {

I was looking at semantic merge conflicts between this and your e2838d85 ("commit-graph: always load commit-graph information", 2018-05-01), the latter of which I planned to merge to 'master' as a part of the first batch in this cycle, and noticed that there are two very similar functions, without enough document the callers would not know which one is the correct one to call. Needless to say, such a code duplication would mean the work required for resolving semantic conflict doubles needlessly X-<.

int parse_commit_in_graph(struct commit *item)

{
	uint32_t pos;

	if (!core_commit_graph)
		return 0;
	if (item->object.parsed)
		return 1;
	prepare_commit_graph();
	if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
		return fill_commit_in_graph(item, commit_graph, pos);
	return 0;
}

void load_commit_graph_info(struct commit *item)

{
	uint32_t pos;
	if (!core_commit_graph)
		return;
	prepare_commit_graph();
	if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
		fill_commit_graph_info(item, commit_graph, pos);
}

As for whether both these functions are necessary in the first place, I think they are.

commit.c struct tree *get_commit_tree(const struct commit *commit) · line 320

Quoted 10 lines

commit.c +1 -1

index 0030e79940..38c12b002f 100644
@@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)
        if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
                BUG("commit has NULL tree, but was not loaded from commit-graph");
 
-       return get_commit_tree_in_graph(commit);
+       return get_commit_tree_in_graph(the_repository, commit);

Here..

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing) · line 416

Quoted 9 lines

commit.c +1 -1

 }
 
 struct object_id *get_commit_tree_oid(const struct commit *commit)

@@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)
                return -1;
        if (item->object.parsed)
                return 0;
-       if (parse_commit_in_graph(item))
+       if (parse_commit_in_graph(the_repository, item))

and here

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing)

Quoted 11 lines

commit.c +11 -0

+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+                                      const struct object_id *commit_oid)
+{
+       struct repository r;
+       struct commit *c;
+       struct commit_list *parent;
+
+       /*
+        * Create a commit independent of any repository.
+        */
+       c = lookup_commit(commit_oid);

.. and this one are unfortunate as the rest of the object store series has not progressed as far as needed.

The lookup_commit series is out there already, and that will teach lookup_commit a repository argument. When rerolling that series I need to switch the order of repo_init and lookup_commit such that we can pass the repo to the lookup.

This is really nice!

Overall this series looks good to me,
Stefan

Quoted 23 lines

commit.c struct tree *get_commit_tree(const struct commit *commit) · line 320

Quoted 10 lines

commit.c +1 -1

index 0030e79940..38c12b002f 100644
@@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)
         if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
                 BUG("commit has NULL tree, but was not loaded from commit-graph");
 
-       return get_commit_tree_in_graph(commit);
+       return get_commit_tree_in_graph(the_repository, commit);

Here..

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing) · line 416

Quoted 9 lines

commit.c +1 -1

  }
 
  struct object_id *get_commit_tree_oid(const struct commit *commit)

@@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)
                 return -1;
         if (item->object.parsed)
                 return 0;
-       if (parse_commit_in_graph(item))
+       if (parse_commit_in_graph(the_repository, item))

and here

Quoted 13 lines

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing)

Quoted 11 lines

commit.c +11 -0

+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+                                      const struct object_id *commit_oid)
+{
+       struct repository r;
+       struct commit *c;
+       struct commit_list *parent;
+
+       /*
+        * Create a commit independent of any repository.
+        */
+       c = lookup_commit(commit_oid);

.. and this one are unfortunate as the rest of the object store series has not progressed as far as needed.

I think the first 2 are in reverse - get_commit_tree depends on get_commit_tree_in_graph and parse_commit_gently depends on parse_commit_in_graph, so we need the commit-graph functions to be changed first. But I agree about lookup_commit.

Quoted 4 lines

The lookup_commit series is out there already, and that will teach lookup_commit a repository argument. When rerolling that series I need to switch the order of repo_init and lookup_commit such that we can pass the repo to the lookup.

For future reference, Stefan is talking about this series:
https://public-inbox.org/git/[email protected]/

Let me know if you want to reroll yours on top of mine, or vice versa. I think it's clearer if mine goes in first, though, since (as you said in that e-mail) parse_commit depends on this change in the commit graph.

Quoted 4 lines

This is really nice!

Overall this series looks good to me,
Stefan

Thanks - let's see what others think.

Quoted 49 lines · Jonathan Tan

Quoted 25 lines

commit.c struct tree *get_commit_tree(const struct · line 321

Quoted 11 lines

commit.c +1 -1

index 0030e79940..38c12b002f 100644
@@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct 
 commit *commit)
          if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
                  BUG("commit has NULL tree, but was not loaded from commit-graph");
 
-       return get_commit_tree_in_graph(commit);
+       return get_commit_tree_in_graph(the_repository, commit);

Here..

commit.c int parse_commit_gently(struct commit *item, · line 417

Quoted 10 lines

commit.c +0 -0

   }
 

  struct object_id *get_commit_tree_oid(const struct commit *commit)

commit.c +1 -1

@@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, 
 int quiet_on_missing)
                  return -1;
          if (item->object.parsed)
                  return 0;
-       if (parse_commit_in_graph(item))
+       if (parse_commit_in_graph(the_repository, item))

and here

Quoted 15 lines

commit.c

Quoted 13 lines

commit.c +2 -0

+static void test_parse_commit_in_graph(const char *gitdir, const 
 char *worktree,
+                                      const struct object_id 

*commit_oid)

commit.c +9 -0

+{
+       struct repository r;
+       struct commit *c;
+       struct commit_list *parent;
+
+       /*
+        * Create a commit independent of any repository.
+        */
+       c = lookup_commit(commit_oid);

.. and this one are unfortunate as the rest of the object store series has not progressed as far as needed.

I think the first 2 are in reverse - get_commit_tree depends on get_commit_tree_in_graph and parse_commit_gently depends on parse_commit_in_graph, so we need the commit-graph functions to be changed first. But I agree about lookup_commit.

Quoted 4 lines

The lookup_commit series is out there already, and that will teach lookup_commit a repository argument. When rerolling that series I need to switch the order of repo_init and lookup_commit such that we can pass the repo to the lookup.

Quoted 6 lines

For future reference, Stefan is talking about this series:
https://public-inbox.org/git/[email protected]/

Let me know if you want to reroll yours on top of mine, or vice versa. I think it's clearer if mine goes in first, though, since (as you said in that e-mail) parse_commit depends on this change in the commit graph.

I was about to comment that I thought 'parse_commit_in_graph' should take a 'struct commit_graph' instead of 'struct repository', except for these lookup_commit() calls will need the repository parameter.

Please also keep in mind that ds/commit-graph-fsck has already updated this method to parse from a specific graph [1]. I'm just waiting for some things like ds/generation-numbers to get into 'master' and some more object-store patches to be final before I re-roll that series. I mentioned this in a message that I had sent, but apparently didn't make it on the list (so I re-sent it recently).

[1] https://public-inbox.org/git/[email protected]/

Quoted 48 lines

Quoted 23 lines

commit.c struct tree *get_commit_tree(const struct commit *commit) · line 320

Quoted 10 lines

commit.c +1 -1

index 0030e79940..38c12b002f 100644
@@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)
         if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
                 BUG("commit has NULL tree, but was not loaded from commit-graph");
 
-       return get_commit_tree_in_graph(commit);
+       return get_commit_tree_in_graph(the_repository, commit);

Here..

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing) · line 416

Quoted 9 lines

commit.c +1 -1

  }
 
  struct object_id *get_commit_tree_oid(const struct commit *commit)

@@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)
                 return -1;
         if (item->object.parsed)
                 return 0;
-       if (parse_commit_in_graph(item))
+       if (parse_commit_in_graph(the_repository, item))

and here

Quoted 13 lines

commit.c int parse_commit_gently(struct commit *item, int quiet_on_missing)

Quoted 11 lines

commit.c +11 -0

+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+                                      const struct object_id *commit_oid)
+{
+       struct repository r;
+       struct commit *c;
+       struct commit_list *parent;
+
+       /*
+        * Create a commit independent of any repository.
+        */
+       c = lookup_commit(commit_oid);

.. and this one are unfortunate as the rest of the object store series has not progressed as far as needed.

I think the first 2 are in reverse - get_commit_tree depends on get_commit_tree_in_graph and parse_commit_gently depends on parse_commit_in_graph, so we need the commit-graph functions to be changed first. But I agree about lookup_commit.

Quoted 4 lines

The lookup_commit series is out there already, and that will teach lookup_commit a repository argument. When rerolling that series I need to switch the order of repo_init and lookup_commit such that we can pass the repo to the lookup.

For future reference, Stefan is talking about this series:
https://public-inbox.org/git/[email protected]/

Quoted 3 lines

Let me know if you want to reroll yours on top of mine, or vice versa. I think it's clearer if mine goes in first, though, since (as you said in that e-mail) parse_commit depends on this change in the commit graph.

I was about to comment that I thought 'parse_commit_in_graph' should take a 'struct commit_graph' instead of 'struct repository', except for these lookup_commit() calls will need the repository parameter.

Please also keep in mind that ds/commit-graph-fsck has already updated this method to parse from a specific graph [1]. I'm just waiting for some things like ds/generation-numbers to get into 'master' and some more object-store patches to be final before I re-roll that series. I mentioned this in a message that I had sent, but apparently didn't make it on the list (so I re-sent it recently).

[1]
https://public-inbox.org/git/[email protected]/

Two functions in the code (1) check if the repository is configured for commit graphs, (2) call prepare_commit_graph(), and (3) check if the graph exists. Move (1) and (3) into prepare_commit_graph(), reducing duplication of code.

Signed-off-by Jonathan Tan <[email protected]>

static int prepare_commit_graph_run_once = 0;

Hi Jonathan,

Quoted 9 lines · Jonathan Tan

Two functions in the code (1) check if the repository is configured for commit graphs, (2) call prepare_commit_graph(), and (3) check if the graph exists. Move (1) and (3) into prepare_commit_graph(), reducing duplication of code.

Signed-off-by Jonathan Tan <[email protected]>

  static int prepare_commit_graph_run_once = 0;

commit-graph.c static void prepare_commit_graph_one(const char *obj_dir) · line 208

Quoted 7 lines

commit-graph.c +6 -1

-static void prepare_commit_graph(void)
+
+/*
+ * Return 1 if commit_graph is non-NULL, and 0 otherwise.
+ *
+ * On the first invocation, this function attemps to load the commit
+ * graph if the_repository is configured to have one.

and as we talk about in-memory commit graph (and not some stale file that may still be around on the fs), we can assertly return 0 when core_commit_graph is false.

Makes sense!

commit-graph.c static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item · line 353

Quoted 13 lines

commit-graph.c +2 -6

@@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item
 
  int parse_commit_in_graph(struct commit *item)
  {
-       if (!core_commit_graph)
+       if (!prepare_commit_graph())
                 return 0;
-
-       prepare_commit_graph();
-       if (commit_graph)
-               return parse_commit_in_graph_one(commit_graph, item);
-       return 0;
+       return parse_commit_in_graph_one(commit_graph, item);

Makes sense.

commit-graph.c static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item · line 362

Quoted 12 lines

commit-graph.c +2 -3

  }
 
  void load_commit_graph_info(struct commit *item)
  {
         uint32_t pos;
-       if (!core_commit_graph)
+       if (!prepare_commit_graph())
                 return;
-       prepare_commit_graph();
-       if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
+       if (find_commit_in_graph(item, commit_graph, &pos))
                 fill_commit_graph_info(item, commit_graph, pos);

here too,

This is

Reviewed-by Stefan Beller <[email protected]>

Add a struct repository argument to the functions in commit-graph.h that read the commit graph. (This commit does not affect functions that write commit graphs.)

Because the commit graph functions can now read the commit graph of any repository, the global variable core_commit_graph has been removed. Instead, the config option core.commitGraph is now read on the first time in a repository that a commit is attempted to be parsed using its commit graph.

This commit includes a test that exercises the functionality on an arbitrary repository that is not the_repository.

Signed-off-by Jonathan Tan <[email protected]>

---
  Makefile                   |  1 +
  builtin/fsck.c             |  2 +-
  cache.h                    |  1 -
  commit-graph.c             | 60 ++++++++++++++------------
  commit-graph.h             |  7 +--
  commit.c                   |  6 +--
  config.c                   |  5 ---
  environment.c              |  1 -
  ref-filter.c               |  2 +-
  t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++++++++++
  t/helper/test-tool.c       |  1 +
  t/helper/test-tool.h       |  1 +
  t/t5318-commit-graph.sh    | 35 +++++++++++++++
  13 files changed, 168 insertions(+), 42 deletions(-)

create mode 100644 t/helper/test-repository.c

Makefile +1 -0

index 0cb6590f24..bb8bd67201 100644
@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o
  TEST_BUILTINS_OBJS += test-read-cache.o
  TEST_BUILTINS_OBJS += test-ref-store.o
  TEST_BUILTINS_OBJS += test-regex.o
+TEST_BUILTINS_OBJS += test-repository.o
  TEST_BUILTINS_OBJS += test-revision-walking.o
  TEST_BUILTINS_OBJS += test-run-command.o
  TEST_BUILTINS_OBJS += test-scrap-cache-tree.o

builtin/fsck.c +1 -1

index eca7900ee0..2abfb2d782 100644
@@ -825,7 +825,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 
  	check_connectivity();
 
-	if (core_commit_graph) {
+	if (!git_config_get_bool("core.commitgraph", &i) && i) {
  		struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
  		const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL };
 

cache.h +0 -1

index d49092d94d..8dc59bedba 100644
@@ -813,7 +813,6 @@ extern char *git_replace_ref_base;
 
  extern int fsync_object_files;
  extern int core_preload_index;
-extern int core_commit_graph;
  extern int core_apply_sparse_checkout;
  extern int precomposed_unicode;
  extern int protect_hfs;

commit-graph.c +8 -7

index af97a10603..8eab199b1b 100644
@@ -183,15 +183,15 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
  	exit(1);
  }
 
-static void prepare_commit_graph_one(const char *obj_dir)
+static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
  {
  	char *graph_name;
 
-	if (the_repository->objects->commit_graph)
+	if (r->objects->commit_graph)
  		return;
 
  	graph_name = get_commit_graph_filename(obj_dir);
-	the_repository->objects->commit_graph =
+	r->objects->commit_graph =
  		load_commit_graph_one(graph_name);
 
  	FREE_AND_NULL(graph_name);

@@ -203,26 +203,34 @@ static void prepare_commit_graph_one(const char *obj_dir)
   * On the first invocation, this function attemps to load the commit
   * graph if the_repository is configured to have one.
   */
-static int prepare_commit_graph(void)
+static int prepare_commit_graph(struct repository *r)
  {
  	struct alternate_object_database *alt;
  	char *obj_dir;
+	int config_value;
 
-	if (the_repository->objects->commit_graph_attempted)
-		return !!the_repository->objects->commit_graph;
-	the_repository->objects->commit_graph_attempted = 1;
+	if (r->objects->commit_graph_attempted)
+		return !!r->objects->commit_graph;
+	r->objects->commit_graph_attempted = 1;

commit-graph.c +17 -19

  		return 0;
 
-	obj_dir = get_object_directory();
-	prepare_commit_graph_one(obj_dir);
-	prepare_alt_odb(the_repository);
-	for (alt = the_repository->objects->alt_odb_list;
-	     !the_repository->objects->commit_graph && alt;
+	obj_dir = r->objects->objectdir;
+	prepare_commit_graph_one(r, obj_dir);
+	prepare_alt_odb(r);
+	for (alt = r->objects->alt_odb_list;
+	     !r->objects->commit_graph && alt;
  	     alt = alt->next)
-		prepare_commit_graph_one(alt->path);
-	return !!the_repository->objects->commit_graph;
+		prepare_commit_graph_one(r, alt->path);
+	return !!r->objects->commit_graph;
  }
 
  static void close_commit_graph(void)

@@ -323,8 +331,6 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item
  {
  	uint32_t pos;
 
-	if (!core_commit_graph)
-		return 0;
  	if (item->object.parsed)
  		return 1;
 

@@ -334,20 +340,20 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item
  	return 0;
  }
 
-int parse_commit_in_graph(struct commit *item)
+int parse_commit_in_graph(struct repository *r, struct commit *item)
  {
-	if (!prepare_commit_graph())
+	if (!prepare_commit_graph(r))
  		return 0;
-	return parse_commit_in_graph_one(the_repository->objects->commit_graph, item);
+	return parse_commit_in_graph_one(r->objects->commit_graph, item);
  }
 
-void load_commit_graph_info(struct commit *item)
+void load_commit_graph_info(struct repository *r, struct commit *item)
  {
  	uint32_t pos;
-	if (!prepare_commit_graph())
+	if (!prepare_commit_graph(r))
  		return;
-	if (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))
-		fill_commit_graph_info(item, the_repository->objects->commit_graph, pos);
+	if (find_commit_in_graph(item, r->objects->commit_graph, &pos))
+		fill_commit_graph_info(item, r->objects->commit_graph, pos);
  }
 
  static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)

@@ -373,9 +379,9 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
  	return load_tree_for_commit(g, (struct commit *)c);
  }
 
-struct tree *get_commit_tree_in_graph(const struct commit *c)
+struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit *c)
  {
-	return get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);
+	return get_commit_tree_in_graph_one(r->objects->commit_graph, c);
  }
 
  static void write_graph_chunk_fanout(struct hashfile *f,

@@ -691,7 +697,7 @@ void write_commit_graph(const char *obj_dir,
  	oids.alloc = approximate_object_count() / 4;
 
  	if (append) {
-		prepare_commit_graph_one(obj_dir);
+		prepare_commit_graph_one(the_repository, obj_dir);
  		if (the_repository->objects->commit_graph)
  			oids.alloc += the_repository->objects->commit_graph->num_commits;
  	}

commit-graph.h +4 -3

index 94defb04a9..76e098934a 100644
@@ -19,7 +19,7 @@ char *get_commit_graph_filename(const char *obj_dir);
   *
   * See parse_commit_buffer() for the fallback after this call.
   */
-int parse_commit_in_graph(struct commit *item);
+int parse_commit_in_graph(struct repository *r, struct commit *item);
 
  /*
   * It is possible that we loaded commit contents from the commit buffer,

@@ -27,9 +27,10 @@ int parse_commit_in_graph(struct commit *item);
   * checked and filled. Fill the graph_pos and generation members of
   * the given commit.
   */
-void load_commit_graph_info(struct commit *item);
+void load_commit_graph_info(struct repository *r, struct commit *item);
 
-struct tree *get_commit_tree_in_graph(const struct commit *c);
+struct tree *get_commit_tree_in_graph(struct repository *r,
+				      const struct commit *c);
 
  struct commit_graph {
  	int graph_fd;

commit.c +3 -3

index 598cf21cee..b3bbfefc27 100644
@@ -319,7 +319,7 @@ struct tree *get_commit_tree(const struct commit *commit)
  	if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
  		BUG("commit has NULL tree, but was not loaded from commit-graph");
 
-	return get_commit_tree_in_graph(commit);
+	return get_commit_tree_in_graph(the_repository, commit);
  }
 
  struct object_id *get_commit_tree_oid(const struct commit *commit)

@@ -413,7 +413,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
  	item->date = parse_commit_date(bufptr, tail);
 
  	if (check_graph)
-		load_commit_graph_info(item);
+		load_commit_graph_info(the_repository, item);
 
  	return 0;
  }

@@ -429,7 +429,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com
  		return -1;
  	if (item->object.parsed)
  		return 0;
-	if (use_commit_graph && parse_commit_in_graph(item))
+	if (use_commit_graph && parse_commit_in_graph(the_repository, item))
  		return 0;
  	buffer = read_object_file(&item->object.oid, &type, &size);
  	if (!buffer)

config.c +0 -5

index a0a6ae1980..fcf863b667 100644
@@ -1308,11 +1308,6 @@ static int git_default_core_config(const char *var, const char *value)
  		return 0;
  	}
 
-	if (!strcmp(var, "core.commitgraph")) {
-		core_commit_graph = git_config_bool(var, value);
-		return 0;
-	}
-
  	if (!strcmp(var, "core.sparsecheckout")) {
  		core_apply_sparse_checkout = git_config_bool(var, value);
  		return 0;

environment.c +0 -1

index 2a6de2330b..d6e5b39425 100644
@@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
  enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
  char *notes_ref_name;
  int grafts_replace_parents = 1;
-int core_commit_graph;
  int core_apply_sparse_checkout;
  int merge_log_config = -1;
  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */

ref-filter.c +0 -0

index fa3685d91f..889d97b005 100644

t/helper/test-repository.c new file +37 -0

new file mode 100644
index 0000000000..5fff540a26
@@ -0,0 +1,88 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "commit-graph.h"
+#include "commit.h"
+#include "config.h"
+#include "object-store.h"
+#include "object.h"
+#include "repository.h"
+#include "tree.h"
+
+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+				       const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct commit_list *parent;
+
+	/*
+	 * Create a commit independent of any repository.
+	 */
+	c = lookup_commit(commit_oid);
+
+	repo_init(&r, gitdir, worktree);
+
+	if (!parse_commit_in_graph(&r, c))
+		die("Couldn't parse commit");
+
+	printf("%lu", c->date);
+	for (parent = c->parents; parent; parent = parent->next)
+		printf(" %s", oid_to_hex(&parent->item->object.oid));
+	printf("\n");
+
+	repo_clear(&r);
+}
+
+static void test_get_commit_tree_in_graph(const char *gitdir,
+					  const char *worktree,

t/helper/test-repository.c new file +10 -0

+
+	/*
+	 * get_commit_tree_in_graph does not automatically parse the commit, so
+	 * parse it first.
+	 */
+	if (!parse_commit_in_graph(&r, c))
+		die("Couldn't parse commit");
+	tree = get_commit_tree_in_graph(&r, c);
+	if (!tree)
+		die("Couldn't get commit tree");

t/helper/test-repository.c new file +19 -0

+	if (!strcmp(argv[1], "parse_commit_in_graph")) {
+		struct object_id oid;
+		if (argc < 5)
+			die("not enough arguments");
+		if (parse_oid_hex(argv[4], &oid, &argv[4]))
+			die("cannot parse oid '%s'", argv[4]);
+		test_parse_commit_in_graph(argv[2], argv[3], &oid);
+	} else if (!strcmp(argv[1], "get_commit_tree_in_graph")) {
+		struct object_id oid;
+		if (argc < 5)
+			die("not enough arguments");
+		if (parse_oid_hex(argv[4], &oid, &argv[4]))
+			die("cannot parse oid '%s'", argv[4]);
+		test_get_commit_tree_in_graph(argv[2], argv[3], &oid);
+	} else {
+		die("unrecognized '%s'", argv[1]);
+	}
+	return 0;
+}

t/helper/test-tool.c +1 -0

index 805a45de9c..dafc91c240 100644
@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {
  	{ "read-cache", cmd__read_cache },
  	{ "ref-store", cmd__ref_store },
  	{ "regex", cmd__regex },
+	{ "repository", cmd__repository },
  	{ "revision-walking", cmd__revision_walking },
  	{ "run-command", cmd__run_command },
  	{ "scrap-cache-tree", cmd__scrap_cache_tree },

t/helper/test-tool.h +1 -0

index 7116ddfb94..80cbcf0857 100644
@@ -23,6 +23,7 @@ int cmd__prio_queue(int argc, const char **argv);
  int cmd__read_cache(int argc, const char **argv);
  int cmd__ref_store(int argc, const char **argv);
  int cmd__regex(int argc, const char **argv);
+int cmd__repository(int argc, const char **argv);
  int cmd__revision_walking(int argc, const char **argv);
  int cmd__run_command(int argc, const char **argv);
  int cmd__scrap_cache_tree(int argc, const char **argv);

t/t5318-commit-graph.sh +35 -0

index 5947de3d24..4f17d7701e 100755
@@ -431,4 +431,39 @@ test_expect_success 'git fsck (checks commit-graph)' '
  	test_must_fail git fsck
  '
 
+test_expect_success 'setup non-the_repository tests' '
+	rm -rf repo &&
+	git init repo &&
+	test_commit -C repo one &&
+	test_commit -C repo two &&
+	git -C repo config core.commitGraph true &&
+	git -C repo rev-parse two | \
+		git -C repo commit-graph write --stdin-commits
+'
+
+test_expect_success 'parse_commit_in_graph works for non-the_repository' '
+	test-tool repository parse_commit_in_graph \
+		repo/.git repo "$(git -C repo rev-parse two)" >actual &&
+	echo $(git -C repo log --pretty="%ct" -1) \
+		$(git -C repo rev-parse one) >expect &&
+	test_cmp expect actual &&
+
+	test-tool repository parse_commit_in_graph \
+		repo/.git repo "$(git -C repo rev-parse one)" >actual &&
+	echo $(git -C repo log --pretty="%ct" -1 one) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '
+	test-tool repository get_commit_tree_in_graph \
+		repo/.git repo "$(git -C repo rev-parse two)" >actual &&
+	echo $(git -C repo rev-parse two^{tree}) >expect &&
+	test_cmp expect actual &&
+
+	test-tool repository get_commit_tree_in_graph \
+		repo/.git repo "$(git -C repo rev-parse one)" >actual &&
+	echo $(git -C repo rev-parse one^{tree}) >expect &&
+	test_cmp expect actual
+'
+
  test_done

Quoted 4 lines

commit-graph.c +3 -1

-	if (!core_commit_graph)
+	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value)
+		/*

commit-graph.c static void prepare_commit_graph_one(const char *obj_dir) · line 223

Quoted 5 lines

commit-graph.c +5 -0

+		 * This repository is not configured to use commit graphs, so
+		 * do not load one. (But report commit_graph_attempted anyway
+		 * so that commit graph loading is not attempted again for this
+		 * repository.)
+		 */

I reacted first to complain about this extra config lookup, but it is only run once per repository, so that should be fine.

The tests below form a decently-large patch on their own. Perhaps split them out so it is easier to know that we have some interesting things to check here.

It's worth spending some extra time looking at this test pattern as I believe we will want to follow it with other arbitrary repository changes.

Quoted 9 lines

t/helper/test-repository.c new file +9 -0

+static void test_get_commit_tree_in_graph(const char *gitdir,
+					  const char *worktree,
+					  const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct tree *tree;
+
+	/*

t/helper/test-repository.c line 21

Quoted 3 lines

t/helper/test-repository.c new file +3 -0

+	 * Create a commit independent of any repository.
+	 */
+	c = lookup_commit(commit_oid);

Would this be more accurate to say we are creating a commit object stored in the object cache of the_repository? How would you expect this to work if/when lookup_commit() takes an arbitrary repository? You want to provide &r, right (after initializing)?

Also, this will conflict with sb/object-store-lookup, won't it? I'm guessing this is why you didn't touch the "git commit-graph [write|verify]"code paths.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	repo_init(&r, gitdir, worktree);

I think you want to move the lookup_commit() to after this.

t/helper/test-repository.c line 69

Quoted 4 lines

t/helper/test-repository.c new file +4 -0

+int cmd__repository(int argc, const char **argv)
+{
+	if (argc < 2)
+		die("must have at least 2 arguments");

I think this "test-tool repository <verb>" pattern is a good way to get some testing here.

Quoted 17 lines

t/helper/test-repository.c line 47

Quoted 13 lines

t/helper/test-repository.c new file +12 -0

+static void test_get_commit_tree_in_graph(const char *gitdir,
+					  const char *worktree,
+					  const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct tree *tree;
+
+	/*
+	 * Create a commit independent of any repository.
+	 */
+	c = lookup_commit(commit_oid);

Would this be more accurate to say we are creating a commit object stored in the object cache of the_repository? How would you expect this to work if/when lookup_commit() takes an arbitrary repository? You want to provide &r, right (after initializing)?

Yes, you're right - Stefan too mentioned that this will need to be moved below lookup_commit(). I'm not sure what the best way is to handle this
- maybe move this, and add a "needswork" stating that we need to pass r to lookup_commit once it supports taking in a repository argument, as an aid to the person who performs the merge. I'll do that if a reroll is needed.

Quoted 3 lines

Also, this will conflict with sb/object-store-lookup, won't it? I'm guessing this is why you didn't touch the "git commit-graph [write|verify]"code paths.

It will conflict because of the change to lookup_commit(), but the only new code I'm writing is in t/helper/test-repository.c, so hopefully the merge won't be too tedious. The main reason why I didn't touch the writing/verifying part is to reduce the size of this patch set, and because that change is not needed to update parse_commit() and others.

Quoted 8 lines

Quoted 3 lines

Also, this will conflict with sb/object-store-lookup, won't it? I'm guessing this is why you didn't touch the "git commit-graph [write|verify]"code paths.

It will conflict because of the change to lookup_commit(), but the only new code I'm writing is in t/helper/test-repository.c, so hopefully the merge won't be too tedious. The main reason why I didn't touch the writing/verifying part is to reduce the size of this patch set, and because that change is not needed to update parse_commit() and others.

I guess my main complaint is that this won't be an actual "merge" conflict, but the result will not compile. Since Stefan already has a series out that changes this method, I recommend basing your series on it (in addition to basing it on ds/commit-graph-fsck).

t/helper/test-repository.c new file +18 -0

new file mode 100644
index 0000000000..5fff540a26
@@ -0,0 +1,88 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "commit-graph.h"
+#include "commit.h"
+#include "config.h"
+#include "object-store.h"
+#include "object.h"
+#include "repository.h"
+#include "tree.h"
+
+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+				       const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct commit_list *parent;
+
+	/*

t/helper/test-repository.c new file +6 -0

+	for (parent = c->parents; parent; parent = parent->next)
+		printf(" %s", oid_to_hex(&parent->item->object.oid));
+	printf("\n");
+
+	repo_clear(&r);
+}

Quoted 24 lines

t/helper/test-repository.c new file +18 -0

new file mode 100644
index 0000000000..5fff540a26
@@ -0,0 +1,88 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "commit-graph.h"
+#include "commit.h"
+#include "config.h"
+#include "object-store.h"
+#include "object.h"
+#include "repository.h"
+#include "tree.h"
+
+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,
+				       const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct commit_list *parent;
+
+	/*

t/helper/test-repository.c line 28

Quoted 10 lines

t/helper/test-repository.c new file +10 -0

+	 * Create a commit independent of any repository.
+	 */
+	c = lookup_commit(commit_oid);
+
+	repo_init(&r, gitdir, worktree);
+
+	if (!parse_commit_in_graph(&r, c))
+		die("Couldn't parse commit");
+
+	printf("%lu", c->date);

32-bit builds complain about this:

t/helper/test-repository.c: In function 'test_parse_commit_in_graph':
t/helper/test-repository.c:28:9: error: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'timestamp_t {aka long long unsigned int}' [-Werror=format=]

    printf("%lu", c->date);
         ^

cc1: all warnings being treated as errors
Makefile:2262: recipe for target 't/helper/test-repository.o' failed
make: *** [t/helper/test-repository.o] Error 1

Quoted 9 lines

32-bit builds complain about this:

t/helper/test-repository.c: In function 'test_parse_commit_in_graph':
t/helper/test-repository.c:28:9: error: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'timestamp_t {aka long long unsigned int}' [-Werror=format=]

    printf("%lu", c->date);
         ^

cc1: all warnings being treated as errors
Makefile:2262: recipe for target 't/helper/test-repository.o' failed
make: *** [t/helper/test-repository.o] Error 1

Let's also state how we usually fix this:

        printf("%"PRItime, c->date);

This is on ds/commit-graph-fsck.

I've also added a patch (patch 1) that removes some duplication of implementation that Junio talked about in [1].

This is on ds/commit-graph-fsck.

I saw that ds/commit-graph-fsck has been updated to the latest version (v7, including "gc.writeCommitGraph"), so I've rebased my changes on top of that branch. There were some mechanical changes needed during the rebase, so I'm sending the rebased patches out.

I've also added a patch (patch 1) that removes some duplication of implementation that Junio talked about in [1].

[1] https://public-inbox.org/git/[email protected]/

Quoted 10 lines · Jonathan Tan

This is on ds/commit-graph-fsck.

I saw that ds/commit-graph-fsck has been updated to the latest version (v7, including "gc.writeCommitGraph"), so I've rebased my changes on top of that branch. There were some mechanical changes needed during the rebase, so I'm sending the rebased patches out.

I've also added a patch (patch 1) that removes some duplication of implementation that Junio talked about in [1].

Quoted 1 line

[1] https://public-inbox.org/git/[email protected]/

While attempting to merge this topic to 'pu', I noticed that you and Derrick are perhaps playing a game of whack-a-mole by you getting rid of core_commit_graph global and making it a per in-core repository instance, while Derrick adding core_multi_pack_index, making it necessary for yet another round of similar clean-up?

Quoted 12 lines · Junio C Hamano

Quoted 10 lines · Jonathan Tan

This is on ds/commit-graph-fsck.

I saw that ds/commit-graph-fsck has been updated to the latest version (v7, including "gc.writeCommitGraph"), so I've rebased my changes on top of that branch. There were some mechanical changes needed during the rebase, so I'm sending the rebased patches out.

I've also added a patch (patch 1) that removes some duplication of implementation that Junio talked about in [1].

Quoted 7 lines

Quoted 2 lines

[1] https://public-inbox.org/git/[email protected]/

While attempting to merge this topic to 'pu', I noticed that you and Derrick are perhaps playing a game of whack-a-mole by you getting rid of core_commit_graph global and making it a per in-core repository instance, while Derrick adding core_multi_pack_index, making it necessary for yet another round of similar clean-up?

We did have collisions with Jonathan's v1, but this v2 is on my latest commit-graph things so should not have conflicts.

The core_commit_graph variable appears to still be global (do we have config storage in the_repository yet?) so core_multi_pack_index is similar.

I do put the multi_pack_index pointer inside the_repository->objects, so the equivalent of this series will not be necessary for the MIDX series.

This series looks good to me, so please add "Reviewed-by: Derrick Stolee <[email protected]>"

I think we are set for another series on top of this one that lets the commit-graph feature handle arbitrary repositories (pass a 'struct repository *r' in all of the functions).

I've added SQUASH??? patch at the tip of each of the above, rebuilt 'pu' with them and pushed the result out. It seems that Travis is happier with the result.

Please do not forget to squash them in when/if rerolling. If there is no need to change anything else other than squashing them, you can tell me to which commit in your series the fix needs to be squashed in (that would save me time to figure it out, obviously).

Two tests added in dade47c06c (commit-graph: add repo arg to graph readers, 2018-07-11) prepare the contents of 'expect' files by 'echo'ing the results of command substitutions. That's unncessary, avoid them by directly saving the output of the commands executed in those command substitutions.

Signed-off-by SZEDER Gábor <[email protected]>

---
  t/t5318-commit-graph.sh | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

t/t5318-commit-graph.sh +5 -4

index 1c148ebf21..3c1ffad491 100755
@@ -444,25 +444,27 @@ test_expect_success 'setup non-the_repository tests' '
  test_expect_success 'parse_commit_in_graph works for non-the_repository' '
  	test-tool repository parse_commit_in_graph \
  		repo/.git repo "$(git -C repo rev-parse two)" >actual &&
-	echo $(git -C repo log --pretty="%ct" -1) \
-		$(git -C repo rev-parse one) >expect &&
+	{
+		git -C repo log --pretty=format:"%ct " -1 &&
+		git -C repo rev-parse one
+	} >expect &&
  	test_cmp expect actual &&
 
  	test-tool repository parse_commit_in_graph \
  		repo/.git repo "$(git -C repo rev-parse one)" >actual &&
-	echo $(git -C repo log --pretty="%ct" -1 one) >expect &&
+	git -C repo log --pretty="%ct" -1 one >expect &&
  	test_cmp expect actual
  '
 
  test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '
  	test-tool repository get_commit_tree_in_graph \
  		repo/.git repo "$(git -C repo rev-parse two)" >actual &&
-	echo $(git -C repo rev-parse two^{tree}) >expect &&

This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, following Stolee's suggestion.

(It also seems better to build it this way to me, since both these branches are going into "next" according to the latest What's Cooking.)

 Makefile                   |   1 +
 builtin/commit-graph.c     |   2 +
 builtin/fsck.c             |   2 +-
 cache.h                    |   1 -
 commit-graph.c             | 108 +++++++++++++++++++++----------------
 commit-graph.h             |  11 ++--
 commit.c                   |   6 +--
 config.c                   |   5 --
 environment.c              |   1 -
 object-store.h             |   6 +++
 object.c                   |   5 ++
 ref-filter.c               |   2 +-
 t/helper/test-repository.c |  82 ++++++++++++++++++++++++++++
 t/helper/test-tool.c       |   1 +
 t/helper/test-tool.h       |   1 +
 t/t5318-commit-graph.sh    |  35 ++++++++++++
 16 files changed, 207 insertions(+), 62 deletions(-)
 create mode 100644 t/helper/test-repository.c

This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, following Stolee's suggestion.

(It also seems better to build it this way to me, since both these branches are going into "next" according to the latest What's Cooking.)

Junio wrote in [1]:

Quoted 8 lines

I've added SQUASH??? patch at the tip of each of the above, rebuilt 'pu' with them and pushed the result out. It seems that Travis is happier with the result.

Please do not forget to squash them in when/if rerolling. If there is no need to change anything else other than squashing them, you can tell me to which commit in your series the fix needs to be squashed in (that would save me time to figure it out, obviously).

I'm rerolling because I also need to update the last patch with the new lookup_commit() function signature that Stefan's sb/object-store-lookup introduces. I have squashed the SQUASH??? patch into the corresponding patch in this patch set.

Changes from v2:
- now also based on sb/object-store-lookup in addition to ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto sb-object-store-lookup, then rebased this patch set onto the result)
- patches 1-5 are unchanged
- patch 6:
- used "PRItime" instead of "ul" when printing a timestamp (the SQUASH??? patch)
- updated invocations of lookup_commit() to take a repository object

[1] https://public-inbox.org/git/[email protected]/