{"thread":{"id":"51831","subject":"[PATCH] upload-pack: disable commit graph more gently for shallow traversal","startedAt":"2019-09-12T00:04:17Z","lastAt":"2019-09-13T18:05:23Z","messageCount":25,"participants":["Jeff King","Taylor Blau","SZEDER Gábor","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"382191","messageId":"20190912000414.GA31334@sigill.intra.peff.net","threadId":"51831","inReplyTo":null,"subject":"[PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T00:04:15Z","receivedAt":"2019-09-12T00:04:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client has asked for certain shallow options like\n\"deepen-since\", we do a custom rev-list walk that pretends to be\nshallow. Before doing so, we have to disable the commit-graph, since it\nis not compatible with the shallow view of the repository. That's\nhandled by 829a321569 (commit-graph: close_commit_graph before shallow\nwalk, 2018-08-20). That commit literally closes and frees our\nrepo->objects->commit_graph struct.\n\nThat creates an interesting problem for commits that have _already_ been\nparsed using the commit graph. Their commit->object.parsed flag is set,\ntheir commit->graph_pos is set, but their commit->maybe_tree may still\nbe NULL. When somebody later calls repo_get_commit_tree(), we see that\nwe haven't loaded the tree oid yet and try to get it from the commit\ngraph. But since it has been freed, we segfault!\n\nSo the root of the issue is a data dependency between the commit's\nlazy-load of the tree oid and the fact that the commit graph can go\naway mid-process. How can we resolve it?\n\nThere are a couple of general approaches:\n\n  1. The obvious answer is to avoid loading the tree from the graph when\n     we see that it's NULL. But then what do we return for the tree oid?\n     If we return NULL, our caller in do_traverse() will rightly\n     complain that we have no tree. We'd have to fallback to loading the\n     actual commit object and re-parsing it. That requires teaching\n     parse_commit_buffer() to understand re-parsing (i.e., not starting\n     from a clean slate and not leaking any allocated bits like parent\n     list pointers).\n\n  2. When we close the commit graph, walk through the set of in-memory\n     objects and clear any graph_pos pointers. But this means we also\n     have to \"unparse\" any such commits so that we know they still need\n     to open the commit object to fill in their trees. So it's no less\n     complicated than (1), and is more expensive (since we clear objects\n     we might not later need).\n\n  3. Stop freeing the commit-graph struct. Continue to let it be used\n     for lazy-loads of tree oids, but let upload-pack specify that it\n     shouldn't be used for further commit parsing.\n\n  4. Push the whole shallow rev-list out to its own sub-process, with\n     the commit-graph disabled from the start, giving it a clean memory\n     space to work from.\n\nI've chosen (3) here. Options (1) and (2) would work, but are\nnon-trivial to implement. Option (4) is more expensive, and I'm not sure\nhow complicated it is (shelling out for the actual rev-list part is\neasy, but we do then parse the resulting commits internally, and I'm not\nclear which parts need to be handling shallow-ness).\n\nThe new test in t5500 triggers this segfault, but see the comments there\nfor how horribly intimate it has to be with how both upload-pack and\ncommit graphs work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c        | 10 ++++++++++\n commit-graph.h        |  6 ++++++\n t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n upload-pack.c         |  2 +-\n 4 files changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b02d2c426..bc5dd5913f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -41,6 +41,8 @@\n #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n \t\t\t+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)\n \n+static int commit_graph_disabled;\n+\n char *get_commit_graph_filename(const char *obj_dir)\n {\n \tchar *filename = xstrfmt(\"%s/info/commit-graph\", obj_dir);\n@@ -472,6 +474,9 @@ static int prepare_commit_graph(struct repository *r)\n \t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n \t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n \n+\tif (commit_graph_disabled)\n+\t\treturn 0;\n+\n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n@@ -2101,3 +2106,8 @@ void free_commit_graph(struct commit_graph *g)\n \tfree(g->filename);\n \tfree(g);\n }\n+\n+void disable_commit_graph(void)\n+{\n+\tcommit_graph_disabled = 1;\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 486e64e591..42d6e7c289 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -107,4 +107,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n void close_commit_graph(struct raw_object_store *);\n void free_commit_graph(struct commit_graph *);\n \n+/*\n+ * Disable further use of the commit graph in this process when parsing a\n+ * \"struct commit\".\n+ */\n+void disable_commit_graph(void);\n+\n #endif\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8210f63d41..7601664b74 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -783,6 +783,44 @@ test_expect_success 'clone shallow since selects no commits' '\n \t)\n '\n \n+# A few subtle things about the request in this test:\n+#\n+#  - the server must have commit-graphs present and enabled\n+#\n+#  - the history is such that our want/have share a common ancestor (\"base\"\n+#    here)\n+#\n+#  - we send only a single have, which is fewer than a normal client would\n+#    send. This ensures that we don't parse \"base\" up front with\n+#    parse_object(), but rather traverse to it as a parent while deciding if we\n+#    can stop the \"have\" negotiation, and call parse_commit(). The former\n+#    sees the actual object data and so always loads the three oid, whereas the\n+#    latter will try to lod it lazily.\n+#\n+#  - we must use protocol v2, because it handles the \"have\" negotiation before\n+#    processing the shallow direectives\n+#\n+test_expect_success 'shallow since with commit graph and already-seen commit' '\n+\ttest_create_repo shallow-since-graph &&\n+\t(\n+\tcd shallow-since-graph &&\n+\ttest_commit base &&\n+\ttest_commit master &&\n+\tgit checkout -b other HEAD^ &&\n+\ttest_commit other &&\n+\tgit commit-graph write --reachable &&\n+\tgit config core.commitGraph true &&\n+\n+\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n+\t0012command=fetch\n+\t00010013deepen-since 1\n+\t0032want $(git rev-parse other)\n+\t0032have $(git rev-parse master)\n+\t0000\n+\tEOF\n+\t)\n+'\n+\n test_expect_success 'shallow clone exclude tag two' '\n \ttest_create_repo shallow-exclude &&\n \t(\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 222cd3ad89..a260b6b50d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n {\n \tstruct commit_list *result;\n \n-\tclose_commit_graph(the_repository->objects);\n+\tdisable_commit_graph();\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-- \n2.23.0.663.gbe3d117559\n"},{"id":"382192","messageId":"20190912001846.GA31370@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912000414.GA31334@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T00:18:46Z","receivedAt":"2019-09-12T00:18:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2019 at 08:04:14PM -0400, Jeff King wrote:\n\n> When the client has asked for certain shallow options like\n> \"deepen-since\", we do a custom rev-list walk that pretends to be\n> shallow. Before doing so, we have to disable the commit-graph, since it\n> is not compatible with the shallow view of the repository. That's\n> handled by 829a321569 (commit-graph: close_commit_graph before shallow\n> walk, 2018-08-20). That commit literally closes and frees our\n> repo->objects->commit_graph struct.\n\nA few notes and curiosities on my patch and this general area.\n\nThe test suite passes with my patch both with and without\nGIT_TEST_COMMIT_GRAPH=1. But to my surprise, it also passes if I delete\nthe close_commit_graph() line added by 829a321569!\n\nSo it's not clear to me whether this whole thing is truly unnecessary\n(and Stolee was just being overly cautious because the code is related\nto shallow-ness, even though it is OK doing a true-parent traversal\nitself), or if we just don't have good test coverage for the case that\nrequires it.\n\nIf it _is_ necessary, I'm a little worried there are other problems\nlurking. The whole issue is that we've seen and parsed some commits\nbefore we get to this shallow deepen-since code-path. So just disabling\ncommit-graphs isn't enough. Even without them, we might have parsed some\ncommits the old-fashioned way and filled in their parent pointers. Is\nthat a problem?\n\nIf so, then I think we either have to \"unparse\" all of the existing\nin-memory commits, or call out to a rev-list process with a fresh memory\nspace.\n\nOne especially interesting bit is this:\n\n> +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> +#    processing the shallow direectives\n\nIn the v0 protocol, we handle shallows at the end of receive_needs(). So\nit happens before we call into get_common_commits(), which may do\nfurther traversal. That makes it much more likely for the shallow code\nto see a \"clean\" slate. Should v2 be ordering things in the same way?\nBut then would the have negotiation see the shallow parents? If so, is\nthat good or bad?\n\nI'm pretty ignorant of how the shallow bits of upload-pack are supposed\nto work.\n\n> That creates an interesting problem for commits that have _already_ been\n> parsed using the commit graph. Their commit->object.parsed flag is set,\n> their commit->graph_pos is set, but their commit->maybe_tree may still\n> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> we haven't loaded the tree oid yet and try to get it from the commit\n> graph. But since it has been freed, we segfault!\n\nI was surprised we ever called repo_get_commit_tree() at all, since\nwe're literally just traversing commits here. It looks like\nlist-objects.c is very happy to queue pending trees for each commit,\neven if we're just going to throw them away when we get to\nprocess_tree()! I wonder if could be checking revs->tree_objects here\nand saving ourselves some work.\n\n> The new test in t5500 triggers this segfault, but see the comments there\n> for how horribly intimate it has to be with how both upload-pack and\n> commit graphs work.\n\nThis bug was triggered by a real world case. I'm not entirely clear what\nthe client-side state was; I had to reverse engineer the whole thing out\nof the server-side coredump. The test here is my attempt to cut it down\nto a minimum. I don't like having to manually generate the upload-pack\ninput, but I had trouble finding a fetch command that would trigger it.\nAnd anyway, given how weirdly specific the requirements are for\ngenerating this case, it seemed sensible to me to keep the input close\nto the source of the problem.\n\n-Peff\n"},{"id":"382193","messageId":"20190912011137.GA23412@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912001846.GA31370@sigill.intra.peff.net","subject":"[PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T01:11:37Z","receivedAt":"2019-09-12T01:11:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2019 at 08:18:46PM -0400, Jeff King wrote:\n\n> > That creates an interesting problem for commits that have _already_ been\n> > parsed using the commit graph. Their commit->object.parsed flag is set,\n> > their commit->graph_pos is set, but their commit->maybe_tree may still\n> > be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> > we haven't loaded the tree oid yet and try to get it from the commit\n> > graph. But since it has been freed, we segfault!\n> \n> I was surprised we ever called repo_get_commit_tree() at all, since\n> we're literally just traversing commits here. It looks like\n> list-objects.c is very happy to queue pending trees for each commit,\n> even if we're just going to throw them away when we get to\n> process_tree()! I wonder if could be checking revs->tree_objects here\n> and saving ourselves some work.\n\nIndeed, this seems to help quite a bit in the commit-graph case. I think\nit's worth doing (and is independent of the other patch).\n\n-- >8 --\nSubject: list-objects: don't queue root trees unless revs->tree_objects is set\n\nWhen traverse_commit_list() processes each commit, it queues the\ncommit's root tree in the pending array. Then, after all commits are\nprocessed, it calls traverse_trees_and_blobs() to walk over the pending\nlist, calling process_tree() on each. But if revs->tree_objects is not\nset, process_tree() just exists immediately!\n\nWe can save ourselves some work by not even bothering to queue these\ntrees in the first place. There are a few subtle points to make:\n\n  - we also detect commits with a NULL tree pointer here. But this isn't\n    an interesting check for broken commits, since the lookup_tree()\n    we'd have done during commit parsing doesn't actually check that we\n    have the tree on disk. So we're not losing any robustness.\n\n  - besides queueing, we also set the NOT_USER_GIVEN flag on the tree\n    object. This is used by the traverse_commit_list_filtered() variant.\n    But if we're not exploring trees, then we won't actually care about\n    this flag, which is used only inside process_tree() code-paths.\n\n  - queueing trees eventually leads to us queueing blobs, too. But we\n    don't need to check revs->blob_objects here. Even in the current\n    code, we still wouldn't find those blobs, because we'd never open up\n    the tree objects to list their contents.\n\n  - the user-visible impact to the caller is minimal. The pending trees\n    are all cleared by the time the function returns anyway, by\n    traverse_trees_and_blobs(). We do call a show_commit() callback,\n    which technically could be looking at revs->pending during the\n    callback. But it seems like a rather unlikely thing to do (if you\n    want the tree of the current commit, then accessing the tree struct\n    member is a lot simpler).\n\nSo this should be safe to do. Let's look at the benefits:\n\n  [before]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      7.651 s ±  0.021 s    [User: 7.399 s, System: 0.252 s]\n    Range (min … max):    7.607 s …  7.683 s    10 runs\n\n  [after]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      7.593 s ±  0.023 s    [User: 7.329 s, System: 0.264 s]\n    Range (min … max):    7.565 s …  7.634 s    10 runs\n\nNot too impressive, but then we're really just avoiding sticking a\npointer into a growable array. But still, I'll take a free 0.75%\nspeedup.\n\nLet's try it after running \"git commit-graph write\":\n\n  [before]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      1.458 s ±  0.011 s    [User: 1.199 s, System: 0.259 s]\n    Range (min … max):    1.447 s …  1.481 s    10 runs\n\n  [after]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      1.126 s ±  0.023 s    [User: 896.5 ms, System: 229.0 ms]\n    Range (min … max):    1.106 s …  1.181 s    10 runs\n\nNow that's more like it. We saved over 22% of the total time. Part of\nthat is because the runtime is shorter overall, but the absolute\nimprovement is also much larger. What's going on?\n\nWhen we fill in a commit struct using the commit graph, we don't bother\nto set the tree pointer, and instead lazy-load it when somebody calls\nget_commit_tree(). So we're not only skipping the pointer write to the\npending queue, but we're skipping the lazy-load of the tree entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex b5651ddd5b..c837bcaca8 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -370,7 +370,9 @@ static void do_traverse(struct traversal_context *ctx)\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\n \t\t */\n-\t\tif (get_commit_tree(commit)) {\n+\t\tif (!ctx->revs->tree_objects)\n+\t\t\t; /* do not bother loading tree */\n+\t\telse if (get_commit_tree(commit)) {\n \t\t\tstruct tree *tree = get_commit_tree(commit);\n \t\t\ttree->object.flags |= NOT_USER_GIVEN;\n \t\t\tadd_pending_tree(ctx->revs, tree);\n-- \n2.23.0.663.gbe3d117559\n\n"},{"id":"382194","messageId":"20190912011952.GA23637@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912011137.GA23412@sigill.intra.peff.net","subject":"Re: [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T01:19:52Z","receivedAt":"2019-09-12T01:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2019 at 09:11:37PM -0400, Jeff King wrote:\n\n> Let's try it after running \"git commit-graph write\":\n> \n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.458 s ±  0.011 s    [User: 1.199 s, System: 0.259 s]\n>     Range (min … max):    1.447 s …  1.481 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.126 s ±  0.023 s    [User: 896.5 ms, System: 229.0 ms]\n>     Range (min … max):    1.106 s …  1.181 s    10 runs\n> \n> Now that's more like it. We saved over 22% of the total time. Part of\n> that is because the runtime is shorter overall, but the absolute\n> improvement is also much larger. What's going on?\n\nAnother thing I noticed is that rev-list line-buffers when we're writing\nto /dev/null. This is actually the doing of glibc's stdio, as it\nconsider the character device special enough to turn off full buffering\n(we also do our own manual flush after each commit).\n\nI think it's probably a fairer test to time it that way (quite often\nyou'd be writing to a pipe, which would have the same behavior). But our\nimprovement is even better as a percentage when writing to a file:\n\n  [before]\n  Benchmark #1: git -C linux rev-list HEAD >file\n  Time (mean ± σ):      1.046 s ±  0.017 s    [User: 922.7 ms, System: 104.3 ms]\n  Range (min … max):    1.031 s …  1.087 s    10 runs\n\n  [after]\n  Benchmark #1: git -C linux rev-list HEAD >file\n  Time (mean ± σ):     741.4 ms ±  14.1 ms    [User: 644.8 ms, System: 75.9 ms]\n  Range (min … max):   721.2 ms … 766.8 ms    10 runs\n\nThat's a 29% improvement instead of 22% (and shows that write() syscalls\nare wasting close to 30% of our runtime, a well).\n\nI wonder if it would be worth teaching rev-list a --buffer option. Or\njust kicking it in automatically when we're just printing single oids.\nOnce upon a time the single-record flushing was useful for:\n\n  git rev-list HEAD -- <pathspec> | git diff-tree ...\n\nto feed incremental results as soon as we have them (imagine we see one\ncommit which touches the pathspec, then go through 100,000 that don't).\nBut these days \"git log\" does that at all internally (and typically\noutputs quite a bit more between each flush, though one could argue that\n\"log --oneline\" might want the same behavior).\n\nI dunno. Maybe it's not worth micro-optimizing too hard, but I was\nsurprised how big a difference it made.\n\n-Peff\n"},{"id":"382195","messageId":"20190912020748.GA76228@syl.lan","threadId":"51831","inReplyTo":"20190912000414.GA31334@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-12T02:07:48Z","receivedAt":"2019-09-12T02:07:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nThanks in advance for digging into all of this. I feel guilty for having\nfound the issue myself, and then gotten a headache for just long enough\nto have you completely fix the issue by the time that I got back. So,\nthanks :-).\n\nOn Wed, Sep 11, 2019 at 08:04:15PM -0400, Jeff King wrote:\n> When the client has asked for certain shallow options like\n> \"deepen-since\", we do a custom rev-list walk that pretends to be\n> shallow. Before doing so, we have to disable the commit-graph, since it\n> is not compatible with the shallow view of the repository. That's\n> handled by 829a321569 (commit-graph: close_commit_graph before shallow\n> walk, 2018-08-20). That commit literally closes and frees our\n> repo->objects->commit_graph struct.\n>\n> That creates an interesting problem for commits that have _already_ been\n> parsed using the commit graph. Their commit->object.parsed flag is set,\n> their commit->graph_pos is set, but their commit->maybe_tree may still\n> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> we haven't loaded the tree oid yet and try to get it from the commit\n> graph. But since it has been freed, we segfault!\n>\n> So the root of the issue is a data dependency between the commit's\n> lazy-load of the tree oid and the fact that the commit graph can go\n> away mid-process. How can we resolve it?\n>\n> There are a couple of general approaches:\n>\n>   1. The obvious answer is to avoid loading the tree from the graph when\n>      we see that it's NULL. But then what do we return for the tree oid?\n>      If we return NULL, our caller in do_traverse() will rightly\n>      complain that we have no tree. We'd have to fallback to loading the\n>      actual commit object and re-parsing it. That requires teaching\n>      parse_commit_buffer() to understand re-parsing (i.e., not starting\n>      from a clean slate and not leaking any allocated bits like parent\n>      list pointers).\n>\n>   2. When we close the commit graph, walk through the set of in-memory\n>      objects and clear any graph_pos pointers. But this means we also\n>      have to \"unparse\" any such commits so that we know they still need\n>      to open the commit object to fill in their trees. So it's no less\n>      complicated than (1), and is more expensive (since we clear objects\n>      we might not later need).\n>\n>   3. Stop freeing the commit-graph struct. Continue to let it be used\n>      for lazy-loads of tree oids, but let upload-pack specify that it\n>      shouldn't be used for further commit parsing.\n>\n>   4. Push the whole shallow rev-list out to its own sub-process, with\n>      the commit-graph disabled from the start, giving it a clean memory\n>      space to work from.\n>\n> I've chosen (3) here. Options (1) and (2) would work, but are\n> non-trivial to implement. Option (4) is more expensive, and I'm not sure\n> how complicated it is (shelling out for the actual rev-list part is\n> easy, but we do then parse the resulting commits internally, and I'm not\n> clear which parts need to be handling shallow-ness).\n\nOption (3) make the most sense to me, too. When I started hacking on a\nsimilar patch, I implemented roughly what you describe in (1), which is\nto say that I added an extra:\n\n  && r->objects->commit_graph\n\nTo ensure that the 'commit_graph' about to cause a segfault was\nnon-NULL after calling 'close_commit_graph'. But, as you pointed out:\nwhat are we to return after that? NULL, since we expected it to be in\nthe commit-graph, but we don't have a commit-graph to check anymore? I\ndon't think that that makes much sense, and what you implemented here is\nclearly better.\n\n> The new test in t5500 triggers this segfault, but see the comments there\n> for how horribly intimate it has to be with how both upload-pack and\n> commit graphs work.\n\nThanks for the comment, too. I agree that protocol-level hacking is\nsomewhat smelly, at least as far as t5500 is concerned, but for this\nparticular case it seems the only sensible option.\n\nI'm still left scratching my (sore) head about how someone triggered\nthis in production, but maybe that's a riddle for another day.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  commit-graph.c        | 10 ++++++++++\n>  commit-graph.h        |  6 ++++++\n>  t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  upload-pack.c         |  2 +-\n>  4 files changed, 55 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 9b02d2c426..bc5dd5913f 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -41,6 +41,8 @@\n>  #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n>  \t\t\t+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)\n>\n> +static int commit_graph_disabled;\n> +\n>  char *get_commit_graph_filename(const char *obj_dir)\n>  {\n>  \tchar *filename = xstrfmt(\"%s/info/commit-graph\", obj_dir);\n> @@ -472,6 +474,9 @@ static int prepare_commit_graph(struct repository *r)\n>  \t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n>  \t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n>\n> +\tif (commit_graph_disabled)\n> +\t\treturn 0;\n> +\n>  \tif (r->objects->commit_graph_attempted)\n>  \t\treturn !!r->objects->commit_graph;\n>  \tr->objects->commit_graph_attempted = 1;\n> @@ -2101,3 +2106,8 @@ void free_commit_graph(struct commit_graph *g)\n>  \tfree(g->filename);\n>  \tfree(g);\n>  }\n> +\n> +void disable_commit_graph(void)\n> +{\n> +\tcommit_graph_disabled = 1;\n> +}\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 486e64e591..42d6e7c289 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -107,4 +107,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n>  void close_commit_graph(struct raw_object_store *);\n>  void free_commit_graph(struct commit_graph *);\n>\n> +/*\n> + * Disable further use of the commit graph in this process when parsing a\n> + * \"struct commit\".\n> + */\n> +void disable_commit_graph(void);\n> +\n>  #endif\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 8210f63d41..7601664b74 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -783,6 +783,44 @@ test_expect_success 'clone shallow since selects no commits' '\n>  \t)\n>  '\n>\n> +# A few subtle things about the request in this test:\n> +#\n> +#  - the server must have commit-graphs present and enabled\n\nI think \"enabled\" may be somewhat redundant, at least since some recent\nchanges to enable this by default. (As an aside, I tried to dig up the\npatches that Stolee sent to actually enable this and back up my claim,\nbut I couldn't find them on 'master'. I'm not sure if that's my poor use\nof 'git log', or misremembering the fact that these were enabled by\ndefault.)\n\n> +#\n> +#  - the history is such that our want/have share a common ancestor (\"base\"\n> +#    here)\n> +#\n> +#  - we send only a single have, which is fewer than a normal client would\n> +#    send. This ensures that we don't parse \"base\" up front with\n> +#    parse_object(), but rather traverse to it as a parent while deciding if we\n> +#    can stop the \"have\" negotiation, and call parse_commit(). The former\n> +#    sees the actual object data and so always loads the three oid, whereas the\n> +#    latter will try to lod it lazily.\n\ns/lod/load\n\n> +#\n> +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> +#    processing the shallow direectives\n> +#\n> +test_expect_success 'shallow since with commit graph and already-seen commit' '\n> +\ttest_create_repo shallow-since-graph &&\n> +\t(\n\nI'm not sure if this same-level indentation is common, or you're missing\nan extra tab here. Either way.\n\n> +\tcd shallow-since-graph &&\n> +\ttest_commit base &&\n> +\ttest_commit master &&\n> +\tgit checkout -b other HEAD^ &&\n> +\ttest_commit other &&\n> +\tgit commit-graph write --reachable &&\n> +\tgit config core.commitGraph true &&\n\nAnother nit, but do you have any thoughts about using 'test_config' here\ninstead of a pure 'git config'? I don't think that it really would\nmatter much (since none of the other tests hopefully have anything to do\nwith commit-graph, and doubly so if it is enabled by default, _and_\nsince you're using your own repository), but anyway.\n> +\n> +\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> +\t0012command=fetch\n> +\t00010013deepen-since 1\n> +\t0032want $(git rev-parse other)\n> +\t0032have $(git rev-parse master)\n> +\t0000\n> +\tEOF\n> +\t)\n> +'\n> +\n\nAside from my nit-picking above, the protocol exchange here looks\ncorrect, and your rationale backs it up. Likewise, this causes a\nsegfault for me on the tip of 'master', and applying the later part of\nyour patch fixes it for me. Thanks.\n\n>  test_expect_success 'shallow clone exclude tag two' '\n>  \ttest_create_repo shallow-exclude &&\n>  \t(\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 222cd3ad89..a260b6b50d 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n>  {\n>  \tstruct commit_list *result;\n>\n> -\tclose_commit_graph(the_repository->objects);\n> +\tdisable_commit_graph();\n\nThis line made me think to check if we could remove 'close_commit_graph'\nall together, but there is a remaining callsite in packfile.c, and\nclosing the commit-graph _is_ the right thing to do there, so I think we\nought to keep it around.\n\n>  \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n>  \tsend_shallow(writer, result);\n>  \tfree_commit_list(result);\n> --\n> 2.23.0.663.gbe3d117559\n\nThanks,\nTaylor\n"},{"id":"382196","messageId":"20190912020848.GB76228@syl.lan","threadId":"51831","inReplyTo":"20190912001846.GA31370@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-12T02:08:48Z","receivedAt":"2019-09-12T02:08:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 11, 2019 at 08:18:46PM -0400, Jeff King wrote:\n> On Wed, Sep 11, 2019 at 08:04:14PM -0400, Jeff King wrote:\n>\n> > When the client has asked for certain shallow options like\n> > \"deepen-since\", we do a custom rev-list walk that pretends to be\n> > shallow. Before doing so, we have to disable the commit-graph, since it\n> > is not compatible with the shallow view of the repository. That's\n> > handled by 829a321569 (commit-graph: close_commit_graph before shallow\n> > walk, 2018-08-20). That commit literally closes and frees our\n> > repo->objects->commit_graph struct.\n>\n> A few notes and curiosities on my patch and this general area.\n>\n> The test suite passes with my patch both with and without\n> GIT_TEST_COMMIT_GRAPH=1. But to my surprise, it also passes if I delete\n> the close_commit_graph() line added by 829a321569!\n>\n> So it's not clear to me whether this whole thing is truly unnecessary\n> (and Stolee was just being overly cautious because the code is related\n> to shallow-ness, even though it is OK doing a true-parent traversal\n> itself), or if we just don't have good test coverage for the case that\n> requires it.\n>\n> If it _is_ necessary, I'm a little worried there are other problems\n> lurking. The whole issue is that we've seen and parsed some commits\n> before we get to this shallow deepen-since code-path. So just disabling\n> commit-graphs isn't enough. Even without them, we might have parsed some\n> commits the old-fashioned way and filled in their parent pointers. Is\n> that a problem?\n\nI am, too, but I don't think we should hold this patch up which is\nobviously improving the situation in the meantime while we figure that\nout.\n\n> [snip]\n\nThanks,\nTaylor\n"},{"id":"382207","messageId":"20190912110620.GA3729@szeder.dev","threadId":"51831","inReplyTo":"20190912020748.GA76228@syl.lan","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-12T11:06:20Z","receivedAt":"2019-09-12T11:06:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Sep 11, 2019 at 10:07:48PM -0400, Taylor Blau wrote:\n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > index 8210f63d41..7601664b74 100755\n> > --- a/t/t5500-fetch-pack.sh\n> > +++ b/t/t5500-fetch-pack.sh\n\n> > +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> > +#    processing the shallow direectives\n\ns/ee/e/\n\n> > +#\n> > +test_expect_success 'shallow since with commit graph and already-seen commit' '\n> > +\ttest_create_repo shallow-since-graph &&\n> > +\t(\n> \n> I'm not sure if this same-level indentation is common, or you're missing\n> an extra tab here. Either way.\n> \n> > +\tcd shallow-since-graph &&\n> > +\ttest_commit base &&\n> > +\ttest_commit master &&\n> > +\tgit checkout -b other HEAD^ &&\n> > +\ttest_commit other &&\n> > +\tgit commit-graph write --reachable &&\n> > +\tgit config core.commitGraph true &&\n> \n> Another nit, but do you have any thoughts about using 'test_config' here\n> instead of a pure 'git config'? I don't think that it really would\n> matter much (since none of the other tests hopefully have anything to do\n> with commit-graph, and doubly so if it is enabled by default, _and_\n> since you're using your own repository), but anyway.\n\nWe can't simply replace that 'git config' command with 'test_config',\nbecause it is implemented using 'test_when_finished', which doesn't\nwork in a subshell [1].  What we could do is:\n\n  test_create_repo shallow-since-graph &&\n  test_config -C shallow-since-graph core.commitGraph true &&\n  (\n     cd shallow-since-graph &&\n     ....\n\nOr we could entirely avoid the subshell by passing '-C\nshallow-since-graph' to every single command... [2]\n\nHowever, since this repo was specifically created for this test, it\ndoesn't really matter in what state it's left behind, so I don't think\nit's worth it.\n\n\n[1] 0968f12a99 (test-lib-functions: detect test_when_finished in\n    subshell, 2015-09-05)\n[2] Or bite the bullet, and declare that every test case shall start\n    in $TRASH_DIRECTORY.\n\n"},{"id":"382212","messageId":"fdb81ce0-20ee-5880-2a6c-0c8b3f1739b9@gmail.com","threadId":"51831","inReplyTo":"20190912000414.GA31334@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-12T12:23:49Z","receivedAt":"2019-09-12T12:23:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2019 8:04 PM, Jeff King wrote:\n> When the client has asked for certain shallow options like\n> \"deepen-since\", we do a custom rev-list walk that pretends to be\n> shallow. Before doing so, we have to disable the commit-graph, since it\n> is not compatible with the shallow view of the repository. That's\n> handled by 829a321569 (commit-graph: close_commit_graph before shallow\n> walk, 2018-08-20). That commit literally closes and frees our\n> repo->objects->commit_graph struct.\n> \n> That creates an interesting problem for commits that have _already_ been\n> parsed using the commit graph. Their commit->object.parsed flag is set,\n> their commit->graph_pos is set, but their commit->maybe_tree may still\n> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> we haven't loaded the tree oid yet and try to get it from the commit\n> graph. But since it has been freed, we segfault!\n\nOOPS! That is certainly a bad thing. I'm glad you found it, but I\nam sorry for how you (probably) found it.\n\n> So the root of the issue is a data dependency between the commit's\n> lazy-load of the tree oid and the fact that the commit graph can go\n> away mid-process. How can we resolve it?\n> \n> There are a couple of general approaches:\n> \n>   1. The obvious answer is to avoid loading the tree from the graph when\n>      we see that it's NULL. But then what do we return for the tree oid?\n>      If we return NULL, our caller in do_traverse() will rightly\n>      complain that we have no tree. We'd have to fallback to loading the\n>      actual commit object and re-parsing it. That requires teaching\n>      parse_commit_buffer() to understand re-parsing (i.e., not starting\n>      from a clean slate and not leaking any allocated bits like parent\n>      list pointers).\n> \n>   2. When we close the commit graph, walk through the set of in-memory\n>      objects and clear any graph_pos pointers. But this means we also\n>      have to \"unparse\" any such commits so that we know they still need\n>      to open the commit object to fill in their trees. So it's no less\n>      complicated than (1), and is more expensive (since we clear objects\n>      we might not later need).\n> \n>   3. Stop freeing the commit-graph struct. Continue to let it be used\n>      for lazy-loads of tree oids, but let upload-pack specify that it\n>      shouldn't be used for further commit parsing.\n> \n>   4. Push the whole shallow rev-list out to its own sub-process, with\n>      the commit-graph disabled from the start, giving it a clean memory\n>      space to work from.\n> \n> I've chosen (3) here. Options (1) and (2) would work, but are\n> non-trivial to implement. Option (4) is more expensive, and I'm not sure\n> how complicated it is (shelling out for the actual rev-list part is\n> easy, but we do then parse the resulting commits internally, and I'm not\n> clear which parts need to be handling shallow-ness).\n\nI agree that (3) is the best option. The commit-graph is just a database\nof commit data, and we are choosing to interpret the parent data differently.\nThe tree data is perfectly good.\n\n> The new test in t5500 triggers this segfault, but see the comments there\n> for how horribly intimate it has to be with how both upload-pack and\n> commit graphs work.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  commit-graph.c        | 10 ++++++++++\n>  commit-graph.h        |  6 ++++++\n>  t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  upload-pack.c         |  2 +-\n>  4 files changed, 55 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 9b02d2c426..bc5dd5913f 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -41,6 +41,8 @@\n>  #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n>  \t\t\t+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)\n>  \n> +static int commit_graph_disabled;\n\nShould we be putting this inside the repository struct instead?\n\n> +\n>  char *get_commit_graph_filename(const char *obj_dir)\n>  {\n>  \tchar *filename = xstrfmt(\"%s/info/commit-graph\", obj_dir);\n> @@ -472,6 +474,9 @@ static int prepare_commit_graph(struct repository *r)\n>  \t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n>  \t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n>  \n> +\tif (commit_graph_disabled)\n> +\t\treturn 0;\n> +\n>  \tif (r->objects->commit_graph_attempted)\n>  \t\treturn !!r->objects->commit_graph;\n>  \tr->objects->commit_graph_attempted = 1;\n> @@ -2101,3 +2106,8 @@ void free_commit_graph(struct commit_graph *g)\n>  \tfree(g->filename);\n>  \tfree(g);\n>  }\n> +\n> +void disable_commit_graph(void)\n> +{\n> +\tcommit_graph_disabled = 1;\n> +}\n\nThen this would take a struct repository *r.\n\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 486e64e591..42d6e7c289 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -107,4 +107,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n>  void close_commit_graph(struct raw_object_store *);\n>  void free_commit_graph(struct commit_graph *);\n>  \n> +/*\n> + * Disable further use of the commit graph in this process when parsing a\n> + * \"struct commit\".\n> + */\n> +void disable_commit_graph(void);\n> +\n>  #endif\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 8210f63d41..7601664b74 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -783,6 +783,44 @@ test_expect_success 'clone shallow since selects no commits' '\n>  \t)\n>  '\n>  \n> +# A few subtle things about the request in this test:\n> +#\n> +#  - the server must have commit-graphs present and enabled\n> +#\n> +#  - the history is such that our want/have share a common ancestor (\"base\"\n> +#    here)\n> +#\n> +#  - we send only a single have, which is fewer than a normal client would\n> +#    send. This ensures that we don't parse \"base\" up front with\n> +#    parse_object(), but rather traverse to it as a parent while deciding if we\n> +#    can stop the \"have\" negotiation, and call parse_commit(). The former\n> +#    sees the actual object data and so always loads the three oid, whereas the\n> +#    latter will try to lod it lazily.\n> +#\n> +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> +#    processing the shallow direectives\n> +#\n> +test_expect_success 'shallow since with commit graph and already-seen commit' '\n> +\ttest_create_repo shallow-since-graph &&\n> +\t(\n> +\tcd shallow-since-graph &&\n> +\ttest_commit base &&\n> +\ttest_commit master &&\n> +\tgit checkout -b other HEAD^ &&\n> +\ttest_commit other &&\n> +\tgit commit-graph write --reachable &&\n> +\tgit config core.commitGraph true &&\n> +\n> +\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n> +\t0012command=fetch\n> +\t00010013deepen-since 1\n> +\t0032want $(git rev-parse other)\n> +\t0032have $(git rev-parse master)\n> +\t0000\n> +\tEOF\n> +\t)\n> +'\n> +\n>  test_expect_success 'shallow clone exclude tag two' '\n>  \ttest_create_repo shallow-exclude &&\n>  \t(\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 222cd3ad89..a260b6b50d 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n>  {\n>  \tstruct commit_list *result;\n>  \n> -\tclose_commit_graph(the_repository->objects);\n> +\tdisable_commit_graph();\n>  \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n>  \tsend_shallow(writer, result);\n>  \tfree_commit_list(result);\n> \n\nYour patch does not seem to actually cover the \"I've already parsed some commits\"\ncase, as you are only preventing the commit-graph from being prepared. Instead,\nwe need to have a short-circuit inside parse_commit() to avoid future parsing\nfrom the commit-graph file.\n\n(I'm going to the rest of the thread messages now to see if I just repeated\nsomeone else's concerns.)\n\nThanks,\n-Stolee\n"},{"id":"382213","messageId":"b2a4e32c-eb0b-d4b1-a44d-6587a6a77560@gmail.com","threadId":"51831","inReplyTo":"20190912011137.GA23412@sigill.intra.peff.net","subject":"Re: [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-12T12:31:07Z","receivedAt":"2019-09-12T12:31:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2019 9:11 PM, Jeff King wrote:\n> On Wed, Sep 11, 2019 at 08:18:46PM -0400, Jeff King wrote:\n> \n>>> That creates an interesting problem for commits that have _already_ been\n>>> parsed using the commit graph. Their commit->object.parsed flag is set,\n>>> their commit->graph_pos is set, but their commit->maybe_tree may still\n>>> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n>>> we haven't loaded the tree oid yet and try to get it from the commit\n>>> graph. But since it has been freed, we segfault!\n>>\n>> I was surprised we ever called repo_get_commit_tree() at all, since\n>> we're literally just traversing commits here. It looks like\n>> list-objects.c is very happy to queue pending trees for each commit,\n>> even if we're just going to throw them away when we get to\n>> process_tree()! I wonder if could be checking revs->tree_objects here\n>> and saving ourselves some work.\n> \n> Indeed, this seems to help quite a bit in the commit-graph case. I think\n> it's worth doing (and is independent of the other patch).\n\nGood find!\n\n> -- >8 --\n> Subject: list-objects: don't queue root trees unless revs->tree_objects is set\n> \n> When traverse_commit_list() processes each commit, it queues the\n> commit's root tree in the pending array. Then, after all commits are\n> processed, it calls traverse_trees_and_blobs() to walk over the pending\n> list, calling process_tree() on each. But if revs->tree_objects is not\n> set, process_tree() just exists immediately!\n> \n> We can save ourselves some work by not even bothering to queue these\n> trees in the first place. There are a few subtle points to make:\n> \n>   - we also detect commits with a NULL tree pointer here. But this isn't\n>     an interesting check for broken commits, since the lookup_tree()\n>     we'd have done during commit parsing doesn't actually check that we\n>     have the tree on disk. So we're not losing any robustness.\n> \n>   - besides queueing, we also set the NOT_USER_GIVEN flag on the tree\n>     object. This is used by the traverse_commit_list_filtered() variant.\n>     But if we're not exploring trees, then we won't actually care about\n>     this flag, which is used only inside process_tree() code-paths.\n> \n>   - queueing trees eventually leads to us queueing blobs, too. But we\n>     don't need to check revs->blob_objects here. Even in the current\n>     code, we still wouldn't find those blobs, because we'd never open up\n>     the tree objects to list their contents.\n> \n>   - the user-visible impact to the caller is minimal. The pending trees\n>     are all cleared by the time the function returns anyway, by\n>     traverse_trees_and_blobs(). We do call a show_commit() callback,\n>     which technically could be looking at revs->pending during the\n>     callback. But it seems like a rather unlikely thing to do (if you\n>     want the tree of the current commit, then accessing the tree struct\n>     member is a lot simpler).\n\nThese all look reasonable. We shouldn't need to do any of that any more.\n\n> So this should be safe to do. Let's look at the benefits:\n> \n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      7.651 s ±  0.021 s    [User: 7.399 s, System: 0.252 s]\n>     Range (min … max):    7.607 s …  7.683 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      7.593 s ±  0.023 s    [User: 7.329 s, System: 0.264 s]\n>     Range (min … max):    7.565 s …  7.634 s    10 runs\n> \n> Not too impressive, but then we're really just avoiding sticking a\n> pointer into a growable array. But still, I'll take a free 0.75%\n> speedup.\n> \n> Let's try it after running \"git commit-graph write\":\n> \n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.458 s ±  0.011 s    [User: 1.199 s, System: 0.259 s]\n>     Range (min … max):    1.447 s …  1.481 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.126 s ±  0.023 s    [User: 896.5 ms, System: 229.0 ms]\n>     Range (min … max):    1.106 s …  1.181 s    10 runs\n> \n> Now that's more like it. We saved over 22% of the total time. Part of\n> that is because the runtime is shorter overall, but the absolute\n> improvement is also much larger. What's going on?\n\nVery cool.\n\n> When we fill in a commit struct using the commit graph, we don't bother\n> to set the tree pointer, and instead lazy-load it when somebody calls\n> get_commit_tree(). So we're not only skipping the pointer write to the\n> pending queue, but we're skipping the lazy-load of the tree entirely.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  list-objects.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/list-objects.c b/list-objects.c\n> index b5651ddd5b..c837bcaca8 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -370,7 +370,9 @@ static void do_traverse(struct traversal_context *ctx)\n>  \t\t * an uninteresting boundary commit may not have its tree\n>  \t\t * parsed yet, but we are not going to show them anyway\n>  \t\t */\n> -\t\tif (get_commit_tree(commit)) {\n> +\t\tif (!ctx->revs->tree_objects)\n> +\t\t\t; /* do not bother loading tree */\n> +\t\telse if (get_commit_tree(commit)) {\n>  \t\t\tstruct tree *tree = get_commit_tree(commit);\n>  \t\t\ttree->object.flags |= NOT_USER_GIVEN;\n>  \t\t\tadd_pending_tree(ctx->revs, tree);\n\nAnd a simple code fix. LGTM.\n\nThanks!\n-Stolee\n"},{"id":"382214","messageId":"6eb22830-a887-8bbe-412e-7ddb531d196a@gmail.com","threadId":"51831","inReplyTo":"20190912020748.GA76228@syl.lan","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-12T12:46:57Z","receivedAt":"2019-09-12T12:47:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2019 10:07 PM, Taylor Blau wrote:>>\n>> +# A few subtle things about the request in this test:\n>> +#\n>> +#  - the server must have commit-graphs present and enabled\n> \n> I think \"enabled\" may be somewhat redundant, at least since some recent\n> changes to enable this by default. (As an aside, I tried to dig up the\n> patches that Stolee sent to actually enable this and back up my claim,\n> but I couldn't find them on 'master'. I'm not sure if that's my poor use\n> of 'git log', or misremembering the fact that these were enabled by\n> default.)\n\nCommit 31b1de6 (\"commit-graph: turn on commit-graph by default\" 2019-08-13)\nis part of ds/feature-macros and seems to be in master (at least in gitster/git).\n\nHaving core.commitGraph=true does very little if the commit-graph is not\nwritten, but gc.writeCommitGraph is also enabled by default in that commit.\n\nThanks,\n-Stolee\n"},{"id":"382215","messageId":"20190912135952.GB23031@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912020748.GA76228@syl.lan","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T13:59:52Z","receivedAt":"2019-09-12T13:59:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2019 at 10:07:48PM -0400, Taylor Blau wrote:\n\n> > The new test in t5500 triggers this segfault, but see the comments there\n> > for how horribly intimate it has to be with how both upload-pack and\n> > commit graphs work.\n> \n> Thanks for the comment, too. I agree that protocol-level hacking is\n> somewhat smelly, at least as far as t5500 is concerned, but for this\n> particular case it seems the only sensible option.\n> \n> I'm still left scratching my (sore) head about how someone triggered\n> this in production, but maybe that's a riddle for another day.\n\nI'll admit that part of my motivation was my inability to generate a\nworking case just using Git commands (so my justification is that if\nit's so hard to do, then the test is inherently fragile, but you can\nalso just accuse me of being lazy).\n\nI think the key is something to do with the shape of history related to\nthe requests, such that we walk over a commit during the have\nnegotiation, but then also need to walk over it during the deepen-since.\nSo maybe I am just missing something obvious, or maybe it just needs to\nbe a deeper history. I do like that the case I showed is the minimal\nhistory, at least.\n\n> > +# A few subtle things about the request in this test:\n> > +#\n> > +#  - the server must have commit-graphs present and enabled\n> \n> I think \"enabled\" may be somewhat redundant, at least since some recent\n> changes to enable this by default. (As an aside, I tried to dig up the\n> patches that Stolee sent to actually enable this and back up my claim,\n> but I couldn't find them on 'master'. I'm not sure if that's my poor use\n> of 'git log', or misremembering the fact that these were enabled by\n> default.)\n\nYeah, it is redundant these days. I figured this might go to 'maint',\nthough, and 31b1de6a09 (commit-graph: turn on commit-graph by default,\n2019-08-13) is only in master.\n\n> > +#    latter will try to lod it lazily.\n> \n> s/lod/load\n\nThanks, fixed.\n\n> > +#\n> > +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> > +#    processing the shallow direectives\n> > +#\n> > +test_expect_success 'shallow since with commit graph and already-seen commit' '\n> > +\ttest_create_repo shallow-since-graph &&\n> > +\t(\n> \n> I'm not sure if this same-level indentation is common, or you're missing\n> an extra tab here. Either way.\n\nIt's not common, but I was matching the surrounding tests for style (and\nuse of a separate repo, and non-use of test_config).\n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 222cd3ad89..a260b6b50d 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n> >  {\n> >  \tstruct commit_list *result;\n> >\n> > -\tclose_commit_graph(the_repository->objects);\n> > +\tdisable_commit_graph();\n> \n> This line made me think to check if we could remove 'close_commit_graph'\n> all together, but there is a remaining callsite in packfile.c, and\n> closing the commit-graph _is_ the right thing to do there, so I think we\n> ought to keep it around.\n\nYeah, it's sort of inherently dangerous, as it doesn't scrub any\nin-memory commit structs that are in this \"have a graph_pos but not\nmaybe_tree\" state.\n\nThe call in close_object_store() is probably fine, though, as the whole\npoint there is getting rid of everything related to the object store.\nAnd since 14ba97f81c (alloc: allow arbitrary repositories for alloc\nfunctions, 2018-05-15) or thereabouts, that includes the object structs.\n\nThere's another call in write_commit_graph_file(), right before we\nrename a new file into place. This generally happens in a separate\n\"commit-graph write\" process, so I think it's OK.  But it could\neventually happen as part of another process, which would maybe be a\nproblem. I'm actually not convinced the in-memory one needs to be closed\nhere, but maybe it's a Windows thing (closing the file before renaming\nover it)?\n\n-Peff\n"},{"id":"382216","messageId":"20190912140144.GC23031@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912110620.GA3729@szeder.dev","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:01:45Z","receivedAt":"2019-09-12T14:01:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2019 at 01:06:20PM +0200, SZEDER Gábor wrote:\n\n> > > +#  - we must use protocol v2, because it handles the \"have\" negotiation before\n> > > +#    processing the shallow direectives\n> \n> s/ee/e/\n\nThanks, fixed.\n\n> We can't simply replace that 'git config' command with 'test_config',\n> because it is implemented using 'test_when_finished', which doesn't\n> work in a subshell [1].  What we could do is:\n> \n>   test_create_repo shallow-since-graph &&\n>   test_config -C shallow-since-graph core.commitGraph true &&\n>   (\n>      cd shallow-since-graph &&\n>      ....\n> \n> Or we could entirely avoid the subshell by passing '-C\n> shallow-since-graph' to every single command... [2]\n> \n> However, since this repo was specifically created for this test, it\n> doesn't really matter in what state it's left behind, so I don't think\n> it's worth it.\n\nYep, agreed on all of this. :)\n\n-Peff\n"},{"id":"382217","messageId":"20190912140332.GD23031@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912020848.GB76228@syl.lan","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:03:33Z","receivedAt":"2019-09-12T14:03:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2019 at 10:08:48PM -0400, Taylor Blau wrote:\n\n> > The test suite passes with my patch both with and without\n> > GIT_TEST_COMMIT_GRAPH=1. But to my surprise, it also passes if I delete\n> > the close_commit_graph() line added by 829a321569!\n> >\n> > So it's not clear to me whether this whole thing is truly unnecessary\n> > (and Stolee was just being overly cautious because the code is related\n> > to shallow-ness, even though it is OK doing a true-parent traversal\n> > itself), or if we just don't have good test coverage for the case that\n> > requires it.\n> >\n> > If it _is_ necessary, I'm a little worried there are other problems\n> > lurking. The whole issue is that we've seen and parsed some commits\n> > before we get to this shallow deepen-since code-path. So just disabling\n> > commit-graphs isn't enough. Even without them, we might have parsed some\n> > commits the old-fashioned way and filled in their parent pointers. Is\n> > that a problem?\n> \n> I am, too, but I don't think we should hold this patch up which is\n> obviously improving the situation in the meantime while we figure that\n> out.\n\nYeah, I'd agree here, unless we determine quickly that we do need the\nbigger fix, and somebody with a bit more knowledge of this shallow code\noffers a fix. I believe my patch is a strict improvement, and puts the\ncommit-graph code path on par with the regular one.\n\n-Peff\n"},{"id":"382219","messageId":"20190912142306.GE23031@sigill.intra.peff.net","threadId":"51831","inReplyTo":"fdb81ce0-20ee-5880-2a6c-0c8b3f1739b9@gmail.com","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:23:06Z","receivedAt":"2019-09-12T14:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:\n\n> > That creates an interesting problem for commits that have _already_ been\n> > parsed using the commit graph. Their commit->object.parsed flag is set,\n> > their commit->graph_pos is set, but their commit->maybe_tree may still\n> > be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> > we haven't loaded the tree oid yet and try to get it from the commit\n> > graph. But since it has been freed, we segfault!\n> \n> OOPS! That is certainly a bad thing. I'm glad you found it, but I\n> am sorry for how you (probably) found it.\n\nHeh. I'll admit it was quite a slog of debugging, but _most_ of that was\nfiguring out in which circumstance we'd have actually parsed the object.\nFinding the problematic end state was pretty easy from a coredump. :)\n\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 9b02d2c426..bc5dd5913f 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -41,6 +41,8 @@\n> >  #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n> >  \t\t\t+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)\n> >  \n> > +static int commit_graph_disabled;\n> \n> Should we be putting this inside the repository struct instead?\n\nProbably. The only caller will just pass the_repository, but it doesn't\nhurt to scope it down now.\n\nIt could potentially go into the commit_graph itself, but it looks like\nwith the incremental work we may have multiple such structs. It could\nalso go into raw_object_store, but I think conceptually it's a\nrepo-level thing.\n\nSo I put it straight into \"struct repository\".\n\n> Your patch does not seem to actually cover the \"I've already parsed some commits\"\n> case, as you are only preventing the commit-graph from being prepared. Instead,\n> we need to have a short-circuit inside parse_commit() to avoid future parsing\n> from the commit-graph file.\n\nMaybe I was too clever, then. :)\n\nI didn't want to have to sprinkle \"are we disabled\" in parse_commit(),\netc. But any such uses of the commit graph have to do:\n\n  if (!prepare_commit_graph(r))\n\treturn;\n\nto lazy-load it. So the logic to prepare becomes (roughly):\n\n  if (disabled)\n\treturn 0;\n  if (already_loaded)\n\treturn 1;\n  return actually_load() ? 1 : 0;\n\nand \"disabled\" takes precedence.\n\nI've added this comment in prepare_commit_graph():\n\n        /*\n         * This must come before the \"already attempted?\" check below, because\n         * we want to disable even an already-loaded graph file.\n         */\n        if (r->commit_graph_disabled)\n                return 0;\n\n        if (r->objects->commit_graph_attempted)\n                return !!r->objects->commit_graph;\n        r->objects->commit_graph_attempted = 1;\n\nDoes that make more sense?\n\nUnrelated, but I also notice the top of prepare_commit_graph() has:\n\n        if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n                die(\"dying as requested by the '%s' variable on commit-graph load!\",\n                    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n\nas the very first thing. Meaning we're calling getenv() as part of every\nsingle parse_commit(), rather than just once per process. Seems like an\neasy efficiency win.\n\n-Peff\n"},{"id":"382220","messageId":"20190912144122.GA27815@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912000414.GA31334@sigill.intra.peff.net","subject":"[PATCH v2] upload-pack commit graph segfault fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:41:22Z","receivedAt":"2019-09-12T14:41:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of my \"disable commit graph more gently\" fix. Versus\nv1:\n\n  - I've included a preparatory patch that speeds up\n    prepare_commit_graph(). It's not strictly related, but there's a\n    textual dependency. It could be easily spun off to its own series.\n\n  - a comment points out that the ordering of the \"disable\" check is\n    important\n\n  - disable_commit_graph() now works on a repository struct\n\n  - typo fixes in the test comments\n\n  [1/2]: commit-graph: bump DIE_ON_LOAD check to actual load-time\n  [2/2]: upload-pack: disable commit graph more gently for shallow traversal\n\n commit-graph.c        | 18 +++++++++++++++---\n commit-graph.h        |  6 ++++++\n repository.h          |  3 +++\n t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n upload-pack.c         |  2 +-\n 5 files changed, 63 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"382221","messageId":"20190912144320.GA30725@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912144122.GA27815@sigill.intra.peff.net","subject":"Re: [PATCH v2] upload-pack commit graph segfault fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:43:21Z","receivedAt":"2019-09-12T14:43:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2019 at 10:41:22AM -0400, Jeff King wrote:\n\n> Here's a re-roll of my \"disable commit graph more gently\" fix. Versus\n> v1:\n> \n>   - I've included a preparatory patch that speeds up\n>     prepare_commit_graph(). It's not strictly related, but there's a\n>     textual dependency. It could be easily spun off to its own series.\n\nI _didn't_ include here the other speedup from this thread:\n\n  https://public-inbox.org/git/20190912011137.GA23412@sigill.intra.peff.net/\n\nThat's worth doing, but is totally independent (conceptually and\ntextually).\n\n-Peff\n"},{"id":"382222","messageId":"20190912144434.GA25101@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912144122.GA27815@sigill.intra.peff.net","subject":"[PATCH v2 1/2] commit-graph: bump DIE_ON_LOAD check to actual load-time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:44:34Z","receivedAt":"2019-09-12T14:44:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 43d3561805 (commit-graph write: don't die if the existing graph\nis corrupt, 2019-03-25) added an environment variable we use only in the\ntest suite, $GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD. But it put the check for\nthis variable at the very top of prepare_commit_graph(), which is called\nevery time we want to use the commit graph. Most importantly, it comes\n_before_ we check the fast-path \"did we already try to load?\", meaning\nwe end up calling getenv() for every single use of the commit graph,\nrather than just when we load.\n\ngetenv() is allowed to have unexpected side effects, but that shouldn't\nbe a problem here; we're lazy-loading the graph so it's clear that at\nleast _one_ invocation of this function is going to call it.\n\nBut it is inefficient. getenv() typically has to do a linear search\nthrough the environment space.\n\nWe could memoize the call, but it's simpler still to just bump the check\ndown to the actual loading step. That's fine for our sole user in t5318,\nand produces this minor real-world speedup:\n\n  [before]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      1.460 s ±  0.017 s    [User: 1.174 s, System: 0.285 s]\n    Range (min … max):    1.440 s …  1.491 s    10 runs\n\n  [after]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      1.391 s ±  0.005 s    [User: 1.118 s, System: 0.273 s]\n    Range (min … max):    1.385 s …  1.399 s    10 runs\n\nOf course that actual speedup depends on how big your environment is. We\ncan game it like this:\n\n  for i in $(seq 10000); do\n    export dummy$i=$i\n  done\n\nin which case I get:\n\n  [before]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n    Time (mean ± σ):      6.257 s ±  0.061 s    [User: 6.005 s, System: 0.250 s]\n    Range (min … max):    6.174 s …  6.337 s    10 runs\n\n  [after]\n  Benchmark #1: git -C linux rev-list HEAD >/dev/null\n  Time (mean ± σ):      1.403 s ±  0.005 s    [User: 1.146 s, System: 0.256 s]\n  Range (min … max):    1.396 s …  1.412 s    10 runs\n\nSo this is really more about avoiding the pathological case than\nproviding a big real-world speedup.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b02d2c426..baeaf0d1bf 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -468,14 +468,14 @@ static int prepare_commit_graph(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n-\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n-\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n-\t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\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 (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n+\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n+\t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n+\n \tprepare_repo_settings(r);\n \n \tif (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n-- \n2.23.0.667.gcccf1fbb03\n\n"},{"id":"382223","messageId":"20190912144444.GB25101@sigill.intra.peff.net","threadId":"51831","inReplyTo":"20190912144122.GA27815@sigill.intra.peff.net","subject":"[PATCH v2 2/2] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T14:44:45Z","receivedAt":"2019-09-12T14:44:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the client has asked for certain shallow options like\n\"deepen-since\", we do a custom rev-list walk that pretends to be\nshallow. Before doing so, we have to disable the commit-graph, since it\nis not compatible with the shallow view of the repository. That's\nhandled by 829a321569 (commit-graph: close_commit_graph before shallow\nwalk, 2018-08-20). That commit literally closes and frees our\nrepo->objects->commit_graph struct.\n\nThat creates an interesting problem for commits that have _already_ been\nparsed using the commit graph. Their commit->object.parsed flag is set,\ntheir commit->graph_pos is set, but their commit->maybe_tree may still\nbe NULL. When somebody later calls repo_get_commit_tree(), we see that\nwe haven't loaded the tree oid yet and try to get it from the commit\ngraph. But since it has been freed, we segfault!\n\nSo the root of the issue is a data dependency between the commit's\nlazy-load of the tree oid and the fact that the commit graph can go\naway mid-process. How can we resolve it?\n\nThere are a couple of general approaches:\n\n  1. The obvious answer is to avoid loading the tree from the graph when\n     we see that it's NULL. But then what do we return for the tree oid?\n     If we return NULL, our caller in do_traverse() will rightly\n     complain that we have no tree. We'd have to fallback to loading the\n     actual commit object and re-parsing it. That requires teaching\n     parse_commit_buffer() to understand re-parsing (i.e., not starting\n     from a clean slate and not leaking any allocated bits like parent\n     list pointers).\n\n  2. When we close the commit graph, walk through the set of in-memory\n     objects and clear any graph_pos pointers. But this means we also\n     have to \"unparse\" any such commits so that we know they still need\n     to open the commit object to fill in their trees. So it's no less\n     complicated than (1), and is more expensive (since we clear objects\n     we might not later need).\n\n  3. Stop freeing the commit-graph struct. Continue to let it be used\n     for lazy-loads of tree oids, but let upload-pack specify that it\n     shouldn't be used for further commit parsing.\n\n  4. Push the whole shallow rev-list out to its own sub-process, with\n     the commit-graph disabled from the start, giving it a clean memory\n     space to work from.\n\nI've chosen (3) here. Options (1) and (2) would work, but are\nnon-trivial to implement. Option (4) is more expensive, and I'm not sure\nhow complicated it is (shelling out for the actual rev-list part is\neasy, but we do then parse the resulting commits internally, and I'm not\nclear which parts need to be handling shallow-ness).\n\nThe new test in t5500 triggers this segfault, but see the comments there\nfor how horribly intimate it has to be with how both upload-pack and\ncommit graphs work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c        | 12 ++++++++++++\n commit-graph.h        |  6 ++++++\n repository.h          |  3 +++\n t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n upload-pack.c         |  2 +-\n 5 files changed, 60 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex baeaf0d1bf..bbde647f8b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -468,6 +468,13 @@ static int prepare_commit_graph(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n+\t/*\n+\t * This must come before the \"already attempted?\" check below, because\n+\t * we want to disable even an already-loaded graph file.\n+\t */\n+\tif (r->commit_graph_disabled)\n+\t\treturn 0;\n+\n \tif (r->objects->commit_graph_attempted)\n \t\treturn !!r->objects->commit_graph;\n \tr->objects->commit_graph_attempted = 1;\n@@ -2101,3 +2108,8 @@ void free_commit_graph(struct commit_graph *g)\n \tfree(g->filename);\n \tfree(g);\n }\n+\n+void disable_commit_graph(struct repository *r)\n+{\n+\tr->commit_graph_disabled = 1;\n+}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 486e64e591..7f5c933fa2 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -107,4 +107,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n void close_commit_graph(struct raw_object_store *);\n void free_commit_graph(struct commit_graph *);\n \n+/*\n+ * Disable further use of the commit graph in this process when parsing a\n+ * \"struct commit\".\n+ */\n+void disable_commit_graph(struct repository *r);\n+\n #endif\ndiff --git a/repository.h b/repository.h\nindex 4da275e73f..84335292cd 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -124,6 +124,9 @@ struct repository {\n \t/* A unique-id for tracing purposes. */\n \tint trace2_repo_id;\n \n+\t/* True if commit-graph has been disabled within this process. */\n+\tint commit_graph_disabled;\n+\n \t/* Configurations */\n \n \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8210f63d41..244c3e7062 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -783,6 +783,44 @@ test_expect_success 'clone shallow since selects no commits' '\n \t)\n '\n \n+# A few subtle things about the request in this test:\n+#\n+#  - the server must have commit-graphs present and enabled\n+#\n+#  - the history is such that our want/have share a common ancestor (\"base\"\n+#    here)\n+#\n+#  - we send only a single have, which is fewer than a normal client would\n+#    send. This ensures that we don't parse \"base\" up front with\n+#    parse_object(), but rather traverse to it as a parent while deciding if we\n+#    can stop the \"have\" negotiation, and call parse_commit(). The former\n+#    sees the actual object data and so always loads the three oid, whereas the\n+#    latter will try to load it lazily.\n+#\n+#  - we must use protocol v2, because it handles the \"have\" negotiation before\n+#    processing the shallow directives\n+#\n+test_expect_success 'shallow since with commit graph and already-seen commit' '\n+\ttest_create_repo shallow-since-graph &&\n+\t(\n+\tcd shallow-since-graph &&\n+\ttest_commit base &&\n+\ttest_commit master &&\n+\tgit checkout -b other HEAD^ &&\n+\ttest_commit other &&\n+\tgit commit-graph write --reachable &&\n+\tgit config core.commitGraph true &&\n+\n+\tGIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null\n+\t0012command=fetch\n+\t00010013deepen-since 1\n+\t0032want $(git rev-parse other)\n+\t0032have $(git rev-parse master)\n+\t0000\n+\tEOF\n+\t)\n+'\n+\n test_expect_success 'shallow clone exclude tag two' '\n \ttest_create_repo shallow-exclude &&\n \t(\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 222cd3ad89..135bb3f6cc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n {\n \tstruct commit_list *result;\n \n-\tclose_commit_graph(the_repository->objects);\n+\tdisable_commit_graph(the_repository);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-- \n2.23.0.667.gcccf1fbb03\n"},{"id":"382229","messageId":"xmqqd0g5h2xm.fsf@gitster-ct.c.googlers.com","threadId":"51831","inReplyTo":"20190912011137.GA23412@sigill.intra.peff.net","subject":"Re: [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-12T16:52:53Z","receivedAt":"2019-09-12T16:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I was surprised we ever called repo_get_commit_tree() at all, since\n>> we're literally just traversing commits here. It looks like\n>> list-objects.c is very happy to queue pending trees for each commit,\n>> even if we're just going to throw them away when we get to\n>> process_tree()! I wonder if could be checking revs->tree_objects here\n>> and saving ourselves some work.\n>\n> Indeed, this seems to help quite a bit in the commit-graph case. I think\n> it's worth doing (and is independent of the other patch).\n\nYeah, I agree this is very much worth doing and is orthogonal to the\nother one.\n\nThanks for spotting it.  I wonder if it was broken like this forever\nsince the beginning X-<.\n\n>\n> -- >8 --\n> Subject: list-objects: don't queue root trees unless revs->tree_objects is set\n>\n> When traverse_commit_list() processes each commit, it queues the\n> commit's root tree in the pending array. Then, after all commits are\n> processed, it calls traverse_trees_and_blobs() to walk over the pending\n> list, calling process_tree() on each. But if revs->tree_objects is not\n> set, process_tree() just exists immediately!\n>\n> We can save ourselves some work by not even bothering to queue these\n> trees in the first place. There are a few subtle points to make:\n>\n>   - we also detect commits with a NULL tree pointer here. But this isn't\n>     an interesting check for broken commits, since the lookup_tree()\n>     we'd have done during commit parsing doesn't actually check that we\n>     have the tree on disk. So we're not losing any robustness.\n>\n>   - besides queueing, we also set the NOT_USER_GIVEN flag on the tree\n>     object. This is used by the traverse_commit_list_filtered() variant.\n>     But if we're not exploring trees, then we won't actually care about\n>     this flag, which is used only inside process_tree() code-paths.\n>\n>   - queueing trees eventually leads to us queueing blobs, too. But we\n>     don't need to check revs->blob_objects here. Even in the current\n>     code, we still wouldn't find those blobs, because we'd never open up\n>     the tree objects to list their contents.\n>\n>   - the user-visible impact to the caller is minimal. The pending trees\n>     are all cleared by the time the function returns anyway, by\n>     traverse_trees_and_blobs(). We do call a show_commit() callback,\n>     which technically could be looking at revs->pending during the\n>     callback. But it seems like a rather unlikely thing to do (if you\n>     want the tree of the current commit, then accessing the tree struct\n>     member is a lot simpler).\n>\n> So this should be safe to do. Let's look at the benefits:\n>\n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      7.651 s ±  0.021 s    [User: 7.399 s, System: 0.252 s]\n>     Range (min … max):    7.607 s …  7.683 s    10 runs\n>\n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      7.593 s ±  0.023 s    [User: 7.329 s, System: 0.264 s]\n>     Range (min … max):    7.565 s …  7.634 s    10 runs\n>\n> Not too impressive, but then we're really just avoiding sticking a\n> pointer into a growable array. But still, I'll take a free 0.75%\n> speedup.\n>\n> Let's try it after running \"git commit-graph write\":\n>\n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.458 s ±  0.011 s    [User: 1.199 s, System: 0.259 s]\n>     Range (min … max):    1.447 s …  1.481 s    10 runs\n>\n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.126 s ±  0.023 s    [User: 896.5 ms, System: 229.0 ms]\n>     Range (min … max):    1.106 s …  1.181 s    10 runs\n>\n> Now that's more like it. We saved over 22% of the total time. Part of\n> that is because the runtime is shorter overall, but the absolute\n> improvement is also much larger. What's going on?\n>\n> When we fill in a commit struct using the commit graph, we don't bother\n> to set the tree pointer, and instead lazy-load it when somebody calls\n> get_commit_tree(). So we're not only skipping the pointer write to the\n> pending queue, but we're skipping the lazy-load of the tree entirely.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  list-objects.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/list-objects.c b/list-objects.c\n> index b5651ddd5b..c837bcaca8 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -370,7 +370,9 @@ static void do_traverse(struct traversal_context *ctx)\n>  \t\t * an uninteresting boundary commit may not have its tree\n>  \t\t * parsed yet, but we are not going to show them anyway\n>  \t\t */\n> -\t\tif (get_commit_tree(commit)) {\n> +\t\tif (!ctx->revs->tree_objects)\n> +\t\t\t; /* do not bother loading tree */\n> +\t\telse if (get_commit_tree(commit)) {\n>  \t\t\tstruct tree *tree = get_commit_tree(commit);\n>  \t\t\ttree->object.flags |= NOT_USER_GIVEN;\n>  \t\t\tadd_pending_tree(ctx->revs, tree);\n"},{"id":"382230","messageId":"20190912165656.GA86882@syl.lan","threadId":"51831","inReplyTo":"20190912144122.GA27815@sigill.intra.peff.net","subject":"Re: [PATCH v2] upload-pack commit graph segfault fix","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-12T16:56:56Z","receivedAt":"2019-09-12T17:04:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 12, 2019 at 10:41:22AM -0400, Jeff King wrote:\n> Here's a re-roll of my \"disable commit graph more gently\" fix. Versus\n> v1:\n\nThanks for sending a re-roll. I deployed this out to all of our servers\nrunning git at GitHub, and it seems to be working fine.\n\nFor what it's worth, I don't have 'core.commitGraph' enabled on any of\nthose servers for now, but I'll turn it back on shortly.\n\n>   - I've included a preparatory patch that speeds up\n>     prepare_commit_graph(). It's not strictly related, but there's a\n>     textual dependency. It could be easily spun off to its own series.\n>\n>   - a comment points out that the ordering of the \"disable\" check is\n>     important\n>\n>   - disable_commit_graph() now works on a repository struct\n>\n>   - typo fixes in the test comments\n>\n>   [1/2]: commit-graph: bump DIE_ON_LOAD check to actual load-time\n>   [2/2]: upload-pack: disable commit graph more gently for shallow traversal\n>\n>  commit-graph.c        | 18 +++++++++++++++---\n>  commit-graph.h        |  6 ++++++\n>  repository.h          |  3 +++\n>  t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  upload-pack.c         |  2 +-\n>  5 files changed, 63 insertions(+), 4 deletions(-)\n>\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"382246","messageId":"875aea73-eebd-fd8a-4d4b-c3b9df027a04@gmail.com","threadId":"51831","inReplyTo":"20190912142306.GE23031@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-12T19:27:58Z","receivedAt":"2019-09-12T19:28:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/12/2019 10:23 AM, Jeff King wrote:\n> On Thu, Sep 12, 2019 at 08:23:49AM -0400, Derrick Stolee wrote:\n> \n>>> That creates an interesting problem for commits that have _already_ been\n>>> parsed using the commit graph. Their commit->object.parsed flag is set,\n>>> their commit->graph_pos is set, but their commit->maybe_tree may still\n>>> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n>>> we haven't loaded the tree oid yet and try to get it from the commit\n>>> graph. But since it has been freed, we segfault!\n>>\n>> OOPS! That is certainly a bad thing. I'm glad you found it, but I\n>> am sorry for how you (probably) found it.\n> \n> Heh. I'll admit it was quite a slog of debugging, but _most_ of that was\n> figuring out in which circumstance we'd have actually parsed the object.\n> Finding the problematic end state was pretty easy from a coredump. :)\n> \n>>> diff --git a/commit-graph.c b/commit-graph.c\n>>> index 9b02d2c426..bc5dd5913f 100644\n>>> --- a/commit-graph.c\n>>> +++ b/commit-graph.c\n>>> @@ -41,6 +41,8 @@\n>>>  #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \\\n>>>  \t\t\t+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)\n>>>  \n>>> +static int commit_graph_disabled;\n>>\n>> Should we be putting this inside the repository struct instead?\n> \n> Probably. The only caller will just pass the_repository, but it doesn't\n> hurt to scope it down now.\n> \n> It could potentially go into the commit_graph itself, but it looks like\n> with the incremental work we may have multiple such structs. It could\n> also go into raw_object_store, but I think conceptually it's a\n> repo-level thing.\n> \n> So I put it straight into \"struct repository\".\n> \n>> Your patch does not seem to actually cover the \"I've already parsed some commits\"\n>> case, as you are only preventing the commit-graph from being prepared. Instead,\n>> we need to have a short-circuit inside parse_commit() to avoid future parsing\n>> from the commit-graph file.\n> \n> Maybe I was too clever, then. :)\n> \n> I didn't want to have to sprinkle \"are we disabled\" in parse_commit(),\n> etc. But any such uses of the commit graph have to do:\n> \n>   if (!prepare_commit_graph(r))\n> \treturn;\n> \n> to lazy-load it. So the logic to prepare becomes (roughly):\n> \n>   if (disabled)\n> \treturn 0;\n>   if (already_loaded)\n> \treturn 1;\n>   return actually_load() ? 1 : 0;\n> \n> and \"disabled\" takes precedence.\n> \n> I've added this comment in prepare_commit_graph():\n> \n>         /*\n>          * This must come before the \"already attempted?\" check below, because\n>          * we want to disable even an already-loaded graph file.\n>          */\n>         if (r->commit_graph_disabled)\n>                 return 0;\n> \n>         if (r->objects->commit_graph_attempted)\n>                 return !!r->objects->commit_graph;\n>         r->objects->commit_graph_attempted = 1;\n> \n> Does that make more sense?\n\nAh. That does make sense. I now see the connection between parsing and this\nchange.\n\n> Unrelated, but I also notice the top of prepare_commit_graph() has:\n> \n>         if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n>                 die(\"dying as requested by the '%s' variable on commit-graph load!\",\n>                     GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n> \n> as the very first thing. Meaning we're calling getenv() as part of every\n> single parse_commit(), rather than just once per process. Seems like an\n> easy efficiency win.\n\nAbsolutely. Move this to after the \"have we attempted already?\" condition.\n\nThanks,\n-Stolee\n\n"},{"id":"382247","messageId":"ef10d60c-aab1-4336-a330-9ea4d102bbc5@gmail.com","threadId":"51831","inReplyTo":"20190912144434.GA25101@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] commit-graph: bump DIE_ON_LOAD check to actual load-time","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-12T19:30:26Z","receivedAt":"2019-09-12T19:30:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/12/2019 10:44 AM, Jeff King wrote:\n> Commit 43d3561805 (commit-graph write: don't die if the existing graph\n> is corrupt, 2019-03-25) added an environment variable we use only in the\n> test suite, $GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD. But it put the check for\n> this variable at the very top of prepare_commit_graph(), which is called\n> every time we want to use the commit graph. Most importantly, it comes\n> _before_ we check the fast-path \"did we already try to load?\", meaning\n> we end up calling getenv() for every single use of the commit graph,\n> rather than just when we load.\n> \n> getenv() is allowed to have unexpected side effects, but that shouldn't\n> be a problem here; we're lazy-loading the graph so it's clear that at\n> least _one_ invocation of this function is going to call it.\n> \n> But it is inefficient. getenv() typically has to do a linear search\n> through the environment space.\n> \n> We could memoize the call, but it's simpler still to just bump the check\n> down to the actual loading step. That's fine for our sole user in t5318,\n> and produces this minor real-world speedup:\n> \n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.460 s ±  0.017 s    [User: 1.174 s, System: 0.285 s]\n>     Range (min … max):    1.440 s …  1.491 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      1.391 s ±  0.005 s    [User: 1.118 s, System: 0.273 s]\n>     Range (min … max):    1.385 s …  1.399 s    10 runs\n\nThis looks like an important improvement on its own.\n\n> Of course that actual speedup depends on how big your environment is. We\n> can game it like this:\n> \n>   for i in $(seq 10000); do\n>     export dummy$i=$i\n>   done\n> \n> in which case I get:\n> \n>   [before]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>     Time (mean ± σ):      6.257 s ±  0.061 s    [User: 6.005 s, System: 0.250 s]\n>     Range (min … max):    6.174 s …  6.337 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git -C linux rev-list HEAD >/dev/null\n>   Time (mean ± σ):      1.403 s ±  0.005 s    [User: 1.146 s, System: 0.256 s]\n>   Range (min … max):    1.396 s …  1.412 s    10 runs\n> \n> So this is really more about avoiding the pathological case than\n> providing a big real-world speedup.\n\nThis change is stunning. I'm _sure_ someone is hurting with this.\n \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  commit-graph.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 9b02d2c426..baeaf0d1bf 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -468,14 +468,14 @@ static int prepare_commit_graph(struct repository *r)\n>  {\n>  \tstruct object_directory *odb;\n>  \n> -\tif (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n> -\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n> -\t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\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 (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0))\n> +\t\tdie(\"dying as requested by the '%s' variable on commit-graph load!\",\n> +\t\t    GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD);\n> +\n>  \tprepare_repo_settings(r);\n>  \n>  \tif (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&\n\nLGTM, thanks!\n\n-Stolee\n"},{"id":"382276","messageId":"20190912223400.GA2580@sigill.intra.peff.net","threadId":"51831","inReplyTo":"xmqqd0g5h2xm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-12T22:34:00Z","receivedAt":"2019-09-12T22:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2019 at 09:52:53AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> I was surprised we ever called repo_get_commit_tree() at all, since\n> >> we're literally just traversing commits here. It looks like\n> >> list-objects.c is very happy to queue pending trees for each commit,\n> >> even if we're just going to throw them away when we get to\n> >> process_tree()! I wonder if could be checking revs->tree_objects here\n> >> and saving ourselves some work.\n> >\n> > Indeed, this seems to help quite a bit in the commit-graph case. I think\n> > it's worth doing (and is independent of the other patch).\n> \n> Yeah, I agree this is very much worth doing and is orthogonal to the\n> other one.\n> \n> Thanks for spotting it.  I wonder if it was broken like this forever\n> since the beginning X-<.\n\nNot quite since the beginning; it comes from 8d2dfc49b1\n(process_{tree,blob}: show objects without buffering, 2009-04-10). I\nsuspect nobody noticed because the cost for the normal case is really\njust shuffling some pointers around. It's only because it actively works\nagainst the commit-graph optimizations that it's so expensive there.\n\nI was surprised (and pleased) by how much such a simple thing helped.\n\n-Peff\n"},{"id":"382302","messageId":"ef18a12f-fbae-4f78-4426-11ec1515530b@gmail.com","threadId":"51831","inReplyTo":"20190912144444.GB25101@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] upload-pack: disable commit graph more gently for shallow traversal","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-13T13:26:18Z","receivedAt":"2019-09-13T13:27:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/12/2019 10:44 AM, Jeff King wrote:\n> When the client has asked for certain shallow options like\n> \"deepen-since\", we do a custom rev-list walk that pretends to be\n> shallow. Before doing so, we have to disable the commit-graph, since it\n> is not compatible with the shallow view of the repository. That's\n> handled by 829a321569 (commit-graph: close_commit_graph before shallow\n> walk, 2018-08-20). That commit literally closes and frees our\n> repo->objects->commit_graph struct.\n> \n> That creates an interesting problem for commits that have _already_ been\n> parsed using the commit graph. Their commit->object.parsed flag is set,\n> their commit->graph_pos is set, but their commit->maybe_tree may still\n> be NULL. When somebody later calls repo_get_commit_tree(), we see that\n> we haven't loaded the tree oid yet and try to get it from the commit\n> graph. But since it has been freed, we segfault!\n> \n> So the root of the issue is a data dependency between the commit's\n> lazy-load of the tree oid and the fact that the commit graph can go\n> away mid-process. How can we resolve it?\n> \n> There are a couple of general approaches:\n> \n>   1. The obvious answer is to avoid loading the tree from the graph when\n>      we see that it's NULL. But then what do we return for the tree oid?\n>      If we return NULL, our caller in do_traverse() will rightly\n>      complain that we have no tree. We'd have to fallback to loading the\n>      actual commit object and re-parsing it. That requires teaching\n>      parse_commit_buffer() to understand re-parsing (i.e., not starting\n>      from a clean slate and not leaking any allocated bits like parent\n>      list pointers).\n> \n>   2. When we close the commit graph, walk through the set of in-memory\n>      objects and clear any graph_pos pointers. But this means we also\n>      have to \"unparse\" any such commits so that we know they still need\n>      to open the commit object to fill in their trees. So it's no less\n>      complicated than (1), and is more expensive (since we clear objects\n>      we might not later need).\n> \n>   3. Stop freeing the commit-graph struct. Continue to let it be used\n>      for lazy-loads of tree oids, but let upload-pack specify that it\n>      shouldn't be used for further commit parsing.\n> \n>   4. Push the whole shallow rev-list out to its own sub-process, with\n>      the commit-graph disabled from the start, giving it a clean memory\n>      space to work from.\n> \n> I've chosen (3) here. Options (1) and (2) would work, but are\n> non-trivial to implement. Option (4) is more expensive, and I'm not sure\n> how complicated it is (shelling out for the actual rev-list part is\n> easy, but we do then parse the resulting commits internally, and I'm not\n> clear which parts need to be handling shallow-ness).\n> \n> The new test in t5500 triggers this segfault, but see the comments there\n> for how horribly intimate it has to be with how both upload-pack and\n> commit graphs work.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  commit-graph.c        | 12 ++++++++++++\n>  commit-graph.h        |  6 ++++++\n>  repository.h          |  3 +++\n>  t/t5500-fetch-pack.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  upload-pack.c         |  2 +-\n>  5 files changed, 60 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index baeaf0d1bf..bbde647f8b 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -468,6 +468,13 @@ static int prepare_commit_graph(struct repository *r)\n>  {\n>  \tstruct object_directory *odb;\n>  \n> +\t/*\n> +\t * This must come before the \"already attempted?\" check below, because\n> +\t * we want to disable even an already-loaded graph file.\n> +\t */\n> +\tif (r->commit_graph_disabled)\n> +\t\treturn 0;\n> +\n>  \tif (r->objects->commit_graph_attempted)\n>  \t\treturn !!r->objects->commit_graph;\n>  \tr->objects->commit_graph_attempted = 1;\n> @@ -2101,3 +2108,8 @@ void free_commit_graph(struct commit_graph *g)\n>  \tfree(g->filename);\n>  \tfree(g);\n>  }\n> +\n> +void disable_commit_graph(struct repository *r)\n> +{\n> +\tr->commit_graph_disabled = 1;\n> +}\n\nThanks for the additional comment and using the repo struct. LGTM.\n\n-Stolee\n"},{"id":"382317","messageId":"xmqqftl0dqcl.fsf@gitster-ct.c.googlers.com","threadId":"51831","inReplyTo":"20190912223400.GA2580@sigill.intra.peff.net","subject":"Re: [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T18:05:14Z","receivedAt":"2019-09-13T18:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Thanks for spotting it.  I wonder if it was broken like this forever\n>> since the beginning X-<.\n>\n> Not quite since the beginning; it comes from 8d2dfc49b1\n> (process_{tree,blob}: show objects without buffering, 2009-04-10). I\n> suspect nobody noticed because the cost for the normal case is really\n> just shuffling some pointers around. It's only because it actively works\n> against the commit-graph optimizations that it's so expensive there.\n\nYeah, that is what I meant by \"since the beginning\" (of\ncommit-graph, that is).\n\n> I was surprised (and pleased) by how much such a simple thing helped.\n\nYes, it is very pleasing.  Thanks.\n"}]}