threads / patch / 48756

patch, 5 partsObject store refactoring: commit graph

Subject: [PATCH 0/5] Object store refactoring: commit graph

## tl;dr

49 messages between Jun 21, 2018 and Aug 13, 2018. Diffs are folded; open one to read it.

replies: 48people: 6as markdown or json

Jonathan Tan· Jun 21, 2018, 21:29 UTC · lore

This continues the object store refactoring effort by making commit graphs of any repository be readable, not just the commit graph of the_repository.

This enables the conversion of other functions in the future, functions like parse_commit (which first tries to parse from the commit graph).

I didn't modify the function that writes commit graphs - perhaps that can be done in a subsequent series.

Jonathan Tan (5):
  object-store: add missing include
  commit-graph: add missing forward declaration
  commit-graph: add free_commit_graph
  commit-graph: store graph in struct object_store
  commit-graph: add repo arg to graph readers
 Makefile                   |  1 +
 builtin/commit-graph.c     |  2 +
 cache.h                    |  1 -
 commit-graph.c             | 90 ++++++++++++++++++++++----------------
 commit-graph.h             |  9 +++-
 commit.c                   |  4 +-
 config.c                   |  5 ---
 environment.c              |  1 -
 object-store.h             |  6 +++
 object.c                   |  5 +++
 t/helper/test-repository.c | 88 +++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |  1 +
 t/helper/test-tool.h       |  1 +
 t/t5318-commit-graph.sh    | 35 +++++++++++++++
 14 files changed, 201 insertions(+), 48 deletions(-)
 create mode 100644 t/helper/test-repository.c
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Jonathan Tan· Jun 21, 2018, 21:29 UTC · re: Jonathan Tan · lore

[PATCH 1/5] object-store: add missing include

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)
Show changes to object-store.h +3 −0
diff --git a/object-store.h b/object-store.h
index d683112fd7..f0b77146e4 100644
--- a/object-store.h
+++ b/object-store.h
@@ -2,6 +2,9 @@
 #define OBJECT_STORE_H
 
 #include "oidmap.h"
