[PATCH 7/6] t5401: Use a bare repository for the remote peer

[PATCH 7/6] t5401: Use a bare repository for the remote peer

[Shawn O. Pearce]

We want to avoid the warnings (or later, test failures) about
updating the current branch.  It was never my intention to have
this test deal with a repository with a working directory, and it
is a very old bug that the test even used a non-bare repository
for the remote side of the push operations.

This fixes the interleaved output error we were seeing as a test
failure by avoiding the giant warning message we were getting back
about updating the current branch being risky.

Its not a real fix, but is something we should do no matter what,
because the behavior will change in the future to reject, and the
test would break at that time.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 t/t5401-update-hooks.sh |   58 +++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index c3cf397..7240fab 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -17,22 +17,22 @@ test_expect_success setup '
 	commit1=$(echo modify | git commit-tree $tree1 -p $commit0) &&
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
-	git clone ./. victim &&
-	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
+	git clone --bare ./. victim.git &&
+	GIT_DIR=victim.git git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
 '
 
-cat >victim/.git/hooks/pre-receive <<'EOF'
+cat >victim.git/hooks/pre-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/pre-receive.args
 cat - >$GIT_DIR/pre-receive.stdin
 echo STDOUT pre-receive
 echo STDERR pre-receive >&2
 EOF
-chmod u+x victim/.git/hooks/pre-receive
+chmod u+x victim.git/hooks/pre-receive
 
-cat >victim/.git/hooks/update <<'EOF'
+cat >victim.git/hooks/update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/update.args
 read x; printf %s "$x" >$GIT_DIR/update.stdin
@@ -40,77 +40,77 @@ echo STDOUT update $1
 echo STDERR update $1 >&2
 test "$1" = refs/heads/master || exit
 EOF
-chmod u+x victim/.git/hooks/update
+chmod u+x victim.git/hooks/update
 
-cat >victim/.git/hooks/post-receive <<'EOF'
+cat >victim.git/hooks/post-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/post-receive.args
 cat - >$GIT_DIR/post-receive.stdin
 echo STDOUT post-receive
 echo STDERR post-receive >&2
 EOF
-chmod u+x victim/.git/hooks/post-receive
+chmod u+x victim.git/hooks/post-receive
 
-cat >victim/.git/hooks/post-update <<'EOF'
+cat >victim.git/hooks/post-update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/post-update.args
 read x; printf %s "$x" >$GIT_DIR/post-update.stdin
 echo STDOUT post-update
 echo STDERR post-update >&2
 EOF
-chmod u+x victim/.git/hooks/post-update
+chmod u+x victim.git/hooks/post-update
 
 test_expect_success push '
-	test_must_fail git send-pack --force ./victim/.git \
+	test_must_fail git send-pack --force ./victim.git \
 		master tofail >send.out 2>send.err
 '
 
 test_expect_success 'updated as expected' '
-	test $(GIT_DIR=victim/.git git rev-parse master) = $commit1 &&
-	test $(GIT_DIR=victim/.git git rev-parse tofail) = $commit1
+	test $(GIT_DIR=victim.git git rev-parse master) = $commit1 &&
+	test $(GIT_DIR=victim.git git rev-parse tofail) = $commit1
 '
 
 test_expect_success 'hooks ran' '
