{"thread":{"id":"32704","subject":"segmentation fault (nullpointer) with git log --submodule -p","startedAt":"2013-01-23T14:38:16Z","lastAt":"2013-01-27T05:32:07Z","messageCount":24,"participants":["Armin","Jeff King","Stefan Näwe","Duy Nguyen","Junio C Hamano","Jonathan Nieder","Jonathon Mah"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"207595","messageId":"20130123143816.GA579@krypton.darkbyte.org","threadId":"32704","inReplyTo":null,"subject":"segmentation fault (nullpointer) with git log --submodule -p","fromName":"Armin","fromEmail":"netzverweigerer@gmail.com","sentAt":"2013-01-23T14:38:16Z","receivedAt":"2013-01-23T14:38:16Z","isPatch":false,"sender":{"key":"netzverweigerer@gmail.com","avatar":null},"body":"Hello dear git people.\n\nI experience a reproducible segmentation fault on one of my repositories when doing a \"git log --submodule -p\", tested with newest version on Arch Linux (git version 1.8.1.1) and built fresh (git version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:\n\n\nProgram terminated with signal 11, Segmentation fault.\n#0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n752 for (i = 0; msg[i]; i++) {\n (gdb) bt\n#0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n#1 format_commit_one (context=, placeholder=0x526b1e \"s\", sb=0x7ffff69b6ad0) at pretty.c:1157\n#2 format_commit_item (sb=0x7ffff69b6ad0, placeholder=0x526b1e \"s\", context=) at pretty.c:1224\n#3 0x00000000004dacd9 in strbuf_expand (sb=sb@entry=0x7ffff69b6ad0, format=0x526b1e \"s\", format@entry=0x526b18 \" %m %s\", fn=fn@entry=0x4b4730 , context=context@entry=0x7ffff69b6980)\n at strbuf.c:247\n#4 0x00000000004b5816 in format_commit_message (commit=commit@entry=0x1ffafd8, format=format@entry=0x526b18 \" %m %s\", sb=sb@entry=0x7ffff69b6ad0, pretty_ctx=pretty_ctx@entry=0x7ffff69b6af0) at pretty.c:1284\n#5 0x00000000004dde52 in print_submodule_summary (reset=0x754640 \"\\033[m\", add=0x754708 \"\\033[32m\", del=0x7546e0 \"\\033[31m\", f=0x7f0685bac7a0, rev=0x7ffff69b6b40) at submodule.c:236\n#6 show_submodule_summary (f=0x7f0685bac7a0, path=, one=one@entry=0x1ff2af0 \"\\020\\\\vC\\371\\070\\vJ\\352\\344\\205\\340\\226u\\273\\021\\372\\330\\234\\004\", \n two=two@entry=0x2030a60 \"\\301a(\\350\\371\\372\\340mb[խo_\\272\\301\\223V˙\", dirty_submodule=, meta=meta@entry=0x754690 \"\\033[1m\", del=del@entry=0x7546e0 \"\\033[31m\", add=0x754708 \"\\033[32m\", \n reset=reset@entry=0x754640 \"\\033[m\") at submodule.c:307\n#7 0x000000000048dd1d in builtin_diff (name_a=name_a@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", name_b=name_b@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", \n one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, xfrm_msg=0x2039a20 \"\\033[1mindex 105c764..c16128e 160000\\033[m\\n\", must_show_header=must_show_header@entry=0, o=o@entry=0x7ffff69b7b88, \n complete_rewrite=complete_rewrite@entry=0) at diff.c:2267\n#8 0x000000000048e60e in run_diff_cmd (pgm=pgm@entry=0x0, name=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", other=, \n attr_path=attr_path@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, msg=msg@entry=0x7ffff69b74b0, o=o@entry=0x7ffff69b7b88, p=p@entry=0x20371b0)\n at diff.c:3057\n\n(gdb) bt\n#0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n#1 format_commit_one (context=, placeholder=0x526b1e \"s\", sb=0x7ffff69b6ad0) at pretty.c:1157\n#2 format_commit_item (sb=0x7ffff69b6ad0, placeholder=0x526b1e \"s\", context=) at pretty.c:1224\n#3 0x00000000004dacd9 in strbuf_expand (sb=sb@entry=0x7ffff69b6ad0, format=0x526b1e \"s\", format@entry=0x526b18 \" %m %s\", fn=fn@entry=0x4b4730 , context=context@entry=0x7ffff69b6980)\n at strbuf.c:247\n#4 0x00000000004b5816 in format_commit_message (commit=commit@entry=0x1ffafd8, format=format@entry=0x526b18 \" %m %s\", sb=sb@entry=0x7ffff69b6ad0, pretty_ctx=pretty_ctx@entry=0x7ffff69b6af0) at pretty.c:1284\n#5 0x00000000004dde52 in print_submodule_summary (reset=0x754640 \"\\033[m\", add=0x754708 \"\\033[32m\", del=0x7546e0 \"\\033[31m\", f=0x7f0685bac7a0, rev=0x7ffff69b6b40) at submodule.c:236\n#6 show_submodule_summary (f=0x7f0685bac7a0, path=, one=one@entry=0x1ff2af0 \"\\020\\\\vC\\371\\070\\vJ\\352\\344\\205\\340\\226u\\273\\021\\372\\330\\234\\004\", \n two=two@entry=0x2030a60 \"\\301a(\\350\\371\\372\\340mb[խo_\\272\\301\\223V˙\", dirty_submodule=, meta=meta@entry=0x754690 \"\\033[1m\", del=del@entry=0x7546e0 \"\\033[31m\", add=0x754708 \"\\033[32m\", \n reset=reset@entry=0x754640 \"\\033[m\") at submodule.c:307\n#7 0x000000000048dd1d in builtin_diff (name_a=name_a@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", name_b=name_b@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", \n one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, xfrm_msg=0x2039a20 \"\\033[1mindex 105c764..c16128e 160000\\033[m\\n\", must_show_header=must_show_header@entry=0, o=o@entry=0x7ffff69b7b88, \n complete_rewrite=complete_rewrite@entry=0) at diff.c:2267\n#8 0x000000000048e60e in run_diff_cmd (pgm=pgm@entry=0x0, name=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", other=, \n attr_path=attr_path@entry=0x1ff2b50 \"Packages/Application/Amadeus.Somea.Dialog\", one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, msg=msg@entry=0x7ffff69b74b0, o=o@entry=0x7ffff69b7b88, p=p@entry=0x20371b0)\n at diff.c:3057\n#9 0x000000000048eb3d in run_diff (o=0x7ffff69b7b88, p=0x20371b0) at diff.c:3145\n#10 diff_flush_patch (o=0x7ffff69b7b88, p=0x20371b0) at diff.c:3979\n#11 diff_flush_patch (p=0x20371b0, o=0x7ffff69b7b88) at diff.c:3970\n#12 0x000000000048f15f in diff_flush (options=options@entry=0x7ffff69b7b88) at diff.c:4500\n#13 0x00000000004a211a in log_tree_diff_flush (opt=opt@entry=0x7ffff69b7850) at log-tree.c:776\n#14 0x00000000004a22b2 in log_tree_diff (log=0x7ffff69b7720, commit=0x1ffdf60, opt=0x7ffff69b7850) at log-tree.c:836\n#15 log_tree_commit (opt=opt@entry=0x7ffff69b7850, commit=commit@entry=0x1ffdf60) at log-tree.c:859\n#16 0x00000000004393d3 in cmd_log_walk (rev=rev@entry=0x7ffff69b7850) at builtin/log.c:310\n#17 0x0000000000439f38 in cmd_log (argc=3, argv=0x7ffff69b80c0, prefix=0x0) at builtin/log.c:582\n#18 0x0000000000405978 in run_builtin (argv=0x7ffff69b80c0, argc=3, p=0x74fd20) at git.c:281\n#19 handle_internal_command (argc=3, argv=0x7ffff69b80c0) at git.c:442\n#20 0x0000000000404de2 in run_argv (argv=0x7ffff69b7f50, argcp=0x7ffff69b7f5c) at git.c:488\n#21 main (argc=3, argv=0x7ffff69b80c0) at git.c:563\n(gdb) f 0\n#0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n752 for (i = 0; msg[i]; i++) {\n(gdb) l\n747 static void parse_commit_header(struct format_commit_context *context)\n748 {\n749 const char *msg = context->message;\n750 int i;\n751 \n752 for (i = 0; msg[i]; i++) {\n753 int eol;\n754 for (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n755 ; /* do nothing */\n756 \n(gdb) p msg\n$7 = \n(gdb) p context->message\n$8 = 0x0\n(gdb) x/8i $pc\n=> 0x4b51e5 : movzbl (%rcx),%eax\n 0x4b51e8 : mov %rcx,0x18(%rsp)\n 0x4b51ed : mov %rcx,%r10\n 0x4b51f0 : test %al,%al\n 0x4b51f2 : je 0x4b52a3 \n 0x4b51f8 : nopl 0x0(%rax,%rax,1)\n 0x4b5200 : test %al,%al\n 0x4b5202 : je 0x4b529e \n(gdb) i r rcx\nrcx 0x0 0\n\n\nDoes this help in any way? Can i provide any further information that helps?\n\nMany thanks for reading this and all the best,\n\n\nArmin\n"},{"id":"207628","messageId":"20130123200222.GB19832@sigill.intra.peff.net","threadId":"32704","inReplyTo":"20130123143816.GA579@krypton.darkbyte.org","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T20:02:22Z","receivedAt":"2013-01-23T20:02:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 23, 2013 at 03:38:16PM +0100, Armin wrote:\n\n> Hello dear git people.\n> \n> I experience a reproducible segmentation fault on one of my\n> repositories when doing a \"git log --submodule -p\", tested with newest\n> version on Arch Linux (git version 1.8.1.1) and built fresh (git\n> version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:\n> \n> \n> Program terminated with signal 11, Segmentation fault.\n> #0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n> 752 for (i = 0; msg[i]; i++) {\n> [...]\n> (gdb) l\n> 747 static void parse_commit_header(struct format_commit_context *context)\n> 748 {\n> 749 const char *msg = context->message;\n> 750 int i;\n> 751 \n> 752 for (i = 0; msg[i]; i++) {\n> 753 int eol;\n> 754 for (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n> 755 ; /* do nothing */\n> 756 \n> (gdb) p msg\n> $7 = \n> (gdb) p context->message\n> $8 = 0x0\n\nYeah, that should definitely not be NULL. I can't reproduce here with a\nfew simple examples, though.\n\nDoes it fail with older versions of git? If so, can you bisect?\n\nIs it possible for you to make your repo available?\n\n-Peff\n"},{"id":"207690","messageId":"510124F5.9090505@atlas-elektronik.com","threadId":"32704","inReplyTo":"20130123200222.GB19832@sigill.intra.peff.net","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Stefan Näwe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2013-01-24T12:11:33Z","receivedAt":"2013-01-24T12:11:33Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Am 23.01.2013 21:02, schrieb Jeff King:\n> On Wed, Jan 23, 2013 at 03:38:16PM +0100, Armin wrote:\n> \n>> Hello dear git people.\n>>\n>> I experience a reproducible segmentation fault on one of my\n>> repositories when doing a \"git log --submodule -p\", tested with newest\n>> version on Arch Linux (git version 1.8.1.1) and built fresh (git\n>> version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:\n>>\n>>\n>> Program terminated with signal 11, Segmentation fault.\n>> #0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752\n>> 752 for (i = 0; msg[i]; i++) {\n>> [...]\n>> (gdb) l\n>> 747 static void parse_commit_header(struct format_commit_context *context)\n>> 748 {\n>> 749 const char *msg = context->message;\n>> 750 int i;\n>> 751 \n>> 752 for (i = 0; msg[i]; i++) {\n>> 753 int eol;\n>> 754 for (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n>> 755 ; /* do nothing */\n>> 756 \n>> (gdb) p msg\n>> $7 = \n>> (gdb) p context->message\n>> $8 = 0x0\n> \n> Yeah, that should definitely not be NULL. I can't reproduce here with a\n> few simple examples, though.\n> \n> Does it fail with older versions of git? If so, can you bisect?\n\nI did. My bisection told me this is the suspect:\n\nccdc603 (parse_object: try internal cache before reading object db)\n\nMy git-fu is not good enough to analyze that...\n\n> Is it possible for you to make your repo available?\n\nUnfortunately not. It crashes with one particular repos (using submodules)\nthat I can't make available but not with another which is available\nat https://github.com/snaewe/super.git\n\nHTH\n\nStefan\n-- \n----------------------------------------------------------------\n/dev/random says: There must be more to life than compile-and-go.\npython -c \"print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')\"\n"},{"id":"207691","messageId":"CACsJy8B9O=A26_=sv1JEYdtazsWa4khZkqpTgFSSTs_RGGPZqQ@mail.gmail.com","threadId":"32704","inReplyTo":"510124F5.9090505@atlas-elektronik.com","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T13:40:47Z","receivedAt":"2013-01-24T13:40:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe\n wrote:\n>> Does it fail with older versions of git? If so, can you bisect?\n>\n> I did. My bisection told me this is the suspect:\n>\n> ccdc603 (parse_object: try internal cache before reading object db)\n\ndiff --git a/object.c b/object.c\nindex d8d09f9..6b06297 100644\n--- a/object.c\n+++ b/object.c\n@@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)\n enum object_type type;\n int eaten;\n const unsigned char *repl = lookup_replace_object(sha1);\n- void *buffer = read_sha1_file(sha1, &type, &size);\n+ void *buffer;\n+ struct object *obj;\n+\n+ obj = lookup_object(sha1);\n+ if (obj && obj->parsed)\n+ return obj;\n\nAny chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?\nWhat if you change that \"if\" to\n\nif (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit\n*)obj)->buffer))\n\n??\n\nAlso you did not encode commits in any specific encoding, nor set\ni18n.logOutputEncoding?\n-- \nDuy\n"},{"id":"207692","messageId":"51013FDD.5030004@atlas-elektronik.com","threadId":"32704","inReplyTo":"CACsJy8B9O=A26_=sv1JEYdtazsWa4khZkqpTgFSSTs_RGGPZqQ@mail.gmail.com","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Stefan Näwe","fromEmail":"stefan.naewe@atlas-elektronik.com","sentAt":"2013-01-24T14:06:21Z","receivedAt":"2013-01-24T14:06:21Z","isPatch":false,"sender":{"key":"stefan.naewe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4468?v=4"},"body":"Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:\n> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe\n> wrote:\n>>> Does it fail with older versions of git? If so, can you bisect?\n>>\n>> I did. My bisection told me this is the suspect:\n>>\n>> ccdc603 (parse_object: try internal cache before reading object db)\n>\n> diff --git a/object.c b/object.c\n> index d8d09f9..6b06297 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)\n> enum object_type type;\n> int eaten;\n> const unsigned char *repl = lookup_replace_object(sha1);\n> - void *buffer = read_sha1_file(sha1, &type, &size);\n> + void *buffer;\n> + struct object *obj;\n> +\n> + obj = lookup_object(sha1);\n> + if (obj && obj->parsed)\n> + return obj;\n>\n> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?\n> What if you change that \"if\" to\n>\n> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit\n> *)obj)->buffer))\n>\n\nNo more segfault!\n\n> Also you did not encode commits in any specific encoding,\n\nWe're using Git for Windows and some commits contain 'umlauts' (äöü).\nBut those characters should be encoded in UTF-8, shouldn't they?\nBut the 'git log...' only crashes on a Debian/Linux machine.\n\n> nor set i18n.logOutputEncoding?\n\nIt's not set.\n\n(only i18n.filesEncoding is set to utf-8 on my machine)\n\nOh, and it's not crashing if I do:\n\ngit log -p --submodule |cat\n\nStefan\n--\n----------------------------------------------------------------\n/dev/random says: Dumb luck beats sound planning every time. Trust me.\npython -c \"print \n'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')\"\n"},{"id":"207693","messageId":"CACsJy8CEofqi9S8-SDx_O+Ko0i56aRZ4KEJrVnbFum6zzsJrJg@mail.gmail.com","threadId":"32704","inReplyTo":"51013FDD.5030004@atlas-elektronik.com","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-24T14:14:47Z","receivedAt":"2013-01-24T14:14:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 24, 2013 at 9:06 PM, Stefan Näwe\n wrote:\n> Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:\n>> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe\n>> wrote:\n>>>> Does it fail with older versions of git? If so, can you bisect?\n>>>\n>>> I did. My bisection told me this is the suspect:\n>>>\n>>> ccdc603 (parse_object: try internal cache before reading object db)\n>>\n>> diff --git a/object.c b/object.c\n>> index d8d09f9..6b06297 100644\n>> --- a/object.c\n>> +++ b/object.c\n>> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)\n>> enum object_type type;\n>> int eaten;\n>> const unsigned char *repl = lookup_replace_object(sha1);\n>> - void *buffer = read_sha1_file(sha1, &type, &size);\n>> + void *buffer;\n>> + struct object *obj;\n>> +\n>> + obj = lookup_object(sha1);\n>> + if (obj && obj->parsed)\n>> + return obj;\n>>\n>> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?\n>> What if you change that \"if\" to\n>>\n>> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit\n>> *)obj)->buffer))\n>>\n>\n> No more segfault!\n\nSweet. I have no idea how that fixes it. Maybe Jeff can give some\nexplanation after he wakes up.\n-- \nDuy\n"},{"id":"207733","messageId":"20130124232721.GA16036@sigill.intra.peff.net","threadId":"32704","inReplyTo":"CACsJy8CEofqi9S8-SDx_O+Ko0i56aRZ4KEJrVnbFum6zzsJrJg@mail.gmail.com","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T23:27:21Z","receivedAt":"2013-01-24T23:27:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2013 at 09:14:47PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> >>> I did. My bisection told me this is the suspect:\n> >>>\n> >>> ccdc603 (parse_object: try internal cache before reading object db)\n> >>\n> >> diff --git a/object.c b/object.c\n> >> index d8d09f9..6b06297 100644\n> >> --- a/object.c\n> >> +++ b/object.c\n> >> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)\n> >> enum object_type type;\n> >> int eaten;\n> >> const unsigned char *repl = lookup_replace_object(sha1);\n> >> - void *buffer = read_sha1_file(sha1, &type, &size);\n> >> + void *buffer;\n> >> + struct object *obj;\n> >> +\n> >> + obj = lookup_object(sha1);\n> >> + if (obj && obj->parsed)\n> >> + return obj;\n> >>\n> >> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?\n> >> What if you change that \"if\" to\n> >>\n> >> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit\n> >> *)obj)->buffer))\n> >>\n> >\n> > No more segfault!\n> \n> Sweet. I have no idea how that fixes it. Maybe Jeff can give some\n> explanation after he wakes up.\n\nUgh. I think I know why it fixes it. We free the commit's buffer as part\nof the log traversal, but then later want to access it as part of the\ndiff. We presumably call parse_object somewhere in the middle to make\nsure it is parsed.\n\nBefore ccdc603, a side effect of parse_object is that even for a parsed\nobject, we would fill in the buffer field of a commit or tree. See\nparse_object_buffer:\n\n } else if (type == OBJ_COMMIT) {\n struct commit *commit = lookup_commit(sha1);\n if (commit) {\n if (parse_commit_buffer(commit, buffer, size))\n return NULL;\n if (!commit->buffer) {\n commit->buffer = buffer;\n eaten = 1;\n }\n obj = &commit->object;\n }\n\nWhen this patch was originally proposed, I wrote[1]:\n\n On Thu, Jan 05, 2012 at 01:55:22PM -0800, Junio C Hamano wrote:\n > > So I think it is safe short of somebody doing some horrible manual\n > > munging of a \"struct object\".\n >\n > Yeah, I was worried about codepaths like commit-pretty-printing\n > might be mucking with the contents of commit->buffer, perhaps\n > reencoding the text and then calling parse_object() to get the\n > unmodified original back, or something silly like that. But the\n > lookup_object() call at the beginning of the parse_object() already\n > prevents us from doing such a thing, so we should be OK, I would\n > think.\n\n [...]\n\n What saves you is that the parse_*_buffer functions all do nothing\n when the object.parsed flag is set, and the code I added makes sure\n that object.parsed is set in the object that lookup_object returns.\n\n So yeah, anytime you tweak the contents of commit->buffer but don't\n unset the \"parsed\" flag, you are asking for trouble.\n\nWhich is true, but obviously I missed that in addition to calling\nparse_*_buffer, which will be a no-op, we _also_ set the buffer\nindependently. So parse_object was functioning in a belt-and-suspenders\nfor that case. And I think this is probably the same root cause as the\nsegfault which came up here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/214366\n\nSo what to do?\n\nWe can revert ccdc603, but I do not think we need to. We can catch the\nproblematic cases with something like your patch, but still get the\noptimization when the buffer really is already filled in. I think we'd\nneed to extend your patch to handle trees, too, to be totally correct.\n\nBut there are still some loose ends that I note:\n\n 1. Making such a change would be parse_object erring on the side of\n providing the buffer. But it doesn't actually know if the buffer is\n desired or not. For instance, upload-pack benefited from this\n optimization, but does not need save_commit_buffer on at all. So\n commit->buffer is _always_ NULL there, and that's just fine; we\n really don't need to read the object.\n\n Now this may be a bad example, because due to my follow-on patches,\n we avoid calling parse_object at all in most cases, so I don't\n think it matters any longer to upload-pack. But I suspect there are\n other places with similar circumstances. Fundamentally parse_object\n doesn't know what the caller is interested in.\n\n 2. This means that parse_commit and parse_object behave differently in\n this regard. The former will leave the buffer unfilled. Meaning we\n may still have issues with code paths that munge the buffer without\n resetting the parsed flag, independent of ccdc603 and fixing this.\n\nTo me, these highlight that our commit->buffer management is fragile and\nis largely about guessing in various circumstances whether somebody will\nlater want the buffer. I'm not sure of the right solution, though. It\nseems like something that inherently blurs the lines between bits of\ncode (e.g., how should \"log\" know that a submodule diff might later want\nto see the same entry? Should we optimistically free and then make it\neasier for the later user to reliably ensure the buffer is primed? Or\nshould we err on the side of keeping it in place?).\n\n-Peff\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/188000\n"},{"id":"207735","messageId":"7va9ry87a0.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"20130124232721.GA16036@sigill.intra.peff.net","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T23:56:23Z","receivedAt":"2013-01-24T23:56:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... (e.g., how should \"log\" know that a submodule diff might later want\n> to see the same entry? Should we optimistically free and then make it\n> easier for the later user to reliably ensure the buffer is primed? Or\n> should we err on the side of keeping it in place?).\n\nMy knee-jerk reaction is that we should consider that commit->buffer\nbelongs to the revision traversal machinery. Any other uses bolted\non later can borrow it if buffer still exists (I do not think pretty\ncode rewrites the buffer contents in place in any way), or they can\nask read_sha1_file() to read it themselves and free when they are\ndone.\n"},{"id":"207736","messageId":"20130125005528.GA27325@sigill.intra.peff.net","threadId":"32704","inReplyTo":"7va9ry87a0.fsf@alter.siamese.dyndns.org","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-25T00:55:28Z","receivedAt":"2013-01-25T00:55:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... (e.g., how should \"log\" know that a submodule diff might later want\n> > to see the same entry? Should we optimistically free and then make it\n> > easier for the later user to reliably ensure the buffer is primed? Or\n> > should we err on the side of keeping it in place?).\n> \n> My knee-jerk reaction is that we should consider that commit->buffer\n> belongs to the revision traversal machinery. Any other uses bolted\n> on later can borrow it if buffer still exists (I do not think pretty\n> code rewrites the buffer contents in place in any way), or they can\n> ask read_sha1_file() to read it themselves and free when they are\n> done.\n\nYeah, that is probably the sanest way forward. It at least leaves\nownership in one place, and everybody else is an opportunistic consumer.\nWe do need to annotate each user site though with something like the\n\"ensure\" function I mentioned.\n\nIf they are not owners, then the better pattern is probably something\nlike:\n\n /*\n * Get the commit buffer, either opportunistically using\n * the cached version attached to the commit object, or loading it\n * from disk if necessary.\n */\n const char *use_commit_buffer(struct commit *c)\n {\n enum object_type type;\n unsigned long size;\n\n if (c->buffer)\n return c->buffer;\n /*\n * XXX check type == OBJ_COMMIT?\n * XXX die() on NULL as a convenience to callers?\n */\n return read_sha1_file(c->object.sha1, &type, &size);\n }\n\n void unuse_commit_buffer(const char *buf, struct commit *c)\n {\n /*\n * If we used the cached copy attached to the commit,\n * we don't want to free it; it's not our responsibility.\n */\n if (buf == c->buffer)\n return;\n\n free((char *)buf);\n }\n\nI suspect that putting a use/unuse pair inside format_commit_message\nwould handle most cases.\n\n-Peff\n"},{"id":"207742","messageId":"CACsJy8AS3cgF3LXzkXh3JyBpv7xY=Lf4T=sXK1EELU-j+VkUhQ@mail.gmail.com","threadId":"32704","inReplyTo":"20130125005528.GA27325@sigill.intra.peff.net","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-25T02:05:45Z","receivedAt":"2013-01-25T02:05:45Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 25, 2013 at 7:55 AM, Jeff King wrote:\n> On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > ... (e.g., how should \"log\" know that a submodule diff might later want\n>> > to see the same entry? Should we optimistically free and then make it\n>> > easier for the later user to reliably ensure the buffer is primed? Or\n>> > should we err on the side of keeping it in place?).\n>>\n>> My knee-jerk reaction is that we should consider that commit->buffer\n>> belongs to the revision traversal machinery. Any other uses bolted\n>> on later can borrow it if buffer still exists (I do not think pretty\n>> code rewrites the buffer contents in place in any way), or they can\n>> ask read_sha1_file() to read it themselves and free when they are\n>> done.\n>\n> Yeah, that is probably the sanest way forward. It at least leaves\n> ownership in one place, and everybody else is an opportunistic consumer.\n> We do need to annotate each user site though with something like the\n> \"ensure\" function I mentioned.\n>\n> If they are not owners, then the better pattern is probably something\n> like:\n\nYou probably should rename \"buffer\" (to _buffer or something) and let\nthe compiler catches all the places commit->buffer illegally used.\n\n>\n> /*\n> * Get the commit buffer, either opportunistically using\n> * the cached version attached to the commit object, or loading it\n> * from disk if necessary.\n> */\n> const char *use_commit_buffer(struct commit *c)\n> {\n> enum object_type type;\n> unsigned long size;\n>\n> if (c->buffer)\n> return c->buffer;\n> /*\n> * XXX check type == OBJ_COMMIT?\n> * XXX die() on NULL as a convenience to callers?\n> */\n> return read_sha1_file(c->object.sha1, &type, &size);\n> }\n>\n> void unuse_commit_buffer(const char *buf, struct commit *c)\n> {\n> /*\n> * If we used the cached copy attached to the commit,\n> * we don't want to free it; it's not our responsibility.\n> */\n> if (buf == c->buffer)\n> return;\n>\n> free((char *)buf);\n> }\n>\n> I suspect that putting a use/unuse pair inside format_commit_message\n> would handle most cases.\n>\n> -Peff\n-- \nDuy\n"},{"id":"207746","messageId":"7vzjzx7w01.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"7va9ry87a0.fsf@alter.siamese.dyndns.org","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T03:59:58Z","receivedAt":"2013-01-25T03:59:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> ... (e.g., how should \"log\" know that a submodule diff might later want\n>> to see the same entry? Should we optimistically free and then make it\n>> easier for the later user to reliably ensure the buffer is primed? Or\n>> should we err on the side of keeping it in place?).\n>\n> My knee-jerk reaction is that we should consider that commit->buffer\n> belongs to the revision traversal machinery. Any other uses bolted\n> on later can borrow it if buffer still exists (I do not think pretty\n> code rewrites the buffer contents in place in any way), or they can\n> ask read_sha1_file() to read it themselves and free when they are\n> done.\n\nI've been toying with an idea along this line.\n\n commit.h | 16 ++++++++++++++++\n builtin/blame.c | 27 ++++++++-------------------\n commit.c | 20 ++++++++++++++++++++\n 3 files changed, 44 insertions(+), 19 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex c16c8a7..b559535 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -226,4 +226,20 @@ extern void print_commit_list(struct commit_list *list,\n \t\t\t const char *format_cur,\n \t\t\t const char *format_last);\n \n+extern int ensure_commit_buffer(struct commit *);\n+extern void discard_commit_buffer(struct commit *);\n+\n+#define with_commit_buffer(commit) \\\n+\tdo { \\\n+\t\tint had_buffer_ = !!commit->buffer; \\\n+\t\tif (!had_buffer_) \\\n+\t\t\tensure_commit_buffer(commit); \\\n+\t\tdo\n+\n+#define done_with_commit_buffer(commit) \\\n+\t\twhile (0); \\\n+\t\tif (!had_buffer_) \\\n+\t\t\tdiscard_commit_buffer(commit); \\\n+\t} while (0)\n+\n #endif /* COMMIT_H */\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex b431ba3..8b2e4a5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1424,25 +1424,14 @@ static void get_commit_info(struct commit *commit,\n \n \tcommit_info_init(ret);\n \n-\t/*\n-\t * We've operated without save_commit_buffer, so\n-\t * we now need to populate them for output.\n-\t */\n-\tif (!commit->buffer) {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\t\tcommit->buffer =\n-\t\t\tread_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!commit->buffer)\n-\t\t\tdie(\"Cannot read commit %s\",\n-\t\t\t sha1_to_hex(commit->object.sha1));\n-\t}\n-\tencoding = get_log_output_encoding();\n-\treencoded = logmsg_reencode(commit, encoding);\n-\tmessage = reencoded ? reencoded : commit->buffer;\n-\tget_ac_line(message, \"\\nauthor \",\n-\t\t &ret->author, &ret->author_mail,\n-\t\t &ret->author_time, &ret->author_tz);\n+\twith_commit_buffer(commit) {\n+\t\tencoding = get_log_output_encoding();\n+\t\treencoded = logmsg_reencode(commit, encoding);\n+\t\tmessage = reencoded ? reencoded : commit->buffer;\n+\t\tget_ac_line(message, \"\\nauthor \",\n+\t\t\t &ret->author, &ret->author_mail,\n+\t\t\t &ret->author_time, &ret->author_tz);\n+\t} done_with_commit_buffer(commit);\n \n \tif (!detailed) {\n \t\tfree(reencoded);\ndiff --git a/commit.c b/commit.c\nindex e8eb0ae..a627eea 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1357,3 +1357,23 @@ void print_commit_list(struct commit_list *list,\n \t\tprintf(format, sha1_to_hex(list->item->object.sha1));\n \t}\n }\n+\n+int ensure_commit_buffer(struct commit *commit)\n+{\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\tif (commit->buffer)\n+\t\treturn 0;\n+\tcommit->buffer = read_sha1_file(commit->object.sha1, &type, &size);\n+\tif (commit->buffer)\n+\t\treturn -1;\n+\telse\n+\t\treturn 0;\n+}\n+\n+void discard_commit_buffer(struct commit *commit)\n+{\n+\tfree(commit->buffer);\n+\tcommit->buffer = NULL;\n+}\n"},{"id":"207748","messageId":"20130125040856.GA30533@sigill.intra.peff.net","threadId":"32704","inReplyTo":"7vzjzx7w01.fsf@alter.siamese.dyndns.org","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-25T04:08:56Z","receivedAt":"2013-01-25T04:08:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >> ... (e.g., how should \"log\" know that a submodule diff might later want\n> >> to see the same entry? Should we optimistically free and then make it\n> >> easier for the later user to reliably ensure the buffer is primed? Or\n> >> should we err on the side of keeping it in place?).\n> >\n> > My knee-jerk reaction is that we should consider that commit->buffer\n> > belongs to the revision traversal machinery. Any other uses bolted\n> > on later can borrow it if buffer still exists (I do not think pretty\n> > code rewrites the buffer contents in place in any way), or they can\n> > ask read_sha1_file() to read it themselves and free when they are\n> > done.\n> \n> I've been toying with an idea along this line.\n> \n> commit.h | 16 ++++++++++++++++\n> builtin/blame.c | 27 ++++++++-------------------\n> commit.c | 20 ++++++++++++++++++++\n> 3 files changed, 44 insertions(+), 19 deletions(-)\n\nI think we are on the same page as far as what needs to happen at the\ncall sites.\n\nMy suggested implementation had a separate buffer, but you are right\nthat we may need to actually set \"commit->buffer\" because sub-functions\nexpect to find it there (the alternative might be cleaning up the\nsub-function interfaces). I haven't looked at the call-sites yet.\n\nThis:\n\n> +extern int ensure_commit_buffer(struct commit *);\n> +extern void discard_commit_buffer(struct commit *);\n> +\n> +#define with_commit_buffer(commit) \\\n> +\tdo { \\\n> +\t\tint had_buffer_ = !!commit->buffer; \\\n> +\t\tif (!had_buffer_) \\\n> +\t\t\tensure_commit_buffer(commit); \\\n> +\t\tdo\n> +\n> +#define done_with_commit_buffer(commit) \\\n> +\t\twhile (0); \\\n> +\t\tif (!had_buffer_) \\\n> +\t\t\tdiscard_commit_buffer(commit); \\\n> +\t} while (0)\n\nis pretty nasty, though. I know it gets the job done, but in my\nexperience, macros which do not behave syntactically like functions are\nusually a good sign that you are doing something gross and\nunmaintainable.\n\nI dunno.\n\n-Peff\n"},{"id":"207752","messageId":"7vmwvx7v03.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"20130125040856.GA30533@sigill.intra.peff.net","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T04:21:32Z","receivedAt":"2013-01-25T04:21:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I know it gets the job done, but in my\n> experience, macros which do not behave syntactically like functions are\n> usually a good sign that you are doing something gross and\n> unmaintainable.\n\nYeah, it is a bit too cute for my taste, too, even though it was fun\n;-)\n"},{"id":"207763","messageId":"20130125055331.GC26524@elie.Belkin","threadId":"32704","inReplyTo":"7vzjzx7w01.fsf@alter.siamese.dyndns.org","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-25T05:53:31Z","receivedAt":"2013-01-25T05:53:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> I've been toying with an idea along this line.\n\nHeh. Just for fun, here's an uglier version:\n\n\tstruct wcb_data {\n\t\tint had_buffer;\n\t\tint using_buffer;\n\t};\n\t#define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }\n\n\textern void acquire_commit_buffer(struct commit *, struct wcb_data *);\n\textern void done_with_commit_buffer(struct commit *, struct wcb_data *);\n\n\t/*\n\t * usage:\n\t *\tstruct wcb_data buf = WITH_COMMIT_BUFFER_INIT;\n\t *\n\t *\twith_commit_buffer(commit, buf) {\n\t *\t\t...\n\t *\t}\n\t */\n\t#define with_commit_buffer(commit, i) \\\n\t\tfor (acquire_commit_buffer(commit, &i); \\\n\t\t i.using_buffer; \\\n\t\t done_with_commit_buffer(commit, &i))\n\n\tvoid acquire_commit_buffer(struct commit *commit, struct wcb_data *i)\n\t{\n\t\tenum object_type type;\n\t\tunsigned long size;\n\n\t\tassert(!i->using_buffer);\n\t\ti->using_buffer = 1;\n\t\ti->had_buffer = !!commit->buffer;\n\n\t\tif (i->had_buffer)\n\t\t\treturn;\n\t\tcommit->buffer = read_sha1_file(commit->object.sha1, &type, &size);\n\t\tif (!commit->buffer)\n\t\t\tdie(\"unable to read commit %s\", sha1_to_hex(commit->object.sha1));\n\t}\n\n\tvoid done_with_commit_buffer(struct commit *commit, struct wcb_data *i)\n\t{\n\t\tassert(i->using_buffer);\n\t\ti->using_buffer = 0;\n\n\t\tif (!i->had_buffer) {\n\t\t\tfree(commit->buffer);\n\t\t\tcommit->buffer = NULL;\n\t\t}\n\t}\n"},{"id":"207767","messageId":"7vr4l967u2.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"20130125055331.GC26524@elie.Belkin","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T07:27:17Z","receivedAt":"2013-01-25T07:27:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> Junio C Hamano wrote:\n>\n>> I've been toying with an idea along this line.\n>\n> Heh. Just for fun, here's an uglier version:\n\nMuch nicer, though.\n\n>\n> \tstruct wcb_data {\n> \t\tint had_buffer;\n> \t\tint using_buffer;\n> \t};\n> \t#define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }\n>\n> \textern void acquire_commit_buffer(struct commit *, struct wcb_data *);\n> \textern void done_with_commit_buffer(struct commit *, struct wcb_data *);\n>\n> \t/*\n> \t * usage:\n> \t *\tstruct wcb_data buf = WITH_COMMIT_BUFFER_INIT;\n> \t *\n> \t *\twith_commit_buffer(commit, buf) {\n> \t *\t\t...\n> \t *\t}\n> \t */\n> \t#define with_commit_buffer(commit, i) \\\n> \t\tfor (acquire_commit_buffer(commit, &i); \\\n> \t\t i.using_buffer; \\\n> \t\t done_with_commit_buffer(commit, &i))\n>\n> \tvoid acquire_commit_buffer(struct commit *commit, struct wcb_data *i)\n> \t{\n> \t\tenum object_type type;\n> \t\tunsigned long size;\n>\n> \t\tassert(!i->using_buffer);\n> \t\ti->using_buffer = 1;\n> \t\ti->had_buffer = !!commit->buffer;\n>\n> \t\tif (i->had_buffer)\n> \t\t\treturn;\n> \t\tcommit->buffer = read_sha1_file(commit->object.sha1, &type, &size);\n> \t\tif (!commit->buffer)\n> \t\t\tdie(\"unable to read commit %s\", sha1_to_hex(commit->object.sha1));\n> \t}\n>\n> \tvoid done_with_commit_buffer(struct commit *commit, struct wcb_data *i)\n> \t{\n> \t\tassert(i->using_buffer);\n> \t\ti->using_buffer = 0;\n>\n> \t\tif (!i->had_buffer) {\n> \t\t\tfree(commit->buffer);\n> \t\t\tcommit->buffer = NULL;\n> \t\t}\n> \t}\n"},{"id":"207768","messageId":"FE6CC927-1915-4486-BBB8-4C109F7B5295@me.com","threadId":"32704","inReplyTo":"20130125055331.GC26524@elie.Belkin","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Jonathon Mah","fromEmail":"jmah@me.com","sentAt":"2013-01-25T07:32:44Z","receivedAt":"2013-01-25T07:32:44Z","isPatch":false,"sender":{"key":"jmah@me.com","avatar":null},"body":"Just to note, the proposals so far don't prevent a \"smart-ass\" function from freeing the buffer when it's called underneath the use/release scope, as in:\n\nwith_commit_buffer(commit); {\n\tfn1_needing_buffer(commit);\n\twalk_rev_tree_or_something();\n\tfn2_needing_buffer(commit);\n} done_with_commit_buffer(commit);\n\nwalk_rev_tree_or_something() might need to read commits to do its thing, and it could still choose to free their buffers (as in rev-list.c finish_commit()). If those commits includes the one being \"retained\", the call to fn2 will still see NULL despite it being in a 'protected scope'.\n\nAre the objections to using a reference count?\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"},{"id":"207817","messageId":"7vip6l5l71.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"FE6CC927-1915-4486-BBB8-4C109F7B5295@me.com","subject":"Re: segmentation fault (nullpointer) with git log --submodule -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T15:36:18Z","receivedAt":"2013-01-25T15:36:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathon Mah writes:\n\n> Just to note, the proposals so far don't prevent a \"smart-ass\"\n> function from freeing the buffer when it's called underneath the\n> use/release scope, as in:\n>\n> with_commit_buffer(commit); {\n> \tfn1_needing_buffer(commit);\n> \twalk_rev_tree_or_something();\n> \tfn2_needing_buffer(commit);\n> } done_with_commit_buffer(commit);\n\nI think the goal of everybody discussing these ideas is to make sure\nthat all code follows the simple ownership policy proposed at the\nbeginning of this subthread: commit->buffer belongs to the revision\ntraversal machinery, and other users could borrow it when available.\n\nEven though your sample code will break, from that point of view, I\ndo not think it is something worth worrying about. If the function\n\"walk_rev_tree_or_something()\" discards commit->buffer, it by\ndefinition must be a part of the revision traversal machinery, and\nany code that calls it inside with_commit_buffer() or uses the field\nafter such a call without revalidating commit->buffer, is already in\nviolation. With or without such a macro, we would need to be\ncareful about enforcing the ownership rule, and I think a code\nstructure like the above example is easier to spot problems in\nduring the review than the current code.\n\nBecause retaining commit->buffer is done for the benefit of the\nnext/future users of the data, and not for the users that _are_\nusing them right now, I do not think the usual refcounting that\ndiscards when nobody references the data is a good match to the\nproblem we are discussing.\n"},{"id":"207870","messageId":"20130126094026.GA9646@sigill.intra.peff.net","threadId":"32704","inReplyTo":"7vip6l5l71.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/3] lazily load commit->buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-26T09:40:28Z","receivedAt":"2013-01-26T09:40:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 25, 2013 at 07:36:18AM -0800, Junio C Hamano wrote:\n\n> Jonathon Mah writes:\n> \n> > Just to note, the proposals so far don't prevent a \"smart-ass\"\n> > function from freeing the buffer when it's called underneath the\n> > use/release scope, as in:\n> >\n> > with_commit_buffer(commit); {\n> > \tfn1_needing_buffer(commit);\n> > \twalk_rev_tree_or_something();\n> > \tfn2_needing_buffer(commit);\n> > } done_with_commit_buffer(commit);\n> \n> I think the goal of everybody discussing these ideas is to make sure\n> that all code follows the simple ownership policy proposed at the\n> beginning of this subthread: commit->buffer belongs to the revision\n> traversal machinery, and other users could borrow it when available.\n\nYeah, agreed. I started to fix this up with a use/unuse pattern and\nrealized something: all of the call sites are calling logmsg_reencode\nanyway, because that is the next logical step in doing anything with the\nbuffer that is not just parsing out the parent/timestamp/tree info. And\nsince that function already might allocate (for the re-encoded copy),\ncallers have to handle the maybe-borrowed-maybe-free situation already.\n\nSo I came up with this patch series, which I think should fix the\nproblem, and actually makes the call-sites easier to read, rather than\nharder.\n\n [1/3]: commit: drop useless xstrdup of commit message\n [2/3]: logmsg_reencode: never return NULL\n [3/3]: logmsg_reencode: lazily load missing commit buffers\n\nHere's the diffstat:\n\n builtin/blame.c | 22 ++-------\n builtin/commit.c | 14 +-----\n commit.h | 1 +\n pretty.c | 93 ++++++++++++++++++++++++++---------\n t/t4042-diff-textconv-caching.sh | 8 +++\n 5 files changed, 85 insertions(+), 53 deletions(-)\n\nNot too bad, and 27 of the lines added in pretty.c are new comments\nexplaining the flow of logmsg_reencode. So even if this doesn't get\nevery case, I think it's a nice cleanup.\n\n-Peff\n"},{"id":"207871","messageId":"20130126094236.GA6999@sigill.intra.peff.net","threadId":"32704","inReplyTo":"20130126094026.GA9646@sigill.intra.peff.net","subject":"[PATCH 1/3] commit: drop useless xstrdup of commit message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-26T09:42:45Z","receivedAt":"2013-01-26T09:42:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-commit is asked to reuse a commit message via \"-c\",\nwe call read_commit_message, which looks up the commit and\nhands back either the re-encoded result, or a copy of the\noriginal. We make a copy in the latter case so that the\nownership semantics of the return value are clear (in either\ncase, it can be freed).\n\nHowever, since we return a \"const char *\", and since the\nresulting buffer's lifetime is the same as that of the whole\nprogram, we never bother to free it at all.\n\nLet's just drop the copy. That saves us a copy in the common\ncase. While it does mean we leak in the re-encode case, it\ndoesn't matter, since we are relying on program exit to free\nthe memory anyway.\n\nSigned-off-by: Jeff King \n---\nThis one isn't strictly necessary, but it makes it a lot more obvious\nwhat is going on with the memory ownership of this code in the next\npatch.\n\n builtin/commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 38b9a9c..fbbb40f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -962,7 +962,7 @@ static const char *read_commit_message(const char *name)\n \t * encodings are identical.\n \t */\n \tif (out == NULL)\n-\t\tout = xstrdup(commit->buffer);\n+\t\tout = commit->buffer;\n \treturn out;\n }\n \n-- \n1.8.0.2.16.g72e2fc9\n"},{"id":"207872","messageId":"20130126094406.GB6999@sigill.intra.peff.net","threadId":"32704","inReplyTo":"20130126094026.GA9646@sigill.intra.peff.net","subject":"[PATCH 2/3] logmsg_reencode: never return NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-26T09:44:06Z","receivedAt":"2013-01-26T09:44:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The logmsg_reencode function will return the reencoded\ncommit buffer, or NULL if reencoding failed or no reencoding\nwas necessary. Since every caller then ends up checking for NULL\nand just using the commit's original buffer, anyway, we can\nbe a bit more helpful and just return that buffer when we\nwould have returned NULL.\n\nSince the resulting string may or may not need to be freed,\nwe introduce a logmsg_free, which checks whether the buffer\ncame from the commit object or not (callers either\nimplemented the same check already, or kept two separate\npointers, one to mark the buffer to be used, and one for the\nto-be-freed string).\n\nPushing this logic into logmsg_* simplifies the callers, and\nwill let future patches lazily load the commit buffer in a\nsingle place.\n\nSigned-off-by: Jeff King \n---\n builtin/blame.c | 9 ++++-----\n builtin/commit.c | 14 ++------------\n commit.h | 1 +\n pretty.c | 38 ++++++++++++++++++++++----------------\n 4 files changed, 29 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex b431ba3..962e4e3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1420,7 +1420,7 @@ static void get_commit_info(struct commit *commit,\n {\n \tint len;\n \tconst char *subject, *encoding;\n-\tchar *reencoded, *message;\n+\tchar *message;\n \n \tcommit_info_init(ret);\n \n@@ -1438,14 +1438,13 @@ static void get_commit_info(struct commit *commit,\n \t\t\t sha1_to_hex(commit->object.sha1));\n \t}\n \tencoding = get_log_output_encoding();\n-\treencoded = logmsg_reencode(commit, encoding);\n-\tmessage = reencoded ? reencoded : commit->buffer;\n+\tmessage = logmsg_reencode(commit, encoding);\n \tget_ac_line(message, \"\\nauthor \",\n \t\t &ret->author, &ret->author_mail,\n \t\t &ret->author_time, &ret->author_tz);\n \n \tif (!detailed) {\n-\t\tfree(reencoded);\n+\t\tlogmsg_free(message, commit);\n \t\treturn;\n \t}\n \n@@ -1459,7 +1458,7 @@ static void get_commit_info(struct commit *commit,\n \telse\n \t\tstrbuf_addf(&ret->summary, \"(%s)\", sha1_to_hex(commit->object.sha1));\n \n-\tfree(reencoded);\n+\tlogmsg_free(message, commit);\n }\n \n /*\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fbbb40f..6169f1e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -946,24 +946,14 @@ static const char *read_commit_message(const char *name)\n \n static const char *read_commit_message(const char *name)\n {\n-\tconst char *out_enc, *out;\n+\tconst char *out_enc;\n \tstruct commit *commit;\n \n \tcommit = lookup_commit_reference_by_name(name);\n \tif (!commit)\n \t\tdie(_(\"could not lookup commit %s\"), name);\n \tout_enc = get_commit_output_encoding();\n-\tout = logmsg_reencode(commit, out_enc);\n-\n-\t/*\n-\t * If we failed to reencode the buffer, just copy it\n-\t * byte for byte so the user can try to fix it up.\n-\t * This also handles the case where input and output\n-\t * encodings are identical.\n-\t */\n-\tif (out == NULL)\n-\t\tout = commit->buffer;\n-\treturn out;\n+\treturn logmsg_reencode(commit, out_enc);\n }\n \n static int parse_and_validate_options(int argc, const char *argv[],\ndiff --git a/commit.h b/commit.h\nindex c16c8a7..e770649 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -101,6 +101,7 @@ extern char *logmsg_reencode(const struct commit *commit,\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern char *logmsg_reencode(const struct commit *commit,\n \t\t\t const char *output_encoding);\n+extern void logmsg_free(char *msg, const struct commit *commit);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex 07fc062..c675349 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -524,10 +524,11 @@ static char *get_header(const struct commit *commit, const char *key)\n \tstrbuf_addch(sb, '\\n');\n }\n \n-static char *get_header(const struct commit *commit, const char *key)\n+static char *get_header(const struct commit *commit, const char *msg,\n+\t\t\tconst char *key)\n {\n \tint key_len = strlen(key);\n-\tconst char *line = commit->buffer;\n+\tconst char *line = msg;\n \n \twhile (line) {\n \t\tconst char *eol = strchr(line, '\\n'), *next;\n@@ -588,17 +589,18 @@ char *logmsg_reencode(const struct commit *commit,\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n+\tchar *msg = commit->buffer;\n \tchar *out;\n \n \tif (!output_encoding || !*output_encoding)\n-\t\treturn NULL;\n-\tencoding = get_header(commit, \"encoding\");\n+\t\treturn msg;\n+\tencoding = get_header(commit, msg, \"encoding\");\n \tuse_encoding = encoding ? encoding : utf8;\n \tif (same_encoding(use_encoding, output_encoding))\n \t\tif (encoding) /* we'll strip encoding header later */\n \t\t\tout = xstrdup(commit->buffer);\n \t\telse\n-\t\t\treturn NULL; /* nothing to do */\n+\t\t\treturn msg; /* nothing to do */\n \telse\n \t\tout = reencode_string(commit->buffer,\n \t\t\t\t output_encoding, use_encoding);\n@@ -606,7 +608,17 @@ char *logmsg_reencode(const struct commit *commit,\n \t\tout = replace_encoding_header(out, output_encoding);\n \n \tfree(encoding);\n-\treturn out;\n+\t/*\n+\t * If the re-encoding failed, out might be NULL here; in that\n+\t * case we just return the commit message verbatim.\n+\t */\n+\treturn out ? out : msg;\n+}\n+\n+void logmsg_free(char *msg, const struct commit *commit)\n+{\n+\tif (msg != commit->buffer)\n+\t\tfree(msg);\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\n@@ -1278,14 +1290,11 @@ void format_commit_message(const struct commit *commit,\n \tcontext.pretty_ctx = pretty_ctx;\n \tcontext.wrap_start = sb->len;\n \tcontext.message = logmsg_reencode(commit, output_enc);\n-\tif (!context.message)\n-\t\tcontext.message = commit->buffer;\n \n \tstrbuf_expand(sb, format, format_commit_item, &context);\n \trewrap_message_tail(sb, &context, 0, 0, 0);\n \n-\tif (context.message != commit->buffer)\n-\t\tfree(context.message);\n+\tlogmsg_free(context.message, commit);\n \tfree(context.signature.gpg_output);\n \tfree(context.signature.signer);\n }\n@@ -1432,7 +1441,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,\n {\n \tunsigned long beginning_of_body;\n \tint indent = 4;\n-\tconst char *msg = commit->buffer;\n+\tconst char *msg;\n \tchar *reencoded;\n \tconst char *encoding;\n \tint need_8bit_cte = pp->need_8bit_cte;\n@@ -1443,10 +1452,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,\n \t}\n \n \tencoding = get_log_output_encoding();\n-\treencoded = logmsg_reencode(commit, encoding);\n-\tif (reencoded) {\n-\t\tmsg = reencoded;\n-\t}\n+\tmsg = reencoded = logmsg_reencode(commit, encoding);\n \n \tif (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)\n \t\tindent = 0;\n@@ -1503,7 +1509,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,\n \tif (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)\n \t\tstrbuf_addch(sb, '\\n');\n \n-\tfree(reencoded);\n+\tlogmsg_free(reencoded, commit);\n }\n \n void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,\n-- \n1.8.0.2.16.g72e2fc9\n"},{"id":"207873","messageId":"20130126094428.GC6999@sigill.intra.peff.net","threadId":"32704","inReplyTo":"20130126094026.GA9646@sigill.intra.peff.net","subject":"[PATCH 3/3] logmsg_reencode: lazily load missing commit buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-26T09:44:28Z","receivedAt":"2013-01-26T09:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually a commit that makes it to logmsg_reencode will have\nbeen parsed, and the commit->buffer struct member will be\nvalid. However, some code paths will free commit buffers\nafter having used them (for example, the log traversal\nmachinery will do so to keep memory usage down).\n\nMost of the time this is fine; log should only show a commit\nonce, and then exits. However, there are some code paths\nwhere this does not work. At least two are known:\n\n 1. A commit may be shown as part of a regular ref, and\n then it may be shown again as part of a submodule diff\n (e.g., if a repo contains refs to both the superproject\n and subproject).\n\n 2. A notes-cache commit may be shown during \"log --all\",\n and then later used to access a textconv cache during a\n diff.\n\nLazily loading in logmsg_reencode does not necessarily catch\nall such cases, but it should catch most of them. Users of\nthe commit buffer tend to be either parsing for structure\n(in which they will call parse_commit, and either we will\nalready have parsed, or we will load commit->buffer lazily\nthere), or outputting (either to the user, or fetching a\npart of the commit message via format_commit_message). In\nthe latter case, we should always be using logmsg_reencode\nanyway (and typically we do so via the pretty-print\nmachinery).\n\nIf there are any cases that this misses, we can fix them up\nto use logmsg_reencode (or handle them on a case-by-case\nbasis if that is inappropriate).\n\nSigned-off-by: Jeff King \n---\n builtin/blame.c | 13 ---------\n pretty.c | 57 ++++++++++++++++++++++++++++++++++------\n t/t4042-diff-textconv-caching.sh | 8 ++++++\n 3 files changed, 57 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 962e4e3..86100e9 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1424,19 +1424,6 @@ static void get_commit_info(struct commit *commit,\n \n \tcommit_info_init(ret);\n \n-\t/*\n-\t * We've operated without save_commit_buffer, so\n-\t * we now need to populate them for output.\n-\t */\n-\tif (!commit->buffer) {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\t\tcommit->buffer =\n-\t\t\tread_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!commit->buffer)\n-\t\t\tdie(\"Cannot read commit %s\",\n-\t\t\t sha1_to_hex(commit->object.sha1));\n-\t}\n \tencoding = get_log_output_encoding();\n \tmessage = logmsg_reencode(commit, encoding);\n \tget_ac_line(message, \"\\nauthor \",\ndiff --git a/pretty.c b/pretty.c\nindex c675349..eae57ad 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -592,18 +592,59 @@ char *logmsg_reencode(const struct commit *commit,\n \tchar *msg = commit->buffer;\n \tchar *out;\n \n+\tif (!msg) {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\n+\t\tmsg = read_sha1_file(commit->object.sha1, &type, &size);\n+\t\tif (!msg)\n+\t\t\tdie(\"Cannot read commit object %s\",\n+\t\t\t sha1_to_hex(commit->object.sha1));\n+\t\tif (type != OBJ_COMMIT)\n+\t\t\tdie(\"Expected commit for '%s', got %s\",\n+\t\t\t sha1_to_hex(commit->object.sha1), typename(type));\n+\t}\n+\n \tif (!output_encoding || !*output_encoding)\n \t\treturn msg;\n \tencoding = get_header(commit, msg, \"encoding\");\n \tuse_encoding = encoding ? encoding : utf8;\n-\tif (same_encoding(use_encoding, output_encoding))\n-\t\tif (encoding) /* we'll strip encoding header later */\n-\t\t\tout = xstrdup(commit->buffer);\n-\t\telse\n-\t\t\treturn msg; /* nothing to do */\n-\telse\n-\t\tout = reencode_string(commit->buffer,\n-\t\t\t\t output_encoding, use_encoding);\n+\tif (same_encoding(use_encoding, output_encoding)) {\n+\t\t/*\n+\t\t * No encoding work to be done. If we have no encoding header\n+\t\t * at all, then there's nothing to do, and we can return the\n+\t\t * message verbatim (whether newly allocated or not).\n+\t\t */\n+\t\tif (!encoding)\n+\t\t\treturn msg;\n+\n+\t\t/*\n+\t\t * Otherwise, we still want to munge the encoding header in the\n+\t\t * result, which will be done by modifying the buffer. If we\n+\t\t * are using a fresh copy, we can reuse it. But if we are using\n+\t\t * the cached copy from commit->buffer, we need to duplicate it\n+\t\t * to avoid munging commit->buffer.\n+\t\t */\n+\t\tout = msg;\n+\t\tif (out == commit->buffer)\n+\t\t\tout = xstrdup(out);\n+\t}\n+\telse {\n+\t\t/*\n+\t\t * There's actual encoding work to do. Do the reencoding, which\n+\t\t * still leaves the header to be replaced in the next step. At\n+\t\t * this point, we are done with msg. If we allocated a fresh\n+\t\t * copy, we can free it.\n+\t\t */\n+\t\tout = reencode_string(msg, output_encoding, use_encoding);\n+\t\tif (out && msg != commit->buffer)\n+\t\t\tfree(msg);\n+\t}\n+\n+\t/*\n+\t * This replacement actually consumes the buffer we hand it, so we do\n+\t * not have to worry about freeing the old \"out\" here.\n+\t */\n \tif (out)\n \t\tout = replace_encoding_header(out, output_encoding);\n \ndiff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh\nindex 91f8198..04a44d5 100755\n--- a/t/t4042-diff-textconv-caching.sh\n+++ b/t/t4042-diff-textconv-caching.sh\n@@ -106,4 +106,12 @@ test_expect_success 'switching diff driver produces correct results' '\n \ttest_cmp expect actual\n '\n \n+# The point here is to test that we can log the notes cache and still use it to\n+# produce a diff later (older versions of git would segfault on this). It's\n+# much more likely to come up in the real world with \"log --all -p\", but using\n+# --no-walk lets us reliably reproduce the order of traversal.\n+test_expect_success 'log notes cache and still use cache for -p' '\n+\tgit log --no-walk -p refs/notes/textconv/magic HEAD\n+'\n+\n test_done\n-- \n1.8.0.2.16.g72e2fc9\n"},{"id":"207891","messageId":"7v8v7f1vqa.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"20130126094026.GA9646@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] lazily load commit->buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-26T21:26:53Z","receivedAt":"2013-01-26T21:26:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, agreed. I started to fix this up with a use/unuse pattern and\n> realized something: all of the call sites are calling logmsg_reencode\n> anyway, because that is the next logical step in doing anything with the\n> buffer that is not just parsing out the parent/timestamp/tree info. And\n> since that function already might allocate (for the re-encoded copy),\n> callers have to handle the maybe-borrowed-maybe-free situation already.\n>\n> So I came up with this patch series, which I think should fix the\n> problem, and actually makes the call-sites easier to read, rather than\n> harder.\n>\n> [1/3]: commit: drop useless xstrdup of commit message\n> [2/3]: logmsg_reencode: never return NULL\n> [3/3]: logmsg_reencode: lazily load missing commit buffers\n>\n> Here's the diffstat:\n>\n> builtin/blame.c | 22 ++-------\n> builtin/commit.c | 14 +-----\n> commit.h | 1 +\n> pretty.c | 93 ++++++++++++++++++++++++++---------\n> t/t4042-diff-textconv-caching.sh | 8 +++\n> 5 files changed, 85 insertions(+), 53 deletions(-)\n>\n> Not too bad, and 27 of the lines added in pretty.c are new comments\n> explaining the flow of logmsg_reencode. So even if this doesn't get\n> every case, I think it's a nice cleanup.\n\nThis looks very good.\n\nI wonder if this lets us get rid of the hack in cmd_log_walk() that\ndoes this:\n\n while ((commit = get_revision(rev)) != NULL) {\n if (!log_tree_commit(rev, commit) &&\n rev->max_count >= 0)\n rev->max_count++;\n! if (!rev->reflog_info) {\n! /* we allow cycles in reflog ancestry */\n free(commit->buffer);\n commit->buffer = NULL;\n! }\n free_commit_list(commit->parents);\n commit->parents = NULL;\n\nAfter log_tree_commit() handles the commit, using the buffer, we\ndiscard the memory associated to it because we know we no longer\nwill use it in normal cases.\n\nThe \"do not do that if rev->reflog_info is true\" was added in\na6c7306 (--walk-reflogs: do not crash with cyclic reflog ancestry,\n2007-01-20) because the second and subsequent display of \"commit\"\n(which happens to occur only when walking reflogs) needs to look at\ncommit->buffer again, and this hack forces us to retain the buffer\nfor _all_ commit objects.\n\nBut your patches could be seen as a different (and more correct) way\nto fix the same issue. Once the display side learns how to re-read\nthe log text of the commit object, the above becomes unnecessary, no?\n\nWe may still be helped if majority of commit objects that appear in\nthe reflog appear more than once, in which case retaining the buffer\nfor _all_ commits could be an overall win. Not having to read the\nbuffer for the same commit each time it is shown for majority of\nmultiply-appearing commits, in exchange for having to keep the\nbuffer for commits that appears only once that are minority and\nsuffering increasted page cache pressure. That could be seen as an\noptimization.\n\nBut that is a performance thing, not a correctness issue, so \"we\nallow cycles\" implying \"therefore if we discard the buffer, we will\nshow wrong output\" becomes an incorrect justification.\n\nI happen to have HEAD reflog that is 30k entries long; more than 26k\nrepresent a checkout of unique commit. So I suspect that the above\nhack to excessive retain commit->buffer for already used commits will\nnot help us performance-wise, either.\n"},{"id":"207894","messageId":"20130126221400.GA13827@sigill.intra.peff.net","threadId":"32704","inReplyTo":"7v8v7f1vqa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] lazily load commit->buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-26T22:14:01Z","receivedAt":"2013-01-26T22:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 26, 2013 at 01:26:53PM -0800, Junio C Hamano wrote:\n\n> This looks very good.\n> \n> I wonder if this lets us get rid of the hack in cmd_log_walk() that\n> does this:\n> \n> while ((commit = get_revision(rev)) != NULL) {\n> if (!log_tree_commit(rev, commit) &&\n> rev->max_count >= 0)\n> rev->max_count++;\n> ! if (!rev->reflog_info) {\n> ! /* we allow cycles in reflog ancestry */\n> free(commit->buffer);\n> commit->buffer = NULL;\n> ! }\n> free_commit_list(commit->parents);\n> commit->parents = NULL;\n> \n> After log_tree_commit() handles the commit, using the buffer, we\n> discard the memory associated to it because we know we no longer\n> will use it in normal cases.\n> [...]\n> But that is a performance thing, not a correctness issue, so \"we\n> allow cycles\" implying \"therefore if we discard the buffer, we will\n> show wrong output\" becomes an incorrect justification.\n\nRight. I think the correctness issue goes away with my patches, and it\nis just a question of estimating the workload for performance. I doubt\nit makes a big difference either way, especially when compared to\nactually showing the commit (even a single pathspec limiter, or doing\n\"-p\", would likely dwarf a few extra commit decompressions).\n\nMy HEAD has about 400/3000 non-unique commits, which matches your\nnumbers percentage-wise. Dropping the lines above (and always freeing)\ntakes my best-of-five for \"git log -g\" from 0.085s to 0.080s. Which is\nwell within the noise. Doing \"git log -g Makefile\" ended up at 0.183s\nboth before and after.\n\nSo I suspect it does not matter at all in normal cases, and the time is\nindeed dwarfed by adding even a rudimentary pathspec. I'd be in favor of\ndropping the lines just to decrease complexity of the code.\n\n-Peff\n"},{"id":"207934","messageId":"7vtxq3xkbs.fsf@alter.siamese.dyndns.org","threadId":"32704","inReplyTo":"20130126221400.GA13827@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] lazily load commit->buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T05:32:07Z","receivedAt":"2013-01-27T05:32:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My HEAD has about 400/3000 non-unique commits, which matches your\n> numbers percentage-wise. Dropping the lines above (and always freeing)\n> takes my best-of-five for \"git log -g\" from 0.085s to 0.080s. Which is\n> well within the noise. Doing \"git log -g Makefile\" ended up at 0.183s\n> both before and after.\n>\n> ... I'd be in favor of\n> dropping the lines just to decrease complexity of the code.\n\nI think we are in agreement, then.\n"}]}