{"thread":{"id":"48617","subject":"[RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo","startedAt":"2018-05-31T17:40:45Z","lastAt":"2018-06-09T15:47:40Z","messageCount":18,"participants":["Derrick Stolee","Stefan Beller","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"348908","messageId":"20180531174024.124488-1-dstolee@microsoft.com","threadId":"48617","inReplyTo":null,"subject":"[RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:40:39Z","receivedAt":"2018-05-31T17:40:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The commit-graph file stores a condensed version of the commit history.\nThis helps speed up several operations involving commit walks. This\nfeature does not work well if those commits \"change\" using features like\ncommit grafts, replace objects, or shallow clones.\n\nSince the commit-graph feature is optional, hidden behind the\n'core.commitGraph' config option, and requires manual maintenance (until\nds/commit-graph-fsck' is merged), these issues only arise for expert\nusers who decided to opt-in.\n\nThis RFC is a first attempt at rectify the issues that come up when\nthese features interact. I have not succeeded entirely, as I will\ndiscuss below.\n\nThe first two \"DO NOT MERGE\" commits are not intended to be part of the\nmain product, but are ways to help the full test suite run while\ncomputing and consuming commit-graph files. This is acheived by calling\nwrite_commit_graph_reachable() from `git fetch` and `git commit`. I\nbelieve this is the only dependence on ds/commit-graph-fsck. The other\ncommits should only depend on ds/commit-graph-lockfile-fix.\n\nRunning the full test suite after these DO NO MERGE commits results in\nthe following test failures, which I categorize by type of failure.\n\nThe following tests are red herrings. Most work by deleting a commit\nfrom the object database and trying to demonstrate a failure. Others\nrely on how certain objects are loaded. These are not bugs, but will\nadd noise if you run the tests suite with this patch.\n\nt0410-partial-clone.sh\t\t\tFailed tests:  5, 8\nt5307-pack-missing-commit.sh\t\tFailed tests:  3-4\nt6011-rev-list-with-bad-commit.sh\tFailed test:  4\nt6024-recursive-merge.sh\t\tFailed test:  4\n\nThe following tests are fixed in \"commit-graph: enable replace-object\nand grafts\".\n\nt6001-rev-list-graft.sh\t\t\tFailed tests:  3, 5, 7, 9, 11, 13\nt6050-replace.sh\t\t\tFailed tests:  11-15, 23-24, 29\n\nThe following tests involve shallow clones.\n\nt5500-fetch-pack.sh\t\t\tFailed tests:  30-31, 34, 348-349\nt5537-fetch-shallow.sh\t\t\tFailed tests:  4-7, 9\nt5802-connect-helper.sh\t\t\tFailed test:  3\n\nThese tests are mostly fixed by three commits:\n\n* commit-graph: avoid writing when repo is shallow\n* fetch: destroy commit graph on shallow parameters\n* commit-graph: revert to odb on missing parents\n\nEach of these cases cover a different interaction that occurs with\nshallow clones. Some are due to a commit becoming shallow, while others\nare due to a commit becoming unshallow (and hence invalidating its\ngeneration number).\n\nAfter these changes, there is one test case that still fails, and I\ncannot understand why:\n\nt5500-fetch-pack.sh\t\t\tFailed test:  348\n\nThis test fails when performing a `git fetch --shallow-exclude`. When I\nhalt the test using `t5500-fetch-pack.sh -d -i` and navigate to the\ndirectory to replay the fetch it performs as expected. After struggling\nwith it for a while, I figured I should just put this series up for\ndiscussion. Maybe someone with more experience in shallow clones can\npoint out the obvious issues I'm having.\n\nThanks,\n-Stolee\n\nDerrick Stolee (6):\n  DO NOT MERGE: compute commit-graph on every commit\n  DO NOT MERGE: write commit-graph on every fetch\n  commit-graph: enable replace-object and grafts\n  commit-graph: avoid writing when repo is shallow\n  fetch: destroy commit graph on shallow parameters\n  commit-graph: revert to odb on missing parents\n\n builtin/commit.c  |  5 +++++\n builtin/fetch.c   | 10 +++++++++\n builtin/gc.c      |  2 +-\n builtin/replace.c |  3 +++\n commit-graph.c    | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++----\n commit-graph.h    |  9 ++++++++\n commit.c          |  5 +++++\n environment.c     |  2 +-\n 8 files changed, 95 insertions(+), 6 deletions(-)\n\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348909","messageId":"20180531174024.124488-5-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:14Z","receivedAt":"2018-05-31T17:41:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Shallow clones do not interact well with the commit-graph feature for\nseveral reasons. Instead of doing the hard thing to fix those\ninteractions, instead prevent reading or writing a commit-graph file for\nshallow repositories.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n commit-graph.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 95af4ed519..80e377b90f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -208,6 +208,9 @@ static void prepare_commit_graph(void)\n \t\treturn;\n \tprepare_commit_graph_run_once = 1;\n \n+\tif (is_repository_shallow())\n+\t\treturn;\n+\n \tobj_dir = get_object_directory();\n \tprepare_commit_graph_one(obj_dir);\n \tprepare_alt_odb();\n@@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,\n \tint num_extra_edges;\n \tstruct commit_list *parent;\n \n+\t/*\n+\t * Shallow clones are not supproted, as they create bad\n+\t * generation skips as they are un-shallowed.\n+\t */\n+\tif (is_repository_shallow()) {\n+\t\twarning(\"writing a commit-graph in a shallow repository is not supported\");\n+\t\treturn;\n+\t}\n+\n \toids.nr = 0;\n \toids.alloc = approximate_object_count() / 4;\n \n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348910","messageId":"20180531174024.124488-2-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 1/6] DO NOT MERGE: compute commit-graph on every commit","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:08Z","receivedAt":"2018-05-31T17:41:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Also enable core.commitGraph and gc.commitGraph. Helps to test the\ncommit-graph feature with the rest of the test infrastructure.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/commit.c | 5 +++++\n builtin/gc.c     | 2 +-\n environment.c    | 2 +-\n 3 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e8e8d13be4..8751b816c1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -32,6 +32,7 @@\n #include \"column.h\"\n #include \"sequencer.h\"\n #include \"mailmap.h\"\n+#include \"commit-graph.h\"\n \n static const char * const builtin_commit_usage[] = {\n \tN_(\"git commit [<options>] [--] <pathspec>...\"),\n@@ -1623,5 +1624,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \tUNLEAK(err);\n \tUNLEAK(sb);\n+\n+\tif (core_commit_graph)\n+\t\twrite_commit_graph_reachable(get_object_directory(), 1);\n+\n \treturn 0;\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex efd214a59f..999c09d8e2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -35,7 +35,7 @@ static int aggressive_depth = 50;\n static int aggressive_window = 250;\n static int gc_auto_threshold = 6700;\n static int gc_auto_pack_limit = 50;\n-static int gc_commit_graph = 0;\n+static int gc_commit_graph = 1;\n static int detach_auto = 1;\n static timestamp_t gc_log_expire_time;\n static const char *gc_log_expire = \"1.day.ago\";\ndiff --git a/environment.c b/environment.c\nindex 8853e2f0dd..fdb2d56d90 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -62,7 +62,7 @@ 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_commit_graph = 1;\n int core_apply_sparse_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348911","messageId":"20180531174024.124488-6-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 5/6] fetch: destroy commit graph on shallow parameters","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:16Z","receivedAt":"2018-05-31T17:41:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The commit-graph feature is not compatible with history-rewriting\nfeatures such as shallow clones. When running a 'git fetch' with\nany of the shallow/unshallow options, destroy the commit-graph\nfile and override core.commitGraph to be false.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/fetch.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex af896e4b74..2001dca881 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1452,6 +1452,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (update_shallow || depth || deepen_since || deepen_not.nr ||\n+\t    deepen_relative || unshallow || update_shallow || is_repository_shallow()) {\n+\t\tdestroy_commit_graph(get_object_directory());\n+\t\tcore_commit_graph = 0;\n+\t}\n+\n \tif (remote) {\n \t\tif (filter_options.choice || repository_format_partial_clone)\n \t\t\tfetch_one_setup_partial(remote);\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348912","messageId":"20180531174024.124488-3-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 2/6] DO NOT MERGE: write commit-graph on every fetch","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:11Z","receivedAt":"2018-05-31T17:41:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"THIS IS ONLY FOR TESTING TO INCREASE TEST COVERAGE\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/fetch.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8ee998ea2e..af896e4b74 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -20,6 +20,7 @@\n #include \"utf8.h\"\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"commit-graph.h\"\n \n static const char * const builtin_fetch_usage[] = {\n \tN_(\"git fetch [<options>] [<repository> [<refspec>...]]\"),\n@@ -1480,6 +1481,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tclose_all_packs();\n \n+\tif (core_commit_graph)\n+\t\twrite_commit_graph_reachable(get_object_directory(), 1);\n+\n \targv_array_pushl(&argv_gc_auto, \"gc\", \"--auto\", NULL);\n \tif (verbosity < 0)\n \t\targv_array_push(&argv_gc_auto, \"--quiet\");\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348913","messageId":"20180531174024.124488-4-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 3/6] commit-graph: enable replace-object and grafts","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:13Z","receivedAt":"2018-05-31T17:41:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Create destroy_commit_graph() method to delete the commit-graph file\nwhen history is altered by a replace-object call. If the commit-graph\nis rebuilt after that, we will load the correct object while reading\nthe commit-graph.\n\nWhen parsing a commit, first check if the commit was grafted. If so,\nthen ignore the commit-graph for that commit and insted use the parents\nloaded by parsing the commit buffer and comparing against the graft\nfile. \n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/replace.c |  3 +++\n commit-graph.c    | 20 +++++++++++++++++++-\n commit-graph.h    |  9 +++++++++\n commit.c          |  5 +++++\n 4 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 9f01f3fc7f..d553aadcdc 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -15,6 +15,7 @@\n #include \"parse-options.h\"\n #include \"run-command.h\"\n #include \"tag.h\"\n+#include \"commit-graph.h\"\n \n static const char * const git_replace_usage[] = {\n \tN_(\"git replace [-f] <object> <replacement>\"),\n@@ -468,6 +469,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(\"--raw only makes sense with --edit\",\n \t\t\t      git_replace_usage, options);\n \n+\tdestroy_commit_graph(get_object_directory());\n+\n \tswitch (cmdmode) {\n \tcase MODE_DELETE:\n \t\tif (argc < 1)\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e9195dfb17..95af4ed519 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -6,6 +6,7 @@\n #include \"pack.h\"\n #include \"packfile.h\"\n #include \"commit.h\"\n+#include \"dir.h\"\n #include \"object.h\"\n #include \"refs.h\"\n #include \"revision.h\"\n@@ -240,15 +241,22 @@ static struct commit_list **insert_parent_or_die(struct commit_graph *g,\n {\n \tstruct commit *c;\n \tstruct object_id oid;\n+\tconst unsigned char *real;\n \n \tif (pos >= g->num_commits)\n \t\tdie(\"invalid parent position %\"PRIu64, pos);\n \n \thashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);\n+\n+\treal = lookup_replace_object(oid.hash);\n+\n \tc = lookup_commit(&oid);\n \tif (!c)\n \t\tdie(\"could not find commit %s\", oid_to_hex(&oid));\n-\tc->graph_pos = pos;\n+\n+\tif (!hashcmp(real, oid.hash))\n+\t\tc->graph_pos = pos;\n+\n \treturn &commit_list_insert(c, pptr)->next;\n }\n \n@@ -1019,3 +1027,13 @@ int verify_commit_graph(struct commit_graph *g)\n \n \treturn verify_commit_graph_error;\n }\n+\n+void destroy_commit_graph(const char *obj_dir)\n+{\n+\tchar *graph_name;\n+\tclose_commit_graph();\n+\n+\tgraph_name = get_commit_graph_filename(obj_dir);\n+\tremove_path(graph_name);\n+\tFREE_AND_NULL(graph_name);\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9a06a5f188..1d17da1582 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -56,4 +56,13 @@ void write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct commit_graph *g);\n \n+/*\n+ * Delete the commit-graph file in the given object directory.\n+ *\n+ * WARNING: this deletes data, so should only be used when\n+ * performing history-altering actions like replace-object\n+ * or grafts.\n+ */\n+void destroy_commit_graph(const char *obj_dir);\n+\n #endif\ndiff --git a/commit.c b/commit.c\nindex 6eaed0174c..2fe31cde77 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -403,6 +403,11 @@ 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+\n+\tprepare_commit_graft();\n+\tif (commit_graft_pos(item->object.oid.hash) >= 0)\n+\t\tuse_commit_graph = 0;\n+\n \tif (use_commit_graph && parse_commit_in_graph(item))\n \t\treturn 0;\n \tbuffer = read_sha1_file(item->object.oid.hash, &type, &size);\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348914","messageId":"20180531174024.124488-7-dstolee@microsoft.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"[RFC PATCH 6/6] commit-graph: revert to odb on missing parents","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-31T17:41:20Z","receivedAt":"2018-05-31T17:41:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The commit-graph format includes a way to specify a parent is\n\"missing\" from the commit-graph (i.e. we do not have a record of\nthat parent in our list of object IDs, and hence cannot provide\na graph position). For mose cases, this does not occur due to\nthe close_reachable() method adding all reachable commits. However,\nin a shallow clone, we will try to record the parents of a commit\non the shallow boundary, but the parents are not in the repository.\n\nThe GRAPH_PARENT_MISSING value that is stored in the format is\npurposeful, especially for future plans to make the commit-graph file\nincremental or transporting sections of a commit-graph file across\nthe network.\n\nIn the meantime, check if a commit has a missing parent while filling\nits details from the commit-graph. If a parent is missing, still\nassign the generation number and graph position for that item, but\nreport that the commit-graph failed to fill the contents. Then the\ncaller is responsible for filling the rest of the data from a commit\nbuffer.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n commit-graph.c | 33 ++++++++++++++++++++++++++++++---\n 1 file changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 80e377b90f..3e33d061fe 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -278,17 +278,44 @@ static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n \tstruct commit_list **pptr;\n \tconst unsigned char *commit_data = g->chunk_commit_data + (g->hash_len + 16) * pos;\n \n-\titem->object.parsed = 1;\n+\titem->generation = get_be32(commit_data + g->hash_len + 8) >> 2;\n \titem->graph_pos = pos;\n \n+\t/*\n+\t * If we have any edges marked as GRAPH_PARENT_MISSING, we must not parse any\n+\t * more of this object and leave it to the commit buffer to parse.\n+\t */\n+\tedge_value = get_be32(commit_data + g->hash_len);\n+\tif (edge_value == GRAPH_PARENT_MISSING)\n+\t\treturn 0;\n+\tif (edge_value == GRAPH_PARENT_NONE)\n+\t\tgoto continue_parsing;\n+\n+\tedge_value = get_be32(commit_data + g->hash_len + 4);\n+\tif (edge_value == GRAPH_PARENT_MISSING)\n+\t\treturn 0;\n+\tif (edge_value == GRAPH_PARENT_NONE)\n+\t\tgoto continue_parsing;\n+\tif (!(edge_value & GRAPH_OCTOPUS_EDGES_NEEDED))\n+\t\tgoto continue_parsing;\n+\n+\tparent_data_ptr = (uint32_t*)(g->chunk_large_edges +\n+\t\t\t  4 * (uint64_t)(edge_value & GRAPH_EDGE_LAST_MASK));\n+\tdo {\n+\t\tedge_value = get_be32(parent_data_ptr);\n+\t\tif (edge_value == GRAPH_PARENT_MISSING)\n+\t\t\treturn 0;\n+\t\tparent_data_ptr++;\n+\t} while (!(edge_value & GRAPH_LAST_EDGE));\n+\t\n+continue_parsing:\n+\titem->object.parsed = 1;\n \titem->maybe_tree = NULL;\n \n \tdate_high = get_be32(commit_data + g->hash_len + 8) & 0x3;\n \tdate_low = get_be32(commit_data + g->hash_len + 12);\n \titem->date = (timestamp_t)((date_high << 32) | date_low);\n \n-\titem->generation = get_be32(commit_data + g->hash_len + 8) >> 2;\n-\n \tpptr = &item->parents;\n \n \tedge_value = get_be32(commit_data + g->hash_len);\n-- \n2.16.2.338.gcfe06ae955\n\n"},{"id":"348917","messageId":"CAGZ79kYCi_=LRuq35Fh0jGYw9kiW9i=6t1YwLuNj7MfezywZDw@mail.gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T18:33:23Z","receivedAt":"2018-05-31T18:33:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee <dstolee@microsoft.com> wrote:\n> The commit-graph file stores a condensed version of the commit history.\n> This helps speed up several operations involving commit walks. This\n> feature does not work well if those commits \"change\" using features like\n> commit grafts, replace objects, or shallow clones.\n>\n> Since the commit-graph feature is optional, hidden behind the\n> 'core.commitGraph' config option, and requires manual maintenance (until\n> ds/commit-graph-fsck' is merged), these issues only arise for expert\n> users who decided to opt-in.\n>\n> This RFC is a first attempt at rectify the issues that come up when\n> these features interact. I have not succeeded entirely, as I will\n> discuss below.\n>\n> The first two \"DO NOT MERGE\" commits are not intended to be part of the\n> main product, but are ways to help the full test suite run while\n> computing and consuming commit-graph files. This is acheived by calling\n> write_commit_graph_reachable() from `git fetch` and `git commit`. I\n> believe this is the only dependence on ds/commit-graph-fsck. The other\n> commits should only depend on ds/commit-graph-lockfile-fix.\n\nI applied these patches on top of ds/commit-graph-fsck\n(it would have been nice if you mentioned that it applies there)\nRunning the test suite with all patches applied results in:\n\n./t0410-partial-clone.sh                    (Wstat: 256 Tests: 15 Failed: 2)\n  Failed tests:  5, 8\n./t5307-pack-missing-commit.sh              (Wstat: 256 Tests: 5 Failed: 2)\n  Failed tests:  3-4\n./t5500-fetch-pack.sh                       (Wstat: 256 Tests: 353 Failed: 1)\n  Failed test:  348\n./t6011-rev-list-with-bad-commit.sh         (Wstat: 256 Tests: 6 Failed: 1)\n  Failed test:  4\n./t6024-recursive-merge.sh                  (Wstat: 256 Tests: 6 Failed: 1)\n  Failed test:  4\n\nwhich you identified as 4x noise and t5500 as not understood.\n\n$ GIT_TRACE=1 ./t5500-fetch-pack.sh -d -i -v -x\n[...]\nexpecting success:\ngit -C shallow12 fetch --shallow-exclude one origin &&\ngit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\ntest_write_lines three two >expected &&\ntest_cmp expected actual\n\n++ git -C shallow12 fetch --shallow-exclude one origin\ntrace: built-in: git fetch --shallow-exclude one origin\ntrace: run_command: unset GIT_PREFIX; 'git-upload-pack\n'\\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\\'''\ntrace: run_command: git --shallow-file  pack-objects --revs --thin\n--stdout --shallow --progress --delta-base-offset --include-tag\ntrace: built-in: git pack-objects --revs --thin --stdout --shallow\n--progress --delta-base-offset --include-tag\nremote: Counting objects: 4, done.\nremote: Compressing objects: 100% (3/3), done.\nremote: Total 4 (delta 0), reused 0 (delta 0)\ntrace: run_command: git --shallow-file  unpack-objects --pack_header=2,4\ntrace: built-in: git unpack-objects --pack_header=2,4\nUnpacking objects: 100% (4/4), done.\ntrace: run_command: git rev-list --objects --stdin --not --all --quiet\ntrace: built-in: git rev-list --objects --stdin --not --all --quiet\ntrace: run_command: unset GIT_PREFIX; 'git-upload-pack\n'\\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\\'''\ntrace: run_command: git pack-objects --revs --thin --stdout --progress\n--delta-base-offset\ntrace: built-in: git pack-objects --revs --thin --stdout --progress\n--delta-base-offset\nremote: Total 0 (delta 0), reused 0 (delta 0)\ntrace: run_command: git unpack-objects --pack_header=2,0\ntrace: built-in: git unpack-objects --pack_header=2,0\ntrace: run_command: git rev-list --objects --stdin --not --all --quiet\ntrace: built-in: git rev-list --objects --stdin --not --all --quiet\nFrom file:///u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.\n * [new tag]         one        -> one\n * [new tag]         two        -> two\nrun_processes_parallel: preparing to run up to 1 tasks\nrun_processes_parallel: done\ntrace: run_command: git gc --auto\ntrace: built-in: git gc --auto\n++ git -C shallow12 log --pretty=tformat:%s origin/master\ntrace: built-in: git log '--pretty=tformat:%s' origin/master\n++ test_write_lines three two\n++ printf '%s\\n' three two\n++ test_cmp expected actual\n++ diff -u expected actual\n--- expected 2018-05-31 18:14:25.944540810 +0000\n+++ actual 2018-05-31 18:14:25.944540810 +0000\n@@ -1,2 +1,3 @@\n three\n two\n+one\nerror: last command exited with $?=1\nnot ok 348 - fetch exclude tag one\n#\n# git -C shallow12 fetch --shallow-exclude one origin &&\n# git -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n# test_write_lines three two >expected &&\n# test_cmp expected actual\n#\n\n\n> After these changes, there is one test case that still fails, and I\n> cannot understand why:\n>\n> t5500-fetch-pack.sh                     Failed test:  348\n>\n> This test fails when performing a `git fetch --shallow-exclude`. When I\n> halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the\n> directory to replay the fetch it performs as expected.\n\nWhat is \"as expected\" ?\n\nWhen I insert a test_pause before that first line, such that:\n\ntest_expect_success 'fetch exclude tag one' '\n    test_pause &&\n    git -C shallow12 fetch --shallow-exclude one origin &&\n    git -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n    test_write_lines three two >expected &&\n    test_cmp expected actual\n'\n\nand then run\n\n  rm \"shallow-exclude/.git/objects/info/commit-graph\n\nthe test works after exiting the test_pause.\n\nNote how the shallow-exclude is the *remote* of the fetch.\nSo I think you also want to introduce the destruction\nof commit graphs in upload-pack.c which is the remote counter part to fetch.\n\nWhy do you think these other tests are noice?\n\nThanks,\nStefan\n"},{"id":"348922","messageId":"CAGZ79kYcrTHzj_6S5gV2e72DhZf_LTAKHiwX+HZY2Tskk0eqYw@mail.gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-5-dstolee@microsoft.com","subject":"Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T19:07:41Z","receivedAt":"2018-05-31T19:07:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee <dstolee@microsoft.com> wrote:\n> Shallow clones do not interact well with the commit-graph feature for\n> several reasons. Instead of doing the hard thing to fix those\n> interactions, instead prevent reading or writing a commit-graph file for\n> shallow repositories.\n\nMakes sense.\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  commit-graph.c | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 95af4ed519..80e377b90f 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -208,6 +208,9 @@ static void prepare_commit_graph(void)\n>                 return;\n>         prepare_commit_graph_run_once = 1;\n>\n> +       if (is_repository_shallow())\n> +               return;\n> +\n>         obj_dir = get_object_directory();\n>         prepare_commit_graph_one(obj_dir);\n>         prepare_alt_odb();\n> @@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,\n>         int num_extra_edges;\n>         struct commit_list *parent;\n>\n> +       /*\n> +        * Shallow clones are not supproted, as they create bad\n\nsupported\n\n> +        * generation skips as they are un-shallowed.\n> +        */\n> +       if (is_repository_shallow()) {\n> +               warning(\"writing a commit-graph in a shallow repository is not supported\");\n\n_() ?\n"},{"id":"348925","messageId":"CAGZ79kZp3SG8D_aap7k7TDUaGsA7_8ufhkiJK7VR4nGQohVDDQ@mail.gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-6-dstolee@microsoft.com","subject":"Re: [RFC PATCH 5/6] fetch: destroy commit graph on shallow parameters","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T19:29:47Z","receivedAt":"2018-05-31T19:29:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee <dstolee@microsoft.com> wrote:\n> The commit-graph feature is not compatible with history-rewriting\n> features such as shallow clones.\n\nI associate \"history rewriting\" with changing objects in the history.\nFor example interactive rebase or the BFG cleaner[1] / filter-branch\nto remove certain commits from other commits as parents.\nThis history rewriting leads to different sha1s, and the commit\ngraph feature is compatible with that in the sense that you can\nadd all the new sha1s /commits to the graph and prune out the\nold unreferenced commits.\n\nShallow clones are not rewriting history IMHO, as the sha1s do\nnot change. What changes is the assumption of presence of\nthe parent commits (which makes it hard to compute the\ngeneration number), by the grafting trick, that \"overlays\" (?)\nhistory.\n\nThis is more of a nit, though.\n\n[1] https://rtyley.github.io/bfg-repo-cleaner/\n\n> When running a 'git fetch' with\n> any of the shallow/unshallow options, destroy the commit-graph\n> file and override core.commitGraph to be false.\n\nWe do that *before* the actual fetch happens such that\nthe improved negotiation of the future cannot even try to\nbenefit from generation numbers?\n\nWe do it at fetch time instead of other local operations\nas that is an entry point to commit-graph incompatible\nfeatures. Would this also be needed in clone?\n\nI was about to ask if a more fine grained inclusion to\nlookup_commit would make sense, but that is explicitely\ncalled out in the cover letter as 'too hard for now'.\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  builtin/fetch.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index af896e4b74..2001dca881 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1452,6 +1452,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>                 }\n>         }\n>\n> +       if (update_shallow || depth || deepen_since || deepen_not.nr ||\n> +           deepen_relative || unshallow || update_shallow || is_repository_shallow()) {\n> +               destroy_commit_graph(get_object_directory());\n> +               core_commit_graph = 0;\n"},{"id":"348927","messageId":"CAGZ79kZ=Zmd8i9Z565MU=QCQGXixwkRYPY5RKekua67Lq+b2=g@mail.gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-2-dstolee@microsoft.com","subject":"Re: [RFC PATCH 1/6] DO NOT MERGE: compute commit-graph on every commit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-31T19:39:02Z","receivedAt":"2018-05-31T19:39:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 10:41 AM, Derrick Stolee <dstolee@microsoft.com> wrote:\n> Also enable core.commitGraph and gc.commitGraph. Helps to test the\n> commit-graph feature with the rest of the test infrastructure.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  builtin/commit.c | 5 +++++\n>  builtin/gc.c     | 2 +-\n>  environment.c    | 2 +-\n>  3 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e8e8d13be4..8751b816c1 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -32,6 +32,7 @@\n>  #include \"column.h\"\n>  #include \"sequencer.h\"\n>  #include \"mailmap.h\"\n> +#include \"commit-graph.h\"\n>\n>  static const char * const builtin_commit_usage[] = {\n>         N_(\"git commit [<options>] [--] <pathspec>...\"),\n> @@ -1623,5 +1624,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>\n>         UNLEAK(err);\n>         UNLEAK(sb);\n> +\n> +       if (core_commit_graph)\n> +               write_commit_graph_reachable(get_object_directory(), 1);\n> +\n\nI'd personally put it before the UNLEAKS, as then\nwe have the cleanup at the end of the function and\nnot scattered somewhere in the middle.\n"},{"id":"348952","messageId":"2cb85aa9-7c43-78ce-80c6-15d882d9a18b@gmail.com","threadId":"48617","inReplyTo":"CAGZ79kYCi_=LRuq35Fh0jGYw9kiW9i=6t1YwLuNj7MfezywZDw@mail.gmail.com","subject":"Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-06-01T01:09:35Z","receivedAt":"2018-06-01T01:09:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/31/2018 2:33 PM, Stefan Beller wrote:\n> On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee <dstolee@microsoft.com> wrote:\n>> The commit-graph file stores a condensed version of the commit history.\n>> This helps speed up several operations involving commit walks. This\n>> feature does not work well if those commits \"change\" using features like\n>> commit grafts, replace objects, or shallow clones.\n>>\n>> Since the commit-graph feature is optional, hidden behind the\n>> 'core.commitGraph' config option, and requires manual maintenance (until\n>> ds/commit-graph-fsck' is merged), these issues only arise for expert\n>> users who decided to opt-in.\n>>\n>> This RFC is a first attempt at rectify the issues that come up when\n>> these features interact. I have not succeeded entirely, as I will\n>> discuss below.\n>>\n>> The first two \"DO NOT MERGE\" commits are not intended to be part of the\n>> main product, but are ways to help the full test suite run while\n>> computing and consuming commit-graph files. This is acheived by calling\n>> write_commit_graph_reachable() from `git fetch` and `git commit`. I\n>> believe this is the only dependence on ds/commit-graph-fsck. The other\n>> commits should only depend on ds/commit-graph-lockfile-fix.\n> I applied these patches on top of ds/commit-graph-fsck\n> (it would have been nice if you mentioned that it applies there)\n> Running the test suite with all patches applied results in:\n>\n> ./t0410-partial-clone.sh                    (Wstat: 256 Tests: 15 Failed: 2)\n>    Failed tests:  5, 8\n> ./t5307-pack-missing-commit.sh              (Wstat: 256 Tests: 5 Failed: 2)\n>    Failed tests:  3-4\n> ./t5500-fetch-pack.sh                       (Wstat: 256 Tests: 353 Failed: 1)\n>    Failed test:  348\n> ./t6011-rev-list-with-bad-commit.sh         (Wstat: 256 Tests: 6 Failed: 1)\n>    Failed test:  4\n> ./t6024-recursive-merge.sh                  (Wstat: 256 Tests: 6 Failed: 1)\n>    Failed test:  4\n>\n> which you identified as 4x noise and t5500 as not understood.\n>\n> $ GIT_TRACE=1 ./t5500-fetch-pack.sh -d -i -v -x\n> [...]\n> expecting success:\n> git -C shallow12 fetch --shallow-exclude one origin &&\n> git -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n> test_write_lines three two >expected &&\n> test_cmp expected actual\n>\n> ++ git -C shallow12 fetch --shallow-exclude one origin\n> trace: built-in: git fetch --shallow-exclude one origin\n> trace: run_command: unset GIT_PREFIX; 'git-upload-pack\n> '\\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\\'''\n> trace: run_command: git --shallow-file  pack-objects --revs --thin\n> --stdout --shallow --progress --delta-base-offset --include-tag\n> trace: built-in: git pack-objects --revs --thin --stdout --shallow\n> --progress --delta-base-offset --include-tag\n> remote: Counting objects: 4, done.\n> remote: Compressing objects: 100% (3/3), done.\n> remote: Total 4 (delta 0), reused 0 (delta 0)\n> trace: run_command: git --shallow-file  unpack-objects --pack_header=2,4\n> trace: built-in: git unpack-objects --pack_header=2,4\n> Unpacking objects: 100% (4/4), done.\n> trace: run_command: git rev-list --objects --stdin --not --all --quiet\n> trace: built-in: git rev-list --objects --stdin --not --all --quiet\n> trace: run_command: unset GIT_PREFIX; 'git-upload-pack\n> '\\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\\'''\n> trace: run_command: git pack-objects --revs --thin --stdout --progress\n> --delta-base-offset\n> trace: built-in: git pack-objects --revs --thin --stdout --progress\n> --delta-base-offset\n> remote: Total 0 (delta 0), reused 0 (delta 0)\n> trace: run_command: git unpack-objects --pack_header=2,0\n> trace: built-in: git unpack-objects --pack_header=2,0\n> trace: run_command: git rev-list --objects --stdin --not --all --quiet\n> trace: built-in: git rev-list --objects --stdin --not --all --quiet\n>  From file:///u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.\n>   * [new tag]         one        -> one\n>   * [new tag]         two        -> two\n> run_processes_parallel: preparing to run up to 1 tasks\n> run_processes_parallel: done\n> trace: run_command: git gc --auto\n> trace: built-in: git gc --auto\n> ++ git -C shallow12 log --pretty=tformat:%s origin/master\n> trace: built-in: git log '--pretty=tformat:%s' origin/master\n> ++ test_write_lines three two\n> ++ printf '%s\\n' three two\n> ++ test_cmp expected actual\n> ++ diff -u expected actual\n> --- expected 2018-05-31 18:14:25.944540810 +0000\n> +++ actual 2018-05-31 18:14:25.944540810 +0000\n> @@ -1,2 +1,3 @@\n>   three\n>   two\n> +one\n> error: last command exited with $?=1\n> not ok 348 - fetch exclude tag one\n> #\n> # git -C shallow12 fetch --shallow-exclude one origin &&\n> # git -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n> # test_write_lines three two >expected &&\n> # test_cmp expected actual\n> #\n>\n>\n>> After these changes, there is one test case that still fails, and I\n>> cannot understand why:\n>>\n>> t5500-fetch-pack.sh                     Failed test:  348\n>>\n>> This test fails when performing a `git fetch --shallow-exclude`. When I\n>> halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the\n>> directory to replay the fetch it performs as expected.\n> What is \"as expected\" ?\n>\n> When I insert a test_pause before that first line, such that:\n>\n> test_expect_success 'fetch exclude tag one' '\n>      test_pause &&\n>      git -C shallow12 fetch --shallow-exclude one origin &&\n>      git -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n>      test_write_lines three two >expected &&\n>      test_cmp expected actual\n> '\n>\n> and then run\n>\n>    rm \"shallow-exclude/.git/objects/info/commit-graph\n>\n> the test works after exiting the test_pause.\n>\n> Note how the shallow-exclude is the *remote* of the fetch.\n> So I think you also want to introduce the destruction\n> of commit graphs in upload-pack.c which is the remote counter part to fetch.\n\nThanks for the details above. I agree that the solution is probably to \ndisable the commit-graph during certain upload-pack parameters. \nSomething is interfering there. I don't think the \"destroy\" pattern is \nright here.\n\n> Why do you think these other tests are noice?\n\nTake t5307-pack-missing-commit.sh for a typical example: a repo is \nconstructed, a pack is made, and then that pack is manipulated to \n\"remove\" a commit. That missing commit is \"detected\" by running \nrev-list. Except if we are running the commit-graph on every 'git \ncommit' call, the rev-list now succeeds since we are not looking at the \npackfile for the commit details any more!\n\nThis is what I mean by the tests being \"noise\". Adding the \"DO NOT \nMERGE\" commits only interrupted the test mechanism for unrelated behavior.\n\nThanks,\n\n-Stolee\n\n"},{"id":"348964","messageId":"xmqqd0xb9r1k.fsf@gitster-ct.c.googlers.com","threadId":"48617","inReplyTo":"20180531174024.124488-5-dstolee@microsoft.com","subject":"Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-01T02:30:47Z","receivedAt":"2018-06-01T02:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <dstolee@microsoft.com> writes:\n\n> Shallow clones do not interact well with the commit-graph feature for\n> several reasons. Instead of doing the hard thing to fix those\n> interactions, instead prevent reading or writing a commit-graph file for\n> shallow repositories.\n\nThe latter instead would want to vanish, I would guess.\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  commit-graph.c | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 95af4ed519..80e377b90f 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -208,6 +208,9 @@ static void prepare_commit_graph(void)\n>  \t\treturn;\n>  \tprepare_commit_graph_run_once = 1;\n>  \n> +\tif (is_repository_shallow())\n> +\t\treturn;\n> +\n>  \tobj_dir = get_object_directory();\n>  \tprepare_commit_graph_one(obj_dir);\n>  \tprepare_alt_odb();\n> @@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,\n>  \tint num_extra_edges;\n>  \tstruct commit_list *parent;\n>  \n> +\t/*\n> +\t * Shallow clones are not supproted, as they create bad\n> +\t * generation skips as they are un-shallowed.\n> +\t */\n> +\tif (is_repository_shallow()) {\n> +\t\twarning(\"writing a commit-graph in a shallow repository is not supported\");\n> +\t\treturn;\n> +\t}\n> +\n>  \toids.nr = 0;\n>  \toids.alloc = approximate_object_count() / 4;\n"},{"id":"349000","messageId":"95354da4-7590-4fa6-de83-30f883d77188@gmail.com","threadId":"48617","inReplyTo":"xmqqd0xb9r1k.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-06-01T11:46:08Z","receivedAt":"2018-06-01T11:46:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/31/2018 10:30 PM, Junio C Hamano wrote:\n> Derrick Stolee <dstolee@microsoft.com> writes:\n>\n>> Shallow clones do not interact well with the commit-graph feature for\n>> several reasons. Instead of doing the hard thing to fix those\n>> interactions, instead prevent reading or writing a commit-graph file for\n>> shallow repositories.\n> The latter instead would want to vanish, I would guess.\n\nDo you mean that we should call destroy_commit_graph() if we detect a \nshallow repository during write_commit_graph(), then I can make that change.\n\n>\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>>   commit-graph.c | 12 ++++++++++++\n>>   1 file changed, 12 insertions(+)\n>>\n>> diff --git a/commit-graph.c b/commit-graph.c\n>> index 95af4ed519..80e377b90f 100644\n>> --- a/commit-graph.c\n>> +++ b/commit-graph.c\n>> @@ -208,6 +208,9 @@ static void prepare_commit_graph(void)\n>>   \t\treturn;\n>>   \tprepare_commit_graph_run_once = 1;\n>>   \n>> +\tif (is_repository_shallow())\n>> +\t\treturn;\n>> +\n>>   \tobj_dir = get_object_directory();\n>>   \tprepare_commit_graph_one(obj_dir);\n>>   \tprepare_alt_odb();\n>> @@ -711,6 +714,15 @@ void write_commit_graph(const char *obj_dir,\n>>   \tint num_extra_edges;\n>>   \tstruct commit_list *parent;\n>>   \n>> +\t/*\n>> +\t * Shallow clones are not supproted, as they create bad\n>> +\t * generation skips as they are un-shallowed.\n>> +\t */\n>> +\tif (is_repository_shallow()) {\n>> +\t\twarning(\"writing a commit-graph in a shallow repository is not supported\");\n>> +\t\treturn;\n>> +\t}\n>> +\n>>   \toids.nr = 0;\n>>   \toids.alloc = approximate_object_count() / 4;\n\n"},{"id":"349096","messageId":"86r2lpqbgq.fsf@gmail.com","threadId":"48617","inReplyTo":"95354da4-7590-4fa6-de83-30f883d77188@gmail.com","subject":"Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-06-02T18:39:49Z","receivedAt":"2018-06-02T18:39:58Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n> On 5/31/2018 10:30 PM, Junio C Hamano wrote:\n>> Derrick Stolee <dstolee@microsoft.com> writes:\n>>\n>>> Shallow clones do not interact well with the commit-graph feature for\n>>> several reasons. Instead of doing the hard thing to fix those\n>>> interactions, instead prevent reading or writing a commit-graph file for\n>>> shallow repositories.\n>>\n>> The latter instead would want to vanish, I would guess.\n>\n> Do you mean that we should call destroy_commit_graph() if we detect a\n> shallow repository during write_commit_graph(), then I can make that\n> change.\n\nI think Junio meant here the \"instead\" word, because you have it twice\nin the second sentence of quoted paragraph.\n\n-- \nJakub Narębski\n"},{"id":"349201","messageId":"xmqqd0x770p4.fsf@gitster-ct.c.googlers.com","threadId":"48617","inReplyTo":"95354da4-7590-4fa6-de83-30f883d77188@gmail.com","subject":"Re: [RFC PATCH 4/6] commit-graph: avoid writing when repo is shallow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T02:19:35Z","receivedAt":"2018-06-04T02:19:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>>> several reasons. Instead of doing the hard thing to fix those\n>>> interactions, instead prevent reading or writing a commit-graph file for\n>>> shallow repositories.\n>> The latter instead would want to vanish, I would guess.\n>\n> Do you mean that we should call destroy_commit_graph() if we detect a\n> shallow repository during write_commit_graph(), then I can make that\n> change.\n>\n\nNo, I was just having trouble with reading \"Instead of doing X,\ninstead do Y\".\n"},{"id":"349689","messageId":"867en9pjyb.fsf@gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-1-dstolee@microsoft.com","subject":"Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-06-08T11:59:56Z","receivedAt":"2018-06-08T12:00:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Derrick Stolee <dstolee@microsoft.com> writes:\n\n> The commit-graph file stores a condensed version of the commit history.\n> This helps speed up several operations involving commit walks. This\n> feature does not work well if those commits \"change\" using features like\n> commit grafts, replace objects, or shallow clones.\n\nI like to think about those features as providing an overlay for the\ncommit graph (similar to overlay filesystems, like overlayfs); in the\ncase of git-replace quite literally.\n\nI will be calling all those features \"history-altering features\", for\nshort.\n\n> Since the commit-graph feature is optional, hidden behind the\n> 'core.commitGraph' config option, and requires manual maintenance (until\n> ds/commit-graph-fsck' is merged), these issues only arise for expert\n> users who decided to opt-in.\n>\n> This RFC is a first attempt at rectify the issues that come up when\n> these features interact. I have not succeeded entirely, as I will\n> discuss below.\n\nWhat I would like to see here in cover letter is a generic description\nof _strategy_ to deal with those history-altering features.\n\nFrom what I understand you have the following options for each of\nreplacements, shallow clones and grafts:\n - turn off serialized commit-graph if given history-altering feature is\n   present, as if core.commitGraph was set to false\n - invalidate and optionally refresh commit-graph file if given\n   history-altering feature is present (maybe only if it changed the\n   view of the history, is such check is possible)\n\nFor automatic invalidation you would need to have either:\n - cover all possible ways by which given history-altering feature can\n   change the view of history, or\n - keep state of history-altering change for which commit-graph was\n   created (e.g. in proposed VAL4 chunk) and compare with current view\n   of history if it changed\n\nFor automatic turning off you would need only to check if\nhistory-altering feature is present.\n\n\nLet us examine each of those history-altering features that Git\nsupports:\n\n* shallow clone\n\n  - shallow clone usually means having shorter history, so serialized\n    commit-graph is not needed as much; also changing the depth\n    screws-up assumptions about generation numbers\n\n  - there are only a few entry points changing the view of history,\n    namely fetch and push with shallow options (--depth=<depth>,\n    --deepen=<depth>, --shallow-since=<date>, --update-shallow,\n    --shallow-exclude=<revision>, --unshallow)\n\n  - it is easy to check for presence of shallow clone feature by\n    chacking of $GIT_DIR/shallow exists and is not empty\n\n  - different contents of $GIT_DIR/shallow means different view of\n    history\n\n  - NOTE: internally uses grafts mechanism (emulated grafts)\n\n* replacements (replace objects, git-replace)\n\n  - git-replace can be used to join current development repository with\n    historical repository, in which case we would certainly want to make\n    use of serialied commit graph; on the other hand the replacements do\n    not necessary alter the view of the history\n\n  - theoretically you could create replacement refs by hand, but in\n    practice there are TWO ways of getting them:\n\n    - using git-replace command to create, edit/change and delete\n      replacement objects'\n      '\n    - fetching or having pushed-to refs in refs/replace/* namespace\n\n  - you need to check if there are any refs in refs/replace/* namespace\n    to check if the feature is used (but it doesn't necessarily mean\n    that it altered project history)\n\n  - changed list of refs in refs/replace/* namespace (which you can get\n    with for-each-ref command/API) does not necessarily mean that the\n    view of the history changed: you can replace non-commit object, you\n    can replace commits and not change their parents; it is not as easy\n    as checking if file exists\n\n  - NOTE: the feature can be turned off manually with\n    GIT_NO_REPLACE_OBJECTS environment variable and with\n    --no-replace-objects option to git wrapper.\n\n    Also when pushing, fetching and fsck-ing this feature is turned off\n    and refs in refs/replace/* namespace are treated as ordinary refs.\n\n    This may mean that we would want to create commit-graph with\n    replacements for ordinary non-bare repository, and without\n    replacements for server-side bare repository.\n\n* grafts\n\n  - subset of uses of git-replace, older and *obsolete* feature (because\n    it is unsafe to use; that is you need to be careful with it).\n\n  - edited by hand, no automatic entry points\n\n  - if $GIT_DIR/info/grafts file is present, then feature is enabled\n    (barring some corner cases, like empty file or file consisting only\n    of comments)\n\n  - changed contents of this file means changed view of history (well,\n    except for reordering lines, or removing/adding empty lines and\n    comments)\n\n>\n> The first two \"DO NOT MERGE\" commits are not intended to be part of the\n> main product, but are ways to help the full test suite run while\n> computing and consuming commit-graph files. This is acheived by calling\n> write_commit_graph_reachable() from `git fetch` and `git commit`. I\n> believe this is the only dependence on ds/commit-graph-fsck. The other\n> commits should only depend on ds/commit-graph-lockfile-fix.\n\nThat's clever way of increasing coverage.\n\n> Running the full test suite after these DO NO MERGE commits results in\n> the following test failures, which I categorize by type of failure.\n>\n> The following tests are red herrings. Most work by deleting a commit\n> from the object database and trying to demonstrate a failure. Others\n> rely on how certain objects are loaded. These are not bugs, but will\n> add noise if you run the tests suite with this patch.\n>\n> t0410-partial-clone.sh\t\tFailed tests:  5, 8\n> t5307-pack-missing-commit.sh\t\tFailed tests:  3-4\n> t6011-rev-list-with-bad-commit.sh\tFailed test:  4\n> t6024-recursive-merge.sh\t\tFailed test:  4\n\nDoes this means that those tests should be in the end (i.e. when\ncore.commitGraph is turned on by defult) be simply run with\ncore.commitGraph explicitly disabled for the test?\n\n> The following tests are fixed in \"commit-graph: enable replace-object\n> and grafts\".\n\nWould it make sense to split this commit into two dealing separately\nwith replace objects and with grafts?  Or do they use the same\nunderlying API?\n\n> t6001-rev-list-graft.sh\t\tFailed tests:  3, 5, 7, 9, 11, 13\n\nO.K.  It might be good idea to add separate test that does the same, but\nwith git-replace instead of grafts, though.\n\n> t6050-replace.sh\t\t\tFailed tests:  11-15, 23-24, 29\n\nThe t6050-replace.sh does not test changing the DAG of revision\n(excluding changing the SHA-1 of commit), if I read it correctly.  It\nwould be good to test using git-replace to change committer date, and to\nchange parents: shortening or lengthening history (e.g. emulating\njoining two independent lines of development in the latter case).\n\nSee also comment about t6001 above.\n\n>\n> The following tests involve shallow clones.\n>\n> t5500-fetch-pack.sh\t\t\tFailed tests:  30-31, 34, 348-349\n> t5537-fetch-shallow.sh\t\tFailed tests:  4-7, 9\n> t5802-connect-helper.sh\t\tFailed test:  3\n>\n> These tests are mostly fixed by three commits:\n>\n> * commit-graph: avoid writing when repo is shallow\n> * fetch: destroy commit graph on shallow parameters\n\nSeems O.K. from the subject, but I'd have to check the details.\n\n> * commit-graph: revert to odb on missing parents\n\nI wonder if reverting to using object database is a good solution, and\nif it wouldn't be better to invalidate / delete commit-graph file\ninstead...\n\n>\n> Each of these cases cover a different interaction that occurs with\n> shallow clones. Some are due to a commit becoming shallow, while others\n> are due to a commit becoming unshallow (and hence invalidating its\n> generation number).\n\nWhy do not simply check if repository is shallow?\n\n>\n> After these changes, there is one test case that still fails, and I\n> cannot understand why:\n>\n> t5500-fetch-pack.sh\t\t\tFailed test:  348\n>\n> This test fails when performing a `git fetch --shallow-exclude`. When I\n> halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the\n> directory to replay the fetch it performs as expected. After struggling\n> with it for a while, I figured I should just put this series up for\n> discussion. Maybe someone with more experience in shallow clones can\n> point out the obvious issues I'm having.\n>\n> Thanks,\n> -Stolee\n>\n> Derrick Stolee (6):\n>   DO NOT MERGE: compute commit-graph on every commit\n>   DO NOT MERGE: write commit-graph on every fetch\n>   commit-graph: enable replace-object and grafts\n>   commit-graph: avoid writing when repo is shallow\n>   fetch: destroy commit graph on shallow parameters\n>   commit-graph: revert to odb on missing parents\n>\n>  builtin/commit.c  |  5 +++++\n>  builtin/fetch.c   | 10 +++++++++\n>  builtin/gc.c      |  2 +-\n>  builtin/replace.c |  3 +++\n>  commit-graph.c    | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++----\n>  commit-graph.h    |  9 ++++++++\n>  commit.c          |  5 +++++\n>  environment.c     |  2 +-\n>  8 files changed, 95 insertions(+), 6 deletions(-)\n"},{"id":"349805","messageId":"86h8mcynal.fsf@gmail.com","threadId":"48617","inReplyTo":"20180531174024.124488-4-dstolee@microsoft.com","subject":"Re: [RFC PATCH 3/6] commit-graph: enable replace-object and grafts","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-06-09T15:47:30Z","receivedAt":"2018-06-09T15:47:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Derrick Stolee <dstolee@microsoft.com> writes:\n\nFirst, this commit seems to try to do two things at once, and in its\ncurrent form it should be split into adding destroy_commit_graph() and\ninto grafts / replacements check.  Those are separate and unconnected\nfeatures, and should be in separate patches, in my opinion.\n\n> Create destroy_commit_graph() method to delete the commit-graph file\n> when history is altered by a replace-object call. If the commit-graph\n> is rebuilt after that, we will load the correct object while reading\n> the commit-graph.\n\nI'm not sure if this is the best solution.  It would work both for\ninvalidation, and for turning off the feature, but in my opinion there\nare better solutions for both:\n - in case of invalidation, wouldn't it be better to re-create the file?\n - in case of turning off, wouldn't it be better to simply set\n   core_commit_graph to false?\n\nNevertheless I think destroy_commit_graph(), however it would be named\nat the end, would be a good to have.\n\n> When parsing a commit, first check if the commit was grafted. If so,\n> then ignore the commit-graph for that commit and insted use the parents\n> loaded by parsing the commit buffer and comparing against the graft\n> file.\n\nUnless uyou turn off using generation numbers and other such indices,\nthis is not enough to prevent from generating incorrect results if\ncurrent commit-graph file doesn't agree with current altered view of the\nproject history.\n\nTake a simple example of joining two histories using replacements\n(git-replace) or grafts.  Numbers denote generation numbers:\n\nOriginal history:\n\n    A<---B<---C             <===  master\n    1    2    3\n\n\n    a<---b<---c<---d<---e   <===  historical\n    1    2    3    4    5\n\nAltered view of history:\n\n    a<---b<---c<---d<---e<---[A]<---B<---C   <===  master\n\nIn the new view of history, 'e' is reachable from 'C'.  But going by\npre-altering generation numbers, gen_{pre}(e) = 5  >  gen_{pre}(C) = 3.\nThis means that we don't even arrive at '[A]', where replacement object\nor graft changed parents compared to what's loaded from commit-graph\nfile, isn't it?\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  builtin/replace.c |  3 +++\n>  commit-graph.c    | 20 +++++++++++++++++++-\n>  commit-graph.h    |  9 +++++++++\n>  commit.c          |  5 +++++\n>  4 files changed, 36 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 9f01f3fc7f..d553aadcdc 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -15,6 +15,7 @@\n>  #include \"parse-options.h\"\n>  #include \"run-command.h\"\n>  #include \"tag.h\"\n> +#include \"commit-graph.h\"\n>  \n>  static const char * const git_replace_usage[] = {\n>  \tN_(\"git replace [-f] <object> <replacement>\"),\n> @@ -468,6 +469,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n>  \t\tusage_msg_opt(\"--raw only makes sense with --edit\",\n>  \t\t\t      git_replace_usage, options);\n>  \n> +\tdestroy_commit_graph(get_object_directory());\n> +\n>  \tswitch (cmdmode) {\n>  \tcase MODE_DELETE:\n>  \t\tif (argc < 1)\n> diff --git a/commit-graph.c b/commit-graph.c\n> index e9195dfb17..95af4ed519 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -6,6 +6,7 @@\n>  #include \"pack.h\"\n>  #include \"packfile.h\"\n>  #include \"commit.h\"\n> +#include \"dir.h\"\n>  #include \"object.h\"\n>  #include \"refs.h\"\n>  #include \"revision.h\"\n> @@ -240,15 +241,22 @@ static struct commit_list **insert_parent_or_die(struct commit_graph *g,\n>  {\n>  \tstruct commit *c;\n>  \tstruct object_id oid;\n> +\tconst unsigned char *real;\n>  \n>  \tif (pos >= g->num_commits)\n>  \t\tdie(\"invalid parent position %\"PRIu64, pos);\n>  \n>  \thashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);\n> +\n> +\treal = lookup_replace_object(oid.hash);\n\nSo this function returns either original SHA-1 / OID of object, or if\nobject should be replaced then it returns the replacement object's name\n(replaced recursively, if necessary).\n\nThis deals with replacements, but not with grafts (from graft file)!\n\n> +\n>  \tc = lookup_commit(&oid);\n>  \tif (!c)\n>  \t\tdie(\"could not find commit %s\", oid_to_hex(&oid));\n> -\tc->graph_pos = pos;\n> +\n> +\tif (!hashcmp(real, oid.hash))\n> +\t\tc->graph_pos = pos;\n\nI don't quite understand this reasoning here.  The serialized\ncommit-graph file can either follow the current state of altered\nhistory, or not.  In the later case the commit-graph can either follow\noriginal history, or altered but not current view of history.\n\nWhat does it matter of the object was replaced or not when marking\nobject as coming from graph at position 'pos'?  If commit is replaced,\nit still comes from commit-graph at position 'pos', isn't it?\n\nAlso, what if the starting commit was replaced?  Then this logic\nwouldn't file, is it?\n\n\nI don't see how it relates to what's written in the commit message:\n\nDS> When parsing a commit, first check if the commit was grafted. If so,\nDS> then ignore the commit-graph for that commit and insted use the parents\nDS> loaded by parsing the commit buffer and comparing against the graft\nDS> file.\n\n> +\n>  \treturn &commit_list_insert(c, pptr)->next;\n>  }\n>  \n> @@ -1019,3 +1027,13 @@ int verify_commit_graph(struct commit_graph *g)\n>  \n>  \treturn verify_commit_graph_error;\n>  }\n> +\n> +void destroy_commit_graph(const char *obj_dir)\n> +{\n> +\tchar *graph_name;\n> +\tclose_commit_graph();\n> +\n> +\tgraph_name = get_commit_graph_filename(obj_dir);\n> +\tremove_path(graph_name);\n> +\tFREE_AND_NULL(graph_name);\n> +}\n\nAll right, though I onder if it wouldn't be better to first\ndelete/remove/unlink the graph file, and only then close it.\n\nOtherwise I think you might be introducing subtle race condition here.\nBut I may be wrong here.\n\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 9a06a5f188..1d17da1582 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -56,4 +56,13 @@ void write_commit_graph(const char *obj_dir,\n>  \n>  int verify_commit_graph(struct commit_graph *g);\n>  \n> +/*\n> + * Delete the commit-graph file in the given object directory.\n> + *\n> + * WARNING: this deletes data, so should only be used when\n> + * performing history-altering actions like replace-object\n> + * or grafts.\n> + */\n> +void destroy_commit_graph(const char *obj_dir);\n\nWhy it is not called delete_commit_graph(), then?\n\n> +\n>  #endif\n> diff --git a/commit.c b/commit.c\n> index 6eaed0174c..2fe31cde77 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -403,6 +403,11 @@ 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> +\n> +\tprepare_commit_graft();\n> +\tif (commit_graft_pos(item->object.oid.hash) >= 0)\n> +\t\tuse_commit_graph = 0;\n\nAll right, this allegedly handles grafts.\n\nI don't understand why you treat replacement objects in different place\nthan grafts; I can understand that you might want to treat them\ndifferently (because grafts are deprecated).\n\n> +\n>  \tif (use_commit_graph && parse_commit_in_graph(item))\n>  \t\treturn 0;\n>  \tbuffer = read_sha1_file(item->object.oid.hash, &type, &size);\n"}]}