{"thread":{"id":"17878","subject":"[PATCH] Avoid segfault with 'git branch' when the HEAD is detached","startedAt":"2009-02-18T18:14:59Z","lastAt":"2009-02-19T11:31:21Z","messageCount":12,"participants":["Johannes Schindelin","Jay Soffian","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"105329","messageId":"ad680bce413ddea084d84b3fcd7c4cc356c3cb0e.1234980819u.git.johannes.schindelin@gmx.de","threadId":"17878","inReplyTo":"cover.1234980819u.git.johannes.schindelin@gmx.de","subject":"[PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-18T18:14:59Z","receivedAt":"2009-02-18T18:14:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A recent addition to the ref_item struct was not taken care of, leading\nto a segmentation fault when accessing the (uninitialized) \"dest\" member.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tUnfortunately not found by valgrind.\n\n builtin-branch.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 8c37c78..ded0a82 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \t is_descendant_of(head_commit, with_commit)) {\n \t\tstruct ref_item item;\n \t\titem.name = xstrdup(\"(no branch)\");\n+\t\titem.len = strlen(item.name);\n \t\titem.kind = REF_LOCAL_BRANCH;\n+\t\titem.dest = NULL;\n \t\titem.commit = head_commit;\n \t\tif (strlen(item.name) > ref_list.maxwidth)\n \t\t\tref_list.maxwidth = strlen(item.name);\n-- \n1.6.2.rc1.349.g70b801\n"},{"id":"105337","messageId":"76718490902181236r1058ed6dr3ae881dfb820f068@mail.gmail.com","threadId":"17878","inReplyTo":"ad680bce413ddea084d84b3fcd7c4cc356c3cb0e.1234980819u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-18T20:36:49Z","receivedAt":"2009-02-18T20:36:49Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 18, 2009 at 1:14 PM, Johannes Schindelin\n wrote:\n> A recent addition to the ref_item struct was not taken care of, leading\n> to a segmentation fault when accessing the (uninitialized) \"dest\" member.\n>\n> Signed-off-by: Johannes Schindelin \n\nACK.\n\nThanks,\n\nj.\n"},{"id":"105362","messageId":"20090219004504.GB25808@coredump.intra.peff.net","threadId":"17878","inReplyTo":"ad680bce413ddea084d84b3fcd7c4cc356c3cb0e.1234980819u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T00:45:04Z","receivedAt":"2009-02-19T00:45:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:\n\n> A recent addition to the ref_item struct was not taken care of, leading\n> to a segmentation fault when accessing the (uninitialized) \"dest\" member.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> \n> \tUnfortunately not found by valgrind.\n\nMeaning that the bug was created after your valgrind testing (which\ntakes a painfully long time to run, and so only happens occasionally),\nand therefore you found it by hand? Or meaning that even running the\ntest suite with valgrind did not reveal the problem?\n\nIf the latter, isn't that an indication that this code path was not\nbeing exercised by the test suite and it should be?\n\nNow if only we had a way of measuring test coverage...\n\n> --- a/builtin-branch.c\n> +++ b/builtin-branch.c\n> @@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n> \t is_descendant_of(head_commit, with_commit)) {\n> \t\tstruct ref_item item;\n> \t\titem.name = xstrdup(\"(no branch)\");\n> +\t\titem.len = strlen(item.name);\n> \t\titem.kind = REF_LOCAL_BRANCH;\n> +\t\titem.dest = NULL;\n> \t\titem.commit = head_commit;\n> \t\tif (strlen(item.name) > ref_list.maxwidth)\n> \t\t\tref_list.maxwidth = strlen(item.name);\n\nMaybe replace the repeated strlens below with item.len? I.e., squash in\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 13e4de8..14d4b91 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \t\titem.kind = REF_LOCAL_BRANCH;\n \t\titem.dest = NULL;\n \t\titem.commit = head_commit;\n-\t\tif (strlen(item.name) > ref_list.maxwidth)\n-\t\t\tref_list.maxwidth = strlen(item.name);\n+\t\tif (item.len > ref_list.maxwidth)\n+\t\t\tref_list.maxwidth = item.len;\n \t\tprint_ref_item(&item, ref_list.maxwidth, verbose, abbrev, 1, \"\");\n \t\tfree(item.name);\n \t}\n\nOther than that, patch looks obviously correct (and I did a quick scan\nto see that there were no other locations).\n\n-Peff\n"},{"id":"105369","messageId":"alpine.DEB.1.00.0902190212290.10279@pacific.mpi-cbg.de","threadId":"17878","inReplyTo":"20090219004504.GB25808@coredump.intra.peff.net","subject":"Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-19T01:15:00Z","receivedAt":"2009-02-19T01:15:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 18 Feb 2009, Jeff King wrote:\n\n> On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:\n> \n> > A recent addition to the ref_item struct was not taken care of, leading\n> > to a segmentation fault when accessing the (uninitialized) \"dest\" member.\n> > \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > \n> > \tUnfortunately not found by valgrind.\n> \n> Meaning that the bug was created after your valgrind testing (which \n> takes a painfully long time to run, and so only happens occasionally), \n> and therefore you found it by hand? Or meaning that even running the \n> test suite with valgrind did not reveal the problem?\n\nIt bit me.\n\nIOW I had to fix it before I could finish up the work for the day.\n\n> If the latter, isn't that an indication that this code path was not \n> being exercised by the test suite and it should be?\n\nLike I said, I had to finish up some work for the day, that's why I did \nnot have time to add a test.\n\n> Now if only we had a way of measuring test coverage...\n\nYes, I also want the gcov series. Patience, grass hopper, patience: after \n1.6.2.\n\n> > --- a/builtin-branch.c\n> > +++ b/builtin-branch.c\n> > @@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n> > \t is_descendant_of(head_commit, with_commit)) {\n> > \t\tstruct ref_item item;\n> > \t\titem.name = xstrdup(\"(no branch)\");\n> > +\t\titem.len = strlen(item.name);\n> > \t\titem.kind = REF_LOCAL_BRANCH;\n> > +\t\titem.dest = NULL;\n> > \t\titem.commit = head_commit;\n> > \t\tif (strlen(item.name) > ref_list.maxwidth)\n> > \t\t\tref_list.maxwidth = strlen(item.name);\n> \n> Maybe replace the repeated strlens below with item.len? I.e., squash in\n> \n> diff --git a/builtin-branch.c b/builtin-branch.c\n> index 13e4de8..14d4b91 100644\n> --- a/builtin-branch.c\n> +++ b/builtin-branch.c\n> @@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n> \t\titem.kind = REF_LOCAL_BRANCH;\n> \t\titem.dest = NULL;\n> \t\titem.commit = head_commit;\n> -\t\tif (strlen(item.name) > ref_list.maxwidth)\n> -\t\t\tref_list.maxwidth = strlen(item.name);\n> +\t\tif (item.len > ref_list.maxwidth)\n> +\t\t\tref_list.maxwidth = item.len;\n\nYeah, I did not think of that. I checked that there are no other \ninstances where a member of ref_item was uninitialized, and that took \nalready more time than I had.\n\nCiao,\nDscho\n"},{"id":"105377","messageId":"20090219032420.GA26488@coredump.intra.peff.net","threadId":"17878","inReplyTo":"alpine.DEB.1.00.0902190212290.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:24:20Z","receivedAt":"2009-02-19T03:24:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 19, 2009 at 02:15:00AM +0100, Johannes Schindelin wrote:\n\n> > Meaning that the bug was created after your valgrind testing (which \n> > takes a painfully long time to run, and so only happens occasionally), \n> > and therefore you found it by hand? Or meaning that even running the \n> > test suite with valgrind did not reveal the problem?\n> \n> It bit me.\n> \n> IOW I had to fix it before I could finish up the work for the day.\n\nOK. I wasn't sure if it was \"valgrind didn't find\" or \"valgrind wouldn't\nfind\". And your answer is \"didn't\", but as it turns out, it also\n\"wouldn't.\n\nUpdated series with tests to follow.\n\n-Peff\n"},{"id":"105378","messageId":"20090219033329.GA13666@coredump.intra.peff.net","threadId":"17878","inReplyTo":"20090219032420.GA26488@coredump.intra.peff.net","subject":"Re: [PATCH] Avoid segfault with 'git branch' when the HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:33:29Z","receivedAt":"2009-02-19T03:33:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 18, 2009 at 10:24:20PM -0500, Jeff King wrote:\n\n> Updated series with tests to follow.\n\nAh, it looks like Junio picked up your original patch. But I still think\nit is worth doing these on top:\n\n 1/2: add basic branch display tests\n 2/2: branch: clean up repeated strlen\n\n-Peff\n"},{"id":"105379","messageId":"20090219033444.GA18970@coredump.intra.peff.net","threadId":"17878","inReplyTo":"20090219033329.GA13666@coredump.intra.peff.net","subject":"[PATCH 1/2] add basic branch display tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:34:44Z","receivedAt":"2009-02-19T03:34:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We were not testing the output of \"git branch\" anywhere.\nNot only does this not protect us against regressions in the\noutput, but we are not exercising code paths which may have\nbugs (such as the one fixed by 45e2b61).\n\nSigned-off-by: Jeff King \n---\n t/t3203-branch-output.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 81 insertions(+), 0 deletions(-)\n create mode 100755 t/t3203-branch-output.sh\n\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nnew file mode 100755\nindex 0000000..809d1c4\n--- /dev/null\n+++ b/t/t3203-branch-output.sh\n@@ -0,0 +1,81 @@\n+#!/bin/sh\n+\n+test_description='git branch display tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'make commits' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\techo content >>file &&\n+\tgit commit -a -m two\n+'\n+\n+test_expect_success 'make branches' '\n+\tgit branch branch-one\n+\tgit branch branch-two HEAD^\n+'\n+\n+test_expect_success 'make remote branches' '\n+\tgit update-ref refs/remotes/origin/branch-one branch-one\n+\tgit update-ref refs/remotes/origin/branch-two branch-two\n+\tgit symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/branch-one\n+'\n+\n+cat >expect <<'EOF'\n+ branch-one\n+ branch-two\n+* master\n+EOF\n+test_expect_success 'git branch shows local branches' '\n+\tgit branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+ origin/HEAD -> origin/branch-one\n+ origin/branch-one\n+ origin/branch-two\n+EOF\n+test_expect_success 'git branch -r shows remote branches' '\n+\tgit branch -r >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+ branch-one\n+ branch-two\n+* master\n+ remotes/origin/HEAD -> origin/branch-one\n+ remotes/origin/branch-one\n+ remotes/origin/branch-two\n+EOF\n+test_expect_success 'git branch -a shows local and remote branches' '\n+\tgit branch -a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+two\n+one\n+two\n+EOF\n+test_expect_success 'git branch -v shows branch summaries' '\n+\tgit branch -v >tmp &&\n+\tawk \"{print \\$NF}\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<'EOF'\n+* (no branch)\n+ branch-one\n+ branch-two\n+ master\n+EOF\n+test_expect_success 'git branch shows detached HEAD properly' '\n+\tgit checkout HEAD^0 &&\n+\tgit branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.6.2.rc1.210.g1210c\n"},{"id":"105380","messageId":"20090219033545.GB18970@coredump.intra.peff.net","threadId":"17878","inReplyTo":"20090219033329.GA13666@coredump.intra.peff.net","subject":"[PATCH 2/2] branch: clean up repeated strlen","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:35:45Z","receivedAt":"2009-02-19T03:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 45e2b61 fixed the initialization of a \"len\" struct\nparameter via strlen. We can use that to clean up what is\nnow 3 strlens in a 6-line sequence.\n\nSigned-off-by: Jeff King \n---\nI guess a good compiler could optimize these out, but I think it\nactually reads a little bit nicer.\n\n builtin-branch.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 13e4de8..14d4b91 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \t\titem.kind = REF_LOCAL_BRANCH;\n \t\titem.dest = NULL;\n \t\titem.commit = head_commit;\n-\t\tif (strlen(item.name) > ref_list.maxwidth)\n-\t\t\tref_list.maxwidth = strlen(item.name);\n+\t\tif (item.len > ref_list.maxwidth)\n+\t\t\tref_list.maxwidth = item.len;\n \t\tprint_ref_item(&item, ref_list.maxwidth, verbose, abbrev, 1, \"\");\n \t\tfree(item.name);\n \t}\n-- \n1.6.2.rc1.210.g1210c\n"},{"id":"105381","messageId":"20090219034504.GA19862@coredump.intra.peff.net","threadId":"17878","inReplyTo":"20090219033444.GA18970@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] add basic branch display tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:45:04Z","receivedAt":"2009-02-19T03:45:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 18, 2009 at 10:34:44PM -0500, Jeff King wrote:\n\n> We were not testing the output of \"git branch\" anywhere.\n\nThere is one thing that occurred to me while writing these tests that I\nwanted to mention.\n\nWhen we show a remote symref with \"git branch -r\", it looks like this:\n\n> +cat >expect <<'EOF'\n> + origin/HEAD -> origin/branch-one\n> + origin/branch-one\n> + origin/branch-two\n> +EOF\n> +test_expect_success 'git branch -r shows remote branches' '\n> +\tgit branch -r >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nwhich makes sense. / -> /\n\n> +cat >expect <<'EOF'\n> + branch-one\n> + branch-two\n> +* master\n> + remotes/origin/HEAD -> origin/branch-one\n> + remotes/origin/branch-one\n> + remotes/origin/branch-two\n> +EOF\n> +test_expect_success 'git branch -a shows local and remote branches' '\n> +\tgit branch -a >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nBut here we stick the \"remotes/\" head on, since we are showing both\ntypes. But the right hand side of the symref doesn't get the same\ntreatment.\n\nI don't think it's a big deal, but I wasn't sure if it was intentional,\na bug, or simply that nobody cares (and since I have now codified it in\na test script, it seems like we should make sure it is intentional).\n\nI also had a brief thought that reprinting the is pointless.\nThat is, printing\n\n origin/HEAD -> master\n\nshows what is happening with less text due to the context (i.e., we\nalready know we are talking about remote \"origin\" -- and if it isn't in\norigin, we already show more). But that is probably a bad idea; that\ncontext is missing if you were to try to do something like \"git show\";\n/ would work, but wouldn't.\n\n-Peff\n"},{"id":"105384","messageId":"76718490902181951p54dc222ai5ac6397f3471fc44@mail.gmail.com","threadId":"17878","inReplyTo":"20090219034504.GA19862@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] add basic branch display tests","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-19T03:51:27Z","receivedAt":"2009-02-19T03:51:27Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 18, 2009 at 10:45 PM, Jeff King wrote:\n> But here we stick the \"remotes/\" head on, since we are showing both\n> types. But the right hand side of the symref doesn't get the same\n> treatment.\n>\n> I don't think it's a big deal, but I wasn't sure if it was intentional,\n\nIt was intentional on my part. I thought appending it a second time\nwas redundant, and the remotes/ prefix on the LHS is just their to\ndistinguish remote branches from local.\n\n> I also had a brief thought that reprinting the is pointless.\n> That is, printing\n>\n> origin/HEAD -> master\n>\n> shows what is happening with less text due to the context (i.e., we\n> already know we are talking about remote \"origin\" -- and if it isn't in\n> origin, we already show more). But that is probably a bad idea; that\n> context is missing if you were to try to do something like \"git show\";\n> / would work, but wouldn't.\n\nExactly. :-)\n\nThanks for the tests. If I had added them myself I wouldn't have you\npublicly questioning my intent. ;-)\n\nj.\n"},{"id":"105385","messageId":"20090219035332.GC19862@coredump.intra.peff.net","threadId":"17878","inReplyTo":"76718490902181951p54dc222ai5ac6397f3471fc44@mail.gmail.com","subject":"Re: [PATCH 1/2] add basic branch display tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-19T03:53:32Z","receivedAt":"2009-02-19T03:53:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 18, 2009 at 10:51:27PM -0500, Jay Soffian wrote:\n\n> On Wed, Feb 18, 2009 at 10:45 PM, Jeff King wrote:\n> > But here we stick the \"remotes/\" head on, since we are showing both\n> > types. But the right hand side of the symref doesn't get the same\n> > treatment.\n> >\n> > I don't think it's a big deal, but I wasn't sure if it was intentional,\n> \n> It was intentional on my part. I thought appending it a second time\n> was redundant, and the remotes/ prefix on the LHS is just their to\n> distinguish remote branches from local.\n\nOK. The more I think about it, the more I think what is currently there\nis best.\n\n> Thanks for the tests. If I had added them myself I wouldn't have you\n> publicly questioning my intent. ;-)\n\nYou're welcome. :)\n\n-Peff\n"},{"id":"105446","messageId":"alpine.DEB.1.00.0902191231050.6223@intel-tinevez-2-302","threadId":"17878","inReplyTo":"20090219033545.GB18970@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] branch: clean up repeated strlen","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-19T11:31:21Z","receivedAt":"2009-02-19T11:31:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 18 Feb 2009, Jeff King wrote:\n\n> Commit 45e2b61 fixed the initialization of a \"len\" struct\n> parameter via strlen. We can use that to clean up what is\n> now 3 strlens in a 6-line sequence.\n> \n> Signed-off-by: Jeff King \n> ---\n> I guess a good compiler could optimize these out, but I think it\n> actually reads a little bit nicer.\n\nDefinitely. And thanks for the tests!\n\nCiao,\nDscho\n"}]}