+#include "list.h"
+#include "sha1-array.h"
+#include "strbuf.h"
 
 struct alternate_object_database {
 	struct alternate_object_database *next;
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Jonathan Tan· Jun 21, 2018, 21:29 UTC · re: Jonathan Tan · lore

[PATCH 2/5] commit-graph: add missing forward declaration

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to commit-graph.h +2 −0
diff --git a/commit-graph.h b/commit-graph.h
index 260a468e73..7004dfdca9 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -3,6 +3,8 @@
 
 #include "git-compat-util.h"
 
+struct commit;
+
 char *get_commit_graph_filename(const char *obj_dir);
 
 /*
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Stefan Beller· Jun 21, 2018, 21:43 UTC · re: Jonathan Tan · lore

Re: [PATCH 2/5] commit-graph: add missing forward declaration

Hi Jonathan, On Thu, Jun 21, 2018 at 2:29 PM Jonathan Tan <jonathantanmy@google.com> wrote:

>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

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)

Jonathan Tan· Jun 21, 2018, 22:39 UTC · re: Stefan Beller · lore

Re: [PATCH 2/5] commit-graph: add missing forward declaration

> 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.

Derrick Stolee· Jun 22, 2018, 00:17 UTC · re: Jonathan Tan · lore

Re: [PATCH 2/5] commit-graph: add missing forward declaration

On 6/21/2018 6:39 PM, Jonathan Tan wrote:
Show 6 quoted 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.

I discovered similar missing headers in object-store.h while reworking the MIDX patch series. Thanks for fixing them here!

Jonathan Tan· Jun 21, 2018, 21:29 UTC · re: Jonathan Tan · lore

[PATCH 3/5] commit-graph: add free_commit_graph

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit-graph.c |  2 ++
 commit-graph.c         | 24 ++++++++++++++----------
 commit-graph.h         |  2 ++
 3 files changed, 18 insertions(+), 10 deletions(-)
Show changes to 3 files +18 −10

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 37420ae0fd..9c2d55221c 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -74,6 +74,8 @@ static int graph_read(int argc, const char **argv)
 		printf(" large_edges");
 	printf("\n");
 
+	free_commit_graph(graph);
+
 	return 0;
 }
 
diff --git a/commit-graph.c b/commit-graph.c
index 4c6127088f..9f4e761229 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -216,16 +216,8 @@ static void prepare_commit_graph(void)
 
 static void close_commit_graph(void)
 {
-	if (!commit_graph)
-		return;
-
-	if (commit_graph->graph_fd >= 0) {
-		munmap((void *)commit_graph->data, commit_graph->data_len);
-		commit_graph->data = NULL;
-		close(commit_graph->graph_fd);
-	}
-
-	FREE_AND_NULL(commit_graph);
+	free_commit_graph(commit_graph);
+	commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -759,3 +751,15 @@ void write_commit_graph(const char *obj_dir,
 	oids.alloc = 0;
 	oids.nr = 0;
 }
+
+void free_commit_graph(struct commit_graph *g)
+{
+	if (!g)
+		return;
+	if (g->graph_fd >= 0) {
+		munmap((void *)g->data, g->data_len);
+		g->data = NULL;
+		close(g->graph_fd);
+	}
+	free(g);
+}
diff --git a/commit-graph.h b/commit-graph.h
index 7004dfdca9..320ee9fd8a 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -47,4 +47,6 @@ void write_commit_graph(const char *obj_dir,
 			int nr_commits,
 			int append);
 
+void free_commit_graph(struct commit_graph *);
+
 #endif
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Jonathan Tan· Jun 21, 2018, 21:29 UTC · re: Jonathan Tan · lore

[PATCH 4/5] commit-graph: store graph in struct object_store

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 <jonathantanmy@google.com>
---
 commit-graph.c | 34 ++++++++++++++++++----------------
 object-store.h |  3 +++
 object.c       |  5 +++++
 3 files changed, 26 insertions(+), 16 deletions(-)
Show changes to 3 files +26 −16

commit-graph.c, object-store.h, object.c

diff --git a/commit-graph.c b/commit-graph.c
index 9f4e761229..61b4fbb925 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -179,45 +179,42 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	exit(1);
 }
 
-/* global storage */
-static struct commit_graph *commit_graph = NULL;
-
 static void prepare_commit_graph_one(const char *obj_dir)
 {
 	char *graph_name;
 
-	if (commit_graph)
+	if (the_repository->objects->commit_graph)
 		return;
 
 	graph_name = get_commit_graph_filename(obj_dir);
-	commit_graph = load_commit_graph_one(graph_name);
+	the_repository->objects->commit_graph =
+		load_commit_graph_one(graph_name);
 
 	FREE_AND_NULL(graph_name);
 }
 
-static int prepare_commit_graph_run_once = 0;
 static void prepare_commit_graph(void)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
 
-	if (prepare_commit_graph_run_once)
+	if (the_repository->objects->commit_graph_attempted)
 		return;
-	prepare_commit_graph_run_once = 1;
+	the_repository->objects->commit_graph_attempted = 1;
 
 	obj_dir = get_object_directory();
 	prepare_commit_graph_one(obj_dir);
 	prepare_alt_odb(the_repository);
 	for (alt = the_repository->objects->alt_odb_list;
-	     !commit_graph && alt;
+	     !the_repository->objects->commit_graph && alt;
 	     alt = alt->next)
 		prepare_commit_graph_one(alt->path);
 }
 
 static void close_commit_graph(void)
 {
-	free_commit_graph(commit_graph);
-	commit_graph = NULL;
+	free_commit_graph(the_repository->objects->commit_graph);
+	the_repository->objects->commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -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) {
@@ -329,7 +328,8 @@ struct tree *get_commit_tree_in_graph(const struct commit *c)
 	if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
 		BUG("get_commit_tree_in_graph called from non-commit-graph commit");
 
-	return load_tree_for_commit(commit_graph, (struct commit *)c);
+	return load_tree_for_commit(the_repository->objects->commit_graph,
+				    (struct commit *)c);
 }
 
 static void write_graph_chunk_fanout(struct hashfile *f,
@@ -589,15 +589,17 @@ void write_commit_graph(const char *obj_dir,
 
 	if (append) {
 		prepare_commit_graph_one(obj_dir);
-		if (commit_graph)
-			oids.alloc += commit_graph->num_commits;
+		if (the_repository->objects->commit_graph)
+			oids.alloc += the_repository->objects->commit_graph->num_commits;
 	}
 
 	if (oids.alloc < 1024)
 		oids.alloc = 1024;
 	ALLOC_ARRAY(oids.list, oids.alloc);
 
-	if (append && commit_graph) {
+	if (append && the_repository->objects->commit_graph) {
+		struct commit_graph *commit_graph =
+			the_repository->objects->commit_graph;
 		for (i = 0; i < commit_graph->num_commits; i++) {
 			const unsigned char *hash = commit_graph->chunk_oid_lookup +
 				commit_graph->hash_len * i;
diff --git a/object-store.h b/object-store.h
index f0b77146e4..39f4a3d5b9 100644
--- a/object-store.h
+++ b/object-store.h
@@ -106,6 +106,9 @@ struct raw_object_store {
 	 */
 	struct oidmap *replace_map;
 
+	struct commit_graph *commit_graph;
+	unsigned commit_graph_attempted : 1; /* if loading has been attempted */
+
 	/*
 	 * private data
 	 *
diff --git a/object.c b/object.c
index f7f4de3aaf..04954f4f3e 100644
--- a/object.c
+++ b/object.c
@@ -7,6 +7,7 @@
 #include "tag.h"
 #include "object-store.h"
 #include "packfile.h"
+#include "commit-graph.h"
 
 static struct object **obj_hash;
 static int nr_objs, obj_hash_size;
@@ -484,6 +485,10 @@ void raw_object_store_clear(struct raw_object_store *o)
 	oidmap_free(o->replace_map, 1);
 	FREE_AND_NULL(o->replace_map);
 
+	free_commit_graph(o->commit_graph);
+	o->commit_graph = NULL;
+	o->commit_graph_attempted = 0;
+
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Junio C Hamano· Jun 25, 2018, 20:57 UTC · re: Jonathan Tan · lore

Re: [PATCH 4/5] commit-graph: store graph in struct object_store

Jonathan Tan <jonathantanmy@google.com> writes:
Show 32 quoted lines
> 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 <jonathantanmy@google.com>
> ---
>  commit-graph.c | 34 ++++++++++++++++++----------------
>  object-store.h |  3 +++
>  object.c       |  5 +++++
>  3 files changed, 26 insertions(+), 16 deletions(-)
> ...
> @@ -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) {
> @@ -329,7 +328,8 @@ struct tree *get_commit_tree_in_graph(const struct commit *c)
>  	if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
>  		BUG("get_commit_tree_in_graph called from non-commit-graph commit");
>  
> -	return load_tree_for_commit(commit_graph, (struct commit *)c);
> +	return load_tree_for_commit(the_repository->objects->commit_graph,
> +				    (struct commit *)c);
>  }

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);
}
Jonathan Tan· Jun 25, 2018, 22:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/5] commit-graph: store graph in struct object_store

Show 33 quoted lines
> 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);
> }

Thanks for letting me know - when I reroll, I'll ensure that I reroll on top of this change.

As for whether both these functions are necessary in the first place, I think they are: we could have a load_commit_graph_info() that ignores the parsedness of the commit and just copies everything over, but that means that we can't have the optimization of returning immediately without consulting the graph when attempting to reparse a parsed object.

So we can't delete parse_commit_in_graph(). And we can't delete load_commit_graph_info() either, because after reading the documentation in commit-graph.h, looking at the places where it is used, and observing some test failures when I make parse_object_buffer() not use the graph, I see that there are code paths in Git in which we use both parsed-from-buffer and parsed-without-buffer commits at the same time.

So, I now look at the code duplication, and I see that it is mostly the check-prepare-check step; my patch set reduces it a little, but I'll try to reduce it nearly completely when I reroll too.

Maybe when the object store becomes more pervasive, we can more clearly separate out the buffers that come from a repository (and maybe, when obtaining those buffers, we should obtain graph information at the same time) and buffers that we obtain from some arbitrary non-repository source, but that will take some time. In the meantime, I'll do what I suggested above when I reroll, maintaining these 2 functions.

Junio C Hamano· Jun 26, 2018, 16:40 UTC · re: Jonathan Tan · lore

Re: [PATCH 4/5] commit-graph: store graph in struct object_store

Jonathan Tan <jonathantanmy@google.com> writes:
> As for whether both these functions are necessary in the first place, I
> think they are.
I do not mind their existence.  

I was wondering if they can share more implementation; such a design would need s/the_commit_graph/the_repo->objstore->commit_graph/ only once as a side effect.

Jonathan Tan· Jun 21, 2018, 21:29 UTC · re: Jonathan Tan · lore

[PATCH 5/5] commit-graph: add repo arg to graph readers

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 <jonathantanmy@google.com>
---
 Makefile                   |  1 +
 cache.h                    |  1 -
 commit-graph.c             | 52 +++++++++++++---------
 commit-graph.h             |  5 ++-
 commit.c                   |  4 +-
 config.c                   |  5 ---
 environment.c              |  1 -
 t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |  1 +
 t/helper/test-tool.h       |  1 +
 t/t5318-commit-graph.sh    | 35 +++++++++++++++
 11 files changed, 162 insertions(+), 32 deletions(-)
 create mode 100644 t/helper/test-repository.c
Show changes to 11 files +162 −32

Makefile, cache.h, commit-graph.c, commit-graph.h, commit.c, config.c, environment.c, t/helper/test-repository.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t5318-commit-graph.sh

diff --git a/Makefile b/Makefile
index e4b503d259..12d754cb70 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/cache.h b/cache.h
index 89a107a7f7..ebf699fb0f 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/commit-graph.c b/commit-graph.c
index 61b4fbb925..12c4addf75 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -179,36 +179,47 @@ 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);
 }
 
-static void prepare_commit_graph(void)
+static void prepare_commit_graph(struct repository *r)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
+	int config_value;
 
-	if (the_repository->objects->commit_graph_attempted)
+	if (r->objects->commit_graph_attempted)
+		return;
+	r->objects->commit_graph_attempted = 1;
+
+	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value)
+		/*
+		 * 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.)
+		 */
 		return;
-	the_repository->objects->commit_graph_attempted = 1;
 
-	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);
+		prepare_commit_graph_one(r, alt->path);
 }
 
 static void close_commit_graph(void)
@@ -282,17 +293,15 @@ static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, uin
 	return 1;
 }
 
-int parse_commit_in_graph(struct commit *item)
+int parse_commit_in_graph(struct repository *r, struct commit *item)
 {
-	if (!core_commit_graph)
-		return 0;
 	if (item->object.parsed)
 		return 1;
 
-	prepare_commit_graph();
-	if (the_repository->objects->commit_graph) {
+	prepare_commit_graph(r);
+	if (r->objects->commit_graph) {
 		struct commit_graph *commit_graph =
-			the_repository->objects->commit_graph;
+			r->objects->commit_graph;
 		uint32_t pos;
 		int found;
 		if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
@@ -321,14 +330,15 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *
 	return c->maybe_tree;
 }
 
-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)
 {
 	if (c->maybe_tree)
 		return c->maybe_tree;
 	if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
 		BUG("get_commit_tree_in_graph called from non-commit-graph commit");
 
-	return load_tree_for_commit(the_repository->objects->commit_graph,
+	return load_tree_for_commit(r->objects->commit_graph,
 				    (struct commit *)c);
 }
 
@@ -588,7 +598,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;
 	}
diff --git a/commit-graph.h b/commit-graph.h
index 320ee9fd8a..192ea2ca77 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -17,9 +17,10 @@ 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);
 
-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;
diff --git a/commit.c b/commit.c
index 0030e79940..38c12b002f 100644
--- a/commit.c
+++ b/commit.c
@@ -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);
 }
 
 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))
 		return 0;
 	buffer = read_object_file(&item->object.oid, &type, &size);
 	if (!buffer)
