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

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

[Jonathan Tan]

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

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)

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

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)

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

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

[Stefan Beller]

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)

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

[Jonathan Tan]

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

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

[Derrick Stolee]

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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

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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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);
}

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

[Jonathan Tan]

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

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

[Junio C Hamano]

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.

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

[Jonathan Tan]

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

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

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

[Stefan Beller]

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

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

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

[Jonathan Tan]

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

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

[Derrick Stolee]

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/

>> This is really nice!
>>
>> Overall this series looks good to me,
>> Stefan
> Thanks - let's see what others think.

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

[Derrick Stolee]

On 6/21/2018 8:33 PM, Derrick Stolee wrote:
> 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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

This is on ds/commit-graph-fsck.

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

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

[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

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

[Jonathan Tan]

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

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

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

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

[Stefan Beller]

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>

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

[Derrick Stolee]

On 7/9/2018 5:41 PM, Stefan Beller wrote:
> 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!

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)

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

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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.)
+		 */
 		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

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

[Derrick Stolee]

On 7/9/2018 4:44 PM, Jonathan Tan wrote:
> 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.

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


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

> +
> +	/*
> +	 * 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.

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

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

[Jonathan Tan]

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

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

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

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

[Derrick Stolee]

On 7/10/2018 1:31 PM, Jonathan Tan wrote:
>
>>> +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

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

[Jonathan Tan]

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

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

[SZEDER Gábor]

> 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

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Hi,

On Tue, 10 Jul 2018, SZEDER Gábor wrote:

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

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

[Jonathan Tan]

On Tue, Jul 10, 2018 at 6:18 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> 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.

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

[Stefan Beller]

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

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

[Junio C Hamano]

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?

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

[Derrick Stolee]

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

Thanks,

-Stolee

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

[Derrick Stolee]

On 7/9/2018 8:30 PM, Derrick Stolee wrote:
> 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.

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

[Jonathan Tan]

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/

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

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

[Jonathan Tan]

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

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

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

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 object-store.h | 3 +++
 1 file changed, 3 insertions(+)

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

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

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 commit-graph.h | 2 ++
 1 file changed, 2 insertions(+)

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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

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

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

[Jonathan Tan]

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

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

[PATCH] t5318: avoid unnecessary command substitutions

[SZEDER Gábor]

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
 '
 
-- 
2.18.0.408.g42635c01bc

Re: [PATCH] t5318: avoid unnecessary command substitutions

[Derrick Stolee]

On 8/12/2018 8:30 PM, SZEDER Gábor wrote:
> 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>

Re: [PATCH] t5318: avoid unnecessary command substitutions

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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


>  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

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

[Derrick Stolee]

On 7/11/2018 6:42 PM, Jonathan Tan wrote:
> 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>