{"thread":{"id":"14518","subject":"git-remote SEGV on t5505 test.","startedAt":"2008-07-18T01:46:02Z","lastAt":"2008-07-18T09:13:37Z","messageCount":7,"participants":["SungHyun Nam","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"83814","messageId":"g5osl6$4g3$1@ger.gmane.org","threadId":"14518","inReplyTo":null,"subject":"git-remote SEGV on t5505 test.","fromName":"SungHyun Nam","fromEmail":"namsh@posdata.co.kr","sentAt":"2008-07-18T01:46:02Z","receivedAt":"2008-07-18T01:46:02Z","isPatch":false,"sender":{"key":"namsh@posdata.co.kr","avatar":null},"body":"Hello,\n\nThe gdb backtrace said:\n(gdb) bt\n#0  0xff03455c in strlen () from /usr/lib/libc.so.1\n#1  0xff087058 in _doprnt () from /usr/lib/libc.so.1\n#2  0xff088c18 in printf () from /usr/lib/libc.so.1\n#3  0x0007df20 in prune (argc=1556136, argv=0x139cd0) at\nbuiltin-remote.c:590\n\nThe codes in builtin-remote.c:590,\n         printf(\" * [%s] %s\\n\", dry_run ? \"would prune\" : \"prune\",\n                skip_prefix(refname, \"refs/remotes/\"));\n\nAnd the 'skip_prefix()' returns NULL in this case.\n(The old skip_prefix() never returns NULL).\n\nIt seems there is another place need to be checked:\n   builtin-remote.c:464 : can pass NULL to the strdup().\n\nRegards,\nnamsh\n"},{"id":"83817","messageId":"7vsku7es3n.fsf@gitster.siamese.dyndns.org","threadId":"14518","inReplyTo":"g5osl6$4g3$1@ger.gmane.org","subject":"Re: git-remote SEGV on t5505 test.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T04:25:32Z","receivedAt":"2008-07-18T04:25:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SungHyun Nam <namsh@posdata.co.kr> writes:\n\n> And the 'skip_prefix()' returns NULL in this case.\n> (The old skip_prefix() never returns NULL).\n\nThanks.  Something like this?\n\n builtin-remote.c |   51 +++++++++++++++++++--------------------------------\n 1 files changed, 19 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 1491354..db12668 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -147,6 +147,15 @@ struct branch_info {\n \n static struct path_list branch_list;\n \n+static const char *abbrev_ref(const char *name, const char *prefix)\n+{\n+\tconst char *abbrev = skip_prefix(name, prefix);\n+\tif (abbrev)\n+\t\treturn abbrev;\n+\treturn name;\n+}\n+#define abbrev_branch(name) abbrev_ref((name), \"refs/heads/\")\n+\n static int config_read_branches(const char *key, const char *value, void *cb)\n {\n \tif (!prefixcmp(key, \"branch.\")) {\n@@ -176,18 +185,12 @@ static int config_read_branches(const char *key, const char *value, void *cb)\n \t\t\tinfo->remote = xstrdup(value);\n \t\t} else {\n \t\t\tchar *space = strchr(value, ' ');\n-\t\t\tconst char *ptr = skip_prefix(value, \"refs/heads/\");\n-\t\t\tif (ptr)\n-\t\t\t\tvalue = ptr;\n+\t\t\tvalue = abbrev_branch(value);\n \t\t\twhile (space) {\n \t\t\t\tchar *merge;\n \t\t\t\tmerge = xstrndup(value, space - value);\n \t\t\t\tpath_list_append(merge, &info->merge);\n-\t\t\t\tptr = skip_prefix(space + 1, \"refs/heads/\");\n-\t\t\t\tif (ptr)\n-\t\t\t\t\tvalue = ptr;\n-\t\t\t\telse\n-\t\t\t\t\tvalue = space + 1;\n+\t\t\t\tvalue = abbrev_branch(space + 1);\n \t\t\t\tspace = strchr(value, ' ');\n \t\t\t}\n \t\t\tpath_list_append(xstrdup(value), &info->merge);\n@@ -219,12 +222,7 @@ static int handle_one_branch(const char *refname,\n \trefspec.dst = (char *)refname;\n \tif (!remote_find_tracking(states->remote, &refspec)) {\n \t\tstruct path_list_item *item;\n-\t\tconst char *name, *ptr;\n-\t\tptr = skip_prefix(refspec.src, \"refs/heads/\");\n-\t\tif (ptr)\n-\t\t\tname = ptr;\n-\t\telse\n-\t\t\tname = refspec.src;\n+\t\tconst char *name = abbrev_branch(refspec.src);\n \t\t/* symbolic refs pointing nowhere were handled already */\n \t\tif ((flags & REF_ISSYMREF) ||\n \t\t\t\tunsorted_path_list_has_path(&states->tracked,\n@@ -253,7 +251,6 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)\n \t\tstruct path_list *target = &states->tracked;\n \t\tunsigned char sha1[20];\n \t\tvoid *util = NULL;\n-\t\tconst char *ptr;\n \n \t\tif (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))\n \t\t\ttarget = &states->new;\n@@ -262,10 +259,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)\n \t\t\tif (hashcmp(sha1, ref->new_sha1))\n \t\t\t\tutil = &states;\n \t\t}\n-\t\tptr = skip_prefix(ref->name, \"refs/heads/\");\n-\t\tif (!ptr)\n-\t\t\tptr = ref->name;\n-\t\tpath_list_append(ptr, target)->util = util;\n+\t\tpath_list_append(abbrev_branch(ref->name), target)->util = util;\n \t}\n \tfree_refs(fetch_map);\n \n@@ -460,10 +454,8 @@ static int append_ref_to_tracked_list(const char *refname,\n \n \tmemset(&refspec, 0, sizeof(refspec));\n \trefspec.dst = (char *)refname;\n-\tif (!remote_find_tracking(states->remote, &refspec)) {\n-\t\tpath_list_append(skip_prefix(refspec.src, \"refs/heads/\"),\n-\t\t\t&states->tracked);\n-\t}\n+\tif (!remote_find_tracking(states->remote, &refspec))\n+\t\tpath_list_append(abbrev_branch(refspec.src), &states->tracked);\n \n \treturn 0;\n }\n@@ -530,15 +522,10 @@ static int show(int argc, const char **argv)\n \t\t\t\t\t\"es\" : \"\");\n \t\t\tfor (i = 0; i < states.remote->push_refspec_nr; i++) {\n \t\t\t\tstruct refspec *spec = states.remote->push + i;\n-\t\t\t\tconst char *p = \"\", *q = \"\";\n-\t\t\t\tif (spec->src)\n-\t\t\t\t\tp = skip_prefix(spec->src, \"refs/heads/\");\n-\t\t\t\tif (spec->dst)\n-\t\t\t\t\tq = skip_prefix(spec->dst, \"refs/heads/\");\n \t\t\t\tprintf(\" %s%s%s%s\", spec->force ? \"+\" : \"\",\n-\t\t\t\t\tp ? p : spec->src,\n-\t\t\t\t\tspec->dst ? \":\" : \"\",\n-\t\t\t\t\tq ? q : spec->dst);\n+\t\t\t\t       abbrev_branch(spec->src),\n+\t\t\t\t       spec->dst ? \":\" : \"\",\n+\t\t\t\t       spec->dst ? abbrev_branch(spec->dst) : \"\");\n \t\t\t}\n \t\t\tprintf(\"\\n\");\n \t\t}\n@@ -588,7 +575,7 @@ static int prune(int argc, const char **argv)\n \t\t\t\tresult |= delete_ref(refname, NULL);\n \n \t\t\tprintf(\" * [%s] %s\\n\", dry_run ? \"would prune\" : \"pruned\",\n-\t\t\t       skip_prefix(refname, \"refs/remotes/\"));\n+\t\t\t       abbrev_ref(refname, \"refs/remotes/\"));\n \t\t}\n \n \t\t/* NEEDSWORK: free remote */\n"},{"id":"83823","messageId":"48802DCD.2090704@posdata.co.kr","threadId":"14518","inReplyTo":"7vsku7es3n.fsf@gitster.siamese.dyndns.org","subject":"Re: git-remote SEGV on t5505 test.","fromName":"SungHyun Nam","fromEmail":"namsh@posdata.co.kr","sentAt":"2008-07-18T05:44:45Z","receivedAt":"2008-07-18T05:44:45Z","isPatch":false,"sender":{"key":"namsh@posdata.co.kr","avatar":null},"body":"Junio C Hamano wrote:\n> SungHyun Nam <namsh@posdata.co.kr> writes:\n> \n>> And the 'skip_prefix()' returns NULL in this case.\n>> (The old skip_prefix() never returns NULL).\n> \n> Thanks.  Something like this?\n\nWith this patch, 'make test' passed all the tests successfully.\nI ran test with GIT_SKIP_TESTS=\"t90?? t91?? t92?? t94??\".\n\nRegards,\nnamsh\n\nP.S I skipped svn/cvs test because I don't need them (And no\n     svn/cvs installed).  But, I skipped send-email test because it\n     fails.  I thought I don't need to use send-email.\n\n     And now I check what caused fail.  The problem was this test\n     script generates 'fake.sendmail' which uses '/bin/sh'.\n\n     Is it possible that we can use 'SHELL_PATH' here?  Or could\n     you apply the patch below?\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex de5b980..0320aa1 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -16,7 +16,7 @@ test_expect_success \\\n      '(echo \"#!/bin/sh\"\n        echo shift\n        echo output=1\n-      echo \"while test -f commandline\\$output; do\noutput=\\$((\\$output+1)); done\"\n+      echo \"while test -f commandline\\$output; do output=\\`expr\n\\$output + 1\\`; done\"\n        echo for a\n        echo do\n        echo \"  echo \\\"!\\$a!\\\"\"\n"},{"id":"83831","messageId":"7vsku7d8ak.fsf_-_@gitster.siamese.dyndns.org","threadId":"14518","inReplyTo":"48802DCD.2090704@posdata.co.kr","subject":"Re* git-remote SEGV on t5505 test.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T06:18:43Z","receivedAt":"2008-07-18T06:18:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SungHyun Nam <namsh@posdata.co.kr> writes:\n\n>     Is it possible that we can use 'SHELL_PATH' here?\n\nIt is not just possible but we really should.  There are other test\nscripts that use hardcoded /bin/sh, but by setting SHELL_PATH the user is\nalready telling us that what the vendor has in /bin/sh isn't adequately\nPOSIX enough, and we really should try to honor that.\n\n\"git grep -n /bin/sh t/t*sh | grep -v ':1:#!'\" would tell you which ones\nare suspect.\n\n---\n\n t/t9001-send-email.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex de5b980..1c857cf 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -13,7 +13,7 @@ test_expect_success \\\n \n test_expect_success \\\n     'Setup helper tool' \\\n-    '(echo \"#!/bin/sh\"\n+    '(echo \"#!$SHELL_PATH\"\n       echo shift\n       echo output=1\n       echo \"while test -f commandline\\$output; do output=\\$((\\$output+1)); done\"\n@@ -138,7 +138,7 @@ test_expect_success 'Valid In-Reply-To when prompting' '\n '\n \n test_expect_success 'setup fake editor' '\n-\t(echo \"#!/bin/sh\" &&\n+\t(echo \"#!$SHELL_PATH\" &&\n \t echo \"echo fake edit >>\\\"\\$1\\\"\"\n \t) >fake-editor &&\n \tchmod +x fake-editor\n@@ -235,7 +235,7 @@ test_expect_success 'sendemail.cc unset' '\n \n test_expect_success '--compose adds MIME for utf8 body' '\n \tclean_fake_sendmail &&\n-\t(echo \"#!/bin/sh\" &&\n+\t(echo \"#!$SHELL_PATH\" &&\n \t echo \"echo utf8 body: àéìöú >>\\\"\\$1\\\"\"\n \t) >fake-editor-utf8 &&\n \tchmod +x fake-editor-utf8 &&\n@@ -254,7 +254,7 @@ test_expect_success '--compose adds MIME for utf8 body' '\n \n test_expect_success '--compose respects user mime type' '\n \tclean_fake_sendmail &&\n-\t(echo \"#!/bin/sh\" &&\n+\t(echo \"#!$SHELL_PATH\" &&\n \t echo \"(echo MIME-Version: 1.0\"\n \t echo \" echo Content-Type: text/plain\\\\; charset=iso-8859-1\"\n \t echo \" echo Content-Transfer-Encoding: 8bit\"\n"},{"id":"83833","messageId":"7vk5fjd7x5.fsf@gitster.siamese.dyndns.org","threadId":"14518","inReplyTo":"7vsku7d8ak.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Re* git-remote SEGV on t5505 test.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T06:26:46Z","receivedAt":"2008-07-18T06:26:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> SungHyun Nam <namsh@posdata.co.kr> writes:\n>\n>>     Is it possible that we can use 'SHELL_PATH' here?\n>\n> It is not just possible but we really should.  There are other test\n> scripts that use hardcoded /bin/sh, but by setting SHELL_PATH the user is\n> already telling us that what the vendor has in /bin/sh isn't adequately\n> POSIX enough, and we really should try to honor that.\n>\n> \"git grep -n /bin/sh t/t*sh | grep -v ':1:#!'\" would tell you which ones\n> are suspect.\n\nSungHyun, I did not test this patch myself (all my shells grok $() command\nsubstitutions), so I won't be committing this until/unless I see a \"tested\non system X and works fine\".\n"},{"id":"83849","messageId":"48805D65.1060704@posdata.co.kr","threadId":"14518","inReplyTo":"7vk5fjd7x5.fsf@gitster.siamese.dyndns.org","subject":"Re: Re* git-remote SEGV on t5505 test.","fromName":"SungHyun Nam","fromEmail":"namsh@posdata.co.kr","sentAt":"2008-07-18T09:07:49Z","receivedAt":"2008-07-18T09:07:49Z","isPatch":false,"sender":{"key":"namsh@posdata.co.kr","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> SungHyun Nam <namsh@posdata.co.kr> writes:\n>>\n>>>     Is it possible that we can use 'SHELL_PATH' here?\n>> It is not just possible but we really should.  There are other test\n>> scripts that use hardcoded /bin/sh, but by setting SHELL_PATH the user is\n>> already telling us that what the vendor has in /bin/sh isn't adequately\n>> POSIX enough, and we really should try to honor that.\n>>\n>> \"git grep -n /bin/sh t/t*sh | grep -v ':1:#!'\" would tell you which ones\n>> are suspect.\n> \n> SungHyun, I did not test this patch myself (all my shells grok $() command\n> substitutions), so I won't be committing this until/unless I see a \"tested\n> on system X and works fine\".\n\nI tested it on Solaris and works fine. ^^\n\n$ uname -sro\nSunOS 5.9 Solaris\n\n$ SHELL_PATH=/bin/bash bash ./t9001-send-email.sh\n*   ok 1: prepare reference tree\n*   ok 2: Setup helper tool\n*   ok 3: Extract patches\n*   ok 4: Send patches\n*   ok 5: Verify commandline\n*   ok 6: Show all headers\n*   ok 7: reject long lines\n*   ok 8: no patch was sent\n*   ok 9: allow long lines with --no-validate\n*   ok 10: Invalid In-Reply-To\n*   ok 11: Valid In-Reply-To when prompting\n*   ok 12: setup fake editor\n*   ok 13: --compose works\n*   ok 14: first message is compose text\n*   ok 15: second message is patch\n*   ok 16: sendemail.cc set\n*   ok 17: sendemail.cc unset\n*   ok 18: --compose adds MIME for utf8 body\n*   ok 19: --compose respects user mime type\n*   ok 20: --compose adds MIME for utf8 subject\n* passed all 20 test(s)\n"},{"id":"83851","messageId":"7vk5fj8shq.fsf@gitster.siamese.dyndns.org","threadId":"14518","inReplyTo":"48805D65.1060704@posdata.co.kr","subject":"Re: Re* git-remote SEGV on t5505 test.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T09:13:37Z","receivedAt":"2008-07-18T09:13:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SungHyun Nam <namsh@posdata.co.kr> writes:\n\n>> SungHyun, I did not test this patch myself (all my shells grok $() command\n>> substitutions), so I won't be committing this until/unless I see a \"tested\n>> on system X and works fine\".\n>\n> I tested it on Solaris and works fine. ^^\n\nThanks.\n"}]}