diff --git a/config.c b/config.c
index fbbf0f8e9f..072e9bdb4d 100644
--- a/config.c
+++ b/config.c
@@ -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;
diff --git a/environment.c b/environment.c
index 2a6de2330b..d6e5b39425 100644
--- a/environment.c
+++ b/environment.c
@@ -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() */
diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
new file mode 100644
index 0000000000..5fff540a26
--- /dev/null
+++ b/t/helper/test-repository.c
@@ -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,
+					  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);
+
+	repo_init(&r, gitdir, worktree);
+
+	/*
+	 * 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");
+
+	printf("%s\n", oid_to_hex(&tree->object.oid));
+
+	repo_clear(&r);
+}
+
+int cmd__repository(int argc, const char **argv)
+{
+	if (argc < 2)
+		die("must have at least 2 arguments");
+	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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..dafc91c240 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..80cbcf0857 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a380419b65..f80bbaec7f 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -221,4 +221,39 @@ test_expect_success 'write graph in bare repo' '
 graph_git_behavior 'bare repo with graph, commit 8 vs merge 1' bare commits/8 merge/1
 graph_git_behavior 'bare repo with graph, commit 8 vs merge 2' bare commits/8 merge/2
 
+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
-- 
2.18.0.rc2.347.g0da03f3a46.dirty
Stefan Beller· Jun 21, 2018, 22:41 UTC · re: Jonathan Tan · lore

Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

Show 10 quoted lines
> diff --git a/commit.c b/commit.c
> index 0030e79940..38c12b002f 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -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..
Show 9 quoted lines
>  }
>
>  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
Show 11 quoted lines
> +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.

> +test_expect_success 'setup non-the_repository tests' '
> +test_expect_success 'parse_commit_in_graph works for non-the_repository' '
> +test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '
This is really nice!

Overall this series looks good to me, Stefan

Jonathan Tan· Jun 21, 2018, 23:06 UTC · re: Stefan Beller · lore

Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

Show 39 quoted lines
> > diff --git a/commit.c b/commit.c
> > index 0030e79940..38c12b002f 100644
> > --- a/commit.c
> > +++ b/commit.c
> > @@ -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..
> 
> >  }
> >
> >  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
> 
> > +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.

> 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/20180613230522.55335-1-sbeller@google.com/

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.

> This is really nice!
> 
> Overall this series looks good to me,
> Stefan
Thanks - let's see what others think.
Derrick Stolee· Jun 22, 2018, 00:33 UTC · re: Jonathan Tan · lore

Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

On 6/21/2018 7:06 PM, Jonathan Tan wrote:
Show 51 quoted lines
>>> diff --git a/commit.c b/commit.c
>>> index 0030e79940..38c12b002f 100644
>>> --- a/commit.c
>>> +++ b/commit.c
>>> @@ -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..
>>
>>>   }
>>>
>>>   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
>>
>>> +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.
>
>> 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/20180613230522.55335-1-sbeller@google.com/
>
> 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/20180608135548.216405-4-dstolee@microsoft.com/

Show 5 quoted lines
>> This is really nice!
>>
>> Overall this series looks good to me,
>> Stefan
> Thanks - let's see what others think.
Derrick Stolee· Jun 22, 2018, 01:01 UTC · re: Derrick Stolee · lore

Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

On 6/21/2018 8:33 PM, Derrick Stolee wrote:
Show 71 quoted lines
> On 6/21/2018 7:06 PM, Jonathan Tan wrote:
>>>> diff --git a/commit.c b/commit.c
>>>> index 0030e79940..38c12b002f 100644
>>>> --- a/commit.c
>>>> +++ b/commit.c
>>>> @@ -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..
>>>
>>>>   }
>>>>
>>>>   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
>>>
>>>> +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.
>>
>>> 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/20180613230522.55335-1-sbeller@google.com/
>>
>> 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/20180608135548.216405-4-dstolee@microsoft.com/

Here is the re-send: https://public-inbox.org/git/4e7600f1-6dd0-3b21-5f5d-26af2b3c0b1a@gmail.com/T/#t

Jonathan Tan· Jun 22, 2018, 17:21 UTC · re: Derrick Stolee · lore

Re: [PATCH 5/5] commit-graph: add repo arg to graph readers

Show 56 quoted lines
> On 6/21/2018 7:06 PM, Jonathan Tan wrote:
> >>> diff --git a/commit.c b/commit.c
> >>> index 0030e79940..38c12b002f 100644
> >>> --- a/commit.c
> >>> +++ b/commit.c
> >>> @@ -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..
> >>
> >>>   }
> >>>
> >>>   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
> >>
> >>> +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.
> >
> >> 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/20180613230522.55335-1-sbeller@google.com/
> >
> > 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.

Not sure what you mean by the lookup_commit() calls (if you're referring to the part quoted above, that is test code), but parse_commit_in_graph() has to take a struct repository (or at least a struct object_store, perhaps) because it needs to load the commit graph for the repository if it is not already loaded.

(An alternative, of course, is to require the user to explicitly load the graph, but since parse_commit_in_graph() is called from parse_commit(), I think that this implicit loading is fine.)

Show 9 quoted lines
> 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/20180608135548.216405-4-dstolee@microsoft.com/

Thanks - I see that you introduced a new parse_commit_in_graph_one(struct commit_graph *, struct commit *) and made the existing parse_commit_in_graph(struct commit *item) use the former. Combining our changes would be just adding a repository argument to parse_commit_in_graph() and passing the graph through parse_commit_in_graph_one() (after prepare_commit_graph() and ensuring that the graph exists).

Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph

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/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/
Jonathan Tan (6):
  commit-graph: refactor preparing commit graph
  object-store: add missing include
  commit-graph: add missing forward declaration
  commit-graph: add free_commit_graph
  commit-graph: store graph in struct object_store
  commit-graph: add repo arg to graph readers
 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 |  88 ++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |   1 +
 t/helper/test-tool.h       |   1 +
 t/t5318-commit-graph.sh    |  35 ++++++++++++
 16 files changed, 213 insertions(+), 62 deletions(-)
 create mode 100644 t/helper/test-repository.c
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 1/6] commit-graph: refactor preparing commit graph

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 <jonathantanmy@google.com>
---
 commit-graph.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to commit-graph.c +17 −11
diff --git a/commit-graph.c b/commit-graph.c
index 212232e752..5ba60f63f9 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -200,15 +200,25 @@ static void prepare_commit_graph_one(const char *obj_dir)
 }
 
 static int prepare_commit_graph_run_once = 0;
-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.
+ */
+static int prepare_commit_graph(void)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
 
 	if (prepare_commit_graph_run_once)