-	test -f victim/.git/pre-receive.args &&
-	test -f victim/.git/pre-receive.stdin &&
-	test -f victim/.git/update.args &&
-	test -f victim/.git/update.stdin &&
-	test -f victim/.git/post-receive.args &&
-	test -f victim/.git/post-receive.stdin &&
-	test -f victim/.git/post-update.args &&
-	test -f victim/.git/post-update.stdin
+	test -f victim.git/pre-receive.args &&
+	test -f victim.git/pre-receive.stdin &&
+	test -f victim.git/update.args &&
+	test -f victim.git/update.stdin &&
+	test -f victim.git/post-receive.args &&
+	test -f victim.git/post-receive.stdin &&
+	test -f victim.git/post-update.args &&
+	test -f victim.git/post-update.stdin
 '
 
 test_expect_success 'pre-receive hook input' '
 	(echo $commit0 $commit1 refs/heads/master;
 	 echo $commit1 $commit0 refs/heads/tofail
-	) | test_cmp - victim/.git/pre-receive.stdin
+	) | test_cmp - victim.git/pre-receive.stdin
 '
 
 test_expect_success 'update hook arguments' '
 	(echo refs/heads/master $commit0 $commit1;
 	 echo refs/heads/tofail $commit1 $commit0
-	) | test_cmp - victim/.git/update.args
+	) | test_cmp - victim.git/update.args
 '
 
 test_expect_success 'post-receive hook input' '
 	echo $commit0 $commit1 refs/heads/master |
-	test_cmp - victim/.git/post-receive.stdin
+	test_cmp - victim.git/post-receive.stdin
 '
 
 test_expect_success 'post-update hook arguments' '
 	echo refs/heads/master |
-	test_cmp - victim/.git/post-update.args
+	test_cmp - victim.git/post-update.args
 '
 
 test_expect_success 'all hook stdin is /dev/null' '
-	! test -s victim/.git/update.stdin &&
-	! test -s victim/.git/post-update.stdin
+	! test -s victim.git/update.stdin &&
+	! test -s victim.git/post-update.stdin
 '
 
 test_expect_success 'all *-receive hook args are empty' '
-	! test -s victim/.git/pre-receive.args &&
-	! test -s victim/.git/post-receive.args
+	! test -s victim.git/pre-receive.args &&
+	! test -s victim.git/post-receive.args
 '
 
 test_expect_success 'send-pack produced no output' '
-- 
1.7.0.rc2.170.gbc565

[PATCH 8/6] receive-pack: Send internal errors over side-band #2

[Shawn O. Pearce]

If the client has requested side-band-64k capability, send any
of the internal error or warning messages in the muxed side-band
stream using the same band as our hook output, band #2.  By putting
everything in one stream we ensure all messages are processed by
the side-band demuxer, avoiding interleaving between our own stderr
and the side-band demuxer's stderr buffers.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-receive-pack.c  |   68 +++++++++++++++++++++++++++++++++++-----------
 t/t5401-update-hooks.sh |    3 +-
 2 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index da1c26b..e98c2f1 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -139,6 +139,40 @@ static struct command *commands;
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
+static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+
+static void report_message(const char *prefix, const char *err, va_list params)
+{
+	int sz = strlen(prefix);
+	char msg[4096];
+
+	strncpy(msg, prefix, sz);
+	sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);
+	msg[sz++] = '\n';
+
+	if (use_sideband)
+		send_sideband(1, 2, msg, sz, use_sideband);
+	else
+		xwrite(2, msg, sz);
+}
+
+static void rp_warning(const char *err, ...)
+{
+	va_list params;
+	va_start(params, err);
+	report_message("warning: ", err, params);
+	va_end(params);
+}
+
+static void rp_error(const char *err, ...)
+{
+	va_list params;
+	va_start(params, err);
+	report_message("error: ", err, params);
+	va_end(params);
+}
+
 static int copy_to_sideband(int in, int out, void *arg)
 {
 	char data[128];
@@ -276,7 +310,7 @@ static void warn_unconfigured_deny(void)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)
-		warning("%s", warn_unconfigured_deny_msg[i]);
+		rp_warning("%s", warn_unconfigured_deny_msg[i]);
 }
 
 static char *warn_unconfigured_deny_delete_current_msg[] = {
@@ -302,7 +336,7 @@ static void warn_unconfigured_deny_delete_current(void)
 	for (i = 0;
 	     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);
 	     i++)
-		warning("%s", warn_unconfigured_deny_delete_current_msg[i]);
+		rp_warning("%s", warn_unconfigured_deny_delete_current_msg[i]);
 }
 
 static const char *update(struct command *cmd)
