{"thread":{"id":"48756","subject":"[PATCH 0/5] Object store refactoring: commit graph","startedAt":"2018-06-21T21:29:34Z","lastAt":"2018-08-13T19:05:29Z","messageCount":49,"participants":["Jonathan Tan","Stefan Beller","Derrick Stolee","Junio C Hamano","SZEDER Gábor","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"350683","messageId":"cover.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":null,"subject":"[PATCH 0/5] Object store refactoring: commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:20Z","receivedAt":"2018-06-21T21:29:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This continues the object store refactoring effort by making commit\ngraphs of any repository be readable, not just the commit graph of\nthe_repository.\n\nThis enables the conversion of other functions in the future, functions\nlike parse_commit (which first tries to parse from the commit graph).\n\nI didn't modify the function that writes commit graphs - perhaps that\ncan be done in a subsequent series.\n\nJonathan Tan (5):\n object-store: add missing include\n commit-graph: add missing forward declaration\n commit-graph: add free_commit_graph\n commit-graph: store graph in struct object_store\n commit-graph: add repo arg to graph readers\n\n Makefile | 1 +\n builtin/commit-graph.c | 2 +\n cache.h | 1 -\n commit-graph.c | 90 ++++++++++++++++++++++----------------\n commit-graph.h | 9 +++-\n commit.c | 4 +-\n config.c | 5 ---\n environment.c | 1 -\n object-store.h | 6 +++\n object.c | 5 +++\n t/helper/test-repository.c | 88 +++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 +++++++++++++++\n 14 files changed, 201 insertions(+), 48 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350684","messageId":"d8daccadb7eeb90f024b027a8a0c5ea86a7c179b.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH 1/5] object-store: add missing include","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:21Z","receivedAt":"2018-06-21T21:29:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n object-store.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/object-store.h b/object-store.h\nindex d683112fd7..f0b77146e4 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -2,6 +2,9 @@\n #define OBJECT_STORE_H\n \n #include \"oidmap.h\"\n+#include \"list.h\"\n+#include \"sha1-array.h\"\n+#include \"strbuf.h\"\n \n struct alternate_object_database {\n \tstruct alternate_object_database *next;\n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350685","messageId":"82c9edb9108f49468031bfd7c65a9eb8db2cb671.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH 2/5] commit-graph: add missing forward declaration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:22Z","receivedAt":"2018-06-21T21:29:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n commit-graph.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 260a468e73..7004dfdca9 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -3,6 +3,8 @@\n \n #include \"git-compat-util.h\"\n \n+struct commit;\n+\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350686","messageId":"10e98b79ecdc7aa8d6abc2d6870fca2692d1956f.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH 3/5] commit-graph: add free_commit_graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:23Z","receivedAt":"2018-06-21T21:29:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/commit-graph.c | 2 ++\n commit-graph.c | 24 ++++++++++++++----------\n commit-graph.h | 2 ++\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 37420ae0fd..9c2d55221c 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -74,6 +74,8 @@ static int graph_read(int argc, const char **argv)\n \t\tprintf(\" large_edges\");\n \tprintf(\"\\n\");\n \n+\tfree_commit_graph(graph);\n+\n \treturn 0;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 4c6127088f..9f4e761229 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -216,16 +216,8 @@ static void prepare_commit_graph(void)\n \n static void close_commit_graph(void)\n {\n-\tif (!commit_graph)\n-\t\treturn;\n-\n-\tif (commit_graph->graph_fd >= 0) {\n-\t\tmunmap((void *)commit_graph->data, commit_graph->data_len);\n-\t\tcommit_graph->data = NULL;\n-\t\tclose(commit_graph->graph_fd);\n-\t}\n-\n-\tFREE_AND_NULL(commit_graph);\n+\tfree_commit_graph(commit_graph);\n+\tcommit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -759,3 +751,15 @@ void write_commit_graph(const char *obj_dir,\n \toids.alloc = 0;\n \toids.nr = 0;\n }\n+\n+void free_commit_graph(struct commit_graph *g)\n+{\n+\tif (!g)\n+\t\treturn;\n+\tif (g->graph_fd >= 0) {\n+\t\tmunmap((void *)g->data, g->data_len);\n+\t\tg->data = NULL;\n+\t\tclose(g->graph_fd);\n+\t}\n+\tfree(g);\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 7004dfdca9..320ee9fd8a 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -47,4 +47,6 @@ void write_commit_graph(const char *obj_dir,\n \t\t\tint nr_commits,\n \t\t\tint append);\n \n+void free_commit_graph(struct commit_graph *);\n+\n #endif\n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350687","messageId":"3410494e3505a7b1b895869c3702dbba0f0cf5a6.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH 4/5] commit-graph: store graph in struct object_store","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:24Z","receivedAt":"2018-06-21T21:29:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of storing commit graphs in static variables, store them in\nstruct object_store. There are no changes to the signatures of existing\nfunctions - they all still only support the_repository, and support for\nother instances of struct repository will be added in a subsequent\ncommit.\n\nSigned-off-by: Jonathan Tan \n---\n commit-graph.c | 34 ++++++++++++++++++----------------\n object-store.h | 3 +++\n object.c | 5 +++++\n 3 files changed, 26 insertions(+), 16 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9f4e761229..61b4fbb925 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -179,45 +179,42 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-/* global storage */\n-static struct commit_graph *commit_graph = NULL;\n-\n static void prepare_commit_graph_one(const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (commit_graph)\n+\tif (the_repository->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tcommit_graph = load_commit_graph_one(graph_name);\n+\tthe_repository->objects->commit_graph =\n+\t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n }\n \n-static int prepare_commit_graph_run_once = 0;\n static void prepare_commit_graph(void)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n \n-\tif (prepare_commit_graph_run_once)\n+\tif (the_repository->objects->commit_graph_attempted)\n \t\treturn;\n-\tprepare_commit_graph_run_once = 1;\n+\tthe_repository->objects->commit_graph_attempted = 1;\n \n \tobj_dir = get_object_directory();\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb(the_repository);\n \tfor (alt = the_repository->objects->alt_odb_list;\n-\t !commit_graph && alt;\n+\t !the_repository->objects->commit_graph && alt;\n \t alt = alt->next)\n \t\tprepare_commit_graph_one(alt->path);\n }\n \n static void close_commit_graph(void)\n {\n-\tfree_commit_graph(commit_graph);\n-\tcommit_graph = NULL;\n+\tfree_commit_graph(the_repository->objects->commit_graph);\n+\tthe_repository->objects->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -293,7 +290,9 @@ int parse_commit_in_graph(struct commit *item)\n \t\treturn 1;\n \n \tprepare_commit_graph();\n-\tif (commit_graph) {\n+\tif (the_repository->objects->commit_graph) {\n+\t\tstruct commit_graph *commit_graph =\n+\t\t\tthe_repository->objects->commit_graph;\n \t\tuint32_t pos;\n \t\tint found;\n \t\tif (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n@@ -329,7 +328,8 @@ struct tree *get_commit_tree_in_graph(const struct commit *c)\n \tif (c->graph_pos == COMMIT_NOT_FROM_GRAPH)\n \t\tBUG(\"get_commit_tree_in_graph called from non-commit-graph commit\");\n \n-\treturn load_tree_for_commit(commit_graph, (struct commit *)c);\n+\treturn load_tree_for_commit(the_repository->objects->commit_graph,\n+\t\t\t\t (struct commit *)c);\n }\n \n static void write_graph_chunk_fanout(struct hashfile *f,\n@@ -589,15 +589,17 @@ void write_commit_graph(const char *obj_dir,\n \n \tif (append) {\n \t\tprepare_commit_graph_one(obj_dir);\n-\t\tif (commit_graph)\n-\t\t\toids.alloc += commit_graph->num_commits;\n+\t\tif (the_repository->objects->commit_graph)\n+\t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\n \n \tif (oids.alloc < 1024)\n \t\toids.alloc = 1024;\n \tALLOC_ARRAY(oids.list, oids.alloc);\n \n-\tif (append && commit_graph) {\n+\tif (append && the_repository->objects->commit_graph) {\n+\t\tstruct commit_graph *commit_graph =\n+\t\t\tthe_repository->objects->commit_graph;\n \t\tfor (i = 0; i < commit_graph->num_commits; i++) {\n \t\t\tconst unsigned char *hash = commit_graph->chunk_oid_lookup +\n \t\t\t\tcommit_graph->hash_len * i;\ndiff --git a/object-store.h b/object-store.h\nindex f0b77146e4..39f4a3d5b9 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -106,6 +106,9 @@ struct raw_object_store {\n \t */\n \tstruct oidmap *replace_map;\n \n+\tstruct commit_graph *commit_graph;\n+\tunsigned commit_graph_attempted : 1; /* if loading has been attempted */\n+\n \t/*\n \t * private data\n \t *\ndiff --git a/object.c b/object.c\nindex f7f4de3aaf..04954f4f3e 100644\n--- a/object.c\n+++ b/object.c\n@@ -7,6 +7,7 @@\n #include \"tag.h\"\n #include \"object-store.h\"\n #include \"packfile.h\"\n+#include \"commit-graph.h\"\n \n static struct object **obj_hash;\n static int nr_objs, obj_hash_size;\n@@ -484,6 +485,10 @@ void raw_object_store_clear(struct raw_object_store *o)\n \toidmap_free(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n \n+\tfree_commit_graph(o->commit_graph);\n+\to->commit_graph = NULL;\n+\to->commit_graph_attempted = 0;\n+\n \tfree_alt_odbs(o);\n \to->alt_odb_tail = NULL;\n \n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350688","messageId":"da1a513f2508602e0736b6fbe142b2445948bd5c.1529616356.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T21:29:25Z","receivedAt":"2018-06-21T21:29:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a struct repository argument to the functions in commit-graph.h that\nread the commit graph. (This commit does not affect functions that write\ncommit graphs.)\n\nBecause the commit graph functions can now read the commit graph of any\nrepository, the global variable core_commit_graph has been removed.\nInstead, the config option core.commitGraph is now read on the first\ntime in a repository that a commit is attempted to be parsed using its\ncommit graph.\n\nThis commit includes a test that exercises the functionality on an\narbitrary repository that is not the_repository.\n\nSigned-off-by: Jonathan Tan \n---\n Makefile | 1 +\n cache.h | 1 -\n commit-graph.c | 52 +++++++++++++---------\n commit-graph.h | 5 ++-\n commit.c | 4 +-\n config.c | 5 ---\n environment.c | 1 -\n t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 +++++++++++++++\n 11 files changed, 162 insertions(+), 32 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\ndiff --git a/Makefile b/Makefile\nindex e4b503d259..12d754cb70 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-ref-store.o\n TEST_BUILTINS_OBJS += test-regex.o\n+TEST_BUILTINS_OBJS += test-repository.o\n TEST_BUILTINS_OBJS += test-revision-walking.o\n TEST_BUILTINS_OBJS += test-run-command.o\n TEST_BUILTINS_OBJS += test-scrap-cache-tree.o\ndiff --git a/cache.h b/cache.h\nindex 89a107a7f7..ebf699fb0f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -813,7 +813,6 @@ extern char *git_replace_ref_base;\n \n extern int fsync_object_files;\n extern int core_preload_index;\n-extern int core_commit_graph;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 61b4fbb925..12c4addf75 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -179,36 +179,47 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-static void prepare_commit_graph_one(const char *obj_dir)\n+static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (the_repository->objects->commit_graph)\n+\tif (r->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tthe_repository->objects->commit_graph =\n+\tr->objects->commit_graph =\n \t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n }\n \n-static void prepare_commit_graph(void)\n+static void prepare_commit_graph(struct repository *r)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n+\tint config_value;\n \n-\tif (the_repository->objects->commit_graph_attempted)\n+\tif (r->objects->commit_graph_attempted)\n+\t\treturn;\n+\tr->objects->commit_graph_attempted = 1;\n+\n+\tif (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n+\t !config_value)\n+\t\t/*\n+\t\t * This repository is not configured to use commit graphs, so\n+\t\t * do not load one. (But report commit_graph_attempted anyway\n+\t\t * so that commit graph loading is not attempted again for this\n+\t\t * repository.)\n+\t\t */\n \t\treturn;\n-\tthe_repository->objects->commit_graph_attempted = 1;\n \n-\tobj_dir = get_object_directory();\n-\tprepare_commit_graph_one(obj_dir);\n-\tprepare_alt_odb(the_repository);\n-\tfor (alt = the_repository->objects->alt_odb_list;\n-\t !the_repository->objects->commit_graph && alt;\n+\tobj_dir = r->objects->objectdir;\n+\tprepare_commit_graph_one(r, obj_dir);\n+\tprepare_alt_odb(r);\n+\tfor (alt = r->objects->alt_odb_list;\n+\t !r->objects->commit_graph && alt;\n \t alt = alt->next)\n-\t\tprepare_commit_graph_one(alt->path);\n+\t\tprepare_commit_graph_one(r, alt->path);\n }\n \n static void close_commit_graph(void)\n@@ -282,17 +293,15 @@ static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n \treturn 1;\n }\n \n-int parse_commit_in_graph(struct commit *item)\n+int parse_commit_in_graph(struct repository *r, struct commit *item)\n {\n-\tif (!core_commit_graph)\n-\t\treturn 0;\n \tif (item->object.parsed)\n \t\treturn 1;\n \n-\tprepare_commit_graph();\n-\tif (the_repository->objects->commit_graph) {\n+\tprepare_commit_graph(r);\n+\tif (r->objects->commit_graph) {\n \t\tstruct commit_graph *commit_graph =\n-\t\t\tthe_repository->objects->commit_graph;\n+\t\t\tr->objects->commit_graph;\n \t\tuint32_t pos;\n \t\tint found;\n \t\tif (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n@@ -321,14 +330,15 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *\n \treturn c->maybe_tree;\n }\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c)\n+struct tree *get_commit_tree_in_graph(struct repository *r,\n+\t\t\t\t const struct commit *c)\n {\n \tif (c->maybe_tree)\n \t\treturn c->maybe_tree;\n \tif (c->graph_pos == COMMIT_NOT_FROM_GRAPH)\n \t\tBUG(\"get_commit_tree_in_graph called from non-commit-graph commit\");\n \n-\treturn load_tree_for_commit(the_repository->objects->commit_graph,\n+\treturn load_tree_for_commit(r->objects->commit_graph,\n \t\t\t\t (struct commit *)c);\n }\n \n@@ -588,7 +598,7 @@ void write_commit_graph(const char *obj_dir,\n \toids.alloc = approximate_object_count() / 4;\n \n \tif (append) {\n-\t\tprepare_commit_graph_one(obj_dir);\n+\t\tprepare_commit_graph_one(the_repository, obj_dir);\n \t\tif (the_repository->objects->commit_graph)\n \t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 320ee9fd8a..192ea2ca77 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -17,9 +17,10 @@ char *get_commit_graph_filename(const char *obj_dir);\n *\n * See parse_commit_buffer() for the fallback after this call.\n */\n-int parse_commit_in_graph(struct commit *item);\n+int parse_commit_in_graph(struct repository *r, struct commit *item);\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c);\n+struct tree *get_commit_tree_in_graph(struct repository *r,\n+\t\t\t\t const struct commit *c);\n \n struct commit_graph {\n \tint graph_fd;\ndiff --git a/commit.c b/commit.c\nindex 0030e79940..38c12b002f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n \tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n \t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n \n-\treturn get_commit_tree_in_graph(commit);\n+\treturn get_commit_tree_in_graph(the_repository, commit);\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n@@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (parse_commit_in_graph(item))\n+\tif (parse_commit_in_graph(the_repository, item))\n \t\treturn 0;\n \tbuffer = read_object_file(&item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/config.c b/config.c\nindex fbbf0f8e9f..072e9bdb4d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1308,11 +1308,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.commitgraph\")) {\n-\t\tcore_commit_graph = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"core.sparsecheckout\")) {\n \t\tcore_apply_sparse_checkout = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 2a6de2330b..d6e5b39425 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n int grafts_replace_parents = 1;\n-int core_commit_graph;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nnew file mode 100644\nindex 0000000000..5fff540a26\n--- /dev/null\n+++ b/t/helper/test-repository.c\n@@ -0,0 +1,88 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"commit-graph.h\"\n+#include \"commit.h\"\n+#include \"config.h\"\n+#include \"object-store.h\"\n+#include \"object.h\"\n+#include \"repository.h\"\n+#include \"tree.h\"\n+\n+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n+\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct commit_list *parent;\n+\n+\t/*\n+\t * Create a commit independent of any repository.\n+\t */\n+\tc = lookup_commit(commit_oid);\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\n+\tprintf(\"%lu\", c->date);\n+\tfor (parent = c->parents; parent; parent = parent->next)\n+\t\tprintf(\" %s\", oid_to_hex(&parent->item->object.oid));\n+\tprintf(\"\\n\");\n+\n+\trepo_clear(&r);\n+}\n+\n+static void test_get_commit_tree_in_graph(const char *gitdir,\n+\t\t\t\t\t const char *worktree,\n+\t\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct tree *tree;\n+\n+\t/*\n+\t * Create a commit independent of any repository.\n+\t */\n+\tc = lookup_commit(commit_oid);\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\t/*\n+\t * get_commit_tree_in_graph does not automatically parse the commit, so\n+\t * parse it first.\n+\t */\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\ttree = get_commit_tree_in_graph(&r, c);\n+\tif (!tree)\n+\t\tdie(\"Couldn't get commit tree\");\n+\n+\tprintf(\"%s\\n\", oid_to_hex(&tree->object.oid));\n+\n+\trepo_clear(&r);\n+}\n+\n+int cmd__repository(int argc, const char **argv)\n+{\n+\tif (argc < 2)\n+\t\tdie(\"must have at least 2 arguments\");\n+\tif (!strcmp(argv[1], \"parse_commit_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_parse_commit_in_graph(argv[2], argv[3], &oid);\n+\t} else if (!strcmp(argv[1], \"get_commit_tree_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_get_commit_tree_in_graph(argv[2], argv[3], &oid);\n+\t} else {\n+\t\tdie(\"unrecognized '%s'\", argv[1]);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 805a45de9c..dafc91c240 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"ref-store\", cmd__ref_store },\n \t{ \"regex\", cmd__regex },\n+\t{ \"repository\", cmd__repository },\n \t{ \"revision-walking\", cmd__revision_walking },\n \t{ \"run-command\", cmd__run_command },\n \t{ \"scrap-cache-tree\", cmd__scrap_cache_tree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb94..80cbcf0857 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -23,6 +23,7 @@ int cmd__prio_queue(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\n int cmd__regex(int argc, const char **argv);\n+int cmd__repository(int argc, const char **argv);\n int cmd__revision_walking(int argc, const char **argv);\n int cmd__run_command(int argc, const char **argv);\n int cmd__scrap_cache_tree(int argc, const char **argv);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex a380419b65..f80bbaec7f 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -221,4 +221,39 @@ test_expect_success 'write graph in bare repo' '\n graph_git_behavior 'bare repo with graph, commit 8 vs merge 1' bare commits/8 merge/1\n graph_git_behavior 'bare repo with graph, commit 8 vs merge 2' bare commits/8 merge/2\n \n+test_expect_success 'setup non-the_repository tests' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\ttest_commit -C repo one &&\n+\ttest_commit -C repo two &&\n+\tgit -C repo config core.commitGraph true &&\n+\tgit -C repo rev-parse two | \\\n+\t\tgit -C repo commit-graph write --stdin-commits\n+'\n+\n+test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n+\t\t$(git -C repo rev-parse one) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo rev-parse two^{tree}) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo rev-parse one^{tree}) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.18.0.rc2.347.g0da03f3a46.dirty\n\n"},{"id":"350690","messageId":"CAGZ79kbC0e1ujAebTUexDXH4+cFOpek3a0VTrdRUfx6Z=cpBVw@mail.gmail.com","threadId":"48756","inReplyTo":"82c9edb9108f49468031bfd7c65a9eb8db2cb671.1529616356.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/5] commit-graph: add missing forward declaration","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-21T21:43:52Z","receivedAt":"2018-06-21T21:44:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jonathan,\nOn Thu, Jun 21, 2018 at 2:29 PM Jonathan Tan wrote:\n>\n> Signed-off-by: Jonathan Tan \n\nBoth this and the previous patch look good to me;\nyou seem to have better (stricter) checking for\nmissing includes/forward declarations, am I missing\na compile option? (I have DEVELOPER=1 in config.mak)\n"},{"id":"350694","messageId":"20180621223942.81451-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"CAGZ79kbC0e1ujAebTUexDXH4+cFOpek3a0VTrdRUfx6Z=cpBVw@mail.gmail.com","subject":"Re: [PATCH 2/5] commit-graph: add missing forward declaration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T22:39:42Z","receivedAt":"2018-06-21T22:39:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Both this and the previous patch look good to me;\n> you seem to have better (stricter) checking for\n> missing includes/forward declarations, am I missing\n> a compile option? (I have DEVELOPER=1 in config.mak)\n\nThanks. No I don't - I discovered that these were missing as I was\nincluding these header files in other files.\n"},{"id":"350695","messageId":"CAGZ79kYYD48PQiSqjqHkrqX9stKbGmEAHc1ArsuUgSHhwv5=Vw@mail.gmail.com","threadId":"48756","inReplyTo":"da1a513f2508602e0736b6fbe142b2445948bd5c.1529616356.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-21T22:41:49Z","receivedAt":"2018-06-21T22:42:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> diff --git a/commit.c b/commit.c\n> index 0030e79940..38c12b002f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n> if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n> BUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n>\n> - return get_commit_tree_in_graph(commit);\n> + return get_commit_tree_in_graph(the_repository, commit);\n\nHere..\n\n> }\n>\n> struct object_id *get_commit_tree_oid(const struct commit *commit)\n> @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)\n> return -1;\n> if (item->object.parsed)\n> return 0;\n> - if (parse_commit_in_graph(item))\n> + if (parse_commit_in_graph(the_repository, item))\n\nand here\n\n> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> + const struct object_id *commit_oid)\n> +{\n> + struct repository r;\n> + struct commit *c;\n> + struct commit_list *parent;\n> +\n> + /*\n> + * Create a commit independent of any repository.\n> + */\n> + c = lookup_commit(commit_oid);\n\n.. and this one are unfortunate as the rest of the object store series\nhas not progressed as far as needed.\n\nThe lookup_commit series is out there already, and that will\nteach lookup_commit a repository argument. When rerolling\nthat series I need to switch the order of repo_init and lookup_commit\nsuch that we can pass the repo to the lookup.\n\n> +test_expect_success 'setup non-the_repository tests' '\n\n> +test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n\n> +test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n\nThis is really nice!\n\nOverall this series looks good to me,\nStefan\n"},{"id":"350696","messageId":"20180621230608.86117-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"CAGZ79kYYD48PQiSqjqHkrqX9stKbGmEAHc1ArsuUgSHhwv5=Vw@mail.gmail.com","subject":"Re: [PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-21T23:06:08Z","receivedAt":"2018-06-21T23:06:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > diff --git a/commit.c b/commit.c\n> > index 0030e79940..38c12b002f 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n> > if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n> > BUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n> >\n> > - return get_commit_tree_in_graph(commit);\n> > + return get_commit_tree_in_graph(the_repository, commit);\n> \n> Here..\n> \n> > }\n> >\n> > struct object_id *get_commit_tree_oid(const struct commit *commit)\n> > @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)\n> > return -1;\n> > if (item->object.parsed)\n> > return 0;\n> > - if (parse_commit_in_graph(item))\n> > + if (parse_commit_in_graph(the_repository, item))\n> \n> and here\n> \n> > +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> > + const struct object_id *commit_oid)\n> > +{\n> > + struct repository r;\n> > + struct commit *c;\n> > + struct commit_list *parent;\n> > +\n> > + /*\n> > + * Create a commit independent of any repository.\n> > + */\n> > + c = lookup_commit(commit_oid);\n> \n> .. and this one are unfortunate as the rest of the object store series\n> has not progressed as far as needed.\n\nI think the first 2 are in reverse - get_commit_tree depends on\nget_commit_tree_in_graph and parse_commit_gently depends on\nparse_commit_in_graph, so we need the commit-graph functions to be\nchanged first. But I agree about lookup_commit.\n\n> The lookup_commit series is out there already, and that will\n> teach lookup_commit a repository argument. When rerolling\n> that series I need to switch the order of repo_init and lookup_commit\n> such that we can pass the repo to the lookup.\n\nFor future reference, Stefan is talking about this series:\nhttps://public-inbox.org/git/20180613230522.55335-1-sbeller@google.com/\n\nLet me know if you want to reroll yours on top of mine, or vice versa. I\nthink it's clearer if mine goes in first, though, since (as you said in\nthat e-mail) parse_commit depends on this change in the commit graph.\n\n> This is really nice!\n> \n> Overall this series looks good to me,\n> Stefan\n\nThanks - let's see what others think.\n"},{"id":"350697","messageId":"316b6a43-a5d0-d10f-f995-94b3165ac325@gmail.com","threadId":"48756","inReplyTo":"20180621223942.81451-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/5] commit-graph: add missing forward declaration","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-06-22T00:17:37Z","receivedAt":"2018-06-22T00:17:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/21/2018 6:39 PM, Jonathan Tan wrote:\n>> Both this and the previous patch look good to me;\n>> you seem to have better (stricter) checking for\n>> missing includes/forward declarations, am I missing\n>> a compile option? (I have DEVELOPER=1 in config.mak)\n> Thanks. No I don't - I discovered that these were missing as I was\n> including these header files in other files.\n\nI discovered similar missing headers in object-store.h while reworking \nthe MIDX patch series. Thanks for fixing them here!\n\n"},{"id":"350698","messageId":"d7a42ecf-7261-ec14-5736-b1f9bc70ba64@gmail.com","threadId":"48756","inReplyTo":"20180621230608.86117-1-jonathantanmy@google.com","subject":"Re: [PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-06-22T00:33:54Z","receivedAt":"2018-06-22T00:33:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/21/2018 7:06 PM, Jonathan Tan wrote:\n>>> diff --git a/commit.c b/commit.c\n>>> index 0030e79940..38c12b002f 100644\n>>> --- a/commit.c\n>>> +++ b/commit.c\n>>> @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n>>> if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n>>> BUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n>>>\n>>> - return get_commit_tree_in_graph(commit);\n>>> + return get_commit_tree_in_graph(the_repository, commit);\n>> Here..\n>>\n>>> }\n>>>\n>>> struct object_id *get_commit_tree_oid(const struct commit *commit)\n>>> @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)\n>>> return -1;\n>>> if (item->object.parsed)\n>>> return 0;\n>>> - if (parse_commit_in_graph(item))\n>>> + if (parse_commit_in_graph(the_repository, item))\n>> and here\n>>\n>>> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n>>> + const struct object_id *commit_oid)\n>>> +{\n>>> + struct repository r;\n>>> + struct commit *c;\n>>> + struct commit_list *parent;\n>>> +\n>>> + /*\n>>> + * Create a commit independent of any repository.\n>>> + */\n>>> + c = lookup_commit(commit_oid);\n>> .. and this one are unfortunate as the rest of the object store series\n>> has not progressed as far as needed.\n> I think the first 2 are in reverse - get_commit_tree depends on\n> get_commit_tree_in_graph and parse_commit_gently depends on\n> parse_commit_in_graph, so we need the commit-graph functions to be\n> changed first. But I agree about lookup_commit.\n>\n>> The lookup_commit series is out there already, and that will\n>> teach lookup_commit a repository argument. When rerolling\n>> that series I need to switch the order of repo_init and lookup_commit\n>> such that we can pass the repo to the lookup.\n> For future reference, Stefan is talking about this series:\n> https://public-inbox.org/git/20180613230522.55335-1-sbeller@google.com/\n>\n> Let me know if you want to reroll yours on top of mine, or vice versa. I\n> think it's clearer if mine goes in first, though, since (as you said in\n> that e-mail) parse_commit depends on this change in the commit graph.\n\nI was about to comment that I thought 'parse_commit_in_graph' should \ntake a 'struct commit_graph' instead of 'struct repository', except for \nthese lookup_commit() calls will need the repository parameter.\n\nPlease also keep in mind that ds/commit-graph-fsck has already updated \nthis method to parse from a specific graph [1]. I'm just waiting for \nsome things like ds/generation-numbers to get into 'master' and some \nmore object-store patches to be final before I re-roll that series. I \nmentioned this in a message that I had sent, but apparently didn't make \nit on the list (so I re-sent it recently).\n\n[1] \nhttps://public-inbox.org/git/20180608135548.216405-4-dstolee@microsoft.com/\n\n>> This is really nice!\n>>\n>> Overall this series looks good to me,\n>> Stefan\n> Thanks - let's see what others think.\n"},{"id":"350700","messageId":"e01ee415-2623-6aed-f003-dfe68b4e32eb@gmail.com","threadId":"48756","inReplyTo":"d7a42ecf-7261-ec14-5736-b1f9bc70ba64@gmail.com","subject":"Re: [PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-06-22T01:01:24Z","receivedAt":"2018-06-22T01:01:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/21/2018 8:33 PM, Derrick Stolee wrote:\n> On 6/21/2018 7:06 PM, Jonathan Tan wrote:\n>>>> diff --git a/commit.c b/commit.c\n>>>> index 0030e79940..38c12b002f 100644\n>>>> --- a/commit.c\n>>>> +++ b/commit.c\n>>>> @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct \n>>>> commit *commit)\n>>>>          if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n>>>>                  BUG(\"commit has NULL tree, but was not loaded from \n>>>> commit-graph\");\n>>>>\n>>>> -       return get_commit_tree_in_graph(commit);\n>>>> +       return get_commit_tree_in_graph(the_repository, commit);\n>>> Here..\n>>>\n>>>>   }\n>>>>\n>>>>   struct object_id *get_commit_tree_oid(const struct commit *commit)\n>>>> @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, \n>>>> int quiet_on_missing)\n>>>>                  return -1;\n>>>>          if (item->object.parsed)\n>>>>                  return 0;\n>>>> -       if (parse_commit_in_graph(item))\n>>>> +       if (parse_commit_in_graph(the_repository, item))\n>>> and here\n>>>\n>>>> +static void test_parse_commit_in_graph(const char *gitdir, const \n>>>> char *worktree,\n>>>> +                                      const struct object_id \n>>>> *commit_oid)\n>>>> +{\n>>>> +       struct repository r;\n>>>> +       struct commit *c;\n>>>> +       struct commit_list *parent;\n>>>> +\n>>>> +       /*\n>>>> +        * Create a commit independent of any repository.\n>>>> +        */\n>>>> +       c = lookup_commit(commit_oid);\n>>> .. and this one are unfortunate as the rest of the object store series\n>>> has not progressed as far as needed.\n>> I think the first 2 are in reverse - get_commit_tree depends on\n>> get_commit_tree_in_graph and parse_commit_gently depends on\n>> parse_commit_in_graph, so we need the commit-graph functions to be\n>> changed first. But I agree about lookup_commit.\n>>\n>>> The lookup_commit series is out there already, and that will\n>>> teach lookup_commit a repository argument. When rerolling\n>>> that series I need to switch the order of repo_init and lookup_commit\n>>> such that we can pass the repo to the lookup.\n>> For future reference, Stefan is talking about this series:\n>> https://public-inbox.org/git/20180613230522.55335-1-sbeller@google.com/\n>>\n>> Let me know if you want to reroll yours on top of mine, or vice versa. I\n>> think it's clearer if mine goes in first, though, since (as you said in\n>> that e-mail) parse_commit depends on this change in the commit graph.\n>\n> I was about to comment that I thought 'parse_commit_in_graph' should \n> take a 'struct commit_graph' instead of 'struct repository', except \n> for these lookup_commit() calls will need the repository parameter.\n>\n> Please also keep in mind that ds/commit-graph-fsck has already updated \n> this method to parse from a specific graph [1]. I'm just waiting for \n> some things like ds/generation-numbers to get into 'master' and some \n> more object-store patches to be final before I re-roll that series. I \n> mentioned this in a message that I had sent, but apparently didn't \n> make it on the list (so I re-sent it recently).\n>\n> [1] \n> https://public-inbox.org/git/20180608135548.216405-4-dstolee@microsoft.com/\n\nHere is the re-send: \nhttps://public-inbox.org/git/4e7600f1-6dd0-3b21-5f5d-26af2b3c0b1a@gmail.com/T/#t\n\n"},{"id":"350748","messageId":"20180622172146.1727-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"d7a42ecf-7261-ec14-5736-b1f9bc70ba64@gmail.com","subject":"Re: [PATCH 5/5] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-22T17:21:46Z","receivedAt":"2018-06-22T17:21:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On 6/21/2018 7:06 PM, Jonathan Tan wrote:\n> >>> diff --git a/commit.c b/commit.c\n> >>> index 0030e79940..38c12b002f 100644\n> >>> --- a/commit.c\n> >>> +++ b/commit.c\n> >>> @@ -317,7 +317,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n> >>> if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n> >>> BUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n> >>>\n> >>> - return get_commit_tree_in_graph(commit);\n> >>> + return get_commit_tree_in_graph(the_repository, commit);\n> >> Here..\n> >>\n> >>> }\n> >>>\n> >>> struct object_id *get_commit_tree_oid(const struct commit *commit)\n> >>> @@ -413,7 +413,7 @@ int parse_commit_gently(struct commit *item, int quiet_on_missing)\n> >>> return -1;\n> >>> if (item->object.parsed)\n> >>> return 0;\n> >>> - if (parse_commit_in_graph(item))\n> >>> + if (parse_commit_in_graph(the_repository, item))\n> >> and here\n> >>\n> >>> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> >>> + const struct object_id *commit_oid)\n> >>> +{\n> >>> + struct repository r;\n> >>> + struct commit *c;\n> >>> + struct commit_list *parent;\n> >>> +\n> >>> + /*\n> >>> + * Create a commit independent of any repository.\n> >>> + */\n> >>> + c = lookup_commit(commit_oid);\n> >> .. and this one are unfortunate as the rest of the object store series\n> >> has not progressed as far as needed.\n> > I think the first 2 are in reverse - get_commit_tree depends on\n> > get_commit_tree_in_graph and parse_commit_gently depends on\n> > parse_commit_in_graph, so we need the commit-graph functions to be\n> > changed first. But I agree about lookup_commit.\n> >\n> >> The lookup_commit series is out there already, and that will\n> >> teach lookup_commit a repository argument. When rerolling\n> >> that series I need to switch the order of repo_init and lookup_commit\n> >> such that we can pass the repo to the lookup.\n> > For future reference, Stefan is talking about this series:\n> > https://public-inbox.org/git/20180613230522.55335-1-sbeller@google.com/\n> >\n> > Let me know if you want to reroll yours on top of mine, or vice versa. I\n> > think it's clearer if mine goes in first, though, since (as you said in\n> > that e-mail) parse_commit depends on this change in the commit graph.\n> \n> I was about to comment that I thought 'parse_commit_in_graph' should \n> take a 'struct commit_graph' instead of 'struct repository', except for \n> these lookup_commit() calls will need the repository parameter.\n\nNot sure what you mean by the lookup_commit() calls (if you're referring\nto the part quoted above, that is test code), but\nparse_commit_in_graph() has to take a struct repository (or at least a\nstruct object_store, perhaps) because it needs to load the commit graph\nfor the repository if it is not already loaded.\n\n(An alternative, of course, is to require the user to explicitly load\nthe graph, but since parse_commit_in_graph() is called from\nparse_commit(), I think that this implicit loading is fine.)\n\n> Please also keep in mind that ds/commit-graph-fsck has already updated \n> this method to parse from a specific graph [1]. I'm just waiting for \n> some things like ds/generation-numbers to get into 'master' and some \n> more object-store patches to be final before I re-roll that series. I \n> mentioned this in a message that I had sent, but apparently didn't make \n> it on the list (so I re-sent it recently).\n> \n> [1] \n> https://public-inbox.org/git/20180608135548.216405-4-dstolee@microsoft.com/\n\nThanks - I see that you introduced a new\nparse_commit_in_graph_one(struct commit_graph *, struct commit *) and\nmade the existing parse_commit_in_graph(struct commit *item) use the\nformer. Combining our changes would be just adding a repository argument\nto parse_commit_in_graph() and passing the graph through\nparse_commit_in_graph_one() (after prepare_commit_graph() and ensuring\nthat the graph exists).\n"},{"id":"350900","messageId":"xmqqwoummvmw.fsf@gitster-ct.c.googlers.com","threadId":"48756","inReplyTo":"3410494e3505a7b1b895869c3702dbba0f0cf5a6.1529616356.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/5] commit-graph: store graph in struct object_store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-25T20:57:59Z","receivedAt":"2018-06-25T20:58:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Instead of storing commit graphs in static variables, store them in\n> struct object_store. There are no changes to the signatures of existing\n> functions - they all still only support the_repository, and support for\n> other instances of struct repository will be added in a subsequent\n> commit.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> commit-graph.c | 34 ++++++++++++++++++----------------\n> object-store.h | 3 +++\n> object.c | 5 +++++\n> 3 files changed, 26 insertions(+), 16 deletions(-)\n> ...\n> @@ -293,7 +290,9 @@ int parse_commit_in_graph(struct commit *item)\n> \t\treturn 1;\n> \n> \tprepare_commit_graph();\n> -\tif (commit_graph) {\n> +\tif (the_repository->objects->commit_graph) {\n> +\t\tstruct commit_graph *commit_graph =\n> +\t\t\tthe_repository->objects->commit_graph;\n> \t\tuint32_t pos;\n> \t\tint found;\n> \t\tif (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n> @@ -329,7 +328,8 @@ struct tree *get_commit_tree_in_graph(const struct commit *c)\n> \tif (c->graph_pos == COMMIT_NOT_FROM_GRAPH)\n> \t\tBUG(\"get_commit_tree_in_graph called from non-commit-graph commit\");\n> \n> -\treturn load_tree_for_commit(commit_graph, (struct commit *)c);\n> +\treturn load_tree_for_commit(the_repository->objects->commit_graph,\n> +\t\t\t\t (struct commit *)c);\n> }\n\nI was looking at semantic merge conflicts between this and your\ne2838d85 (\"commit-graph: always load commit-graph information\",\n2018-05-01), the latter of which I planned to merge to 'master' as a\npart of the first batch in this cycle, and noticed that there are\ntwo very similar functions, without enough document the callers\nwould not know which one is the correct one to call. Needless to\nsay, such a code duplication would mean the work required for\nresolving semantic conflict doubles needlessly X-<.\n\n\nint parse_commit_in_graph(struct commit *item)\n{\n\tuint32_t pos;\n\n\tif (!core_commit_graph)\n\t\treturn 0;\n\tif (item->object.parsed)\n\t\treturn 1;\n\tprepare_commit_graph();\n\tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n\t\treturn fill_commit_in_graph(item, commit_graph, pos);\n\treturn 0;\n}\n\nvoid load_commit_graph_info(struct commit *item)\n{\n\tuint32_t pos;\n\tif (!core_commit_graph)\n\t\treturn;\n\tprepare_commit_graph();\n\tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n\t\tfill_commit_graph_info(item, commit_graph, pos);\n}\n"},{"id":"350906","messageId":"20180625220915.198932-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"xmqqwoummvmw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/5] commit-graph: store graph in struct object_store","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-25T22:09:15Z","receivedAt":"2018-06-25T22:09:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I was looking at semantic merge conflicts between this and your\n> e2838d85 (\"commit-graph: always load commit-graph information\",\n> 2018-05-01), the latter of which I planned to merge to 'master' as a\n> part of the first batch in this cycle, and noticed that there are\n> two very similar functions, without enough document the callers\n> would not know which one is the correct one to call. Needless to\n> say, such a code duplication would mean the work required for\n> resolving semantic conflict doubles needlessly X-<.\n> \n> \n> int parse_commit_in_graph(struct commit *item)\n> {\n> \tuint32_t pos;\n> \n> \tif (!core_commit_graph)\n> \t\treturn 0;\n> \tif (item->object.parsed)\n> \t\treturn 1;\n> \tprepare_commit_graph();\n> \tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n> \t\treturn fill_commit_in_graph(item, commit_graph, pos);\n> \treturn 0;\n> }\n> \n> void load_commit_graph_info(struct commit *item)\n> {\n> \tuint32_t pos;\n> \tif (!core_commit_graph)\n> \t\treturn;\n> \tprepare_commit_graph();\n> \tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n> \t\tfill_commit_graph_info(item, commit_graph, pos);\n> }\n\nThanks for letting me know - when I reroll, I'll ensure that I reroll on\ntop of this change.\n\nAs for whether both these functions are necessary in the first place, I\nthink they are: we could have a load_commit_graph_info() that ignores\nthe parsedness of the commit and just copies everything over, but that\nmeans that we can't have the optimization of returning immediately\nwithout consulting the graph when attempting to reparse a parsed object.\n\nSo we can't delete parse_commit_in_graph(). And we can't delete\nload_commit_graph_info() either, because after reading the documentation\nin commit-graph.h, looking at the places where it is used, and observing\nsome test failures when I make parse_object_buffer() not use the graph,\nI see that there are code paths in Git in which we use both\nparsed-from-buffer and parsed-without-buffer commits at the same time.\n\nSo, I now look at the code duplication, and I see that it is mostly the\ncheck-prepare-check step; my patch set reduces it a little, but I'll try\nto reduce it nearly completely when I reroll too.\n\nMaybe when the object store becomes more pervasive, we can more clearly\nseparate out the buffers that come from a repository (and maybe, when\nobtaining those buffers, we should obtain graph information at the same\ntime) and buffers that we obtain from some arbitrary non-repository\nsource, but that will take some time. In the meantime, I'll do what I\nsuggested above when I reroll, maintaining these 2 functions.\n"},{"id":"350978","messageId":"xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com","threadId":"48756","inReplyTo":"20180625220915.198932-1-jonathantanmy@google.com","subject":"Re: [PATCH 4/5] commit-graph: store graph in struct object_store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T16:40:29Z","receivedAt":"2018-06-26T16:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> As for whether both these functions are necessary in the first place, I\n> think they are.\n\nI do not mind their existence. \n\nI was wondering if they can share more implementation; such a design\nwould need s/the_commit_graph/the_repo->objstore->commit_graph/ only\nonce as a side effect.\n\n"},{"id":"352017","messageId":"cover.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:34Z","receivedAt":"2018-07-09T20:44:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is on ds/commit-graph-fsck.\n\nI saw that ds/commit-graph-fsck has been updated to the latest version\n(v7, including \"gc.writeCommitGraph\"), so I've rebased my changes on top\nof that branch. There were some mechanical changes needed during the\nrebase, so I'm sending the rebased patches out.\n\nI've also added a patch (patch 1) that removes some duplication of\nimplementation that Junio talked about in [1].\n\n[1] https://public-inbox.org/git/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/\n\nJonathan Tan (6):\n commit-graph: refactor preparing commit graph\n object-store: add missing include\n commit-graph: add missing forward declaration\n commit-graph: add free_commit_graph\n commit-graph: store graph in struct object_store\n commit-graph: add repo arg to graph readers\n\n Makefile | 1 +\n builtin/commit-graph.c | 2 +\n builtin/fsck.c | 2 +-\n cache.h | 1 -\n commit-graph.c | 108 +++++++++++++++++++++----------------\n commit-graph.h | 11 ++--\n commit.c | 6 +--\n config.c | 5 --\n environment.c | 1 -\n object-store.h | 6 +++\n object.c | 5 ++\n ref-filter.c | 2 +-\n t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 ++++++++++++\n 16 files changed, 213 insertions(+), 62 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352018","messageId":"9769e2a7471c2b32185896e833cc74824ac5437e.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 1/6] commit-graph: refactor preparing commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:35Z","receivedAt":"2018-07-09T20:44:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Two functions in the code (1) check if the repository is configured for\ncommit graphs, (2) call prepare_commit_graph(), and (3) check if the\ngraph exists. Move (1) and (3) into prepare_commit_graph(), reducing\nduplication of code.\n\nSigned-off-by: Jonathan Tan \n---\n commit-graph.c | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 212232e752..5ba60f63f9 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -200,15 +200,25 @@ static void prepare_commit_graph_one(const char *obj_dir)\n }\n \n static int prepare_commit_graph_run_once = 0;\n-static void prepare_commit_graph(void)\n+\n+/*\n+ * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n+ *\n+ * On the first invocation, this function attemps to load the commit\n+ * graph if the_repository is configured to have one.\n+ */\n+static int prepare_commit_graph(void)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n \n \tif (prepare_commit_graph_run_once)\n-\t\treturn;\n+\t\treturn !!commit_graph;\n \tprepare_commit_graph_run_once = 1;\n \n+\tif (!core_commit_graph)\n+\t\treturn 0;\n+\n \tobj_dir = get_object_directory();\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb(the_repository);\n@@ -216,6 +226,7 @@ static void prepare_commit_graph(void)\n \t !commit_graph && alt;\n \t alt = alt->next)\n \t\tprepare_commit_graph_one(alt->path);\n+\treturn !!commit_graph;\n }\n \n static void close_commit_graph(void)\n@@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n \n int parse_commit_in_graph(struct commit *item)\n {\n-\tif (!core_commit_graph)\n+\tif (!prepare_commit_graph())\n \t\treturn 0;\n-\n-\tprepare_commit_graph();\n-\tif (commit_graph)\n-\t\treturn parse_commit_in_graph_one(commit_graph, item);\n-\treturn 0;\n+\treturn parse_commit_in_graph_one(commit_graph, item);\n }\n \n void load_commit_graph_info(struct commit *item)\n {\n \tuint32_t pos;\n-\tif (!core_commit_graph)\n+\tif (!prepare_commit_graph())\n \t\treturn;\n-\tprepare_commit_graph();\n-\tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n+\tif (find_commit_in_graph(item, commit_graph, &pos))\n \t\tfill_commit_graph_info(item, commit_graph, pos);\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352019","messageId":"b0077ef5d43184bc1d2df24ba6e4415e8e0878ea.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 2/6] object-store: add missing include","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:36Z","receivedAt":"2018-07-09T20:44:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n object-store.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/object-store.h b/object-store.h\nindex d683112fd7..f0b77146e4 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -2,6 +2,9 @@\n #define OBJECT_STORE_H\n \n #include \"oidmap.h\"\n+#include \"list.h\"\n+#include \"sha1-array.h\"\n+#include \"strbuf.h\"\n \n struct alternate_object_database {\n \tstruct alternate_object_database *next;\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352020","messageId":"c7849a302bafe224ca3240b4c0af546c6e6e7bd4.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 3/6] commit-graph: add missing forward declaration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:37Z","receivedAt":"2018-07-09T20:44:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n commit-graph.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 506cb45fb1..674052bef4 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -5,6 +5,8 @@\n #include \"repository.h\"\n #include \"string-list.h\"\n \n+struct commit;\n+\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352021","messageId":"06fd347b21dbc039936dd9ab2701d16b9befbd19.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 4/6] commit-graph: add free_commit_graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:38Z","receivedAt":"2018-07-09T20:45:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/commit-graph.c | 2 ++\n commit-graph.c | 24 ++++++++++++++----------\n commit-graph.h | 2 ++\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c7d0db5ab4..0bf0c48657 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -115,6 +115,8 @@ static int graph_read(int argc, const char **argv)\n \t\tprintf(\" large_edges\");\n \tprintf(\"\\n\");\n \n+\tfree_commit_graph(graph);\n+\n \treturn 0;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5ba60f63f9..6d1bc4ff7c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -231,16 +231,8 @@ static int prepare_commit_graph(void)\n \n static void close_commit_graph(void)\n {\n-\tif (!commit_graph)\n-\t\treturn;\n-\n-\tif (commit_graph->graph_fd >= 0) {\n-\t\tmunmap((void *)commit_graph->data, commit_graph->data_len);\n-\t\tcommit_graph->data = NULL;\n-\t\tclose(commit_graph->graph_fd);\n-\t}\n-\n-\tFREE_AND_NULL(commit_graph);\n+\tfree_commit_graph(commit_graph);\n+\tcommit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -1033,3 +1025,15 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \n \treturn verify_commit_graph_error;\n }\n+\n+void free_commit_graph(struct commit_graph *g)\n+{\n+\tif (!g)\n+\t\treturn;\n+\tif (g->graph_fd >= 0) {\n+\t\tmunmap((void *)g->data, g->data_len);\n+\t\tg->data = NULL;\n+\t\tclose(g->graph_fd);\n+\t}\n+\tfree(g);\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 674052bef4..94defb04a9 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -58,4 +58,6 @@ void write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n+void free_commit_graph(struct commit_graph *);\n+\n #endif\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352022","messageId":"19ccb4643a68df95051cf4513038471aa1bdd900.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 5/6] commit-graph: store graph in struct object_store","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:39Z","receivedAt":"2018-07-09T20:45:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of storing commit graphs in static variables, store them in\nstruct object_store. There are no changes to the signatures of existing\nfunctions - they all still only support the_repository, and support for\nother instances of struct repository will be added in a subsequent\ncommit.\n\nSigned-off-by: Jonathan Tan \n---\n commit-graph.c | 40 +++++++++++++++++++---------------------\n object-store.h | 3 +++\n object.c | 5 +++++\n 3 files changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6d1bc4ff7c..af97a10603 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -183,24 +183,20 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-/* global storage */\n-static struct commit_graph *commit_graph = NULL;\n-\n static void prepare_commit_graph_one(const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (commit_graph)\n+\tif (the_repository->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tcommit_graph = load_commit_graph_one(graph_name);\n+\tthe_repository->objects->commit_graph =\n+\t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n }\n \n-static int prepare_commit_graph_run_once = 0;\n-\n /*\n * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n *\n@@ -212,9 +208,9 @@ static int prepare_commit_graph(void)\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n \n-\tif (prepare_commit_graph_run_once)\n-\t\treturn !!commit_graph;\n-\tprepare_commit_graph_run_once = 1;\n+\tif (the_repository->objects->commit_graph_attempted)\n+\t\treturn !!the_repository->objects->commit_graph;\n+\tthe_repository->objects->commit_graph_attempted = 1;\n \n \tif (!core_commit_graph)\n \t\treturn 0;\n@@ -223,16 +219,16 @@ static int prepare_commit_graph(void)\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb(the_repository);\n \tfor (alt = the_repository->objects->alt_odb_list;\n-\t !commit_graph && alt;\n+\t !the_repository->objects->commit_graph && alt;\n \t alt = alt->next)\n \t\tprepare_commit_graph_one(alt->path);\n-\treturn !!commit_graph;\n+\treturn !!the_repository->objects->commit_graph;\n }\n \n static void close_commit_graph(void)\n {\n-\tfree_commit_graph(commit_graph);\n-\tcommit_graph = NULL;\n+\tfree_commit_graph(the_repository->objects->commit_graph);\n+\tthe_repository->objects->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -342,7 +338,7 @@ int parse_commit_in_graph(struct commit *item)\n {\n \tif (!prepare_commit_graph())\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(commit_graph, item);\n+\treturn parse_commit_in_graph_one(the_repository->objects->commit_graph, item);\n }\n \n void load_commit_graph_info(struct commit *item)\n@@ -350,8 +346,8 @@ void load_commit_graph_info(struct commit *item)\n \tuint32_t pos;\n \tif (!prepare_commit_graph())\n \t\treturn;\n-\tif (find_commit_in_graph(item, commit_graph, &pos))\n-\t\tfill_commit_graph_info(item, commit_graph, pos);\n+\tif (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))\n+\t\tfill_commit_graph_info(item, the_repository->objects->commit_graph, pos);\n }\n \n static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)\n@@ -379,7 +375,7 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,\n \n struct tree *get_commit_tree_in_graph(const struct commit *c)\n {\n-\treturn get_commit_tree_in_graph_one(commit_graph, c);\n+\treturn get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);\n }\n \n static void write_graph_chunk_fanout(struct hashfile *f,\n@@ -696,15 +692,17 @@ void write_commit_graph(const char *obj_dir,\n \n \tif (append) {\n \t\tprepare_commit_graph_one(obj_dir);\n-\t\tif (commit_graph)\n-\t\t\toids.alloc += commit_graph->num_commits;\n+\t\tif (the_repository->objects->commit_graph)\n+\t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\n \n \tif (oids.alloc < 1024)\n \t\toids.alloc = 1024;\n \tALLOC_ARRAY(oids.list, oids.alloc);\n \n-\tif (append && commit_graph) {\n+\tif (append && the_repository->objects->commit_graph) {\n+\t\tstruct commit_graph *commit_graph =\n+\t\t\tthe_repository->objects->commit_graph;\n \t\tfor (i = 0; i < commit_graph->num_commits; i++) {\n \t\t\tconst unsigned char *hash = commit_graph->chunk_oid_lookup +\n \t\t\t\tcommit_graph->hash_len * i;\ndiff --git a/object-store.h b/object-store.h\nindex f0b77146e4..39f4a3d5b9 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -106,6 +106,9 @@ struct raw_object_store {\n \t */\n \tstruct oidmap *replace_map;\n \n+\tstruct commit_graph *commit_graph;\n+\tunsigned commit_graph_attempted : 1; /* if loading has been attempted */\n+\n \t/*\n \t * private data\n \t *\ndiff --git a/object.c b/object.c\nindex 10d167825e..6672820b9e 100644\n--- a/object.c\n+++ b/object.c\n@@ -8,6 +8,7 @@\n #include \"alloc.h\"\n #include \"object-store.h\"\n #include \"packfile.h\"\n+#include \"commit-graph.h\"\n \n unsigned int get_max_object_index(void)\n {\n@@ -501,6 +502,10 @@ void raw_object_store_clear(struct raw_object_store *o)\n \toidmap_free(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n \n+\tfree_commit_graph(o->commit_graph);\n+\to->commit_graph = NULL;\n+\to->commit_graph_attempted = 0;\n+\n \tfree_alt_odbs(o);\n \to->alt_odb_tail = NULL;\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352023","messageId":"f1ccfdccc851039cf62978d162cd9de99ea619de.1531168854.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"[PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-09T20:44:40Z","receivedAt":"2018-07-09T20:45:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a struct repository argument to the functions in commit-graph.h that\nread the commit graph. (This commit does not affect functions that write\ncommit graphs.)\n\nBecause the commit graph functions can now read the commit graph of any\nrepository, the global variable core_commit_graph has been removed.\nInstead, the config option core.commitGraph is now read on the first\ntime in a repository that a commit is attempted to be parsed using its\ncommit graph.\n\nThis commit includes a test that exercises the functionality on an\narbitrary repository that is not the_repository.\n\nSigned-off-by: Jonathan Tan \n---\n Makefile | 1 +\n builtin/fsck.c | 2 +-\n cache.h | 1 -\n commit-graph.c | 60 ++++++++++++++------------\n commit-graph.h | 7 +--\n commit.c | 6 +--\n config.c | 5 ---\n environment.c | 1 -\n ref-filter.c | 2 +-\n t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 +++++++++++++++\n 13 files changed, 168 insertions(+), 42 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\ndiff --git a/Makefile b/Makefile\nindex 0cb6590f24..bb8bd67201 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-ref-store.o\n TEST_BUILTINS_OBJS += test-regex.o\n+TEST_BUILTINS_OBJS += test-repository.o\n TEST_BUILTINS_OBJS += test-revision-walking.o\n TEST_BUILTINS_OBJS += test-run-command.o\n TEST_BUILTINS_OBJS += test-scrap-cache-tree.o\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex eca7900ee0..2abfb2d782 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -825,7 +825,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (core_commit_graph) {\n+\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/cache.h b/cache.h\nindex d49092d94d..8dc59bedba 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -813,7 +813,6 @@ extern char *git_replace_ref_base;\n \n extern int fsync_object_files;\n extern int core_preload_index;\n-extern int core_commit_graph;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex af97a10603..8eab199b1b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -183,15 +183,15 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-static void prepare_commit_graph_one(const char *obj_dir)\n+static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (the_repository->objects->commit_graph)\n+\tif (r->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tthe_repository->objects->commit_graph =\n+\tr->objects->commit_graph =\n \t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n@@ -203,26 +203,34 @@ static void prepare_commit_graph_one(const char *obj_dir)\n * On the first invocation, this function attemps to load the commit\n * graph if the_repository is configured to have one.\n */\n-static int prepare_commit_graph(void)\n+static int prepare_commit_graph(struct repository *r)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n+\tint config_value;\n \n-\tif (the_repository->objects->commit_graph_attempted)\n-\t\treturn !!the_repository->objects->commit_graph;\n-\tthe_repository->objects->commit_graph_attempted = 1;\n+\tif (r->objects->commit_graph_attempted)\n+\t\treturn !!r->objects->commit_graph;\n+\tr->objects->commit_graph_attempted = 1;\n \n-\tif (!core_commit_graph)\n+\tif (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n+\t !config_value)\n+\t\t/*\n+\t\t * This repository is not configured to use commit graphs, so\n+\t\t * do not load one. (But report commit_graph_attempted anyway\n+\t\t * so that commit graph loading is not attempted again for this\n+\t\t * repository.)\n+\t\t */\n \t\treturn 0;\n \n-\tobj_dir = get_object_directory();\n-\tprepare_commit_graph_one(obj_dir);\n-\tprepare_alt_odb(the_repository);\n-\tfor (alt = the_repository->objects->alt_odb_list;\n-\t !the_repository->objects->commit_graph && alt;\n+\tobj_dir = r->objects->objectdir;\n+\tprepare_commit_graph_one(r, obj_dir);\n+\tprepare_alt_odb(r);\n+\tfor (alt = r->objects->alt_odb_list;\n+\t !r->objects->commit_graph && alt;\n \t alt = alt->next)\n-\t\tprepare_commit_graph_one(alt->path);\n-\treturn !!the_repository->objects->commit_graph;\n+\t\tprepare_commit_graph_one(r, alt->path);\n+\treturn !!r->objects->commit_graph;\n }\n \n static void close_commit_graph(void)\n@@ -323,8 +331,6 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n {\n \tuint32_t pos;\n \n-\tif (!core_commit_graph)\n-\t\treturn 0;\n \tif (item->object.parsed)\n \t\treturn 1;\n \n@@ -334,20 +340,20 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n \treturn 0;\n }\n \n-int parse_commit_in_graph(struct commit *item)\n+int parse_commit_in_graph(struct repository *r, struct commit *item)\n {\n-\tif (!prepare_commit_graph())\n+\tif (!prepare_commit_graph(r))\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(the_repository->objects->commit_graph, item);\n+\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n }\n \n-void load_commit_graph_info(struct commit *item)\n+void load_commit_graph_info(struct repository *r, struct commit *item)\n {\n \tuint32_t pos;\n-\tif (!prepare_commit_graph())\n+\tif (!prepare_commit_graph(r))\n \t\treturn;\n-\tif (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))\n-\t\tfill_commit_graph_info(item, the_repository->objects->commit_graph, pos);\n+\tif (find_commit_in_graph(item, r->objects->commit_graph, &pos))\n+\t\tfill_commit_graph_info(item, r->objects->commit_graph, pos);\n }\n \n static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)\n@@ -373,9 +379,9 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,\n \treturn load_tree_for_commit(g, (struct commit *)c);\n }\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c)\n+struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit *c)\n {\n-\treturn get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);\n+\treturn get_commit_tree_in_graph_one(r->objects->commit_graph, c);\n }\n \n static void write_graph_chunk_fanout(struct hashfile *f,\n@@ -691,7 +697,7 @@ void write_commit_graph(const char *obj_dir,\n \toids.alloc = approximate_object_count() / 4;\n \n \tif (append) {\n-\t\tprepare_commit_graph_one(obj_dir);\n+\t\tprepare_commit_graph_one(the_repository, obj_dir);\n \t\tif (the_repository->objects->commit_graph)\n \t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 94defb04a9..76e098934a 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -19,7 +19,7 @@ char *get_commit_graph_filename(const char *obj_dir);\n *\n * See parse_commit_buffer() for the fallback after this call.\n */\n-int parse_commit_in_graph(struct commit *item);\n+int parse_commit_in_graph(struct repository *r, struct commit *item);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\n@@ -27,9 +27,10 @@ int parse_commit_in_graph(struct commit *item);\n * checked and filled. Fill the graph_pos and generation members of\n * the given commit.\n */\n-void load_commit_graph_info(struct commit *item);\n+void load_commit_graph_info(struct repository *r, struct commit *item);\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c);\n+struct tree *get_commit_tree_in_graph(struct repository *r,\n+\t\t\t\t const struct commit *c);\n \n struct commit_graph {\n \tint graph_fd;\ndiff --git a/commit.c b/commit.c\nindex 598cf21cee..b3bbfefc27 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -319,7 +319,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n \tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n \t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n \n-\treturn get_commit_tree_in_graph(commit);\n+\treturn get_commit_tree_in_graph(the_repository, commit);\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n@@ -413,7 +413,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \titem->date = parse_commit_date(bufptr, tail);\n \n \tif (check_graph)\n-\t\tload_commit_graph_info(item);\n+\t\tload_commit_graph_info(the_repository, item);\n \n \treturn 0;\n }\n@@ -429,7 +429,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(item))\n+\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n \t\treturn 0;\n \tbuffer = read_object_file(&item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/config.c b/config.c\nindex a0a6ae1980..fcf863b667 100644\n--- a/config.c\n+++ b/config.c\n@@ -1308,11 +1308,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.commitgraph\")) {\n-\t\tcore_commit_graph = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"core.sparsecheckout\")) {\n \t\tcore_apply_sparse_checkout = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 2a6de2330b..d6e5b39425 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n int grafts_replace_parents = 1;\n-int core_commit_graph;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fa3685d91f..889d97b005 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1710,7 +1710,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,\n \n \tfor (p = want; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n-\t\tload_commit_graph_info(c);\n+\t\tload_commit_graph_info(the_repository, c);\n \t\tif (c->generation < cutoff)\n \t\t\tcutoff = c->generation;\n \t}\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nnew file mode 100644\nindex 0000000000..5fff540a26\n--- /dev/null\n+++ b/t/helper/test-repository.c\n@@ -0,0 +1,88 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"commit-graph.h\"\n+#include \"commit.h\"\n+#include \"config.h\"\n+#include \"object-store.h\"\n+#include \"object.h\"\n+#include \"repository.h\"\n+#include \"tree.h\"\n+\n+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n+\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct commit_list *parent;\n+\n+\t/*\n+\t * Create a commit independent of any repository.\n+\t */\n+\tc = lookup_commit(commit_oid);\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\n+\tprintf(\"%lu\", c->date);\n+\tfor (parent = c->parents; parent; parent = parent->next)\n+\t\tprintf(\" %s\", oid_to_hex(&parent->item->object.oid));\n+\tprintf(\"\\n\");\n+\n+\trepo_clear(&r);\n+}\n+\n+static void test_get_commit_tree_in_graph(const char *gitdir,\n+\t\t\t\t\t const char *worktree,\n+\t\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct tree *tree;\n+\n+\t/*\n+\t * Create a commit independent of any repository.\n+\t */\n+\tc = lookup_commit(commit_oid);\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\t/*\n+\t * get_commit_tree_in_graph does not automatically parse the commit, so\n+\t * parse it first.\n+\t */\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\ttree = get_commit_tree_in_graph(&r, c);\n+\tif (!tree)\n+\t\tdie(\"Couldn't get commit tree\");\n+\n+\tprintf(\"%s\\n\", oid_to_hex(&tree->object.oid));\n+\n+\trepo_clear(&r);\n+}\n+\n+int cmd__repository(int argc, const char **argv)\n+{\n+\tif (argc < 2)\n+\t\tdie(\"must have at least 2 arguments\");\n+\tif (!strcmp(argv[1], \"parse_commit_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_parse_commit_in_graph(argv[2], argv[3], &oid);\n+\t} else if (!strcmp(argv[1], \"get_commit_tree_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_get_commit_tree_in_graph(argv[2], argv[3], &oid);\n+\t} else {\n+\t\tdie(\"unrecognized '%s'\", argv[1]);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 805a45de9c..dafc91c240 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"ref-store\", cmd__ref_store },\n \t{ \"regex\", cmd__regex },\n+\t{ \"repository\", cmd__repository },\n \t{ \"revision-walking\", cmd__revision_walking },\n \t{ \"run-command\", cmd__run_command },\n \t{ \"scrap-cache-tree\", cmd__scrap_cache_tree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb94..80cbcf0857 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -23,6 +23,7 @@ int cmd__prio_queue(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\n int cmd__regex(int argc, const char **argv);\n+int cmd__repository(int argc, const char **argv);\n int cmd__revision_walking(int argc, const char **argv);\n int cmd__run_command(int argc, const char **argv);\n int cmd__scrap_cache_tree(int argc, const char **argv);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5947de3d24..4f17d7701e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -431,4 +431,39 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \ttest_must_fail git fsck\n '\n \n+test_expect_success 'setup non-the_repository tests' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\ttest_commit -C repo one &&\n+\ttest_commit -C repo two &&\n+\tgit -C repo config core.commitGraph true &&\n+\tgit -C repo rev-parse two | \\\n+\t\tgit -C repo commit-graph write --stdin-commits\n+'\n+\n+test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n+\t\t$(git -C repo rev-parse one) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo rev-parse two^{tree}) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo rev-parse one^{tree}) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352033","messageId":"CAGZ79kaE_42A2B=VjRcNdJzueeVZmrsj6iNgk=6u9bi6pA4hQA@mail.gmail.com","threadId":"48756","inReplyTo":"9769e2a7471c2b32185896e833cc74824ac5437e.1531168854.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/6] commit-graph: refactor preparing commit graph","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-09T21:41:23Z","receivedAt":"2018-07-09T21:41:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jonathan,\nOn Mon, Jul 9, 2018 at 1:44 PM Jonathan Tan wrote:\n>\n> Two functions in the code (1) check if the repository is configured for\n> commit graphs, (2) call prepare_commit_graph(), and (3) check if the\n> graph exists. Move (1) and (3) into prepare_commit_graph(), reducing\n> duplication of code.\n>\n> Signed-off-by: Jonathan Tan \n\n\n\n> static int prepare_commit_graph_run_once = 0;\n> -static void prepare_commit_graph(void)\n> +\n> +/*\n> + * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n> + *\n> + * On the first invocation, this function attemps to load the commit\n> + * graph if the_repository is configured to have one.\n\nand as we talk about in-memory commit graph (and not some\nstale file that may still be around on the fs), we can assertly return\n0 when core_commit_graph is false.\n\nMakes sense!\n\n> @@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n>\n> int parse_commit_in_graph(struct commit *item)\n> {\n> - if (!core_commit_graph)\n> + if (!prepare_commit_graph())\n> return 0;\n> -\n> - prepare_commit_graph();\n> - if (commit_graph)\n> - return parse_commit_in_graph_one(commit_graph, item);\n> - return 0;\n> + return parse_commit_in_graph_one(commit_graph, item);\n\nMakes sense.\n\n> }\n>\n> void load_commit_graph_info(struct commit *item)\n> {\n> uint32_t pos;\n> - if (!core_commit_graph)\n> + if (!prepare_commit_graph())\n> return;\n> - prepare_commit_graph();\n> - if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n> + if (find_commit_in_graph(item, commit_graph, &pos))\n> fill_commit_graph_info(item, commit_graph, pos);\n\nhere too,\n\nThis is\nReviewed-by: Stefan Beller \n"},{"id":"352035","messageId":"CAGZ79kadsaoiM4o+GJQGX=HF2nLrgcLfHbvarvF+22cNDsn95w@mail.gmail.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-09T21:47:05Z","receivedAt":"2018-07-09T21:47:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jonathan,\n\n> This is on ds/commit-graph-fsck.\n>\n[...]\n> I've also added a patch (patch 1) that removes some duplication of\n> implementation that Junio talked about in [1].\n\nI think this series is good;\nThanks,\nStefan\n"},{"id":"352039","messageId":"xmqq4lh8avus.fsf@gitster-ct.c.googlers.com","threadId":"48756","inReplyTo":"cover.1531168854.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T22:27:07Z","receivedAt":"2018-07-09T22:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This is on ds/commit-graph-fsck.\n>\n> I saw that ds/commit-graph-fsck has been updated to the latest version\n> (v7, including \"gc.writeCommitGraph\"), so I've rebased my changes on top\n> of that branch. There were some mechanical changes needed during the\n> rebase, so I'm sending the rebased patches out.\n>\n> I've also added a patch (patch 1) that removes some duplication of\n> implementation that Junio talked about in [1].\n>\n> [1] https://public-inbox.org/git/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/\n\nWhile attempting to merge this topic to 'pu', I noticed that you and\nDerrick are perhaps playing a game of whack-a-mole by you getting\nrid of core_commit_graph global and making it a per in-core\nrepository instance, while Derrick adding core_multi_pack_index,\nmaking it necessary for yet another round of similar clean-up?\n\n\n\n\n\n"},{"id":"352052","messageId":"45e838b8-7741-bc6a-f0e5-d7604639a006@gmail.com","threadId":"48756","inReplyTo":"CAGZ79kaE_42A2B=VjRcNdJzueeVZmrsj6iNgk=6u9bi6pA4hQA@mail.gmail.com","subject":"Re: [PATCH v2 1/6] commit-graph: refactor preparing commit graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-10T00:23:08Z","receivedAt":"2018-07-10T00:23:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2018 5:41 PM, Stefan Beller wrote:\n> Hi Jonathan,\n> On Mon, Jul 9, 2018 at 1:44 PM Jonathan Tan wrote:\n>> Two functions in the code (1) check if the repository is configured for\n>> commit graphs, (2) call prepare_commit_graph(), and (3) check if the\n>> graph exists. Move (1) and (3) into prepare_commit_graph(), reducing\n>> duplication of code.\n>>\n>> Signed-off-by: Jonathan Tan \n>\n>\n>> static int prepare_commit_graph_run_once = 0;\n>> -static void prepare_commit_graph(void)\n>> +\n>> +/*\n>> + * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n>> + *\n>> + * On the first invocation, this function attemps to load the commit\n>> + * graph if the_repository is configured to have one.\n> and as we talk about in-memory commit graph (and not some\n> stale file that may still be around on the fs), we can assertly return\n> 0 when core_commit_graph is false.\n>\n> Makes sense!\n>\n>> @@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n>>\n>> int parse_commit_in_graph(struct commit *item)\n>> {\n>> - if (!core_commit_graph)\n>> + if (!prepare_commit_graph())\n>> return 0;\n>> -\n>> - prepare_commit_graph();\n>> - if (commit_graph)\n>> - return parse_commit_in_graph_one(commit_graph, item);\n>> - return 0;\n>> + return parse_commit_in_graph_one(commit_graph, item);\n> Makes sense.\n>\n>> }\n>>\n>> void load_commit_graph_info(struct commit *item)\n>> {\n>> uint32_t pos;\n>> - if (!core_commit_graph)\n>> + if (!prepare_commit_graph())\n>> return;\n>> - prepare_commit_graph();\n>> - if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n>> + if (find_commit_in_graph(item, commit_graph, &pos))\n>> fill_commit_graph_info(item, commit_graph, pos);\n> here too,\n>\n> This is\n> Reviewed-by: Stefan Beller \n\nThese changes make sense. Thanks!\n\n"},{"id":"352053","messageId":"3ced0e1e-1312-2f3b-bb67-fad3525e5500@gmail.com","threadId":"48756","inReplyTo":"xmqq4lh8avus.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-10T00:30:10Z","receivedAt":"2018-07-10T00:30:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2018 6:27 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> This is on ds/commit-graph-fsck.\n>>\n>> I saw that ds/commit-graph-fsck has been updated to the latest version\n>> (v7, including \"gc.writeCommitGraph\"), so I've rebased my changes on top\n>> of that branch. There were some mechanical changes needed during the\n>> rebase, so I'm sending the rebased patches out.\n>>\n>> I've also added a patch (patch 1) that removes some duplication of\n>> implementation that Junio talked about in [1].\n>>\n>> [1] https://public-inbox.org/git/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/\n> While attempting to merge this topic to 'pu', I noticed that you and\n> Derrick are perhaps playing a game of whack-a-mole by you getting\n> rid of core_commit_graph global and making it a per in-core\n> repository instance, while Derrick adding core_multi_pack_index,\n> making it necessary for yet another round of similar clean-up?\n\nWe did have collisions with Jonathan's v1, but this v2 is on my latest \ncommit-graph things so should not have conflicts.\n\nThe core_commit_graph variable appears to still be global (do we have \nconfig storage in the_repository yet?) so core_multi_pack_index is similar.\n\nI do put the multi_pack_index pointer inside the_repository->objects, so \nthe equivalent of this series will not be necessary for the MIDX series.\n\n\nThis series looks good to me, so please add \"Reviewed-by: Derrick Stolee \n\"\n\nI think we are set for another series on top of this one that lets the \ncommit-graph feature handle arbitrary repositories (pass a 'struct \nrepository *r' in all of the functions).\n\nThanks,\n\n-Stolee\n\n"},{"id":"352054","messageId":"b5733625-29c8-4317-ff44-d27c2fca11ce@gmail.com","threadId":"48756","inReplyTo":"3ced0e1e-1312-2f3b-bb67-fad3525e5500@gmail.com","subject":"Re: [PATCH v2 on ds/commit-graph-fsck 0/6] Object store refactoring: commit graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-10T00:32:51Z","receivedAt":"2018-07-10T00:33:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\nOn 7/9/2018 8:30 PM, Derrick Stolee wrote:\n> On 7/9/2018 6:27 PM, Junio C Hamano wrote:\n>> Jonathan Tan writes:\n>>\n>>> This is on ds/commit-graph-fsck.\n>>>\n>>> I saw that ds/commit-graph-fsck has been updated to the latest version\n>>> (v7, including \"gc.writeCommitGraph\"), so I've rebased my changes on \n>>> top\n>>> of that branch. There were some mechanical changes needed during the\n>>> rebase, so I'm sending the rebased patches out.\n>>>\n>>> I've also added a patch (patch 1) that removes some duplication of\n>>> implementation that Junio talked about in [1].\n>>>\n>>> [1] \n>>> https://public-inbox.org/git/xmqqefgtmrgi.fsf@gitster-ct.c.googlers.com/ \n>>>\n>> While attempting to merge this topic to 'pu', I noticed that you and\n>> Derrick are perhaps playing a game of whack-a-mole by you getting\n>> rid of core_commit_graph global and making it a per in-core\n>> repository instance, while Derrick adding core_multi_pack_index,\n>> making it necessary for yet another round of similar clean-up?\n>\n> We did have collisions with Jonathan's v1, but this v2 is on my latest \n> commit-graph things so should not have conflicts.\n>\n> The core_commit_graph variable appears to still be global (do we have \n> config storage in the_repository yet?) so core_multi_pack_index is \n> similar.\n>\n> I do put the multi_pack_index pointer inside the_repository->objects, \n> so the equivalent of this series will not be necessary for the MIDX \n> series.\n>\n>\n> This series looks good to me, so please add \"Reviewed-by: Derrick \n> Stolee \"\n>\n> I think we are set for another series on top of this one that lets the \n> commit-graph feature handle arbitrary repositories (pass a 'struct \n> repository *r' in all of the functions).\n>\n\nOf course, after I send this message I see that my inbox had poorly \nthreaded this patch and item 6/6 was below these messages. Patch 6/6 \ndoes convert this variable and passes an arbitrary repository.\n\nI'll update the MIDX patch in v4 to use that model for the config setting.\n\n"},{"id":"352055","messageId":"75ef9935-342a-bbda-4ce6-e5a033f273a9@gmail.com","threadId":"48756","inReplyTo":"f1ccfdccc851039cf62978d162cd9de99ea619de.1531168854.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-10T00:48:22Z","receivedAt":"2018-07-10T00:48:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/9/2018 4:44 PM, Jonathan Tan wrote:\n> Add a struct repository argument to the functions in commit-graph.h that\n> read the commit graph. (This commit does not affect functions that write\n> commit graphs.)\n>\n> Because the commit graph functions can now read the commit graph of any\n> repository, the global variable core_commit_graph has been removed.\n> Instead, the config option core.commitGraph is now read on the first\n> time in a repository that a commit is attempted to be parsed using its\n> commit graph.\n>\n> This commit includes a test that exercises the functionality on an\n> arbitrary repository that is not the_repository.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Makefile | 1 +\n> builtin/fsck.c | 2 +-\n> cache.h | 1 -\n> commit-graph.c | 60 ++++++++++++++------------\n> commit-graph.h | 7 +--\n> commit.c | 6 +--\n> config.c | 5 ---\n> environment.c | 1 -\n> ref-filter.c | 2 +-\n> t/helper/test-repository.c | 88 ++++++++++++++++++++++++++++++++++++++\n> t/helper/test-tool.c | 1 +\n> t/helper/test-tool.h | 1 +\n> t/t5318-commit-graph.sh | 35 +++++++++++++++\n> 13 files changed, 168 insertions(+), 42 deletions(-)\n> create mode 100644 t/helper/test-repository.c\n>\n> diff --git a/Makefile b/Makefile\n> index 0cb6590f24..bb8bd67201 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n> TEST_BUILTINS_OBJS += test-read-cache.o\n> TEST_BUILTINS_OBJS += test-ref-store.o\n> TEST_BUILTINS_OBJS += test-regex.o\n> +TEST_BUILTINS_OBJS += test-repository.o\n> TEST_BUILTINS_OBJS += test-revision-walking.o\n> TEST_BUILTINS_OBJS += test-run-command.o\n> TEST_BUILTINS_OBJS += test-scrap-cache-tree.o\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index eca7900ee0..2abfb2d782 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -825,7 +825,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n> \n> \tcheck_connectivity();\n> \n> -\tif (core_commit_graph) {\n> +\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n> \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n> \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n> \n> diff --git a/cache.h b/cache.h\n> index d49092d94d..8dc59bedba 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -813,7 +813,6 @@ extern char *git_replace_ref_base;\n> \n> extern int fsync_object_files;\n> extern int core_preload_index;\n> -extern int core_commit_graph;\n> extern int core_apply_sparse_checkout;\n> extern int precomposed_unicode;\n> extern int protect_hfs;\n> diff --git a/commit-graph.c b/commit-graph.c\n> index af97a10603..8eab199b1b 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -183,15 +183,15 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n> \texit(1);\n> }\n> \n> -static void prepare_commit_graph_one(const char *obj_dir)\n> +static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n> {\n> \tchar *graph_name;\n> \n> -\tif (the_repository->objects->commit_graph)\n> +\tif (r->objects->commit_graph)\n> \t\treturn;\n> \n> \tgraph_name = get_commit_graph_filename(obj_dir);\n> -\tthe_repository->objects->commit_graph =\n> +\tr->objects->commit_graph =\n> \t\tload_commit_graph_one(graph_name);\n> \n> \tFREE_AND_NULL(graph_name);\n> @@ -203,26 +203,34 @@ static void prepare_commit_graph_one(const char *obj_dir)\n> * On the first invocation, this function attemps to load the commit\n> * graph if the_repository is configured to have one.\n> */\n> -static int prepare_commit_graph(void)\n> +static int prepare_commit_graph(struct repository *r)\n> {\n> \tstruct alternate_object_database *alt;\n> \tchar *obj_dir;\n> +\tint config_value;\n> \n> -\tif (the_repository->objects->commit_graph_attempted)\n> -\t\treturn !!the_repository->objects->commit_graph;\n> -\tthe_repository->objects->commit_graph_attempted = 1;\n> +\tif (r->objects->commit_graph_attempted)\n> +\t\treturn !!r->objects->commit_graph;\n> +\tr->objects->commit_graph_attempted = 1;\n> \n> -\tif (!core_commit_graph)\n> +\tif (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n> +\t !config_value)\n> +\t\t/*\n> +\t\t * This repository is not configured to use commit graphs, so\n> +\t\t * do not load one. (But report commit_graph_attempted anyway\n> +\t\t * so that commit graph loading is not attempted again for this\n> +\t\t * repository.)\n> +\t\t */\n\nI reacted first to complain about this extra config lookup, but it is \nonly run once per repository, so that should be fine.\n\n> \t\treturn 0;\n> \n> -\tobj_dir = get_object_directory();\n> -\tprepare_commit_graph_one(obj_dir);\n> -\tprepare_alt_odb(the_repository);\n> -\tfor (alt = the_repository->objects->alt_odb_list;\n> -\t !the_repository->objects->commit_graph && alt;\n> +\tobj_dir = r->objects->objectdir;\n> +\tprepare_commit_graph_one(r, obj_dir);\n> +\tprepare_alt_odb(r);\n> +\tfor (alt = r->objects->alt_odb_list;\n> +\t !r->objects->commit_graph && alt;\n> \t alt = alt->next)\n> -\t\tprepare_commit_graph_one(alt->path);\n> -\treturn !!the_repository->objects->commit_graph;\n> +\t\tprepare_commit_graph_one(r, alt->path);\n> +\treturn !!r->objects->commit_graph;\n> }\n> \n> static void close_commit_graph(void)\n> @@ -323,8 +331,6 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n> {\n> \tuint32_t pos;\n> \n> -\tif (!core_commit_graph)\n> -\t\treturn 0;\n> \tif (item->object.parsed)\n> \t\treturn 1;\n> \n> @@ -334,20 +340,20 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n> \treturn 0;\n> }\n> \n> -int parse_commit_in_graph(struct commit *item)\n> +int parse_commit_in_graph(struct repository *r, struct commit *item)\n> {\n> -\tif (!prepare_commit_graph())\n> +\tif (!prepare_commit_graph(r))\n> \t\treturn 0;\n> -\treturn parse_commit_in_graph_one(the_repository->objects->commit_graph, item);\n> +\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n> }\n> \n> -void load_commit_graph_info(struct commit *item)\n> +void load_commit_graph_info(struct repository *r, struct commit *item)\n> {\n> \tuint32_t pos;\n> -\tif (!prepare_commit_graph())\n> +\tif (!prepare_commit_graph(r))\n> \t\treturn;\n> -\tif (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))\n> -\t\tfill_commit_graph_info(item, the_repository->objects->commit_graph, pos);\n> +\tif (find_commit_in_graph(item, r->objects->commit_graph, &pos))\n> +\t\tfill_commit_graph_info(item, r->objects->commit_graph, pos);\n> }\n> \n> static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)\n> @@ -373,9 +379,9 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,\n> \treturn load_tree_for_commit(g, (struct commit *)c);\n> }\n> \n> -struct tree *get_commit_tree_in_graph(const struct commit *c)\n> +struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit *c)\n> {\n> -\treturn get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);\n> +\treturn get_commit_tree_in_graph_one(r->objects->commit_graph, c);\n> }\n> \n> static void write_graph_chunk_fanout(struct hashfile *f,\n> @@ -691,7 +697,7 @@ void write_commit_graph(const char *obj_dir,\n> \toids.alloc = approximate_object_count() / 4;\n> \n> \tif (append) {\n> -\t\tprepare_commit_graph_one(obj_dir);\n> +\t\tprepare_commit_graph_one(the_repository, obj_dir);\n> \t\tif (the_repository->objects->commit_graph)\n> \t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n> \t}\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 94defb04a9..76e098934a 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -19,7 +19,7 @@ char *get_commit_graph_filename(const char *obj_dir);\n> *\n> * See parse_commit_buffer() for the fallback after this call.\n> */\n> -int parse_commit_in_graph(struct commit *item);\n> +int parse_commit_in_graph(struct repository *r, struct commit *item);\n> \n> /*\n> * It is possible that we loaded commit contents from the commit buffer,\n> @@ -27,9 +27,10 @@ int parse_commit_in_graph(struct commit *item);\n> * checked and filled. Fill the graph_pos and generation members of\n> * the given commit.\n> */\n> -void load_commit_graph_info(struct commit *item);\n> +void load_commit_graph_info(struct repository *r, struct commit *item);\n> \n> -struct tree *get_commit_tree_in_graph(const struct commit *c);\n> +struct tree *get_commit_tree_in_graph(struct repository *r,\n> +\t\t\t\t const struct commit *c);\n> \n> struct commit_graph {\n> \tint graph_fd;\n> diff --git a/commit.c b/commit.c\n> index 598cf21cee..b3bbfefc27 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -319,7 +319,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n> \tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n> \t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n> \n> -\treturn get_commit_tree_in_graph(commit);\n> +\treturn get_commit_tree_in_graph(the_repository, commit);\n> }\n> \n> struct object_id *get_commit_tree_oid(const struct commit *commit)\n> @@ -413,7 +413,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \titem->date = parse_commit_date(bufptr, tail);\n> \n> \tif (check_graph)\n> -\t\tload_commit_graph_info(item);\n> +\t\tload_commit_graph_info(the_repository, item);\n> \n> \treturn 0;\n> }\n> @@ -429,7 +429,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n> \t\treturn -1;\n> \tif (item->object.parsed)\n> \t\treturn 0;\n> -\tif (use_commit_graph && parse_commit_in_graph(item))\n> +\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n> \t\treturn 0;\n> \tbuffer = read_object_file(&item->object.oid, &type, &size);\n> \tif (!buffer)\n> diff --git a/config.c b/config.c\n> index a0a6ae1980..fcf863b667 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1308,11 +1308,6 @@ static int git_default_core_config(const char *var, const char *value)\n> \t\treturn 0;\n> \t}\n> \n> -\tif (!strcmp(var, \"core.commitgraph\")) {\n> -\t\tcore_commit_graph = git_config_bool(var, value);\n> -\t\treturn 0;\n> -\t}\n> -\n> \tif (!strcmp(var, \"core.sparsecheckout\")) {\n> \t\tcore_apply_sparse_checkout = git_config_bool(var, value);\n> \t\treturn 0;\n> diff --git a/environment.c b/environment.c\n> index 2a6de2330b..d6e5b39425 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n> enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n> char *notes_ref_name;\n> int grafts_replace_parents = 1;\n> -int core_commit_graph;\n> int core_apply_sparse_checkout;\n> int merge_log_config = -1;\n> int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n> diff --git a/ref-filter.c b/ref-filter.c\n> index fa3685d91f..889d97b005 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1710,7 +1710,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,\n> \n> \tfor (p = want; p; p = p->next) {\n> \t\tstruct commit *c = p->item;\n> -\t\tload_commit_graph_info(c);\n> +\t\tload_commit_graph_info(the_repository, c);\n> \t\tif (c->generation < cutoff)\n> \t\t\tcutoff = c->generation;\n> \t}\n\nThe changes above are mostly obvious. They don't need too much reading \nto know they are correct.\n\nThe tests below form a decently-large patch on their own. Perhaps split \nthem out so it is easier to know that we have some interesting things to \ncheck here.\n\nIt's worth spending some extra time looking at this test pattern as I \nbelieve we will want to follow it with other arbitrary repository changes.\n\n\n> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\n> new file mode 100644\n> index 0000000000..5fff540a26\n> --- /dev/null\n> +++ b/t/helper/test-repository.c\n> @@ -0,0 +1,88 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +#include \"commit-graph.h\"\n> +#include \"commit.h\"\n> +#include \"config.h\"\n> +#include \"object-store.h\"\n> +#include \"object.h\"\n> +#include \"repository.h\"\n> +#include \"tree.h\"\n> +\n> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> +\t\t\t\t const struct object_id *commit_oid)\n> +{\n> +\tstruct repository r;\n> +\tstruct commit *c;\n> +\tstruct commit_list *parent;\n> +\n> +\t/*\n> +\t * Create a commit independent of any repository.\n> +\t */\n> +\tc = lookup_commit(commit_oid);\n> +\n> +\trepo_init(&r, gitdir, worktree);\n> +\n> +\tif (!parse_commit_in_graph(&r, c))\n> +\t\tdie(\"Couldn't parse commit\");\n> +\n> +\tprintf(\"%lu\", c->date);\n> +\tfor (parent = c->parents; parent; parent = parent->next)\n> +\t\tprintf(\" %s\", oid_to_hex(&parent->item->object.oid));\n> +\tprintf(\"\\n\");\n> +\n> +\trepo_clear(&r);\n> +}\n> +\n> +static void test_get_commit_tree_in_graph(const char *gitdir,\n> +\t\t\t\t\t const char *worktree,\n> +\t\t\t\t\t const struct object_id *commit_oid)\n> +{\n> +\tstruct repository r;\n> +\tstruct commit *c;\n> +\tstruct tree *tree;\n> +\n> +\t/*\n> +\t * Create a commit independent of any repository.\n> +\t */\n> +\tc = lookup_commit(commit_oid);\n\nWould this be more accurate to say we are creating a commit object \nstored in the object cache of the_repository? How would you expect this \nto work if/when lookup_commit() takes an arbitrary repository? You want \nto provide &r, right (after initializing)?\n\nAlso, this will conflict with sb/object-store-lookup, won't it? I'm \nguessing this is why you didn't touch the \"git commit-graph \n[write|verify]\"code paths.\n\n> +\n> +\trepo_init(&r, gitdir, worktree);\n\nI think you want to move the lookup_commit() to after this.\n\n> +\n> +\t/*\n> +\t * get_commit_tree_in_graph does not automatically parse the commit, so\n> +\t * parse it first.\n> +\t */\n> +\tif (!parse_commit_in_graph(&r, c))\n> +\t\tdie(\"Couldn't parse commit\");\n> +\ttree = get_commit_tree_in_graph(&r, c);\n> +\tif (!tree)\n> +\t\tdie(\"Couldn't get commit tree\");\n> +\n> +\tprintf(\"%s\\n\", oid_to_hex(&tree->object.oid));\n> +\n> +\trepo_clear(&r);\n> +}\n> +\n> +int cmd__repository(int argc, const char **argv)\n> +{\n> +\tif (argc < 2)\n> +\t\tdie(\"must have at least 2 arguments\");\n\nI think this \"test-tool repository \" pattern is a good way to get \nsome testing here.\n\n> +\tif (!strcmp(argv[1], \"parse_commit_in_graph\")) {\n> +\t\tstruct object_id oid;\n> +\t\tif (argc < 5)\n> +\t\t\tdie(\"not enough arguments\");\n> +\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n> +\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n> +\t\ttest_parse_commit_in_graph(argv[2], argv[3], &oid);\n> +\t} else if (!strcmp(argv[1], \"get_commit_tree_in_graph\")) {\n> +\t\tstruct object_id oid;\n> +\t\tif (argc < 5)\n> +\t\t\tdie(\"not enough arguments\");\n> +\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n> +\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n> +\t\ttest_get_commit_tree_in_graph(argv[2], argv[3], &oid);\n> +\t} else {\n> +\t\tdie(\"unrecognized '%s'\", argv[1]);\n> +\t}\n> +\treturn 0;\n> +}\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 805a45de9c..dafc91c240 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {\n> \t{ \"read-cache\", cmd__read_cache },\n> \t{ \"ref-store\", cmd__ref_store },\n> \t{ \"regex\", cmd__regex },\n> +\t{ \"repository\", cmd__repository },\n> \t{ \"revision-walking\", cmd__revision_walking },\n> \t{ \"run-command\", cmd__run_command },\n> \t{ \"scrap-cache-tree\", cmd__scrap_cache_tree },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 7116ddfb94..80cbcf0857 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -23,6 +23,7 @@ int cmd__prio_queue(int argc, const char **argv);\n> int cmd__read_cache(int argc, const char **argv);\n> int cmd__ref_store(int argc, const char **argv);\n> int cmd__regex(int argc, const char **argv);\n> +int cmd__repository(int argc, const char **argv);\n> int cmd__revision_walking(int argc, const char **argv);\n> int cmd__run_command(int argc, const char **argv);\n> int cmd__scrap_cache_tree(int argc, const char **argv);\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 5947de3d24..4f17d7701e 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -431,4 +431,39 @@ test_expect_success 'git fsck (checks commit-graph)' '\n> \ttest_must_fail git fsck\n> '\n> \n> +test_expect_success 'setup non-the_repository tests' '\n> +\trm -rf repo &&\n> +\tgit init repo &&\n> +\ttest_commit -C repo one &&\n> +\ttest_commit -C repo two &&\n> +\tgit -C repo config core.commitGraph true &&\n> +\tgit -C repo rev-parse two | \\\n> +\t\tgit -C repo commit-graph write --stdin-commits\n> +'\n> +\n> +test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n> +\ttest-tool repository parse_commit_in_graph \\\n> +\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n> +\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n> +\t\t$(git -C repo rev-parse one) >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest-tool repository parse_commit_in_graph \\\n> +\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n> +\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n> +\ttest-tool repository get_commit_tree_in_graph \\\n> +\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n> +\techo $(git -C repo rev-parse two^{tree}) >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest-tool repository get_commit_tree_in_graph \\\n> +\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n> +\techo $(git -C repo rev-parse one^{tree}) >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"352086","messageId":"20180710115344.14392-1-szeder.dev@gmail.com","threadId":"48756","inReplyTo":"f1ccfdccc851039cf62978d162cd9de99ea619de.1531168854.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-10T11:53:44Z","receivedAt":"2018-07-10T11:53:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\n> new file mode 100644\n> index 0000000000..5fff540a26\n> --- /dev/null\n> +++ b/t/helper/test-repository.c\n> @@ -0,0 +1,88 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +#include \"commit-graph.h\"\n> +#include \"commit.h\"\n> +#include \"config.h\"\n> +#include \"object-store.h\"\n> +#include \"object.h\"\n> +#include \"repository.h\"\n> +#include \"tree.h\"\n> +\n> +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> +\t\t\t\t const struct object_id *commit_oid)\n> +{\n> +\tstruct repository r;\n> +\tstruct commit *c;\n> +\tstruct commit_list *parent;\n> +\n> +\t/*\n> +\t * Create a commit independent of any repository.\n> +\t */\n> +\tc = lookup_commit(commit_oid);\n> +\n> +\trepo_init(&r, gitdir, worktree);\n> +\n> +\tif (!parse_commit_in_graph(&r, c))\n> +\t\tdie(\"Couldn't parse commit\");\n> +\n> +\tprintf(\"%lu\", c->date);\n\n32-bit builds complain about this:\n\n t/helper/test-repository.c: In function 'test_parse_commit_in_graph':\n 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=]\n printf(\"%lu\", c->date);\n ^\n cc1: all warnings being treated as errors\n Makefile:2262: recipe for target 't/helper/test-repository.o' failed\n make: *** [t/helper/test-repository.o] Error 1\n\n> +\tfor (parent = c->parents; parent; parent = parent->next)\n> +\t\tprintf(\" %s\", oid_to_hex(&parent->item->object.oid));\n> +\tprintf(\"\\n\");\n> +\n> +\trepo_clear(&r);\n> +}\n"},{"id":"352108","messageId":"nycvar.QRO.7.76.6.1807101517290.75@tvgsbejvaqbjf.bet","threadId":"48756","inReplyTo":"20180710115344.14392-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-10T13:18:19Z","receivedAt":"2018-07-10T13:18:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 10 Jul 2018, SZEDER Gábor wrote:\n\n> > diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\n> > new file mode 100644\n> > index 0000000000..5fff540a26\n> > --- /dev/null\n> > +++ b/t/helper/test-repository.c\n> > @@ -0,0 +1,88 @@\n> > +#include \"test-tool.h\"\n> > +#include \"cache.h\"\n> > +#include \"commit-graph.h\"\n> > +#include \"commit.h\"\n> > +#include \"config.h\"\n> > +#include \"object-store.h\"\n> > +#include \"object.h\"\n> > +#include \"repository.h\"\n> > +#include \"tree.h\"\n> > +\n> > +static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> > +\t\t\t\t const struct object_id *commit_oid)\n> > +{\n> > +\tstruct repository r;\n> > +\tstruct commit *c;\n> > +\tstruct commit_list *parent;\n> > +\n> > +\t/*\n> > +\t * Create a commit independent of any repository.\n> > +\t */\n> > +\tc = lookup_commit(commit_oid);\n> > +\n> > +\trepo_init(&r, gitdir, worktree);\n> > +\n> > +\tif (!parse_commit_in_graph(&r, c))\n> > +\t\tdie(\"Couldn't parse commit\");\n> > +\n> > +\tprintf(\"%lu\", c->date);\n> \n> 32-bit builds complain about this:\n> \n> t/helper/test-repository.c: In function 'test_parse_commit_in_graph':\n> 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=]\n> printf(\"%lu\", c->date);\n> ^\n> cc1: all warnings being treated as errors\n> Makefile:2262: recipe for target 't/helper/test-repository.o' failed\n> make: *** [t/helper/test-repository.o] Error 1\n\nLet's also state how we usually fix this:\n\n\tprintf(\"%\"PRItime, c->date);\n\nCiao,\nDscho"},{"id":"352143","messageId":"CAGf8dgK3bWgAEE6Fuck=BHUG47V0kXi-Ph_gJnhk0mz2_84h3g@mail.gmail.com","threadId":"48756","inReplyTo":"nycvar.QRO.7.76.6.1807101517290.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-10T17:23:55Z","receivedAt":"2018-07-10T18:18:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, Jul 10, 2018 at 6:18 AM, Johannes Schindelin\n wrote:\n>> 32-bit builds complain about this:\n>>\n>> t/helper/test-repository.c: In function 'test_parse_commit_in_graph':\n>> 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=]\n>> printf(\"%lu\", c->date);\n>> ^\n>> cc1: all warnings being treated as errors\n>> Makefile:2262: recipe for target 't/helper/test-repository.o' failed\n>> make: *** [t/helper/test-repository.o] Error 1\n>\n> Let's also state how we usually fix this:\n>\n> printf(\"%\"PRItime, c->date);\n\nThanks for noticing this - I'll do this if a reroll is needed.\n"},{"id":"352157","messageId":"20180710173147.104757-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"75ef9935-342a-bbda-4ce6-e5a033f273a9@gmail.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-10T17:31:47Z","receivedAt":"2018-07-10T18:41:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > -\tif (!core_commit_graph)\n> > +\tif (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n> > +\t !config_value)\n> > +\t\t/*\n> > +\t\t * This repository is not configured to use commit graphs, so\n> > +\t\t * do not load one. (But report commit_graph_attempted anyway\n> > +\t\t * so that commit graph loading is not attempted again for this\n> > +\t\t * repository.)\n> > +\t\t */\n> \n> I reacted first to complain about this extra config lookup, but it is \n> only run once per repository, so that should be fine.\n\nThanks for checking. It is indeed run at most once per repository, and\nonly if a commit graph is requested - the same as the current code.\n\n> The tests below form a decently-large patch on their own. Perhaps split \n> them out so it is easier to know that we have some interesting things to \n> check here.\n\nThe patch is 168+ 42-, which doesn't seem that large to me, but I'll do\nthis if others think that it is large too.\n\n> It's worth spending some extra time looking at this test pattern as I \n> believe we will want to follow it with other arbitrary repository changes.\n\nI agree - let me know if you notice anything you think should be\nchanged.\n\n> > +static void test_get_commit_tree_in_graph(const char *gitdir,\n> > +\t\t\t\t\t const char *worktree,\n> > +\t\t\t\t\t const struct object_id *commit_oid)\n> > +{\n> > +\tstruct repository r;\n> > +\tstruct commit *c;\n> > +\tstruct tree *tree;\n> > +\n> > +\t/*\n> > +\t * Create a commit independent of any repository.\n> > +\t */\n> > +\tc = lookup_commit(commit_oid);\n> \n> Would this be more accurate to say we are creating a commit object \n> stored in the object cache of the_repository? How would you expect this \n> to work if/when lookup_commit() takes an arbitrary repository? You want \n> to provide &r, right (after initializing)?\n\nYes, you're right - Stefan too mentioned that this will need to be moved\nbelow lookup_commit(). I'm not sure what the best way is to handle this\n- maybe move this, and add a \"needswork\" stating that we need to pass r\nto lookup_commit once it supports taking in a repository argument, as an\naid to the person who performs the merge. I'll do that if a reroll is\nneeded.\n\n> Also, this will conflict with sb/object-store-lookup, won't it? I'm \n> guessing this is why you didn't touch the \"git commit-graph \n> [write|verify]\"code paths.\n\nIt will conflict because of the change to lookup_commit(), but the only\nnew code I'm writing is in t/helper/test-repository.c, so hopefully the\nmerge won't be too tedious. The main reason why I didn't touch the\nwriting/verifying part is to reduce the size of this patch set, and\nbecause that change is not needed to update parse_commit() and others.\n\n> > +\n> > +\trepo_init(&r, gitdir, worktree);\n> \n> I think you want to move the lookup_commit() to after this.\n\nYes, that's right.\n\n> > +int cmd__repository(int argc, const char **argv)\n> > +{\n> > +\tif (argc < 2)\n> > +\t\tdie(\"must have at least 2 arguments\");\n> \n> I think this \"test-tool repository \" pattern is a good way to get \n> some testing here.\n\nThanks.\n"},{"id":"352291","messageId":"1f0ab606-cf7d-e61c-1761-7a2982a2628e@gmail.com","threadId":"48756","inReplyTo":"20180710173147.104757-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-11T19:41:42Z","receivedAt":"2018-07-11T19:41:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/10/2018 1:31 PM, Jonathan Tan wrote:\n>\n>>> +static void test_get_commit_tree_in_graph(const char *gitdir,\n>>> +\t\t\t\t\t const char *worktree,\n>>> +\t\t\t\t\t const struct object_id *commit_oid)\n>>> +{\n>>> +\tstruct repository r;\n>>> +\tstruct commit *c;\n>>> +\tstruct tree *tree;\n>>> +\n>>> +\t/*\n>>> +\t * Create a commit independent of any repository.\n>>> +\t */\n>>> +\tc = lookup_commit(commit_oid);\n>> Would this be more accurate to say we are creating a commit object\n>> stored in the object cache of the_repository? How would you expect this\n>> to work if/when lookup_commit() takes an arbitrary repository? You want\n>> to provide &r, right (after initializing)?\n> Yes, you're right - Stefan too mentioned that this will need to be moved\n> below lookup_commit(). I'm not sure what the best way is to handle this\n> - maybe move this, and add a \"needswork\" stating that we need to pass r\n> to lookup_commit once it supports taking in a repository argument, as an\n> aid to the person who performs the merge. I'll do that if a reroll is\n> needed.\n>\n>> Also, this will conflict with sb/object-store-lookup, won't it? I'm\n>> guessing this is why you didn't touch the \"git commit-graph\n>> [write|verify]\"code paths.\n> It will conflict because of the change to lookup_commit(), but the only\n> new code I'm writing is in t/helper/test-repository.c, so hopefully the\n> merge won't be too tedious. The main reason why I didn't touch the\n> writing/verifying part is to reduce the size of this patch set, and\n> because that change is not needed to update parse_commit() and others.\n\nI guess my main complaint is that this won't be an actual \"merge\" \nconflict, but the result will not compile. Since Stefan already has a \nseries out that changes this method, I recommend basing your series on \nit (in addition to basing it on ds/commit-graph-fsck).\n\nThanks,\n\n-Stolee\n\n"},{"id":"352299","messageId":"20180711210842.17443-1-jonathantanmy@google.com","threadId":"48756","inReplyTo":"1f0ab606-cf7d-e61c-1761-7a2982a2628e@gmail.com","subject":"Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T21:08:42Z","receivedAt":"2018-07-11T21:08:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> >> Also, this will conflict with sb/object-store-lookup, won't it? I'm\n> >> guessing this is why you didn't touch the \"git commit-graph\n> >> [write|verify]\"code paths.\n> > It will conflict because of the change to lookup_commit(), but the only\n> > new code I'm writing is in t/helper/test-repository.c, so hopefully the\n> > merge won't be too tedious. The main reason why I didn't touch the\n> > writing/verifying part is to reduce the size of this patch set, and\n> > because that change is not needed to update parse_commit() and others.\n> \n> I guess my main complaint is that this won't be an actual \"merge\" \n> conflict, but the result will not compile. Since Stefan already has a \n> series out that changes this method, I recommend basing your series on \n> it (in addition to basing it on ds/commit-graph-fsck).\n\nGood point. Junio requested a reroll in his What's Cooking e-mail [1],\nand the same e-mail states that ds/commit-graph-fsck and\nsb/object-store-lookup will be merged to next, so there are a few good\nreasons to base it on both. I'll do that and send out an updated version\nsoon.\n\n[1] https://public-inbox.org/git/xmqq7em138a5.fsf@gitster-ct.c.googlers.com/\n"},{"id":"352309","messageId":"cover.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1529616356.git.jonathantanmy@google.com","subject":"[PATCH v3 0/6] Object store refactoring: commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:36Z","receivedAt":"2018-07-11T22:42:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup,\nfollowing Stolee's suggestion.\n\n(It also seems better to build it this way to me, since both these\nbranches are going into \"next\" according to the latest What's Cooking.)\n\nJunio wrote in [1]:\n\n> I've added SQUASH??? patch at the tip of each of the above,\n> rebuilt 'pu' with them and pushed the result out. It seems that\n> Travis is happier with the result.\n>\n> Please do not forget to squash them in when/if rerolling. If there\n> is no need to change anything else other than squashing them, you\n> can tell me to which commit in your series the fix needs to be\n> squashed in (that would save me time to figure it out, obviously).\n\nI'm rerolling because I also need to update the last patch with the new\nlookup_commit() function signature that Stefan's sb/object-store-lookup\nintroduces. I have squashed the SQUASH??? patch into the corresponding\npatch in this patch set.\n\nChanges from v2:\n - now also based on sb/object-store-lookup in addition to\n ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto\n sb-object-store-lookup, then rebased this patch set onto the result)\n - patches 1-5 are unchanged\n - patch 6:\n - used \"PRItime\" instead of \"ul\" when printing a timestamp (the\n SQUASH??? patch)\n - updated invocations of lookup_commit() to take a repository object\n\n[1] https://public-inbox.org/git/xmqqpnzt1myi.fsf@gitster-ct.c.googlers.com/\n\nJonathan Tan (6):\n commit-graph: refactor preparing commit graph\n object-store: add missing include\n commit-graph: add missing forward declaration\n commit-graph: add free_commit_graph\n commit-graph: store graph in struct object_store\n commit-graph: add repo arg to graph readers\n\n Makefile | 1 +\n builtin/commit-graph.c | 2 +\n builtin/fsck.c | 2 +-\n cache.h | 1 -\n commit-graph.c | 108 +++++++++++++++++++++----------------\n commit-graph.h | 11 ++--\n commit.c | 6 +--\n config.c | 5 --\n environment.c | 1 -\n object-store.h | 6 +++\n object.c | 5 ++\n ref-filter.c | 2 +-\n t/helper/test-repository.c | 82 ++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 ++++++++++++\n 16 files changed, 207 insertions(+), 62 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352310","messageId":"2e221f35c9657377f98c53f437bb950e7d4c8a1b.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 1/6] commit-graph: refactor preparing commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:37Z","receivedAt":"2018-07-11T22:42:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Two functions in the code (1) check if the repository is configured for\ncommit graphs, (2) call prepare_commit_graph(), and (3) check if the\ngraph exists. Move (1) and (3) into prepare_commit_graph(), reducing\nduplication of code.\n\nSigned-off-by: Jonathan Tan \n---\n commit-graph.c | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 41a0133ff7..1ea701ed69 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -200,15 +200,25 @@ static void prepare_commit_graph_one(const char *obj_dir)\n }\n \n static int prepare_commit_graph_run_once = 0;\n-static void prepare_commit_graph(void)\n+\n+/*\n+ * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n+ *\n+ * On the first invocation, this function attemps to load the commit\n+ * graph if the_repository is configured to have one.\n+ */\n+static int prepare_commit_graph(void)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n \n \tif (prepare_commit_graph_run_once)\n-\t\treturn;\n+\t\treturn !!commit_graph;\n \tprepare_commit_graph_run_once = 1;\n \n+\tif (!core_commit_graph)\n+\t\treturn 0;\n+\n \tobj_dir = get_object_directory();\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb(the_repository);\n@@ -216,6 +226,7 @@ static void prepare_commit_graph(void)\n \t !commit_graph && alt;\n \t alt = alt->next)\n \t\tprepare_commit_graph_one(alt->path);\n+\treturn !!commit_graph;\n }\n \n static void close_commit_graph(void)\n@@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n \n int parse_commit_in_graph(struct commit *item)\n {\n-\tif (!core_commit_graph)\n+\tif (!prepare_commit_graph())\n \t\treturn 0;\n-\n-\tprepare_commit_graph();\n-\tif (commit_graph)\n-\t\treturn parse_commit_in_graph_one(commit_graph, item);\n-\treturn 0;\n+\treturn parse_commit_in_graph_one(commit_graph, item);\n }\n \n void load_commit_graph_info(struct commit *item)\n {\n \tuint32_t pos;\n-\tif (!core_commit_graph)\n+\tif (!prepare_commit_graph())\n \t\treturn;\n-\tprepare_commit_graph();\n-\tif (commit_graph && find_commit_in_graph(item, commit_graph, &pos))\n+\tif (find_commit_in_graph(item, commit_graph, &pos))\n \t\tfill_commit_graph_info(item, commit_graph, pos);\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352311","messageId":"ab6c2d449dcec37234b051aa3ad489ffe713404c.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 2/6] object-store: add missing include","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:38Z","receivedAt":"2018-07-11T22:42:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n object-store.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/object-store.h b/object-store.h\nindex a3db17bbf5..0e13543bab 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -2,6 +2,9 @@\n #define OBJECT_STORE_H\n \n #include \"oidmap.h\"\n+#include \"list.h\"\n+#include \"sha1-array.h\"\n+#include \"strbuf.h\"\n \n struct alternate_object_database {\n \tstruct alternate_object_database *next;\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352312","messageId":"3f5988895ff97818e1b30ffd301ef2d1b940d05c.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 3/6] commit-graph: add missing forward declaration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:39Z","receivedAt":"2018-07-11T22:42:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n commit-graph.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 506cb45fb1..674052bef4 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -5,6 +5,8 @@\n #include \"repository.h\"\n #include \"string-list.h\"\n \n+struct commit;\n+\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352313","messageId":"0d422eaf12ff16e962d65c0db45c67099ba91ab9.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 4/6] commit-graph: add free_commit_graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:40Z","receivedAt":"2018-07-11T22:42:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/commit-graph.c | 2 ++\n commit-graph.c | 24 ++++++++++++++----------\n commit-graph.h | 2 ++\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c7d0db5ab4..0bf0c48657 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -115,6 +115,8 @@ static int graph_read(int argc, const char **argv)\n \t\tprintf(\" large_edges\");\n \tprintf(\"\\n\");\n \n+\tfree_commit_graph(graph);\n+\n \treturn 0;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1ea701ed69..143a587581 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -231,16 +231,8 @@ static int prepare_commit_graph(void)\n \n static void close_commit_graph(void)\n {\n-\tif (!commit_graph)\n-\t\treturn;\n-\n-\tif (commit_graph->graph_fd >= 0) {\n-\t\tmunmap((void *)commit_graph->data, commit_graph->data_len);\n-\t\tcommit_graph->data = NULL;\n-\t\tclose(commit_graph->graph_fd);\n-\t}\n-\n-\tFREE_AND_NULL(commit_graph);\n+\tfree_commit_graph(commit_graph);\n+\tcommit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -1033,3 +1025,15 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \n \treturn verify_commit_graph_error;\n }\n+\n+void free_commit_graph(struct commit_graph *g)\n+{\n+\tif (!g)\n+\t\treturn;\n+\tif (g->graph_fd >= 0) {\n+\t\tmunmap((void *)g->data, g->data_len);\n+\t\tg->data = NULL;\n+\t\tclose(g->graph_fd);\n+\t}\n+\tfree(g);\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 674052bef4..94defb04a9 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -58,4 +58,6 @@ void write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n+void free_commit_graph(struct commit_graph *);\n+\n #endif\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352314","messageId":"a045aef6d3a8600759c8186b41fa9c4960d65176.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 5/6] commit-graph: store graph in struct object_store","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:41Z","receivedAt":"2018-07-11T22:43:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Instead of storing commit graphs in static variables, store them in\nstruct object_store. There are no changes to the signatures of existing\nfunctions - they all still only support the_repository, and support for\nother instances of struct repository will be added in a subsequent\ncommit.\n\nSigned-off-by: Jonathan Tan \n---\n commit-graph.c | 40 +++++++++++++++++++---------------------\n object-store.h | 3 +++\n object.c | 5 +++++\n 3 files changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 143a587581..b6a76a1413 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -183,24 +183,20 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-/* global storage */\n-static struct commit_graph *commit_graph = NULL;\n-\n static void prepare_commit_graph_one(const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (commit_graph)\n+\tif (the_repository->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tcommit_graph = load_commit_graph_one(graph_name);\n+\tthe_repository->objects->commit_graph =\n+\t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n }\n \n-static int prepare_commit_graph_run_once = 0;\n-\n /*\n * Return 1 if commit_graph is non-NULL, and 0 otherwise.\n *\n@@ -212,9 +208,9 @@ static int prepare_commit_graph(void)\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n \n-\tif (prepare_commit_graph_run_once)\n-\t\treturn !!commit_graph;\n-\tprepare_commit_graph_run_once = 1;\n+\tif (the_repository->objects->commit_graph_attempted)\n+\t\treturn !!the_repository->objects->commit_graph;\n+\tthe_repository->objects->commit_graph_attempted = 1;\n \n \tif (!core_commit_graph)\n \t\treturn 0;\n@@ -223,16 +219,16 @@ static int prepare_commit_graph(void)\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb(the_repository);\n \tfor (alt = the_repository->objects->alt_odb_list;\n-\t !commit_graph && alt;\n+\t !the_repository->objects->commit_graph && alt;\n \t alt = alt->next)\n \t\tprepare_commit_graph_one(alt->path);\n-\treturn !!commit_graph;\n+\treturn !!the_repository->objects->commit_graph;\n }\n \n static void close_commit_graph(void)\n {\n-\tfree_commit_graph(commit_graph);\n-\tcommit_graph = NULL;\n+\tfree_commit_graph(the_repository->objects->commit_graph);\n+\tthe_repository->objects->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -342,7 +338,7 @@ int parse_commit_in_graph(struct commit *item)\n {\n \tif (!prepare_commit_graph())\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(commit_graph, item);\n+\treturn parse_commit_in_graph_one(the_repository->objects->commit_graph, item);\n }\n \n void load_commit_graph_info(struct commit *item)\n@@ -350,8 +346,8 @@ void load_commit_graph_info(struct commit *item)\n \tuint32_t pos;\n \tif (!prepare_commit_graph())\n \t\treturn;\n-\tif (find_commit_in_graph(item, commit_graph, &pos))\n-\t\tfill_commit_graph_info(item, commit_graph, pos);\n+\tif (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))\n+\t\tfill_commit_graph_info(item, the_repository->objects->commit_graph, pos);\n }\n \n static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)\n@@ -379,7 +375,7 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,\n \n struct tree *get_commit_tree_in_graph(const struct commit *c)\n {\n-\treturn get_commit_tree_in_graph_one(commit_graph, c);\n+\treturn get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);\n }\n \n static void write_graph_chunk_fanout(struct hashfile *f,\n@@ -696,15 +692,17 @@ void write_commit_graph(const char *obj_dir,\n \n \tif (append) {\n \t\tprepare_commit_graph_one(obj_dir);\n-\t\tif (commit_graph)\n-\t\t\toids.alloc += commit_graph->num_commits;\n+\t\tif (the_repository->objects->commit_graph)\n+\t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\n \n \tif (oids.alloc < 1024)\n \t\toids.alloc = 1024;\n \tALLOC_ARRAY(oids.list, oids.alloc);\n \n-\tif (append && commit_graph) {\n+\tif (append && the_repository->objects->commit_graph) {\n+\t\tstruct commit_graph *commit_graph =\n+\t\t\tthe_repository->objects->commit_graph;\n \t\tfor (i = 0; i < commit_graph->num_commits; i++) {\n \t\t\tconst unsigned char *hash = commit_graph->chunk_oid_lookup +\n \t\t\t\tcommit_graph->hash_len * i;\ndiff --git a/object-store.h b/object-store.h\nindex 0e13543bab..e481f7ad41 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -106,6 +106,9 @@ struct raw_object_store {\n \t */\n \tstruct oidmap *replace_map;\n \n+\tstruct commit_graph *commit_graph;\n+\tunsigned commit_graph_attempted : 1; /* if loading has been attempted */\n+\n \t/*\n \t * private data\n \t *\ndiff --git a/object.c b/object.c\nindex b0faab85d4..e2c112cc1a 100644\n--- a/object.c\n+++ b/object.c\n@@ -9,6 +9,7 @@\n #include \"alloc.h\"\n #include \"object-store.h\"\n #include \"packfile.h\"\n+#include \"commit-graph.h\"\n \n unsigned int get_max_object_index(void)\n {\n@@ -507,6 +508,10 @@ void raw_object_store_clear(struct raw_object_store *o)\n \toidmap_free(o->replace_map, 1);\n \tFREE_AND_NULL(o->replace_map);\n \n+\tfree_commit_graph(o->commit_graph);\n+\to->commit_graph = NULL;\n+\to->commit_graph_attempted = 0;\n+\n \tfree_alt_odbs(o);\n \to->alt_odb_tail = NULL;\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352315","messageId":"b2f5b69806c4c4a4009267701245aee4cabd2cf1.1531348540.git.jonathantanmy@google.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"[PATCH v3 6/6] commit-graph: add repo arg to graph readers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-11T22:42:42Z","receivedAt":"2018-07-11T22:43:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a struct repository argument to the functions in commit-graph.h that\nread the commit graph. (This commit does not affect functions that write\ncommit graphs.)\n\nBecause the commit graph functions can now read the commit graph of any\nrepository, the global variable core_commit_graph has been removed.\nInstead, the config option core.commitGraph is now read on the first\ntime in a repository that a commit is attempted to be parsed using its\ncommit graph.\n\nThis commit includes a test that exercises the functionality on an\narbitrary repository that is not the_repository.\n\nSigned-off-by: Jonathan Tan \n---\n Makefile | 1 +\n builtin/fsck.c | 2 +-\n cache.h | 1 -\n commit-graph.c | 60 +++++++++++++++-------------\n commit-graph.h | 7 ++--\n commit.c | 6 +--\n config.c | 5 ---\n environment.c | 1 -\n ref-filter.c | 2 +-\n t/helper/test-repository.c | 82 ++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t5318-commit-graph.sh | 35 ++++++++++++++++\n 13 files changed, 162 insertions(+), 42 deletions(-)\n create mode 100644 t/helper/test-repository.c\n\ndiff --git a/Makefile b/Makefile\nindex 0cb6590f24..bb8bd67201 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-ref-store.o\n TEST_BUILTINS_OBJS += test-regex.o\n+TEST_BUILTINS_OBJS += test-repository.o\n TEST_BUILTINS_OBJS += test-revision-walking.o\n TEST_BUILTINS_OBJS += test-run-command.o\n TEST_BUILTINS_OBJS += test-scrap-cache-tree.o\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex ea5e2a03e6..c96f3f4fcc 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -830,7 +830,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tcheck_connectivity();\n \n-\tif (core_commit_graph) {\n+\tif (!git_config_get_bool(\"core.commitgraph\", &i) && i) {\n \t\tstruct child_process commit_graph_verify = CHILD_PROCESS_INIT;\n \t\tconst char *verify_argv[] = { \"commit-graph\", \"verify\", NULL, NULL, NULL };\n \ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..3311f4c9e2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -813,7 +813,6 @@ extern char *git_replace_ref_base;\n \n extern int fsync_object_files;\n extern int core_preload_index;\n-extern int core_commit_graph;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b6a76a1413..b0a55ad128 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -183,15 +183,15 @@ struct commit_graph *load_commit_graph_one(const char *graph_file)\n \texit(1);\n }\n \n-static void prepare_commit_graph_one(const char *obj_dir)\n+static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)\n {\n \tchar *graph_name;\n \n-\tif (the_repository->objects->commit_graph)\n+\tif (r->objects->commit_graph)\n \t\treturn;\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tthe_repository->objects->commit_graph =\n+\tr->objects->commit_graph =\n \t\tload_commit_graph_one(graph_name);\n \n \tFREE_AND_NULL(graph_name);\n@@ -203,26 +203,34 @@ static void prepare_commit_graph_one(const char *obj_dir)\n * On the first invocation, this function attemps to load the commit\n * graph if the_repository is configured to have one.\n */\n-static int prepare_commit_graph(void)\n+static int prepare_commit_graph(struct repository *r)\n {\n \tstruct alternate_object_database *alt;\n \tchar *obj_dir;\n+\tint config_value;\n \n-\tif (the_repository->objects->commit_graph_attempted)\n-\t\treturn !!the_repository->objects->commit_graph;\n-\tthe_repository->objects->commit_graph_attempted = 1;\n+\tif (r->objects->commit_graph_attempted)\n+\t\treturn !!r->objects->commit_graph;\n+\tr->objects->commit_graph_attempted = 1;\n \n-\tif (!core_commit_graph)\n+\tif (repo_config_get_bool(r, \"core.commitgraph\", &config_value) ||\n+\t !config_value)\n+\t\t/*\n+\t\t * This repository is not configured to use commit graphs, so\n+\t\t * do not load one. (But report commit_graph_attempted anyway\n+\t\t * so that commit graph loading is not attempted again for this\n+\t\t * repository.)\n+\t\t */\n \t\treturn 0;\n \n-\tobj_dir = get_object_directory();\n-\tprepare_commit_graph_one(obj_dir);\n-\tprepare_alt_odb(the_repository);\n-\tfor (alt = the_repository->objects->alt_odb_list;\n-\t !the_repository->objects->commit_graph && alt;\n+\tobj_dir = r->objects->objectdir;\n+\tprepare_commit_graph_one(r, obj_dir);\n+\tprepare_alt_odb(r);\n+\tfor (alt = r->objects->alt_odb_list;\n+\t !r->objects->commit_graph && alt;\n \t alt = alt->next)\n-\t\tprepare_commit_graph_one(alt->path);\n-\treturn !!the_repository->objects->commit_graph;\n+\t\tprepare_commit_graph_one(r, alt->path);\n+\treturn !!r->objects->commit_graph;\n }\n \n static void close_commit_graph(void)\n@@ -323,8 +331,6 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n {\n \tuint32_t pos;\n \n-\tif (!core_commit_graph)\n-\t\treturn 0;\n \tif (item->object.parsed)\n \t\treturn 1;\n \n@@ -334,20 +340,20 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item\n \treturn 0;\n }\n \n-int parse_commit_in_graph(struct commit *item)\n+int parse_commit_in_graph(struct repository *r, struct commit *item)\n {\n-\tif (!prepare_commit_graph())\n+\tif (!prepare_commit_graph(r))\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(the_repository->objects->commit_graph, item);\n+\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n }\n \n-void load_commit_graph_info(struct commit *item)\n+void load_commit_graph_info(struct repository *r, struct commit *item)\n {\n \tuint32_t pos;\n-\tif (!prepare_commit_graph())\n+\tif (!prepare_commit_graph(r))\n \t\treturn;\n-\tif (find_commit_in_graph(item, the_repository->objects->commit_graph, &pos))\n-\t\tfill_commit_graph_info(item, the_repository->objects->commit_graph, pos);\n+\tif (find_commit_in_graph(item, r->objects->commit_graph, &pos))\n+\t\tfill_commit_graph_info(item, r->objects->commit_graph, pos);\n }\n \n static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *c)\n@@ -373,9 +379,9 @@ static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,\n \treturn load_tree_for_commit(g, (struct commit *)c);\n }\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c)\n+struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit *c)\n {\n-\treturn get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);\n+\treturn get_commit_tree_in_graph_one(r->objects->commit_graph, c);\n }\n \n static void write_graph_chunk_fanout(struct hashfile *f,\n@@ -691,7 +697,7 @@ void write_commit_graph(const char *obj_dir,\n \toids.alloc = approximate_object_count() / 4;\n \n \tif (append) {\n-\t\tprepare_commit_graph_one(obj_dir);\n+\t\tprepare_commit_graph_one(the_repository, obj_dir);\n \t\tif (the_repository->objects->commit_graph)\n \t\t\toids.alloc += the_repository->objects->commit_graph->num_commits;\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 94defb04a9..76e098934a 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -19,7 +19,7 @@ char *get_commit_graph_filename(const char *obj_dir);\n *\n * See parse_commit_buffer() for the fallback after this call.\n */\n-int parse_commit_in_graph(struct commit *item);\n+int parse_commit_in_graph(struct repository *r, struct commit *item);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\n@@ -27,9 +27,10 @@ int parse_commit_in_graph(struct commit *item);\n * checked and filled. Fill the graph_pos and generation members of\n * the given commit.\n */\n-void load_commit_graph_info(struct commit *item);\n+void load_commit_graph_info(struct repository *r, struct commit *item);\n \n-struct tree *get_commit_tree_in_graph(const struct commit *c);\n+struct tree *get_commit_tree_in_graph(struct repository *r,\n+\t\t\t\t const struct commit *c);\n \n struct commit_graph {\n \tint graph_fd;\ndiff --git a/commit.c b/commit.c\nindex c0a83d2644..39b80bd21d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -342,7 +342,7 @@ struct tree *get_commit_tree(const struct commit *commit)\n \tif (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)\n \t\tBUG(\"commit has NULL tree, but was not loaded from commit-graph\");\n \n-\treturn get_commit_tree_in_graph(commit);\n+\treturn get_commit_tree_in_graph(the_repository, commit);\n }\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n@@ -438,7 +438,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \titem->date = parse_commit_date(bufptr, tail);\n \n \tif (check_graph)\n-\t\tload_commit_graph_info(item);\n+\t\tload_commit_graph_info(the_repository, item);\n \n \treturn 0;\n }\n@@ -454,7 +454,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(item))\n+\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n \t\treturn 0;\n \tbuffer = read_object_file(&item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/config.c b/config.c\nindex 139c903f6b..3aacddfec4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1309,11 +1309,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.commitgraph\")) {\n-\t\tcore_commit_graph = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"core.sparsecheckout\")) {\n \t\tcore_apply_sparse_checkout = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 013e845235..6cf0079389 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n int grafts_replace_parents = 1;\n-int core_commit_graph;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 49021ee446..9b2da88392 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1713,7 +1713,7 @@ static enum contains_result contains_tag_algo(struct commit *candidate,\n \n \tfor (p = want; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n-\t\tload_commit_graph_info(c);\n+\t\tload_commit_graph_info(the_repository, c);\n \t\tif (c->generation < cutoff)\n \t\t\tcutoff = c->generation;\n \t}\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nnew file mode 100644\nindex 0000000000..2762ca6562\n--- /dev/null\n+++ b/t/helper/test-repository.c\n@@ -0,0 +1,82 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"commit-graph.h\"\n+#include \"commit.h\"\n+#include \"config.h\"\n+#include \"object-store.h\"\n+#include \"object.h\"\n+#include \"repository.h\"\n+#include \"tree.h\"\n+\n+static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n+\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct commit_list *parent;\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\tc = lookup_commit(&r, commit_oid);\n+\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\n+\tprintf(\"%\"PRItime, c->date);\n+\tfor (parent = c->parents; parent; parent = parent->next)\n+\t\tprintf(\" %s\", oid_to_hex(&parent->item->object.oid));\n+\tprintf(\"\\n\");\n+\n+\trepo_clear(&r);\n+}\n+\n+static void test_get_commit_tree_in_graph(const char *gitdir,\n+\t\t\t\t\t const char *worktree,\n+\t\t\t\t\t const struct object_id *commit_oid)\n+{\n+\tstruct repository r;\n+\tstruct commit *c;\n+\tstruct tree *tree;\n+\n+\trepo_init(&r, gitdir, worktree);\n+\n+\tc = lookup_commit(&r, commit_oid);\n+\n+\t/*\n+\t * get_commit_tree_in_graph does not automatically parse the commit, so\n+\t * parse it first.\n+\t */\n+\tif (!parse_commit_in_graph(&r, c))\n+\t\tdie(\"Couldn't parse commit\");\n+\ttree = get_commit_tree_in_graph(&r, c);\n+\tif (!tree)\n+\t\tdie(\"Couldn't get commit tree\");\n+\n+\tprintf(\"%s\\n\", oid_to_hex(&tree->object.oid));\n+\n+\trepo_clear(&r);\n+}\n+\n+int cmd__repository(int argc, const char **argv)\n+{\n+\tif (argc < 2)\n+\t\tdie(\"must have at least 2 arguments\");\n+\tif (!strcmp(argv[1], \"parse_commit_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_parse_commit_in_graph(argv[2], argv[3], &oid);\n+\t} else if (!strcmp(argv[1], \"get_commit_tree_in_graph\")) {\n+\t\tstruct object_id oid;\n+\t\tif (argc < 5)\n+\t\t\tdie(\"not enough arguments\");\n+\t\tif (parse_oid_hex(argv[4], &oid, &argv[4]))\n+\t\t\tdie(\"cannot parse oid '%s'\", argv[4]);\n+\t\ttest_get_commit_tree_in_graph(argv[2], argv[3], &oid);\n+\t} else {\n+\t\tdie(\"unrecognized '%s'\", argv[1]);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 805a45de9c..dafc91c240 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -29,6 +29,7 @@ static struct test_cmd cmds[] = {\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"ref-store\", cmd__ref_store },\n \t{ \"regex\", cmd__regex },\n+\t{ \"repository\", cmd__repository },\n \t{ \"revision-walking\", cmd__revision_walking },\n \t{ \"run-command\", cmd__run_command },\n \t{ \"scrap-cache-tree\", cmd__scrap_cache_tree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7116ddfb94..80cbcf0857 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -23,6 +23,7 @@ int cmd__prio_queue(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__ref_store(int argc, const char **argv);\n int cmd__regex(int argc, const char **argv);\n+int cmd__repository(int argc, const char **argv);\n int cmd__revision_walking(int argc, const char **argv);\n int cmd__run_command(int argc, const char **argv);\n int cmd__scrap_cache_tree(int argc, const char **argv);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5947de3d24..4f17d7701e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -431,4 +431,39 @@ test_expect_success 'git fsck (checks commit-graph)' '\n \ttest_must_fail git fsck\n '\n \n+test_expect_success 'setup non-the_repository tests' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\ttest_commit -C repo one &&\n+\ttest_commit -C repo two &&\n+\tgit -C repo config core.commitGraph true &&\n+\tgit -C repo rev-parse two | \\\n+\t\tgit -C repo commit-graph write --stdin-commits\n+'\n+\n+test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n+\t\t$(git -C repo rev-parse one) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository parse_commit_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n+\techo $(git -C repo rev-parse two^{tree}) >expect &&\n+\ttest_cmp expect actual &&\n+\n+\ttest-tool repository get_commit_tree_in_graph \\\n+\t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n+\techo $(git -C repo rev-parse one^{tree}) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352365","messageId":"xmqqd0vsz93u.fsf@gitster-ct.c.googlers.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/6] Object store refactoring: commit graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T16:56:05Z","receivedAt":"2018-07-12T16:56:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup,\n> following Stolee's suggestion.\n>\n> (It also seems better to build it this way to me, since both these\n> branches are going into \"next\" according to the latest What's Cooking.)\n\nOK. I am perfectly OK if this left lookup_commit() with older\nfunction signature and instead asked the integrator to fix up with\nevil merge with sb/object-store-lookup, but given that this topic is\nno more urgent than the other one, making them intertwined is fine.\n\nMerging ds/commit-graph-fsck into sb/object-store-lookup already\nrequires an evil merge to build correctly due to this new parameter.\nI really wish we did the more canonical \"do not change signature of\na widely used function. If the widely used one is a mere\nspecialization that calls the new one with the default parameter,\nmake that widely used one a thin wraper (or a macro) of the new one\"\napproach to avoid having to repeatedly create such pointless evil\nmerges X-<.\n\n\n> Makefile | 1 +\n> builtin/commit-graph.c | 2 +\n> builtin/fsck.c | 2 +-\n> cache.h | 1 -\n> commit-graph.c | 108 +++++++++++++++++++++----------------\n> commit-graph.h | 11 ++--\n> commit.c | 6 +--\n> config.c | 5 --\n> environment.c | 1 -\n> object-store.h | 6 +++\n> object.c | 5 ++\n> ref-filter.c | 2 +-\n> t/helper/test-repository.c | 82 ++++++++++++++++++++++++++++\n> t/helper/test-tool.c | 1 +\n> t/helper/test-tool.h | 1 +\n> t/t5318-commit-graph.sh | 35 ++++++++++++\n> 16 files changed, 207 insertions(+), 62 deletions(-)\n> create mode 100644 t/helper/test-repository.c\n"},{"id":"352373","messageId":"b9d9df97-c950-fec3-83dd-aed32441dae0@gmail.com","threadId":"48756","inReplyTo":"cover.1531348540.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/6] Object store refactoring: commit graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-12T17:43:28Z","receivedAt":"2018-07-12T17:43:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/11/2018 6:42 PM, Jonathan Tan wrote:\n> This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup,\n> following Stolee's suggestion.\n>\n> (It also seems better to build it this way to me, since both these\n> branches are going into \"next\" according to the latest What's Cooking.)\n>\n> Junio wrote in [1]:\n>\n>> I've added SQUASH??? patch at the tip of each of the above,\n>> rebuilt 'pu' with them and pushed the result out. It seems that\n>> Travis is happier with the result.\n>>\n>> Please do not forget to squash them in when/if rerolling. If there\n>> is no need to change anything else other than squashing them, you\n>> can tell me to which commit in your series the fix needs to be\n>> squashed in (that would save me time to figure it out, obviously).\n> I'm rerolling because I also need to update the last patch with the new\n> lookup_commit() function signature that Stefan's sb/object-store-lookup\n> introduces. I have squashed the SQUASH??? patch into the corresponding\n> patch in this patch set.\n>\n> Changes from v2:\n> - now also based on sb/object-store-lookup in addition to\n> ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto\n> sb-object-store-lookup, then rebased this patch set onto the result)\n> - patches 1-5 are unchanged\n> - patch 6:\n> - used \"PRItime\" instead of \"ul\" when printing a timestamp (the\n> SQUASH??? patch)\n> - updated invocations of lookup_commit() to take a repository object\n>\n> [1] https://public-inbox.org/git/xmqqpnzt1myi.fsf@gitster-ct.c.googlers.com/\n\nI re-read the patch series and think you addressed all feedback. I have \nno more to add.\n\nReviewed-by: Derrick Stolee \n\n"},{"id":"355302","messageId":"20180813003010.17610-1-szeder.dev@gmail.com","threadId":"48756","inReplyTo":"b2f5b69806c4c4a4009267701245aee4cabd2cf1.1531348540.git.jonathantanmy@google.com","subject":"[PATCH] t5318: avoid unnecessary command substitutions","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-13T00:30:10Z","receivedAt":"2018-08-13T00:30:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Two tests added in dade47c06c (commit-graph: add repo arg to graph\nreaders, 2018-07-11) prepare the contents of 'expect' files by\n'echo'ing the results of command substitutions. That's unncessary,\navoid them by directly saving the output of the commands executed in\nthose command substitutions.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t5318-commit-graph.sh | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 1c148ebf21..3c1ffad491 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -444,25 +444,27 @@ test_expect_success 'setup non-the_repository tests' '\n test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n \ttest-tool repository parse_commit_in_graph \\\n \t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n-\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n-\t\t$(git -C repo rev-parse one) >expect &&\n+\t{\n+\t\tgit -C repo log --pretty=format:\"%ct \" -1 &&\n+\t\tgit -C repo rev-parse one\n+\t} >expect &&\n \ttest_cmp expect actual &&\n \n \ttest-tool repository parse_commit_in_graph \\\n \t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n-\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n+\tgit -C repo log --pretty=\"%ct\" -1 one >expect &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n \ttest-tool repository get_commit_tree_in_graph \\\n \t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n-\techo $(git -C repo rev-parse two^{tree}) >expect &&\n+\tgit -C repo rev-parse two^{tree} >expect &&\n \ttest_cmp expect actual &&\n \n \ttest-tool repository get_commit_tree_in_graph \\\n \t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n-\techo $(git -C repo rev-parse one^{tree}) >expect &&\n+\tgit -C repo rev-parse one^{tree} >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"355321","messageId":"9e934039-6b97-28bd-b5c9-34ac5f6754bb@gmail.com","threadId":"48756","inReplyTo":"20180813003010.17610-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t5318: avoid unnecessary command substitutions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-13T11:23:18Z","receivedAt":"2018-08-13T11:23:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/12/2018 8:30 PM, SZEDER Gábor wrote:\n> Two tests added in dade47c06c (commit-graph: add repo arg to graph\n> readers, 2018-07-11) prepare the contents of 'expect' files by\n> 'echo'ing the results of command substitutions. That's unncessary,\n> avoid them by directly saving the output of the commands executed in\n> those command substitutions.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> t/t5318-commit-graph.sh | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 1c148ebf21..3c1ffad491 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -444,25 +444,27 @@ test_expect_success 'setup non-the_repository tests' '\n> test_expect_success 'parse_commit_in_graph works for non-the_repository' '\n> \ttest-tool repository parse_commit_in_graph \\\n> \t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n> -\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n> -\t\t$(git -C repo rev-parse one) >expect &&\n> +\t{\n> +\t\tgit -C repo log --pretty=format:\"%ct \" -1 &&\n> +\t\tgit -C repo rev-parse one\n> +\t} >expect &&\n> \ttest_cmp expect actual &&\n> \n> \ttest-tool repository parse_commit_in_graph \\\n> \t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n> -\techo $(git -C repo log --pretty=\"%ct\" -1 one) >expect &&\n> +\tgit -C repo log --pretty=\"%ct\" -1 one >expect &&\n> \ttest_cmp expect actual\n> '\n> \n> test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n> \ttest-tool repository get_commit_tree_in_graph \\\n> \t\trepo/.git repo \"$(git -C repo rev-parse two)\" >actual &&\n> -\techo $(git -C repo rev-parse two^{tree}) >expect &&\n> +\tgit -C repo rev-parse two^{tree} >expect &&\n> \ttest_cmp expect actual &&\n> \n> \ttest-tool repository get_commit_tree_in_graph \\\n> \t\trepo/.git repo \"$(git -C repo rev-parse one)\" >actual &&\n> -\techo $(git -C repo rev-parse one^{tree}) >expect &&\n> +\tgit -C repo rev-parse one^{tree} >expect &&\n> \ttest_cmp expect actual\n> '\n> \nThese make sense and are good examples for future test patterns. Thanks, \nSzeder.\n\nReviewed-by: Derrick Stolee \n"},{"id":"355452","messageId":"xmqqa7pq5bpn.fsf@gitster-ct.c.googlers.com","threadId":"48756","inReplyTo":"20180813003010.17610-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t5318: avoid unnecessary command substitutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-13T19:05:24Z","receivedAt":"2018-08-13T19:05:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> -\techo $(git -C repo log --pretty=\"%ct\" -1) \\\n> -\t\t$(git -C repo rev-parse one) >expect &&\n> +\t{\n> +\t\tgit -C repo log --pretty=format:\"%ct \" -1 &&\n> +\t\tgit -C repo rev-parse one\n> +\t} >expect &&\n\nHeh, \"format:\"%ct \" to make the first one end with an incomplete\nline? Neat trick.\n"}]}