-		return;
+		return !!commit_graph;
 	prepare_commit_graph_run_once = 1;
 
+	if (!core_commit_graph)
+		return 0;
+
 	obj_dir = get_object_directory();
 	prepare_commit_graph_one(obj_dir);
 	prepare_alt_odb(the_repository);
@@ -216,6 +226,7 @@ static void prepare_commit_graph(void)
 	     !commit_graph && alt;
 	     alt = alt->next)
 		prepare_commit_graph_one(alt->path);
+	return !!commit_graph;
 }
 
 static void close_commit_graph(void)
@@ -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);
 }
 
 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);
 }
 
-- 
2.18.0.203.gfac676dfb9-goog
Stefan Beller· Jul 9, 2018, 21:41 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 1/6] commit-graph: refactor preparing commit graph

Hi Jonathan, On Mon, Jul 9, 2018 at 1:44 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 7 quoted lines
>
> 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 <jonathantanmy@google.com>
Show 8 quoted lines
>  static int prepare_commit_graph_run_once = 0;
> -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!
Show 13 quoted lines
> @@ -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.
Show 12 quoted lines
>  }
>
>  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 <sbeller@google.com>
Derrick Stolee· Jul 10, 2018, 00:23 UTC · re: Stefan Beller · lore

Re: [PATCH v2 1/6] commit-graph: refactor preparing commit graph

On 7/9/2018 5:41 PM, Stefan Beller wrote:
Show 55 quoted lines
> Hi Jonathan,
> On Mon, Jul 9, 2018 at 1:44 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>> 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 <jonathantanmy@google.com>
>
>
>>   static int prepare_commit_graph_run_once = 0;
>> -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!
>
>> @@ -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.
>
>>   }
>>
>>   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 <sbeller@google.com>
These changes make sense. Thanks!
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 2/6] object-store: add missing include

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)
Show changes to object-store.h +3 −0
diff --git a/object-store.h b/object-store.h
index d683112fd7..f0b77146e4 100644
--- a/object-store.h
+++ b/object-store.h
@@ -2,6 +2,9 @@
 #define OBJECT_STORE_H
 
 #include "oidmap.h"
+#include "list.h"
+#include "sha1-array.h"
+#include "strbuf.h"
 
 struct alternate_object_database {
 	struct alternate_object_database *next;
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 3/6] commit-graph: add missing forward declaration

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to commit-graph.h +2 −0
diff --git a/commit-graph.h b/commit-graph.h
index 506cb45fb1..674052bef4 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -5,6 +5,8 @@
 #include "repository.h"
 #include "string-list.h"
 
+struct commit;
+
 char *get_commit_graph_filename(const char *obj_dir);
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 4/6] commit-graph: add free_commit_graph

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit-graph.c |  2 ++
 commit-graph.c         | 24 ++++++++++++++----------
 commit-graph.h         |  2 ++
 3 files changed, 18 insertions(+), 10 deletions(-)
Show changes to 3 files +18 −10

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c7d0db5ab4..0bf0c48657 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -115,6 +115,8 @@ static int graph_read(int argc, const char **argv)
 		printf(" large_edges");
 	printf("\n");
 
+	free_commit_graph(graph);
+
 	return 0;
 }
 
diff --git a/commit-graph.c b/commit-graph.c
index 5ba60f63f9..6d1bc4ff7c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -231,16 +231,8 @@ static int prepare_commit_graph(void)
 
 static void close_commit_graph(void)
 {
-	if (!commit_graph)
-		return;
-
-	if (commit_graph->graph_fd >= 0) {
-		munmap((void *)commit_graph->data, commit_graph->data_len);
-		commit_graph->data = NULL;
-		close(commit_graph->graph_fd);
-	}
-
-	FREE_AND_NULL(commit_graph);
+	free_commit_graph(commit_graph);
+	commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -1033,3 +1025,15 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 
 	return verify_commit_graph_error;
 }
+
+void free_commit_graph(struct commit_graph *g)
+{
+	if (!g)
+		return;
+	if (g->graph_fd >= 0) {
+		munmap((void *)g->data, g->data_len);
+		g->data = NULL;
+		close(g->graph_fd);
+	}
+	free(g);
+}
diff --git a/commit-graph.h b/commit-graph.h
index 674052bef4..94defb04a9 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -58,4 +58,6 @@ void write_commit_graph(const char *obj_dir,
 
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 
+void free_commit_graph(struct commit_graph *);
+
 #endif
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 5/6] commit-graph: store graph in struct object_store

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 <jonathantanmy@google.com>
---
 commit-graph.c | 40 +++++++++++++++++++---------------------
 object-store.h |  3 +++
 object.c       |  5 +++++
 3 files changed, 27 insertions(+), 21 deletions(-)
Show changes to 3 files +27 −21

commit-graph.c, object-store.h, object.c

diff --git a/commit-graph.c b/commit-graph.c
index 6d1bc4ff7c..af97a10603 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -183,24 +183,20 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	exit(1);
 }
 
-/* global storage */
-static struct commit_graph *commit_graph = NULL;
-
 static void prepare_commit_graph_one(const char *obj_dir)
 {
 	char *graph_name;
 
-	if (commit_graph)
+	if (the_repository->objects->commit_graph)
 		return;
 
 	graph_name = get_commit_graph_filename(obj_dir);
-	commit_graph = load_commit_graph_one(graph_name);
+	the_repository->objects->commit_graph =
+		load_commit_graph_one(graph_name);
 
 	FREE_AND_NULL(graph_name);
 }
 