@@ -314,7 +348,7 @@ static const char *update(struct command *cmd)
 
 	/* only refs/... are allowed */
 	if (prefixcmp(name, "refs/") || check_ref_format(name + 5)) {
-		error("refusing to create funny ref '%s' remotely", name);
+		rp_error("refusing to create funny ref '%s' remotely", name);
 		return "funny refname";
 	}
 
@@ -324,25 +358,25 @@ static const char *update(struct command *cmd)
 			break;
 		case DENY_UNCONFIGURED:
 		case DENY_WARN:
-			warning("updating the current branch");
+			rp_warning("updating the current branch");
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				warn_unconfigured_deny();
 			break;
 		case DENY_REFUSE:
-			error("refusing to update checked out branch: %s", name);
+			rp_error("refusing to update checked out branch: %s", name);
 			return "branch is currently checked out";
 		}
 	}
 
 	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
-		error("unpack should have generated %s, "
-		      "but I can't find it!", sha1_to_hex(new_sha1));
+		rp_error("unpack should have generated %s, "
+			 "but I can't find it!", sha1_to_hex(new_sha1));
 		return "bad pack";
 	}
 
 	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
 		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
-			error("denying ref deletion for %s", name);
+			rp_error("denying ref deletion for %s", name);
 			return "deletion prohibited";
 		}
 
@@ -354,10 +388,10 @@ static const char *update(struct command *cmd)
 			case DENY_UNCONFIGURED:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					warn_unconfigured_deny_delete_current();
-				warning("deleting the current branch");
+				rp_warning("deleting the current branch");
 				break;
 			case DENY_REFUSE:
-				error("refusing to delete the current branch: %s", name);
+				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
 			}
 		}
@@ -376,7 +410,7 @@ static const char *update(struct command *cmd)
 		if (!old_object || !new_object ||
 		    old_object->type != OBJ_COMMIT ||
 		    new_object->type != OBJ_COMMIT) {
-			error("bad sha1 objects for %s", name);
+			rp_error("bad sha1 objects for %s", name);
 			return "bad ref";
 		}
 		old_commit = (struct commit *)old_object;
