{"thread":{"id":"22594","subject":"[PATCH 7/6] t5401: Use a bare repository for the remote peer","startedAt":"2010-02-10T02:01:29Z","lastAt":"2010-02-11T19:04:22Z","messageCount":10,"participants":["Shawn O. Pearce","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"134104","messageId":"1265767290-25863-1-git-send-email-spearce@spearce.org","threadId":"22594","inReplyTo":null,"subject":"[PATCH 7/6] t5401: Use a bare repository for the remote peer","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T02:01:29Z","receivedAt":"2010-02-10T02:01:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We want to avoid the warnings (or later, test failures) about\nupdating the current branch.  It was never my intention to have\nthis test deal with a repository with a working directory, and it\nis a very old bug that the test even used a non-bare repository\nfor the remote side of the push operations.\n\nThis fixes the interleaved output error we were seeing as a test\nfailure by avoiding the giant warning message we were getting back\nabout updating the current branch being risky.\n\nIts not a real fix, but is something we should do no matter what,\nbecause the behavior will change in the future to reject, and the\ntest would break at that time.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n t/t5401-update-hooks.sh |   58 +++++++++++++++++++++++-----------------------\n 1 files changed, 29 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex c3cf397..7240fab 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -17,22 +17,22 @@ test_expect_success setup '\n \tcommit1=$(echo modify | git commit-tree $tree1 -p $commit0) &&\n \tgit update-ref refs/heads/master $commit0 &&\n \tgit update-ref refs/heads/tofail $commit1 &&\n-\tgit clone ./. victim &&\n-\tGIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&\n+\tgit clone --bare ./. victim.git &&\n+\tGIT_DIR=victim.git git update-ref refs/heads/tofail $commit1 &&\n \tgit update-ref refs/heads/master $commit1 &&\n \tgit update-ref refs/heads/tofail $commit0\n '\n \n-cat >victim/.git/hooks/pre-receive <<'EOF'\n+cat >victim.git/hooks/pre-receive <<'EOF'\n #!/bin/sh\n printf %s \"$@\" >>$GIT_DIR/pre-receive.args\n cat - >$GIT_DIR/pre-receive.stdin\n echo STDOUT pre-receive\n echo STDERR pre-receive >&2\n EOF\n-chmod u+x victim/.git/hooks/pre-receive\n+chmod u+x victim.git/hooks/pre-receive\n \n-cat >victim/.git/hooks/update <<'EOF'\n+cat >victim.git/hooks/update <<'EOF'\n #!/bin/sh\n echo \"$@\" >>$GIT_DIR/update.args\n read x; printf %s \"$x\" >$GIT_DIR/update.stdin\n@@ -40,77 +40,77 @@ echo STDOUT update $1\n echo STDERR update $1 >&2\n test \"$1\" = refs/heads/master || exit\n EOF\n-chmod u+x victim/.git/hooks/update\n+chmod u+x victim.git/hooks/update\n \n-cat >victim/.git/hooks/post-receive <<'EOF'\n+cat >victim.git/hooks/post-receive <<'EOF'\n #!/bin/sh\n printf %s \"$@\" >>$GIT_DIR/post-receive.args\n cat - >$GIT_DIR/post-receive.stdin\n echo STDOUT post-receive\n echo STDERR post-receive >&2\n EOF\n-chmod u+x victim/.git/hooks/post-receive\n+chmod u+x victim.git/hooks/post-receive\n \n-cat >victim/.git/hooks/post-update <<'EOF'\n+cat >victim.git/hooks/post-update <<'EOF'\n #!/bin/sh\n echo \"$@\" >>$GIT_DIR/post-update.args\n read x; printf %s \"$x\" >$GIT_DIR/post-update.stdin\n echo STDOUT post-update\n echo STDERR post-update >&2\n EOF\n-chmod u+x victim/.git/hooks/post-update\n+chmod u+x victim.git/hooks/post-update\n \n test_expect_success push '\n-\ttest_must_fail git send-pack --force ./victim/.git \\\n+\ttest_must_fail git send-pack --force ./victim.git \\\n \t\tmaster tofail >send.out 2>send.err\n '\n \n test_expect_success 'updated as expected' '\n-\ttest $(GIT_DIR=victim/.git git rev-parse master) = $commit1 &&\n-\ttest $(GIT_DIR=victim/.git git rev-parse tofail) = $commit1\n+\ttest $(GIT_DIR=victim.git git rev-parse master) = $commit1 &&\n+\ttest $(GIT_DIR=victim.git git rev-parse tofail) = $commit1\n '\n \n test_expect_success 'hooks ran' '\n-\ttest -f victim/.git/pre-receive.args &&\n-\ttest -f victim/.git/pre-receive.stdin &&\n-\ttest -f victim/.git/update.args &&\n-\ttest -f victim/.git/update.stdin &&\n-\ttest -f victim/.git/post-receive.args &&\n-\ttest -f victim/.git/post-receive.stdin &&\n-\ttest -f victim/.git/post-update.args &&\n-\ttest -f victim/.git/post-update.stdin\n+\ttest -f victim.git/pre-receive.args &&\n+\ttest -f victim.git/pre-receive.stdin &&\n+\ttest -f victim.git/update.args &&\n+\ttest -f victim.git/update.stdin &&\n+\ttest -f victim.git/post-receive.args &&\n+\ttest -f victim.git/post-receive.stdin &&\n+\ttest -f victim.git/post-update.args &&\n+\ttest -f victim.git/post-update.stdin\n '\n \n test_expect_success 'pre-receive hook input' '\n \t(echo $commit0 $commit1 refs/heads/master;\n \t echo $commit1 $commit0 refs/heads/tofail\n-\t) | test_cmp - victim/.git/pre-receive.stdin\n+\t) | test_cmp - victim.git/pre-receive.stdin\n '\n \n test_expect_success 'update hook arguments' '\n \t(echo refs/heads/master $commit0 $commit1;\n \t echo refs/heads/tofail $commit1 $commit0\n-\t) | test_cmp - victim/.git/update.args\n+\t) | test_cmp - victim.git/update.args\n '\n \n test_expect_success 'post-receive hook input' '\n \techo $commit0 $commit1 refs/heads/master |\n-\ttest_cmp - victim/.git/post-receive.stdin\n+\ttest_cmp - victim.git/post-receive.stdin\n '\n \n test_expect_success 'post-update hook arguments' '\n \techo refs/heads/master |\n-\ttest_cmp - victim/.git/post-update.args\n+\ttest_cmp - victim.git/post-update.args\n '\n \n test_expect_success 'all hook stdin is /dev/null' '\n-\t! test -s victim/.git/update.stdin &&\n-\t! test -s victim/.git/post-update.stdin\n+\t! test -s victim.git/update.stdin &&\n+\t! test -s victim.git/post-update.stdin\n '\n \n test_expect_success 'all *-receive hook args are empty' '\n-\t! test -s victim/.git/pre-receive.args &&\n-\t! test -s victim/.git/post-receive.args\n+\t! test -s victim.git/pre-receive.args &&\n+\t! test -s victim.git/post-receive.args\n '\n \n test_expect_success 'send-pack produced no output' '\n-- \n1.7.0.rc2.170.gbc565\n"},{"id":"134105","messageId":"1265767290-25863-2-git-send-email-spearce@spearce.org","threadId":"22594","inReplyTo":"1265767290-25863-1-git-send-email-spearce@spearce.org","subject":"[PATCH 8/6] receive-pack: Send internal errors over side-band #2","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T02:01:30Z","receivedAt":"2010-02-10T02:01:30Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the client has requested side-band-64k capability, send any\nof the internal error or warning messages in the muxed side-band\nstream using the same band as our hook output, band #2.  By putting\neverything in one stream we ensure all messages are processed by\nthe side-band demuxer, avoiding interleaving between our own stderr\nand the side-band demuxer's stderr buffers.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-receive-pack.c  |   68 +++++++++++++++++++++++++++++++++++-----------\n t/t5401-update-hooks.sh |    3 +-\n 2 files changed, 53 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex da1c26b..e98c2f1 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -139,6 +139,40 @@ static struct command *commands;\n static const char pre_receive_hook[] = \"hooks/pre-receive\";\n static const char post_receive_hook[] = \"hooks/post-receive\";\n \n+static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+\n+static void report_message(const char *prefix, const char *err, va_list params)\n+{\n+\tint sz = strlen(prefix);\n+\tchar msg[4096];\n+\n+\tstrncpy(msg, prefix, sz);\n+\tsz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);\n+\tmsg[sz++] = '\\n';\n+\n+\tif (use_sideband)\n+\t\tsend_sideband(1, 2, msg, sz, use_sideband);\n+\telse\n+\t\txwrite(2, msg, sz);\n+}\n+\n+static void rp_warning(const char *err, ...)\n+{\n+\tva_list params;\n+\tva_start(params, err);\n+\treport_message(\"warning: \", err, params);\n+\tva_end(params);\n+}\n+\n+static void rp_error(const char *err, ...)\n+{\n+\tva_list params;\n+\tva_start(params, err);\n+\treport_message(\"error: \", err, params);\n+\tva_end(params);\n+}\n+\n static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n@@ -276,7 +310,7 @@ static void warn_unconfigured_deny(void)\n {\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)\n-\t\twarning(\"%s\", warn_unconfigured_deny_msg[i]);\n+\t\trp_warning(\"%s\", warn_unconfigured_deny_msg[i]);\n }\n \n static char *warn_unconfigured_deny_delete_current_msg[] = {\n@@ -302,7 +336,7 @@ static void warn_unconfigured_deny_delete_current(void)\n \tfor (i = 0;\n \t     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);\n \t     i++)\n-\t\twarning(\"%s\", warn_unconfigured_deny_delete_current_msg[i]);\n+\t\trp_warning(\"%s\", warn_unconfigured_deny_delete_current_msg[i]);\n }\n \n static const char *update(struct command *cmd)\n@@ -314,7 +348,7 @@ static const char *update(struct command *cmd)\n \n \t/* only refs/... are allowed */\n \tif (prefixcmp(name, \"refs/\") || check_ref_format(name + 5)) {\n-\t\terror(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n \t\treturn \"funny refname\";\n \t}\n \n@@ -324,25 +358,25 @@ static const char *update(struct command *cmd)\n \t\t\tbreak;\n \t\tcase DENY_UNCONFIGURED:\n \t\tcase DENY_WARN:\n-\t\t\twarning(\"updating the current branch\");\n+\t\t\trp_warning(\"updating the current branch\");\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\twarn_unconfigured_deny();\n \t\t\tbreak;\n \t\tcase DENY_REFUSE:\n-\t\t\terror(\"refusing to update checked out branch: %s\", name);\n+\t\t\trp_error(\"refusing to update checked out branch: %s\", name);\n \t\t\treturn \"branch is currently checked out\";\n \t\t}\n \t}\n \n \tif (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {\n-\t\terror(\"unpack should have generated %s, \"\n-\t\t      \"but I can't find it!\", sha1_to_hex(new_sha1));\n+\t\trp_error(\"unpack should have generated %s, \"\n+\t\t\t \"but I can't find it!\", sha1_to_hex(new_sha1));\n \t\treturn \"bad pack\";\n \t}\n \n \tif (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {\n \t\tif (deny_deletes && !prefixcmp(name, \"refs/heads/\")) {\n-\t\t\terror(\"denying ref deletion for %s\", name);\n+\t\t\trp_error(\"denying ref deletion for %s\", name);\n \t\t\treturn \"deletion prohibited\";\n \t\t}\n \n@@ -354,10 +388,10 @@ static const char *update(struct command *cmd)\n \t\t\tcase DENY_UNCONFIGURED:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\twarn_unconfigured_deny_delete_current();\n-\t\t\t\twarning(\"deleting the current branch\");\n+\t\t\t\trp_warning(\"deleting the current branch\");\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n-\t\t\t\terror(\"refusing to delete the current branch: %s\", name);\n+\t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n \t\t\t}\n \t\t}\n@@ -376,7 +410,7 @@ static const char *update(struct command *cmd)\n \t\tif (!old_object || !new_object ||\n \t\t    old_object->type != OBJ_COMMIT ||\n \t\t    new_object->type != OBJ_COMMIT) {\n-\t\t\terror(\"bad sha1 objects for %s\", name);\n+\t\t\trp_error(\"bad sha1 objects for %s\", name);\n \t\t\treturn \"bad ref\";\n \t\t}\n \t\told_commit = (struct commit *)old_object;\n@@ -387,23 +421,23 @@ static const char *update(struct command *cmd)\n \t\t\t\tbreak;\n \t\tfree_commit_list(bases);\n \t\tif (!ent) {\n-\t\t\terror(\"denying non-fast-forward %s\"\n-\t\t\t      \" (you should pull first)\", name);\n+\t\t\trp_error(\"denying non-fast-forward %s\"\n+\t\t\t\t \" (you should pull first)\", name);\n \t\t\treturn \"non-fast-forward\";\n \t\t}\n \t}\n \tif (run_update_hook(cmd)) {\n-\t\terror(\"hook declined to update %s\", name);\n+\t\trp_error(\"hook declined to update %s\", name);\n \t\treturn \"hook declined\";\n \t}\n \n \tif (is_null_sha1(new_sha1)) {\n \t\tif (!parse_object(old_sha1)) {\n-\t\t\twarning (\"Allowing deletion of corrupt ref.\");\n+\t\t\trp_warning(\"Allowing deletion of corrupt ref.\");\n \t\t\told_sha1 = NULL;\n \t\t}\n \t\tif (delete_ref(name, old_sha1, 0)) {\n-\t\t\terror(\"failed to delete %s\", name);\n+\t\t\trp_error(\"failed to delete %s\", name);\n \t\t\treturn \"failed to delete\";\n \t\t}\n \t\treturn NULL; /* good */\n@@ -411,7 +445,7 @@ static const char *update(struct command *cmd)\n \telse {\n \t\tlock = lock_any_ref_for_update(name, old_sha1, 0);\n \t\tif (!lock) {\n-\t\t\terror(\"failed to lock %s\", name);\n+\t\t\trp_error(\"failed to lock %s\", name);\n \t\t\treturn \"failed to lock\";\n \t\t}\n \t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex 7240fab..17bcb0b 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master\n remote: STDERR update refs/heads/master\n remote: STDOUT update refs/heads/tofail\n remote: STDERR update refs/heads/tofail\n+remote: error: hook declined to update refs/heads/tofail\n remote: STDOUT post-receive\n remote: STDERR post-receive\n remote: STDOUT post-update\n@@ -131,7 +132,7 @@ remote: STDERR post-update\n EOF\n test_expect_success 'send-pack stderr contains hook messages' '\n \tgrep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n-\ttest_cmp - actual <expect\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n1.7.0.rc2.170.gbc565\n"},{"id":"134129","messageId":"4B725CB1.1080908@viscovery.net","threadId":"22594","inReplyTo":"1265767290-25863-2-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-10T07:13:53Z","receivedAt":"2010-02-10T07:13:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> +\n> +static void report_message(const char *prefix, const char *err, va_list params)\n> +{\n> +\tint sz = strlen(prefix);\n> +\tchar msg[4096];\n> +\n> +\tstrncpy(msg, prefix, sz);\n> +\tsz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);\n> +\tmsg[sz++] = '\\n';\n\nThis writes beyond the buffer if it is too small because the return value\ntells how many characters *would* have been written if it were\nsufficiently large, no?\n\n> +\n> +\tif (use_sideband)\n> +\t\tsend_sideband(1, 2, msg, sz, use_sideband);\n> +\telse\n> +\t\txwrite(2, msg, sz);\n> +}\n> +\n> +static void rp_warning(const char *err, ...)\n> ...\n> +static void rp_error(const char *err, ...)\n> ...\n\nLooks like we need set_report_routine().\n\nOr did you replace only selected error() and warning() calls by rp_error()\nand rp_warning()?\n\n-- Hannes\n"},{"id":"134130","messageId":"7vwryleeq5.fsf@alter.siamese.dyndns.org","threadId":"22594","inReplyTo":"4B725CB1.1080908@viscovery.net","subject":"Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-10T07:23:30Z","receivedAt":"2010-02-10T07:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Looks like we need set_report_routine().\n>\n> Or did you replace only selected error() and warning() calls by rp_error()\n> and rp_warning()?\n\nThat actually sounds like a good suggestion.\n"},{"id":"134132","messageId":"4B726A8C.6010600@viscovery.net","threadId":"22594","inReplyTo":"4B725CB1.1080908@viscovery.net","subject":"Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-10T08:13:00Z","receivedAt":"2010-02-10T08:13:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n> Or did you replace only selected error() and warning() calls by rp_error()\n> and rp_warning()?\n\nActually, you want to send only selected messages to the pusher. For\nexample, these look like errors on the server side (right?) and should go\nto the site administrator:\n\n>  \tif (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {\n> -\t\terror(\"unpack should have generated %s, \"\n> -\t\t      \"but I can't find it!\", sha1_to_hex(new_sha1));\n> +\t\trp_error(\"unpack should have generated %s, \"\n> +\t\t\t \"but I can't find it!\", sha1_to_hex(new_sha1));\n>  \t\treturn \"bad pack\";\n>  \t}\n\n>  \t\tif (!old_object || !new_object ||\n>  \t\t    old_object->type != OBJ_COMMIT ||\n>  \t\t    new_object->type != OBJ_COMMIT) {\n> -\t\t\terror(\"bad sha1 objects for %s\", name);\n> +\t\t\trp_error(\"bad sha1 objects for %s\", name);\n>  \t\t\treturn \"bad ref\";\n>  \t\t}\n>  \t\told_commit = (struct commit *)old_object;\n\nIn particular, your patch does not send errors produced by unpack-objects\nor index-pack to the pusher over the sideband, and this is the right thing\nto do.\n\n-- Hannes\n"},{"id":"134156","messageId":"20100210171708.GF2747@spearce.org","threadId":"22594","inReplyTo":"4B725CB1.1080908@viscovery.net","subject":"Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T17:17:08Z","receivedAt":"2010-02-10T17:17:08Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Shawn O. Pearce schrieb:\n> > +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> > +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> > +\n> > +static void report_message(const char *prefix, const char *err, va_list params)\n> > +{\n> > +\tint sz = strlen(prefix);\n> > +\tchar msg[4096];\n> > +\n> > +\tstrncpy(msg, prefix, sz);\n> > +\tsz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);\n> > +\tmsg[sz++] = '\\n';\n> \n> This writes beyond the buffer if it is too small because the return value\n> tells how many characters *would* have been written if it were\n> sufficiently large, no?\n\nUgh.  I don't code C often enough anymore.\n\nThank you for catching that.\n\n \n> > +static void rp_warning(const char *err, ...)\n> > ...\n> > +static void rp_error(const char *err, ...)\n> > ...\n> \n> Looks like we need set_report_routine().\n> \n> Or did you replace only selected error() and warning() calls by rp_error()\n> and rp_warning()?\n\nAs described by others in the thread... I only replaced selected\ncalls.  Well, most of them, maybe too many.  But I didn't want to\nexpose everything to the client.  So I added new functions.  Yes, it\nwas painful.  I wasn't happy about it.  But I also wasn't happy about\nexposing every message to the client over the side-band channel.\n\n-- \nShawn.\n"},{"id":"134159","messageId":"20100210173412.GG2747@spearce.org","threadId":"22594","inReplyTo":"4B726A8C.6010600@viscovery.net","subject":"[PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T17:34:12Z","receivedAt":"2010-02-10T17:34:12Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the client has requested side-band-64k capability, send any\nof the internal error or warning messages in the muxed side-band\nstream using the same band as our hook output, band #2.  By putting\neverything in one stream we ensure all messages are processed by\nthe side-band demuxer, avoiding interleaving between our own stderr\nand the side-band demuxer's stderr buffers.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n - Buffer overflow fixed.\n - Kept the two corruption errors on the server side.\n\n builtin-receive-pack.c  |   64 ++++++++++++++++++++++++++++++++++++----------\n t/t5401-update-hooks.sh |    3 +-\n 2 files changed, 52 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex da1c26b..a5543f9 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -139,6 +139,42 @@ static struct command *commands;\n static const char pre_receive_hook[] = \"hooks/pre-receive\";\n static const char post_receive_hook[] = \"hooks/post-receive\";\n \n+static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+\n+static void report_message(const char *prefix, const char *err, va_list params)\n+{\n+\tint sz = strlen(prefix);\n+\tchar msg[4096];\n+\n+\tstrncpy(msg, prefix, sz);\n+\tsz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);\n+\tif (sz > (sizeof(msg) - 1))\n+\t\tsz = sizeof(msg) - 1;\n+\tmsg[sz++] = '\\n';\n+\n+\tif (use_sideband)\n+\t\tsend_sideband(1, 2, msg, sz, use_sideband);\n+\telse\n+\t\txwrite(2, msg, sz);\n+}\n+\n+static void rp_warning(const char *err, ...)\n+{\n+\tva_list params;\n+\tva_start(params, err);\n+\treport_message(\"warning: \", err, params);\n+\tva_end(params);\n+}\n+\n+static void rp_error(const char *err, ...)\n+{\n+\tva_list params;\n+\tva_start(params, err);\n+\treport_message(\"error: \", err, params);\n+\tva_end(params);\n+}\n+\n static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n@@ -276,7 +312,7 @@ static void warn_unconfigured_deny(void)\n {\n \tint i;\n \tfor (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)\n-\t\twarning(\"%s\", warn_unconfigured_deny_msg[i]);\n+\t\trp_warning(\"%s\", warn_unconfigured_deny_msg[i]);\n }\n \n static char *warn_unconfigured_deny_delete_current_msg[] = {\n@@ -302,7 +338,7 @@ static void warn_unconfigured_deny_delete_current(void)\n \tfor (i = 0;\n \t     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);\n \t     i++)\n-\t\twarning(\"%s\", warn_unconfigured_deny_delete_current_msg[i]);\n+\t\trp_warning(\"%s\", warn_unconfigured_deny_delete_current_msg[i]);\n }\n \n static const char *update(struct command *cmd)\n@@ -314,7 +350,7 @@ static const char *update(struct command *cmd)\n \n \t/* only refs/... are allowed */\n \tif (prefixcmp(name, \"refs/\") || check_ref_format(name + 5)) {\n-\t\terror(\"refusing to create funny ref '%s' remotely\", name);\n+\t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n \t\treturn \"funny refname\";\n \t}\n \n@@ -324,12 +360,12 @@ static const char *update(struct command *cmd)\n \t\t\tbreak;\n \t\tcase DENY_UNCONFIGURED:\n \t\tcase DENY_WARN:\n-\t\t\twarning(\"updating the current branch\");\n+\t\t\trp_warning(\"updating the current branch\");\n \t\t\tif (deny_current_branch == DENY_UNCONFIGURED)\n \t\t\t\twarn_unconfigured_deny();\n \t\t\tbreak;\n \t\tcase DENY_REFUSE:\n-\t\t\terror(\"refusing to update checked out branch: %s\", name);\n+\t\t\trp_error(\"refusing to update checked out branch: %s\", name);\n \t\t\treturn \"branch is currently checked out\";\n \t\t}\n \t}\n@@ -342,7 +378,7 @@ static const char *update(struct command *cmd)\n \n \tif (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {\n \t\tif (deny_deletes && !prefixcmp(name, \"refs/heads/\")) {\n-\t\t\terror(\"denying ref deletion for %s\", name);\n+\t\t\trp_error(\"denying ref deletion for %s\", name);\n \t\t\treturn \"deletion prohibited\";\n \t\t}\n \n@@ -354,10 +390,10 @@ static const char *update(struct command *cmd)\n \t\t\tcase DENY_UNCONFIGURED:\n \t\t\t\tif (deny_delete_current == DENY_UNCONFIGURED)\n \t\t\t\t\twarn_unconfigured_deny_delete_current();\n-\t\t\t\twarning(\"deleting the current branch\");\n+\t\t\t\trp_warning(\"deleting the current branch\");\n \t\t\t\tbreak;\n \t\t\tcase DENY_REFUSE:\n-\t\t\t\terror(\"refusing to delete the current branch: %s\", name);\n+\t\t\t\trp_error(\"refusing to delete the current branch: %s\", name);\n \t\t\t\treturn \"deletion of the current branch prohibited\";\n \t\t\t}\n \t\t}\n@@ -387,23 +423,23 @@ static const char *update(struct command *cmd)\n \t\t\t\tbreak;\n \t\tfree_commit_list(bases);\n \t\tif (!ent) {\n-\t\t\terror(\"denying non-fast-forward %s\"\n-\t\t\t      \" (you should pull first)\", name);\n+\t\t\trp_error(\"denying non-fast-forward %s\"\n+\t\t\t\t \" (you should pull first)\", name);\n \t\t\treturn \"non-fast-forward\";\n \t\t}\n \t}\n \tif (run_update_hook(cmd)) {\n-\t\terror(\"hook declined to update %s\", name);\n+\t\trp_error(\"hook declined to update %s\", name);\n \t\treturn \"hook declined\";\n \t}\n \n \tif (is_null_sha1(new_sha1)) {\n \t\tif (!parse_object(old_sha1)) {\n-\t\t\twarning (\"Allowing deletion of corrupt ref.\");\n+\t\t\trp_warning(\"Allowing deletion of corrupt ref.\");\n \t\t\told_sha1 = NULL;\n \t\t}\n \t\tif (delete_ref(name, old_sha1, 0)) {\n-\t\t\terror(\"failed to delete %s\", name);\n+\t\t\trp_error(\"failed to delete %s\", name);\n \t\t\treturn \"failed to delete\";\n \t\t}\n \t\treturn NULL; /* good */\n@@ -411,7 +447,7 @@ static const char *update(struct command *cmd)\n \telse {\n \t\tlock = lock_any_ref_for_update(name, old_sha1, 0);\n \t\tif (!lock) {\n-\t\t\terror(\"failed to lock %s\", name);\n+\t\t\trp_error(\"failed to lock %s\", name);\n \t\t\treturn \"failed to lock\";\n \t\t}\n \t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex 7240fab..17bcb0b 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master\n remote: STDERR update refs/heads/master\n remote: STDOUT update refs/heads/tofail\n remote: STDERR update refs/heads/tofail\n+remote: error: hook declined to update refs/heads/tofail\n remote: STDOUT post-receive\n remote: STDERR post-receive\n remote: STDOUT post-update\n@@ -131,7 +132,7 @@ remote: STDERR post-update\n EOF\n test_expect_success 'send-pack stderr contains hook messages' '\n \tgrep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n-\ttest_cmp - actual <expect\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n1.7.0.rc2.170.gbc565\n\n-- \nShawn.\n"},{"id":"134211","messageId":"4B73C0FF.5020503@viscovery.net","threadId":"22594","inReplyTo":"20100210173412.GG2747@spearce.org","subject":"Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-11T08:34:07Z","receivedAt":"2010-02-11T08:34:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> +static void report_message(const char *prefix, const char *err, va_list params)\n> +{\n> +\tint sz = strlen(prefix);\n> +\tchar msg[4096];\n> +\n> +\tstrncpy(msg, prefix, sz);\n> +\tsz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);\n> +\tif (sz > (sizeof(msg) - 1))\n> +\t\tsz = sizeof(msg) - 1;\n> +\tmsg[sz++] = '\\n';\n\nSorry, still no joy - the terminating NUL is missing (I should have \nnoticed this in your v1 already). I suggest to forgo the length check for \nsimplicity because this function is only called with data that is already \nguaranteed to be less than 1000 bytes, i.e.:\n\n\tstrncpy(msg, prefix, sz);\n\t/* data is guaranteed to fit due to packet length limit in \nread_head_info() */\n\tsz += vsprintf(msg + sz, err, params);\n\tmsg[sz++] = '\\n';\n\tmsg[sz++] = '\\0';\n\n-- Hannes\n"},{"id":"134237","messageId":"20100211150547.GL2747@spearce.org","threadId":"22594","inReplyTo":"4B73C0FF.5020503@viscovery.net","subject":"Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-11T15:05:47Z","receivedAt":"2010-02-11T15:05:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Shawn O. Pearce schrieb:\n>> +static void report_message(const char *prefix, const char *err, va_list params)\n>> +{\n>> +\tint sz = strlen(prefix);\n>> +\tchar msg[4096];\n>> +\n>> +\tstrncpy(msg, prefix, sz);\n>> +\tsz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);\n>> +\tif (sz > (sizeof(msg) - 1))\n>> +\t\tsz = sizeof(msg) - 1;\n>> +\tmsg[sz++] = '\\n';\n>\n> Sorry, still no joy - the terminating NUL is missing (I should have  \n> noticed this in your v1 already).\n\nWhy is it necessary?\n\nOnce the msg buffer is prepared, its written using its length, sz,\nnot its NUL termination status.  Neither send_sideband() nor xwrite()\ncare about NUL termination.\n\nThe only reason to put a NUL onto this buffer is so you can do\n\"p msg\" within GDB and get a useful result.  We don't typically do\nthis in these contexts.\n\nOr did my MTA inject additional C code I didn't write?\n\n> I suggest to forgo the length check for \n> simplicity because this function is only called with data that is already \n> guaranteed to be less than 1000 bytes, i.e.:\n>\n> \tstrncpy(msg, prefix, sz);\n> \t/* data is guaranteed to fit due to packet length limit in  \n> read_head_info() */\n\nYea, that's true now.  We probably could have used 1200 bytes for\nthe msg buffer, rather than 4096.  Even the huge warnings about\ncurrent branch behavior are broken up into one string per line.\n\n-- \nShawn.\n"},{"id":"134251","messageId":"4B7454B6.50703@viscovery.net","threadId":"22594","inReplyTo":"20100211150547.GL2747@spearce.org","subject":"Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-11T19:04:22Z","receivedAt":"2010-02-11T19:04:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Sorry, still no joy - the terminating NUL is missing (I should have  \n>> noticed this in your v1 already).\n> \n> Why is it necessary?\n\nIt isn't, sorry - I'm sick, and it shows :-/ Your version is fine.\n\n-- Hannes\n"}]}