-static int prepare_commit_graph_run_once = 0;
-
 /*
  * Return 1 if commit_graph is non-NULL, and 0 otherwise.
  *
@@ -212,9 +208,9 @@ static int prepare_commit_graph(void)
 	struct alternate_object_database *alt;
 	char *obj_dir;
 
-	if (prepare_commit_graph_run_once)
-		return !!commit_graph;
-	prepare_commit_graph_run_once = 1;
+	if (the_repository->objects->commit_graph_attempted)
+		return !!the_repository->objects->commit_graph;
+	the_repository->objects->commit_graph_attempted = 1;
 
 	if (!core_commit_graph)
 		return 0;
@@ -223,16 +219,16 @@ static int prepare_commit_graph(void)
 	prepare_commit_graph_one(obj_dir);
 	prepare_alt_odb(the_repository);
 	for (alt = the_repository->objects->alt_odb_list;
-	     !commit_graph && alt;
+	     !the_repository->objects->commit_graph && alt;
 	     alt = alt->next)
 		prepare_commit_graph_one(alt->path);
-	return !!commit_graph;
+	return !!the_repository->objects->commit_graph;
 }
 
 static void close_commit_graph(void)
 {
-	free_commit_graph(commit_graph);
-	commit_graph = NULL;
+	free_commit_graph(the_repository->objects->commit_graph);
+	the_repository->objects->commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -342,7 +338,7 @@ int parse_commit_in_graph(struct commit *item)
 {
 	if (!prepare_commit_graph())
 		return 0;
-	return parse_commit_in_graph_one(commit_graph, item);
+	return parse_commit_in_graph_one(the_repository->objects->commit_graph, item);
 }
 
 void load_commit_graph_info(struct commit *item)
@@ -350,8 +346,8 @@ void load_commit_graph_info(struct commit *item)
 	uint32_t pos;
 	if (!prepare_commit_graph())
 		return;
-	if (find_commit_in_graph(item, commit_graph, &pos))
-		fill_commit_graph_info(item, commit_graph, pos);
+	if (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))
+		fill_commit_graph_info(item, the_repository->objects->commit_graph, pos);
 }
 
 static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)
@@ -379,7 +375,7 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
 
 struct tree *get_commit_tree_in_graph(const struct commit *c)
 {
-	return get_commit_tree_in_graph_one(commit_graph, c);
+	return get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);
 }
 
 static void write_graph_chunk_fanout(struct hashfile *f,
@@ -696,15 +692,17 @@ void write_commit_graph(const char *obj_dir,
 
 	if (append) {
 		prepare_commit_graph_one(obj_dir);
-		if (commit_graph)
-			oids.alloc += commit_graph->num_commits;
+		if (the_repository->objects->commit_graph)
+			oids.alloc += the_repository->objects->commit_graph->num_commits;
 	}
 
 	if (oids.alloc < 1024)
 		oids.alloc = 1024;
 	ALLOC_ARRAY(oids.list, oids.alloc);
 
-	if (append && commit_graph) {
+	if (append && the_repository->objects->commit_graph) {
+		struct commit_graph *commit_graph =
+			the_repository->objects->commit_graph;
 		for (i = 0; i < commit_graph->num_commits; i++) {
 			const unsigned char *hash = commit_graph->chunk_oid_lookup +
 				commit_graph->hash_len * i;
diff --git a/object-store.h b/object-store.h
index f0b77146e4..39f4a3d5b9 100644
--- a/object-store.h
+++ b/object-store.h
@@ -106,6 +106,9 @@ struct raw_object_store {
 	 */
 	struct oidmap *replace_map;
 
+	struct commit_graph *commit_graph;
+	unsigned commit_graph_attempted : 1; /* if loading has been attempted */
+
 	/*
 	 * private data
 	 *
diff --git a/object.c b/object.c
index 10d167825e..6672820b9e 100644
--- a/object.c
+++ b/object.c
@@ -8,6 +8,7 @@
 #include "alloc.h"
 #include "object-store.h"
 #include "packfile.h"
+#include "commit-graph.h"
 
 unsigned int get_max_object_index(void)
 {
@@ -501,6 +502,10 @@ void raw_object_store_clear(struct raw_object_store *o)
 	oidmap_free(o->replace_map, 1);
 	FREE_AND_NULL(o->replace_map);
 
+	free_commit_graph(o->commit_graph);
+	o->commit_graph = NULL;
+	o->commit_graph_attempted = 0;
+
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 9, 2018, 20:44 UTC · re: Jonathan Tan · lore

[PATCH v2 6/6] commit-graph: add repo arg to graph readers

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 <jonathantanmy@google.com>
---
 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
Show changes to 13 files +168 −42

Makefile, builtin/fsck.c, cache.h, commit-graph.c, commit-graph.h, commit.c, config.c, environment.c, ref-filter.c, t/helper/test-repository.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t5318-commit-graph.sh

diff --git a/Makefile b/Makefile
index 0cb6590f24..bb8bd67201 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/builtin/fsck.c b/builtin/fsck.c
index eca7900ee0..2abfb2d782 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -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 };
 
diff --git a/cache.h b/cache.h
index d49092d94d..8dc59bedba 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/commit-graph.c b/commit-graph.c
index af97a10603..8eab199b1b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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;
 
-	if (!core_commit_graph)
+	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value)
+		/*
+		 * 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.)
+		 */
 		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;
 	}
diff --git a/commit-graph.h b/commit-graph.h
index 94defb04a9..76e098934a 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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;
diff --git a/commit.c b/commit.c
index 598cf21cee..b3bbfefc27 100644
--- a/commit.c
+++ b/commit.c
@@ -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)
diff --git a/config.c b/config.c
index a0a6ae1980..fcf863b667 100644
--- a/config.c
+++ b/config.c
@@ -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;
diff --git a/environment.c b/environment.c
index 2a6de2330b..d6e5b39425 100644
--- a/environment.c
+++ b/environment.c
@@ -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() */
diff --git a/ref-filter.c b/ref-filter.c
index fa3685d91f..889d97b005 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1710,7 +1710,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
 
 	for (p = want; p; p = p->next) {
 		struct commit *c = p->item;
-		load_commit_graph_info(c);
+		load_commit_graph_info(the_repository, c);
 		if (c->generation < cutoff)
 			cutoff = c->generation;
 	}
diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
new file mode 100644
index 0000000000..5fff540a26
--- /dev/null
+++ b/t/helper/test-repository.c
@@ -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,
+					  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);
+
+	repo_init(&r, gitdir, worktree);
+
+	/*
+	 * 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");
+
+	printf("%s\n", oid_to_hex(&tree->object.oid));
+
+	repo_clear(&r);
+}
+
+int cmd__repository(int argc, const char **argv)
+{
+	if (argc < 2)
+		die("must have at least 2 arguments");
+	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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..dafc91c240 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..80cbcf0857 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 5947de3d24..4f17d7701e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -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
-- 
2.18.0.203.gfac676dfb9-goog
Derrick Stolee· Jul 10, 2018, 00:48 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

On 7/9/2018 4:44 PM, Jonathan Tan wrote:
Show 118 quoted lines
> 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 <jonathantanmy@google.com>
> ---
>   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
>
> diff --git a/Makefile b/Makefile
> index 0cb6590f24..bb8bd67201 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index eca7900ee0..2abfb2d782 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -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 };
>   
> diff --git a/cache.h b/cache.h
> index d49092d94d..8dc59bedba 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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;
> diff --git a/commit-graph.c b/commit-graph.c
> index af97a10603..8eab199b1b 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -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;
>   
> -	if (!core_commit_graph)
> +	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
> +	    !config_value)
> +		/*
> +		 * 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.

Show 176 quoted lines
>   		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;
>   	}
> diff --git a/commit-graph.h b/commit-graph.h
> index 94defb04a9..76e098934a 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -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;
> diff --git a/commit.c b/commit.c
> index 598cf21cee..b3bbfefc27 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -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)
> diff --git a/config.c b/config.c
> index a0a6ae1980..fcf863b667 100644
> --- a/config.c
> +++ b/config.c
> @@ -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;
> diff --git a/environment.c b/environment.c
> index 2a6de2330b..d6e5b39425 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -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() */
> diff --git a/ref-filter.c b/ref-filter.c
> index fa3685d91f..889d97b005 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1710,7 +1710,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
>   
>   	for (p = want; p; p = p->next) {
>   		struct commit *c = p->item;
> -		load_commit_graph_info(c);
> +		load_commit_graph_info(the_repository, c);
>   		if (c->generation < cutoff)
>   			cutoff = c->generation;
>   	}