@@ -387,23 +421,23 @@ static const char *update(struct command *cmd)
 				break;
 		free_commit_list(bases);
 		if (!ent) {
-			error("denying non-fast-forward %s"
-			      " (you should pull first)", name);
+			rp_error("denying non-fast-forward %s"
+				 " (you should pull first)", name);
 			return "non-fast-forward";
 		}
 	}
 	if (run_update_hook(cmd)) {
-		error("hook declined to update %s", name);
+		rp_error("hook declined to update %s", name);
 		return "hook declined";
 	}
 
 	if (is_null_sha1(new_sha1)) {
 		if (!parse_object(old_sha1)) {
-			warning ("Allowing deletion of corrupt ref.");
+			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
 		if (delete_ref(name, old_sha1, 0)) {
-			error("failed to delete %s", name);
+			rp_error("failed to delete %s", name);
 			return "failed to delete";
 		}
 		return NULL; /* good */
@@ -411,7 +445,7 @@ static const char *update(struct command *cmd)
 	else {
 		lock = lock_any_ref_for_update(name, old_sha1, 0);
 		if (!lock) {
-			error("failed to lock %s", name);
+			rp_error("failed to lock %s", name);
 			return "failed to lock";
 		}
 		if (write_ref_sha1(lock, new_sha1, "push")) {
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 7240fab..17bcb0b 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master
 remote: STDERR update refs/heads/master
 remote: STDOUT update refs/heads/tofail
 remote: STDERR update refs/heads/tofail
+remote: error: hook declined to update refs/heads/tofail
 remote: STDOUT post-receive
 remote: STDERR post-receive
 remote: STDOUT post-update
@@ -131,7 +132,7 @@ remote: STDERR post-update
 EOF
 test_expect_success 'send-pack stderr contains hook messages' '
 	grep ^remote: send.err | sed "s/ *\$//" >actual &&
-	test_cmp - actual <expect
+	test_cmp expect actual
 '
 
 test_done
-- 
1.7.0.rc2.170.gbc565

Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2

[Johannes Sixt]

Shawn O. Pearce schrieb:
> +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
> +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> +
> +static void report_message(const char *prefix, const char *err, va_list params)
> +{
> +	int sz = strlen(prefix);
> +	char msg[4096];
> +
> +	strncpy(msg, prefix, sz);
> +	sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);
> +	msg[sz++] = '\n';

This writes beyond the buffer if it is too small because the return value
tells how many characters *would* have been written if it were
sufficiently large, no?

> +
> +	if (use_sideband)
> +		send_sideband(1, 2, msg, sz, use_sideband);
> +	else
> +		xwrite(2, msg, sz);
> +}
> +
> +static void rp_warning(const char *err, ...)
> ...
> +static void rp_error(const char *err, ...)
> ...

Looks like we need set_report_routine().

Or did you replace only selected error() and warning() calls by rp_error()
and rp_warning()?

-- Hannes

Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Looks like we need set_report_routine().
>
> Or did you replace only selected error() and warning() calls by rp_error()
> and rp_warning()?

That actually sounds like a good suggestion.

Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2

[Johannes Sixt]

Johannes Sixt schrieb:
> Or did you replace only selected error() and warning() calls by rp_error()
> and rp_warning()?

Actually, you want to send only selected messages to the pusher. For
example, these look like errors on the server side (right?) and should go
to the site administrator:

>  	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> -		error("unpack should have generated %s, "
> -		      "but I can't find it!", sha1_to_hex(new_sha1));
> +		rp_error("unpack should have generated %s, "
> +			 "but I can't find it!", sha1_to_hex(new_sha1));
>  		return "bad pack";
>  	}

>  		if (!old_object || !new_object ||
>  		    old_object->type != OBJ_COMMIT ||
>  		    new_object->type != OBJ_COMMIT) {
> -			error("bad sha1 objects for %s", name);
> +			rp_error("bad sha1 objects for %s", name);
>  			return "bad ref";
>  		}
>  		old_commit = (struct commit *)old_object;

In particular, your patch does not send errors produced by unpack-objects
or index-pack to the pusher over the sideband, and this is the right thing
to do.

-- Hannes

Re: [PATCH 8/6] receive-pack: Send internal errors over side-band #2

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> Shawn O. Pearce schrieb:
> > +static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > +static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > +
> > +static void report_message(const char *prefix, const char *err, va_list params)
> > +{
> > +	int sz = strlen(prefix);
> > +	char msg[4096];
> > +
> > +	strncpy(msg, prefix, sz);
> > +	sz += vsnprintf(msg + sz, sizeof(msg) - (sz + 1), err, params);
> > +	msg[sz++] = '\n';
> 
> This writes beyond the buffer if it is too small because the return value
> tells how many characters *would* have been written if it were
> sufficiently large, no?

Ugh.  I don't code C often enough anymore.

Thank you for catching that.

 
> > +static void rp_warning(const char *err, ...)
> > ...
> > +static void rp_error(const char *err, ...)
> > ...
> 
> Looks like we need set_report_routine().
> 
> Or did you replace only selected error() and warning() calls by rp_error()
> and rp_warning()?

As described by others in the thread... I only replaced selected
calls.  Well, most of them, maybe too many.  But I didn't want to
expose everything to the client.  So I added new functions.  Yes, it
was painful.  I wasn't happy about it.  But I also wasn't happy about
exposing every message to the client over the side-band channel.

-- 
Shawn.

[PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2

[Shawn O. Pearce]

If the client has requested side-band-64k capability, send any
of the internal error or warning messages in the muxed side-band
stream using the same band as our hook output, band #2.  By putting
everything in one stream we ensure all messages are processed by
the side-band demuxer, avoiding interleaving between our own stderr
and the side-band demuxer's stderr buffers.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 - Buffer overflow fixed.
 - Kept the two corruption errors on the server side.

 builtin-receive-pack.c  |   64 ++++++++++++++++++++++++++++++++++++----------
 t/t5401-update-hooks.sh |    3 +-
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index da1c26b..a5543f9 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -139,6 +139,42 @@ static struct command *commands;
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
+static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+
+static void report_message(const char *prefix, const char *err, va_list params)
+{
+	int sz = strlen(prefix);
+	char msg[4096];
+
+	strncpy(msg, prefix, sz);
+	sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);
+	if (sz > (sizeof(msg) - 1))
+		sz = sizeof(msg) - 1;
+	msg[sz++] = '\n';
+
+	if (use_sideband)
+		send_sideband(1, 2, msg, sz, use_sideband);
+	else
+		xwrite(2, msg, sz);
+}
+
+static void rp_warning(const char *err, ...)
+{
+	va_list params;
+	va_start(params, err);
+	report_message("warning: ", err, params);
+	va_end(params);
+}
+
+static void rp_error(const char *err, ...)
+{
+	va_list params;
+	va_start(params, err);
+	report_message("error: ", err, params);
+	va_end(params);
+}
+
 static int copy_to_sideband(int in, int out, void *arg)
 {
 	char data[128];
@@ -276,7 +312,7 @@ static void warn_unconfigured_deny(void)
 {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)
-		warning("%s", warn_unconfigured_deny_msg[i]);
+		rp_warning("%s", warn_unconfigured_deny_msg[i]);
 }
 
 static char *warn_unconfigured_deny_delete_current_msg[] = {
@@ -302,7 +338,7 @@ static void warn_unconfigured_deny_delete_current(void)
 	for (i = 0;
 	     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);
 	     i++)
-		warning("%s", warn_unconfigured_deny_delete_current_msg[i]);
+		rp_warning("%s", warn_unconfigured_deny_delete_current_msg[i]);
 }
 
 static const char *update(struct command *cmd)
@@ -314,7 +350,7 @@ static const char *update(struct command *cmd)
 
 	/* only refs/... are allowed */
 	if (prefixcmp(name, "refs/") || check_ref_format(name + 5)) {
-		error("refusing to create funny ref '%s' remotely", name);
+		rp_error("refusing to create funny ref '%s' remotely", name);
 		return "funny refname";
 	}
 
@@ -324,12 +360,12 @@ static const char *update(struct command *cmd)
 			break;
 		case DENY_UNCONFIGURED:
 		case DENY_WARN:
-			warning("updating the current branch");
+			rp_warning("updating the current branch");
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				warn_unconfigured_deny();
 			break;
 		case DENY_REFUSE:
-			error("refusing to update checked out branch: %s", name);
+			rp_error("refusing to update checked out branch: %s", name);
 			return "branch is currently checked out";
 		}
 	}
