{"thread":{"id":"49947","subject":"[PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","startedAt":"2018-12-04T22:42:47Z","lastAt":"2019-01-28T17:01:35Z","messageCount":28,"participants":["Jonathan Tan","Stefan Beller","Jeff King","Junio C Hamano","Derrick Stolee","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364581","messageId":"20181204224238.50966-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":null,"subject":"[PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-04T22:42:38Z","receivedAt":"2018-12-04T22:42:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching into a repository, a connectivity check is first made by\ncheck_exist_and_connected() in builtin/fetch.c that runs:\n\n git rev-list --objects --stdin --not --all --quiet <(list of objects)\n\nIf the client repository has many refs, this command can be slow,\nregardless of the nature of the server repository or what is being\nfetched. A profiler reveals that most of the time is spent in\nsetup_revisions() (approx. 60/63), and of the time spent in\nsetup_revisions(), most of it is spent in parse_object() (approx.\n49/60). This is because setup_revisions() parses the target of every ref\n(from \"--all\"), and parse_object() reads the buffer of the object.\n\nReading the buffer is unnecessary if the repository has a commit graph\nand if the ref points to a commit (which is typically the case). This\npatch uses the commit graph wherever possible; on my computer, when I\nrun the above command with a list of 1 object on a many-ref repository,\nI get a speedup from 1.8s to 1.0s.\n\nAnother way to accomplish this effect would be to modify parse_object()\nto use the commit graph if possible; however, I did not want to change\nparse_object()'s current behavior of always checking the object\nsignature of the returned object.\n\nSigned-off-by: Jonathan Tan \n---\nThis is on sb/more-repo-in-api because I'm using the repo_parse_commit()\nfunction.\n\nA colleague noticed this issue when handling a mirror clone.\n\nLooking at the bigger picture, the speed of the connectivity check\nduring a fetch might be further improved by passing only the negotiation\ntips (obtained through --negotiation-tip) instead of \"--all\". This patch\njust handles the low-hanging fruit first.\n---\n revision.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex b5108b75ab..e7da2c57ab 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n {\n \tstruct object *object;\n \n-\tobject = parse_object(revs->repo, oid);\n+\t/*\n+\t * If the repository has commit graphs, repo_parse_commit() avoids\n+\t * reading the object buffer, so use it whenever possible.\n+\t */\n+\tif (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n+\t\tstruct commit *c = lookup_commit(revs->repo, oid);\n+\t\tif (!repo_parse_commit(revs->repo, c))\n+\t\t\tobject = (struct object *) c;\n+\t\telse\n+\t\t\tobject = NULL;\n+\t} else {\n+\t\tobject = parse_object(revs->repo, oid);\n+\t}\n+\n \tif (!object) {\n \t\tif (revs->ignore_missing)\n \t\t\treturn object;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364582","messageId":"CAGZ79kYOOk2ODYgRcSZgDUqBfx2HeywnEGpbJB9BrrVzEUi_JA@mail.gmail.com","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-04T23:12:21Z","receivedAt":"2018-12-04T23:12:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 4, 2018 at 2:42 PM Jonathan Tan wrote:\n>\n> When fetching into a repository, a connectivity check is first made by\n> check_exist_and_connected() in builtin/fetch.c that runs:\n>\n> git rev-list --objects --stdin --not --all --quiet <(list of objects)\n>\n> If the client repository has many refs, this command can be slow,\n> regardless of the nature of the server repository or what is being\n> fetched. A profiler reveals that most of the time is spent in\n> setup_revisions() (approx. 60/63), and of the time spent in\n> setup_revisions(), most of it is spent in parse_object() (approx.\n> 49/60). This is because setup_revisions() parses the target of every ref\n> (from \"--all\"), and parse_object() reads the buffer of the object.\n>\n> Reading the buffer is unnecessary if the repository has a commit graph\n> and if the ref points to a commit (which is typically the case). This\n> patch uses the commit graph wherever possible; on my computer, when I\n> run the above command with a list of 1 object on a many-ref repository,\n> I get a speedup from 1.8s to 1.0s.\n>\n> Another way to accomplish this effect would be to modify parse_object()\n> to use the commit graph if possible; however, I did not want to change\n> parse_object()'s current behavior of always checking the object\n> signature of the returned object.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> This is on sb/more-repo-in-api because I'm using the repo_parse_commit()\n> function.\n\nThis is a mere nicety, not strictly required.\nBefore we had parse_commit(struct commit *) which would accomplish the\nsame, (and we'd still have that afterwards as a #define falling back onto\nthe_repository). As the function get_reference() is not the_repository safe\nas it contains a call to is_promisor_object() that is repository\nagnostic, I think\nit would be fair game to not depend on that series. I am not\ncomplaining, though.\n\n> A colleague noticed this issue when handling a mirror clone.\n>\n> Looking at the bigger picture, the speed of the connectivity check\n> during a fetch might be further improved by passing only the negotiation\n> tips (obtained through --negotiation-tip) instead of \"--all\". This patch\n> just handles the low-hanging fruit first.\n> ---\n> revision.c | 15 ++++++++++++++-\n> 1 file changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index b5108b75ab..e7da2c57ab 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> {\n> struct object *object;\n>\n> - object = parse_object(revs->repo, oid);\n> + /*\n> + * If the repository has commit graphs, repo_parse_commit() avoids\n> + * reading the object buffer, so use it whenever possible.\n> + */\n> + if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n> + struct commit *c = lookup_commit(revs->repo, oid);\n> + if (!repo_parse_commit(revs->repo, c))\n> + object = (struct object *) c;\n> + else\n> + object = NULL;\n\nWould it make sense in this case to rely on parse_object below\ninstead of assigning NULL? The reason for that would be that\nwhen lookup_commit returns NULL, we would try more broadly.\n\nAFAICT oid_object_info doesn't take advantage of the commit graph,\nbut just looks up the object header, which is still less than completely\nparsing it. Then lookup_commit is overly strict, as it may return\nNULL as when there still is a type mismatch (I don't think a mismatch\ncould happen here, as both rely on just the object store, and not the\ncommit graph.), so this would be just defensive programming for\nthe sake of it. I dunno.\n\n struct commit *c;\n\n if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT &&\n (c = lookup_commit(revs->repo, oid)) &&\n !repo_parse_commit(revs->repo, c))\n object = (struct object *) c;\n else\n object = parse_object(revs->repo, oid);\n\n\nSo with all that said, I still think this is a good patch.\n\nThanks,\nStefan\n\n> + } else {\n> + object = parse_object(revs->repo, oid);\n> + }\n> +\n> if (!object) {\n> if (revs->ignore_missing)\n> return object;\n> --\n> 2.19.0.271.gfe8321ec05.dirty\n>\n"},{"id":"364602","messageId":"20181205045416.GB12284@sigill.intra.peff.net","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-05T04:54:16Z","receivedAt":"2018-12-05T04:54:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 04, 2018 at 02:42:38PM -0800, Jonathan Tan wrote:\n\n> diff --git a/revision.c b/revision.c\n> index b5108b75ab..e7da2c57ab 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> {\n> \tstruct object *object;\n> \n> -\tobject = parse_object(revs->repo, oid);\n> +\t/*\n> +\t * If the repository has commit graphs, repo_parse_commit() avoids\n> +\t * reading the object buffer, so use it whenever possible.\n> +\t */\n> +\tif (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n> +\t\tstruct commit *c = lookup_commit(revs->repo, oid);\n> +\t\tif (!repo_parse_commit(revs->repo, c))\n> +\t\t\tobject = (struct object *) c;\n> +\t\telse\n> +\t\t\tobject = NULL;\n> +\t} else {\n> +\t\tobject = parse_object(revs->repo, oid);\n> +\t}\n\nThis seems like a reasonable thing to do, but I have sort of a\nmeta-comment. In several places we've started doing this kind of \"if\nit's this type of object, do X, otherwise do Y\" optimization (e.g.,\nhandling large blobs for streaming).\n\nAnd in the many cases we end up doubling the effort to do object\nlookups: here we do one lookup to get the type, and then if it's not a\ncommit (or if we don't have a commit graph) we end up parsing it anyway.\n\nI wonder if we could do better. In this instance, it might make sense\nto first see if we actually have a commit graph available (it might not\nhave this object, of course, but at least we'd expect it to have most\ncommits). In general, it would be nice if we had a more incremental API\nfor accessing objects: open, get metadata, then read the data. That\nwould make these kinds of optimizations \"free\".\n\nI don't have numbers for how much the extra lookups cost. The lookups\nare probably dwarfed by parse_object() in general, so even if we save\nonly a few full object loads, it may be a win. It just seems a shame\nthat we may be making the \"slow\" paths (when our type-specific check\ndoesn't match) even slower.\n\n-Peff\n"},{"id":"364658","messageId":"xmqqin07bm5w.fsf@gitster-ct.c.googlers.com","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T23:15:23Z","receivedAt":"2018-12-05T23:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Looking at the bigger picture, the speed of the connectivity check\n> during a fetch might be further improved by passing only the negotiation\n> tips (obtained through --negotiation-tip) instead of \"--all\". This patch\n> just handles the low-hanging fruit first.\n\nThat sounds like a good direction, when having to list excessive\nnumber of refs is the primary problem. When fetching their 'master'\ninto our 'remotes/origin/master' and doing nothing else, we may end\nup showing only the latter, which will miss optimization opportunity\na lot if the latest change made over there is to merge in the change\nwe asked them to pull earlier (which would be greatly helped if we\nlet them know about the tip of the topic they earlier pulled from\nus), but also avoids having to send irrelevant refs that point at\ntags addded to months old states. So there is a subtle trade-off\nbetween sending more refs to reduce the resulting packfile, and\nsending fewer refs to reduce the cost of the \"have\" exchange.\n\nChanging the way to throw each object pointed at by a ref into the\nqueue to be emitted in the \"have\" exchange from regular object\nparsing to peeking of precomputed data would reduce the local cost\nof \"have\" exchange, but it does not reduce the network cost at all,\nthough.\n\nAs to the change being specific to get_reference() and not to\nparse_object(), I think what we see here is probably better, simply\nbecause parse_object() is not in the position to asssume that it is\nlikely to be asked to parse commits, but I think get_reference() is,\nafter looking at its callsites in revision.c.\n\nI do share the meta-comment concern with Peff, though.\n\n> ---\n> revision.c | 15 ++++++++++++++-\n> 1 file changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index b5108b75ab..e7da2c57ab 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> {\n> \tstruct object *object;\n> \n> -\tobject = parse_object(revs->repo, oid);\n> +\t/*\n> +\t * If the repository has commit graphs, repo_parse_commit() avoids\n> +\t * reading the object buffer, so use it whenever possible.\n> +\t */\n> +\tif (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n> +\t\tstruct commit *c = lookup_commit(revs->repo, oid);\n> +\t\tif (!repo_parse_commit(revs->repo, c))\n> +\t\t\tobject = (struct object *) c;\n> +\t\telse\n> +\t\t\tobject = NULL;\n> +\t} else {\n> +\t\tobject = parse_object(revs->repo, oid);\n> +\t}\n> +\n> \tif (!object) {\n> \t\tif (revs->ignore_missing)\n> \t\t\treturn object;\n"},{"id":"364724","messageId":"20181206233626.144072-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"CAGZ79kYOOk2ODYgRcSZgDUqBfx2HeywnEGpbJB9BrrVzEUi_JA@mail.gmail.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-06T23:36:26Z","receivedAt":"2018-12-06T23:36:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > This is on sb/more-repo-in-api because I'm using the repo_parse_commit()\n> > function.\n> \n> This is a mere nicety, not strictly required.\n> Before we had parse_commit(struct commit *) which would accomplish the\n> same, (and we'd still have that afterwards as a #define falling back onto\n> the_repository). As the function get_reference() is not the_repository safe\n> as it contains a call to is_promisor_object() that is repository\n> agnostic, I think\n> it would be fair game to not depend on that series. I am not\n> complaining, though.\n\nGood point - I'll base the next version on master (and add a TODO\nexplaining which functions are not yet converted).\n\n> AFAICT oid_object_info doesn't take advantage of the commit graph,\n> but just looks up the object header, which is still less than completely\n> parsing it. Then lookup_commit is overly strict, as it may return\n> NULL as when there still is a type mismatch (I don't think a mismatch\n> could happen here, as both rely on just the object store, and not the\n> commit graph.), so this would be just defensive programming for\n> the sake of it. I dunno.\n> \n> struct commit *c;\n> \n> if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT &&\n> (c = lookup_commit(revs->repo, oid)) &&\n> !repo_parse_commit(revs->repo, c))\n> object = (struct object *) c;\n> else\n> object = parse_object(revs->repo, oid);\n\nI like this way better - I'll do it in the next version.\n"},{"id":"364725","messageId":"20181206235446.147173-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"20181205045416.GB12284@sigill.intra.peff.net","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-06T23:54:46Z","receivedAt":"2018-12-06T23:54:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Also CC-ing Stolee since I mention multi-pack indices at the end.\n\n> This seems like a reasonable thing to do, but I have sort of a\n> meta-comment. In several places we've started doing this kind of \"if\n> it's this type of object, do X, otherwise do Y\" optimization (e.g.,\n> handling large blobs for streaming).\n> \n> And in the many cases we end up doubling the effort to do object\n> lookups: here we do one lookup to get the type, and then if it's not a\n> commit (or if we don't have a commit graph) we end up parsing it anyway.\n> \n> I wonder if we could do better. In this instance, it might make sense\n> to first see if we actually have a commit graph available (it might not\n> have this object, of course, but at least we'd expect it to have most\n> commits).\n\nThis makes sense - I thought I shouldn't mention the commit graph in the\ncode since it seems like a layering violation, but I felt the need to\nmention commit graph in a comment, so maybe the need to mention commit\ngraph in the code is there too. Subsequently, maybe the lookup-for-type\ncould be replaced by a lookup-in-commit-graph (maybe by using\nparse_commit_in_graph() directly), which should be at least slightly\nfaster.\n\n> In general, it would be nice if we had a more incremental API\n> for accessing objects: open, get metadata, then read the data. That\n> would make these kinds of optimizations \"free\".\n\nWould this be assuming that to read the data, you would (1) first need to\nread the metadata, and (2) there would be no redundancy in reading the\ntwo? It seems to me that for loose objects, you would want to perform\nall your reads at once, since any read requires opening the file, and\nfor commit graphs, you just want to read what you want, since the\nmetadata and the data are in separate places.\n\n> I don't have numbers for how much the extra lookups cost. The lookups\n> are probably dwarfed by parse_object() in general, so even if we save\n> only a few full object loads, it may be a win. It just seems a shame\n> that we may be making the \"slow\" paths (when our type-specific check\n> doesn't match) even slower.\n\nI agree. I think it will always remain a tradeoff when we have multiple\ndata sources of objects (loose, packed, commit graph - and we can't\nunify them all, since they each have their uses). Unless the multi-pack\nindex can reference commit graphs as well...then it could be our first\npoint of reference without introducing any inefficiencies...\n"},{"id":"364738","messageId":"20181207085334.GA5167@sigill.intra.peff.net","threadId":"49947","inReplyTo":"20181206235446.147173-1-jonathantanmy@google.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-07T08:53:34Z","receivedAt":"2018-12-07T08:53:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 06, 2018 at 03:54:46PM -0800, Jonathan Tan wrote:\n\n> This makes sense - I thought I shouldn't mention the commit graph in the\n> code since it seems like a layering violation, but I felt the need to\n> mention commit graph in a comment, so maybe the need to mention commit\n> graph in the code is there too. Subsequently, maybe the lookup-for-type\n> could be replaced by a lookup-in-commit-graph (maybe by using\n> parse_commit_in_graph() directly), which should be at least slightly\n> faster.\n\nThat makes more sense to me. If we don't have a commit graph at all,\nit's a quick noop. If we do, we might binary search in the list of\ncommits for a non-commit. But that's strictly faster than finding the\nobject's type (which involves a binary search of a larger list, followed\nby actually accessing the type info).\n\n> > In general, it would be nice if we had a more incremental API\n> > for accessing objects: open, get metadata, then read the data. That\n> > would make these kinds of optimizations \"free\".\n> \n> Would this be assuming that to read the data, you would (1) first need to\n> read the metadata, and (2) there would be no redundancy in reading the\n> two? It seems to me that for loose objects, you would want to perform\n> all your reads at once, since any read requires opening the file, and\n> for commit graphs, you just want to read what you want, since the\n> metadata and the data are in separate places.\n\nBy metadata here, I don't mean the commit-graph data, but just the\nobject type and size. So I'm imagining an interface more like:\n\n - object_open() locates the object, and stores either the pack\n file/offset or a descriptor to a loose path in an opaque handle\n struct\n\n - object_size() and object_type() on that handle would do what you\n expect. For loose objects, these would parse the header (the\n equivalent of unpack_sha1_header()). For packed ones, they'd use the\n object header in the pack (and chase down the delta bits as needed).\n\n - object_contents() would return the full content\n\n - object_read() could sequentially read a subset of the file (this\n could replace the streaming interface we currently have)\n\nWe have most of the low-level bits for this already, if you poke into\nwhat object_info_extended() is doing. We just don't have them packaged\nin an interface which can persist across multiple calls.\n\nWith an interface like that, parse_object()'s large-blob check could be\nsomething like the patch below.\n\nBut your case here is a bit more interesting. If we have a commit graph,\nthen we can avoid opening (or even finding!) the on-disk object at all.\nSo I actually think it makes sense to just check the commit-graph first,\nas discussed above.\n\n---\ndiff --git a/object.c b/object.c\nindex e54160550c..afce58c0bc 100644\n--- a/object.c\n+++ b/object.c\n@@ -254,23 +254,31 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \tconst struct object_id *repl = lookup_replace_object(r, oid);\n \tvoid *buffer;\n \tstruct object *obj;\n+\tstruct object_handle oh;\n \n \tobj = lookup_object(r, oid->hash);\n \tif (obj && obj->parsed)\n \t\treturn obj;\n \n-\tif ((obj && obj->type == OBJ_BLOB && has_object_file(oid)) ||\n-\t (!obj && has_object_file(oid) &&\n-\t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n-\t\tif (check_object_signature(repl, NULL, 0, NULL) < 0) {\n+\tif (object_open(&oh, oid) < 0)\n+\t\treturn NULL; /* missing object */\n+\n+\tif (object_type(&oh) == OBJ_BLOB) {\n+\t\t/* this will call object_read() on 4k chunks */\n+\t\tif (check_object_signature_stream(&oh, oid)) {\n \t\t\terror(_(\"sha1 mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\n \t\t}\n+\t\tobject_close(&oh); /* we don't care about contents */\n \t\tparse_blob_buffer(lookup_blob(r, oid), NULL, 0);\n \t\treturn lookup_object(r, oid->hash);\n \t}\n \n-\tbuffer = read_object_file(oid, &type, &size);\n+\ttype = object_type(&oh);\n+\tsize = object_size(&oh);\n+\tbuffer = object_contents(&oh);\n+\tobject_close(&oh);\n+\n \tif (buffer) {\n \t\tif (check_object_signature(repl, buffer, size, type_name(type)) < 0) {\n \t\t\tfree(buffer);\n"},{"id":"364744","messageId":"aa0cd481-c135-47aa-2a69-e3dc71661caa@gmail.com","threadId":"49947","inReplyTo":"20181206233626.144072-1-jonathantanmy@google.com","subject":"Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-07T13:49:21Z","receivedAt":"2018-12-07T13:50:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/6/2018 6:36 PM, Jonathan Tan wrote:\n>> AFAICT oid_object_info doesn't take advantage of the commit graph,\n>> but just looks up the object header, which is still less than completely\n>> parsing it. Then lookup_commit is overly strict, as it may return\n>> NULL as when there still is a type mismatch (I don't think a mismatch\n>> could happen here, as both rely on just the object store, and not the\n>> commit graph.), so this would be just defensive programming for\n>> the sake of it. I dunno.\n>>\n>> struct commit *c;\n>>\n>> if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT &&\n>> (c = lookup_commit(revs->repo, oid)) &&\n>> !repo_parse_commit(revs->repo, c))\n>> object = (struct object *) c;\n>> else\n>> object = parse_object(revs->repo, oid);\n> I like this way better - I'll do it in the next version.\n\nIf we do _not_ have a commit-graph or if the commit-graph does not have\nthat commit, this will have the same performance problem, right?\n\nShould we instead create a direct dependence on the commit-graph, and try\nto parse the oid from the graph directly? If it succeeds, then we learn\nthat the object is a commit, in addition to all of the parsing work. This\nmeans we could avoid oid_object_info() loading data if we succeed. We\nwould fall back to parse_object() if it fails.\n\nI was thinking this should be a simple API call to parse_commit_in_graph(),\nbut that requires a struct commit filled with an oid, which is not the\nbest idea if we don't actually know it is a commit yet.\n\nThe approach I recommend would then be more detailed:\n\n1. Modify find_commit_in_graph() to take a struct object_id instead of a\n    struct commit. This helps find the integer position in the graph. That\n    position can be used in fill_commit_in_graph() to load the commit\n    contents. Keep find_commit_in_graph() static as it should not be a\n    public function.\n\n2. Create a public function with prototype\n\nstruct commit *try_parse_commit_from_graph(struct repository *r, struct \nobject_id *oid)\n\n    that returns a commit struct fully parsed if and only if the repository\n    has that oid. It can call find_commit_in_graph(), then \nlookup_commit() and\n    fill_commit_in_graph() to create the commit and parse the data.\n\n3. In replace of the snippet above, do:\n\n     struct commit *c;\n\n     if ((c = try_parse_commit_from_graph(revs->repo, oid))\n         object = (struct object *)c;\n     else\n         object = parse_object(revs->repo, oid);\n\nA similar pattern _could_ be used in parse_object(), but I don't recommend\ndoing this pattern unless we have a reasonable suspicion that we are going\nto parse commits more often than other objects. (It adds an O(log(# \ncommits))\nbinary search to each object.)\n\nA final thought: consider making this \"try the commit graph first, but fall\nback to parse_object()\" a library function with a name like\n\n     struct object *parse_probably_commit(struct repository *r, struct \nobject_id *oid)\n\nso other paths that are parsing a lot of commits (but also maybe tags) could\nuse the logic.\n\nThanks!\n-Stolee\n"},{"id":"364755","messageId":"20181207215034.213211-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"[PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-07T21:50:34Z","receivedAt":"2018-12-07T21:50:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching into a repository, a connectivity check is first made by\ncheck_exist_and_connected() in builtin/fetch.c that runs:\n\n git rev-list --objects --stdin --not --all --quiet <(list of objects)\n\nIf the client repository has many refs, this command can be slow,\nregardless of the nature of the server repository or what is being\nfetched. A profiler reveals that most of the time is spent in\nsetup_revisions() (approx. 60/63), and of the time spent in\nsetup_revisions(), most of it is spent in parse_object() (approx.\n49/60). This is because setup_revisions() parses the target of every ref\n(from \"--all\"), and parse_object() reads the buffer of the object.\n\nReading the buffer is unnecessary if the repository has a commit graph\nand if the ref points to a commit (which is typically the case). This\npatch uses the commit graph wherever possible; on my computer, when I\nrun the above command with a list of 1 object on a many-ref repository,\nI get a speedup from 1.8s to 1.0s.\n\nAnother way to accomplish this effect would be to modify parse_object()\nto use the commit graph if possible; however, I did not want to change\nparse_object()'s current behavior of always checking the object\nsignature of the returned object.\n\nSigned-off-by: Jonathan Tan \n---\nThis patch is now on master.\n\nv2 makes use of the optimization Stolee describes in [1], except that I\nhave arranged the functions slightly differently. In particular, I\ndidn't want to add even more ways to obtain objects, so I let\nparse_commit_in_graph() be able to take in either a commit shell or an\nOID, and did not create the parse_probably_commit() function he\nsuggested. But I'm not really attached to this design choice, and can\nchange it if requested.\n\n[1] https://public-inbox.org/git/aa0cd481-c135-47aa-2a69-e3dc71661caa@gmail.com/\n---\n commit-graph.c | 38 ++++++++++++++++++++++++++++----------\n commit-graph.h | 12 ++++++++----\n commit.c | 2 +-\n revision.c | 5 ++++-\n t/helper/test-repository.c | 4 ++--\n 5 files changed, 43 insertions(+), 18 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..a571b523b7 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -286,7 +286,8 @@ void close_commit_graph(struct repository *r)\n \tr->objects->commit_graph = NULL;\n }\n \n-static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n+static int bsearch_graph(struct commit_graph *g, const struct object_id *oid,\n+\t\t\t uint32_t *pos)\n {\n \treturn bsearch_hash(oid->hash, g->chunk_oid_fanout,\n \t\t\t g->chunk_oid_lookup, g->hash_len, pos);\n@@ -374,24 +375,41 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n \t}\n }\n \n-static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n+static struct commit *parse_commit_in_graph_one(struct repository *r,\n+\t\t\t\t\t\tstruct commit_graph *g,\n+\t\t\t\t\t\tstruct commit *shell,\n+\t\t\t\t\t\tconst struct object_id *oid)\n {\n \tuint32_t pos;\n \n-\tif (item->object.parsed)\n-\t\treturn 1;\n+\tif (shell && shell->object.parsed)\n+\t\treturn shell;\n \n-\tif (find_commit_in_graph(item, g, &pos))\n-\t\treturn fill_commit_in_graph(item, g, pos);\n+\tif (shell && shell->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n+\t\tpos = shell->graph_pos;\n+\t} else if (bsearch_graph(g, shell ? &shell->object.oid : oid, &pos)) {\n+\t\t/* bsearch_graph sets pos */\n+\t} else {\n+\t\treturn NULL;\n+\t}\n \n-\treturn 0;\n+\tif (!shell) {\n+\t\tshell = lookup_commit(r, oid);\n+\t\tif (!shell)\n+\t\t\treturn NULL;\n+\t}\n+\n+\tfill_commit_in_graph(shell, g, pos);\n+\treturn shell;\n }\n \n-int parse_commit_in_graph(struct repository *r, struct commit *item)\n+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n+\t\t\t\t const struct object_id *oid)\n {\n \tif (!prepare_commit_graph(r))\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n+\treturn parse_commit_in_graph_one(r, r->objects->commit_graph, shell,\n+\t\t\t\t\t oid);\n }\n \n void load_commit_graph_info(struct repository *r, struct commit *item)\n@@ -1025,7 +1043,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n-\t\tif (!parse_commit_in_graph_one(g, graph_commit))\n+\t\tif (!parse_commit_in_graph_one(r, g, graph_commit, NULL))\n \t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..8b7b5985dc 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -13,16 +13,20 @@ struct commit;\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n- * Given a commit struct, try to fill the commit struct info, including:\n+ * If the given commit (identified by shell->object.oid or oid) is in the\n+ * commit graph, returns a commit struct (reusing shell if it is not NULL)\n+ * including the following info:\n * 1. tree object\n * 2. date\n * 3. parents.\n *\n- * Returns 1 if and only if the commit was found in the packed graph.\n+ * If not, returns NULL. See parse_commit_buffer() for the fallback after this\n+ * call.\n *\n- * See parse_commit_buffer() for the fallback after this call.\n+ * Either shell or oid must be non-NULL. If both are non-NULL, oid is ignored.\n */\n-int parse_commit_in_graph(struct repository *r, struct commit *item);\n+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n+\t\t\t\t const struct object_id *oid);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\ndiff --git a/commit.c b/commit.c\nindex d13a7bc374..88eb580c5a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -456,7 +456,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n+\tif (use_commit_graph && parse_commit_in_graph(the_repository, item, NULL))\n \t\treturn 0;\n \tbuffer = read_object_file(&item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/revision.c b/revision.c\nindex 13e0519c02..05fddb5880 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -213,7 +213,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n {\n \tstruct object *object;\n \n-\tobject = parse_object(revs->repo, oid);\n+\tobject = (struct object *) parse_commit_in_graph(revs->repo, NULL, oid);\n+\tif (!object)\n+\t\tobject = parse_object(revs->repo, oid);\n+\n \tif (!object) {\n \t\tif (revs->ignore_missing)\n \t\t\treturn object;\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex 6a84a53efb..63b928a883 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -22,7 +22,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \n \tc = lookup_commit(&r, commit_oid);\n \n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!parse_commit_in_graph(&r, c, NULL))\n \t\tdie(\"Couldn't parse commit\");\n \n \tprintf(\"%\"PRItime, c->date);\n@@ -52,7 +52,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \t * get_commit_tree_in_graph does not automatically parse the commit, so\n \t * parse it first.\n \t */\n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!parse_commit_in_graph(&r, c, NULL))\n \t\tdie(\"Couldn't parse commit\");\n \ttree = get_commit_tree_in_graph(&r, c);\n \tif (!tree)\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364826","messageId":"xmqqwooj5xpr.fsf@gitster-ct.c.googlers.com","threadId":"49947","inReplyTo":"20181207215034.213211-1-jonathantanmy@google.com","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T00:51:28Z","receivedAt":"2018-12-09T00:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When fetching into a repository, a connectivity check is first made by\n> check_exist_and_connected() in builtin/fetch.c that runs:\n> ...\n> Another way to accomplish this effect would be to modify parse_object()\n> to use the commit graph if possible; however, I did not want to change\n> parse_object()'s current behavior of always checking the object\n> signature of the returned object.\n\nSounds good.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> This patch is now on master.\n\nOK. \n\nObviously that won't apply to the base for v1 without conflicts, and\nit of course applies cleanly on 'master', but the result of doing so\nwill cause the same conflicts when sb/more-repo-in-api is merged on\ntop, which means that the same conflicts need to be resolved if this\nwants to be merged to 'next' (or 'pu', FWIW).\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 40c855f185..a571b523b7 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -374,24 +375,41 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n> \t}\n> }\n> \n> -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n> +static struct commit *parse_commit_in_graph_one(struct repository *r,\n> +\t\t\t\t\t\tstruct commit_graph *g,\n> +\t\t\t\t\t\tstruct commit *shell,\n> +\t\t\t\t\t\tconst struct object_id *oid)\n\nNow the complexity of the behaviour of this function deserves to be\ndocumented in a comment in front. Let me see if I can get it\ncorrectly without such a comment by explaining the function aloud.\n\nThe caller may or may not have already obtained an in-core commit\nobject for a given object name, so shell could be NULL but otherwise\nit could be used for optimization. When shell==NULL, the function\nlooks up the commit object using the oid parameter instead. The\nreturned in-core commit has the parents etc. filled as if we ran\nparse_commit() on it. If the commit is not yet in the graph, the\ncaller may get a NULL even if the commit exists.\n\n> {\n> \tuint32_t pos;\n> \n> -\tif (item->object.parsed)\n> -\t\treturn 1;\n> +\tif (shell && shell->object.parsed)\n> +\t\treturn shell;\n> \n> -\tif (find_commit_in_graph(item, g, &pos))\n> -\t\treturn fill_commit_in_graph(item, g, pos);\n> +\tif (shell && shell->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n> +\t\tpos = shell->graph_pos;\n> +\t} else if (bsearch_graph(g, shell ? &shell->object.oid : oid, &pos)) {\n> +\t\t/* bsearch_graph sets pos */\n\nPlease spell an empty statement like so:\n\n\t\t; /* comment */\n\n> +\t} else {\n> +\t\treturn NULL;\n\nWe come here when the commit (either came from shell or from oid) is\nnot found by bsearch_graph(). \"Is the caller prepared for it, and\nhow?\" is a natural question a reader would have. Let's read on.\n\n> +\t}\n> \n> -\treturn 0;\n> +\tif (!shell) {\n> +\t\tshell = lookup_commit(r, oid);\n> +\t\tif (!shell)\n> +\t\t\treturn NULL;\n> +\t}\n> +\n> +\tfill_commit_in_graph(shell, g, pos);\n> +\treturn shell;\n> }\n> \n> -int parse_commit_in_graph(struct repository *r, struct commit *item)\n> +struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n> +\t\t\t\t const struct object_id *oid)\n> {\n> \tif (!prepare_commit_graph(r))\n> \t\treturn 0;\n> -\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n> +\treturn parse_commit_in_graph_one(r, r->objects->commit_graph, shell,\n> +\t\t\t\t\t oid);\n> }\n> \n> void load_commit_graph_info(struct repository *r, struct commit *item)\n> @@ -1025,7 +1043,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n> \t\t}\n> \n> \t\tgraph_commit = lookup_commit(r, &cur_oid);\n> -\t\tif (!parse_commit_in_graph_one(g, graph_commit))\n> +\t\tif (!parse_commit_in_graph_one(r, g, graph_commit, NULL))\n> \t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n> \t\t\t\t oid_to_hex(&cur_oid));\n> \t}\n> diff --git a/commit-graph.h b/commit-graph.h\n> index 9db40b4d3a..8b7b5985dc 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -13,16 +13,20 @@ struct commit;\n> char *get_commit_graph_filename(const char *obj_dir);\n> \n> /*\n> - * Given a commit struct, try to fill the commit struct info, including:\n> + * If the given commit (identified by shell->object.oid or oid) is in the\n> + * commit graph, returns a commit struct (reusing shell if it is not NULL)\n> + * including the following info:\n> * 1. tree object\n> * 2. date\n> * 3. parents.\n> *\n> - * Returns 1 if and only if the commit was found in the packed graph.\n> + * If not, returns NULL. See parse_commit_buffer() for the fallback after this\n> + * call.\n> *\n> - * See parse_commit_buffer() for the fallback after this call.\n> + * Either shell or oid must be non-NULL. If both are non-NULL, oid is ignored.\n> */\n\nOK, the eventual caller is the caller of this thing, which should\nhave been prepared to see NULL for a commit that is too new. So\nthat should be OK.\n\n> -int parse_commit_in_graph(struct repository *r, struct commit *item);\n> +struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n> +\t\t\t\t const struct object_id *oid);\n> \n> /*\n> * It is possible that we loaded commit contents from the commit buffer,\n> diff --git a/commit.c b/commit.c\n> index d13a7bc374..88eb580c5a 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -456,7 +456,7 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n> \t\treturn -1;\n> \tif (item->object.parsed)\n> \t\treturn 0;\n> -\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n> +\tif (use_commit_graph && parse_commit_in_graph(the_repository, item, NULL))\n> \t\treturn 0;\n> \tbuffer = read_object_file(&item->object.oid, &type, &size);\n> \tif (!buffer)\n> diff --git a/revision.c b/revision.c\n> index 13e0519c02..05fddb5880 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -213,7 +213,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> {\n> \tstruct object *object;\n> \n> -\tobject = parse_object(revs->repo, oid);\n> +\tobject = (struct object *) parse_commit_in_graph(revs->repo, NULL, oid);\n> +\tif (!object)\n> +\t\tobject = parse_object(revs->repo, oid);\n\nOK and this is such a caller. I think a general rule of thumb is\nthat we need to access recent history a lot more often than the\nolder part of the history, and having to fall back for more recent\ncommits feels a bit disturbing, but I do not see an easy way to\nreverse the performance characteristics offhand.\n"},{"id":"364831","messageId":"xmqqbm5v5v0f.fsf@gitster-ct.c.googlers.com","threadId":"49947","inReplyTo":"xmqqwooj5xpr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T01:49:52Z","receivedAt":"2018-12-09T01:50:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Tan writes:\n>\n>> When fetching into a repository, a connectivity check is first made by\n>> check_exist_and_connected() in builtin/fetch.c that runs:\n>> ...\n>> Another way to accomplish this effect would be to modify parse_object()\n>> to use the commit graph if possible; however, I did not want to change\n>> parse_object()'s current behavior of always checking the object\n>> signature of the returned object.\n>\n> Sounds good.\n>\n>> Signed-off-by: Jonathan Tan \n>> ---\n>> This patch is now on master.\n>\n> OK. \n>\n> Obviously that won't apply to the base for v1 without conflicts, and\n> it of course applies cleanly on 'master', but the result of doing so\n> will cause the same conflicts when sb/more-repo-in-api is merged on\n> top, which means that the same conflicts need to be resolved if this\n> wants to be merged to 'next' (or 'pu', FWIW).\n\nSo,... as I had to do the reverse rebase anyway, here is the\ndifference since the previous round, which I came up with by\ncomparing these two:\n\n (A) merge 'sb/more-repo-in-api' to 'master' and then merge v1 of\n this topic to the result.\n\n (B) apply your patch to 'master', and then merge\n 'sb/more-repo-in-api' to the result.\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f78a8e96b5..74a17789f8 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -286,7 +286,8 @@ void close_commit_graph(struct repository *r)\n \tr->objects->commit_graph = NULL;\n }\n \n-static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n+static int bsearch_graph(struct commit_graph *g, const struct object_id *oid,\n+\t\t\t uint32_t *pos)\n {\n \treturn bsearch_hash(oid->hash, g->chunk_oid_fanout,\n \t\t\t g->chunk_oid_lookup, g->hash_len, pos);\n@@ -377,26 +378,41 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n \t}\n }\n \n-static int parse_commit_in_graph_one(struct repository *r,\n-\t\t\t\t struct commit_graph *g,\n-\t\t\t\t struct commit *item)\n+static struct commit *parse_commit_in_graph_one(struct repository *r,\n+\t\t\t\t\t\tstruct commit_graph *g,\n+\t\t\t\t\t\tstruct commit *shell,\n+\t\t\t\t\t\tconst struct object_id *oid)\n {\n \tuint32_t pos;\n \n-\tif (item->object.parsed)\n-\t\treturn 1;\n+\tif (shell && shell->object.parsed)\n+\t\treturn shell;\n \n-\tif (find_commit_in_graph(item, g, &pos))\n-\t\treturn fill_commit_in_graph(r, item, g, pos);\n+\tif (shell && shell->graph_pos != COMMIT_NOT_FROM_GRAPH) {\n+\t\tpos = shell->graph_pos;\n+\t} else if (bsearch_graph(g, shell ? &shell->object.oid : oid, &pos)) {\n+\t\t/* bsearch_graph sets pos */\n+\t} else {\n+\t\treturn NULL;\n+\t}\n \n-\treturn 0;\n+\tif (!shell) {\n+\t\tshell = lookup_commit(r, oid);\n+\t\tif (!shell)\n+\t\t\treturn NULL;\n+\t}\n+\n+\tfill_commit_in_graph(r, shell, g, pos);\n+\treturn shell;\n }\n \n-int parse_commit_in_graph(struct repository *r, struct commit *item)\n+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n+\t\t\t\t const struct object_id *oid)\n {\n \tif (!prepare_commit_graph(r))\n \t\treturn 0;\n-\treturn parse_commit_in_graph_one(r, r->objects->commit_graph, item);\n+\treturn parse_commit_in_graph_one(r, r->objects->commit_graph, shell,\n+\t\t\t\t\t oid);\n }\n \n void load_commit_graph_info(struct repository *r, struct commit *item)\n@@ -1033,7 +1049,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n-\t\tif (!parse_commit_in_graph_one(r, g, graph_commit))\n+\t\tif (!parse_commit_in_graph_one(r, g, graph_commit, NULL))\n \t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..8b7b5985dc 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -13,16 +13,20 @@ struct commit;\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n- * Given a commit struct, try to fill the commit struct info, including:\n+ * If the given commit (identified by shell->object.oid or oid) is in the\n+ * commit graph, returns a commit struct (reusing shell if it is not NULL)\n+ * including the following info:\n * 1. tree object\n * 2. date\n * 3. parents.\n *\n- * Returns 1 if and only if the commit was found in the packed graph.\n+ * If not, returns NULL. See parse_commit_buffer() for the fallback after this\n+ * call.\n *\n- * See parse_commit_buffer() for the fallback after this call.\n+ * Either shell or oid must be non-NULL. If both are non-NULL, oid is ignored.\n */\n-int parse_commit_in_graph(struct repository *r, struct commit *item);\n+struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell,\n+\t\t\t\t const struct object_id *oid);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\ndiff --git a/commit.c b/commit.c\nindex a5333c7ac6..da7a1d3262 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -462,7 +462,7 @@ int repo_parse_commit_internal(struct repository *r,\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(r, item))\n+\tif (use_commit_graph && parse_commit_in_graph(r, item, NULL))\n \t\treturn 0;\n \tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/revision.c b/revision.c\nindex 22aa109c14..05fddb5880 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -213,19 +213,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n {\n \tstruct object *object;\n \n-\t/*\n-\t * If the repository has commit graphs, repo_parse_commit() avoids\n-\t * reading the object buffer, so use it whenever possible.\n-\t */\n-\tif (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n-\t\tstruct commit *c = lookup_commit(revs->repo, oid);\n-\t\tif (!repo_parse_commit(revs->repo, c))\n-\t\t\tobject = (struct object *) c;\n-\t\telse\n-\t\t\tobject = NULL;\n-\t} else {\n+\tobject = (struct object *) parse_commit_in_graph(revs->repo, NULL, oid);\n+\tif (!object)\n \t\tobject = parse_object(revs->repo, oid);\n-\t}\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex f7f8618445..689a0b652e 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -27,7 +27,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \n \tc = lookup_commit(&r, commit_oid);\n \n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!parse_commit_in_graph(&r, c, NULL))\n \t\tdie(\"Couldn't parse commit\");\n \n \tprintf(\"%\"PRItime, c->date);\n@@ -62,7 +62,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \t * get_commit_tree_in_graph does not automatically parse the commit, so\n \t * parse it first.\n \t */\n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!parse_commit_in_graph(&r, c, NULL))\n \t\tdie(\"Couldn't parse commit\");\n \ttree = get_commit_tree_in_graph(&r, c);\n \tif (!tree)\n\n"},{"id":"365062","messageId":"20181211105439.GA8452@sigill.intra.peff.net","threadId":"49947","inReplyTo":"xmqqwooj5xpr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:54:40Z","receivedAt":"2018-12-11T10:55:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 09, 2018 at 09:51:28AM +0900, Junio C Hamano wrote:\n\n> > -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n> > +static struct commit *parse_commit_in_graph_one(struct repository *r,\n> > +\t\t\t\t\t\tstruct commit_graph *g,\n> > +\t\t\t\t\t\tstruct commit *shell,\n> > +\t\t\t\t\t\tconst struct object_id *oid)\n> \n> Now the complexity of the behaviour of this function deserves to be\n> documented in a comment in front. Let me see if I can get it\n> correctly without such a comment by explaining the function aloud.\n> \n> The caller may or may not have already obtained an in-core commit\n> object for a given object name, so shell could be NULL but otherwise\n> it could be used for optimization. When shell==NULL, the function\n> looks up the commit object using the oid parameter instead. The\n> returned in-core commit has the parents etc. filled as if we ran\n> parse_commit() on it. If the commit is not yet in the graph, the\n> caller may get a NULL even if the commit exists.\n\nYeah, this was the part that took me a bit to figure out, as well. The\noptimization here is really just avoiding a call to lookup_commit(),\nwhich will do a single hash-table lookup. I wonder if that's actually\nworth this more complex interface (as opposed to just always taking an\noid and then always returning a \"struct commit\", which could be old or\nnew).\n\n-Peff\n"},{"id":"365196","messageId":"20181212195812.232726-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"20181211105439.GA8452@sigill.intra.peff.net","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-12T19:58:12Z","receivedAt":"2018-12-12T19:58:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Sun, Dec 09, 2018 at 09:51:28AM +0900, Junio C Hamano wrote:\n> \n> > > -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n> > > +static struct commit *parse_commit_in_graph_one(struct repository *r,\n> > > +\t\t\t\t\t\tstruct commit_graph *g,\n> > > +\t\t\t\t\t\tstruct commit *shell,\n> > > +\t\t\t\t\t\tconst struct object_id *oid)\n> > \n> > Now the complexity of the behaviour of this function deserves to be\n> > documented in a comment in front. Let me see if I can get it\n> > correctly without such a comment by explaining the function aloud.\n> > \n> > The caller may or may not have already obtained an in-core commit\n> > object for a given object name, so shell could be NULL but otherwise\n> > it could be used for optimization. When shell==NULL, the function\n> > looks up the commit object using the oid parameter instead. The\n> > returned in-core commit has the parents etc. filled as if we ran\n> > parse_commit() on it. If the commit is not yet in the graph, the\n> > caller may get a NULL even if the commit exists.\n\nIn the next revision, I'll unify parse_commit_in_graph_one() (quoted\nabove) with parse_commit_in_graph(), so that the comment I wrote for the\nlatter can cover the entire functionality. I think the comment covers\nthe details that you outline here.\n\n> Yeah, this was the part that took me a bit to figure out, as well. The\n> optimization here is really just avoiding a call to lookup_commit(),\n> which will do a single hash-table lookup. I wonder if that's actually\n> worth this more complex interface (as opposed to just always taking an\n> oid and then always returning a \"struct commit\", which could be old or\n> new).\n\nAvoidance of lookup_commit() is more important than an optimization, I\nthink. Here, we call lookup_commit() only when we know that that object\nis a commit (by its presence in a commit graph). If we just called it\nblindly, we might mistakenly create a commit for that hash when it is\nactually an object of another type. (We could inline lookup_commit() in\nparse_commit_in_graph_one(), removing the object creation part, but that\nadds complexity as well.)\n"},{"id":"365211","messageId":"20181213012707.GC26210@sigill.intra.peff.net","threadId":"49947","inReplyTo":"20181212195812.232726-1-jonathantanmy@google.com","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-13T01:27:07Z","receivedAt":"2018-12-13T01:27:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 12, 2018 at 11:58:12AM -0800, Jonathan Tan wrote:\n\n> > Yeah, this was the part that took me a bit to figure out, as well. The\n> > optimization here is really just avoiding a call to lookup_commit(),\n> > which will do a single hash-table lookup. I wonder if that's actually\n> > worth this more complex interface (as opposed to just always taking an\n> > oid and then always returning a \"struct commit\", which could be old or\n> > new).\n> \n> Avoidance of lookup_commit() is more important than an optimization, I\n> think. Here, we call lookup_commit() only when we know that that object\n> is a commit (by its presence in a commit graph). If we just called it\n> blindly, we might mistakenly create a commit for that hash when it is\n> actually an object of another type. (We could inline lookup_commit() in\n> parse_commit_in_graph_one(), removing the object creation part, but that\n> adds complexity as well.)\n\nI was thinking we would only do so in the happy path when we find a\ncommit. I.e., something like:\n\n obj = lookup_object(oid); /* does not auto-vivify */\n if (obj && obj->parsed)\n\treturn obj;\n\n if (we_have_it_in_commit_graph) {\n\tcommit = obj || lookup_commit(oid);\n\tfill_in_details_from_commit_graph(commit);\n\treturn &commit->obj;\n } else {\n\treturn parse_object(oid);\n }\n\nwhich is more along the lines of that parse_probably_commit() that\nStolee mentioned.\n\n-Peff\n"},{"id":"365272","messageId":"f1d40014-0e05-5fcb-cedc-e07a22c80628@gmail.com","threadId":"49947","inReplyTo":"20181213012707.GC26210@sigill.intra.peff.net","subject":"Re: [PATCH on master v2] revision: use commit graph in get_reference()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-12-13T16:20:17Z","receivedAt":"2018-12-13T16:20:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/12/2018 8:27 PM, Jeff King wrote:\n> On Wed, Dec 12, 2018 at 11:58:12AM -0800, Jonathan Tan wrote:\n>\n>>> Yeah, this was the part that took me a bit to figure out, as well. The\n>>> optimization here is really just avoiding a call to lookup_commit(),\n>>> which will do a single hash-table lookup. I wonder if that's actually\n>>> worth this more complex interface (as opposed to just always taking an\n>>> oid and then always returning a \"struct commit\", which could be old or\n>>> new).\n>> Avoidance of lookup_commit() is more important than an optimization, I\n>> think. Here, we call lookup_commit() only when we know that that object\n>> is a commit (by its presence in a commit graph). If we just called it\n>> blindly, we might mistakenly create a commit for that hash when it is\n>> actually an object of another type. (We could inline lookup_commit() in\n>> parse_commit_in_graph_one(), removing the object creation part, but that\n>> adds complexity as well.)\n> I was thinking we would only do so in the happy path when we find a\n> commit. I.e., something like:\n>\n> obj = lookup_object(oid); /* does not auto-vivify */\n> if (obj && obj->parsed)\n> \treturn obj;\n>\n> if (we_have_it_in_commit_graph) {\n> \tcommit = obj || lookup_commit(oid);\n> \tfill_in_details_from_commit_graph(commit);\n> \treturn &commit->obj;\n> } else {\n> \treturn parse_object(oid);\n> }\n>\n> which is more along the lines of that parse_probably_commit() that\n> Stolee mentioned.\n\nThis approach is what I had in mind. Thanks for making it more concrete!\n\n-Stolee\n\n"},{"id":"365281","messageId":"20181213185450.230953-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"[PATCH v3] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-13T18:54:50Z","receivedAt":"2018-12-13T18:54:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching into a repository, a connectivity check is first made by\ncheck_exist_and_connected() in builtin/fetch.c that runs:\n\n git rev-list --objects --stdin --not --all --quiet <(list of objects)\n\nIf the client repository has many refs, this command can be slow,\nregardless of the nature of the server repository or what is being\nfetched. A profiler reveals that most of the time is spent in\nsetup_revisions() (approx. 60/63), and of the time spent in\nsetup_revisions(), most of it is spent in parse_object() (approx.\n49/60). This is because setup_revisions() parses the target of every ref\n(from \"--all\"), and parse_object() reads the buffer of the object.\n\nReading the buffer is unnecessary if the repository has a commit graph\nand if the ref points to a commit (which is typically the case). This\npatch uses the commit graph wherever possible; on my computer, when I\nrun the above command with a list of 1 object on a many-ref repository,\nI get a speedup from 1.8s to 1.0s.\n\nAnother way to accomplish this effect would be to modify parse_object()\nto use the commit graph if possible; however, I did not want to change\nparse_object()'s current behavior of always checking the object\nsignature of the returned object.\n\nSigned-off-by: Jonathan Tan \n---\nThis patch is still on master. Junio, let me know if you would rather\nhave me base it on sb/more-repo-in-api instead.\n\nI mentioned [1] that I would unify parse_commit_in_graph_one() and\nparse_commit_in_graph() so that one documentation comment could cover\nall the functionality, but with the simpler API, I decided not to do\nthat to minimize the diff.\n\nChange in v3: Now uses a simpler API with the algorithm suggested by\nPeff in [2], except that I also retain the existing optimization that\nchecks if graph_pos is already set.\n\n[1] https://public-inbox.org/git/20181212195812.232726-1-jonathantanmy@google.com/\n[2] https://public-inbox.org/git/20181213012707.GC26210@sigill.intra.peff.net/\n---\n commit-graph.c | 44 ++++++++++++++++++++++++++------------\n commit-graph.h | 11 +++++-----\n commit.c | 4 +++-\n revision.c | 5 ++++-\n t/helper/test-repository.c | 8 ++-----\n 5 files changed, 45 insertions(+), 27 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 40c855f185..0aca7ec0fe 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -286,7 +286,8 @@ void close_commit_graph(struct repository *r)\n \tr->objects->commit_graph = NULL;\n }\n \n-static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n+static int bsearch_graph(struct commit_graph *g, const struct object_id *oid,\n+\t\t\t uint32_t *pos)\n {\n \treturn bsearch_hash(oid->hash, g->chunk_oid_fanout,\n \t\t\t g->chunk_oid_lookup, g->hash_len, pos);\n@@ -374,24 +375,42 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin\n \t}\n }\n \n-static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n+static struct commit *parse_commit_in_graph_one(struct repository *r,\n+\t\t\t\t\t\tstruct commit_graph *g,\n+\t\t\t\t\t\tconst struct object_id *oid)\n {\n+\tstruct object *obj;\n+\tstruct commit *commit;\n \tuint32_t pos;\n \n-\tif (item->object.parsed)\n-\t\treturn 1;\n+\tobj = lookup_object(r, oid->hash);\n+\tcommit = obj && obj->type == OBJ_COMMIT ? (struct commit *) obj : NULL;\n+\tif (commit && obj->parsed)\n+\t\treturn commit;\n \n-\tif (find_commit_in_graph(item, g, &pos))\n-\t\treturn fill_commit_in_graph(item, g, pos);\n+\tif (commit && commit->graph_pos != COMMIT_NOT_FROM_GRAPH)\n+\t\tpos = commit->graph_pos;\n+\telse if (bsearch_graph(g, oid, &pos))\n+\t\t; /* bsearch_graph sets pos */\n+\telse\n+\t\treturn NULL;\n \n-\treturn 0;\n+\tif (!commit) {\n+\t\tcommit = lookup_commit(r, oid);\n+\t\tif (!commit)\n+\t\t\treturn NULL;\n+\t}\n+\n+\tfill_commit_in_graph(commit, g, pos);\n+\treturn commit;\n }\n \n-int parse_commit_in_graph(struct repository *r, struct commit *item)\n+struct commit *parse_commit_in_graph(struct repository *r,\n+\t\t\t\t const struct object_id *oid)\n {\n \tif (!prepare_commit_graph(r))\n-\t\treturn 0;\n-\treturn parse_commit_in_graph_one(r->objects->commit_graph, item);\n+\t\treturn NULL;\n+\treturn parse_commit_in_graph_one(r, r->objects->commit_graph, oid);\n }\n \n void load_commit_graph_info(struct repository *r, struct commit *item)\n@@ -1004,8 +1023,6 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t}\n \n \tfor (i = 0; i < g->num_commits; i++) {\n-\t\tstruct commit *graph_commit;\n-\n \t\thashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);\n \n \t\tif (i && oidcmp(&prev_oid, &cur_oid) >= 0)\n@@ -1024,8 +1041,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g)\n \t\t\tcur_fanout_pos++;\n \t\t}\n \n-\t\tgraph_commit = lookup_commit(r, &cur_oid);\n-\t\tif (!parse_commit_in_graph_one(g, graph_commit))\n+\t\tif (!parse_commit_in_graph_one(r, g, &cur_oid))\n \t\t\tgraph_report(\"failed to parse %s from commit-graph\",\n \t\t\t\t oid_to_hex(&cur_oid));\n \t}\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 9db40b4d3a..b67aac1125 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -13,16 +13,17 @@ struct commit;\n char *get_commit_graph_filename(const char *obj_dir);\n \n /*\n- * Given a commit struct, try to fill the commit struct info, including:\n+ * If the given commit is in the commit graph, returns a commit struct\n+ * including the following info:\n * 1. tree object\n * 2. date\n * 3. parents.\n *\n- * Returns 1 if and only if the commit was found in the packed graph.\n- *\n- * See parse_commit_buffer() for the fallback after this call.\n+ * If not, returns NULL. See parse_commit_buffer() for the fallback after this\n+ * call.\n */\n-int parse_commit_in_graph(struct repository *r, struct commit *item);\n+struct commit *parse_commit_in_graph(struct repository *r,\n+\t\t\t\t const struct object_id *oid);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\ndiff --git a/commit.c b/commit.c\nindex d13a7bc374..19ce5e34a2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -456,7 +456,9 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n+\tif (use_commit_graph &&\n+\t parse_commit_in_graph(the_repository, &item->object.oid) &&\n+\t item->object.parsed)\n \t\treturn 0;\n \tbuffer = read_object_file(&item->object.oid, &type, &size);\n \tif (!buffer)\ndiff --git a/revision.c b/revision.c\nindex 13e0519c02..7f54f3b4c7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -213,7 +213,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n {\n \tstruct object *object;\n \n-\tobject = parse_object(revs->repo, oid);\n+\tobject = (struct object *) parse_commit_in_graph(revs->repo, oid);\n+\tif (!object)\n+\t\tobject = parse_object(revs->repo, oid);\n+\n \tif (!object) {\n \t\tif (revs->ignore_missing)\n \t\t\treturn object;\ndiff --git a/t/helper/test-repository.c b/t/helper/test-repository.c\nindex 6a84a53efb..35bfd1233d 100644\n--- a/t/helper/test-repository.c\n+++ b/t/helper/test-repository.c\n@@ -20,9 +20,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,\n \tif (repo_init(&r, gitdir, worktree))\n \t\tdie(\"Couldn't init repo\");\n \n-\tc = lookup_commit(&r, commit_oid);\n-\n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!(c = parse_commit_in_graph(&r, commit_oid)))\n \t\tdie(\"Couldn't parse commit\");\n \n \tprintf(\"%\"PRItime, c->date);\n@@ -46,13 +44,11 @@ static void test_get_commit_tree_in_graph(const char *gitdir,\n \tif (repo_init(&r, gitdir, worktree))\n \t\tdie(\"Couldn't init repo\");\n \n-\tc = lookup_commit(&r, commit_oid);\n-\n \t/*\n \t * get_commit_tree_in_graph does not automatically parse the commit, so\n \t * parse it first.\n \t */\n-\tif (!parse_commit_in_graph(&r, c))\n+\tif (!(c = parse_commit_in_graph(&r, commit_oid)))\n \t\tdie(\"Couldn't parse commit\");\n \ttree = get_commit_tree_in_graph(&r, c);\n \tif (!tree)\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365323","messageId":"xmqqk1kcdcab.fsf@gitster-ct.c.googlers.com","threadId":"49947","inReplyTo":"20181213185450.230953-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] revision: use commit graph in get_reference()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T03:20:44Z","receivedAt":"2018-12-14T03:20:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When fetching into a repository, a connectivity check is first made by\n> check_exist_and_connected() in builtin/fetch.c that runs:\n>\n> git rev-list --objects --stdin --not --all --quiet <(list of objects)\n>\n> If the client repository has many refs, this command can be slow,\n> regardless of the nature of the server repository or what is being\n> fetched. A profiler reveals that most of the time is spent in\n> setup_revisions() (approx. 60/63), and of the time spent in\n> setup_revisions(), most of it is spent in parse_object() (approx.\n> 49/60). This is because setup_revisions() parses the target of every ref\n> (from \"--all\"), and parse_object() reads the buffer of the object.\n>\n> Reading the buffer is unnecessary if the repository has a commit graph\n> and if the ref points to a commit (which is typically the case). This\n> patch uses the commit graph wherever possible; on my computer, when I\n> run the above command with a list of 1 object on a many-ref repository,\n> I get a speedup from 1.8s to 1.0s.\n>\n> Another way to accomplish this effect would be to modify parse_object()\n> to use the commit graph if possible; however, I did not want to change\n> parse_object()'s current behavior of always checking the object\n> signature of the returned object.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> This patch is still on master. Junio, let me know if you would rather\n> have me base it on sb/more-repo-in-api instead.\n\nUnless we all agree that we will abandon sb/more-repo-in-api,\nrerolling this on 'master' will force me to resolve similar but\ndifferent conflicts every time. Unless we fast-track that other\ntopic, that is, but I do not think that is what you meant to do.\n\n> Change in v3: Now uses a simpler API with the algorithm suggested by\n> Peff in [2], except that I also retain the existing optimization that\n> checks if graph_pos is already set.\n\nOK.\n"},{"id":"365332","messageId":"20181214084528.GC11777@sigill.intra.peff.net","threadId":"49947","inReplyTo":"20181213185450.230953-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] revision: use commit graph in get_reference()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T08:45:28Z","receivedAt":"2018-12-14T08:45:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 10:54:50AM -0800, Jonathan Tan wrote:\n\n> -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)\n> +static struct commit *parse_commit_in_graph_one(struct repository *r,\n> +\t\t\t\t\t\tstruct commit_graph *g,\n> +\t\t\t\t\t\tconst struct object_id *oid)\n\nMaking sure I understand the new logic...\n\n> {\n> +\tstruct object *obj;\n> +\tstruct commit *commit;\n> \tuint32_t pos;\n> \n> -\tif (item->object.parsed)\n> -\t\treturn 1;\n> +\tobj = lookup_object(r, oid->hash);\n> +\tcommit = obj && obj->type == OBJ_COMMIT ? (struct commit *) obj : NULL;\n> +\tif (commit && obj->parsed)\n> +\t\treturn commit;\n\nOK, so if it's a commit and we have it parsed, we return that. By using\nlookup_object(), if it's a non-commit, we haven't changed anything.\nGood.\n\n> -\tif (find_commit_in_graph(item, g, &pos))\n> -\t\treturn fill_commit_in_graph(item, g, pos);\n> +\tif (commit && commit->graph_pos != COMMIT_NOT_FROM_GRAPH)\n> +\t\tpos = commit->graph_pos;\n> +\telse if (bsearch_graph(g, oid, &pos))\n> +\t\t; /* bsearch_graph sets pos */\n> +\telse\n> +\t\treturn NULL;\n\nAnd then we try to find it in the commit graph. If we didn't, then we'll\nend up returning NULL. Good.\n\n> -\treturn 0;\n> +\tif (!commit) {\n> +\t\tcommit = lookup_commit(r, oid);\n> +\t\tif (!commit)\n> +\t\t\treturn NULL;\n> +\t}\n\nAnd at this point we found it in the commit graph, so we know it's a\ncommit. lookup_commit() should succeed, but in the off chance that it's\nin the commit graph _and_ we previously found it as a non-commit\n(yikes!), we'll return NULL. That's equivalent to just pretending we\ndidn't find it in the commit graph, and the caller can sort it out (when\nthey read the object, either it will match the previous type, or it\nreally will be a commit and they'll follow the normal complaining path).\nGood.\n\nSo this all makes sense. The one thing we don't do here is actually\nparse an unparsed commit that isn't in the graph, and instead leave that\nto the caller. E.g. get_reference() now does:\n\n> -\tobject = parse_object(revs->repo, oid);\n> +\tobject = (struct object *) parse_commit_in_graph(revs->repo, oid);\n> +\tif (!object)\n> +\t\tobject = parse_object(revs->repo, oid);\n\nIn theory we could save another lookup_object() in parse_object() by\ncombining these steps, but I don't think it's really worth worrying too\nmuch about.\n\nSo overall this looks good to me.\n\n-Peff\n"},{"id":"367664","messageId":"20190125153348.GF6702@szeder.dev","threadId":"49947","inReplyTo":"20181204224238.50966-1-jonathantanmy@google.com","subject":"Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-25T15:33:48Z","receivedAt":"2019-01-25T15:33:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 04, 2018 at 02:42:38PM -0800, Jonathan Tan wrote:\n> When fetching into a repository, a connectivity check is first made by\n> check_exist_and_connected() in builtin/fetch.c that runs:\n> \n> git rev-list --objects --stdin --not --all --quiet <(list of objects)\n> \n> If the client repository has many refs, this command can be slow,\n> regardless of the nature of the server repository or what is being\n> fetched. A profiler reveals that most of the time is spent in\n> setup_revisions() (approx. 60/63), and of the time spent in\n> setup_revisions(), most of it is spent in parse_object() (approx.\n> 49/60). This is because setup_revisions() parses the target of every ref\n> (from \"--all\"), and parse_object() reads the buffer of the object.\n> \n> Reading the buffer is unnecessary if the repository has a commit graph\n> and if the ref points to a commit (which is typically the case). This\n> patch uses the commit graph wherever possible; on my computer, when I\n> run the above command with a list of 1 object on a many-ref repository,\n> I get a speedup from 1.8s to 1.0s.\n> \n> Another way to accomplish this effect would be to modify parse_object()\n> to use the commit graph if possible; however, I did not want to change\n> parse_object()'s current behavior of always checking the object\n> signature of the returned object.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> This is on sb/more-repo-in-api because I'm using the repo_parse_commit()\n> function.\n> \n> A colleague noticed this issue when handling a mirror clone.\n> \n> Looking at the bigger picture, the speed of the connectivity check\n> during a fetch might be further improved by passing only the negotiation\n> tips (obtained through --negotiation-tip) instead of \"--all\". This patch\n> just handles the low-hanging fruit first.\n> ---\n\nI stumbled upon a regression that bisects down to this commit\nec0c5798ee (revision: use commit graph in get_reference(),\n2018-12-04):\n\n $ ~/src/git/bin-wrappers/git version\n git version 2.19.1.566.gec0c5798ee\n $ ~/src/git/bin-wrappers/git commit-graph write --reachable\n Computing commit graph generation numbers: 100% (58994/58994), done.\n $ ~/src/git/bin-wrappers/git status\n HEAD detached at origin/pu\n nothing to commit, working tree clean\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=false describe --dirty\n v2.20.1-833-gcb3b9e7ee3\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --dirty\n v2.20.1-833-gcb3b9e7ee3\n\nIt's all good with only '--dirty', but watch this with '--all\n--dirty':\n\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=false describe --all --dirty\n remotes/origin/pu\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty\n remotes/origin/pu-dirty\n\nIOW if the commit-graph is enabled, then my clean worktree is reported\nas dirty.\n\nAnd to add a cherry on top of my confusion:\n\n $ git checkout v2.20.0\n Previous HEAD position was cb3b9e7ee3 Merge branch 'jh/trace2' into pu\n HEAD is now at 5d826e9729 Git 2.20\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty\n tags/v2.20.0\n\nIt's clean even with '--all' and commit-graph enabled, but watch this:\n\n $ git branch this-will-screw-it-up\n $ ~/src/git/bin-wrappers/git -c core.commitGraph=true describe --all --dirty\n tags/v2.20.0-dirty\n\nHave fun! :)\n\n\n> revision.c | 15 ++++++++++++++-\n> 1 file changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/revision.c b/revision.c\n> index b5108b75ab..e7da2c57ab 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> {\n> \tstruct object *object;\n> \n> -\tobject = parse_object(revs->repo, oid);\n> +\t/*\n> +\t * If the repository has commit graphs, repo_parse_commit() avoids\n> +\t * reading the object buffer, so use it whenever possible.\n> +\t */\n> +\tif (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n> +\t\tstruct commit *c = lookup_commit(revs->repo, oid);\n> +\t\tif (!repo_parse_commit(revs->repo, c))\n> +\t\t\tobject = (struct object *) c;\n> +\t\telse\n> +\t\t\tobject = NULL;\n> +\t} else {\n> +\t\tobject = parse_object(revs->repo, oid);\n> +\t}\n> +\n> \tif (!object) {\n> \t\tif (revs->ignore_missing)\n> \t\t\treturn object;\n> -- \n> 2.19.0.271.gfe8321ec05.dirty\n> \n"},{"id":"367680","messageId":"CAGZ79kZRnuTU3ukP1UdBUZD1x+nubYSwLxYgJse1mcj8JUOa2g@mail.gmail.com","threadId":"49947","inReplyTo":"20190125153348.GF6702@szeder.dev","subject":"Re: Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-25T19:56:38Z","receivedAt":"2019-01-25T19:56:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Have fun! :)\n\n$ git gc\n...\nComputing commit graph generation numbers: 100% (164264/164264), done.\n$ ./git version\ngit version 2.20.1.775.g2313a6b87fe.dirty\n# pu + one commit addressing\n# https://public-inbox.org/git/CAGZ79kaUg3NTRPRi5mLk6ag87iDB_Ltq_kEiLwZ2HGZ+-Vsd8w@mail.gmail.com/\n\n$ ./git -c core.commitGraph=false describe --dirty --all\nremotes/gitgitgadget/pu-1-g03745a36e6\n$ ./git -c core.commitGraph=true describe --dirty --all\nremotes/gitgitgadget/pu-1-g03745a36e6\n$ ./git -c core.commitGraph=true describe --dirty\nv2.20.1-776-g03745a36e6\n$ ./git -c core.commitGraph=false describe --dirty\nv2.20.1-776-g03745a36e6\n\nit looks like it is working correctly here?\nOr did I miss some hint as in how to setup the reproduction properly?\n"},{"id":"367689","messageId":"20190125220124.68769-1-jonathantanmy@google.com","threadId":"49947","inReplyTo":"CAGZ79kZRnuTU3ukP1UdBUZD1x+nubYSwLxYgJse1mcj8JUOa2g@mail.gmail.com","subject":"Re: Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-25T22:01:24Z","receivedAt":"2019-01-25T22:01:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Have fun! :)\n> \n> $ git gc\n> ...\n> Computing commit graph generation numbers: 100% (164264/164264), done.\n> $ ./git version\n> git version 2.20.1.775.g2313a6b87fe.dirty\n> # pu + one commit addressing\n> # https://public-inbox.org/git/CAGZ79kaUg3NTRPRi5mLk6ag87iDB_Ltq_kEiLwZ2HGZ+-Vsd8w@mail.gmail.com/\n> \n> $ ./git -c core.commitGraph=false describe --dirty --all\n> remotes/gitgitgadget/pu-1-g03745a36e6\n> $ ./git -c core.commitGraph=true describe --dirty --all\n> remotes/gitgitgadget/pu-1-g03745a36e6\n> $ ./git -c core.commitGraph=true describe --dirty\n> v2.20.1-776-g03745a36e6\n> $ ./git -c core.commitGraph=false describe --dirty\n> v2.20.1-776-g03745a36e6\n> \n> it looks like it is working correctly here?\n> Or did I miss some hint as in how to setup the reproduction properly?\n\nI could reproduce it with version ec0c5798ee (as stated in Szeder's\noriginal email) - as stated by Szeder, it doesn't work, but its parent\ndoes. I'm looking into this, but any help is appreciated.\n"},{"id":"367690","messageId":"20190125221414.GG6702@szeder.dev","threadId":"49947","inReplyTo":"CAGZ79kZRnuTU3ukP1UdBUZD1x+nubYSwLxYgJse1mcj8JUOa2g@mail.gmail.com","subject":"Re: Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-25T22:14:14Z","receivedAt":"2019-01-25T22:14:21Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 25, 2019 at 11:56:38AM -0800, Stefan Beller wrote:\n> > Have fun! :)\n> \n> $ git gc\n> ...\n> Computing commit graph generation numbers: 100% (164264/164264), done.\n> $ ./git version\n> git version 2.20.1.775.g2313a6b87fe.dirty\n> # pu + one commit addressing\n> # https://public-inbox.org/git/CAGZ79kaUg3NTRPRi5mLk6ag87iDB_Ltq_kEiLwZ2HGZ+-Vsd8w@mail.gmail.com/\n> \n> $ ./git -c core.commitGraph=false describe --dirty --all\n> remotes/gitgitgadget/pu-1-g03745a36e6\n> $ ./git -c core.commitGraph=true describe --dirty --all\n> remotes/gitgitgadget/pu-1-g03745a36e6\n> $ ./git -c core.commitGraph=true describe --dirty\n> v2.20.1-776-g03745a36e6\n> $ ./git -c core.commitGraph=false describe --dirty\n> v2.20.1-776-g03745a36e6\n> \n> it looks like it is working correctly here?\n> Or did I miss some hint as in how to setup the reproduction properly?\n\nHow many refs are pointing to the commits you tried to describe? In\nthe git repo, with an all-encompassing commit-graph it seems to be\nimportant that more than one refs point there. I could reproduce the\nissue in a fresh git.git clone with Git built from commit 2313a6b87fe:\n\n $ git clone https://github.com/git/git\n Cloning into 'git'...\n <...>\n $ git commit-graph write --reachable\n Computing commit graph generation numbers: 100% (56722/56722), done.\n # 'HOME=.' makes sure that this command doesn't read my global\n # gitconfig.\n $ HOME=. ~/src/git/git describe --all --dirty\n heads/master-dirty\n $ git checkout origin/pu \n HEAD is now at cb3b9e7ee3 Merge branch 'jh/trace2' into pu\n $ HOME=. ~/src/git/git -c core.commitGraph=true describe --all --dirty\n remotes/origin/pu\n $ git branch a-second-ref-pointing-at-pu buzz ~/src/tmp/git\n $ HOME=. ~/src/git/git -c core.commitGraph=true describe --all --dirty\n heads/a-second-ref-pointing-at-pu-dirty\n\nI could also reproduce it in other repositories lying around here, but\ncould not manage to reproduce it in a minimal repository.\n\nThe smallest I could get is the test script below, where the last test\nfails, i.e. the clean worktree is described as '-dirty', when the\nto-be-described HEAD is not in the commit-graph. I suspect this is\nthe same issue, because it bisects down to this same commit.\n\n --- >8 ---\n\nSubject: [PATCH] test\n\n---\n t/t9999-test.sh | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n create mode 100755 t/t9999-test.sh\n\ndiff --git a/t/t9999-test.sh b/t/t9999-test.sh\nnew file mode 100755\nindex 0000000000..cd1286e157\n--- /dev/null\n+++ b/t/t9999-test.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='test'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\t# Two refs point there.\n+\tgit for-each-ref --points-at=two &&\n+\tgit config core.commitGraph true\n+'\n+\n+test_expect_success 'full commit-graph' '\n+\tgit commit-graph write --reachable &&\n+\tverbose test \"$(git describe --all --dirty)\" = tags/two\n+'\n+\n+test_expect_success 'partial commit-graph, described HEAD is not in C-G' '\n+\tgit rev-parse one | git commit-graph write --stdin-commits &&\n+\tgit status &&\n+\tverbose test \"$(git describe --all --dirty)\" = tags/two\n+'\n+\n+test_done\n-- \n2.20.1.642.gc55a771460\n\n"},{"id":"367692","messageId":"20190125222126.GH6702@szeder.dev","threadId":"49947","inReplyTo":"20190125221414.GG6702@szeder.dev","subject":"Re: Regression in: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-25T22:21:26Z","receivedAt":"2019-01-25T22:21:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 25, 2019 at 11:14:14PM +0100, SZEDER Gábor wrote:\n> On Fri, Jan 25, 2019 at 11:56:38AM -0800, Stefan Beller wrote:\n> > > Have fun! :)\n> > \n> > $ git gc\n> > ...\n> > Computing commit graph generation numbers: 100% (164264/164264), done.\n> > $ ./git version\n> > git version 2.20.1.775.g2313a6b87fe.dirty\n> > # pu + one commit addressing\n> > # https://public-inbox.org/git/CAGZ79kaUg3NTRPRi5mLk6ag87iDB_Ltq_kEiLwZ2HGZ+-Vsd8w@mail.gmail.com/\n> > \n> > $ ./git -c core.commitGraph=false describe --dirty --all\n> > remotes/gitgitgadget/pu-1-g03745a36e6\n> > $ ./git -c core.commitGraph=true describe --dirty --all\n> > remotes/gitgitgadget/pu-1-g03745a36e6\n> > $ ./git -c core.commitGraph=true describe --dirty\n> > v2.20.1-776-g03745a36e6\n> > $ ./git -c core.commitGraph=false describe --dirty\n> > v2.20.1-776-g03745a36e6\n> > \n> > it looks like it is working correctly here?\n> > Or did I miss some hint as in how to setup the reproduction properly?\n> \n> How many refs are pointing to the commits you tried to describe? In\n> the git repo, with an all-encompassing commit-graph it seems to be\n> important that more than one refs point there.\n\nErm, let me try to clarify this sentence.\n\nIn general it seems to be important that more than one refs point to\nthe described HEAD. In the git repo (and in other non-toy repos) I\ncould reproduce the issue with a commit-graph file containing all\ncommits, but in a minimal repo only when the described HEAD was not in\nthe commit-graph.\n\n> I could reproduce the\n> issue in a fresh git.git clone with Git built from commit 2313a6b87fe:\n> \n> $ git clone https://github.com/git/git\n> Cloning into 'git'...\n> <...>\n> $ git commit-graph write --reachable\n> Computing commit graph generation numbers: 100% (56722/56722), done.\n> # 'HOME=.' makes sure that this command doesn't read my global\n> # gitconfig.\n> $ HOME=. ~/src/git/git describe --all --dirty\n> heads/master-dirty\n> $ git checkout origin/pu \n> HEAD is now at cb3b9e7ee3 Merge branch 'jh/trace2' into pu\n> $ HOME=. ~/src/git/git -c core.commitGraph=true describe --all --dirty\n> remotes/origin/pu\n> $ git branch a-second-ref-pointing-at-pu buzz ~/src/tmp/git\n> $ HOME=. ~/src/git/git -c core.commitGraph=true describe --all --dirty\n> heads/a-second-ref-pointing-at-pu-dirty\n> \n> I could also reproduce it in other repositories lying around here, but\n> could not manage to reproduce it in a minimal repository.\n> \n> The smallest I could get is the test script below, where the last test\n> fails, i.e. the clean worktree is described as '-dirty', when the\n> to-be-described HEAD is not in the commit-graph. I suspect this is\n> the same issue, because it bisects down to this same commit.\n> \n> --- >8 ---\n> \n> Subject: [PATCH] test\n> \n> ---\n> t/t9999-test.sh | 26 ++++++++++++++++++++++++++\n> 1 file changed, 26 insertions(+)\n> create mode 100755 t/t9999-test.sh\n> \n> diff --git a/t/t9999-test.sh b/t/t9999-test.sh\n> new file mode 100755\n> index 0000000000..cd1286e157\n> --- /dev/null\n> +++ b/t/t9999-test.sh\n> @@ -0,0 +1,26 @@\n> +#!/bin/sh\n> +\n> +test_description='test'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\t# Two refs point there.\n> +\tgit for-each-ref --points-at=two &&\n> +\tgit config core.commitGraph true\n> +'\n> +\n> +test_expect_success 'full commit-graph' '\n> +\tgit commit-graph write --reachable &&\n> +\tverbose test \"$(git describe --all --dirty)\" = tags/two\n> +'\n> +\n> +test_expect_success 'partial commit-graph, described HEAD is not in C-G' '\n> +\tgit rev-parse one | git commit-graph write --stdin-commits &&\n> +\tgit status &&\n> +\tverbose test \"$(git describe --all --dirty)\" = tags/two\n> +'\n> +\n> +test_done\n> -- \n> 2.20.1.642.gc55a771460\n> \n"},{"id":"367751","messageId":"20190127130832.23652-1-szeder.dev@gmail.com","threadId":"49947","inReplyTo":"20190125222126.GH6702@szeder.dev","subject":"[PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-27T13:08:32Z","receivedAt":"2019-01-27T13:08:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When the commit graph and generation numbers were introduced in\ncommits 177722b344 (commit: integrate commit graph with commit\nparsing, 2018-04-10) and 83073cc994 (commit: add generation number to\nstruct commit, 2018-04-25), they tried to make sure that the\ncorresponding 'graph_pos' and 'generation' fields of 'struct commit'\nare initialized conservatively, as if the commit were not included in\nthe commit-graph file.\n\nAlas, initializing those fields only in alloc_commit_node() missed the\ncase when an object that happens to be a commit is first looked up via\nlookup_unknown_object(), and is then later converted to a 'struct\ncommit' via the object_as_type() helper function (either calling it\ndirectly, or as part of a subsequent lookup_commit() call).\nConsequently, both of those fields incorrectly remain set to zero,\nwhich means e.g. that the commit is present in and is the first entry\nof the commit-graph file. This will result in wrong timestamp, parent\nand root tree hashes, if such a 'struct commit' instance is later\nfilled from the commit-graph.\n\nExtract the initialization of 'struct commit's fields from\nalloc_commit_node() into a helper function, and call it from\nobject_as_type() as well, to make sure that it properly initializes\nthe two commit-graph-related fields, too. With this helper function\nit is hopefully less likely that any new fields added to 'struct\ncommit' in the future would remain uninitialized.\n\nWith this change alloc_commit_index() won't have any remaining callers\noutside of 'alloc.c', so mark it as static.\n\nSigned-off-by: SZEDER Gábor \n---\n\nSo, it turns out that ec0c5798ee (revision: use commit graph in\nget_reference(), 2018-12-04) is not the culprit after all, it merely\nhighlighted a bug that is as old as the commit-graph feature itself.\nThis patch fixes this and all other related issues I reported\nupthread.\n\nI couldn't find any other place where an object of unknown type is\nturned into a 'struct commit', so this might have been the only place\nthat needed fixing.\n\nOther object types seem to be fine, because they contain only fields\nthat should be zero initialized. At least for now, because a similar\nissue might arise in the future, if one of them gains a new field that\nshould not be initialized to zero... but will they ever get such a\nfield? So I'm not too keen on introducing similar init_tree_node(),\netc. helper funcions at the moment.\n\n alloc.c | 11 ++++++++---\n alloc.h | 2 +-\n object.c | 5 +++--\n 3 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex e7aa81b7aa..1c64c4dd16 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -99,18 +99,23 @@ void *alloc_object_node(struct repository *r)\n \treturn obj;\n }\n \n-unsigned int alloc_commit_index(struct repository *r)\n+static unsigned int alloc_commit_index(struct repository *r)\n {\n \treturn r->parsed_objects->commit_count++;\n }\n \n-void *alloc_commit_node(struct repository *r)\n+void init_commit_node(struct repository *r, struct commit *c)\n {\n-\tstruct commit *c = alloc_node(r->parsed_objects->commit_state, sizeof(struct commit));\n \tc->object.type = OBJ_COMMIT;\n \tc->index = alloc_commit_index(r);\n \tc->graph_pos = COMMIT_NOT_FROM_GRAPH;\n \tc->generation = GENERATION_NUMBER_INFINITY;\n+}\n+\n+void *alloc_commit_node(struct repository *r)\n+{\n+\tstruct commit *c = alloc_node(r->parsed_objects->commit_state, sizeof(struct commit));\n+\tinit_commit_node(r, c);\n \treturn c;\n }\n \ndiff --git a/alloc.h b/alloc.h\nindex ba356ed847..ed1071c11e 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -9,11 +9,11 @@ struct repository;\n \n void *alloc_blob_node(struct repository *r);\n void *alloc_tree_node(struct repository *r);\n+void init_commit_node(struct repository *r, struct commit *c);\n void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n void alloc_report(struct repository *r);\n-unsigned int alloc_commit_index(struct repository *r);\n \n struct alloc_state *allocate_alloc_state(void);\n void clear_alloc_state(struct alloc_state *s);\ndiff --git a/object.c b/object.c\nindex c4170d2d0c..7bccfd5d8e 100644\n--- a/object.c\n+++ b/object.c\n@@ -164,8 +164,9 @@ void *object_as_type(struct repository *r, struct object *obj, enum object_type\n \t\treturn obj;\n \telse if (obj->type == OBJ_NONE) {\n \t\tif (type == OBJ_COMMIT)\n-\t\t\t((struct commit *)obj)->index = alloc_commit_index(r);\n-\t\tobj->type = type;\n+\t\t\tinit_commit_node(r, (struct commit *) obj);\n+\t\telse\n+\t\t\tobj->type = type;\n \t\treturn obj;\n \t}\n \telse {\n-- \n2.20.1.642.gc55a771460\n\n"},{"id":"367752","messageId":"20190127132854.GI6702@szeder.dev","threadId":"49947","inReplyTo":"20190127130832.23652-1-szeder.dev@gmail.com","subject":"Re: [PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-27T13:28:54Z","receivedAt":"2019-01-27T13:29:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Jan 27, 2019 at 02:08:32PM +0100, SZEDER Gábor wrote:\n> When the commit graph and generation numbers were introduced in\n> commits 177722b344 (commit: integrate commit graph with commit\n> parsing, 2018-04-10) and 83073cc994 (commit: add generation number to\n> struct commit, 2018-04-25), they tried to make sure that the\n> corresponding 'graph_pos' and 'generation' fields of 'struct commit'\n> are initialized conservatively, as if the commit were not included in\n> the commit-graph file.\n> \n> Alas, initializing those fields only in alloc_commit_node() missed the\n> case when an object that happens to be a commit is first looked up via\n> lookup_unknown_object(), and is then later converted to a 'struct\n> commit' via the object_as_type() helper function (either calling it\n> directly, or as part of a subsequent lookup_commit() call).\n> Consequently, both of those fields incorrectly remain set to zero,\n> which means e.g. that the commit is present in and is the first entry\n> of the commit-graph file. This will result in wrong timestamp, parent\n> and root tree hashes, if such a 'struct commit' instance is later\n> filled from the commit-graph.\n> \n> Extract the initialization of 'struct commit's fields from\n> alloc_commit_node() into a helper function, and call it from\n> object_as_type() as well, to make sure that it properly initializes\n> the two commit-graph-related fields, too. With this helper function\n> it is hopefully less likely that any new fields added to 'struct\n> commit' in the future would remain uninitialized.\n> \n> With this change alloc_commit_index() won't have any remaining callers\n> outside of 'alloc.c', so mark it as static.\n> \n> Signed-off-by: SZEDER Gábor \n> ---\n> \n> So, it turns out that ec0c5798ee (revision: use commit graph in\n> get_reference(), 2018-12-04) is not the culprit after all, it merely\n> highlighted a bug that is as old as the commit-graph feature itself.\n> This patch fixes this and all other related issues I reported\n> upthread.\n\nAnd how/why does this affect 'git describe --dirty'?\n\n - 'git describe' first iterates over all refs, and somewhere deep\n inside for_each_ref() each commit (well, object) a ref points to\n is looked up via lookup_unknown_object(). This leaves all fields\n of the created object zero initialized.\n\n - Then it dereferences HEAD for '--dirty' and ec0c5798ee's changes\n to get_reference() kick in: lookup_commit() doesn't instantiate a\n brand new and freshly initialized 'struct commit', but returns the\n object created in the previous step converted into 'struct\n commit'. This conversion doesn't set the commit-graph fields in\n 'struct commit', but leaves both as zero. get_reference() then\n tries to load HEAD's commit information from the commit-graph,\n find_commit_in_graph() sees the the still zero 'graph_pos' field\n and doesn't perform a search through the commit-graph file, and\n the subsequent fill_commit_in_graph() reads the commit info from\n the first entry.\n\n In case of the failing test I posted earlier, where only the first\n commit is in the commit-graph but HEAD isn't, this means that the\n HEAD's 'struct commit' is filled with the info of HEAD^.\n\n - Ultimately, the diff machinery then doesn't compare the worktree\n to HEAD's tree, but to HEAD^'s, finds that they differ, hence the\n incorrect '-dirty' flag in the output.\n\nBefore ec0c5798ee get_reference() simply called parse_object(), which\nignored the commit-graph, so the issue could remain hidden.\n\n"},{"id":"367757","messageId":"ae9229c7-926c-fb55-4d1d-658c8b6acfc4@gmail.com","threadId":"49947","inReplyTo":"20190127132854.GI6702@szeder.dev","subject":"Re: [PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-01-27T18:40:33Z","receivedAt":"2019-01-27T18:40:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/27/2019 8:28 AM, SZEDER Gábor wrote:\n> On Sun, Jan 27, 2019 at 02:08:32PM +0100, SZEDER Gábor wrote:\n>> When the commit graph and generation numbers were introduced in\n>> commits 177722b344 (commit: integrate commit graph with commit\n>> parsing, 2018-04-10) and 83073cc994 (commit: add generation number to\n>> struct commit, 2018-04-25), they tried to make sure that the\n>> corresponding 'graph_pos' and 'generation' fields of 'struct commit'\n>> are initialized conservatively, as if the commit were not included in\n>> the commit-graph file.\n>>\n>> Alas, initializing those fields only in alloc_commit_node() missed the\n>> case when an object that happens to be a commit is first looked up via\n>> lookup_unknown_object(), and is then later converted to a 'struct\n>> commit' via the object_as_type() helper function (either calling it\n>> directly, or as part of a subsequent lookup_commit() call).\n>> Consequently, both of those fields incorrectly remain set to zero,\n>> which means e.g. that the commit is present in and is the first entry\n>> of the commit-graph file. This will result in wrong timestamp, parent\n>> and root tree hashes, if such a 'struct commit' instance is later\n>> filled from the commit-graph.\n>>\n>> Extract the initialization of 'struct commit's fields from\n>> alloc_commit_node() into a helper function, and call it from\n>> object_as_type() as well, to make sure that it properly initializes\n>> the two commit-graph-related fields, too. With this helper function\n>> it is hopefully less likely that any new fields added to 'struct\n>> commit' in the future would remain uninitialized.\n>>\n>> With this change alloc_commit_index() won't have any remaining callers\n>> outside of 'alloc.c', so mark it as static.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>>\n>> So, it turns out that ec0c5798ee (revision: use commit graph in\n>> get_reference(), 2018-12-04) is not the culprit after all, it merely\n>> highlighted a bug that is as old as the commit-graph feature itself.\n>> This patch fixes this and all other related issues I reported\n>> upthread.\n> \n> And how/why does this affect 'git describe --dirty'?\n> \n> - 'git describe' first iterates over all refs, and somewhere deep\n> inside for_each_ref() each commit (well, object) a ref points to\n> is looked up via lookup_unknown_object(). This leaves all fields\n> of the created object zero initialized.\n> \n> - Then it dereferences HEAD for '--dirty' and ec0c5798ee's changes\n> to get_reference() kick in: lookup_commit() doesn't instantiate a\n> brand new and freshly initialized 'struct commit', but returns the\n> object created in the previous step converted into 'struct\n> commit'. This conversion doesn't set the commit-graph fields in\n> 'struct commit', but leaves both as zero. get_reference() then\n> tries to load HEAD's commit information from the commit-graph,\n> find_commit_in_graph() sees the the still zero 'graph_pos' field\n> and doesn't perform a search through the commit-graph file, and\n> the subsequent fill_commit_in_graph() reads the commit info from\n> the first entry.\n> \n> In case of the failing test I posted earlier, where only the first\n> commit is in the commit-graph but HEAD isn't, this means that the\n> HEAD's 'struct commit' is filled with the info of HEAD^.\n> \n> - Ultimately, the diff machinery then doesn't compare the worktree\n> to HEAD's tree, but to HEAD^'s, finds that they differ, hence the\n> incorrect '-dirty' flag in the output.\n> \n> Before ec0c5798ee get_reference() simply called parse_object(), which\n> ignored the commit-graph, so the issue could remain hidden.\n\nThanks for digging in, Szeder. This is a very subtle interaction, and\nI'm glad you caught the issue. There are likely other ways this could\nbecome problematic, including hitting BUG() statements regarding\ngeneration numbers.\n\nI recommend this be merged to 'maint' if possible.\n\nThanks,\n-Stolee\n\n"},{"id":"367837","messageId":"CAGf8dgLzNJLrT-XW25=1SqJiH47hiTNVHf7-sx8efHw5oMnC7Q@mail.gmail.com","threadId":"49947","inReplyTo":"20190127130832.23652-1-szeder.dev@gmail.com","subject":"Re: [PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-28T16:57:36Z","receivedAt":"2019-01-28T16:57:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Sun, Jan 27, 2019 at 5:08 AM SZEDER Gábor wrote:\n>\n> When the commit graph and generation numbers were introduced in\n> commits 177722b344 (commit: integrate commit graph with commit\n> parsing, 2018-04-10) and 83073cc994 (commit: add generation number to\n> struct commit, 2018-04-25), they tried to make sure that the\n> corresponding 'graph_pos' and 'generation' fields of 'struct commit'\n> are initialized conservatively, as if the commit were not included in\n> the commit-graph file.\n\nThanks for looking into this! The patch looks good to me.\n"},{"id":"367838","messageId":"20190128161503.GC23588@sigill.intra.peff.net","threadId":"49947","inReplyTo":"20190127130832.23652-1-szeder.dev@gmail.com","subject":"Re: [PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-28T16:15:03Z","receivedAt":"2019-01-28T17:01:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 27, 2019 at 02:08:32PM +0100, SZEDER Gábor wrote:\n\n> When the commit graph and generation numbers were introduced in\n> commits 177722b344 (commit: integrate commit graph with commit\n> parsing, 2018-04-10) and 83073cc994 (commit: add generation number to\n> struct commit, 2018-04-25), they tried to make sure that the\n> corresponding 'graph_pos' and 'generation' fields of 'struct commit'\n> are initialized conservatively, as if the commit were not included in\n> the commit-graph file.\n> \n> Alas, initializing those fields only in alloc_commit_node() missed the\n> case when an object that happens to be a commit is first looked up via\n> lookup_unknown_object(), and is then later converted to a 'struct\n> commit' via the object_as_type() helper function (either calling it\n> directly, or as part of a subsequent lookup_commit() call).\n> Consequently, both of those fields incorrectly remain set to zero,\n> which means e.g. that the commit is present in and is the first entry\n> of the commit-graph file. This will result in wrong timestamp, parent\n> and root tree hashes, if such a 'struct commit' instance is later\n> filled from the commit-graph.\n> \n> Extract the initialization of 'struct commit's fields from\n> alloc_commit_node() into a helper function, and call it from\n> object_as_type() as well, to make sure that it properly initializes\n> the two commit-graph-related fields, too. With this helper function\n> it is hopefully less likely that any new fields added to 'struct\n> commit' in the future would remain uninitialized.\n> \n> With this change alloc_commit_index() won't have any remaining callers\n> outside of 'alloc.c', so mark it as static.\n\nGood find, and nicely explained.\n\n> ---\n> \n> So, it turns out that ec0c5798ee (revision: use commit graph in\n> get_reference(), 2018-12-04) is not the culprit after all, it merely\n> highlighted a bug that is as old as the commit-graph feature itself.\n> This patch fixes this and all other related issues I reported\n> upthread.\n> \n> I couldn't find any other place where an object of unknown type is\n> turned into a 'struct commit', so this might have been the only place\n> that needed fixing.\n\nThis should be the only place. We already ran into this with the\ncommit-index field, which was what caused us to create object_as_type()\nin the first place, to give a central place for coercing OBJ_NONE into\nother types.\n\n> Other object types seem to be fine, because they contain only fields\n> that should be zero initialized. At least for now, because a similar\n> issue might arise in the future, if one of them gains a new field that\n> should not be initialized to zero... but will they ever get such a\n> field? So I'm not too keen on introducing similar init_tree_node(),\n> etc. helper funcions at the moment.\n\nAgreed. We can deal with those if it ever becomes necessary. In theory\nadding empty placeholder functions might help somebody realize they'd\nneed to handle this case, but I have the feeling that they'd be as\nlikely to miss init_tree_node() as they would object_as_type().\n\nI dunno. I guess if init_tree_node() were actually called from\nalloc_tree_node(), it might be harder to miss.\n\n> alloc.c | 11 ++++++++---\n> alloc.h | 2 +-\n> object.c | 5 +++--\n> 3 files changed, 12 insertions(+), 6 deletions(-)\n\nThe patch itself looks good to me.\n\n-Peff\n"}]}