The changes above are mostly obvious. They don't need too much reading to know they are correct.

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.

Show 53 quoted lines
> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
> new file mode 100644
> index 0000000000..5fff540a26
> --- /dev/null
> +++ b/t/helper/test-repository.c
> @@ -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,
> +					  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)?

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.

> +
> +	repo_init(&r, gitdir, worktree);
I think you want to move the lookup_commit() to after this.
Show 20 quoted lines
> +
> +	/*
> +	 * 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");
> +
> +	printf("%s\n", oid_to_hex(&tree->object.oid));
> +
> +	repo_clear(&r);
> +}
> +
> +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.

Show 87 quoted lines
> +	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;
> +}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 805a45de9c..dafc91c240 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -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 },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 7116ddfb94..80cbcf0857 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -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);
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 5947de3d24..4f17d7701e 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -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
Jonathan Tan· Jul 10, 2018, 17:31 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

Show 12 quoted lines
> > -	if (!core_commit_graph)
> > +	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
> > +	    !config_value)
> > +		/*
> > +		 * 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.

Thanks for checking. It is indeed run at most once per repository, and only if a commit graph is requested - the same as the current code.

> 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.

The patch is 168+ 42-, which doesn't seem that large to me, but I'll do this if others think that it is large too.

> 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.

I agree - let me know if you notice anything you think should be changed.

Show 17 quoted lines
> > +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.
> 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.

> > +
> > +	repo_init(&r, gitdir, worktree);
> 
> I think you want to move the lookup_commit() to after this.
Yes, that's right.
Show 7 quoted lines
> > +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.
Thanks.
Derrick Stolee· Jul 11, 2018, 19:41 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

On 7/10/2018 1:31 PM, Jonathan Tan wrote:
Show 32 quoted lines
>
>>> +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.
>
>> 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).

Thanks,
-Stolee
Jonathan Tan· Jul 11, 2018, 21:08 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

Show 13 quoted 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).

Good point. Junio requested a reroll in his What's Cooking e-mail [1], and the same e-mail states that ds/commit-graph-fsck and sb/object-store-lookup will be merged to next, so there are a few good reasons to base it on both. I'll do that and send out an updated version soon.

[1] https://public-inbox.org/git/xmqq7em138a5.fsf@gitster-ct.c.googlers.com/
SZEDER Gábor· Jul 10, 2018, 11:53 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

Show 34 quoted lines
> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
> new file mode 100644
> index 0000000000..5fff540a26
> --- /dev/null
> +++ b/t/helper/test-repository.c
> @@ -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);
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
Show 6 quoted lines
> +	for (parent = c->parents; parent; parent = parent->next)
> +		printf(" %s", oid_to_hex(&parent->item->object.oid));
> +	printf("\n");
> +
> +	repo_clear(&r);
> +}
Johannes Schindelin· Jul 10, 2018, 13:18 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

Hi,
On Tue, 10 Jul 2018, SZEDER Gábor wrote:
Show 44 quoted lines
> > diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
> > new file mode 100644
> > index 0000000000..5fff540a26
> > --- /dev/null
> > +++ b/t/helper/test-repository.c
> > @@ -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);
> 
> 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);

Ciao, Dscho

Jonathan Tan· Jul 10, 2018, 17:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers

On Tue, Jul 10, 2018 at 6:18 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted 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);
Thanks for noticing this - I'll do this if a reroll is needed.
Stefan Beller· Jul 9, 2018, 21:47 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph

Hi Jonathan,
> 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].

I think this series is good; Thanks, Stefan

Junio C Hamano· Jul 9, 2018, 22:27 UTC · re: Jonathan Tan · lore

Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph

Jonathan Tan <jonathantanmy@google.com> writes:
Show 11 quoted lines
> 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/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/

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?

Derrick Stolee· Jul 10, 2018, 00:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph

On 7/9/2018 6:27 PM, Junio C Hamano wrote:
Show 18 quoted lines
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> 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/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/
> 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 <dstolee@microsoft.com>"

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).

Thanks,
-Stolee
Derrick Stolee· Jul 10, 2018, 00:32 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph

On 7/9/2018 8:30 PM, Derrick Stolee wrote:
Show 42 quoted lines
> On 7/9/2018 6:27 PM, Junio C Hamano wrote:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> 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/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/ 
>>>
>> 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 <dstolee@microsoft.com>"
>
> 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).
>

Of course, after I send this message I see that my inbox had poorly threaded this patch and item 6/6 was below these messages. Patch 6/6 does convert this variable and passes an arbitrary repository.

I'll update the MIDX patch in v4 to use that model for the config setting.
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 0/6] Object store refactoring: commit graph

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]:
Show 8 quoted 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/xmqqpnzt1myi.fsf@gitster-ct.c.googlers.com/
Jonathan Tan (6):
  commit-graph: refactor preparing commit graph
  object-store: add missing include
  commit-graph: add missing forward declaration
  commit-graph: add free_commit_graph
  commit-graph: store graph in struct object_store
  commit-graph: add repo arg to graph readers
 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
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 1/6] commit-graph: refactor preparing commit graph

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 <jonathantanmy@google.com>
---
 commit-graph.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to commit-graph.c +17 −11
diff --git a/commit-graph.c b/commit-graph.c
index 41a0133ff7..1ea701ed69 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -200,15 +200,25 @@ static void prepare_commit_graph_one(const char *obj_dir)
 }
 
 static int prepare_commit_graph_run_once = 0;
-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.
+ */
+static int prepare_commit_graph(void)
 {
 	struct alternate_object_database *alt;
 	char *obj_dir;
 
 	if (prepare_commit_graph_run_once)
-		return;
+		return !!commit_graph;
 	prepare_commit_graph_run_once = 1;
 
+	if (!core_commit_graph)
+		return 0;
+
 	obj_dir = get_object_directory();
 	prepare_commit_graph_one(obj_dir);
 	prepare_alt_odb(the_repository);
@@ -216,6 +226,7 @@ static void prepare_commit_graph(void)
 	     !commit_graph && alt;
 	     alt = alt->next)
 		prepare_commit_graph_one(alt->path);
+	return !!commit_graph;
 }
 
 static void close_commit_graph(void)
@@ -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);
 }
 
 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);
 }
 
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 2/6] object-store: add missing include

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)
Show changes to object-store.h +3 −0
diff --git a/object-store.h b/object-store.h
index a3db17bbf5..0e13543bab 100644
--- a/object-store.h
+++ b/object-store.h
@@ -2,6 +2,9 @@
 #define OBJECT_STORE_H
 
 #include "oidmap.h"