@@ -342,7 +378,7 @@ static const char *update(struct command *cmd)
 
 	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
 		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
-			error("denying ref deletion for %s", name);
+			rp_error("denying ref deletion for %s", name);
 			return "deletion prohibited";
 		}
 
@@ -354,10 +390,10 @@ static const char *update(struct command *cmd)
 			case DENY_UNCONFIGURED:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					warn_unconfigured_deny_delete_current();
-				warning("deleting the current branch");
+				rp_warning("deleting the current branch");
 				break;
 			case DENY_REFUSE:
-				error("refusing to delete the current branch: %s", name);
+				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
 			}
 		}
@@ -387,23 +423,23 @@ static const char *update(struct command *cmd)
 				break;
 		free_commit_list(bases);
 		if (!ent) {
-			error("denying non-fast-forward %s"
-			      " (you should pull first)", name);
+			rp_error("denying non-fast-forward %s"
+				 " (you should pull first)", name);
 			return "non-fast-forward";
 		}
 	}
 	if (run_update_hook(cmd)) {
-		error("hook declined to update %s", name);
+		rp_error("hook declined to update %s", name);
 		return "hook declined";
 	}
 
 	if (is_null_sha1(new_sha1)) {
 		if (!parse_object(old_sha1)) {
-			warning ("Allowing deletion of corrupt ref.");
+			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
 		if (delete_ref(name, old_sha1, 0)) {
-			error("failed to delete %s", name);
+			rp_error("failed to delete %s", name);
 			return "failed to delete";
 		}
 		return NULL; /* good */
@@ -411,7 +447,7 @@ static const char *update(struct command *cmd)
 	else {
 		lock = lock_any_ref_for_update(name, old_sha1, 0);
 		if (!lock) {
-			error("failed to lock %s", name);
+			rp_error("failed to lock %s", name);
 			return "failed to lock";
 		}
 		if (write_ref_sha1(lock, new_sha1, "push")) {
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 7240fab..17bcb0b 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -124,6 +124,7 @@ remote: STDOUT update refs/heads/master
 remote: STDERR update refs/heads/master
 remote: STDOUT update refs/heads/tofail
 remote: STDERR update refs/heads/tofail
+remote: error: hook declined to update refs/heads/tofail
 remote: STDOUT post-receive
 remote: STDERR post-receive
 remote: STDOUT post-update
@@ -131,7 +132,7 @@ remote: STDERR post-update
 EOF
 test_expect_success 'send-pack stderr contains hook messages' '
 	grep ^remote: send.err | sed "s/ *\$//" >actual &&
-	test_cmp - actual <expect
+	test_cmp expect actual
 '
 
 test_done
-- 
1.7.0.rc2.170.gbc565

-- 
Shawn.

Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2

[Johannes Sixt]

Shawn O. Pearce schrieb:
> +static void report_message(const char *prefix, const char *err, va_list params)
> +{
> +	int sz = strlen(prefix);
> +	char msg[4096];
> +
> +	strncpy(msg, prefix, sz);
> +	sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);
> +	if (sz > (sizeof(msg) - 1))
> +		sz = sizeof(msg) - 1;
> +	msg[sz++] = '\n';

Sorry, still no joy - the terminating NUL is missing (I should have 
noticed this in your v1 already). I suggest to forgo the length check for 
simplicity because this function is only called with data that is already 
guaranteed to be less than 1000 bytes, i.e.:

	strncpy(msg, prefix, sz);
	/* data is guaranteed to fit due to packet length limit in 
read_head_info() */
	sz += vsprintf(msg + sz, err, params);
	msg[sz++] = '\n';
	msg[sz++] = '\0';

-- Hannes

Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> Shawn O. Pearce schrieb:
>> +static void report_message(const char *prefix, const char *err, va_list params)
>> +{
>> +	int sz = strlen(prefix);
>> +	char msg[4096];
>> +
>> +	strncpy(msg, prefix, sz);
>> +	sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);
>> +	if (sz > (sizeof(msg) - 1))
>> +		sz = sizeof(msg) - 1;
>> +	msg[sz++] = '\n';
>
> Sorry, still no joy - the terminating NUL is missing (I should have  
> noticed this in your v1 already).

Why is it necessary?

Once the msg buffer is prepared, its written using its length, sz,
not its NUL termination status.  Neither send_sideband() nor xwrite()
care about NUL termination.

The only reason to put a NUL onto this buffer is so you can do
"p msg" within GDB and get a useful result.  We don't typically do
this in these contexts.

Or did my MTA inject additional C code I didn't write?

> I suggest to forgo the length check for 
> simplicity because this function is only called with data that is already 
> guaranteed to be less than 1000 bytes, i.e.:
>
> 	strncpy(msg, prefix, sz);
> 	/* data is guaranteed to fit due to packet length limit in  
> read_head_info() */

Yea, that's true now.  We probably could have used 1200 bytes for
the msg buffer, rather than 4096.  Even the huge warnings about
current branch behavior are broken up into one string per line.

-- 
Shawn.

Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2

[Johannes Sixt]

Shawn O. Pearce schrieb:
> Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Sorry, still no joy - the terminating NUL is missing (I should have  
>> noticed this in your v1 already).
> 
> Why is it necessary?

It isn't, sorry - I'm sick, and it shows :-/ Your version is fine.

-- Hannes