{"thread":{"id":"48907","subject":"[PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","startedAt":"2018-07-18T15:15:38Z","lastAt":"2018-08-21T18:40:00Z","messageCount":47,"participants":["Derrick Stolee via GitGitGadget","Stefan Beller via GitGitGadget","Derrick Stolee","Junio C Hamano","Stefan Beller","Jeff King","Jakub Narebski","Eric Sunshine","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"352872","messageId":"pull.11.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":null,"subject":"[PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:34Z","receivedAt":"2018-07-18T15:15:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"One unresolved issue with the commit-graph feature is that it can cause issues when combined with replace objects, commit grafts, or shallow clones. These are not 100% incompatible, as one could be reasonably successful writing a commit-graph after replacing some objects and not have issues. The problems happen when commits that are already in the commit-graph file are replaced, or when git is run with the `--no-replace-objects` option; this can cause incorrect parents or incorrect generation numbers. Similar things occur with commit grafts and shallow clones, especially when running `git fetch --unshallow` in a shallow repo.\n\nInstead of trying (and probably failing) to make these features work together, default to making the commit-graph feature unavailable in these situations. Create a new method 'commit_graph_compatible(r)' that checks if the repository 'r' has any of these features enabled.\n\nI will send a follow-up patch that shows how I tested these interactions by computing the commit-graph on every 'git commit'.\n\nThis approach works for most cases, but I found one nagging test case that was causing problems. This led to the commit \"commit-graph: close_commit_graph before shallow walk\" and is the patch I am least confident about. Please take a close look at that one and suggest alternatives.\n\nThis approach is very different from the previous RFC on the subject [1].\n\nWhile building this series, I got some test failures in the non-the_repository tests. These issues are related to missing references to an arbitrary repository (instead of the_repository) and some uninitialized values in the tests. Stefan already sent a patch to address this [2], and I've included those commits (along with a small tweak [3]). These are only included for convenience.\n\nThanks,\n-Stolee\n\n[1] https://public-inbox.org/git/20180531174024.124488-1-dstolee@microsoft.com/\n [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo\n\n[2] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#t\n [PATCH 0/2] RFC ref store to repository migration\n\n[3] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a\n [PATCH] TO-SQUASH: replace the_repository with arbitrary r\n\nBased-On: jt/commit-graph-per-object-store\nCc: jonathantanmy@google.com\nCc: sbeller@google.com\n\nDerrick Stolee (6):\n commit-graph: update design document\n test-repository: properly init repo\n commit-graph: not compatible with replace objects\n commit-graph: not compatible with grafts\n commit-graph: not compatible with uninitialized repo\n commit-graph: close_commit_graph before shallow walk\n\nStefan Beller (2):\n refs.c: migrate internal ref iteration to pass thru repository\n argument\n refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback\n\n Documentation/technical/commit-graph.txt | 18 ++++++--\n builtin/replace.c | 8 ++--\n commit-graph.c | 34 ++++++++++++--\n commit-graph.h | 1 +\n commit.c | 2 +-\n commit.h | 1 +\n refs.c | 48 +++++++++++++++++---\n refs.h | 12 ++++-\n refs/iterator.c | 6 +--\n refs/refs-internal.h | 5 +-\n replace-object.c | 7 +--\n replace-object.h | 2 +\n t/helper/test-repository.c | 10 +++-\n t/t5318-commit-graph.sh | 58 ++++++++++++++++++++++++\n upload-pack.c | 2 +\n 15 files changed, 184 insertions(+), 30 deletions(-)\n\n\nbase-commit: dade47c06cf849b0ca180a8e6383b55ea6f75812\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-11%2Fderrickstolee%2Fshallow%2Fupstream-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-11/derrickstolee/shallow/upstream-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/11\n-- \ngitgitgadget\n"},{"id":"352873","messageId":"43ddcc9ef9267d54fb521319175871e5bf7e4d12.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 1/8] refs.c: migrate internal ref iteration to pass thru repository argument","fromName":"Stefan Beller via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:35Z","receivedAt":"2018-07-18T15:15:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Stefan Beller \n\nIn 60ce76d3581 (refs: add repository argument to for_each_replace_ref,\n2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle\narbitrary repositories, 2018-04-11), for_each_replace_ref learned how\nto iterate over refs by a given arbitrary repository.\nNew attempts in the object store conversion have shown that it is useful\nto have the repository handle available that the refs iteration is\ncurrently iterating over.\n\nTo achieve this goal we will need to add a repository argument to\neach_ref_fn in refs.h. However as many callers rely on the signature\nsuch a patch would be too large.\n\nSo convert the internals of the ref subsystem first to pass through a\nrepository argument without exposing the change to the user. Assume\nthe_repository for the passed through repository, although it is not\nused anywhere yet.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Derrick Stolee \n---\n refs.c | 39 +++++++++++++++++++++++++++++++++++++--\n refs.h | 10 ++++++++++\n refs/iterator.c | 6 +++---\n refs/refs-internal.h | 5 +++--\n 4 files changed, 53 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fcfd3171e..2513f77ac 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1390,17 +1390,50 @@ struct ref_iterator *refs_ref_iterator_begin(\n * non-zero value, stop the iteration and return that value;\n * otherwise, return 0.\n */\n+static int do_for_each_repo_ref(struct repository *r, const char *prefix,\n+\t\t\t\teach_repo_ref_fn fn, int trim, int flags,\n+\t\t\t\tvoid *cb_data)\n+{\n+\tstruct ref_iterator *iter;\n+\tstruct ref_store *refs = get_main_ref_store(r);\n+\n+\tif (!refs)\n+\t\treturn 0;\n+\n+\titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n+\n+\treturn do_for_each_repo_ref_iterator(r, iter, fn, cb_data);\n+}\n+\n+struct do_for_each_ref_help {\n+\teach_ref_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_ref_helper(struct repository *r,\n+\t\t\t\t const char *refname,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct do_for_each_ref_help *hp = cb_data;\n+\n+\treturn hp->fn(refname, oid, flags, hp->cb_data);\n+}\n+\n static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n \t\t\t each_ref_fn fn, int trim, int flags, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \tif (!refs)\n \t\treturn 0;\n \n \titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\tdo_for_each_ref_helper, &hp);\n }\n \n int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n@@ -2029,10 +2062,12 @@ cleanup:\n int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\t do_for_each_ref_helper, &hp);\n }\n \n int for_each_reflog(each_ref_fn fn, void *cb_data)\ndiff --git a/refs.h b/refs.h\nindex cc2fb4c68..80eec8bbc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -274,6 +274,16 @@ struct ref_transaction;\n typedef int each_ref_fn(const char *refname,\n \t\t\tconst struct object_id *oid, int flags, void *cb_data);\n \n+/*\n+ * The same as each_ref_fn, but also with a repository argument that\n+ * contains the repository associated with the callback.\n+ */\n+typedef int each_repo_ref_fn(struct repository *r,\n+\t\t\t const char *refname,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int flags,\n+\t\t\t void *cb_data);\n+\n /*\n * The following functions invoke the specified callback function for\n * each reference indicated. If the function ever returns a nonzero\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 2ac91ac34..629e00a12 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \n struct ref_iterator *current_ref_iter = NULL;\n \n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t each_ref_fn fn, void *cb_data)\n+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,\n+\t\t\t\t each_repo_ref_fn fn, void *cb_data)\n {\n \tint retval = 0, ok;\n \tstruct ref_iterator *old_ref_iter = current_ref_iter;\n \n \tcurrent_ref_iter = iter;\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n-\t\tretval = fn(iter->refname, iter->oid, iter->flags, cb_data);\n+\t\tretval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);\n \t\tif (retval) {\n \t\t\t/*\n \t\t\t * If ref_iterator_abort() returns ITER_ERROR,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex dd834314b..5c7414bf0 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -473,8 +473,9 @@ extern struct ref_iterator *current_ref_iter;\n * adapter between the callback style of reference iteration and the\n * iterator style.\n */\n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t each_ref_fn fn, void *cb_data);\n+int do_for_each_repo_ref_iterator(struct repository *r,\n+\t\t\t\t struct ref_iterator *iter,\n+\t\t\t\t each_repo_ref_fn fn, void *cb_data);\n \n /*\n * Only include per-worktree refs in a do_for_each_ref*() iteration.\n-- \ngitgitgadget\n\n"},{"id":"352874","messageId":"22dc9ce8364dd20876ab5eb626ecc6a7396d3b3e.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Stefan Beller via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:37Z","receivedAt":"2018-07-18T15:15:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Stefan Beller \n\nSigned-off-by: Stefan Beller \nSigned-off-by: Derrick Stolee \n---\n builtin/replace.c | 8 ++++----\n refs.c | 9 ++++-----\n refs.h | 2 +-\n replace-object.c | 5 +++--\n 4 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex ef22d724b..d0b1cdb06 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -39,7 +39,8 @@ struct show_data {\n \tenum replace_format format;\n };\n \n-static int show_reference(const char *refname, const struct object_id *oid,\n+static int show_reference(struct repository *r, const char *refname,\n+\t\t\t const struct object_id *oid,\n \t\t\t int flag, void *cb_data)\n {\n \tstruct show_data *data = cb_data;\n@@ -56,9 +57,8 @@ static int show_reference(const char *refname, const struct object_id *oid,\n \t\t\tif (get_oid(refname, &object))\n \t\t\t\treturn error(\"Failed to resolve '%s' as a valid ref.\", refname);\n \n-\t\t\tobj_type = oid_object_info(the_repository, &object,\n-\t\t\t\t\t\t NULL);\n-\t\t\trepl_type = oid_object_info(the_repository, oid, NULL);\n+\t\t\tobj_type = oid_object_info(r, &object, NULL);\n+\t\t\trepl_type = oid_object_info(r, oid, NULL);\n \n \t\t\tprintf(\"%s (%s) -> %s (%s)\\n\", refname, type_name(obj_type),\n \t\t\t oid_to_hex(oid), type_name(repl_type));\ndiff --git a/refs.c b/refs.c\nindex 2513f77ac..5700cd468 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n \treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n }\n \n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_main_ref_store(r),\n-\t\t\t git_replace_ref_base, fn,\n-\t\t\t strlen(git_replace_ref_base),\n-\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n+\treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n+\t\t\t\t strlen(git_replace_ref_base),\n+\t\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \n int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\ndiff --git a/refs.h b/refs.h\nindex 80eec8bbc..a0a18223a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \t\t\t const char *prefix, void *cb_data);\ndiff --git a/replace-object.c b/replace-object.c\nindex 801b5c167..017f02f8e 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -6,7 +6,8 @@\n #include \"repository.h\"\n #include \"commit.h\"\n \n-static int register_replace_ref(const char *refname,\n+static int register_replace_ref(struct repository *r,\n+\t\t\t\tconst char *refname,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tint flag, void *cb_data)\n {\n@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,\n \toidcpy(&repl_obj->replacement, oid);\n \n \t/* Register new object */\n-\tif (oidmap_put(the_repository->objects->replace_map, repl_obj))\n+\tif (oidmap_put(r->objects->replace_map, repl_obj))\n \t\tdie(\"duplicate replace ref: %s\", refname);\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"352876","messageId":"5e90d36f8482e03df842d483bfe6f237f0247166.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 3/8] commit-graph: update design document","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:38Z","receivedAt":"2018-07-18T15:15:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nAs it exists right now, the commit-graph feature may provide\ninconsistent results when combined with commit grafts, replace objects,\nand shallow clones. Update the design document to discuss why these\ninteractions are difficult to reconcile and how we will avoid errors by\npreventing updates to and reads from the commit-graph file when these\nother features exist.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/technical/commit-graph.txt | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt\nindex c664acbd7..18948f11a 100644\n--- a/Documentation/technical/commit-graph.txt\n+++ b/Documentation/technical/commit-graph.txt\n@@ -112,12 +112,24 @@ Design Details\n - The file format includes parameters for the object ID hash function,\n so a future change of hash algorithm does not require a change in format.\n \n+- Commit grafts and replace objects can change the shape of the commit\n+ history. These can also be enabled/disabled on the fly using\n+ `--no-replace-objects`. This leads to difficultly storing both possible\n+ interpretations of a commit id, especially when computing generation\n+ numbers. The commit-graph will not be read or written when\n+ replace-objects or grafts are present.\n+\n+- Shallow clones create grafts of commits by dropping their parents. This\n+ leads the commit-graph to think those commits have generation number 1.\n+ If and when those commits are made unshallow, those generation numbers\n+ become invalid. Since shallow clones are intended to restrict the commit\n+ history to a very small set of commits, the commit-graph feature is less\n+ helpful for these clones, anyway. The commit-graph will not be read or\n+ written when shallow commits are present.\n+\n Future Work\n -----------\n \n-- The commit graph feature currently does not honor commit grafts. This can\n- be remedied by duplicating or refactoring the current graft logic.\n-\n - After computing and storing generation numbers, we must make graph\n walks aware of generation numbers to gain the performance benefits they\n enable. This will mostly be accomplished by swapping a commit-date-ordered\n-- \ngitgitgadget\n\n"},{"id":"352875","messageId":"88711a3cf4c045365a40504db94c34675f3bdc4e.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 4/8] test-repository: properly init repo","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:40Z","receivedAt":"2018-07-18T15:15:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/helper/test-repository.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex 2762ca656..6a84a53ef 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \tstruct commit *c;\n \tstruct commit_list *parent;\n \n-\trepo_init(&r, gitdir, worktree);\n+\tsetup_git_env(gitdir);\n+\n+\tif (repo_init(&r, gitdir, worktree))\n+\t\tdie(\"Couldn't init repo\");\n \n \tc = lookup_commit(&r, commit_oid);\n \n@@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \tstruct commit *c;\n \tstruct tree *tree;\n \n-\trepo_init(&r, gitdir, worktree);\n+\tsetup_git_env(gitdir);\n+\n+\tif (repo_init(&r, gitdir, worktree))\n+\t\tdie(\"Couldn't init repo\");\n \n \tc = lookup_commit(&r, commit_oid);\n \n-- \ngitgitgadget\n\n"},{"id":"352877","messageId":"7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:41Z","receivedAt":"2018-07-18T15:15:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nCreate new method commit_graph_compatible(r) to check if a given\nrepository r is compatible with the commit-graph feature. Fill the\nmethod with a check to see if replace-objects exist. Test this\ninteraction succeeds, including ignoring an existing commit-graph and\nfailing to write a new commit-graph.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 17 +++++++++++++++++\n replace-object.c | 2 +-\n replace-object.h | 2 ++\n t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b0a55ad12..711099858 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -13,6 +13,8 @@\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n #include \"alloc.h\"\n+#include \"hashmap.h\"\n+#include \"replace-object.h\"\n \n #define GRAPH_SIGNATURE 0x43475048 /* \"CGPH\" */\n #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* \"OIDF\" */\n@@ -56,6 +58,15 @@ static struct commit_graph *alloc_commit_graph(void)\n \treturn g;\n }\n \n+static int commit_graph_compatible(struct repository *r)\n+{\n+\tprepare_replace_object(r);\n+\tif (hashmap_get_size(&r->objects->replace_map->map))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n@@ -223,6 +234,9 @@ static int prepare_commit_graph(struct repository *r)\n \t\t */\n \t\treturn 0;\n \n+\tif (!commit_graph_compatible(r))\n+\t\treturn 0;\n+\n \tobj_dir = r->objects->objectdir;\n \tprepare_commit_graph_one(r, obj_dir);\n \tprepare_alt_odb(r);\n@@ -693,6 +707,9 @@ void write_commit_graph(const char *obj_dir,\n \tint num_extra_edges;\n \tstruct commit_list *parent;\n \n+\tif (!commit_graph_compatible(the_repository))\n+\t\treturn;\n+\n \toids.nr = 0;\n \toids.alloc = approximate_object_count() / 4;\n \ndiff --git a/replace-object.c b/replace-object.c\nindex 017f02f8e..4d2d84cf4 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,\n \treturn 0;\n }\n \n-static void prepare_replace_object(struct repository *r)\n+void prepare_replace_object(struct repository *r)\n {\n \tif (r->objects->replace_map)\n \t\treturn;\ndiff --git a/replace-object.h b/replace-object.h\nindex f996de3d6..c7a99fc35 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -10,6 +10,8 @@ struct replace_object {\n \tstruct object_id replacement;\n };\n \n+void prepare_replace_object(struct repository *r);\n+\n /*\n * This internal function is only declared here for the benefit of\n * lookup_replace_object(). Please do not call it directly.\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 4f17d7701..c90626f5d 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '\n \ttest_cmp commit-graph-after-gc $objdir/info/commit-graph\n '\n \n+test_expect_success 'replace-objects invalidates commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf replace &&\n+\tgit clone full replace &&\n+\t(\n+\t\tcd replace &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph &&\n+\t\tgit replace HEAD~1 HEAD~2 &&\n+\t\tgit -c core.commitGraph=false log >expect &&\n+\t\tgit -c core.commitGraph=true log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n+\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\trm -rf .git/objects/info/commit-graph &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_missing .git/objects/info/commit-graph\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \ngitgitgadget\n\n"},{"id":"352878","messageId":"94dd91ac35006ebee3c8808af1fce4ad69975234.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 6/8] commit-graph: not compatible with grafts","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:43Z","receivedAt":"2018-07-18T15:15:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nAugment commit_graph_compatible(r) to return false when the given\nrepository r has commit grafts or is a shallow clone. Test that in these\nsituations we ignore existing commit-graph files and we do not write new\ncommit-graph files.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 6 ++++++\n commit.c | 2 +-\n commit.h | 1 +\n t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 711099858..5097c7c12 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)\n \n static int commit_graph_compatible(struct repository *r)\n {\n+\tprepare_commit_graft(r);\n+\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n+\t\treturn 0;\n+\tif (is_repository_shallow(r))\n+\t\treturn 0;\n+\n \tprepare_replace_object(r);\n \tif (hashmap_get_size(&r->objects->replace_map->map))\n \t\treturn 0;\ndiff --git a/commit.c b/commit.c\nindex 39b80bd21..609adaf8a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n \treturn 0;\n }\n \n-static void prepare_commit_graft(struct repository *r)\n+void prepare_commit_graft(struct repository *r)\n {\n \tchar *graft_file;\n \ndiff --git a/commit.h b/commit.h\nindex da0db36eb..5459e279f 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n+void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n \n extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex c90626f5d..1d9f49af5 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '\n \t)\n '\n \n+test_expect_success 'commit grafts invalidate commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf graft &&\n+\tgit clone full graft &&\n+\t(\n+\t\tcd graft &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph &&\n+\t\tgit replace --graft HEAD~1 HEAD~3 &&\n+\t\tgit -c core.commitGraph=false log >expect &&\n+\t\tgit -c core.commitGraph=true log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n+\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\trm -rf .git/objects/info/commit-graph &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_missing .git/objects/info/commit-graph\n+\t)\n+'\n+\n+test_expect_success 'replace-objects invalidates commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf shallow &&\n+\tgit clone --depth 2 \"file://$TRASH_DIRECTORY/full\" shallow &&\n+\t(\n+\t\tcd shallow &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_missing .git/objects/info/commit-graph &&\n+\t\tgit fetch origin --unshallow &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \ngitgitgadget\n\n"},{"id":"352879","messageId":"5314a5a93dfc224edd995fa12d195087c4685208.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 7/8] commit-graph: not compatible with uninitialized repo","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:44Z","receivedAt":"2018-07-18T15:15:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5097c7c12..233958e10 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -60,6 +60,9 @@ static struct commit_graph *alloc_commit_graph(void)\n \n static int commit_graph_compatible(struct repository *r)\n {\n+\tif (!r->gitdir)\n+\t\treturn 0;\n+\n \tprepare_commit_graft(r);\n \tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n \t\treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"352880","messageId":"f4ab234ed20ac813f41cb4be418528d3e59303fd.1531926932.git.gitgitgadget@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH 8/8] commit-graph: close_commit_graph before shallow walk","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-07-18T15:15:45Z","receivedAt":"2018-07-18T15:15:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nMake close_commit_graph() work for arbitrary repositories.\n\nCall close_commit_graph() when about to start a rev-list walk that\nincludes shallow commits. This is necessary in code paths that \"fake\"\nshallow commits for the sake of fetch. Specifically, test 351 in\nt5500-fetch-pack.sh runs\n\n\tgit fetch --shallow-exclude one origin\n\nwith a file-based transfer. When the \"remote\" has a commit-graph, we do\nnot prevent the commit-graph from being loaded, but then the commits are\nintended to be dynamically transferred into shallow commits during\nget_shallow_commits_by_rev_list(). By closing the commit-graph before\nthis call, we prevent this interaction.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 8 ++++----\n commit-graph.h | 1 +\n upload-pack.c | 2 ++\n 3 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 233958e10..237d4e7d1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -256,10 +256,10 @@ static int prepare_commit_graph(struct repository *r)\n \treturn !!r->objects->commit_graph;\n }\n \n-static void close_commit_graph(void)\n+void close_commit_graph(struct repository *r)\n {\n-\tfree_commit_graph(the_repository->objects->commit_graph);\n-\tthe_repository->objects->commit_graph = NULL;\n+\tfree_commit_graph(r->objects->commit_graph);\n+\tr->objects->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,\n \twrite_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);\n \twrite_graph_chunk_large_edges(f, commits.list, commits.nr);\n \n-\tclose_commit_graph();\n+\tclose_commit_graph(the_repository);\n \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n \tcommit_lock_file(&lk);\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 76e098934..13d736cdd 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -59,6 +59,7 @@ void write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n+void close_commit_graph(struct repository *);\n void free_commit_graph(struct commit_graph *);\n \n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4ca052d0b..52ad6c8e5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -24,6 +24,7 @@\n #include \"quote.h\"\n #include \"upload-pack.h\"\n #include \"serve.h\"\n+#include \"commit-graph.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -739,6 +740,7 @@ static void deepen_by_rev_list(int ac, const char **av,\n {\n \tstruct commit_list *result;\n \n+\tclose_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(result);\n \tfree_commit_list(result);\n-- \ngitgitgadget\n"},{"id":"352881","messageId":"20180718152244.45513-1-dstolee@microsoft.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH] DO-NOT-MERGE: write and read commit-graph always","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-07-18T15:22:58Z","receivedAt":"2018-07-18T15:24:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This commit is not intended to be merged, but is only to create a test\nenvironment to see what works with the commit-graph feature and what\ndoes not. The tests that fail are primarily related to corrupting the\nobject store to remove a commit from visibility and testing that\nrev-list fails -- except it doesn't when there is a commit-graph that\nprevents parsing from the object database. The following tests will fail\nwith this commit, but are not \"real\" bugs:\n\nt0410-partial-clone.sh, Test 9\nt5307-pack-missing-commit.sh, Tests 3-4\nt6011-rev-list-with-bad-commit.sh, Test 4\n\nThe following test fails because the repo has ambiguous merge-bases, and\nthe commit-graph changes the walk order so we select a different one.\nThis alters the resulting merge from the expected result.\n\nt6024-recursive-merge.sh, Test 4\n\nThe tests above are made to pass by deleting the commit-graph file\nbefore the necessary steps.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/commit.c | 2 ++\n builtin/gc.c | 3 +--\n commit-graph.c | 11 -----------\n t/t0410-partial-clone.sh | 1 +\n t/t5307-pack-missing-commit.sh | 2 ++\n t/t6011-rev-list-with-bad-commit.sh | 1 +\n t/t6024-recursive-merge.sh | 1 +\n 7 files changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 158e3f843a..acc31252a9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -33,6 +33,7 @@\n #include \"sequencer.h\"\n #include \"mailmap.h\"\n #include \"help.h\"\n+#include \"commit-graph.h\"\n \n static const char * const builtin_commit_usage[] = {\n \tN_(\"git commit [] [--] ...\"),\n@@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t \"not exceeded, and then \\\"git reset HEAD\\\" to recover.\"));\n \n \trerere(0);\n+\twrite_commit_graph_reachable(get_object_directory(), 1);\n \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex e103f0f85d..60ab773087 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -41,7 +41,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_write_commit_graph;\n+static int gc_write_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\";\n@@ -131,7 +131,6 @@ static void gc_config(void)\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n-\tgit_config_get_bool(\"gc.writecommitgraph\", &gc_write_commit_graph);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 237d4e7d1b..ed0d27c12e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -227,22 +227,11 @@ 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 (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\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 0;\n-\n \tif (!commit_graph_compatible(r))\n \t\treturn 0;\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 4984ca583d..c235672b03 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -181,6 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\trm -rf repo/.git/objects/info/commit-graph &&\n \tgit -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n \tgrep $(git -C repo rev-parse bar) out &&\n \t! grep $FOO out\ndiff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh\nindex ae52a1882d..0bb54ae227 100755\n--- a/t/t5307-pack-missing-commit.sh\n+++ b/t/t5307-pack-missing-commit.sh\n@@ -24,10 +24,12 @@ test_expect_success 'check corruption' '\n '\n \n test_expect_success 'rev-list notices corruption (1)' '\n+\trm -rf .git/objects/info/commit-graph &&\n \ttest_must_fail git rev-list HEAD\n '\n \n test_expect_success 'rev-list notices corruption (2)' '\n+\trm -rf .git/objects/info/commit-graph &&\n \ttest_must_fail git rev-list --objects HEAD\n '\n \ndiff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\nindex e51eb41f4b..9c9cc4d540 100755\n--- a/t/t6011-rev-list-with-bad-commit.sh\n+++ b/t/t6011-rev-list-with-bad-commit.sh\n@@ -43,6 +43,7 @@ test_expect_success 'corrupt second commit object' \\\n \n test_expect_success 'rev-list should fail' \\\n '\n+ rm -rf .git/objects/info/commit-graph &&\n test_must_fail git rev-list --all > /dev/null\n '\n \ndiff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\nindex 3f59e58dfb..cec10983cd 100755\n--- a/t/t6024-recursive-merge.sh\n+++ b/t/t6024-recursive-merge.sh\n@@ -61,6 +61,7 @@ GIT_AUTHOR_DATE=\"2006-12-12 23:00:08\" git commit -m F\n '\n \n test_expect_success \"combined merge conflicts\" \"\n+\trm -rf .git/objects/info/commit-graph &&\n \ttest_must_fail git merge -m final G\n \"\n \n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"352943","messageId":"xmqqd0vkjsx3.fsf@gitster-ct.c.googlers.com","threadId":"48907","inReplyTo":"22dc9ce8364dd20876ab5eb626ecc6a7396d3b3e.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T18:32:56Z","receivedAt":"2018-07-18T18:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stefan Beller via GitGitGadget\" writes:\n\n> From: Stefan Beller \n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/replace.c | 8 ++++----\n> refs.c | 9 ++++-----\n> refs.h | 2 +-\n> replace-object.c | 5 +++--\n> 4 files changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index ef22d724b..d0b1cdb06 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -39,7 +39,8 @@ struct show_data {\n> \tenum replace_format format;\n> };\n> \n> -static int show_reference(const char *refname, const struct object_id *oid,\n> +static int show_reference(struct repository *r, const char *refname,\n> +\t\t\t const struct object_id *oid,\n> \t\t\t int flag, void *cb_data)\n> {\n> \tstruct show_data *data = cb_data;\n> @@ -56,9 +57,8 @@ static int show_reference(const char *refname, const struct object_id *oid,\n> \t\t\tif (get_oid(refname, &object))\n> \t\t\t\treturn error(\"Failed to resolve '%s' as a valid ref.\", refname);\n> \n> -\t\t\tobj_type = oid_object_info(the_repository, &object,\n> -\t\t\t\t\t\t NULL);\n> -\t\t\trepl_type = oid_object_info(the_repository, oid, NULL);\n> +\t\t\tobj_type = oid_object_info(r, &object, NULL);\n> +\t\t\trepl_type = oid_object_info(r, oid, NULL);\n> \n> \t\t\tprintf(\"%s (%s) -> %s (%s)\\n\", refname, type_name(obj_type),\n> \t\t\t oid_to_hex(oid), type_name(repl_type));\n> diff --git a/refs.c b/refs.c\n> index 2513f77ac..5700cd468 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1478,12 +1478,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n> \treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n> }\n> \n> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)\n> +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n> {\n> -\treturn do_for_each_ref(get_main_ref_store(r),\n> -\t\t\t git_replace_ref_base, fn,\n> -\t\t\t strlen(git_replace_ref_base),\n> -\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n> +\treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n> +\t\t\t\t strlen(git_replace_ref_base),\n> +\t\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n> }\n> \n> int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n> diff --git a/refs.h b/refs.h\n> index 80eec8bbc..a0a18223a 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n> int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n> int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n> int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n> +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n\nWith a signature change like this, any change that introduces new\ncall to for_each_replace_ref() using eac_ref_fn() would get\ncompilation error, so this is minimally correct.\n\nTwo things that bothersome are that\n\n - for_each_tag/branch/remote_ref() and for_each_replace_ref() now\n work and look quite differently.\n\n - existing users of for_each_replace_ref() who were all happy\n working in the_repository have to pass it explicitly, even\n thought they do not have any need to.\n\nIn this case, even if you introduced for_each_replace_ref_in_repo(),\nmaking for_each_replace_ref() a thin wrapper that always uses\nthe_repository is a bit more cumbersome than just a simple macro.\n\nBut it *is* doable (you'd need to use a wrapping structure around\ncb_data), and a developer who case about maintainability during API\ntransition would have taken pains to do so. A bit dissapointing.\n\n> int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n> int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n> \t\t\t const char *prefix, void *cb_data);\n> diff --git a/replace-object.c b/replace-object.c\n> index 801b5c167..017f02f8e 100644\n> --- a/replace-object.c\n> +++ b/replace-object.c\n> @@ -6,7 +6,8 @@\n> #include \"repository.h\"\n> #include \"commit.h\"\n> \n> -static int register_replace_ref(const char *refname,\n> +static int register_replace_ref(struct repository *r,\n> +\t\t\t\tconst char *refname,\n> \t\t\t\tconst struct object_id *oid,\n> \t\t\t\tint flag, void *cb_data)\n> {\n> @@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,\n> \toidcpy(&repl_obj->replacement, oid);\n> \n> \t/* Register new object */\n> -\tif (oidmap_put(the_repository->objects->replace_map, repl_obj))\n> +\tif (oidmap_put(r->objects->replace_map, repl_obj))\n> \t\tdie(\"duplicate replace ref: %s\", refname);\n> \n> \treturn 0;\n"},{"id":"352947","messageId":"CAGZ79kZ9oeqidNMppOx+PNfp-4RdDhLzvQHhm45Ceni=g1Q_7Q@mail.gmail.com","threadId":"48907","inReplyTo":"xmqqd0vkjsx3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T19:19:05Z","receivedAt":"2018-07-18T19:19:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > @@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n> > int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n> > int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n> > int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n> > -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n> > +int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n>\n> With a signature change like this, any change that introduces new\n> call to for_each_replace_ref() using eac_ref_fn() would get\n> compilation error, so this is minimally correct.\n>\n> Two things that bothersome are that\n>\n> - for_each_tag/branch/remote_ref() and for_each_replace_ref() now\n> work and look quite differently.\n\nYes, this series is not finished; we need to convert/upgrade for_each_tag\net al, too.\n\n> - existing users of for_each_replace_ref() who were all happy\n> working in the_repository have to pass it explicitly, even\n> thought they do not have any need to.\n\nAll callbacks that are passed to for_each_replace_ref now\noperate on 'r' instead of the_repository, and that actually fixes\na bug (commit message is lacking), but the cover letter hints at:\n\n While building this series, I got some test failures in the\n non-the_repository tests. These issues are related to missing\n references to an arbitrary repository (instead of the_repository)\n and some uninitialized values in the tests. Stefan already sent\n a patch to address this [2], and I've included those commits\n (along with a small tweak [3]). These are only included for\n convenience.\n\n> In this case, even if you introduced for_each_replace_ref_in_repo(),\n> making for_each_replace_ref() a thin wrapper that always uses\n> the_repository is a bit more cumbersome than just a simple macro.\n\nYes, but such a callback would do the *wrong* subtle thing in some cases\nas you really want to work in the correct repository for e.g. looking up\ncommits.\n\n> But it *is* doable (you'd need to use a wrapping structure around\n> cb_data), and a developer who case about maintainability during API\n> transition would have taken pains to do so. A bit dissapointing.\n\nMy original patches were RFC-ish and a trade off as for the reflog only\nthere is nothing in flight to care about.\n\nGiven that we would want to upgrade all the ref callbacks, we have to\ntake this route, I think.\n\nThanks,\nStefan\n"},{"id":"352962","messageId":"20180718194657.GC7778@sigill.intra.peff.net","threadId":"48907","inReplyTo":"7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T19:46:57Z","receivedAt":"2018-07-18T19:47:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> Create new method commit_graph_compatible(r) to check if a given\n> repository r is compatible with the commit-graph feature. Fill the\n> method with a check to see if replace-objects exist. Test this\n> interaction succeeds, including ignoring an existing commit-graph and\n> failing to write a new commit-graph.\n\nI think this approach is sensible. These are optimizations, and it's not\na big deal to just punt no cases we can't handle.\n\nI do have one complaint about the implementation, though:\n\n> +static int commit_graph_compatible(struct repository *r)\n> +{\n> +\tprepare_replace_object(r);\n> +\tif (hashmap_get_size(&r->objects->replace_map->map))\n> +\t\treturn 0;\n> +\n> +\treturn 1;\n> +}\n\nIf I'm reading the code correctly, this will predicate the decision\nentirely on the presence of refs in refs/replace/. But you may have a\nsituation where those refs exist, but you are not respecting them in\nthis run.\n\nFor instance, imagine[1] a server that hosts arbitrary repositories, but\nwants to use commit graphs to speed up server-side operations (e.g.,\nserving fetches, but also perhaps a web interface doing --contains,\netc). If it runs all of the server-side commands with\nGIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user\npushes anything to refs/replace (because they _do_ use replace refs\nlocally, and want to share them with other clients), that would disable\ngraphs on the server.\n\nSo I think this should at least be loosened to:\n\n if (check_replace_refs) {\n\tprepare_replace_object(r);\n\tif (...)\n }\n\nwhich would make this case work. I'd even go so far as to say that for\nwriting, we should just always ignore replace refs and generate the full\ngraph (just like pack-objects does so when writing a pack). Then the\nresulting graph can be used selectively by disabling replace refs for\nparticular commands. But for the scenario I described above, the\ndistinction is mostly academic, as replacements would be disabled anyway\nduring the write command anyway.\n\n-Peff\n\n[1] You can probably guess that this is how GitHub handles replace refs.\n We ran into this long ago because replacements and grafts mess up\n any other caches external to Git that rely on the immutability of\n the hash.\n\n We do it with a config option, though, which requires a trivial\n patch. I'll post that momentarily.\n"},{"id":"352963","messageId":"20180718194846.GD7778@sigill.intra.peff.net","threadId":"48907","inReplyTo":"20180718194657.GC7778@sigill.intra.peff.net","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T19:48:47Z","receivedAt":"2018-07-18T19:48:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 03:46:57PM -0400, Jeff King wrote:\n\n> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:\n> \n> > From: Derrick Stolee \n> > \n> > Create new method commit_graph_compatible(r) to check if a given\n> > repository r is compatible with the commit-graph feature. Fill the\n> > method with a check to see if replace-objects exist. Test this\n> > interaction succeeds, including ignoring an existing commit-graph and\n> > failing to write a new commit-graph.\n> \n> I think this approach is sensible. These are optimizations, and it's not\n> a big deal to just punt no cases we can't handle.\n\nEr, this should be \"punt _on_ cases we can't handle\". Sorry for my\nbelated proof-reading.\n\n-Peff\n"},{"id":"352971","messageId":"f5d2f06d-1736-57e4-edbd-aa638ae34238@gmail.com","threadId":"48907","inReplyTo":"20180718194657.GC7778@sigill.intra.peff.net","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-18T19:52:54Z","receivedAt":"2018-07-18T19:52:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/18/2018 3:46 PM, Jeff King wrote:\n> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via GitGitGadget wrote:\n>\n>> From: Derrick Stolee \n>>\n>> Create new method commit_graph_compatible(r) to check if a given\n>> repository r is compatible with the commit-graph feature. Fill the\n>> method with a check to see if replace-objects exist. Test this\n>> interaction succeeds, including ignoring an existing commit-graph and\n>> failing to write a new commit-graph.\n> I think this approach is sensible. These are optimizations, and it's not\n> a big deal to just punt no cases we can't handle.\n>\n> I do have one complaint about the implementation, though:\n>\n>> +static int commit_graph_compatible(struct repository *r)\n>> +{\n>> +\tprepare_replace_object(r);\n>> +\tif (hashmap_get_size(&r->objects->replace_map->map))\n>> +\t\treturn 0;\n>> +\n>> +\treturn 1;\n>> +}\n> If I'm reading the code correctly, this will predicate the decision\n> entirely on the presence of refs in refs/replace/. But you may have a\n> situation where those refs exist, but you are not respecting them in\n> this run.\n>\n> For instance, imagine[1] a server that hosts arbitrary repositories, but\n> wants to use commit graphs to speed up server-side operations (e.g.,\n> serving fetches, but also perhaps a web interface doing --contains,\n> etc). If it runs all of the server-side commands with\n> GIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user\n> pushes anything to refs/replace (because they _do_ use replace refs\n> locally, and want to share them with other clients), that would disable\n> graphs on the server.\n>\n> So I think this should at least be loosened to:\n>\n> if (check_replace_refs) {\n> \tprepare_replace_object(r);\n> \tif (...)\n> }\n>\n> which would make this case work. I'd even go so far as to say that for\n> writing, we should just always ignore replace refs and generate the full\n> graph (just like pack-objects does so when writing a pack). Then the\n> resulting graph can be used selectively by disabling replace refs for\n> particular commands. But for the scenario I described above, the\n> distinction is mostly academic, as replacements would be disabled anyway\n> during the write command anyway.\n>\n> -Peff\n>\n> [1] You can probably guess that this is how GitHub handles replace refs.\n> We ran into this long ago because replacements and grafts mess up\n> any other caches external to Git that rely on the immutability of\n> the hash.\n>\n> We do it with a config option, though, which requires a trivial\n> patch. I'll post that momentarily.\n\nThanks for the details! I never considered that someone would have these \nrefs around, but would ignore them most of the time.\n\nThe biggest reason I wanted to punt here was that it is easy to toggle \nbetween using replace refs and not using them. Writing and reading as \nlong as we are ignoring those refs is a good idea, and I'll use that \napproach in v2.\n\nThanks,\n\n-Stolee\n\n"},{"id":"352974","messageId":"xmqq4lgwjoa4.fsf@gitster-ct.c.googlers.com","threadId":"48907","inReplyTo":"CAGZ79kZ9oeqidNMppOx+PNfp-4RdDhLzvQHhm45Ceni=g1Q_7Q@mail.gmail.com","subject":"Re: [PATCH 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T20:13:07Z","receivedAt":"2018-07-18T20:13:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> - existing users of for_each_replace_ref() who were all happy\n>> working in the_repository have to pass it explicitly, even\n>> thought they do not have any need to.\n>\n> All callbacks that are passed to for_each_replace_ref now\n> operate on 'r' instead of the_repository, and that actually fixes\n> a bug (commit message is lacking), but the cover letter hints at:\n> ...\n>> In this case, even if you introduced for_each_replace_ref_in_repo(),\n>> making for_each_replace_ref() a thin wrapper that always uses\n>> the_repository is a bit more cumbersome than just a simple macro.\n>\n> Yes, but such a callback would do the *wrong* subtle thing in some cases\n> as you really want to work in the correct repository for e.g. looking up\n> commits.\n>\n>> But it *is* doable (you'd need to use a wrapping structure around\n>> cb_data), and a developer who case about maintainability during API\n>> transition would have taken pains to do so. A bit dissapointing.\n>\n> My original patches were RFC-ish and a trade off as for the reflog only\n> there is nothing in flight to care about.\n>\n> Given that we would want to upgrade all the ref callbacks, we have to\n> take this route, I think.\n\nTry to rebuild 'pu' on top of 'master' yoruself to realize how many\nin-flight topics you are hurting and causing unnecessary load on the\nmaintainer with the \"let's add the 'r' parameter without changing\nthe function names; compiler would catch and cause breakages\"\napproach. \n\nUntil that happens, I won't waste any more time trying to educate\nyou on this further.\n\n\n"},{"id":"353149","messageId":"a3640919-95cf-cca4-d552-4715a031dd7f@gmail.com","threadId":"48907","inReplyTo":"f5d2f06d-1736-57e4-edbd-aa638ae34238@gmail.com","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-20T16:45:11Z","receivedAt":"2018-07-20T16:45:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/18/2018 3:52 PM, Derrick Stolee wrote:\n> On 7/18/2018 3:46 PM, Jeff King wrote:\n>> On Wed, Jul 18, 2018 at 08:15:41AM -0700, Derrick Stolee via \n>> GitGitGadget wrote:\n>>\n>>> From: Derrick Stolee \n>>>\n>>> Create new method commit_graph_compatible(r) to check if a given\n>>> repository r is compatible with the commit-graph feature. Fill the\n>>> method with a check to see if replace-objects exist. Test this\n>>> interaction succeeds, including ignoring an existing commit-graph and\n>>> failing to write a new commit-graph.\n>> I think this approach is sensible. These are optimizations, and it's not\n>> a big deal to just punt no cases we can't handle.\n>>\n>> I do have one complaint about the implementation, though:\n>>\n>>> +static int commit_graph_compatible(struct repository *r)\n>>> +{\n>>> +    prepare_replace_object(r);\n>>> +    if (hashmap_get_size(&r->objects->replace_map->map))\n>>> +        return 0;\n>>> +\n>>> +    return 1;\n>>> +}\n>> If I'm reading the code correctly, this will predicate the decision\n>> entirely on the presence of refs in refs/replace/. But you may have a\n>> situation where those refs exist, but you are not respecting them in\n>> this run.\n>>\n>> For instance, imagine[1] a server that hosts arbitrary repositories, but\n>> wants to use commit graphs to speed up server-side operations (e.g.,\n>> serving fetches, but also perhaps a web interface doing --contains,\n>> etc). If it runs all of the server-side commands with\n>> GIT_NO_REPLACE_OBJECTS=1, then there should be no problem. But if a user\n>> pushes anything to refs/replace (because they _do_ use replace refs\n>> locally, and want to share them with other clients), that would disable\n>> graphs on the server.\n>>\n>> So I think this should at least be loosened to:\n>>\n>>    if (check_replace_refs) {\n>>     prepare_replace_object(r);\n>>     if (...)\n>>    }\n>>\n>> which would make this case work. I'd even go so far as to say that for\n>> writing, we should just always ignore replace refs and generate the full\n>> graph (just like pack-objects does so when writing a pack). Then the\n>> resulting graph can be used selectively by disabling replace refs for\n>> particular commands. But for the scenario I described above, the\n>> distinction is mostly academic, as replacements would be disabled anyway\n>> during the write command anyway.\n>>\n>> -Peff\n>>\n>> [1] You can probably guess that this is how GitHub handles replace refs.\n>>      We ran into this long ago because replacements and grafts mess up\n>>      any other caches external to Git that rely on the immutability of\n>>      the hash.\n>>\n>>      We do it with a config option, though, which requires a trivial\n>>      patch. I'll post that momentarily.\n>\n> Thanks for the details! I never considered that someone would have \n> these refs around, but would ignore them most of the time.\n>\n> The biggest reason I wanted to punt here was that it is easy to toggle \n> between using replace refs and not using them. Writing and reading as \n> long as we are ignoring those refs is a good idea, and I'll use that \n> approach in v2.\n\n\nThanks, Peff, for improving the check_replace_refs interaction [1].\n\nSince this series now has two dependencies (including Stefan's ref-store \nfix [2] that I had included in my v1), I'll let those topics settle \nbefore I send a new v2.\n\nIf there are more comments about how I'm handling these cases, then \nplease jump in and tell me. I'll revisit this topic in a few weeks.\n\nThanks,\n\n-Stolee\n\n[1] \nhttps://public-inbox.org/git/20180718204449.GA31816@sigill.intra.peff.net/T/#u\n\n      [PATCH 1/3] check_replace_refs: fix outdated comment\n\n[2] \nhttps://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#u\n\n     [PATCH 0/2] RFC ref store to repository migration\n\n"},{"id":"353150","messageId":"CAGZ79kZatUUO6s6h7uH6K3Ta5a9k-t6xuWeQ0uiAUF0sjp1avg@mail.gmail.com","threadId":"48907","inReplyTo":"a3640919-95cf-cca4-d552-4715a031dd7f@gmail.com","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-20T16:49:04Z","receivedAt":"2018-07-20T16:49:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Thanks, Peff, for improving the check_replace_refs interaction [1].\n>\n> Since this series now has two dependencies (including Stefan's ref-store\n> fix [2] that I had included in my v1), I'll let those topics settle\n> before I send a new v2.\n\nFYI: I have not been working on, nor plan to work on (in the short term)\non the ref store fix series, which needs another abstraction layer IIUC to\nmake it easier to integrate such a thing as well as extend the series to all\nfunctions in the refstore.\n\nFeel free to take over that series partially (and defer the extension for\nall functions in the ref store until later) ?\n\nStefan\n"},{"id":"353151","messageId":"16007477-d22a-4a3b-87db-07f11abdfb7f@gmail.com","threadId":"48907","inReplyTo":"CAGZ79kZatUUO6s6h7uH6K3Ta5a9k-t6xuWeQ0uiAUF0sjp1avg@mail.gmail.com","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-20T16:57:45Z","receivedAt":"2018-07-20T16:57:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/20/2018 12:49 PM, Stefan Beller wrote:\n>> Thanks, Peff, for improving the check_replace_refs interaction [1].\n>>\n>> Since this series now has two dependencies (including Stefan's ref-store\n>> fix [2] that I had included in my v1), I'll let those topics settle\n>> before I send a new v2.\n> FYI: I have not been working on, nor plan to work on (in the short term)\n> on the ref store fix series, which needs another abstraction layer IIUC to\n> make it easier to integrate such a thing as well as extend the series to all\n> functions in the refstore.\n>\n> Feel free to take over that series partially (and defer the extension for\n> all functions in the ref store until later) ?\n\nOK. Thanks for letting me know.\n\nPerhaps I'll give it a try to pass the repository struct in the cbdata \nfor the function pointers, as a minimum amount of change option.\n\nThanks,\n\n-Stolee\n\n"},{"id":"353822","messageId":"86h8ki89sp.fsf@gmail.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T13:13:10Z","receivedAt":"2018-07-29T13:14:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\nSidenote: the GitGitGadget doesn't fold/wrap lines properly in the\ncover-letter. Not that it is much of a problem.\n\n> One unresolved issue with the commit-graph feature is that it can\n> cause issues when combined with replace objects, commit grafts, or\n> shallow clones. These are not 100% incompatible, as one could be\n> reasonably successful writing a commit-graph after replacing some\n> objects and not have issues. The problems happen when commits that are\n> already in the commit-graph file are replaced, or when git is run with\n> the `--no-replace-objects` option; this can cause incorrect parents or\n> incorrect generation numbers. Similar things occur with commit grafts\n> and shallow clones, especially when running `git fetch --unshallow` in\n> a shallow repo.\n\nThis means that these features can change the view of the history, and\nif the commit-graph was created before the change, it can have stale\nincorrect information -- which would lead to incorrect results if the\ncommit-graph feature is used.\n\n>\n> Instead of trying (and probably failing) to make these features work\n> together, default to making the commit-graph feature unavailable in\n> these situations. Create a new method 'commit_graph_compatible(r)'\n> that checks if the repository 'r' has any of these features enabled.\n\nYes, the alternative would be trying to keep the commit-graph file\nup-to-date with the current view of history. But there are problems\nwith this approach.\n\n\nWith grafts (which is a deprecated feature), there is no automated entry\npoint. Best what one could do is to store some kind of fingerprint of\ngraft file, and invalidate / recreate commit-graft file if it does not\nmatch what is in graft file at the time of running git command that may\nuse commit-graft feature.\n\nWith replace objects we have two separate entry points: the git-replace\ncommand and fetching references in refs/replace/* namespace (and\nequivalent). While the first should be not difficult to do, the second\none seems to be harder. Additionally, there is '--no-replace-objects'\noption to turn off the feature (fetch and push ignore replacement\nobjects automatically); so we might want to have two versions of\ncommit-graph: one with replacement objects feature and one without (or\nsomething like that).\n\nThe shallow clone seems easiest, with only one automated entry points\nfor changing the view of history this way, and no option to disable this\nfeature -- but it is also the least interesting, as the intent of\nshallow clone is to have less history, so the commit-graph is less\nnecessary, as you wrote.\n\n\nWe might want to revisit it later, but I agree that starting from this\nsimple approach would be best.\n\nOne thing I would like to see added is for the user to know when\ncommit-graph is not available (in manpages), and maybe even a way to see\nif it is on (e.g. with 'git commit-graph verify' and/or maybe in\n'git status' output). But this is a separate issue.\n\n>\n> I will send a follow-up patch that shows how I tested these\n> interactions by computing the commit-graph on every 'git commit'.\n\nWouldn't it be enough to create commit-graph file, change the view of\nthe history (via grafts, via replace objects, via shallow clone), and\ncheck that you still get correct results?\n\n[cut]\n>\n> This approach is very different from the previous RFC on the subject [1].\n\nThe previous RFC was in my opinion needlessly complicated. This one is\nmuch simpler, and better.\n\n[...]\n> Thanks,\n> -Stolee\n>\n> [1] https://public-inbox.org/git/20180531174024.124488-1-dstolee@microsoft.com/\n> [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo\n>\n> [2] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#t\n> [PATCH 0/2] RFC ref store to repository migration\n>\n> [3] https://public-inbox.org/git/20180717224935.96397-1-sbeller@google.com/T/#m966eac85fd58c66523654ddaf0bec72877d3295a\n> [PATCH] TO-SQUASH: replace the_repository with arbitrary r\n>\n> Based-On: jt/commit-graph-per-object-store\n> Cc: jonathantanmy@google.com\n> Cc: sbeller@google.com\n>\n> Derrick Stolee (6):\n> commit-graph: update design document\n> test-repository: properly init repo\n> commit-graph: not compatible with replace objects\n> commit-graph: not compatible with grafts\n> commit-graph: not compatible with uninitialized repo\n> commit-graph: close_commit_graph before shallow walk\n>\n> Stefan Beller (2):\n> refs.c: migrate internal ref iteration to pass thru repository\n> argument\n> refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback\n>\n> Documentation/technical/commit-graph.txt | 18 ++++++--\n> builtin/replace.c | 8 ++--\n> commit-graph.c | 34 ++++++++++++--\n> commit-graph.h | 1 +\n> commit.c | 2 +-\n> commit.h | 1 +\n> refs.c | 48 +++++++++++++++++---\n> refs.h | 12 ++++-\n> refs/iterator.c | 6 +--\n> refs/refs-internal.h | 5 +-\n> replace-object.c | 7 +--\n> replace-object.h | 2 +\n> t/helper/test-repository.c | 10 +++-\n> t/t5318-commit-graph.sh | 58 ++++++++++++++++++++++++\n> upload-pack.c | 2 +\n> 15 files changed, 184 insertions(+), 30 deletions(-)\n>\n>\n> base-commit: dade47c06cf849b0ca180a8e6383b55ea6f75812\n> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-11%2Fderrickstolee%2Fshallow%2Fupstream-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-11/derrickstolee/shallow/upstream-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/11\n"},{"id":"353823","messageId":"86bmaq88br.fsf@gmail.com","threadId":"48907","inReplyTo":"5e90d36f8482e03df842d483bfe6f237f0247166.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/8] commit-graph: update design document","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T13:44:56Z","receivedAt":"2018-07-29T13:45:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> As it exists right now, the commit-graph feature may provide\n> inconsistent results when combined with commit grafts, replace objects,\n> and shallow clones. Update the design document to discuss why these\n> interactions are difficult to reconcile and how we will avoid errors by\n> preventing updates to and reads from the commit-graph file when these\n> other features exist.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> Documentation/technical/commit-graph.txt | 18 +++++++++++++++---\n> 1 file changed, 15 insertions(+), 3 deletions(-)\n\nIt would be nice to have this information not only in the technical\ndocumentation, but also in user-facing documentation, for example\ngit-commit-graph manpage.\n\n>\n> diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt\n> index c664acbd7..18948f11a 100644\n> --- a/Documentation/technical/commit-graph.txt\n> +++ b/Documentation/technical/commit-graph.txt\n> @@ -112,12 +112,24 @@ Design Details\n> - The file format includes parameters for the object ID hash function,\n> so a future change of hash algorithm does not require a change in format.\n> \n> +- Commit grafts and replace objects can change the shape of the commit\n> + history. These can also be enabled/disabled on the fly using\n\nI think you meant \"The latter can...\" instead of \"These can...\", as\ngrafts cannot be disabled with `--no-replace-objects` option.\n\n> + `--no-replace-objects`. This leads to difficultly storing both possible\n> + interpretations of a commit id, especially when computing generation\n> + numbers. The commit-graph will not be read or written when\n> + replace-objects or grafts are present.\n> +\n> +- Shallow clones create grafts of commits by dropping their parents. This\n> + leads the commit-graph to think those commits have generation number 1.\n> + If and when those commits are made unshallow, those generation numbers\n> + become invalid. Since shallow clones are intended to restrict the commit\n> + history to a very small set of commits, the commit-graph feature is less\n> + helpful for these clones, anyway. The commit-graph will not be read or\n> + written when shallow commits are present.\n> +\n> Future Work\n> -----------\n> \n> -- The commit graph feature currently does not honor commit grafts. This can\n> - be remedied by duplicating or refactoring the current graft logic.\n> -\n> - After computing and storing generation numbers, we must make graph\n> walks aware of generation numbers to gain the performance benefits they\n> enable. This will mostly be accomplished by swapping a commit-date-ordered\n"},{"id":"353832","messageId":"CAPig+cSrTnrXRdxRG3b2jatbOtgX6hJ_tP8QFmVbnnE57RETyw@mail.gmail.com","threadId":"48907","inReplyTo":"86h8ki89sp.fsf@gmail.com","subject":"Re: [PATCH 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-29T19:27:56Z","receivedAt":"2018-07-29T19:28:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jul 29, 2018 at 9:13 AM Jakub Narebski wrote:\n> Sidenote: the GitGitGadget doesn't fold/wrap lines properly in the\n> cover-letter. Not that it is much of a problem.\n\nStefan opened a ticket at the gitgitgadget project:\nhttps://github.com/gitgitgadget/gitgitgadget/issues/26\n"},{"id":"353841","messageId":"86wotd7o64.fsf@gmail.com","threadId":"48907","inReplyTo":"7f596c1718d35539f02828edbf933c8e660f123b.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/8] commit-graph: not compatible with replace objects","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T21:00:19Z","receivedAt":"2018-07-29T21:00:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Create new method commit_graph_compatible(r) to check if a given\n> repository r is compatible with the commit-graph feature. Fill the\n> method with a check to see if replace-objects exist.\n\nAll right, looks sensible. Did you start with the history-\"altering\"\nfeature with simplest detection mechanism?\n\n> Test this\n> interaction succeeds, including ignoring an existing commit-graph and\n> failing to write a new commit-graph.\n\nI wonder if we should test the implementation, or just that you get\ncorrect answer when history view is altered after writing commit-graph\nfile... oh, wait, you do that (well, except for testing that\n`git commit-graph write` does not create commit-graph file instead of\ntesting that the commit-graph file stores correct information even after\nthe feature is removed (with `git replace --delete`) or turned off (with\n`--no-replace-objects` or equivalent). Good.\n\nSidenote: the inconsistency between command options and\nsubcommands... ehh... compare e.g. git-replace and git-remote.\n\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 17 +++++++++++++++++\n> replace-object.c | 2 +-\n> replace-object.h | 2 ++\n> t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++\n> 4 files changed, 42 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index b0a55ad12..711099858 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -13,6 +13,8 @@\n> #include \"commit-graph.h\"\n> #include \"object-store.h\"\n> #include \"alloc.h\"\n> +#include \"hashmap.h\"\n> +#include \"replace-object.h\"\n> \n> #define GRAPH_SIGNATURE 0x43475048 /* \"CGPH\" */\n> #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* \"OIDF\" */\n> @@ -56,6 +58,15 @@ static struct commit_graph *alloc_commit_graph(void)\n> \treturn g;\n> }\n> \n> +static int commit_graph_compatible(struct repository *r)\n> +{\n> +\tprepare_replace_object(r);\n> +\tif (hashmap_get_size(&r->objects->replace_map->map))\n\nA very minor nitpick, feel free to ignore. I think that the condition\nwould be more readable written as\n\n +\tif (hashmap_get_size(&r->objects->replace_map->map) > 0)\n\nAnother issue is that the condition may be too strict (but it is also\nthe easiest to test for). You might have replacement objects for\nnon-commit objects (e.g. replacing contents of configuration files with\none with API keys and/or other secrets - not that I know any tool that\ndoes that), or replacing commit-objects without changing their history\n(e.g. to fix some annoying error in demo repository, where you want to\nshow reflog). But I think that would be rare, so easier if maybe\nover-eager test is fine for me.\n\n> +\t\treturn 0;\n> +\n> +\treturn 1;\n> +}\n> +\n> struct commit_graph *load_commit_graph_one(const char *graph_file)\n> {\n> \tvoid *graph_map;\n> @@ -223,6 +234,9 @@ static int prepare_commit_graph(struct repository *r)\n> \t\t */\n> \t\treturn 0;\n> \n> +\tif (!commit_graph_compatible(r))\n> +\t\treturn 0;\n> +\n> \tobj_dir = r->objects->objectdir;\n> \tprepare_commit_graph_one(r, obj_dir);\n> \tprepare_alt_odb(r);\n> @@ -693,6 +707,9 @@ void write_commit_graph(const char *obj_dir,\n> \tint num_extra_edges;\n> \tstruct commit_list *parent;\n> \n> +\tif (!commit_graph_compatible(the_repository))\n> +\t\treturn;\n> +\n\nHmmm... we might want to write commit-graph for immutable history; but\nnot all history-\"altering\" feature can be easily turned off. So if it\nis to stay universal, without aadditional complications, it is good\nenough.\n\n> \toids.nr = 0;\n> \toids.alloc = approximate_object_count() / 4;\n> \n> diff --git a/replace-object.c b/replace-object.c\n> index 017f02f8e..4d2d84cf4 100644\n> --- a/replace-object.c\n> +++ b/replace-object.c\n> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,\n> \treturn 0;\n> }\n> \n> -static void prepare_replace_object(struct repository *r)\n> +void prepare_replace_object(struct repository *r)\n> {\n> \tif (r->objects->replace_map)\n> \t\treturn;\n> diff --git a/replace-object.h b/replace-object.h\n> index f996de3d6..c7a99fc35 100644\n> --- a/replace-object.h\n> +++ b/replace-object.h\n> @@ -10,6 +10,8 @@ struct replace_object {\n> \tstruct object_id replacement;\n> };\n> \n> +void prepare_replace_object(struct repository *r);\n> +\n\nHmmm... how it is that this function didn't need to be exported before,\nI do wonder?\n\n> /*\n> * This internal function is only declared here for the benefit of\n> * lookup_replace_object(). Please do not call it directly.\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index 4f17d7701..c90626f5d 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '\n> \ttest_cmp commit-graph-after-gc $objdir/info/commit-graph\n> '\n> \n> +test_expect_success 'replace-objects invalidates commit-graph' '\n> +\tcd \"$TRASH_DIRECTORY\" &&\n> +\ttest_when_finished rm -rf replace &&\n> +\tgit clone full replace &&\n> +\t(\n> +\t\tcd replace &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_file .git/objects/info/commit-graph &&\n\nAll right, this is preparation / setup; create commit-graph file, and\ncheck that it was created.\n\n> +\t\tgit replace HEAD~1 HEAD~2 &&\n\nAll right, so before replacement the history looks like this:\n\n ... <--- A <--- B <--- C <--- D <--- master <=== HEAD\n\n ~3 ~2 ~1 HEAD\n\nWhile after the replacement the history view looks like this, if I\nunderstand it correctly:\n\n ... <--- A <--- B\n \\\n \\---[B] <--- D <--- master <=== HEAD\n\n\nWouldn't it be better (and more similar to the grafts test) to use\n`git replace --graft`, e.g.:\n\n +\t\tgit replace --graft HEAD~1 HEAD~3 &&\n\nBut that might be just a matter of taste.\n\n> +\t\tgit -c core.commitGraph=false log >expect &&\n> +\t\tgit -c core.commitGraph=true log >actual &&\n> +\t\ttest_cmp expect actual &&\n\nAll right, so instead of testing if the output is what is expected, we\ntest that the commit-graph feature did not change it. Good.\n\n> +\t\tgit commit-graph write --reachable &&\n\nIs this necessary [see below]?\n\n> +\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n> +\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n\nAll right, and here we test the same for `--no-replace-objects` case.\nGood.\n\nBy the way, is this `git commit-graph write --reachable` preceding this\npart of test necessary?\n\n> +\t\ttest_cmp expect actual &&\n> +\t\trm -rf .git/objects/info/commit-graph &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_missing .git/objects/info/commit-graph\n\nAll right, so here we test that we wont write commit-graph file with\naltered view of history; which would store wrong information if the\nreplace feature is turned off or if replace object is deleted (or if\ncommit graph gets transferred in the furure, but the replacement refs do\nnot).\n\nChecking that the commit-graph file ws not created might be seen as\ntesting implementation details, but full test would require testing with\n`--no-replace-objects` and after `git replace -d`; the above is simpler,\nso good enough.\n\n> +\t)\n> +'\n> +\n> # the verify tests below expect the commit-graph to contain\n> # exactly the commits reachable from the commits/8 branch.\n> # If the file changes the set of commits in the list, then the\n"},{"id":"353842","messageId":"86pnz57ntm.fsf@gmail.com","threadId":"48907","inReplyTo":"88711a3cf4c045365a40504db94c34675f3bdc4e.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/8] test-repository: properly init repo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T21:07:49Z","receivedAt":"2018-07-29T21:07:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/helper/test-repository.c | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\n> index 2762ca656..6a84a53ef 100644\n> --- a/t/helper/test-repository.c\n> +++ b/t/helper/test-repository.c\n> @@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n> \tstruct commit *c;\n> \tstruct commit_list *parent;\n> \n> -\trepo_init(&r, gitdir, worktree);\n> +\tsetup_git_env(gitdir);\n> +\n> +\tif (repo_init(&r, gitdir, worktree))\n> +\t\tdie(\"Couldn't init repo\");\n\nShouldn't this message be marked for translation?\n\n +\t\tdie(_(\"Couldn't init repo\"));\n\nPerhaps we could provide also more detail about error, if possible.\n\n> \n> \tc = lookup_commit(&r, commit_oid);\n> \n> @@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n> \tstruct commit *c;\n> \tstruct tree *tree;\n> \n> -\trepo_init(&r, gitdir, worktree);\n> +\tsetup_git_env(gitdir);\n> +\n> +\tif (repo_init(&r, gitdir, worktree))\n> +\t\tdie(\"Couldn't init repo\");\n\nI wonder if it would be worth it to extract this idiom into separate\nfunction, called for exaple repo_init_or_die(), to avoid code\nduplication (and maybe use it in other places?).\n\n> \n> \tc = lookup_commit(&r, commit_oid);\n"},{"id":"353849","messageId":"86bmap7l7a.fsf@gmail.com","threadId":"48907","inReplyTo":"94dd91ac35006ebee3c8808af1fce4ad69975234.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/8] commit-graph: not compatible with grafts","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T22:04:25Z","receivedAt":"2018-07-29T22:04:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Augment commit_graph_compatible(r) to return false when the given\n> repository r has commit grafts or is a shallow clone. Test that in these\n> situations we ignore existing commit-graph files and we do not write new\n> commit-graph files.\n\nTests for grafts are wrong, as they test replace objects.\n\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 6 ++++++\n> commit.c | 2 +-\n> commit.h | 1 +\n> t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++++++++\n> 4 files changed, 44 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 711099858..5097c7c12 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -60,6 +60,12 @@ static struct commit_graph *alloc_commit_graph(void)\n> \n> static int commit_graph_compatible(struct repository *r)\n> {\n> +\tprepare_commit_graft(r);\n> +\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n> +\t\treturn 0;\n> +\tif (is_repository_shallow(r))\n> +\t\treturn 0;\n\nDo I assume correctly that is_repository_shallow(r) needs\nprepare_commit_graph(r) done earlier? If it is so, all right.\n\nIf it is not so, then why put two separate history-\"altering\" features,\nnamely grafts and shallow clones in single commit instead of in separate\ncommits?\n\n> +\n> \tprepare_replace_object(r);\n> \tif (hashmap_get_size(&r->objects->replace_map->map))\n> \t\treturn 0;\n> diff --git a/commit.c b/commit.c\n> index 39b80bd21..609adaf8a 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n> \treturn 0;\n> }\n> \n> -static void prepare_commit_graft(struct repository *r)\n> +void prepare_commit_graft(struct repository *r)\n> {\n> \tchar *graft_file;\n> \n> diff --git a/commit.h b/commit.h\n> index da0db36eb..5459e279f 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n> \n> struct commit_graft *read_graft_line(struct strbuf *line);\n> int register_commit_graft(struct repository *r, struct commit_graft *, int);\n> +void prepare_commit_graft(struct repository *r);\n> struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n>\n\nI guess that prepare_commit_graft() was not needed outside commit.c\nbefore.\n\n> extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index c90626f5d..1d9f49af5 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -281,6 +281,42 @@ test_expect_success 'replace-objects invalidates commit-graph' '\n> \t)\n> '\n> \n> +test_expect_success 'commit grafts invalidate commit-graph' '\n> +\tcd \"$TRASH_DIRECTORY\" &&\n> +\ttest_when_finished rm -rf graft &&\n> +\tgit clone full graft &&\n> +\t(\n> +\t\tcd graft &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_file .git/objects/info/commit-graph &&\n> +\t\tgit replace --graft HEAD~1 HEAD~3 &&\n\nErrr... this uses *replace objects* mechanism, not graft file!! The\n`--graft` option of git-remote command is here to make it easier to use\nreplace objects to alter the view of history, and make if easy to\nconvert from deprected grafts feature to replace objects.\n\nGrafts file structure is described in gitrepository-layout(5) manpage.\nYou would need to do something like the following:\n\n +\t\tH1=$(git rev-parse --verify HEAD~1) &&\n +\t\tH3=$(git rev-parse --verify HEAD~3) &&\n +\t\techo '$H1 $H3' >.git/info/grafts &&\n\n\nYou could have used \"git replace --graft HEAD~1 HEAD~3\" in previous\ncommit test, like I wrote in reply to it.\n\n> +\t\tgit -c core.commitGraph=false log >expect &&\n> +\t\tgit -c core.commitGraph=true log >actual &&\n> +\t\ttest_cmp expect actual &&\n\nAll right, we test that we get the same result (when graft-file is\nwritten before altering the view of history) with and without the\ncommit-graph feature.\n\nSidenote: shouldn't we use rev-list instead? Though here git-log is\nactually safe to use...\n\n> +\t\tgit commit-graph write --reachable &&\n> +\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n> +\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n\nThe `--no-replace-objects` does not affect the graph file!! You would\nwant to remove graft file instead:\n\n +\t\tgit commit-graph write --reachable &&\n +\t\trm -f .git/info/grafts &&\n +\t\tgit -c core.commitGraph=false log >expect &&\n +\t\tgit -c core.commitGraph=true log >actual &&\n\nAnd then check the results.\n\n> +\t\ttest_cmp expect actual &&\n> +\t\trm -rf .git/objects/info/commit-graph &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_missing .git/objects/info/commit-graph\n\nAll right.\n\nThough, if it is the same for grafts and for replacements... nah.\nAdding a feature and deleting a feature is different, and it is only a\nsingle repetition.\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'replace-objects invalidates commit-graph' '\n> +\tcd \"$TRASH_DIRECTORY\" &&\n> +\ttest_when_finished rm -rf shallow &&\n> +\tgit clone --depth 2 \"file://$TRASH_DIRECTORY/full\" shallow &&\n\nWhy do we need to use \"file://...\" URL, instead of simply \"full\"?\n\n> +\t(\n> +\t\tcd shallow &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_missing .git/objects/info/commit-graph &&\n> +\t\tgit fetch origin --unshallow &&\n> +\t\tgit commit-graph write --reachable &&\n> +\t\ttest_path_is_file .git/objects/info/commit-graph\n\nAll right, so in this case because you need to start (clone) with\nshallow feature, the test could be simplified.\n\n> +\t)\n> +'\n> +\n> # the verify tests below expect the commit-graph to contain\n> # exactly the commits reachable from the commits/8 branch.\n> # If the file changes the set of commits in the list, then the\n"},{"id":"353851","messageId":"86600x7j9e.fsf@gmail.com","threadId":"48907","inReplyTo":"5314a5a93dfc224edd995fa12d195087c4685208.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 7/8] commit-graph: not compatible with uninitialized repo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-29T22:46:21Z","receivedAt":"2018-07-29T22:46:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n[I hope that the rest of replies would make it all right through\nGitGitGadget]\n\n> From: Derrick Stolee \n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 5097c7c12..233958e10 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -60,6 +60,9 @@ static struct commit_graph *alloc_commit_graph(void)\n> \n> static int commit_graph_compatible(struct repository *r)\n> {\n> +\tif (!r->gitdir)\n> +\t\treturn 0;\n> +\n\nNice catch.\n\nI wonder if this is the problem because of the following tests (in which\ncase this test should probably be earlier in this series), or not (and\nit doesn't need to).\n\nI wonder how this issue was caught.\n\n> \tprepare_commit_graft(r);\n> \tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n> \t\treturn 0;\n\nNo tests?\n"},{"id":"353876","messageId":"86y3ds6b40.fsf@gmail.com","threadId":"48907","inReplyTo":"20180718152244.45513-1-dstolee@microsoft.com","subject":"Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-30T14:39:59Z","receivedAt":"2018-07-30T14:40:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Derrick Stolee writes:\n\n> This commit is not intended to be merged, but is only to create a test\n> environment to see what works with the commit-graph feature and what\n> does not. The tests that fail are primarily related to corrupting the\n> object store to remove a commit from visibility and testing that\n> rev-list fails -- except it doesn't when there is a commit-graph that\n> prevents parsing from the object database. The following tests will fail\n> with this commit, but are not \"real\" bugs:\n>\n> t0410-partial-clone.sh, Test 9\n> t5307-pack-missing-commit.sh, Tests 3-4\n> t6011-rev-list-with-bad-commit.sh, Test 4\n>\n> The following test fails because the repo has ambiguous merge-bases, and\n> the commit-graph changes the walk order so we select a different one.\n> This alters the resulting merge from the expected result.\n>\n> t6024-recursive-merge.sh, Test 4\n>\n> The tests above are made to pass by deleting the commit-graph file\n> before the necessary steps.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/commit.c | 2 ++\n> builtin/gc.c | 3 +--\n> commit-graph.c | 11 -----------\n> t/t0410-partial-clone.sh | 1 +\n> t/t5307-pack-missing-commit.sh | 2 ++\n> t/t6011-rev-list-with-bad-commit.sh | 1 +\n> t/t6024-recursive-merge.sh | 1 +\n> 7 files changed, 8 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 158e3f843a..acc31252a9 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -33,6 +33,7 @@\n> #include \"sequencer.h\"\n> #include \"mailmap.h\"\n> #include \"help.h\"\n> +#include \"commit-graph.h\"\n> \n> static const char * const builtin_commit_usage[] = {\n> \tN_(\"git commit [] [--] ...\"),\n> @@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> \t\t \"not exceeded, and then \\\"git reset HEAD\\\" to recover.\"));\n> \n> \trerere(0);\n> +\twrite_commit_graph_reachable(get_object_directory(), 1);\n> \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n> \trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n> \tif (amend && !no_post_rewrite) {\n\nThis is certainly not for merging.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index e103f0f85d..60ab773087 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -41,7 +41,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_write_commit_graph;\n> +static int gc_write_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\";\n\nThis is switching from default to off to default to on. I think with\nthis feature we would default to off for a long time (wishful thinking:\nmaybe automatically enabling it for large repositories, or if\ncommit-graph file exists already?).\n\n> @@ -131,7 +131,6 @@ static void gc_config(void)\n> \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n> \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n> \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n> -\tgit_config_get_bool(\"gc.writecommitgraph\", &gc_write_commit_graph);\n> \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n> \tgit_config_get_expiry(\"gc.pruneexpire\", &prune_expire);\n> \tgit_config_get_expiry(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n\nThis is certainly not for merging, as it disables gc.writeCommitGraph\nconfig option entirely.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 237d4e7d1b..ed0d27c12e 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -227,22 +227,11 @@ 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 (r->objects->commit_graph_attempted)\n> \t\treturn !!r->objects->commit_graph;\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 0;\n> -\n> \tif (!commit_graph_compatible(r))\n> \t\treturn 0;\n>\n\nThis is certainly not for merging, as it disables core.commitGraph\nconfig option entirely.\n\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 4984ca583d..c235672b03 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -181,6 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '\n> \n> \tgit -C repo config core.repositoryformatversion 1 &&\n> \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n> +\trm -rf repo/.git/objects/info/commit-graph &&\n> \tgit -C repo rev-list --exclude-promisor-objects --objects bar >out &&\n> \tgrep $(git -C repo rev-parse bar) out &&\n> \t! grep $FOO out\n> diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh\n> index ae52a1882d..0bb54ae227 100755\n> --- a/t/t5307-pack-missing-commit.sh\n> +++ b/t/t5307-pack-missing-commit.sh\n> @@ -24,10 +24,12 @@ test_expect_success 'check corruption' '\n> '\n> \n> test_expect_success 'rev-list notices corruption (1)' '\n> +\trm -rf .git/objects/info/commit-graph &&\n> \ttest_must_fail git rev-list HEAD\n> '\n> \n> test_expect_success 'rev-list notices corruption (2)' '\n> +\trm -rf .git/objects/info/commit-graph &&\n> \ttest_must_fail git rev-list --objects HEAD\n> '\n> \n> diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\n> index e51eb41f4b..9c9cc4d540 100755\n> --- a/t/t6011-rev-list-with-bad-commit.sh\n> +++ b/t/t6011-rev-list-with-bad-commit.sh\n> @@ -43,6 +43,7 @@ test_expect_success 'corrupt second commit object' \\\n> \n> test_expect_success 'rev-list should fail' \\\n> '\n> + rm -rf .git/objects/info/commit-graph &&\n> test_must_fail git rev-list --all > /dev/null\n> '\n> \n> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\n> index 3f59e58dfb..cec10983cd 100755\n> --- a/t/t6024-recursive-merge.sh\n> +++ b/t/t6024-recursive-merge.sh\n> @@ -61,6 +61,7 @@ GIT_AUTHOR_DATE=\"2006-12-12 23:00:08\" git commit -m F\n> '\n> \n> test_expect_success \"combined merge conflicts\" \"\n> +\trm -rf .git/objects/info/commit-graph &&\n> \ttest_must_fail git merge -m final G\n> \"\n\nI wonder though if all those changes to the testsuite shouldn't be\nmerged.\n\nRegards,\n-- \nJakub Narębski\n"},{"id":"353913","messageId":"CAGZ79kbAocJU2oqVCr9Dzhy2Qi-iESYGU6z2LbtFF89obaBgqw@mail.gmail.com","threadId":"48907","inReplyTo":"86y3ds6b40.fsf@gmail.com","subject":"Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T17:06:25Z","receivedAt":"2018-07-30T17:06:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I wonder though if all those changes to the testsuite shouldn't be\n> merged.\n\nI think Stolee doesn't want this to be merged after rereading\nsubject and the commit message.\n\nThanks,\nStefan\n"},{"id":"353939","messageId":"86tvog5xx6.fsf@gmail.com","threadId":"48907","inReplyTo":"f4ab234ed20ac813f41cb4be418528d3e59303fd.1531926932.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 8/8] commit-graph: close_commit_graph before shallow walk","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-30T19:24:53Z","receivedAt":"2018-07-30T19:24:58Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Make close_commit_graph() work for arbitrary repositories.\n\nThe first line (above) does not match the rest of the commit message.\n\n> Call close_commit_graph() when about to start a rev-list walk that\n> includes shallow commits. This is necessary in code paths that \"fake\"\n> shallow commits for the sake of fetch. Specifically, test 351 in\n> t5500-fetch-pack.sh runs\n>\n> \tgit fetch --shallow-exclude one origin\n>\n> with a file-based transfer. When the \"remote\" has a commit-graph, we do\n> not prevent the commit-graph from being loaded, but then the commits are\n> intended to be dynamically transferred into shallow commits during\n> get_shallow_commits_by_rev_list(). By closing the commit-graph before\n> this call, we prevent this interaction.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 8 ++++----\n> commit-graph.h | 1 +\n> upload-pack.c | 2 ++\n> 3 files changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 233958e10..237d4e7d1 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -256,10 +256,10 @@ static int prepare_commit_graph(struct repository *r)\n> \treturn !!r->objects->commit_graph;\n> }\n> \n> -static void close_commit_graph(void)\n> +void close_commit_graph(struct repository *r)\n> {\n> -\tfree_commit_graph(the_repository->objects->commit_graph);\n> -\tthe_repository->objects->commit_graph = NULL;\n> +\tfree_commit_graph(r->objects->commit_graph);\n> +\tr->objects->commit_graph = NULL;\n> }\n> \n> static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n> @@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,\n> \twrite_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);\n> \twrite_graph_chunk_large_edges(f, commits.list, commits.nr);\n> \n> -\tclose_commit_graph();\n> +\tclose_commit_graph(the_repository);\n> \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n> \tcommit_lock_file(&lk);\n> \n> diff --git a/commit-graph.h b/commit-graph.h\n> index 76e098934..13d736cdd 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -59,6 +59,7 @@ void write_commit_graph(const char *obj_dir,\n> \n> int verify_commit_graph(struct repository *r, struct commit_graph *g);\n> \n> +void close_commit_graph(struct repository *);\n> void free_commit_graph(struct commit_graph *);\n> \n> #endif\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 4ca052d0b..52ad6c8e5 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -24,6 +24,7 @@\n> #include \"quote.h\"\n> #include \"upload-pack.h\"\n> #include \"serve.h\"\n> +#include \"commit-graph.h\"\n> \n> /* Remember to update object flag allocation in object.h */\n> #define THEY_HAVE\t(1u << 11)\n> @@ -739,6 +740,7 @@ static void deepen_by_rev_list(int ac, const char **av,\n> {\n> \tstruct commit_list *result;\n> \n> +\tclose_commit_graph(the_repository);\n> \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n> \tsend_shallow(result);\n> \tfree_commit_list(result);\n"},{"id":"354059","messageId":"86muu7pcr9.fsf@gmail.com","threadId":"48907","inReplyTo":"CAGZ79kbAocJU2oqVCr9Dzhy2Qi-iESYGU6z2LbtFF89obaBgqw@mail.gmail.com","subject":"Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-07-31T16:54:02Z","receivedAt":"2018-07-31T16:54:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Stefan Beller writes:\n\n>> I wonder though if all those changes to the testsuite shouldn't be\n>> merged.\n>\n> I think Stolee doesn't want this to be merged after rereading\n> subject and the commit message.\n\nYes, I understand that, and for the most part I agree with it. This\ncommit main purpose is to thoroughly exercise the new feature.\n\nBut I think that changes to the testsuite could have been extracted into\nseparate commit, and merged (and only those changes). It could serve as\nnote about the intent of the test. Well, perhaps after encapsulating it\nin some function with a good name... ;-)\n\nAnyway, this is a minor insignificant thing.\n-- \nJakub Narębski\n"},{"id":"354072","messageId":"CABPp-BHPXG7M682haJgia2RYK5Sc3MYkcQxeBOLaRSbEXoLbUw@mail.gmail.com","threadId":"48907","inReplyTo":"20180718152244.45513-1-dstolee@microsoft.com","subject":"Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-31T17:40:37Z","receivedAt":"2018-07-31T17:40:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 18, 2018 at 8:22 AM, Derrick Stolee wrote:\n> The following test fails because the repo has ambiguous merge-bases, and\n> the commit-graph changes the walk order so we select a different one.\n> This alters the resulting merge from the expected result.\n>\n> t6024-recursive-merge.sh, Test 4\n>\n> The tests above are made to pass by deleting the commit-graph file\n> before the necessary steps.\n\nI know you meant for these to not be merged, but perhaps the test in\nt6024 could be made to be less stringent about order of merge bases.\nIn particular, instead of expecting a certain sha1 to be at stage 2\nand a different one to be at stage 3, it could just check that both\nshas appear in the `git ls-files --stage` output.\n\n> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh\n> index 3f59e58dfb..cec10983cd 100755\n> --- a/t/t6024-recursive-merge.sh\n> +++ b/t/t6024-recursive-merge.sh\n> @@ -61,6 +61,7 @@ GIT_AUTHOR_DATE=\"2006-12-12 23:00:08\" git commit -m F\n> '\n>\n> test_expect_success \"combined merge conflicts\" \"\n> + rm -rf .git/objects/info/commit-graph &&\n> test_must_fail git merge -m final G\n> \"\n>\n> --\n> 2.18.0.118.gd4f65b8d14\n"},{"id":"356113","messageId":"20180820182359.165929-1-dstolee@microsoft.com","threadId":"48907","inReplyTo":"pull.11.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:14Z","receivedAt":"2018-08-20T18:24:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"One unresolved issue with the commit-graph feature is that it can cause\nissues when combined with replace objects, commit grafts, or shallow\nclones. These are not 100% incompatible, as one could be reasonably\nsuccessful writing a commit-graph after replacing some objects and not\nhave issues. The problems happen when commits that are already in the\ncommit-graph file are replaced, or when git is run with the\n`--no-replace-objects` option; this can cause incorrect parents or\nincorrect generation numbers. Similar things occur with commit grafts\nand shallow clones, especially when running `git fetch --unshallow` in a\nshallow repo.\n\nInstead of trying (and probably failing) to make these features work\ntogether, default to making the commit-graph feature unavailable in these\nsituations. Create a new method 'commit_graph_compatible(r)' that checks\nif the repository 'r' has any of these features enabled.\n\nCHANGES IN V2:\n\n* The first two commits regarding the ref iterators are unchanged, despite\n a lot of discussion on the subject [1].\n\n* I included Peff's changes in jk/core-use-replace-refs, changing the base\n commit for the series to 1689c22c1c328e9135ed51458e9f9a5d224c5057 (the merge\n that brought that topic into 'msater').\n\n* I fixed the tests for the interactions with the graft feature.\n\nBecause of the change of base, it is hard to provide a side-by-side diff\nfrom v1.\n\nThanks,\n-Stolee\n\n[1] https://public-inbox.org/git/CAGZ79kZ3PzqpGzXWcmxjzi98gA+LT2MBOf8KaA89hOa-Qig=Og@mail.gmail.com/\n Stefan's response recommending we keep the first two commits.\n\nDerrick Stolee (6):\n commit-graph: update design document\n test-repository: properly init repo\n commit-graph: not compatible with replace objects\n commit-graph: not compatible with grafts\n commit-graph: not compatible with uninitialized repo\n commit-graph: close_commit_graph before shallow walk\n\nStefan Beller (2):\n refs.c: migrate internal ref iteration to pass thru repository\n argument\n refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback\n\n Documentation/technical/commit-graph.txt | 18 +++++--\n builtin/commit-graph.c | 4 ++\n builtin/replace.c | 8 ++--\n commit-graph.c | 38 +++++++++++++--\n commit-graph.h | 1 +\n commit.c | 2 +-\n commit.h | 1 +\n refs.c | 48 ++++++++++++++++---\n refs.h | 12 ++++-\n refs/iterator.c | 6 +--\n refs/refs-internal.h | 5 +-\n replace-object.c | 7 +--\n replace-object.h | 2 +\n t/helper/test-repository.c | 10 +++-\n t/t5318-commit-graph.sh | 60 ++++++++++++++++++++++++\n upload-pack.c | 2 +\n 16 files changed, 194 insertions(+), 30 deletions(-)\n\n\nbase-commit: 1689c22c1c328e9135ed51458e9f9a5d224c5057\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356114","messageId":"20180820182359.165929-7-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 6/8] commit-graph: not compatible with grafts","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:30Z","receivedAt":"2018-08-20T18:24:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Augment commit_graph_compatible(r) to return false when the given\nrepository r has commit grafts or is a shallow clone. Test that in these\nsituations we ignore existing commit-graph files and we do not write new\ncommit-graph files.\n\nHelped-by: Jakub Narebski \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 6 ++++++\n commit.c | 2 +-\n commit.h | 1 +\n t/t5318-commit-graph.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2c01fa433f..c4eaedf4e5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -68,6 +68,12 @@ static int commit_graph_compatible(struct repository *r)\n \t\t\treturn 0;\n \t}\n \n+\tprepare_commit_graft(r);\n+\tif (r->parsed_objects && r->parsed_objects->grafts_nr)\n+\t\treturn 0;\n+\tif (is_repository_shallow(r))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \ndiff --git a/commit.c b/commit.c\nindex 30d1af2b20..ef9a2cbb23 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -209,7 +209,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n \treturn 0;\n }\n \n-static void prepare_commit_graft(struct repository *r)\n+void prepare_commit_graft(struct repository *r)\n {\n \tchar *graft_file;\n \ndiff --git a/commit.h b/commit.h\nindex da0db36eba..5459e279fe 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -202,6 +202,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n struct commit_graft *read_graft_line(struct strbuf *line);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n+void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n \n extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex e0c3c60f66..6aee861f78 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -281,6 +281,44 @@ test_expect_success 'replace-objects invalidates commit-graph' '\n \t)\n '\n \n+test_expect_success 'commit grafts invalidate commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf graft &&\n+\tgit clone full graft &&\n+\t(\n+\t\tcd graft &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph &&\n+\t\tH1=$(git rev-parse --verify HEAD~1) &&\n+\t\tH3=$(git rev-parse --verify HEAD~3) &&\n+\t\techo \"$H1 $H3\" >.git/info/grafts &&\n+\t\tgit -c core.commitGraph=false log >expect &&\n+\t\tgit -c core.commitGraph=true log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n+\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\trm -rf .git/objects/info/commit-graph &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_missing .git/objects/info/commit-graph\n+\t)\n+'\n+\n+test_expect_success 'replace-objects invalidates commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf shallow &&\n+\tgit clone --depth 2 \"file://$TRASH_DIRECTORY/full\" shallow &&\n+\t(\n+\t\tcd shallow &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_missing .git/objects/info/commit-graph &&\n+\t\tgit fetch origin --unshallow &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356115","messageId":"20180820182359.165929-8-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 7/8] commit-graph: not compatible with uninitialized repo","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:32Z","receivedAt":"2018-08-20T18:24:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Signed-off-by: Derrick Stolee \n---\n commit-graph.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex c4eaedf4e5..cee2caab5c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -62,6 +62,9 @@ extern int read_replace_refs;\n \n static int commit_graph_compatible(struct repository *r)\n {\n+\tif (!r->gitdir)\n+\t\treturn 0;\n+\n \tif (read_replace_refs) {\n \t\tprepare_replace_object(r);\n \t\tif (hashmap_get_size(&r->objects->replace_map->map))\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356116","messageId":"20180820182359.165929-3-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 2/8] refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:19Z","receivedAt":"2018-08-20T18:24:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Stefan Beller \n\nSigned-off-by: Stefan Beller \nSigned-off-by: Derrick Stolee \n---\n builtin/replace.c | 8 ++++----\n refs.c | 9 ++++-----\n refs.h | 2 +-\n replace-object.c | 5 +++--\n 4 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex e997a562f0..b5861a0ee9 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -39,7 +39,8 @@ struct show_data {\n \tenum replace_format format;\n };\n \n-static int show_reference(const char *refname, const struct object_id *oid,\n+static int show_reference(struct repository *r, const char *refname,\n+\t\t\t const struct object_id *oid,\n \t\t\t int flag, void *cb_data)\n {\n \tstruct show_data *data = cb_data;\n@@ -56,9 +57,8 @@ static int show_reference(const char *refname, const struct object_id *oid,\n \t\t\tif (get_oid(refname, &object))\n \t\t\t\treturn error(\"Failed to resolve '%s' as a valid ref.\", refname);\n \n-\t\t\tobj_type = oid_object_info(the_repository, &object,\n-\t\t\t\t\t\t NULL);\n-\t\t\trepl_type = oid_object_info(the_repository, oid, NULL);\n+\t\t\tobj_type = oid_object_info(r, &object, NULL);\n+\t\t\trepl_type = oid_object_info(r, oid, NULL);\n \n \t\t\tprintf(\"%s (%s) -> %s (%s)\\n\", refname, type_name(obj_type),\n \t\t\t oid_to_hex(oid), type_name(repl_type));\ndiff --git a/refs.c b/refs.c\nindex 7cd76f72d2..c5a5f727e8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1474,12 +1474,11 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n \treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n }\n \n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_main_ref_store(r),\n-\t\t\t git_replace_ref_base, fn,\n-\t\t\t strlen(git_replace_ref_base),\n-\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n+\treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n+\t\t\t\t strlen(git_replace_ref_base),\n+\t\t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \n int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\ndiff --git a/refs.h b/refs.h\nindex 80eec8bbc6..a0a18223a1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -317,7 +317,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,\n int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);\n+int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data);\n int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,\n \t\t\t const char *prefix, void *cb_data);\ndiff --git a/replace-object.c b/replace-object.c\nindex 4162df6324..3c17864eb7 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -6,7 +6,8 @@\n #include \"repository.h\"\n #include \"commit.h\"\n \n-static int register_replace_ref(const char *refname,\n+static int register_replace_ref(struct repository *r,\n+\t\t\t\tconst char *refname,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tint flag, void *cb_data)\n {\n@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,\n \toidcpy(&repl_obj->replacement, oid);\n \n \t/* Register new object */\n-\tif (oidmap_put(the_repository->objects->replace_map, repl_obj))\n+\tif (oidmap_put(r->objects->replace_map, repl_obj))\n \t\tdie(\"duplicate replace ref: %s\", refname);\n \n \treturn 0;\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356117","messageId":"20180820182359.165929-4-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 3/8] commit-graph: update design document","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:21Z","receivedAt":"2018-08-20T18:25:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As it exists right now, the commit-graph feature may provide\ninconsistent results when combined with commit grafts, replace objects,\nand shallow clones. Update the design document to discuss why these\ninteractions are difficult to reconcile and how we will avoid errors by\npreventing updates to and reads from the commit-graph file when these\nother features exist.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/technical/commit-graph.txt | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt\nindex c664acbd76..001395e950 100644\n--- a/Documentation/technical/commit-graph.txt\n+++ b/Documentation/technical/commit-graph.txt\n@@ -112,12 +112,24 @@ Design Details\n - The file format includes parameters for the object ID hash function,\n so a future change of hash algorithm does not require a change in format.\n \n+- Commit grafts and replace objects can change the shape of the commit\n+ history. The latter can also be enabled/disabled on the fly using\n+ `--no-replace-objects`. This leads to difficultly storing both possible\n+ interpretations of a commit id, especially when computing generation\n+ numbers. The commit-graph will not be read or written when\n+ replace-objects or grafts are present.\n+\n+- Shallow clones create grafts of commits by dropping their parents. This\n+ leads the commit-graph to think those commits have generation number 1.\n+ If and when those commits are made unshallow, those generation numbers\n+ become invalid. Since shallow clones are intended to restrict the commit\n+ history to a very small set of commits, the commit-graph feature is less\n+ helpful for these clones, anyway. The commit-graph will not be read or\n+ written when shallow commits are present.\n+\n Future Work\n -----------\n \n-- The commit graph feature currently does not honor commit grafts. This can\n- be remedied by duplicating or refactoring the current graft logic.\n-\n - After computing and storing generation numbers, we must make graph\n walks aware of generation numbers to gain the performance benefits they\n enable. This will mostly be accomplished by swapping a commit-date-ordered\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356118","messageId":"20180820182359.165929-5-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 4/8] test-repository: properly init repo","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:24Z","receivedAt":"2018-08-20T18:25:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Signed-off-by: Derrick Stolee \n---\n t/helper/test-repository.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex 2762ca6562..6a84a53efb 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -15,7 +15,10 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \tstruct commit *c;\n \tstruct commit_list *parent;\n \n-\trepo_init(&r, gitdir, worktree);\n+\tsetup_git_env(gitdir);\n+\n+\tif (repo_init(&r, gitdir, worktree))\n+\t\tdie(\"Couldn't init repo\");\n \n \tc = lookup_commit(&r, commit_oid);\n \n@@ -38,7 +41,10 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \tstruct commit *c;\n \tstruct tree *tree;\n \n-\trepo_init(&r, gitdir, worktree);\n+\tsetup_git_env(gitdir);\n+\n+\tif (repo_init(&r, gitdir, worktree))\n+\t\tdie(\"Couldn't init repo\");\n \n \tc = lookup_commit(&r, commit_oid);\n \n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356119","messageId":"20180820182359.165929-6-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:27Z","receivedAt":"2018-08-20T18:25:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Create new method commit_graph_compatible(r) to check if a given\nrepository r is compatible with the commit-graph feature. Fill the\nmethod with a check to see if replace-objects exist. Test this\ninteraction succeeds, including ignoring an existing commit-graph and\nfailing to write a new commit-graph. However, we do ensure that\nwe write a new commit-graph by setting read_replace_refs to 0, thereby\nignoring the replace refs.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/commit-graph.c | 4 ++++\n commit-graph.c | 21 +++++++++++++++++++++\n replace-object.c | 2 +-\n replace-object.h | 2 ++\n t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++\n 5 files changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 0bf0c48657..da737df321 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)\n \treturn 0;\n }\n \n+extern int read_replace_refs;\n+\n static int graph_write(int argc, const char **argv)\n {\n \tstruct string_list *pack_indexes = NULL;\n@@ -150,6 +152,8 @@ static int graph_write(int argc, const char **argv)\n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n \n+\tread_replace_refs = 0;\n+\n \tif (opts.reachable) {\n \t\twrite_commit_graph_reachable(opts.obj_dir, opts.append);\n \t\treturn 0;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b0a55ad128..2c01fa433f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -13,6 +13,8 @@\n #include \"commit-graph.h\"\n #include \"object-store.h\"\n #include \"alloc.h\"\n+#include \"hashmap.h\"\n+#include \"replace-object.h\"\n \n #define GRAPH_SIGNATURE 0x43475048 /* \"CGPH\" */\n #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* \"OIDF\" */\n@@ -56,6 +58,19 @@ static struct commit_graph *alloc_commit_graph(void)\n \treturn g;\n }\n \n+extern int read_replace_refs;\n+\n+static int commit_graph_compatible(struct repository *r)\n+{\n+\tif (read_replace_refs) {\n+\t\tprepare_replace_object(r);\n+\t\tif (hashmap_get_size(&r->objects->replace_map->map))\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n struct commit_graph *load_commit_graph_one(const char *graph_file)\n {\n \tvoid *graph_map;\n@@ -223,6 +238,9 @@ static int prepare_commit_graph(struct repository *r)\n \t\t */\n \t\treturn 0;\n \n+\tif (!commit_graph_compatible(r))\n+\t\treturn 0;\n+\n \tobj_dir = r->objects->objectdir;\n \tprepare_commit_graph_one(r, obj_dir);\n \tprepare_alt_odb(r);\n@@ -693,6 +711,9 @@ void write_commit_graph(const char *obj_dir,\n \tint num_extra_edges;\n \tstruct commit_list *parent;\n \n+\tif (!commit_graph_compatible(the_repository))\n+\t\treturn;\n+\n \toids.nr = 0;\n \toids.alloc = approximate_object_count() / 4;\n \ndiff --git a/replace-object.c b/replace-object.c\nindex 3c17864eb7..9821f1477e 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,\n \treturn 0;\n }\n \n-static void prepare_replace_object(struct repository *r)\n+void prepare_replace_object(struct repository *r)\n {\n \tif (r->objects->replace_map)\n \t\treturn;\ndiff --git a/replace-object.h b/replace-object.h\nindex 9345e105dd..16528df942 100644\n--- a/replace-object.h\n+++ b/replace-object.h\n@@ -10,6 +10,8 @@ struct replace_object {\n \tstruct object_id replacement;\n };\n \n+void prepare_replace_object(struct repository *r);\n+\n /*\n * This internal function is only declared here for the benefit of\n * lookup_replace_object(). Please do not call it directly.\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 4f17d7701e..e0c3c60f66 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -259,6 +259,28 @@ test_expect_success 'check that gc computes commit-graph' '\n \ttest_cmp commit-graph-after-gc $objdir/info/commit-graph\n '\n \n+test_expect_success 'replace-objects invalidates commit-graph' '\n+\tcd \"$TRASH_DIRECTORY\" &&\n+\ttest_when_finished rm -rf replace &&\n+\tgit clone full replace &&\n+\t(\n+\t\tcd replace &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph &&\n+\t\tgit replace HEAD~1 HEAD~2 &&\n+\t\tgit -c core.commitGraph=false log >expect &&\n+\t\tgit -c core.commitGraph=true log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit commit-graph write --reachable &&\n+\t\tgit -c core.commitGraph=false --no-replace-objects log >expect &&\n+\t\tgit -c core.commitGraph=true --no-replace-objects log >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\trm -rf .git/objects/info/commit-graph &&\n+\t\tgit commit-graph write --reachable &&\n+\t\ttest_path_is_file .git/objects/info/commit-graph\n+\t)\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356120","messageId":"20180820182359.165929-9-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 8/8] commit-graph: close_commit_graph before shallow walk","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:34Z","receivedAt":"2018-08-20T18:26:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Call close_commit_graph() when about to start a rev-list walk that\nincludes shallow commits. This is necessary in code paths that \"fake\"\nshallow commits for the sake of fetch. Specifically, test 351 in\nt5500-fetch-pack.sh runs\n\n\tgit fetch --shallow-exclude one origin\n\nwith a file-based transfer. When the \"remote\" has a commit-graph, we do\nnot prevent the commit-graph from being loaded, but then the commits are\nintended to be dynamically transferred into shallow commits during\nget_shallow_commits_by_rev_list(). By closing the commit-graph before\nthis call, we prevent this interaction.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 8 ++++----\n commit-graph.h | 1 +\n upload-pack.c | 2 ++\n 3 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex cee2caab5c..4bd1a4abbf 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -260,10 +260,10 @@ static int prepare_commit_graph(struct repository *r)\n \treturn !!r->objects->commit_graph;\n }\n \n-static void close_commit_graph(void)\n+void close_commit_graph(struct repository *r)\n {\n-\tfree_commit_graph(the_repository->objects->commit_graph);\n-\tthe_repository->objects->commit_graph = NULL;\n+\tfree_commit_graph(r->objects->commit_graph);\n+\tr->objects->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -875,7 +875,7 @@ void write_commit_graph(const char *obj_dir,\n \twrite_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);\n \twrite_graph_chunk_large_edges(f, commits.list, commits.nr);\n \n-\tclose_commit_graph();\n+\tclose_commit_graph(the_repository);\n \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n \tcommit_lock_file(&lk);\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 76e098934a..13d736cdde 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -59,6 +59,7 @@ void write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n+void close_commit_graph(struct repository *);\n void free_commit_graph(struct commit_graph *);\n \n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 82b393ec31..2ae9d9bb47 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -24,6 +24,7 @@\n #include \"quote.h\"\n #include \"upload-pack.h\"\n #include \"serve.h\"\n+#include \"commit-graph.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -740,6 +741,7 @@ static void deepen_by_rev_list(int ac, const char **av,\n {\n \tstruct commit_list *result;\n \n+\tclose_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(result);\n \tfree_commit_list(result);\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356121","messageId":"20180820182359.165929-2-dstolee@microsoft.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"[PATCH v2 1/8] refs.c: migrate internal ref iteration to pass thru repository argument","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T18:24:16Z","receivedAt":"2018-08-20T18:27:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Stefan Beller \n\nIn 60ce76d3581 (refs: add repository argument to for_each_replace_ref,\n2018-04-11) and 0d296c57aec (refs: allow for_each_replace_ref to handle\narbitrary repositories, 2018-04-11), for_each_replace_ref learned how\nto iterate over refs by a given arbitrary repository.\nNew attempts in the object store conversion have shown that it is useful\nto have the repository handle available that the refs iteration is\ncurrently iterating over.\n\nTo achieve this goal we will need to add a repository argument to\neach_ref_fn in refs.h. However as many callers rely on the signature\nsuch a patch would be too large.\n\nSo convert the internals of the ref subsystem first to pass through a\nrepository argument without exposing the change to the user. Assume\nthe_repository for the passed through repository, although it is not\nused anywhere yet.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Derrick Stolee \n---\n refs.c | 39 +++++++++++++++++++++++++++++++++++++--\n refs.h | 10 ++++++++++\n refs/iterator.c | 6 +++---\n refs/refs-internal.h | 5 +++--\n 4 files changed, 53 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 457fb78057..7cd76f72d2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1386,17 +1386,50 @@ struct ref_iterator *refs_ref_iterator_begin(\n * non-zero value, stop the iteration and return that value;\n * otherwise, return 0.\n */\n+static int do_for_each_repo_ref(struct repository *r, const char *prefix,\n+\t\t\t\teach_repo_ref_fn fn, int trim, int flags,\n+\t\t\t\tvoid *cb_data)\n+{\n+\tstruct ref_iterator *iter;\n+\tstruct ref_store *refs = get_main_ref_store(r);\n+\n+\tif (!refs)\n+\t\treturn 0;\n+\n+\titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n+\n+\treturn do_for_each_repo_ref_iterator(r, iter, fn, cb_data);\n+}\n+\n+struct do_for_each_ref_help {\n+\teach_ref_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_ref_helper(struct repository *r,\n+\t\t\t\t const char *refname,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct do_for_each_ref_help *hp = cb_data;\n+\n+\treturn hp->fn(refname, oid, flags, hp->cb_data);\n+}\n+\n static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n \t\t\t each_ref_fn fn, int trim, int flags, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \tif (!refs)\n \t\treturn 0;\n \n \titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\tdo_for_each_ref_helper, &hp);\n }\n \n int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n@@ -2025,10 +2058,12 @@ int refs_verify_refname_available(struct ref_store *refs,\n int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n+\tstruct do_for_each_ref_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n-\treturn do_for_each_ref_iterator(iter, fn, cb_data);\n+\treturn do_for_each_repo_ref_iterator(the_repository, iter,\n+\t\t\t\t\t do_for_each_ref_helper, &hp);\n }\n \n int for_each_reflog(each_ref_fn fn, void *cb_data)\ndiff --git a/refs.h b/refs.h\nindex cc2fb4c68c..80eec8bbc6 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -274,6 +274,16 @@ struct ref_transaction;\n typedef int each_ref_fn(const char *refname,\n \t\t\tconst struct object_id *oid, int flags, void *cb_data);\n \n+/*\n+ * The same as each_ref_fn, but also with a repository argument that\n+ * contains the repository associated with the callback.\n+ */\n+typedef int each_repo_ref_fn(struct repository *r,\n+\t\t\t const char *refname,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int flags,\n+\t\t\t void *cb_data);\n+\n /*\n * The following functions invoke the specified callback function for\n * each reference indicated. If the function ever returns a nonzero\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 2ac91ac340..629e00a122 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -407,15 +407,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \n struct ref_iterator *current_ref_iter = NULL;\n \n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t each_ref_fn fn, void *cb_data)\n+int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter,\n+\t\t\t\t each_repo_ref_fn fn, void *cb_data)\n {\n \tint retval = 0, ok;\n \tstruct ref_iterator *old_ref_iter = current_ref_iter;\n \n \tcurrent_ref_iter = iter;\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n-\t\tretval = fn(iter->refname, iter->oid, iter->flags, cb_data);\n+\t\tretval = fn(r, iter->refname, iter->oid, iter->flags, cb_data);\n \t\tif (retval) {\n \t\t\t/*\n \t\t\t * If ref_iterator_abort() returns ITER_ERROR,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 04425d6d1e..89d7dd49cd 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -474,8 +474,9 @@ extern struct ref_iterator *current_ref_iter;\n * adapter between the callback style of reference iteration and the\n * iterator style.\n */\n-int do_for_each_ref_iterator(struct ref_iterator *iter,\n-\t\t\t each_ref_fn fn, void *cb_data);\n+int do_for_each_repo_ref_iterator(struct repository *r,\n+\t\t\t\t struct ref_iterator *iter,\n+\t\t\t\t each_repo_ref_fn fn, void *cb_data);\n \n /*\n * Only include per-worktree refs in a do_for_each_ref*() iteration.\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356124","messageId":"CAGZ79kaAAo+=nrcipfd4kp2iHXFPdo_zNxv8mEgQkN3JbTwicg@mail.gmail.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T19:06:56Z","receivedAt":"2018-08-20T19:07:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee wrote:\n> Because of the change of base, it is hard to provide a side-by-side diff\n> from v1.\n\nI thought that was the whole point of range-diff. ;-)\n\nI'll take a look.\n\nThanks,\nStefan\n"},{"id":"356129","messageId":"CAGZ79ka6=Zw8pFhwVysoE3Wa+mpUm4E83cf7TCKmYZ7XC0ZL2A@mail.gmail.com","threadId":"48907","inReplyTo":"20180820182359.165929-1-dstolee@microsoft.com","subject":"Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T19:37:42Z","receivedAt":"2018-08-20T19:37:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee wrote:\n>\n> One unresolved issue with the commit-graph feature is that it can cause\n> issues when combined with replace objects, commit grafts, or shallow\n> clones. These are not 100% incompatible, as one could be reasonably\n> successful writing a commit-graph after replacing some objects and not\n> have issues. The problems happen when commits that are already in the\n> commit-graph file are replaced, or when git is run with the\n> `--no-replace-objects` option; this can cause incorrect parents or\n> incorrect generation numbers. Similar things occur with commit grafts\n> and shallow clones, especially when running `git fetch --unshallow` in a\n> shallow repo.\n>\n> Instead of trying (and probably failing) to make these features work\n> together, default to making the commit-graph feature unavailable in these\n> situations. Create a new method 'commit_graph_compatible(r)' that checks\n> if the repository 'r' has any of these features enabled.\n>\n> CHANGES IN V2:\n>\n> * The first two commits regarding the ref iterators are unchanged, despite\n> a lot of discussion on the subject [1].\n>\n> * I included Peff's changes in jk/core-use-replace-refs, changing the base\n> commit for the series to 1689c22c1c328e9135ed51458e9f9a5d224c5057 (the merge\n> that brought that topic into 'msater').\n>\n> * I fixed the tests for the interactions with the graft feature.\n>\n> Because of the change of base, it is hard to provide a side-by-side diff\n> from v1.\n>\n> Thanks,\n> -Stolee\n>\n> [1] https://public-inbox.org/git/CAGZ79kZ3PzqpGzXWcmxjzi98gA+LT2MBOf8KaA89hOa-Qig=Og@mail.gmail.com/\n> Stefan's response recommending we keep the first two commits.\n>\n\nAfter reviewing my own patches, I flipped again (Sorry!) and would\nrather not see my patches be merged, but the very original solution\nby you, that you proposed at [1]. That said, I will not insist on it, and\nif this is merged as is, we can fix it up later.\n\nWith that said, I just read through the remaining patches, I think\nthey are a valuable addition to Git and could be merged as-is.\n\n[1] https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1\n\nThanks,\nStefan\n"},{"id":"356134","messageId":"d13e2b64-e078-e409-8b39-01a23385e3c9@gmail.com","threadId":"48907","inReplyTo":"CAGZ79ka6=Zw8pFhwVysoE3Wa+mpUm4E83cf7TCKmYZ7XC0ZL2A@mail.gmail.com","subject":"Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-20T19:54:25Z","receivedAt":"2018-08-20T19:54:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2018 3:37 PM, Stefan Beller wrote:\n> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee wrote:\n>> One unresolved issue with the commit-graph feature is that it can cause\n>> issues when combined with replace objects, commit grafts, or shallow\n>> clones. These are not 100% incompatible, as one could be reasonably\n>> successful writing a commit-graph after replacing some objects and not\n>> have issues. The problems happen when commits that are already in the\n>> commit-graph file are replaced, or when git is run with the\n>> `--no-replace-objects` option; this can cause incorrect parents or\n>> incorrect generation numbers. Similar things occur with commit grafts\n>> and shallow clones, especially when running `git fetch --unshallow` in a\n>> shallow repo.\n>>\n>> Instead of trying (and probably failing) to make these features work\n>> together, default to making the commit-graph feature unavailable in these\n>> situations. Create a new method 'commit_graph_compatible(r)' that checks\n>> if the repository 'r' has any of these features enabled.\n>>\n>> CHANGES IN V2:\n>>\n>> * The first two commits regarding the ref iterators are unchanged, despite\n>> a lot of discussion on the subject [1].\n>>\n>> * I included Peff's changes in jk/core-use-replace-refs, changing the base\n>> commit for the series to 1689c22c1c328e9135ed51458e9f9a5d224c5057 (the merge\n>> that brought that topic into 'msater').\n>>\n>> * I fixed the tests for the interactions with the graft feature.\n>>\n>> Because of the change of base, it is hard to provide a side-by-side diff\n>> from v1.\n>>\n>> Thanks,\n>> -Stolee\n>>\n>> [1] https://public-inbox.org/git/CAGZ79kZ3PzqpGzXWcmxjzi98gA+LT2MBOf8KaA89hOa-Qig=Og@mail.gmail.com/\n>> Stefan's response recommending we keep the first two commits.\n>>\n> After reviewing my own patches, I flipped again (Sorry!) and would\n> rather not see my patches be merged, but the very original solution\n> by you, that you proposed at [1]. That said, I will not insist on it, and\n> if this is merged as is, we can fix it up later.\n>\n> With that said, I just read through the remaining patches, I think\n> they are a valuable addition to Git and could be merged as-is.\n>\n> [1] https://github.com/gitgitgadget/git/pull/11/commits/300db80140dacc927db0d46c804ca0ef4dcc1be1\n>\n> Thanks,\n> Stefan\n\nJust to keep things on-list as possible, here is the patch for the \ncommit linked above. It would replace patches 1 & 2 from this series.\n\nJunio: I can send a v3 that includes this commit if you need it, or if \nthere are other edits to be made in this series.\n\ncommit 300db80140dacc927db0d46c804ca0ef4dcc1be1\nAuthor: Derrick Stolee \nDate:   Fri Jul 20 15:39:06 2018 -0400\n\n     replace-objects: use arbitrary repositories\n\n     This is the smallest possible change that makes prepare_replace_objects\n     work properly with arbitrary repositories. By supplying the repository\n     as the cb_data, we do not need to modify any code in the ref iterator\n     logic. We will likely want to do a full replacement of the ref iterator\n     logic to provide a repository struct as a concrete parameter.\n\n     Signed-off-by: Derrick Stolee \n\ndiff --git a/replace-object.c b/replace-object.c\nindex 801b5c1678..e99fcd1ff6 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -14,6 +14,7 @@ static int register_replace_ref(const char *refname,\n         const char *slash = strrchr(refname, '/');\n         const char *hash = slash ? slash + 1 : refname;\n         struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));\n+       struct repository *r = (struct repository *)cb_data;\n\n         if (get_oid_hex(hash, &repl_obj->original.oid)) {\n                 free(repl_obj);\n@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,\n         oidcpy(&repl_obj->replacement, oid);\n\n         /* Register new object */\n-       if (oidmap_put(the_repository->objects->replace_map, repl_obj))\n+       if (oidmap_put(r->objects->replace_map, repl_obj))\n                 die(\"duplicate replace ref: %s\", refname);\n\n         return 0;\n@@ -40,7 +41,7 @@ static void prepare_replace_object(struct repository *r)\n                 xmalloc(sizeof(*r->objects->replace_map));\n         oidmap_init(r->objects->replace_map, 0);\n\n-       for_each_replace_ref(r, register_replace_ref, NULL);\n+       for_each_replace_ref(r, register_replace_ref, r);\n  }\n\n  /* We allow \"recursive\" replacement. Only within reason, though */\n\n"},{"id":"356177","messageId":"xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com","threadId":"48907","inReplyTo":"20180820182359.165929-6-dstolee@microsoft.com","subject":"Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T17:45:26Z","receivedAt":"2018-08-21T17:45:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Create new method commit_graph_compatible(r) to check if a given\n> repository r is compatible with the commit-graph feature. Fill the\n> method with a check to see if replace-objects exist. Test this\n> interaction succeeds, including ignoring an existing commit-graph and\n> failing to write a new commit-graph. However, we do ensure that\n> we write a new commit-graph by setting read_replace_refs to 0, thereby\n> ignoring the replace refs.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/commit-graph.c | 4 ++++\n> commit-graph.c | 21 +++++++++++++++++++++\n> replace-object.c | 2 +-\n> replace-object.h | 2 ++\n> t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++\n> 5 files changed, 50 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 0bf0c48657..da737df321 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)\n> \treturn 0;\n> }\n> \n> +extern int read_replace_refs;\n> +\n\nWhy do you need this (and also in commit-graph.c)? I thought\ncache.h includes it, which you can just make use of it.\n\n> +static int commit_graph_compatible(struct repository *r)\n> +{\n> +\tif (read_replace_refs) {\n> +\t\tprepare_replace_object(r);\n> +\t\tif (hashmap_get_size(&r->objects->replace_map->map))\n> +\t\t\treturn 0;\n> +\t}\n> +\n> +\treturn 1;\n> +}\n\n> diff --git a/replace-object.c b/replace-object.c\n> index 3c17864eb7..9821f1477e 100644\n> --- a/replace-object.c\n> +++ b/replace-object.c\n> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,\n> \treturn 0;\n> }\n> \n> -static void prepare_replace_object(struct repository *r)\n> +void prepare_replace_object(struct repository *r)\n> {\n> \tif (r->objects->replace_map)\n> \t\treturn;\n\nThe way the new caller is written, I am wondering if this function\nshould return \"we are (or, are not) using the replace object\nfeature\", making it unnecessary for callers on the reading side to\nknow about \"read-replace-refs\" external variable, for example.\n\n\t/*\n\t * To be called on-demand from codepaths that want to make\n\t * sure that replacement objects can be found as necessary.\n\t * \n\t * Return number of replacement defined for the repository, or\n\t * -1 when !read_replace_refs tells us not to use replacement\n\t * mechanism at all.\n\t */\n\tint prepare_replace_object(struct repository *r)\n\t{\n\t\tif (!read_replace_refs)\n\t\t\treturn -1;\n\n\t\tif (!r->objects->replace_map) {\n\t\t\tr->objects->replace_map =\n\t\t\t\txmalloc(...);\n\t\t\toidmap_init(r->objects->replace_map, 0);\n\t\t\tfor_each_refplace_ref(r, register_...);\n\t\t}\n\t\treturn hashmap_get_size(&r->objects->replace_map->map);\n\t}\n\t\t\t\nThen, the caller side can simply become something like:\n\n\t#define cgraph_compat(r) (prepare_replace_object(r) <= 0)\n\nThere are various writers to read_replace_refs variable, but I think\nthey should first be replaced with calls to something like:\n\n\tvoid use_replace_refs(struct repository *r, int enable);\n\nwhich allows us to hide the global variable and later make it per\nrepository if we wanted to.\n\n"},{"id":"356178","messageId":"xmqq6003a9r3.fsf@gitster-ct.c.googlers.com","threadId":"48907","inReplyTo":"CAGZ79kaAAo+=nrcipfd4kp2iHXFPdo_zNxv8mEgQkN3JbTwicg@mail.gmail.com","subject":"Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T17:51:28Z","receivedAt":"2018-08-21T17:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee wrote:\n>> Because of the change of base, it is hard to provide a side-by-side diff\n>> from v1.\n>\n> I thought that was the whole point of range-diff. ;-)\n\nI thought so, too. Was there any change that confused range-diff\nmachinery?\n"},{"id":"356180","messageId":"76ef06b5-87cb-ff40-a424-01a3b0ed435a@gmail.com","threadId":"48907","inReplyTo":"xmqq6003a9r3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/8] Clarify commit-graph and grafts/replace/shallow incompatibilities","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T18:35:48Z","receivedAt":"2018-08-21T18:35:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 1:51 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Mon, Aug 20, 2018 at 11:24 AM Derrick Stolee wrote:\n>>> Because of the change of base, it is hard to provide a side-by-side diff\n>>> from v1.\n>> I thought that was the whole point of range-diff. ;-)\n> I thought so, too. Was there any change that confused range-diff\n> machinery?\n\nI've just been out of the loop and didn't realize what range-diff did. \nHere is the range-diff:\n\n1:  43ddcc9ef9 = 1:  c8edae4179 refs.c: migrate internal ref iteration \nto pass thru repository argument\n2:  22dc9ce836 = 2:  f89451e884 refs.c: upgrade for_each_replace_ref to \nbe a each_repo_ref_fn callback\n3:  5e90d36f84 ! 3:  d95aa3472c commit-graph: update design document\n     @@ -19,7 +19,7 @@\n         so a future change of hash algorithm does not require a change \nin format.\n\n      +- Commit grafts and replace objects can change the shape of the \ncommit\n     -+  history. These can also be enabled/disabled on the fly using\n     ++  history. The latter can also be enabled/disabled on the fly using\n      +  `--no-replace-objects`. This leads to difficultly storing both \npossible\n      +  interpretations of a commit id, especially when computing \ngeneration\n      +  numbers. The commit-graph will not be read or written when\n4:  88711a3cf4 = 4:  ec89c88e14 test-repository: properly init repo\n5:  7f596c1718 ! 5:  0321a3cf10 commit-graph: not compatible with \nreplace objects\n     @@ -6,10 +6,34 @@\n          repository r is compatible with the commit-graph feature. Fill the\n          method with a check to see if replace-objects exist. Test this\n          interaction succeeds, including ignoring an existing \ncommit-graph and\n     -    failing to write a new commit-graph.\n     +    failing to write a new commit-graph. However, we do ensure that\n     +    we write a new commit-graph by setting read_replace_refs to 0, \nthereby\n     +    ignoring the replace refs.\n\n          Signed-off-by: Derrick Stolee \n\n     +diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n     +--- a/builtin/commit-graph.c\n     ++++ b/builtin/commit-graph.c\n     +@@\n     +   return 0;\n     + }\n     +\n     ++extern int read_replace_refs;\n     ++\n     + static int graph_write(int argc, const char **argv)\n     + {\n     +   struct string_list *pack_indexes = NULL;\n     +@@\n     +   if (!opts.obj_dir)\n     +           opts.obj_dir = get_object_directory();\n     +\n     ++  read_replace_refs = 0;\n     ++\n     +   if (opts.reachable) {\n     +           write_commit_graph_reachable(opts.obj_dir, opts.append);\n     +           return 0;\n     +\n      diff --git a/commit-graph.c b/commit-graph.c\n      --- a/commit-graph.c\n      +++ b/commit-graph.c\n     @@ -26,11 +50,15 @@\n         return g;\n       }\n\n     ++extern int read_replace_refs;\n     ++\n      +static int commit_graph_compatible(struct repository *r)\n      +{\n     -+  prepare_replace_object(r);\n     -+  if (hashmap_get_size(&r->objects->replace_map->map))\n     -+          return 0;\n     ++  if (read_replace_refs) {\n     ++          prepare_replace_object(r);\n     ++          if (hashmap_get_size(&r->objects->replace_map->map))\n     ++                  return 0;\n     ++  }\n      +\n      +  return 1;\n      +}\n     @@ -110,7 +138,7 @@\n      +          test_cmp expect actual &&\n      +          rm -rf .git/objects/info/commit-graph &&\n      +          git commit-graph write --reachable &&\n     -+          test_path_is_missing .git/objects/info/commit-graph\n     ++          test_path_is_file .git/objects/info/commit-graph\n      +  )\n      +'\n      +\n6:  94dd91ac35 ! 6:  d18ecc0124 commit-graph: not compatible with grafts\n     @@ -7,24 +7,25 @@\n          situations we ignore existing commit-graph files and we do not \nwrite new\n          commit-graph files.\n\n     +    Helped-by: Jakub Narebski \n          Signed-off-by: Derrick Stolee \n\n      diff --git a/commit-graph.c b/commit-graph.c\n      --- a/commit-graph.c\n      +++ b/commit-graph.c\n      @@\n     +                   return 0;\n     +   }\n\n     - static int commit_graph_compatible(struct repository *r)\n     - {\n      +  prepare_commit_graft(r);\n      +  if (r->parsed_objects && r->parsed_objects->grafts_nr)\n      +          return 0;\n      +  if (is_repository_shallow(r))\n      +          return 0;\n      +\n     -   prepare_replace_object(r);\n     -   if (hashmap_get_size(&r->objects->replace_map->map))\n     -           return 0;\n     +   return 1;\n     + }\n     +\n\n      diff --git a/commit.c b/commit.c\n      --- a/commit.c\n     @@ -66,7 +67,9 @@\n      +          cd graft &&\n      +          git commit-graph write --reachable &&\n      +          test_path_is_file .git/objects/info/commit-graph &&\n     -+          git replace --graft HEAD~1 HEAD~3 &&\n     ++          H1=$(git rev-parse --verify HEAD~1) &&\n     ++          H3=$(git rev-parse --verify HEAD~3) &&\n     ++          echo \"$H1 $H3\" >.git/info/grafts &&\n      +          git -c core.commitGraph=false log >expect &&\n      +          git -c core.commitGraph=true log >actual &&\n      +          test_cmp expect actual &&\n7:  5314a5a93d ! 7:  87d3397700 commit-graph: not compatible with \nuninitialized repo\n     @@ -14,6 +14,6 @@\n      +  if (!r->gitdir)\n      +          return 0;\n      +\n     -   prepare_commit_graft(r);\n     -   if (r->parsed_objects && r->parsed_objects->grafts_nr)\n     -           return 0;\n     +   if (read_replace_refs) {\n     +           prepare_replace_object(r);\n     +           if (hashmap_get_size(&r->objects->replace_map->map))\n8:  f4ab234ed2 ! 8:  a26c175897 commit-graph: close_commit_graph before \nshallow walk\n     @@ -2,8 +2,6 @@\n\n          commit-graph: close_commit_graph before shallow walk\n\n     -    Make close_commit_graph() work for arbitrary repositories.\n     -\n          Call close_commit_graph() when about to start a rev-list walk that\n          includes shallow commits. This is necessary in code paths that \n\"fake\"\n          shallow commits for the sake of fetch. Specifically, test 351 in\n\n"},{"id":"356181","messageId":"78b20554-1522-9661-cb29-09ffd438b206@gmail.com","threadId":"48907","inReplyTo":"xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T18:39:57Z","receivedAt":"2018-08-21T18:40:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 1:45 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>\n>> Create new method commit_graph_compatible(r) to check if a given\n>> repository r is compatible with the commit-graph feature. Fill the\n>> method with a check to see if replace-objects exist. Test this\n>> interaction succeeds, including ignoring an existing commit-graph and\n>> failing to write a new commit-graph. However, we do ensure that\n>> we write a new commit-graph by setting read_replace_refs to 0, thereby\n>> ignoring the replace refs.\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> builtin/commit-graph.c | 4 ++++\n>> commit-graph.c | 21 +++++++++++++++++++++\n>> replace-object.c | 2 +-\n>> replace-object.h | 2 ++\n>> t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++\n>> 5 files changed, 50 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> index 0bf0c48657..da737df321 100644\n>> --- a/builtin/commit-graph.c\n>> +++ b/builtin/commit-graph.c\n>> @@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)\n>> \treturn 0;\n>> }\n>> \n>> +extern int read_replace_refs;\n>> +\n> Why do you need this (and also in commit-graph.c)? I thought\n> cache.h includes it, which you can just make use of it.\n\n\nYou're right. I don't know how I missed that.\n\n\n>> +static int commit_graph_compatible(struct repository *r)\n>> +{\n>> +\tif (read_replace_refs) {\n>> +\t\tprepare_replace_object(r);\n>> +\t\tif (hashmap_get_size(&r->objects->replace_map->map))\n>> +\t\t\treturn 0;\n>> +\t}\n>> +\n>> +\treturn 1;\n>> +}\n>> diff --git a/replace-object.c b/replace-object.c\n>> index 3c17864eb7..9821f1477e 100644\n>> --- a/replace-object.c\n>> +++ b/replace-object.c\n>> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,\n>> \treturn 0;\n>> }\n>> \n>> -static void prepare_replace_object(struct repository *r)\n>> +void prepare_replace_object(struct repository *r)\n>> {\n>> \tif (r->objects->replace_map)\n>> \t\treturn;\n> The way the new caller is written, I am wondering if this function\n> should return \"we are (or, are not) using the replace object\n> feature\", making it unnecessary for callers on the reading side to\n> know about \"read-replace-refs\" external variable, for example.\n>\n> \t/*\n> \t * To be called on-demand from codepaths that want to make\n> \t * sure that replacement objects can be found as necessary.\n> \t *\n> \t * Return number of replacement defined for the repository, or\n> \t * -1 when !read_replace_refs tells us not to use replacement\n> \t * mechanism at all.\n> \t */\n> \tint prepare_replace_object(struct repository *r)\n> \t{\n> \t\tif (!read_replace_refs)\n> \t\t\treturn -1;\n>\n> \t\tif (!r->objects->replace_map) {\n> \t\t\tr->objects->replace_map =\n> \t\t\t\txmalloc(...);\n> \t\t\toidmap_init(r->objects->replace_map, 0);\n> \t\t\tfor_each_refplace_ref(r, register_...);\n> \t\t}\n> \t\treturn hashmap_get_size(&r->objects->replace_map->map);\n> \t}\n\nThis is a good idea. I can incorporate it into v3.\n\n> Then, the caller side can simply become something like:\n>\n> \t#define cgraph_compat(r) (prepare_replace_object(r) <= 0)\n\nWith the next two patches that add more conditions to \ncommit_graph_compatible(), I'd prefer keeping it a method instead of a \nmacro.\n\n> There are various writers to read_replace_refs variable, but I think\n> they should first be replaced with calls to something like:\n>\n> \tvoid use_replace_refs(struct repository *r, int enable);\n>\n> which allows us to hide the global variable and later make it per\n> repository if we wanted to.\n\nI will incorporate this into v3 as well.\n\nThanks,\n\n-Stolee\n\n"}]}