threads / patch / 22594

patch, 6 partst5401: Use a bare repository for the remote peer

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

## tl;dr

10 messages between Feb 10, 2010 and Feb 11, 2010. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Shawn O. Pearce· Feb 10, 2010, 02:01 UTC · lore

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(-)
Show changes to t/t5401-update-hooks.sh +29 −29
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
Shawn O. Pearce· Feb 10, 2010, 02:01 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to 2 files +53 −18

builtin-receive-pack.c, t/t5401-update-hooks.sh

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
Johannes Sixt· Feb 10, 2010, 07:13 UTC · re: Shawn O. Pearce · lore

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

Shawn O. Pearce schrieb:
Show 11 quoted lines
> +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?

Show 11 quoted lines
> +
> +	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
Junio C Hamano· Feb 10, 2010, 07:23 UTC · re: Johannes Sixt · lore

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

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.
Johannes Sixt· Feb 10, 2010, 08:13 UTC · re: Johannes Sixt · lore

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

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:

Show 7 quoted lines
>  	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";
>  	}
Show 8 quoted lines
>  		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
Shawn O. Pearce· Feb 10, 2010, 17:34 UTC · re: Johannes Sixt · lore

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

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(-)
Show changes to 2 files +52 −15

builtin-receive-pack.c, t/t5401-update-hooks.sh

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.
Johannes Sixt· Feb 11, 2010, 08:34 UTC · re: Shawn O. Pearce · lore

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

Shawn O. Pearce schrieb:
Show 10 quoted lines
> +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
Shawn O. Pearce· Feb 11, 2010, 15:05 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 14 quoted lines
> 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?
Show 7 quoted lines
> 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.
Johannes Sixt· Feb 11, 2010, 19:04 UTC · re: Shawn O. Pearce · lore

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

Shawn O. Pearce schrieb:
Show 5 quoted lines
> 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
Shawn O. Pearce· Feb 10, 2010, 17:17 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 16 quoted lines
> 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.
Show 9 quoted lines
> > +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.

← back to recent threads