+#include "list.h"
+#include "sha1-array.h"
+#include "strbuf.h"
 
 struct alternate_object_database {
 	struct alternate_object_database *next;
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 3/6] commit-graph: add missing forward declaration

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to commit-graph.h +2 −0
diff --git a/commit-graph.h b/commit-graph.h
index 506cb45fb1..674052bef4 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -5,6 +5,8 @@
 #include "repository.h"
 #include "string-list.h"
 
+struct commit;
+
 char *get_commit_graph_filename(const char *obj_dir);
 
 /*
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 4/6] commit-graph: add free_commit_graph

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit-graph.c |  2 ++
 commit-graph.c         | 24 ++++++++++++++----------
 commit-graph.h         |  2 ++
 3 files changed, 18 insertions(+), 10 deletions(-)
Show changes to 3 files +18 −10

builtin/commit-graph.c, commit-graph.c, commit-graph.h

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c7d0db5ab4..0bf0c48657 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -115,6 +115,8 @@ static int graph_read(int argc, const char **argv)
 		printf(" large_edges");
 	printf("\n");
 
+	free_commit_graph(graph);
+
 	return 0;
 }
 
diff --git a/commit-graph.c b/commit-graph.c
index 1ea701ed69..143a587581 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -231,16 +231,8 @@ static int prepare_commit_graph(void)
 
 static void close_commit_graph(void)
 {
-	if (!commit_graph)
-		return;
-
-	if (commit_graph->graph_fd >= 0) {
-		munmap((void *)commit_graph->data, commit_graph->data_len);
-		commit_graph->data = NULL;
-		close(commit_graph->graph_fd);
-	}
-
-	FREE_AND_NULL(commit_graph);
+	free_commit_graph(commit_graph);
+	commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -1033,3 +1025,15 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)
 
 	return verify_commit_graph_error;
 }
+
+void free_commit_graph(struct commit_graph *g)
+{
+	if (!g)
+		return;
+	if (g->graph_fd >= 0) {
+		munmap((void *)g->data, g->data_len);
+		g->data = NULL;
+		close(g->graph_fd);
+	}
+	free(g);
+}
diff --git a/commit-graph.h b/commit-graph.h
index 674052bef4..94defb04a9 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -58,4 +58,6 @@ void write_commit_graph(const char *obj_dir,
 
 int verify_commit_graph(struct repository *r, struct commit_graph *g);
 
+void free_commit_graph(struct commit_graph *);
+
 #endif
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 5/6] commit-graph: store graph in struct object_store

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 <jonathantanmy@google.com>
---
 commit-graph.c | 40 +++++++++++++++++++---------------------
 object-store.h |  3 +++
 object.c       |  5 +++++
 3 files changed, 27 insertions(+), 21 deletions(-)
Show changes to 3 files +27 −21

commit-graph.c, object-store.h, object.c

diff --git a/commit-graph.c b/commit-graph.c
index 143a587581..b6a76a1413 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -183,24 +183,20 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)
 	exit(1);
 }
 
-/* global storage */
-static struct commit_graph *commit_graph = NULL;
-
 static void prepare_commit_graph_one(const char *obj_dir)
 {
 	char *graph_name;
 
-	if (commit_graph)
+	if (the_repository->objects->commit_graph)
 		return;
 
 	graph_name = get_commit_graph_filename(obj_dir);
-	commit_graph = load_commit_graph_one(graph_name);
+	the_repository->objects->commit_graph =
+		load_commit_graph_one(graph_name);
 
 	FREE_AND_NULL(graph_name);
 }
 
-static int prepare_commit_graph_run_once = 0;
-
 /*
  * Return 1 if commit_graph is non-NULL, and 0 otherwise.
  *
@@ -212,9 +208,9 @@ static int prepare_commit_graph(void)
 	struct alternate_object_database *alt;
 	char *obj_dir;
 
-	if (prepare_commit_graph_run_once)
-		return !!commit_graph;
-	prepare_commit_graph_run_once = 1;
+	if (the_repository->objects->commit_graph_attempted)
+		return !!the_repository->objects->commit_graph;
+	the_repository->objects->commit_graph_attempted = 1;
 
 	if (!core_commit_graph)
 		return 0;
@@ -223,16 +219,16 @@ static int prepare_commit_graph(void)
 	prepare_commit_graph_one(obj_dir);
 	prepare_alt_odb(the_repository);
 	for (alt = the_repository->objects->alt_odb_list;
-	     !commit_graph && alt;
+	     !the_repository->objects->commit_graph && alt;
 	     alt = alt->next)
 		prepare_commit_graph_one(alt->path);
-	return !!commit_graph;
+	return !!the_repository->objects->commit_graph;
 }
 
 static void close_commit_graph(void)
 {
-	free_commit_graph(commit_graph);
-	commit_graph = NULL;
+	free_commit_graph(the_repository->objects->commit_graph);
+	the_repository->objects->commit_graph = NULL;
 }
 
 static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)
@@ -342,7 +338,7 @@ int parse_commit_in_graph(struct commit *item)
 {
 	if (!prepare_commit_graph())
 		return 0;
-	return parse_commit_in_graph_one(commit_graph, item);
+	return parse_commit_in_graph_one(the_repository->objects->commit_graph, item);
 }
 
 void load_commit_graph_info(struct commit *item)
@@ -350,8 +346,8 @@ void load_commit_graph_info(struct commit *item)
 	uint32_t pos;
 	if (!prepare_commit_graph())
 		return;
-	if (find_commit_in_graph(item, commit_graph, &pos))
-		fill_commit_graph_info(item, commit_graph, pos);
+	if (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))
+		fill_commit_graph_info(item, the_repository->objects->commit_graph, pos);
 }
 
 static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)
@@ -379,7 +375,7 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
 
 struct tree *get_commit_tree_in_graph(const struct commit *c)
 {
-	return get_commit_tree_in_graph_one(commit_graph, c);
+	return get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);
 }
 
 static void write_graph_chunk_fanout(struct hashfile *f,
@@ -696,15 +692,17 @@ void write_commit_graph(const char *obj_dir,
 
 	if (append) {
 		prepare_commit_graph_one(obj_dir);
-		if (commit_graph)
-			oids.alloc += commit_graph->num_commits;
+		if (the_repository->objects->commit_graph)
+			oids.alloc += the_repository->objects->commit_graph->num_commits;
 	}
 
 	if (oids.alloc < 1024)
 		oids.alloc = 1024;
 	ALLOC_ARRAY(oids.list, oids.alloc);
 
-	if (append && commit_graph) {
+	if (append && the_repository->objects->commit_graph) {
+		struct commit_graph *commit_graph =
+			the_repository->objects->commit_graph;
 		for (i = 0; i < commit_graph->num_commits; i++) {
 			const unsigned char *hash = commit_graph->chunk_oid_lookup +
 				commit_graph->hash_len * i;
diff --git a/object-store.h b/object-store.h
index 0e13543bab..e481f7ad41 100644
--- a/object-store.h
+++ b/object-store.h
@@ -106,6 +106,9 @@ struct raw_object_store {
 	 */
 	struct oidmap *replace_map;
 
