{"thread":{"id":"55988","subject":"[PATCH 0/5] some \"log --decorate\" optimizations","startedAt":"2021-06-22T16:03:19Z","lastAt":"2021-06-23T21:52:01Z","messageCount":16,"participants":["Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"428206","messageId":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","threadId":"55988","inReplyTo":null,"subject":"[PATCH 0/5] some \"log --decorate\" optimizations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:03:15Z","receivedAt":"2021-06-22T16:03:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was recently pointed to a repository where:\n\n  git log --format=\"%H %P\" old..new\n\nwas abysmally slow, even though old..new only had a few commits, because\nload_ref_decorations() takes a long time (and kicks in due to the\nauto-decorate behavior). Leaving aside that \"rev-list\" would probably be\na better tool here, it seems there's some low-hanging fruit:\n\n  - a user-format that doesn't show decorations doesn't need to load\n    them (fixed in patch 2 below)\n\n  - we're pretty eager to parse the objects at the tip of each ref\n    (fixed in patch 5 below)\n\nThe other commits are just cleanups and preparatory refactors.\n\n  [1/5]: pretty.h: update and expand docstring for userformat_find_requirements()\n  [2/5]: log: avoid loading decorations for userformats that don't need it\n  [3/5]: object.h: expand docstring for lookup_unknown_object()\n  [4/5]: object.h: add lookup_object_by_type() function\n  [5/5]: load_ref_decorations(): avoid parsing non-tag objects\n\n builtin/log.c |  3 +++\n log-tree.c    |  6 ++++--\n object.c      | 18 ++++++++++++++++++\n object.h      | 20 +++++++++++++++++++-\n pretty.c      |  4 ++++\n pretty.h      |  8 ++++++--\n reachable.c   | 18 ------------------\n 7 files changed, 54 insertions(+), 23 deletions(-)\n\n-Peff\n"},{"id":"428207","messageId":"YNIJ6tZMhn5wZ4Io@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","subject":"[PATCH 1/5] pretty.h: update and expand docstring for userformat_find_requirements()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:03:54Z","receivedAt":"2021-06-22T16:03:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The comment only mentions \"notes\", but there are more fields now (and\nI'm about to add another). Let's make it more general, and stick the\nstruct next to the function to make the list of possibilities obvious.\n\nWhile we're touching this comment, let's also mention the behavior of\nNULL, which some callers rely on (though in the long run, this global is\npretty nasty and probably should get moved into rev_info).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pretty.h | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.h b/pretty.h\nindex f034609e4d..c81cf40d38 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -65,12 +65,15 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)\n \treturn (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);\n }\n \n+/*\n+ * Examine the user-specified format given by \"fmt\" (or if NULL, the global one\n+ * previously saved by get_commit_format()), and set flags based on which items\n+ * the format will need when it is expanded.\n+ */\n struct userformat_want {\n \tunsigned notes:1;\n \tunsigned source:1;\n };\n-\n-/* Set the flag \"w->notes\" if there is placeholder %N in \"fmt\". */\n void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n \n /*\n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428208","messageId":"YNIKItEutZcytcgb@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","subject":"[PATCH 2/5] log: avoid loading decorations for userformats that don't need it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:04:50Z","receivedAt":"2021-06-22T16:04:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If no --decorate option is given, we default to auto-decoration. And\nwhen that kicks in, cmd_log_init_finish() will unconditionally load the\ndecoration refs.\n\nHowever, if we are using a user-format that does not include \"%d\" or\n\"%D\", we won't show the decorations at all, so we don't need to load\nthem. We can detect this case and auto-disable them by adding a new\nfield to our userformat_want helper. We can do this even when the user\nexplicitly asked for --decorate, because it can't affect the output at\nall.\n\nThis patch consistently reduces the time to run \"git log -1 --format=%H\"\non my git.git clone (with ~2k refs) from 34ms to 7ms. On a much more\nextreme real-world repository (with ~220k refs), it goes from 2.5s to\n4ms.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 3 +++\n pretty.c      | 4 ++++\n pretty.h      | 1 +\n 3 files changed, 8 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 6102893fcc..6ba7f20726 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -245,6 +245,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\trev->abbrev_commit = 0;\n \t}\n \n+\tif (rev->commit_format == CMIT_FMT_USERFORMAT && !w.decorate)\n+\t\tdecoration_style = 0;\n+\n \tif (decoration_style) {\n \t\tconst struct string_list *config_exclude =\n \t\t\trepo_config_get_value_multi(the_repository,\ndiff --git a/pretty.c b/pretty.c\nindex b1ecd039ce..9631529c10 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1735,6 +1735,10 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n \tcase 'S':\n \t\tw->source = 1;\n \t\tbreak;\n+\tcase 'd':\n+\tcase 'D':\n+\t\tw->decorate = 1;\n+\t\tbreak;\n \t}\n \treturn 0;\n }\ndiff --git a/pretty.h b/pretty.h\nindex c81cf40d38..2f16acd213 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -73,6 +73,7 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)\n struct userformat_want {\n \tunsigned notes:1;\n \tunsigned source:1;\n+\tunsigned decorate:1;\n };\n void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n \n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428209","messageId":"YNIKSzVjBZgfCAIm@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","subject":"[PATCH 3/5] object.h: expand docstring for lookup_unknown_object()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:05:31Z","receivedAt":"2021-06-22T16:05:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The lookup_unknown_object() system is not often used and is somewhat\nconfusing. Let's try to explain it a bit more (which is especially\nimportant as I'm adding a related but slightly different function in the\nnext commit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object.h | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/object.h b/object.h\nindex 8bca310713..eb7e481c39 100644\n--- a/object.h\n+++ b/object.h\n@@ -144,7 +144,18 @@ struct object *parse_object_or_die(const struct object_id *oid, const char *name\n  */\n struct object *parse_object_buffer(struct repository *r, const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n \n-/** Returns the object, with potentially excess memory allocated. **/\n+/*\n+ * Allocate and return an object struct, even if you do not know the type of\n+ * the object. The returned object may have its \"type\" field set to a real type\n+ * (if somebody previously called lookup_blob(), etc), or it may be set to\n+ * OBJ_NONE. In the latter case, subsequent calls to lookup_blob(), etc, will\n+ * set the type field as appropriate.\n+ *\n+ * Use this when you do not know the expected type of an object and want to\n+ * avoid parsing it for efficiency reasons. Try to avoid it otherwise; it\n+ * may allocate excess memory, since the returned object must be as large as\n+ * the maximum struct of any type.\n+ */\n struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid);\n \n struct object_list *object_list_insert(struct object *item,\n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428211","messageId":"YNIKkY9dBo+6pSwi@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","subject":"[PATCH 4/5] object.h: add lookup_object_by_type() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:06:41Z","receivedAt":"2021-06-22T16:06:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In some cases it's useful for efficiency reasons to get the type of an\nobject before deciding whether to parse it, but we still want an object\nstruct. E.g., in reachable.c, bitmaps give us the type, but we just want\nto mark flags on each object. Likewise, we may loop over every object\nand only parse tags in order to peel them; checking the type first lets\nus avoid parsing the non-tags.\n\nBut our lookup_blob(), etc, functions make getting an object struct\nannoying: we have to call the right function for every type. And we\ncannot just use the generic lookup_object(), because it only returns an\nalready-seen object; it won't allocate a new object struct.\n\nLet's provide a function that dispatches to the correct lookup_*\nfunction based on a run-time type. In fact, reachable.c already has such\na helper, so we'll just make that public.\n\nI did change the return type from \"void *\" to \"struct object *\". While\nthe former is a clever way to avoid casting inside the function, it's\nless safe and less informative to people reading the function\ndeclaration.\n\nThe next commit will add a new caller.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object.c    | 18 ++++++++++++++++++\n object.h    |  7 +++++++\n reachable.c | 18 ------------------\n 3 files changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 14188453c5..07fcf23d7b 100644\n--- a/object.c\n+++ b/object.c\n@@ -185,6 +185,24 @@ struct object *lookup_unknown_object(struct repository *r, const struct object_i\n \treturn obj;\n }\n \n+struct object *lookup_object_by_type(struct repository *r,\n+\t\t\t    const struct object_id *oid,\n+\t\t\t    enum object_type type)\n+{\n+\tswitch (type) {\n+\tcase OBJ_COMMIT:\n+\t\treturn (struct object *)lookup_commit(r, oid);\n+\tcase OBJ_TREE:\n+\t\treturn (struct object *)lookup_tree(r, oid);\n+\tcase OBJ_TAG:\n+\t\treturn (struct object *)lookup_tag(r, oid);\n+\tcase OBJ_BLOB:\n+\t\treturn (struct object *)lookup_blob(r, oid);\n+\tdefault:\n+\t\tdie(\"BUG: unknown object type %d\", type);\n+\t}\n+}\n+\n struct object *parse_object_buffer(struct repository *r, const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p)\n {\n \tstruct object *obj;\ndiff --git a/object.h b/object.h\nindex eb7e481c39..3b38c9cc98 100644\n--- a/object.h\n+++ b/object.h\n@@ -158,6 +158,13 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n  */\n struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid);\n \n+/*\n+ * Dispatch to the appropriate lookup_blob(), lookup_commit(), etc, based on\n+ * \"type\".\n+ */\n+struct object *lookup_object_by_type(struct repository *r, const struct object_id *oid,\n+\t\t\t\t     enum object_type type);\n+\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t       struct object_list **list_p);\n \ndiff --git a/reachable.c b/reachable.c\nindex c59847257a..84e3d0d75e 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -159,24 +159,6 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t      FOR_EACH_OBJECT_LOCAL_ONLY);\n }\n \n-static void *lookup_object_by_type(struct repository *r,\n-\t\t\t\t   const struct object_id *oid,\n-\t\t\t\t   enum object_type type)\n-{\n-\tswitch (type) {\n-\tcase OBJ_COMMIT:\n-\t\treturn lookup_commit(r, oid);\n-\tcase OBJ_TREE:\n-\t\treturn lookup_tree(r, oid);\n-\tcase OBJ_TAG:\n-\t\treturn lookup_tag(r, oid);\n-\tcase OBJ_BLOB:\n-\t\treturn lookup_blob(r, oid);\n-\tdefault:\n-\t\tdie(\"BUG: unknown object type %d\", type);\n-\t}\n-}\n-\n static int mark_object_seen(const struct object_id *oid,\n \t\t\t     enum object_type type,\n \t\t\t     int exclude,\n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428212","messageId":"YNILCDz3LpHX7OX0@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIJw/8p0F3cPfzd@coredump.intra.peff.net","subject":"[PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T16:08:40Z","receivedAt":"2021-06-22T16:08:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we load the ref decorations, we parse the object pointed to by each\nref in order to get a \"struct object\". This is unnecessarily expensive;\nwe really only need the object struct, and don't even look at the parsed\ncontents. The exception is tags, which we do need to peel.\n\nWe can improve this by looking up the object type first (which is much\ncheaper), and skipping the parse entirely for non-tags. This increases\nthe work slightly for annotated tags (which now do a type lookup _and_ a\nparse), but decreases it a lot for other types. On balance, this seems\nto be a good tradeoff.\n\nIn my git.git clone, with ~2k refs, most of which are branches, the time\nto run \"git log -1 --decorate\" drops from 34ms to 11ms. Even on my\nlinux.git clone, which contains mostly tags and only a handful of\nbranches, the time drops from 30ms to 19ms. And on a more extreme\nreal-world case with ~220k refs, mostly non-tags, the time drops from\n2.6s to 650ms.\n\nThat command is a lop-sided example, of course, because it does as\nlittle non-loading work as possible. But it does show the absolute time\nimprovement. Even in something like a full \"git log --decorate\" on that\nextreme repo, we'd still be saving 2s of CPU time.\n\nIdeally we could push this even further, and avoid parsing even tags, by\nrelying on the packed-refs \"peel\" optimization (which we could do by\ncalling peel_iterated_oid() instead of peeling manually). But we can't\ndo that here. The packed-refs file only stores the bottom-layer of the\npeel (so in a \"tag->tag->commit\" chain, it stores only the commit as the\npeel result).  But the decoration code wants to peel the layers\nindividually, annotating the middle layers of the chain.\n\nIf the packed-refs file ever learns to store all of the peeled layers,\nthen we could switch to it. Or even if it stored a flag to indicate the\npeel was not multi-layer (because most of them aren't), then we could\nuse it most of the time and fall back to a manual peel for the rare\ncases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n log-tree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 7b823786c2..8b700e9c14 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\t      int flags, void *cb_data)\n {\n \tstruct object *obj;\n+\tenum object_type objtype;\n \tenum decoration_type type = DECORATION_NONE;\n \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n \n@@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tobj = parse_object(the_repository, oid);\n-\tif (!obj)\n+\tobjtype = oid_object_info(the_repository, oid, NULL);\n+\tif (type < 0)\n \t\treturn 0;\n+\tobj = lookup_object_by_type(the_repository, oid, objtype);\n \n \tif (starts_with(refname, \"refs/heads/\"))\n \t\ttype = DECORATION_REF_LOCAL;\n-- \n2.32.0.352.gff02c21e72\n"},{"id":"428214","messageId":"a93690bc-1d33-0fe0-5ca8-0e8baa9c66e6@gmail.com","threadId":"55988","inReplyTo":"YNILCDz3LpHX7OX0@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-22T16:35:46Z","receivedAt":"2021-06-22T16:35:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/22/2021 12:08 PM, Jeff King wrote:\n\n> -\tobj = parse_object(the_repository, oid);\n> -\tif (!obj)\n> +\tobjtype = oid_object_info(the_repository, oid, NULL);\n> +\tif (type < 0)\n>  \t\treturn 0;\n\nDo you mean \"if (objtype < 0)\" here? There is a 'type' variable,\nbut it is an enum decoration_type and I can't find a reason why\nit would be negative. oid_object_info() _does_ return -1 if there\nis a problem loading the object, so that would make sense.\n\nThis is the only question I had about the entire series. Everything\nelse LGTM.\n\nThanks,\n-Stolee\n"},{"id":"428216","messageId":"YNIYqFFFti73UT5+@coredump.intra.peff.net","threadId":"55988","inReplyTo":"a93690bc-1d33-0fe0-5ca8-0e8baa9c66e6@gmail.com","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T17:06:48Z","receivedAt":"2021-06-22T17:06:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 12:35:46PM -0400, Derrick Stolee wrote:\n\n> On 6/22/2021 12:08 PM, Jeff King wrote:\n> \n> > -\tobj = parse_object(the_repository, oid);\n> > -\tif (!obj)\n> > +\tobjtype = oid_object_info(the_repository, oid, NULL);\n> > +\tif (type < 0)\n> >  \t\treturn 0;\n> \n> Do you mean \"if (objtype < 0)\" here? There is a 'type' variable,\n> but it is an enum decoration_type and I can't find a reason why\n> it would be negative. oid_object_info() _does_ return -1 if there\n> is a problem loading the object, so that would make sense.\n\nWhoops, thanks for catching that. I originally called it \"enum\nobject_type type\", but then of course the compiler informed that there\nwas already a \"type\" variable in the function. So I renamed it to\n\"objtype\" but missed updating that line. But it still compiled. Yikes. :)\n\nIt doesn't trigger in the test suite because it only happens if the\nrepository is corrupted.\n\n> This is the only question I had about the entire series. Everything\n> else LGTM.\n\nThanks. Here's an amended version of the patch, in case there are no\nother fixups necessary (fingers crossed).\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] load_ref_decorations(): avoid parsing non-tag objects\n\nWhen we load the ref decorations, we parse the object pointed to by each\nref in order to get a \"struct object\". This is unnecessarily expensive;\nwe really only need the object struct, and don't even look at the parsed\ncontents. The exception is tags, which we do need to peel.\n\nWe can improve this by looking up the object type first (which is much\ncheaper), and skipping the parse entirely for non-tags. This increases\nthe work slightly for annotated tags (which now do a type lookup _and_ a\nparse), but decreases it a lot for other types. On balance, this seems\nto be a good tradeoff.\n\nIn my git.git clone, with ~2k refs, most of which are branches, the time\nto run \"git log -1 --decorate\" drops from 34ms to 11ms. Even on my\nlinux.git clone, which contains mostly tags and only a handful of\nbranches, the time drops from 30ms to 19ms. And on a more extreme\nreal-world case with ~220k refs, mostly non-tags, the time drops from\n2.6s to 650ms.\n\nThat command is a lop-sided example, of course, because it does as\nlittle non-loading work as possible. But it does show the absolute time\nimprovement. Even in something like a full \"git log --decorate\" on that\nextreme repo, we'd still be saving 2s of CPU time.\n\nIdeally we could push this even further, and avoid parsing even tags, by\nrelying on the packed-refs \"peel\" optimization (which we could do by\ncalling peel_iterated_oid() instead of peeling manually). But we can't\ndo that here. The packed-refs file only stores the bottom-layer of the\npeel (so in a \"tag->tag->commit\" chain, it stores only the commit as the\npeel result).  But the decoration code wants to peel the layers\nindividually, annotating the middle layers of the chain.\n\nIf the packed-refs file ever learns to store all of the peeled layers,\nthen we could switch to it. Or even if it stored a flag to indicate the\npeel was not multi-layer (because most of them aren't), then we could\nuse it most of the time and fall back to a manual peel for the rare\ncases.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n log-tree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 7b823786c2..c3c8e7e1df 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\t      int flags, void *cb_data)\n {\n \tstruct object *obj;\n+\tenum object_type objtype;\n \tenum decoration_type type = DECORATION_NONE;\n \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n \n@@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tobj = parse_object(the_repository, oid);\n-\tif (!obj)\n+\tobjtype = oid_object_info(the_repository, oid, NULL);\n+\tif (objtype < 0)\n \t\treturn 0;\n+\tobj = lookup_object_by_type(the_repository, oid, objtype);\n \n \tif (starts_with(refname, \"refs/heads/\"))\n \t\ttype = DECORATION_REF_LOCAL;\n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428217","messageId":"YNIZRlTic6K+6rvf@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNIYqFFFti73UT5+@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T17:09:26Z","receivedAt":"2021-06-22T17:09:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 01:06:48PM -0400, Jeff King wrote:\n\n> > Do you mean \"if (objtype < 0)\" here? There is a 'type' variable,\n> > but it is an enum decoration_type and I can't find a reason why\n> > it would be negative. oid_object_info() _does_ return -1 if there\n> > is a problem loading the object, so that would make sense.\n> \n> Whoops, thanks for catching that. I originally called it \"enum\n> object_type type\", but then of course the compiler informed that there\n> was already a \"type\" variable in the function. So I renamed it to\n> \"objtype\" but missed updating that line. But it still compiled. Yikes. :)\n> \n> It doesn't trigger in the test suite because it only happens if the\n> repository is corrupted.\n\nI'm tempted by this as a cleanup on top (I don't want to do it inline,\nsince the diff is so noisy). But I'm also content to leave it.\n\n-- >8 --\nSubject: [PATCH] add_ref_decoration(): rename s/type/deco_type/\n\nNow that we have two types (a decoration type and an object type) in the\nfunction, let's give them both unique names to avoid accidentally using\none instead of the other.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n log-tree.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex c3c8e7e1df..4f69ed176d 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -135,7 +135,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n {\n \tstruct object *obj;\n \tenum object_type objtype;\n-\tenum decoration_type type = DECORATION_NONE;\n+\tenum decoration_type deco_type = DECORATION_NONE;\n \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n \n \tif (filter && !ref_filter_match(refname, filter))\n@@ -162,17 +162,17 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \tobj = lookup_object_by_type(the_repository, oid, objtype);\n \n \tif (starts_with(refname, \"refs/heads/\"))\n-\t\ttype = DECORATION_REF_LOCAL;\n+\t\tdeco_type = DECORATION_REF_LOCAL;\n \telse if (starts_with(refname, \"refs/remotes/\"))\n-\t\ttype = DECORATION_REF_REMOTE;\n+\t\tdeco_type = DECORATION_REF_REMOTE;\n \telse if (starts_with(refname, \"refs/tags/\"))\n-\t\ttype = DECORATION_REF_TAG;\n+\t\tdeco_type = DECORATION_REF_TAG;\n \telse if (!strcmp(refname, \"refs/stash\"))\n-\t\ttype = DECORATION_REF_STASH;\n+\t\tdeco_type = DECORATION_REF_STASH;\n \telse if (!strcmp(refname, \"HEAD\"))\n-\t\ttype = DECORATION_REF_HEAD;\n+\t\tdeco_type = DECORATION_REF_HEAD;\n \n-\tadd_name_decoration(type, refname, obj);\n+\tadd_name_decoration(deco_type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n-- \n2.32.0.352.gff02c21e72\n\n"},{"id":"428218","messageId":"875yy53juu.fsf@evledraar.gmail.com","threadId":"55988","inReplyTo":"YNILCDz3LpHX7OX0@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-22T17:06:43Z","receivedAt":"2021-06-22T17:11:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 22 2021, Jeff King wrote:\n\n> When we load the ref decorations, we parse the object pointed to by each\n> ref in order to get a \"struct object\". This is unnecessarily expensive;\n> we really only need the object struct, and don't even look at the parsed\n> contents. The exception is tags, which we do need to peel.\n>\n> We can improve this by looking up the object type first (which is much\n> cheaper), and skipping the parse entirely for non-tags. This increases\n> the work slightly for annotated tags (which now do a type lookup _and_ a\n> parse), but decreases it a lot for other types. On balance, this seems\n> to be a good tradeoff.\n>\n> In my git.git clone, with ~2k refs, most of which are branches, the time\n> to run \"git log -1 --decorate\" drops from 34ms to 11ms. Even on my\n> linux.git clone, which contains mostly tags and only a handful of\n> branches, the time drops from 30ms to 19ms. And on a more extreme\n> real-world case with ~220k refs, mostly non-tags, the time drops from\n> 2.6s to 650ms.\n>\n> That command is a lop-sided example, of course, because it does as\n> little non-loading work as possible. But it does show the absolute time\n> improvement. Even in something like a full \"git log --decorate\" on that\n> extreme repo, we'd still be saving 2s of CPU time.\n>\n> Ideally we could push this even further, and avoid parsing even tags, by\n> relying on the packed-refs \"peel\" optimization (which we could do by\n> calling peel_iterated_oid() instead of peeling manually). But we can't\n> do that here. The packed-refs file only stores the bottom-layer of the\n> peel (so in a \"tag->tag->commit\" chain, it stores only the commit as the\n> peel result).  But the decoration code wants to peel the layers\n> individually, annotating the middle layers of the chain.\n>\n> If the packed-refs file ever learns to store all of the peeled layers,\n> then we could switch to it. Or even if it stored a flag to indicate the\n> peel was not multi-layer (because most of them aren't), then we could\n> use it most of the time and fall back to a manual peel for the rare\n> cases.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  log-tree.c | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index 7b823786c2..8b700e9c14 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>  \t\t\t      int flags, void *cb_data)\n>  {\n>  \tstruct object *obj;\n> +\tenum object_type objtype;\n>  \tenum decoration_type type = DECORATION_NONE;\n>  \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n>  \n> @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tobj = parse_object(the_repository, oid);\n> -\tif (!obj)\n> +\tobjtype = oid_object_info(the_repository, oid, NULL);\n> +\tif (type < 0)\n>  \t\treturn 0;\n> +\tobj = lookup_object_by_type(the_repository, oid, objtype);\n\nThis series looks good. I just wonder if between this and my own\nlookup_{blob,tree,tag,commit}_type() in [1] whether exposing some\nfunction between what we have now in parse_object() and\nparse_object_buffer() wouldn't also do this for you.\n\nI.e. in my patch if you pass a type into parse_object_buffer() I think\nyou'll get the same behavior.\n\nTo be clear I see nothing wrong with this, it's more of a musing about\nhow some functions in object.c discover the type on their own, others\nallow passing it in, sometimes (worse before that series of mine) we\nrelay the not-real-but-inferred-type etc.\n\n1. https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/\n"},{"id":"428219","messageId":"c4c27819-157c-d694-d758-8dbb46499ab3@gmail.com","threadId":"55988","inReplyTo":"YNIZRlTic6K+6rvf@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-22T17:25:53Z","receivedAt":"2021-06-22T17:25:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/22/2021 1:09 PM, Jeff King wrote:\n> On Tue, Jun 22, 2021 at 01:06:48PM -0400, Jeff King wrote:\n> \n>>> Do you mean \"if (objtype < 0)\" here? There is a 'type' variable,\n>>> but it is an enum decoration_type and I can't find a reason why\n>>> it would be negative. oid_object_info() _does_ return -1 if there\n>>> is a problem loading the object, so that would make sense.\n>>\n>> Whoops, thanks for catching that. I originally called it \"enum\n>> object_type type\", but then of course the compiler informed that there\n>> was already a \"type\" variable in the function. So I renamed it to\n>> \"objtype\" but missed updating that line. But it still compiled. Yikes. :)\n>>\n>> It doesn't trigger in the test suite because it only happens if the\n>> repository is corrupted.\n> \n> I'm tempted by this as a cleanup on top (I don't want to do it inline,\n> since the diff is so noisy). But I'm also content to leave it.\n\nNaming is hard. This change seems worth it.\n\nThanks,\n-Stolee\n"},{"id":"428225","messageId":"87zgvh20j3.fsf@evledraar.gmail.com","threadId":"55988","inReplyTo":"YNIYqFFFti73UT5+@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-22T18:27:53Z","receivedAt":"2021-06-22T18:54:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 22 2021, Jeff King wrote:\n\n> On Tue, Jun 22, 2021 at 12:35:46PM -0400, Derrick Stolee wrote:\n>\n>> On 6/22/2021 12:08 PM, Jeff King wrote:\n>> \n>> > -\tobj = parse_object(the_repository, oid);\n>> > -\tif (!obj)\n>> > +\tobjtype = oid_object_info(the_repository, oid, NULL);\n>> > +\tif (type < 0)\n>> >  \t\treturn 0;\n>> \n>> Do you mean \"if (objtype < 0)\" here? There is a 'type' variable,\n>> but it is an enum decoration_type and I can't find a reason why\n>> it would be negative. oid_object_info() _does_ return -1 if there\n>> is a problem loading the object, so that would make sense.\n>\n> Whoops, thanks for catching that. I originally called it \"enum\n> object_type type\", but then of course the compiler informed that there\n> was already a \"type\" variable in the function. So I renamed it to\n> \"objtype\" but missed updating that line. But it still compiled. Yikes. :)\n\n[Enter Captain Hindsight]\n\nIf you use a slightly different coding style and leverage the\ninformation the compiler has to work with you'd get it to error for you,\ne.g. this on your original patch would catch it:\n\n\tdiff --git a/log-tree.c b/log-tree.c\n\tindex 8b700e9c142..7e3a011b533 100644\n\t--- a/log-tree.c\n\t+++ b/log-tree.c\n\t@@ -157,9 +157,12 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n\t \t}\n\t \n\t \tobjtype = oid_object_info(the_repository, oid, NULL);\n\t-\tif (type < 0)\n\t+\tswitch (type) {\n\t+\tcase OBJ_BAD:\n\t \t\treturn 0;\n\t-\tobj = lookup_object_by_type(the_repository, oid, objtype);\n\t+\tdefault:\n\t+\t\tobj = lookup_object_by_type(the_repository, oid, objtype);\n\t+\t}\n\t \n\t \tif (starts_with(refname, \"refs/heads/\"))\n\t \t\ttype = DECORATION_REF_LOCAL;\n\nIMO the real problem is an over-reliance on C being so happy to treat\nenums as ints (well, with them being ints). If you consistently use\nlabels you get the compiler to do the checking. For me with gcc and\nclang with that on top:\n\t\n\tlog-tree.c:161:2: error: case value ‘4294967295’ not in enumerated type ‘enum decoration_type’ [-Werror=switch]\n\t  case OBJ_BAD:\n\t  ^~~~\n\tlog-tree.c:161:7: error: case value not in enumerated type 'enum decoration_type' [-Werror,-Wswitch]\n\t        case OBJ_BAD:\n\t             ^\n\nI think we've disagreed on that exact point before recently, i.e. you\nthink we shouldn't rely on OBJ_BAD in that way, and instead check for\nany negative value:\nhttps://lore.kernel.org/git/YHCZh5nLNVEHCWV2@coredump.intra.peff.net/\n\nThis sort of thing is a good reason to pick the opposite pattern. You\nget the same type checking you'd usually get with anything else in C.\n\nYes, it is more verbose e.g. in this case, and particularly (as noted\ndownthread of what I linked to) because \"enum object_type\" contains so\nmany uncommon things, and really should be split up.\n\nIn practice I don't think it's too verbose, because once you start\nconsistently using the pattern you'll usually not be doing conversions\nall over the place, and would just do this sort of thing via a helper\nthat does the type checking, e.g. something like this (or anything else\nwhere you don't lose the type & labels):\n\t\n\tdiff --git a/log-tree.c b/log-tree.c\n\tindex 8b700e9c142..a61fb01ba3f 100644\n\t--- a/log-tree.c\n\t+++ b/log-tree.c\n\t@@ -130,6 +130,30 @@ static int ref_filter_match(const char *refname,\n\t \treturn 1;\n\t }\n\t \n\t+static enum object_type oid_object_info_ok(struct repository *repo,\n\t+\t\t\t\t\t   struct object_id *oid,\n\t+\t\t\t\t\t   enum object_type *typep,\n\t+\t\t\t\t\t   unsigned long *sizep)\n\t+{\n\t+\tenum object_type type = oid_object_info(repo, oid, sizep);\n\t+\t*typep = type;\n\t+\tswitch (type) {\n\t+\tcase OBJ_BAD:\n\t+\t\treturn 0;\n\t+\tcase OBJ_COMMIT:\n\t+\tcase OBJ_TREE:\n\t+\tcase OBJ_BLOB:\n\t+\tcase OBJ_TAG:\n\t+\t\treturn 1;\n\t+\tcase OBJ_NONE:\n\t+\tcase OBJ_OFS_DELTA:\n\t+\tcase OBJ_REF_DELTA:\n\t+\tcase OBJ_ANY:\n\t+\tcase OBJ_MAX:\n\t+\t\tBUG(\"the enum_object type is too large!\");\n\t+\t}\n\t+}\n\t+\n\t static int add_ref_decoration(const char *refname, const struct object_id *oid,\n\t \t\t\t      int flags, void *cb_data)\n\t {\n\t@@ -156,8 +180,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n\t \t\treturn 0;\n\t \t}\n\t \n\t-\tobjtype = oid_object_info(the_repository, oid, NULL);\n\t-\tif (type < 0)\n\t+\tif (!oid_object_info_ok(the_repository, oid, &type, NULL))\n\t \t\treturn 0;\n\t \tobj = lookup_object_by_type(the_repository, oid, objtype);\n\t \n\nWith that pattern GCC narrowlry pulls ahead with showing 4 warnings just\nabout the loss of the type, with Clang at 3 :)\n"},{"id":"428226","messageId":"YNIyn7dksqMtkuk/@coredump.intra.peff.net","threadId":"55988","inReplyTo":"875yy53juu.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T18:57:35Z","receivedAt":"2021-06-22T18:57:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 07:06:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n> >  \t\treturn 0;\n> >  \t}\n> >  \n> > -\tobj = parse_object(the_repository, oid);\n> > -\tif (!obj)\n> > +\tobjtype = oid_object_info(the_repository, oid, NULL);\n> > +\tif (type < 0)\n> >  \t\treturn 0;\n> > +\tobj = lookup_object_by_type(the_repository, oid, objtype);\n> \n> This series looks good. I just wonder if between this and my own\n> lookup_{blob,tree,tag,commit}_type() in [1] whether exposing some\n> function between what we have now in parse_object() and\n> parse_object_buffer() wouldn't also do this for you.\n> \n> I.e. in my patch if you pass a type into parse_object_buffer() I think\n> you'll get the same behavior.\n\nMaybe, but I'm having trouble seeing what would be a helpful\nabstraction. I don't think I'd want to use parse_object_buffer() here;\nwe don't have a buffer at all (and obviously it could learn to handle\nNULL, but that's extra code there).\n\nparse_object_buffer() could call this lookup_object_by_type() to get the\nstruct, which would save it a few lines. But it still has to do the\nif-else chain for each type, because it does other type-specific\nthings.\n\nSo I dunno. I would be happy if you came up with something, just because\nit's nice to minimize the number of places that do this if-else/switch\non type. But I have a feeling it's diminishing returns in terms of\ncomplexity. If we can at least contain it all to object.c, that's\nsomething.\n\n-Peff\n"},{"id":"428227","messageId":"YNI1KmkGTYDCW3jZ@coredump.intra.peff.net","threadId":"55988","inReplyTo":"87zgvh20j3.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T19:08:26Z","receivedAt":"2021-06-22T19:08:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 08:27:53PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Whoops, thanks for catching that. I originally called it \"enum\n> > object_type type\", but then of course the compiler informed that there\n> > was already a \"type\" variable in the function. So I renamed it to\n> > \"objtype\" but missed updating that line. But it still compiled. Yikes. :)\n> \n> [Enter Captain Hindsight]\n> \n> If you use a slightly different coding style and leverage the\n> information the compiler has to work with you'd get it to error for you,\n> e.g. this on your original patch would catch it:\n> \n> \tdiff --git a/log-tree.c b/log-tree.c\n> \tindex 8b700e9c142..7e3a011b533 100644\n> \t--- a/log-tree.c\n> \t+++ b/log-tree.c\n> \t@@ -157,9 +157,12 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n> \t \t}\n> \t \n> \t \tobjtype = oid_object_info(the_repository, oid, NULL);\n> \t-\tif (type < 0)\n> \t+\tswitch (type) {\n> \t+\tcase OBJ_BAD:\n> \t \t\treturn 0;\n> \t-\tobj = lookup_object_by_type(the_repository, oid, objtype);\n> \t+\tdefault:\n> \t+\t\tobj = lookup_object_by_type(the_repository, oid, objtype);\n> \t+\t}\n> \t \n> \t \tif (starts_with(refname, \"refs/heads/\"))\n> \t \t\ttype = DECORATION_REF_LOCAL;\n\nYeah, I agree that would find it in this case. I do find that style\nslightly harder to read, though. And...\n\n> IMO the real problem is an over-reliance on C being so happy to treat\n> enums as ints (well, with them being ints). If you consistently use\n> labels you get the compiler to do the checking. For me with gcc and\n> clang with that on top:\n> \t\n> \tlog-tree.c:161:2: error: case value ‘4294967295’ not in enumerated type ‘enum decoration_type’ [-Werror=switch]\n> \t  case OBJ_BAD:\n> \t  ^~~~\n> \tlog-tree.c:161:7: error: case value not in enumerated type 'enum decoration_type' [-Werror,-Wswitch]\n> \t        case OBJ_BAD:\n> \t             ^\n\n...it would help in this case because OBJ_BAD happens to have a value\nthat is not defined for decoration_type. If it did, then the compiler\nwould be quite happy to consider them equivalent.\n\nSo I don't disagree with you exactly, but I'm not sure of the tradeoff\nof always using switches instead of conditionals (which IMHO is less\nreadable) for more compiler safety that only works sometimes is worth\nit.\n\n> I think we've disagreed on that exact point before recently, i.e. you\n> think we shouldn't rely on OBJ_BAD in that way, and instead check for\n> any negative value:\n> https://lore.kernel.org/git/YHCZh5nLNVEHCWV2@coredump.intra.peff.net/\n\nTo be clear, my complaint about checking for OBJ_BAD exactly is that it\ncloses the door on other negative return types. And indeed, the switch()\nyou showed above would become a silent bug if we introduced\nOBJ_BAD_FOR_ANOTHER_READ as \"-2\" (without any compiler support, because\nof the \"default\" case in the switch statement).\n\nNow that's somewhat hypothetical, but in the near-term it also means\nconfirming that any of the functions which get converted from \"int\" to\n\"enum object_type\" are not in fact passing back \"-2\" in any\ncircumstances.\n\nThat said...\n\n> In practice I don't think it's too verbose, because once you start\n> consistently using the pattern you'll usually not be doing conversions\n> all over the place, and would just do this sort of thing via a helper\n> that does the type checking, e.g. something like this (or anything else\n> where you don't lose the type & labels):\n> [...]\n> \t-\tobjtype = oid_object_info(the_repository, oid, NULL);\n> \t-\tif (type < 0)\n> \t+\tif (!oid_object_info_ok(the_repository, oid, &type, NULL))\n> \t \t\treturn 0;\n\nYes, that would deal with that problem. It's definitely a different\nstyle, but one that I could get used to. It's a lot more object oriented\n(\"you are not allowed to do numeric logic on an object type; you can\nonly use these accessor methods to query it\"). If we were going that\nroute, I would stop having \"enum object_type\" at all, and instead make\nit \"struct object_type { enum { ... } value }\". That would prevent\nanybody from accidentally just looking at it, and instead force people\ninto that object-oriented style.\n\nI dunno. It is is a big departure from how we do things now. And the bug\nhere notwithstanding, I don't feel like enum confusion has generally\nbeen a big source of error for us.\n\n-Peff\n"},{"id":"428249","messageId":"YNKgkGkPiMgNubNE@nand.local","threadId":"55988","inReplyTo":"YNILCDz3LpHX7OX0@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T02:46:40Z","receivedAt":"2021-06-23T02:46:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 22, 2021 at 12:08:40PM -0400, Jeff King wrote:\n> If the packed-refs file ever learns to store all of the peeled layers,\n> then we could switch to it. Or even if it stored a flag to indicate the\n> peel was not multi-layer (because most of them aren't), then we could\n> use it most of the time and fall back to a manual peel for the rare\n> cases.\n\nYeah, I would be in favor of either of these. Of the two, the latter\nseems like the simplest thing, but the former provides you all of the\ninformation you could hope for.\n\nI suppose that if you are already changing the format of packed-refs,\nthen we might as well do the thing which provides the most information\nand allows us to optimize *all* cases, not just the vast majority of\nthem.\n\nOf course, that's all way outside of the scope of this patch, which\nshouldn't have to deal with either of those.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  log-tree.c | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index 7b823786c2..8b700e9c14 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -134,6 +134,7 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>  \t\t\t      int flags, void *cb_data)\n>  {\n>  \tstruct object *obj;\n> +\tenum object_type objtype;\n>  \tenum decoration_type type = DECORATION_NONE;\n>  \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n>\n> @@ -155,9 +156,10 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n>  \t\treturn 0;\n>  \t}\n>\n> -\tobj = parse_object(the_repository, oid);\n> -\tif (!obj)\n> +\tobjtype = oid_object_info(the_repository, oid, NULL);\n> +\tif (type < 0)\n>  \t\treturn 0;\n> +\tobj = lookup_object_by_type(the_repository, oid, objtype);\n\nThe comments about s/type/obj&/ aside, this looks good to me (and the\namended version below looks ready to be picked up, at least in my eyes).\n\nOne thing I did want to note which is elided by the limited context is\nthat we *do* parse tags like you say, it's just hidden in the \"while\n(obj->type == OBJ_TAG)\" loop below.\n\nSo that's doing the right thing, but it wasn't clear from the limited\ncontext here that this patch was immediately correct.\n\nThanks,\nTaylor\n"},{"id":"428329","messageId":"YNOs/w5JwfBDLbUD@coredump.intra.peff.net","threadId":"55988","inReplyTo":"YNKgkGkPiMgNubNE@nand.local","subject":"Re: [PATCH 5/5] load_ref_decorations(): avoid parsing non-tag objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-23T21:51:59Z","receivedAt":"2021-06-23T21:52:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 10:46:40PM -0400, Taylor Blau wrote:\n\n> On Tue, Jun 22, 2021 at 12:08:40PM -0400, Jeff King wrote:\n> > If the packed-refs file ever learns to store all of the peeled layers,\n> > then we could switch to it. Or even if it stored a flag to indicate the\n> > peel was not multi-layer (because most of them aren't), then we could\n> > use it most of the time and fall back to a manual peel for the rare\n> > cases.\n> \n> Yeah, I would be in favor of either of these. Of the two, the latter\n> seems like the simplest thing, but the former provides you all of the\n> information you could hope for.\n> \n> I suppose that if you are already changing the format of packed-refs,\n> then we might as well do the thing which provides the most information\n> and allows us to optimize *all* cases, not just the vast majority of\n> them.\n\nOne reason not to include all of them is that the list can be\narbitrarily long, and regular readers of packed-refs (who may not even\ncare about peeling at all) have to skip past it. That matters a little\nless these since we binary-search it (but you still might be iterating\nover the ref).\n\nSo I think either way it is a tradeoff, and you are making assumptions\nabout which cases are less likely.\n\nIf I were to work on this (and I don't have any immediate plans to do\nso), I'd probably do whichever is easiest to implement, and to maintain\nbackwards-compatibility. And I suspect that is the \"flag\" approach, but\na lot would depend on the details of our parser and what it permits.\n\n-Peff\n"}]}