{"thread":{"id":"41094","subject":"Segfault in git reflog","startedAt":"2015-12-30T09:24:01Z","lastAt":"2016-01-06T09:30:56Z","messageCount":25,"participants":["Dennis Kaarsemaker","Duy Nguyen","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"275171","messageId":"20151230092400.GA9319@spirit","threadId":"41094","inReplyTo":null,"subject":"Segfault in git reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T09:24:01Z","receivedAt":"2015-12-30T09:24:01Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"I've hit a segfault in git reflog with latest git, reproducable in git.git:\n\nspirit:~/code/git (master)$ ./git describe\nv2.7.0-rc3\n\nI've minimized the reflog to:\n\nspirit:~/code/git (master)$ cat .git/logs/HEAD\n2635c2b8bfc9aec07b7f023d8e3b3d02df715344 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765642 +0100 \n74c855f87d25a5b5c12d0485ec77c785a1c734c5 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765951 +0100 checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n\n...which I realize looks a bit broken. I think at the time I was playing with\nsome patches that also caused segfaults, causing gaps in the reflog.\nNevertheless, I think segfaulting is bad. All objects in the reflog are\nreachable.\n\ngdb has the following to say:\n\nspirit:~/code/git (master)$ gdb --args ./git --no-pager reflog\n(gdb) run\nStarting program: /home/dennis/code/git/git --no-pager reflog\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n28274d0 (HEAD -> master, tag: v2.7.0-rc3, upstream/master, peff/jk/tag-source-propagate, peff/jk/sigpipe-report, gitster/master) HEAD@{0}: checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n\nProgram received signal SIGSEGV, Segmentation fault.\ncopy_commit_list (list=0x4834dc7000000011) at commit.c:450\n450 pp = commit_list_append(list->item, pp);\n(gdb) bt\n#0 copy_commit_list (list=0x4834dc7000000011) at commit.c:450\n#1 0x000000000050705e in save_parents (commit=commit@entry=0x928a90, revs=0x7fffffffcb80) at revision.c:3044\n#2 0x000000000050a54e in get_revision_1 (revs=revs@entry=0x7fffffffcb80) at revision.c:3119\n#3 0x000000000050a710 in get_revision_1 (revs=) at revision.c:3112\n#4 get_revision_internal (revs=0x7fffffffcb80) at revision.c:3248\n#5 0x000000000050a99d in get_revision (revs=revs@entry=0x7fffffffcb80) at revision.c:3322\n#6 0x0000000000446032 in cmd_log_walk (rev=rev@entry=0x7fffffffcb80) at builtin/log.c:344\n#7 0x0000000000446bf8 in cmd_log_reflog (argc=1, argv=0x7fffffffd6a8, prefix=0x0) at builtin/log.c:626\n#8 0x0000000000406126 in run_builtin (argv=0x7fffffffd6a8, argc=1, p=0x7bbec0 ) at git.c:350\n#9 handle_builtin (argc=1, argv=0x7fffffffd6a8) at git.c:536\n#10 0x0000000000405261 in run_argv (argv=0x7fffffffd4c8, argcp=0x7fffffffd4ac) at git.c:582\n#11 main (argc=1, av=) at git.c:690\n(gdb) p list\n$1 = (struct commit_list *) 0x4834dc7000000011\n(gdb) p list->item\nCannot access memory at address 0x4834dc7000000011\n\nA bisect blames 53d00b3 (log: use true parents for diff even when rewriting),\nwhich does indeed touch the code that seems to be segfaulting.\n\nI've tried digging into this, but didn't get very far.\n-- \nDennis Kaarsemaker \n"},{"id":"275174","messageId":"CACsJy8Db-SjNj_tOgERMuRyydq6XC3Fq4pEKNRr8bqgpaUxKig@mail.gmail.com","threadId":"41094","inReplyTo":"20151230092400.GA9319@spirit","subject":"Re: Segfault in git reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-30T10:31:41Z","receivedAt":"2015-12-30T10:31:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 30, 2015 at 4:24 PM, Dennis Kaarsemaker\n wrote:\n> I've hit a segfault in git reflog with latest git, reproducable in git.git:\n>\n> spirit:~/code/git (master)$ ./git describe\n> v2.7.0-rc3\n>\n> I've minimized the reflog to:\n>\n> spirit:~/code/git (master)$ cat .git/logs/HEAD\n> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765642 +0100\n> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765951 +0100 checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n>\n> ...which I realize looks a bit broken. I think at the time I was playing with\n> some patches that also caused segfaults, causing gaps in the reflog.\n> Nevertheless, I think segfaulting is bad. All objects in the reflog are\n> reachable.\n>\n> gdb has the following to say:\n>\n> spirit:~/code/git (master)$ gdb --args ./git --no-pager reflog\n\nIf it helps, if add_ref_decoration is not called at all, there's no\nsegfault. Definitely something with reflog parsing, probably leaving\nan uninitialized pointer. But I have not gotten there yet.\n-- \nDuy\n"},{"id":"275176","messageId":"1451474248.9251.7.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"20151230092400.GA9319@spirit","subject":"Re: Segfault in git reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T11:17:28Z","receivedAt":"2015-12-30T11:17:28Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2015-12-30 at 10:24 +0100, Dennis Kaarsemaker wrote:\n> spirit:~/code/git (master)$ cat .git/logs/HEAD\n> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765642 +0100 \n> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765951 +0100 checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n\n74c855f87d25a5b5c12d0485ec77c785a1c734c5 is actually a tag, pointing to\n3c3d3f629a6176b401ebec455c5dd59ed1b5f910. I'm not sure if that is\nrelevant.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275177","messageId":"CACsJy8CoRs8dNPWag-E947oVTt4R8XbKBLvaQvBPGm4jqZBKNw@mail.gmail.com","threadId":"41094","inReplyTo":"1451474248.9251.7.camel@kaarsemaker.net","subject":"Re: Segfault in git reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-30T11:26:33Z","receivedAt":"2015-12-30T11:26:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 30, 2015 at 6:17 PM, Dennis Kaarsemaker\n wrote:\n> On wo, 2015-12-30 at 10:24 +0100, Dennis Kaarsemaker wrote:\n>> spirit:~/code/git (master)$ cat .git/logs/HEAD\n>> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765642 +0100\n>> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765951 +0100 checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n>\n> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 is actually a tag, pointing to\n> 3c3d3f629a6176b401ebec455c5dd59ed1b5f910. I'm not sure if that is\n> relevant.\n\nIt is. save_parents() expects \"commit\" to be a commit because it needs\ncommit->index, which is not available from struct tag. So when\nsaved_parents_at() tries to read commit->index, it gets random value\n(from the tag). Hell breaks loose from there because this index field\npoints to some memory in the slab mem allocator. The question is, how\ncome a tag is passed in here. Maybe we can sprinkle some\nassert(object->type == OBJ_COMMIT) in revision.c and see..\n-- \nDuy\n"},{"id":"275178","messageId":"CACsJy8Cm6a2FiyZwXsTpzbp7ZkZUGWf=HmjsMtQvJMjzVLkTzA@mail.gmail.com","threadId":"41094","inReplyTo":"CACsJy8CoRs8dNPWag-E947oVTt4R8XbKBLvaQvBPGm4jqZBKNw@mail.gmail.com","subject":"Re: Segfault in git reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-30T11:28:19Z","receivedAt":"2015-12-30T11:28:19Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 30, 2015 at 6:26 PM, Duy Nguyen wrote:\n> On Wed, Dec 30, 2015 at 6:17 PM, Dennis Kaarsemaker\n> wrote:\n>> On wo, 2015-12-30 at 10:24 +0100, Dennis Kaarsemaker wrote:\n>>> spirit:~/code/git (master)$ cat .git/logs/HEAD\n>>> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765642 +0100\n>>> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker 1446765951 +0100 checkout: moving from 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n\nAh... I came from a different angle and did not realize the tag sha1\nis from your reflog. So yeah maybe reflog parsing code should check\nobject type first, don't assume it's a commit!\n\n>>\n>> 74c855f87d25a5b5c12d0485ec77c785a1c734c5 is actually a tag, pointing to\n>> 3c3d3f629a6176b401ebec455c5dd59ed1b5f910. I'm not sure if that is\n>> relevant.\n>\n> It is. save_parents() expects \"commit\" to be a commit because it needs\n> commit->index, which is not available from struct tag. So when\n> saved_parents_at() tries to read commit->index, it gets random value\n> (from the tag). Hell breaks loose from there because this index field\n> points to some memory in the slab mem allocator. The question is, how\n> come a tag is passed in here. Maybe we can sprinkle some\n> assert(object->type == OBJ_COMMIT) in revision.c and see..\n> --\n> Duy\n\n\n\n-- \nDuy\n"},{"id":"275180","messageId":"1451478486.9251.11.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"CACsJy8Cm6a2FiyZwXsTpzbp7ZkZUGWf=HmjsMtQvJMjzVLkTzA@mail.gmail.com","subject":"Re: Segfault in git reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T12:28:06Z","receivedAt":"2015-12-30T12:28:06Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2015-12-30 at 18:28 +0700, Duy Nguyen wrote:\n> On Wed, Dec 30, 2015 at 6:26 PM, Duy Nguyen \n> wrote:\n> > On Wed, Dec 30, 2015 at 6:17 PM, Dennis Kaarsemaker\n> > wrote:\n> >> On wo, 2015-12-30 at 10:24 +0100, Dennis Kaarsemaker wrote:\n> >>> spirit:~/code/git (master)$ cat .git/logs/HEAD\n> >>> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344\n> 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker <\n> dennis@kaarsemaker.net> 1446765642 +0100\n> >>> 74c855f87d25a5b5c12d0485ec77c785a1c734c5\n> 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker <\n> dennis@kaarsemaker.net> 1446765951 +0100 checkout: moving from\n> 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n> \n> Ah... I came from a different angle and did not realize the tag sha1\n> is from your reflog. So yeah maybe reflog parsing code should check\n> object type first, don't assume it's a commit!\n\nSomething like this perhaps?\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..cd538dd 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -25,6 +25,14 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1,\n {\n struct complete_reflogs *array = cb_data;\n struct reflog_info *item;\n+ struct object *obj;\n+\n+ obj = parse_object(osha1);\n+ if(obj && obj->type != OBJ_COMMIT)\n+ die(_(\"Broken reflog, %s is a %s, not a commit\"), sha1_to_hex(obj->oid.hash), typename(obj->type));\n+ obj = parse_object(nsha1);\n+ if(obj && obj->type != OBJ_COMMIT)\n+ die(_(\"Broken reflog, %s is a %s, not a commit\"), sha1_to_hex(obj->oid.hash), typename(obj->type));\n \n ALLOC_GROW(array->items, array->nr + 1, array->alloc);\n item = array->items + array->nr;\n\nThat gives me:\nfatal: Broken reflog, 74c855f87d25a5b5c12d0485ec77c785a1c734c5 is a tag, not a commit\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275182","messageId":"20151230131914.GA27241@lanh","threadId":"41094","inReplyTo":"1451478486.9251.11.camel@kaarsemaker.net","subject":"Re: Segfault in git reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-30T13:19:14Z","receivedAt":"2015-12-30T13:19:14Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 30, 2015 at 01:28:06PM +0100, Dennis Kaarsemaker wrote:\n> On wo, 2015-12-30 at 18:28 +0700, Duy Nguyen wrote:\n> > On Wed, Dec 30, 2015 at 6:26 PM, Duy Nguyen \n> > wrote:\n> > > On Wed, Dec 30, 2015 at 6:17 PM, Dennis Kaarsemaker\n> > > wrote:\n> > >> On wo, 2015-12-30 at 10:24 +0100, Dennis Kaarsemaker wrote:\n> > >>> spirit:~/code/git (master)$ cat .git/logs/HEAD\n> > >>> 2635c2b8bfc9aec07b7f023d8e3b3d02df715344\n> > 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker <\n> > dennis@kaarsemaker.net> 1446765642 +0100\n> > >>> 74c855f87d25a5b5c12d0485ec77c785a1c734c5\n> > 54bc41416c5d3ecb978acb0df80d57aa3e54494c Dennis Kaarsemaker <\n> > dennis@kaarsemaker.net> 1446765951 +0100 checkout: moving from\n> > 3c3d3f629a6176b401ebec455c5dd59ed1b5f910 to master\n> > \n> > Ah... I came from a different angle and did not realize the tag sha1\n> > is from your reflog. So yeah maybe reflog parsing code should check\n> > object type first, don't assume it's a commit!\n> \n> Something like this perhaps?\n> \n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index 85b8a54..cd538dd 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -25,6 +25,14 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1,\n> {\n> struct complete_reflogs *array = cb_data;\n> struct reflog_info *item;\n> + struct object *obj;\n> +\n> + obj = parse_object(osha1);\n> + if(obj && obj->type != OBJ_COMMIT)\n> + die(_(\"Broken reflog, %s is a %s, not a commit\"), sha1_to_hex(obj->oid.hash), typename(obj->type));\n> + obj = parse_object(nsha1);\n> + if(obj && obj->type != OBJ_COMMIT)\n> + die(_(\"Broken reflog, %s is a %s, not a commit\"), sha1_to_hex(obj->oid.hash), typename(obj->type));\n> \n> ALLOC_GROW(array->items, array->nr + 1, array->alloc);\n> item = array->items + array->nr;\n> \n> That gives me:\n> fatal: Broken reflog, 74c855f87d25a5b5c12d0485ec77c785a1c734c5 is a tag, not a commit\n\nI would go with something like this. The typecasting to \"struct commit\n*\" is the bug because parse_object() can return any object type. With\nthis I got\n\nerror: object 74c855f87d25a5b5c12d0485ec77c785a1c734c5 is a tag, not a commit\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..09d18fa 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -236,8 +236,9 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n \tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\tcommit_info->commit = lookup_commit(reflog->osha1);\n+\tif (!commit_info->commit ||\n+\t parse_commit(commit_info->commit)) {\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n--\nDuy\n"},{"id":"275187","messageId":"20151230152245.GA30549@spirit","threadId":"41094","inReplyTo":"20151230131914.GA27241@lanh","subject":"[PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T15:22:49Z","receivedAt":"2015-12-30T15:22:49Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Use lookup_commit instead of parse_object to look up commits mentioned\nin the reflog. This avoids a segfault in save_parents if somehow a sha1\nfor something other than a commit ends up in the reflog.\n\nSigned-off-by: Dennis Kaarsemaker \nHelped-by: Nguyễn Thái Ngọc Duy \n---\nDuy Nguyen wrote:\n\n> I would go with something like this. The typecasting to \"struct commit\n> *\" is the bug because parse_object() can return any object type.\n\nYeah, that's much better. Here it is as a patch with a test. \n\n reflog-walk.c | 4 ++--\n t/t1410-reflog.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..b85c8e8 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -236,8 +236,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n \tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\tcommit_info->commit = lookup_commit(reflog->osha1);\n+\tif (!commit_info->commit || parse_commit(commit_info->commit)) {\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex b79049f..76ccbe5 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -325,4 +325,10 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reflog containing non-commit sha1s' '\n+\tgit checkout -b broken-reflog &&\n+\techo \"$(git rev-parse HEAD^{tree}) $(git rev-parse HEAD) abc 0000000001 +0000\" >> .git/logs/refs/heads/broken-reflog &&\n+\tgit reflog broken-reflog\n+'\n+\n test_done\n-- \n2.7.0-rc1-207-ga35084c\n\n\n-- \nDennis Kaarsemaker \nhttp://twitter.com/seveas\n"},{"id":"275199","messageId":"xmqqege3eiqb.fsf@gitster.mtv.corp.google.com","threadId":"41094","inReplyTo":"20151230152245.GA30549@spirit","subject":"Re: [PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-30T21:20:44Z","receivedAt":"2015-12-30T21:20:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index 85b8a54..b85c8e8 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -236,8 +236,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> \treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> \tinfo->last_commit_reflog = commit_reflog;\n> \tcommit_reflog->recno--;\n> -\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n> -\tif (!commit_info->commit) {\n> +\tcommit_info->commit = lookup_commit(reflog->osha1);\n> +\tif (!commit_info->commit || parse_commit(commit_info->commit)) {\n> \t\tcommit->parents = NULL;\n> \t\treturn;\n\nThis looks somewhat roundabout and illogical. The original was bad\nbecause it blindly assumed reflgo->osha1 refers to a commit without\nmaking sure that assumption holds. Calling lookup_commit() blindly\nis not much better, even though you are helped that the function\nhappens not to barf if the given object is not a commit.\n\nAlso this changes semantics, no? Trace the original flow and think\nwhat happens, when we see a commit object that cannot be parsed in\nparse_commit_buffer(). parse_object() calls parse_object_buffer()\nwhich in turn calls parse_commit_buffer() and the entire callchain\nreturns NULL. commit_info->commit will become NULL in such a case.\n\nWith your code, lookup_commit() will store a non NULL in\ncommit_info->commit, and parse_commit() calls parse_commit_buffer()\nand that would fail, so you clear commit->parents to NULL but fail\nto set commit_info->commit to NULL.\n\nWhy not keep the parse_object() as-is and make sure we error out\nunless the result is a commit with a more explicit check, perhaps\nlike this, instead?\n\n reflog-walk.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..861d7c4 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \tstruct commit_info *commit_info =\n \t\tget_commit_info(commit, &info->reflogs, 0);\n \tstruct commit_reflog *commit_reflog;\n+\tstruct object *logobj;\n \tstruct reflog_info *reflog;\n \n \tinfo->last_commit_reflog = NULL;\n@@ -236,11 +237,13 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n \tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\tlogobj = parse_object(reflog->osha1);\n+\tif (!logobj || logobj->type != OBJ_COMMIT) {\n+\t\tcommit_info->commit = NULL;\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n+\tcommit_info->commit = (struct commit *)logobj;\n \n \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n \tcommit->parents->item = commit_info->commit;\n\n\n> +test_expect_success 'reflog containing non-commit sha1s' '\n> +\tgit checkout -b broken-reflog &&\n> +\techo \"$(git rev-parse HEAD^{tree}) $(git rev-parse HEAD) abc 0000000001 +0000\" >> .git/logs/refs/heads/broken-reflog &&\n> +\tgit reflog broken-reflog\n> +'\n> +\n\nThis will negatively affect the ongoing effort to abstract out the\non-disk implementation of the reflog. In some future installation\nof Git, the reflog may not even be in .git/logs/refs/whatever file.\n\nUse a non-branch ref, so that you can store any valid object not\njust commits, and use a Git command (e.g. \"git update-ref\" or \"git\ntag\") instead of the raw filesystem access to update it, perhaps\nlike this?\n\n\tgit tag --create-reflog test-logs HEAD^ &&\n\tgit tag -f test-logs HEAD^{tree} &&\n\tgit tag -f test-logs HEAD &&\n\tgit reflog test-logs\n"},{"id":"275202","messageId":"1451511208.9251.21.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"xmqqege3eiqb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T21:33:28Z","receivedAt":"2015-12-30T21:33:28Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2015-12-30 at 13:20 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker writes:\n> \n> > diff --git a/reflog-walk.c b/reflog-walk.c\n> > index 85b8a54..b85c8e8 100644\n> > --- a/reflog-walk.c\n> > +++ b/reflog-walk.c\n> > @@ -236,8 +236,8 @@ void fake_reflog_parent(struct reflog_walk_info\n> > *info, struct commit *commit)\n> > \treflog = &commit_reflog->reflogs->items[commit_reflog\n> > ->recno];\n> > \tinfo->last_commit_reflog = commit_reflog;\n> > \tcommit_reflog->recno--;\n> > -\tcommit_info->commit = (struct commit *)parse_object(reflog\n> > ->osha1);\n> > -\tif (!commit_info->commit) {\n> > +\tcommit_info->commit = lookup_commit(reflog->osha1);\n> > +\tif (!commit_info->commit || parse_commit(commit_info\n> > ->commit)) {\n> > \t\tcommit->parents = NULL;\n> > \t\treturn;\n> \n> This looks somewhat roundabout and illogical. The original was bad\n> because it blindly assumed reflgo->osha1 refers to a commit without\n> making sure that assumption holds. Calling lookup_commit() blindly\n> is not much better, even though you are helped that the function\n> happens not to barf if the given object is not a commit.\n> \n> Also this changes semantics, no? Trace the original flow and think\n> what happens, when we see a commit object that cannot be parsed in\n> parse_commit_buffer(). parse_object() calls parse_object_buffer()\n> which in turn calls parse_commit_buffer() and the entire callchain\n> returns NULL. commit_info->commit will become NULL in such a case.\n> \n> With your code, lookup_commit() will store a non NULL in\n> commit_info->commit, and parse_commit() calls parse_commit_buffer()\n> and that would fail, so you clear commit->parents to NULL but fail\n> to set commit_info->commit to NULL.\n>\n> Why not keep the parse_object() as-is and make sure we error out\n> unless the result is a commit with a more explicit check, perhaps\n> like this, instead?\n\nlookup_commit actually returns NULL (via object_as_type) for objects\nthat are not commits, so I don't think the above is true. The code\nbelow also loses the diagnostic message about the object not being a\ncommit.\n\n> reflog-walk.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index 85b8a54..861d7c4 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info\n> *info, struct commit *commit)\n> \tstruct commit_info *commit_info =\n> \t\tget_commit_info(commit, &info->reflogs, 0);\n> \tstruct commit_reflog *commit_reflog;\n> +\tstruct object *logobj;\n> \tstruct reflog_info *reflog;\n> \n> \tinfo->last_commit_reflog = NULL;\n> @@ -236,11 +237,13 @@ void fake_reflog_parent(struct reflog_walk_info\n> *info, struct commit *commit)\n> \treflog = &commit_reflog->reflogs->items[commit_reflog\n> ->recno];\n> \tinfo->last_commit_reflog = commit_reflog;\n> \tcommit_reflog->recno--;\n> -\tcommit_info->commit = (struct commit *)parse_object(reflog\n> ->osha1);\n> -\tif (!commit_info->commit) {\n> +\tlogobj = parse_object(reflog->osha1);\n> +\tif (!logobj || logobj->type != OBJ_COMMIT) {\n> +\t\tcommit_info->commit = NULL;\n> \t\tcommit->parents = NULL;\n> \t\treturn;\n> \t}\n> +\tcommit_info->commit = (struct commit *)logobj;\n> \n> \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n> \tcommit->parents->item = commit_info->commit;\n> \n> \n> > +test_expect_success 'reflog containing non-commit sha1s' '\n> > +\tgit checkout -b broken-reflog &&\n> > +\techo \"$(git rev-parse HEAD^{tree}) $(git rev-parse HEAD)\n> > abc 0000000001 +0000\" >> .git/logs/refs/heads/broken-reflog\n> > &&\n> > +\tgit reflog broken-reflog\n> > +'\n> > +\n> \n> This will negatively affect the ongoing effort to abstract out the\n> on-disk implementation of the reflog. In some future installation\n> of Git, the reflog may not even be in .git/logs/refs/whatever file.\n\nI was following the style of the test above it, will fix.\n\n> Use a non-branch ref, so that you can store any valid object not\n> just commits, and use a Git command (e.g. \"git update-ref\" or \"git\n> tag\") instead of the raw filesystem access to update it, perhaps\n> like this?\n> \n> \tgit tag --create-reflog test-logs HEAD^ &&\n> \tgit tag -f test-logs HEAD^{tree} &&\n> \tgit tag -f test-logs HEAD &&\n> \tgit reflog test-logs\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275204","messageId":"xmqq1ta3ehr1.fsf@gitster.mtv.corp.google.com","threadId":"41094","inReplyTo":"1451511208.9251.21.camel@kaarsemaker.net","subject":"Re: [PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-30T21:41:54Z","receivedAt":"2015-12-30T21:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> On wo, 2015-12-30 at 13:20 -0800, Junio C Hamano wrote:\n>> Dennis Kaarsemaker writes:\n>> \n>> > diff --git a/reflog-walk.c b/reflog-walk.c\n>> > index 85b8a54..b85c8e8 100644\n>> > --- a/reflog-walk.c\n>> > +++ b/reflog-walk.c\n>> > @@ -236,8 +236,8 @@ void fake_reflog_parent(struct reflog_walk_info\n>> > *info, struct commit *commit)\n>> > \treflog = &commit_reflog->reflogs->items[commit_reflog\n>> > ->recno];\n>> > \tinfo->last_commit_reflog = commit_reflog;\n>> > \tcommit_reflog->recno--;\n>> > -\tcommit_info->commit = (struct commit *)parse_object(reflog\n>> > ->osha1);\n>> > -\tif (!commit_info->commit) {\n>> > +\tcommit_info->commit = lookup_commit(reflog->osha1);\n>> > +\tif (!commit_info->commit || parse_commit(commit_info\n>> > ->commit)) {\n>> > \t\tcommit->parents = NULL;\n>> > \t\treturn;\n>> \n>> This looks somewhat roundabout and illogical. The original was bad\n>> because it blindly assumed reflgo->osha1 refers to a commit without\n>> making sure that assumption holds. Calling lookup_commit() blindly\n>> is not much better, even though you are helped that the function\n>> happens not to barf if the given object is not a commit.\n>> \n>> Also this changes semantics, no? Trace the original flow and think\n>> what happens, when we see a commit object that cannot be parsed in\n>> parse_commit_buffer(). parse_object() calls parse_object_buffer()\n>> which in turn calls parse_commit_buffer() and the entire callchain\n>> returns NULL. commit_info->commit will become NULL in such a case.\n>> \n>> With your code, lookup_commit() will store a non NULL in\n>> commit_info->commit, and parse_commit() calls parse_commit_buffer()\n>> and that would fail, so you clear commit->parents to NULL but fail\n>> to set commit_info->commit to NULL.\n>>\n>> Why not keep the parse_object() as-is and make sure we error out\n>> unless the result is a commit with a more explicit check, perhaps\n>> like this, instead?\n>\n> lookup_commit actually returns NULL (via object_as_type) for objects\n> that are not commits, so I don't think the above is true.\n\nI think you did not read what you are responding to. I was talking\nabout the error case where the object _is_ a commit (hence lookup\nreturns it), but parse_commit_buffer() does not like its contents.\n\n> The code below also loses the diagnostic message about the object\n> not being a commit.\n\nGiving such a diagnostic message is a BUG.\n\nA ref can legitimately point at any type of object (only refs under\nrefs/heads/, aka \"branches\", must point at commits), so you MUST NOT\ncomplain about seeing a non-commit in a reflog in general.\n"},{"id":"275207","messageId":"1451512152.9251.23.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"xmqq1ta3ehr1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T21:49:12Z","receivedAt":"2015-12-30T21:49:12Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2015-12-30 at 13:41 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker writes:\n> \n> > On wo, 2015-12-30 at 13:20 -0800, Junio C Hamano wrote:\n> > > Dennis Kaarsemaker writes:\n> > > \n> > > > diff --git a/reflog-walk.c b/reflog-walk.c\n> > > > index 85b8a54..b85c8e8 100644\n> > > > --- a/reflog-walk.c\n> > > > +++ b/reflog-walk.c\n> > > > @@ -236,8 +236,8 @@ void fake_reflog_parent(struct\n> > > > reflog_walk_info\n> > > > *info, struct commit *commit)\n> > > > \treflog = &commit_reflog->reflogs->items[commit_reflog\n> > > > ->recno];\n> > > > \tinfo->last_commit_reflog = commit_reflog;\n> > > > \tcommit_reflog->recno--;\n> > > > -\tcommit_info->commit = (struct commit\n> > > > *)parse_object(reflog\n> > > > ->osha1);\n> > > > -\tif (!commit_info->commit) {\n> > > > +\tcommit_info->commit = lookup_commit(reflog->osha1);\n> > > > +\tif (!commit_info->commit || parse_commit(commit_info\n> > > > ->commit)) {\n> > > > \t\tcommit->parents = NULL;\n> > > > \t\treturn;\n> > > \n> > > This looks somewhat roundabout and illogical. The original was\n> > > bad\n> > > because it blindly assumed reflgo->osha1 refers to a commit\n> > > without\n> > > making sure that assumption holds. Calling lookup_commit()\n> > > blindly\n> > > is not much better, even though you are helped that the function\n> > > happens not to barf if the given object is not a commit.\n> > > \n> > > Also this changes semantics, no? Trace the original flow and\n> > > think\n> > > what happens, when we see a commit object that cannot be parsed\n> > > in\n> > > parse_commit_buffer(). parse_object() calls\n> > > parse_object_buffer()\n> > > which in turn calls parse_commit_buffer() and the entire\n> > > callchain\n> > > returns NULL. commit_info->commit will become NULL in such a\n> > > case.\n> > > \n> > > With your code, lookup_commit() will store a non NULL in\n> > > commit_info->commit, and parse_commit() calls\n> > > parse_commit_buffer()\n> > > and that would fail, so you clear commit->parents to NULL but\n> > > fail\n> > > to set commit_info->commit to NULL.\n> > > \n> > > Why not keep the parse_object() as-is and make sure we error out\n> > > unless the result is a commit with a more explicit check, perhaps\n> > > like this, instead?\n> > \n> > lookup_commit actually returns NULL (via object_as_type) for\n> > objects\n> > that are not commits, so I don't think the above is true.\n> \n> I think you did not read what you are responding to. I was talking\n> about the error case where the object _is_ a commit (hence lookup\n> returns it), but parse_commit_buffer() does not like its contents.\n\nI read it, but misunderstood it. Thanks for clarifying.\n\n> > The code below also loses the diagnostic message about the object\n> > not being a commit.\n> \n> Giving such a diagnostic message is a BUG.\n> \n> A ref can legitimately point at any type of object (only refs under\n> refs/heads/, aka \"branches\", must point at commits), so you MUST NOT\n> complain about seeing a non-commit in a reflog in general.\n\nYeah, that makes sense, didn't think of that.\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275209","messageId":"20151230221705.GA4025@spirit","threadId":"41094","inReplyTo":"1451512152.9251.23.camel@kaarsemaker.net","subject":"[PATCH v2] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T22:17:08Z","receivedAt":"2015-12-30T22:17:08Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Use lookup_commit instead of parse_object to look up commits mentioned\nin the reflog. This avoids a segfault in save_parents if somehow a sha1\nfor something other than a commit ends up in the reflog.\n\nSigned-off-by: Dennis Kaarsemaker \nHelped-by: Nguyễn Thái Ngọc Duy \nHelped-by: Junio C Hamano \n---\n reflog-walk.c | 7 +++++--\n t/t1410-reflog.sh | 13 +++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\nThis version implements Junio's way of doing this, to avoid spurious warnings\nand to handle broken commit objects better.\n\nI also added a failing test to show that git reflog still fails to display the\nfull reflog when it encounters such non-commit objects (it just stops when it\nseems them). I don't know how to fix that.\n\nThe really correct way of fixing this bug may actually be a level higher, and\nmaking git reflog not rely on information about parent commits, whether they\nare fake or not.\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..861d7c4 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \tstruct commit_info *commit_info =\n \t\tget_commit_info(commit, &info->reflogs, 0);\n \tstruct commit_reflog *commit_reflog;\n+\tstruct object *logobj;\n \tstruct reflog_info *reflog;\n \n \tinfo->last_commit_reflog = NULL;\n@@ -236,11 +237,13 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n \tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\tlogobj = parse_object(reflog->osha1);\n+\tif (!logobj || logobj->type != OBJ_COMMIT) {\n+\t\tcommit_info->commit = NULL;\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n+\tcommit_info->commit = (struct commit *)logobj;\n \n \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n \tcommit->parents->item = commit_info->commit;\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex b79049f..130d671 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n+\tgit update-ref --create-reflog -m \"Creating ref\" \\\n+\t\trefs/tests/tree-in-reflog HEAD &&\n+\tgit update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n+\tgit update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n+\tgit reflog refs/tests/tree-in-reflog\n+'\n+\n+test_expect_failure 'reflog containing non-commit sha1s displays fully' '\n+\tgit reflog refs/tests/tree-in-reflog > actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n test_done\n-- \n2.7.0-rc1-207-ga35084c\n"},{"id":"275212","messageId":"xmqqk2nvd0cz.fsf@gitster.mtv.corp.google.com","threadId":"41094","inReplyTo":"20151230221705.GA4025@spirit","subject":"Re: [PATCH v2] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-30T22:42:52Z","receivedAt":"2015-12-30T22:42:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> The really correct way of fixing this bug may actually be a level higher, and\n> making git reflog not rely on information about parent commits, whether they\n> are fake or not.\n\nI tend to agree.\n\nThe only common thing between \"git reflog\" wants to do (i.e. showing\nthe objects referred to by reflog entries) and what the normal \"git\nlog\" was/is designed to do is \"we have many things, and we show them\none by one\". As the \"many things\" we have in the context of the\nnormal \"git log\" are all commits, it is reasonable that the internal\ninterface (i.e. revision.c::get_revision()) to iterate over these\n\"many things\" returns a \"struct commit *\" and it also is reasonable\nthat \"show them one by one\" is done by calling log_tree_commit() in\nbuiltin/log.c::cmd_log_walk(). Neither is suitable to deal with\nseries of reflog entries in general. A proper implementation of\n\"git reflog\" would have liked to be able to iterate over \"many\nthings\" by returning \"struct object *\" one-by-one, and then do the\nequivalent of the switch() statement in builtin/log.c::cmd_show()\nto show these objects.\n\nThe way \"git log\" was abused and made to show entries from reflog is\none of the ugly and unfortunate hacks in our codebase.\n\nHowever, I see that there are one of two things that you could do to\nmake this part of code do slightly better than stopping at the first\nnon-commit object:\n\n - pretend that the non-commit entry never existed in the first\n place and return the commit that appears in the reflog next.\n\n - fabricate a fake \"commit\" object that says \"I am not a commit;\n I merely exist to represent that the reflog you are walking has\n this non-commit object at this point in the sequence\" and return\n it, instead of giving NULL in the error path.\n\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index b79049f..130d671 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n> +\tgit update-ref --create-reflog -m \"Creating ref\" \\\n> +\t\trefs/tests/tree-in-reflog HEAD &&\n> +\tgit update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n> +\tgit update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n> +\tgit reflog refs/tests/tree-in-reflog\n> +'\n> +\n> +test_expect_failure 'reflog containing non-commit sha1s displays fully' '\n> +\tgit reflog refs/tests/tree-in-reflog > actual &&\n\nPlease write this without space after the redirection operator, i.e.\n\n\tgit reflog refs/tests/tree-in-reflog >actual &&\n\n> +\ttest_line_count = 3 actual\n> +'\n> +\n> test_done\n"},{"id":"275218","messageId":"20151230233301.GA9194@spirit","threadId":"41094","inReplyTo":"xmqqk2nvd0cz.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-30T23:33:03Z","receivedAt":"2015-12-30T23:33:03Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"git reflog (ab)uses the log machinery to display its list of log\nentries. To do so it must fake commit parent information for the log\nwalker.\n\nFor refs in refs/heads this is no problem, as they should only ever\npoint to commits. Tags and other refs however can point to anything,\nthus their reflog may contain non-commit objects.\n\nTo avoid segfaulting, we check whether reflog entries are commits before\nfeeding them to the log walker and skip any non-commits. This means that\ngit reflog output will be incomplete for such refs, but that's one step\nup from segfaulting. A more complete solution would be to decouple git\nreflog from the log walker machinery.\n\nSigned-off-by: Dennis Kaarsemaker \nHelped-by: Nguyễn Thái Ngọc Duy \nHelped-by: Junio C Hamano \n---\n reflog-walk.c | 16 +++++++++++-----\n t/t1410-reflog.sh | 13 +++++++++++++\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\nJunio C Hamano wrote:\n\n> However, I see that there are one of two things that you could do to\n> make this part of code do slightly better than stopping at the first\n> non-commit object:\n> \n> - pretend that the non-commit entry never existed in the first\n> place and return the commit that appears in the reflog next.\n\nThis turned out to be doable in the same code segment: just keep on\nprocessing reflog entries until you hit a commit or run out of entries.\nThat (and the updated foremerly-failing test) are the only changes\nbetween v2 and v3.\n\nI'll try to actually implement the proper solution, but that'll take a\nwhile. Until then, this at least stops a segfault :)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..0ebd1da 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \tstruct commit_info *commit_info =\n \t\tget_commit_info(commit, &info->reflogs, 0);\n \tstruct commit_reflog *commit_reflog;\n+\tstruct object *logobj;\n \tstruct reflog_info *reflog;\n \n \tinfo->last_commit_reflog = NULL;\n@@ -232,15 +233,20 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n-\n-\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n-\tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\n+\tdo {\n+\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n+\t\tcommit_reflog->recno--;\n+\t\tlogobj = parse_object(reflog->osha1);\n+\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n+\n+\tif (!logobj || logobj->type != OBJ_COMMIT) {\n+\t\tcommit_info->commit = NULL;\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n+\tcommit_info->commit = (struct commit *)logobj;\n \n \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n \tcommit->parents->item = commit_info->commit;\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex b79049f..f97513c 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n+\tgit update-ref --create-reflog -m \"Creating ref\" \\\n+\t\trefs/tests/tree-in-reflog HEAD &&\n+\tgit update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n+\tgit update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n+\tgit reflog refs/tests/tree-in-reflog\n+'\n+\n+test_expect_success 'reflog containing non-commit sha1s displays properly' '\n+\tgit reflog refs/tests/tree-in-reflog >actual &&\n+\ttest_line_count = 2 actual\n+'\n+\n test_done\n-- \n2.7.0-rc1-207-ga35084c\n\n\n-- \nDennis Kaarsemaker \nhttp://twitter.com/seveas\n"},{"id":"275220","messageId":"xmqq37ujcwny.fsf@gitster.mtv.corp.google.com","threadId":"41094","inReplyTo":"20151230233301.GA9194@spirit","subject":"Re: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-31T00:02:41Z","receivedAt":"2015-12-31T00:02:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> This turned out to be doable in the same code segment: just keep on\n> processing reflog entries until you hit a commit or run out of entries.\n> That (and the updated foremerly-failing test) are the only changes\n> between v2 and v3.\n>\n> I'll try to actually implement the proper solution, but that'll take a\n> while. Until then, this at least stops a segfault :)\n\nYeah, that would be an ambitious project.\n\n> diff --git a/reflog-walk.c b/reflog-walk.c\n> index 85b8a54..0ebd1da 100644\n> --- a/reflog-walk.c\n> +++ b/reflog-walk.c\n> @@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> \tstruct commit_info *commit_info =\n> \t\tget_commit_info(commit, &info->reflogs, 0);\n> \tstruct commit_reflog *commit_reflog;\n> +\tstruct object *logobj;\n\nThis thing is not initialized...\n\n> \tstruct reflog_info *reflog;\n> \n> \tinfo->last_commit_reflog = NULL;\n> @@ -232,15 +233,20 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n> \t\tcommit->parents = NULL;\n> \t\treturn;\n> \t}\n> -\n> -\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> \tinfo->last_commit_reflog = commit_reflog;\n> -\tcommit_reflog->recno--;\n> -\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n> -\tif (!commit_info->commit) {\n> +\n> +\tdo {\n> +\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n> +\t\tcommit_reflog->recno--;\n> +\t\tlogobj = parse_object(reflog->osha1);\n> +\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n\nBut this loop runs at least once, so logobj will always have some\nsane value when the loop exits.\n\n> +\tif (!logobj || logobj->type != OBJ_COMMIT) {\n\nAnd the only case where this should trigger is when we ran out of\nrecno. Am I reading the updated code correctly?\n\nWith the updated code, the number of times we return from this\nfunction is different from the number initially set to recno. I had\nto wonder if the caller cares and misbehaves, but the caller does\nnot know how long the reflog is before starting to call\nget_revision() in a loop anyway, so it already has to deal with a\ncase where it did .recno=20 and get_revision() did not return that\nmany times. So this probably is safe.\n\n> +\t\tcommit_info->commit = NULL;\n> \t\tcommit->parents = NULL;\n> \t\treturn;\n> \t}\n\n> +\tcommit_info->commit = (struct commit *)logobj;\n> \n> \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n> \tcommit->parents->item = commit_info->commit;\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index b79049f..f97513c 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n> +\tgit update-ref --create-reflog -m \"Creating ref\" \\\n> +\t\trefs/tests/tree-in-reflog HEAD &&\n> +\tgit update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n> +\tgit update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n> +\tgit reflog refs/tests/tree-in-reflog\n> +'\n> +\n> +test_expect_success 'reflog containing non-commit sha1s displays properly' '\n\nIn general, \"properly\" is a poor word to use in test description (or\na commit log message or a bug report, for that matter), as the whole\npoint of a test is to precisely define what is \"proper\".\n\nAnd the code change declares that a proper thing to do is to omit\nnon-commit entries without segfaulting, so something like\n\n s/displays properly/omits them/\n\nperhaps?\n\n> +\tgit reflog refs/tests/tree-in-reflog >actual &&\n> +\ttest_line_count = 2 actual\n> +'\n> +\n> test_done\n> -- \n> 2.7.0-rc1-207-ga35084c\n"},{"id":"275233","messageId":"1451552227.11138.6.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"xmqq37ujcwny.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-31T08:57:07Z","receivedAt":"2015-12-31T08:57:07Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On wo, 2015-12-30 at 16:02 -0800, Junio C Hamano wrote:\n> Dennis Kaarsemaker writes:\n> \n> > diff --git a/reflog-walk.c b/reflog-walk.c\n> > index 85b8a54..0ebd1da 100644\n> > --- a/reflog-walk.c\n> > +++ b/reflog-walk.c\n> > @@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info\n> > *info, struct commit *commit)\n> > \tstruct commit_info *commit_info =\n> > \t\tget_commit_info(commit, &info->reflogs, 0);\n> > \tstruct commit_reflog *commit_reflog;\n> > +\tstruct object *logobj;\n> \n> This thing is not initialized...\n> \n> > \tstruct reflog_info *reflog;\n> > \n> > \tinfo->last_commit_reflog = NULL;\n> > @@ -232,15 +233,20 @@ void fake_reflog_parent(struct\n> > reflog_walk_info *info, struct commit *commit)\n> > \t\tcommit->parents = NULL;\n> > \t\treturn;\n> > \t}\n> > -\n> > -\treflog = &commit_reflog->reflogs->items[commit_reflog\n> > ->recno];\n> > \tinfo->last_commit_reflog = commit_reflog;\n> > -\tcommit_reflog->recno--;\n> > -\tcommit_info->commit = (struct commit *)parse_object(reflog\n> > ->osha1);\n> > -\tif (!commit_info->commit) {\n> > +\n> > +\tdo {\n> > +\t\treflog = &commit_reflog->reflogs\n> > ->items[commit_reflog->recno];\n> > +\t\tcommit_reflog->recno--;\n> > +\t\tlogobj = parse_object(reflog->osha1);\n> > +\t} while (commit_reflog->recno && (logobj && logobj->type\n> > != OBJ_COMMIT));\n> \n> But this loop runs at least once, so logobj will always have some\n> sane value when the loop exits.\n> \n> > +\tif (!logobj || logobj->type != OBJ_COMMIT) {\n> \n> And the only case where this should trigger is when we ran out of\n> recno. Am I reading the updated code correctly?\n\nYes, your description matches what I tried to implement.\n\n> With the updated code, the number of times we return from this\n> function is different from the number initially set to recno. I had\n> to wonder if the caller cares and misbehaves, but the caller does\n> not know how long the reflog is before starting to call\n> get_revision() in a loop anyway, so it already has to deal with a\n> case where it did .recno=20 and get_revision() did not return that\n> many times. So this probably is safe.\n\nThat corresponds to what I see when experimenting with different\nreflogs.\n\n> > +test_expect_success 'reflog containing non-commit sha1s displays\n> > properly' '\n> \n> In general, \"properly\" is a poor word to use in test description (or\n> a commit log message or a bug report, for that matter), as the whole\n> point of a test is to precisely define what is \"proper\".\n> \n> And the code change declares that a proper thing to do is to omit\n> non-commit entries without segfaulting, so something like\n> \n> s/displays properly/omits them/\n> \n> perhaps?\n\nI did find the test title a bit iffy but couldn't really figure out\nwhy. What you're saying makes a lot of sense, will fix.\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275246","messageId":"1451576580.11138.14.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"1451552227.11138.6.camel@kaarsemaker.net","subject":"Re: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-12-31T15:43:00Z","receivedAt":"2015-12-31T15:43:00Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On do, 2015-12-31 at 09:57 +0100, Dennis Kaarsemaker wrote:\n> > > +test_expect_success 'reflog containing non-commit sha1s displays\n> > > properly' '\n> > \n> > In general, \"properly\" is a poor word to use in test description\n> (or\n> > a commit log message or a bug report, for that matter), as the\n> whole\n> > point of a test is to precisely define what is \"proper\".\n> > \n> > And the code change declares that a proper thing to do is to omit\n> > non-commit entries without segfaulting, so something like\n> > \n> > s/displays properly/omits them/\n> > \n> > perhaps?\n> \n> I did find the test title a bit iffy but couldn't really figure out\n> why. What you're saying makes a lot of sense, will fix.\n\nThinking about it a bit more: 'proper' would be to show everything, we\njust expect that not to work yet. So I'll make it\n\ntest_expect_failure 'reflog with non-commit entries displays all entries' '\n\tgit reflog refs/tests/tree-in-reflog >actual &&\n\ttest_line_count = 3 actual\n'\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275399","messageId":"20160105211206.GA12057@spirit","threadId":"41094","inReplyTo":"1451552227.11138.6.camel@kaarsemaker.net","subject":"[PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-01-05T21:12:10Z","receivedAt":"2016-01-05T21:12:10Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"git reflog (ab)uses the log machinery to display its list of log\nentries. To do so it must fake commit parent information for the log\nwalker.\n\nFor refs in refs/heads this is no problem, as they should only ever\npoint to commits. Tags and other refs however can point to anything,\nthus their reflog may contain non-commit objects.\n\nTo avoid segfaulting, we check whether reflog entries are commits before\nfeeding them to the log walker and skip any non-commits. This means that\ngit reflog output will be incomplete for such refs, but that's one step\nup from segfaulting. A more complete solution would be to decouple git\nreflog from the log walker machinery.\n\nSigned-off-by: Dennis Kaarsemaker \nHelped-by: Nguyễn Thái Ngọc Duy \nHelped-by: Junio C Hamano \n---\nChanges since v3: tweaks to the second test.\n\n reflog-walk.c | 16 +++++++++++-----\n t/t1410-reflog.sh | 13 +++++++++++++\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 85b8a54..0ebd1da 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \tstruct commit_info *commit_info =\n \t\tget_commit_info(commit, &info->reflogs, 0);\n \tstruct commit_reflog *commit_reflog;\n+\tstruct object *logobj;\n \tstruct reflog_info *reflog;\n \n \tinfo->last_commit_reflog = NULL;\n@@ -232,15 +233,20 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n-\n-\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n \tinfo->last_commit_reflog = commit_reflog;\n-\tcommit_reflog->recno--;\n-\tcommit_info->commit = (struct commit *)parse_object(reflog->osha1);\n-\tif (!commit_info->commit) {\n+\n+\tdo {\n+\t\treflog = &commit_reflog->reflogs->items[commit_reflog->recno];\n+\t\tcommit_reflog->recno--;\n+\t\tlogobj = parse_object(reflog->osha1);\n+\t} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));\n+\n+\tif (!logobj || logobj->type != OBJ_COMMIT) {\n+\t\tcommit_info->commit = NULL;\n \t\tcommit->parents = NULL;\n \t\treturn;\n \t}\n+\tcommit_info->commit = (struct commit *)logobj;\n \n \tcommit->parents = xcalloc(1, sizeof(struct commit_list));\n \tcommit->parents->item = commit_info->commit;\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex b79049f..17a194b 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n+\tgit update-ref --create-reflog -m \"Creating ref\" \\\n+\t\trefs/tests/tree-in-reflog HEAD &&\n+\tgit update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n+\tgit update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n+\tgit reflog refs/tests/tree-in-reflog\n+'\n+\n+test_expect_failure 'reflog with non-commit entries displays all entries' '\n+\tgit reflog refs/tests/tree-in-reflog >actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n test_done\n-- \n2.7.0-rc3-219-g24972d4\n\n\n-- \nDennis Kaarsemaker \nhttp://twitter.com/seveas\n"},{"id":"275408","messageId":"CAPig+cRufd4qOwZRpw2TR39npkRGg=7S+7YwfSu6EvRR95kRSA@mail.gmail.com","threadId":"41094","inReplyTo":"20160105211206.GA12057@spirit","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-06T01:05:46Z","receivedAt":"2016-01-06T01:05:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker\n wrote:\n> git reflog (ab)uses the log machinery to display its list of log\n> entries. To do so it must fake commit parent information for the log\n> walker.\n>\n> For refs in refs/heads this is no problem, as they should only ever\n> point to commits. Tags and other refs however can point to anything,\n> thus their reflog may contain non-commit objects.\n>\n> To avoid segfaulting, we check whether reflog entries are commits before\n> feeding them to the log walker and skip any non-commits. This means that\n> git reflog output will be incomplete for such refs, but that's one step\n> up from segfaulting. A more complete solution would be to decouple git\n> reflog from the log walker machinery.\n>\n> Signed-off-by: Dennis Kaarsemaker \n> ---\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '\n> +test_expect_success 'no segfaults for reflog containing non-commit sha1s' '\n\nNit: It's kind of strange for a test title to talk about not\nsegfaulting; that's behavior you'd expect to be true for all tests.\nPerhaps describe it as \"non-commit reflog entries handled sanely\" or\nsomething.\n\n> + git update-ref --create-reflog -m \"Creating ref\" \\\n> + refs/tests/tree-in-reflog HEAD &&\n> + git update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog HEAD^{tree} &&\n> + git update-ref -m \"Restoring to commit\" refs/tests/tree-in-reflog HEAD &&\n> + git reflog refs/tests/tree-in-reflog\n> +'\n\nHmm, this test is successful for me on OS X even without the\nreflog-walk.c changes applied.\n\n> +test_expect_failure 'reflog with non-commit entries displays all entries' '\n> + git reflog refs/tests/tree-in-reflog >actual &&\n> + test_line_count = 3 actual\n> +'\n\nAnd this test actually fails (inversely) because it's expecting a\nfailure, but doesn't get one since the command produces the expected\noutput.\n\nBy the way, it may make sense to combine these two tests. If a\nsegfault occurs, the actual output likely will not match the expected\noutput, thus the test will fail anyhow (unless the segfault occurs\nafter all output).\n\n> +\n> test_done\n> --\n> 2.7.0-rc3-219-g24972d4\n"},{"id":"275410","messageId":"1452043212.5562.18.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"CAPig+cRufd4qOwZRpw2TR39npkRGg=7S+7YwfSu6EvRR95kRSA@mail.gmail.com","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-01-06T01:20:12Z","receivedAt":"2016-01-06T01:20:12Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker\n> wrote:\n> > git reflog (ab)uses the log machinery to display its list of log\n> > entries. To do so it must fake commit parent information for the\n> > log\n> > walker.\n> > \n> > For refs in refs/heads this is no problem, as they should only ever\n> > point to commits. Tags and other refs however can point to\n> > anything,\n> > thus their reflog may contain non-commit objects.\n> > \n> > To avoid segfaulting, we check whether reflog entries are commits\n> > before\n> > feeding them to the log walker and skip any non-commits. This means\n> > that\n> > git reflog output will be incomplete for such refs, but that's one\n> > step\n> > up from segfaulting. A more complete solution would be to decouple\n> > git\n> > reflog from the log walker machinery.\n> > \n> > Signed-off-by: Dennis Kaarsemaker \n> > ---\n> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> > @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs\n> > at BUFSIZ boundaries' '\n> > +test_expect_success 'no segfaults for reflog containing non-commit\n> > sha1s' '\n> \n> Nit: It's kind of strange for a test title to talk about not\n> segfaulting; that's behavior you'd expect to be true for all tests.\n> Perhaps describe it as \"non-commit reflog entries handled sanely\" or\n> something.\n\nTo paraphrase what Junio said earlier in this thread: tests determine\nwhat is sane behavior, so using the word 'sanely' isn't really\nappropriate. This is a regression test to make sure we don't\naccidentally reintroduce behavior that segfaults, which I think is an\neasy mistake to make with the current code, so I think the title is\nappropriate.\n\n> > + git update-ref --create-reflog -m \"Creating ref\" \\\n> > + refs/tests/tree-in-reflog HEAD &&\n> > + git update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog\n> > HEAD^{tree} &&\n> > + git update-ref -m \"Restoring to commit\" refs/tests/tree-in\n> > -reflog HEAD &&\n> > + git reflog refs/tests/tree-in-reflog\n> > +'\n> \n> Hmm, this test is successful for me on OS X even without the\n> reflog-walk.c changes applied.\n> \n> > +test_expect_failure 'reflog with non-commit entries displays all\n> > entries' '\n> > + git reflog refs/tests/tree-in-reflog >actual &&\n> > + test_line_count = 3 actual\n> > +'\n> \n> And this test actually fails (inversely) because it's expecting a\n> failure, but doesn't get one since the command produces the expected\n> output.\n\nThat's... surprising to say the least. What's the content of 'actual',\nand which git.git commit are you on?\n\n> By the way, it may make sense to combine these two tests. If a\n> segfault occurs, the actual output likely will not match the expected\n> output, thus the test will fail anyhow (unless the segfault occurs\n> after all output).\n\nI kept them separate to show that while this no longer segfaults, it's\nstill not the correct output, but showing correct output is a much\nbigger project.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"275411","messageId":"CAPig+cThSHKBiUk5CmtE59mci3JLMv7QPNfHZAYmkqQvZHHofA@mail.gmail.com","threadId":"41094","inReplyTo":"1452043212.5562.18.camel@kaarsemaker.net","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-06T01:28:25Z","receivedAt":"2016-01-06T01:28:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker\n wrote:\n> On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:\n>> On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker\n>> > + git update-ref --create-reflog -m \"Creating ref\" \\\n>> > + refs/tests/tree-in-reflog HEAD &&\n>> > + git update-ref -m \"Forcing tree\" refs/tests/tree-in-reflog\n>> > HEAD^{tree} &&\n>> > + git update-ref -m \"Restoring to commit\" refs/tests/tree-in\n>> > -reflog HEAD &&\n>> > + git reflog refs/tests/tree-in-reflog\n>> > +'\n>>\n>> Hmm, this test is successful for me on OS X even without the\n>> reflog-walk.c changes applied.\n>>\n>> > +test_expect_failure 'reflog with non-commit entries displays all\n>> > entries' '\n>> > + git reflog refs/tests/tree-in-reflog >actual &&\n>> > + test_line_count = 3 actual\n>> > +'\n>>\n>> And this test actually fails (inversely) because it's expecting a\n>> failure, but doesn't get one since the command produces the expected\n>> output.\n>\n> That's... surprising to say the least. What's the content of 'actual',\n> and which git.git commit are you on?\n\n% cat t/trash\\ directory.t1410-reflog/actual\nb60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit\n140c527 refs/tests/tree-in-reflog@{1}: Forcing tree\nb60a214 refs/tests/tree-in-reflog@{2}: Creating ref\n%\n\nThis is with only the t/t1410-reflog.sh changes from your patch\napplied atop current 'master' (SHA1 7548842).\n"},{"id":"275412","messageId":"CAPig+cQ7MineqezZXxpfAotVwoM9Ju1qwVGpEnEh9qNKBF1Pjg@mail.gmail.com","threadId":"41094","inReplyTo":"CAPig+cThSHKBiUk5CmtE59mci3JLMv7QPNfHZAYmkqQvZHHofA@mail.gmail.com","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-06T01:52:57Z","receivedAt":"2016-01-06T01:52:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker\n> wrote:\n>> On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:\n>>> Hmm, this test is successful for me on OS X even without the\n>>> reflog-walk.c changes applied.\n>>>\n>>> And this test actually fails (inversely) because it's expecting a\n>>> failure, but doesn't get one since the command produces the expected\n>>> output.\n>>\n>> That's... surprising to say the least. What's the content of 'actual',\n>> and which git.git commit are you on?\n>\n> % cat t/trash\\ directory.t1410-reflog/actual\n> b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit\n> 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree\n> b60a214 refs/tests/tree-in-reflog@{2}: Creating ref\n> %\n>\n> This is with only the t/t1410-reflog.sh changes from your patch\n> applied atop current 'master' (SHA1 7548842).\n\nBy the way, the segfault does occur for me on Linux and FreeBSD.\n\nAnd, in all cases, on all tested platforms, with the full patch\napplied, both tests behave sanely (in the expected fashion). So, even\nthough the crash doesn't manifest everywhere, the fact that the tests\nare meaningfully testing it on the \"affected\" platforms may mean that\nit's not worth worrying about why it doesn't segfault on OS X.\n\n(Of course, practicality aside, one might want to satisfy one's\nintellectual curiosity about why it behaves differently on OS X.)\n"},{"id":"275421","messageId":"1452071624.2668.21.camel@kaarsemaker.net","threadId":"41094","inReplyTo":"CAPig+cQ7MineqezZXxpfAotVwoM9Ju1qwVGpEnEh9qNKBF1Pjg@mail.gmail.com","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-01-06T09:13:44Z","receivedAt":"2016-01-06T09:13:44Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On di, 2016-01-05 at 20:52 -0500, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine <\n> sunshine@sunshineco.com> wrote:\n> > On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker\n> > wrote:\n> > > On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:\n> > > > Hmm, this test is successful for me on OS X even without the\n> > > > reflog-walk.c changes applied.\n> > > > \n> > > > And this test actually fails (inversely) because it's expecting\n> > > > a\n> > > > failure, but doesn't get one since the command produces the\n> > > > expected\n> > > > output.\n> > > \n> > > That's... surprising to say the least. What's the content of\n> > > 'actual',\n> > > and which git.git commit are you on?\n> > \n> > % cat t/trash\\ directory.t1410-reflog/actual\n> > b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit\n> > 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree\n> > b60a214 refs/tests/tree-in-reflog@{2}: Creating ref\n> > %\n> > \n> > This is with only the t/t1410-reflog.sh changes from your patch\n> > applied atop current 'master' (SHA1 7548842).\n>\n> By the way, the segfault does occur for me on Linux and FreeBSD.\n> \n> And, in all cases, on all tested platforms, with the full patch\n> applied, both tests behave sanely (in the expected fashion). So, even\n> though the crash doesn't manifest everywhere, the fact that the tests\n> are meaningfully testing it on the \"affected\" platforms may mean that\n> it's not worth worrying about why it doesn't segfault on OS X.\n> \n> (Of course, practicality aside, one might want to satisfy one's\n> intellectual curiosity about why it behaves differently on OS X.)\n\nThe only explanation I can think of (and that's with practically no\nknowledge of OS X internals) is that OS X's memory allocation strategy\nis unlucky. Git is definitely writing to a location it should not write\nto. On linux and freebsd this is unallocated memory, so you get a\nsegfault. On OS X, it happens to be memory actually allocated by git,\nresulting not in a segfault but in silent corruption of other in-memory\ndata. I would argue that this is a much worse result, even though in\nthis small test that corruption seems to not trigger a crash.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"275423","messageId":"CACsJy8DLWex0fCaLxCe+ZGeoVcStWcdhBwmeU8fMxonO4VfKcg@mail.gmail.com","threadId":"41094","inReplyTo":"1452071624.2668.21.camel@kaarsemaker.net","subject":"Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-01-06T09:30:56Z","receivedAt":"2016-01-06T09:30:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 6, 2016 at 4:13 PM, Dennis Kaarsemaker\n wrote:\n> On di, 2016-01-05 at 20:52 -0500, Eric Sunshine wrote:\n>> On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine <\n>> sunshine@sunshineco.com> wrote:\n>> > On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker\n>> > wrote:\n>> > > On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:\n>> > > > Hmm, this test is successful for me on OS X even without the\n>> > > > reflog-walk.c changes applied.\n>> > > >\n>> > > > And this test actually fails (inversely) because it's expecting\n>> > > > a\n>> > > > failure, but doesn't get one since the command produces the\n>> > > > expected\n>> > > > output.\n>> > >\n>> > > That's... surprising to say the least. What's the content of\n>> > > 'actual',\n>> > > and which git.git commit are you on?\n>> >\n>> > % cat t/trash\\ directory.t1410-reflog/actual\n>> > b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit\n>> > 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree\n>> > b60a214 refs/tests/tree-in-reflog@{2}: Creating ref\n>> > %\n>> >\n>> > This is with only the t/t1410-reflog.sh changes from your patch\n>> > applied atop current 'master' (SHA1 7548842).\n>>\n>> By the way, the segfault does occur for me on Linux and FreeBSD.\n>>\n>> And, in all cases, on all tested platforms, with the full patch\n>> applied, both tests behave sanely (in the expected fashion). So, even\n>> though the crash doesn't manifest everywhere, the fact that the tests\n>> are meaningfully testing it on the \"affected\" platforms may mean that\n>> it's not worth worrying about why it doesn't segfault on OS X.\n>>\n>> (Of course, practicality aside, one might want to satisfy one's\n>> intellectual curiosity about why it behaves differently on OS X.)\n>\n> The only explanation I can think of (and that's with practically no\n> knowledge of OS X internals) is that OS X's memory allocation strategy\n> is unlucky. Git is definitely writing to a location it should not write\n> to. On linux and freebsd this is unallocated memory, so you get a\n> segfault. On OS X, it happens to be memory actually allocated by git,\n> resulting not in a segfault but in silent corruption of other in-memory\n> data. I would argue that this is a much worse result, even though in\n> this small test that corruption seems to not trigger a crash.\n\nAgreed. For a guaranteed crash, put assert(c->object.type ==\nOBJ_COMMIT); in the macro function \"slabname## _at_peek\" in\ncommit-slab.h. That is if I analyzed the crash correctly. I'm not\nsuggesting to put the assert() in the code permanently though because\nI don't know how often this function is called.\n-- \nDuy\n"}]}