+	struct commit_graph *commit_graph;
+	unsigned commit_graph_attempted : 1; /* if loading has been attempted */
+
 	/*
 	 * private data
 	 *
diff --git a/object.c b/object.c
index b0faab85d4..e2c112cc1a 100644
--- a/object.c
+++ b/object.c
@@ -9,6 +9,7 @@
 #include "alloc.h"
 #include "object-store.h"
 #include "packfile.h"
+#include "commit-graph.h"
 
 unsigned int get_max_object_index(void)
 {
@@ -507,6 +508,10 @@ void raw_object_store_clear(struct raw_object_store *o)
 	oidmap_free(o->replace_map, 1);
 	FREE_AND_NULL(o->replace_map);
 
+	free_commit_graph(o->commit_graph);
+	o->commit_graph = NULL;
+	o->commit_graph_attempted = 0;
+
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 
-- 
2.18.0.203.gfac676dfb9-goog
Jonathan Tan· Jul 11, 2018, 22:42 UTC · re: Jonathan Tan · lore

[PATCH v3 6/6] commit-graph: add repo arg to graph readers

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 <jonathantanmy@google.com>
---
 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 | 82 ++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c       |  1 +
 t/helper/test-tool.h       |  1 +
 t/t5318-commit-graph.sh    | 35 ++++++++++++++++
 13 files changed, 162 insertions(+), 42 deletions(-)
 create mode 100644 t/helper/test-repository.c
Show changes to 13 files +162 −42

Makefile, builtin/fsck.c, cache.h, commit-graph.c, commit-graph.h, commit.c, config.c, environment.c, ref-filter.c, t/helper/test-repository.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t5318-commit-graph.sh

diff --git a/Makefile b/Makefile
index 0cb6590f24..bb8bd67201 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ea5e2a03e6..c96f3f4fcc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -830,7 +830,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 };
 
diff --git a/cache.h b/cache.h
index 8b447652a7..3311f4c9e2 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/commit-graph.c b/commit-graph.c
index b6a76a1413..b0a55ad128 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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;
 
-	if (!core_commit_graph)
+	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value)
+		/*
+		 * 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.)
+		 */
 		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;
 	}
diff --git a/commit-graph.h b/commit-graph.h
index 94defb04a9..76e098934a 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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;
diff --git a/commit.c b/commit.c
index c0a83d2644..39b80bd21d 100644
--- a/commit.c
+++ b/commit.c
@@ -342,7 +342,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)
@@ -438,7 +438,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	item->date = parse_commit_date(bufptr, tail);
 
 	if (check_graph)
-		load_commit_graph_info(item);
+		load_commit_graph_info(the_repository, item);
 
 	return 0;
 }
@@ -454,7 +454,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)
diff --git a/config.c b/config.c
index 139c903f6b..3aacddfec4 100644
--- a/config.c
+++ b/config.c
@@ -1309,11 +1309,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;
diff --git a/environment.c b/environment.c
index 013e845235..6cf0079389 100644
--- a/environment.c
+++ b/environment.c
@@ -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() */
diff --git a/ref-filter.c b/ref-filter.c
index 49021ee446..9b2da88392 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1713,7 +1713,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
 
 	for (p = want; p; p = p->next) {
 		struct commit *c = p->item;
-		load_commit_graph_info(c);
+		load_commit_graph_info(the_repository, c);
 		if (c->generation < cutoff)
 			cutoff = c->generation;
 	}
diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c
new file mode 100644
index 0000000000..2762ca6562
--- /dev/null
+++ b/t/helper/test-repository.c
@@ -0,0 +1,82 @@
+#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;
+
+	repo_init(&r, gitdir, worktree);
+
+	c = lookup_commit(&r, commit_oid);
+
+	if (!parse_commit_in_graph(&r, c))
+		die("Couldn't parse commit");
+
+	printf("%"PRItime, 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,
+					  const struct object_id *commit_oid)
+{
+	struct repository r;
+	struct commit *c;
+	struct tree *tree;
+
+	repo_init(&r, gitdir, worktree);
+
+	c = lookup_commit(&r, commit_oid);
+
+	/*
+	 * 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");
+
+	printf("%s\n", oid_to_hex(&tree->object.oid));
+
+	repo_clear(&r);
+}
+
+int cmd__repository(int argc, const char **argv)
+{
+	if (argc < 2)
+		die("must have at least 2 arguments");
+	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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..dafc91c240 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..80cbcf0857 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 5947de3d24..4f17d7701e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -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
-- 
2.18.0.203.gfac676dfb9-goog
SZEDER Gábor· Aug 13, 2018, 00:30 UTC · re: Jonathan Tan · lore

[PATCH] t5318: avoid unnecessary command substitutions

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 <szeder.dev@gmail.com>
---
 t/t5318-commit-graph.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
Show changes to t/t5318-commit-graph.sh +7 −5
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 1c148ebf21..3c1ffad491 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -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 &&
+	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 &&
+	git -C repo rev-parse one^{tree} >expect &&
 	test_cmp expect actual
 '
 
-- 
2.18.0.408.g42635c01bc
Derrick Stolee· Aug 13, 2018, 11:23 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t5318: avoid unnecessary command substitutions

On 8/12/2018 8:30 PM, SZEDER Gábor wrote:
Show 48 quoted lines
> 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 <szeder.dev@gmail.com>
> ---
>   t/t5318-commit-graph.sh | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 1c148ebf21..3c1ffad491 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -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 &&
> +	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 &&
> +	git -C repo rev-parse one^{tree} >expect &&
>   	test_cmp expect actual
>   '
>   

These make sense and are good examples for future test patterns. Thanks, Szeder.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Junio C Hamano· Aug 13, 2018, 19:05 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t5318: avoid unnecessary command substitutions

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 6 quoted lines
> -	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 &&

Heh, "format:"%ct " to make the first one end with an incomplete line? Neat trick.

Junio C Hamano· Jul 12, 2018, 16:56 UTC · re: Jonathan Tan · lore

Re: [PATCH v3 0/6] Object store refactoring: commit graph

Jonathan Tan <jonathantanmy@google.com> writes:
Show 5 quoted lines
> 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.)

OK. I am perfectly OK if this left lookup_commit() with older function signature and instead asked the integrator to fix up with evil merge with sb/object-store-lookup, but given that this topic is no more urgent than the other one, making them intertwined is fine.

Merging ds/commit-graph-fsck into sb/object-store-lookup already requires an evil merge to build correctly due to this new parameter. I really wish we did the more canonical "do not change signature of a widely used function. If the widely used one is a mere specialization that calls the new one with the default parameter, make that widely used one a thin wraper (or a macro) of the new one" approach to avoid having to repeatedly create such pointless evil merges X-<.

Show 18 quoted lines
>  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
Derrick Stolee· Jul 12, 2018, 17:43 UTC · re: Jonathan Tan · lore

Re: [PATCH v3 0/6] Object store refactoring: commit graph

On 7/11/2018 6:42 PM, Jonathan Tan wrote:
Show 32 quoted lines
> 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]:
>
>> 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/xmqqpnzt1myi.fsf@gitster-ct.c.googlers.com/

I re-read the patch series and think you addressed all feedback. I have no more to add.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

← back to recent threads