{"thread":{"id":"35189","subject":"[PATCH] for-each-ref: avoid loading objects to print %(objectname)","startedAt":"2013-10-24T08:46:38Z","lastAt":"2013-10-30T17:34:40Z","messageCount":4,"participants":["Jeff King","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229388","messageId":"20131024084638.GA761@sigill.intra.peff.net","threadId":"35189","inReplyTo":null,"subject":"[PATCH] for-each-ref: avoid loading objects to print %(objectname)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-24T08:46:38Z","receivedAt":"2013-10-24T08:46:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you ask for-each-ref to print each ref and its object,\nlike:\n\n  git for-each-ref --format='%(objectname) %(refname)'\n\nthis should involve little more work than looking at the ref\nfiles (and packed-refs) themselves. However, for-each-ref\nwill actually load each object from disk just to print its\nsha1. For most repositories, this isn't a big deal, but it\ncan be noticeable if you have a large number of refs to\nprint. Here are best-of-five timings for the command above\non a repo with ~10K refs:\n\n  [before]\n  real    0m0.112s\n  user    0m0.092s\n  sys     0m0.016s\n\n  [after]\n  real    0m0.014s\n  user    0m0.012s\n  sys     0m0.000s\n\nThis patch checks for %(objectname) and %(objectname:short)\nbefore we actually parse the object (and the rest of the\ncode is smart enough to avoid parsing if we have filled all\nof our placeholders).\n\nNote that we can't simply move the objectname parsing code\ninto the early loop. If the \"deref\" form %(*objectname) is\nused, then we do need to parse the object in order to peel\nthe tag. So instead of moving the code, we factor it out\ninto a separate function that can be called for both cases.\n\nWhile we're at it, we add some basic tests for the\ndereferenced placeholders, which were not tested at all\nbefore. This helps ensure we didn't regress that case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a repost of:\n\n  http://article.gmane.org/gmane.comp.version-control.git/235704\n\nI think the original was just overlooked.\n\n builtin/for-each-ref.c  | 29 ++++++++++++++++++++---------\n t/t6300-for-each-ref.sh |  4 ++++\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 1d4083c..d096051 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -205,6 +205,22 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n \treturn buf;\n }\n \n+static int grab_objectname(const char *name, const unsigned char *sha1,\n+\t\t\t    struct atom_value *v)\n+{\n+\tif (!strcmp(name, \"objectname\")) {\n+\t\tchar *s = xmalloc(41);\n+\t\tstrcpy(s, sha1_to_hex(sha1));\n+\t\tv->s = s;\n+\t\treturn 1;\n+\t}\n+\tif (!strcmp(name, \"objectname:short\")) {\n+\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n /* See grab_values */\n static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n {\n@@ -225,15 +241,8 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\t\tv->ul = sz;\n \t\t\tv->s = s;\n \t\t}\n-\t\telse if (!strcmp(name, \"objectname\")) {\n-\t\t\tchar *s = xmalloc(41);\n-\t\t\tstrcpy(s, sha1_to_hex(obj->sha1));\n-\t\t\tv->s = s;\n-\t\t}\n-\t\telse if (!strcmp(name, \"objectname:short\")) {\n-\t\t\tv->s = xstrdup(find_unique_abbrev(obj->sha1,\n-\t\t\t\t\t\t\t  DEFAULT_ABBREV));\n-\t\t}\n+\t\telse if (deref)\n+\t\t\tgrab_objectname(name, obj->sha1, v);\n \t}\n }\n \n@@ -676,6 +685,8 @@ static void populate_value(struct refinfo *ref)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\telse if (!deref && grab_objectname(name, ref->objectname, v))\n+\t\t\tcontinue;\n \t\telse\n \t\t\tcontinue;\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 752f5cb..2b4b9a9 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -58,6 +58,8 @@ test_atom head parent ''\n test_atom head numparent 0\n test_atom head object ''\n test_atom head type ''\n+test_atom head *objectname ''\n+test_atom head *objecttype ''\n test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'\n test_atom head authorname 'A U Thor'\n test_atom head authoremail '<author@example.com>'\n@@ -91,6 +93,8 @@ test_atom tag parent ''\n test_atom tag numparent ''\n test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'\n test_atom tag type 'commit'\n+test_atom tag *objectname '67a36f10722846e891fbada1ba48ed035de75581'\n+test_atom tag *objecttype 'commit'\n test_atom tag author ''\n test_atom tag authorname ''\n test_atom tag authoremail ''\n-- \n1.8.4.1.898.g8bf8a41.dirty\n"},{"id":"229554","messageId":"877gd0pgze.fsf@linux-k42r.v.cablecom.net","threadId":"35189","inReplyTo":"20131024084638.GA761@sigill.intra.peff.net","subject":"Re: [PATCH] for-each-ref: avoid loading objects to print %(objectname)","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-10-26T08:35:17Z","receivedAt":"2013-10-26T08:35:17Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 752f5cb..2b4b9a9 100755\n[...]\n> +test_atom head *objectname ''\n> +test_atom head *objecttype ''\n[...]\n> +test_atom tag *objectname '67a36f10722846e891fbada1ba48ed035de75581'\n> +test_atom tag *objecttype 'commit'\n\nCan you quote the *?  I may have become somewhat paranoid, but still.\nThis is the first use of the *<field> syntax, and test_atom seems\nwritten to correctly quote its arguments, so why risk it? :-)\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"229783","messageId":"20131030065016.GB11317@sigill.intra.peff.net","threadId":"35189","inReplyTo":"877gd0pgze.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH] for-each-ref: avoid loading objects to print %(objectname)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-30T06:50:16Z","receivedAt":"2013-10-30T06:50:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 26, 2013 at 10:35:17AM +0200, Thomas Rast wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> > index 752f5cb..2b4b9a9 100755\n> [...]\n> > +test_atom head *objectname ''\n> > +test_atom head *objecttype ''\n> [...]\n> > +test_atom tag *objectname '67a36f10722846e891fbada1ba48ed035de75581'\n> > +test_atom tag *objecttype 'commit'\n> \n> Can you quote the *?  I may have become somewhat paranoid, but still.\n> This is the first use of the *<field> syntax, and test_atom seems\n> written to correctly quote its arguments, so why risk it? :-)\n\nYeah, that is a very reasonable suggestion. And the patch is still in\npu, so it's not too late to squash.\n\nJunio, here is a resend with the asterisks quoted, to replace what's in\njk/for-each-ref-skip-parsing.  I also double-checked that test_atom\nkeeps them properly quoted (it does).\n\n-- >8 --\nSubject: for-each-ref: avoid loading objects to print %(objectname)\n\nIf you ask for-each-ref to print each ref and its object,\nlike:\n\n  git for-each-ref --format='%(objectname) %(refname)'\n\nthis should involve little more work than looking at the ref\nfiles (and packed-refs) themselves. However, for-each-ref\nwill actually load each object from disk just to print its\nsha1. For most repositories, this isn't a big deal, but it\ncan be noticeable if you have a large number of refs to\nprint. Here are best-of-five timings for the command above\non a repo with ~10K refs:\n\n  [before]\n  real    0m0.112s\n  user    0m0.092s\n  sys     0m0.016s\n\n  [after]\n  real    0m0.014s\n  user    0m0.012s\n  sys     0m0.000s\n\nThis patch checks for %(objectname) and %(objectname:short)\nbefore we actually parse the object (and the rest of the\ncode is smart enough to avoid parsing if we have filled all\nof our placeholders).\n\nNote that we can't simply move the objectname parsing code\ninto the early loop. If the \"deref\" form %(*objectname) is\nused, then we do need to parse the object in order to peel\nthe tag. So instead of moving the code, we factor it out\ninto a separate function that can be called for both cases.\n\nWhile we're at it, we add some basic tests for the\ndereferenced placeholders, which were not tested at all\nbefore. This helps ensure we didn't regress that case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/for-each-ref.c  | 29 ++++++++++++++++++++---------\n t/t6300-for-each-ref.sh |  4 ++++\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 1d4083c..d096051 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -205,6 +205,22 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n \treturn buf;\n }\n \n+static int grab_objectname(const char *name, const unsigned char *sha1,\n+\t\t\t    struct atom_value *v)\n+{\n+\tif (!strcmp(name, \"objectname\")) {\n+\t\tchar *s = xmalloc(41);\n+\t\tstrcpy(s, sha1_to_hex(sha1));\n+\t\tv->s = s;\n+\t\treturn 1;\n+\t}\n+\tif (!strcmp(name, \"objectname:short\")) {\n+\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n /* See grab_values */\n static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n {\n@@ -225,15 +241,8 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\t\tv->ul = sz;\n \t\t\tv->s = s;\n \t\t}\n-\t\telse if (!strcmp(name, \"objectname\")) {\n-\t\t\tchar *s = xmalloc(41);\n-\t\t\tstrcpy(s, sha1_to_hex(obj->sha1));\n-\t\t\tv->s = s;\n-\t\t}\n-\t\telse if (!strcmp(name, \"objectname:short\")) {\n-\t\t\tv->s = xstrdup(find_unique_abbrev(obj->sha1,\n-\t\t\t\t\t\t\t  DEFAULT_ABBREV));\n-\t\t}\n+\t\telse if (deref)\n+\t\t\tgrab_objectname(name, obj->sha1, v);\n \t}\n }\n \n@@ -676,6 +685,8 @@ static void populate_value(struct refinfo *ref)\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\telse if (!deref && grab_objectname(name, ref->objectname, v))\n+\t\t\tcontinue;\n \t\telse\n \t\t\tcontinue;\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 752f5cb..da5fb6c 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -58,6 +58,8 @@ test_atom head parent ''\n test_atom head numparent 0\n test_atom head object ''\n test_atom head type ''\n+test_atom head '*objectname' ''\n+test_atom head '*objecttype' ''\n test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'\n test_atom head authorname 'A U Thor'\n test_atom head authoremail '<author@example.com>'\n@@ -91,6 +93,8 @@ test_atom tag parent ''\n test_atom tag numparent ''\n test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'\n test_atom tag type 'commit'\n+test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'\n+test_atom tag '*objecttype' 'commit'\n test_atom tag author ''\n test_atom tag authorname ''\n test_atom tag authoremail ''\n-- \n1.8.4.1.898.g8bf8a41.dirty\n"},{"id":"229838","messageId":"xmqqa9hqd5n3.fsf@gitster.dls.corp.google.com","threadId":"35189","inReplyTo":"20131030065016.GB11317@sigill.intra.peff.net","subject":"Re: [PATCH] for-each-ref: avoid loading objects to print %(objectname)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-30T17:34:40Z","receivedAt":"2013-10-30T17:34:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Oct 26, 2013 at 10:35:17AM +0200, Thomas Rast wrote:\n> ...\n>> Can you quote the *?  I may have become somewhat paranoid, but still.\n>> This is the first use of the *<field> syntax, and test_atom seems\n>> written to correctly quote its arguments, so why risk it? :-)\n>\n> Yeah, that is a very reasonable suggestion. And the patch is still in\n> pu, so it's not too late to squash.\n>\n> Junio, here is a resend with the asterisks quoted, to replace what's in\n> jk/for-each-ref-skip-parsing.  I also double-checked that test_atom\n> keeps them properly quoted (it does).\n\nThanks, both, for being careful.\n"}]}