From 59bfdfb82a0e51f6e0a40197cd662196406b1bec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 21 Sep 2012 01:32:52 -0400 Subject: receive-pack: redirect unpack-objects stdout to /dev/null The unpack-objects command should not generally produce any output on stdout. However, if it's given extra input after the packfile, it will spew the remainder to stdout. When called by receive-pack, this means we will break protocol, since our stdout is connected to the remote send-pack. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2cb854feb..f8d2f1710 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -821,6 +821,7 @@ static const char *unpack(void) if (ntohl(hdr.hdr_entries) < unpack_limit) { int code, i = 0; + struct child_process child; const char *unpacker[5]; unpacker[i++] = "unpack-objects"; if (quiet) @@ -829,7 +830,11 @@ static const char *unpack(void) unpacker[i++] = "--strict"; unpacker[i++] = hdr_arg; unpacker[i++] = NULL; - code = run_command_v_opt(unpacker, RUN_GIT_CMD); + memset(&child, 0, sizeof(child)); + child.argv = unpacker; + child.no_stdout = 1; + child.git_cmd = 1; + code = run_command(&child); if (!code) return NULL; return "unpack-objects abnormal exit"; -- cgit v1.2.1 From a22e6f85470f47d33ce392a11a5f9e2bb9a5c308 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 21 Sep 2012 01:34:55 -0400 Subject: receive-pack: send pack-processing stderr over sideband Receive-pack invokes either unpack-objects or index-pack to handle the incoming pack. However, we do not redirect the stderr of the sub-processes at all, so it is never seen by the client. From the initial thread adding sideband support, which is here: http://thread.gmane.org/gmane.comp.version-control.git/139471 it is clear that some messages are specifically kept off the sideband (with the assumption that they are of interest only to an administrator, not the client). The stderr of the subprocesses is mentioned in the thread, but it's unclear if they are included in that group, or were simply forgotten. However, there are a few good reasons to show them to the client: 1. In many cases, they are directly about the incoming packfile (e.g., fsck warnings with --strict, corruption in the packfile, etc). Without these messages, the client just gets "unpacker error" with no extra useful diagnosis. 2. No matter what the cause, we are probably better off showing the errors to the client. If the client and the server admin are not the same entity, it is probably much easier for the client to cut-and-paste the errors they see than for the admin to try to dig them out of a log and correlate them with a particular session. 3. Users of the ssh transport typically already see these stderr messages, as the remote's stderr is copied literally by ssh. This brings other transports (http, and push-over-git if you are crazy enough to enable it) more in line with ssh. As a bonus for ssh users, because the messages are now fed through the sideband and printed by the local git, they will have "remote:" prepended and be properly interleaved with any local output to stderr. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f8d2f1710..2062b141d 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -801,7 +801,7 @@ static const char *parse_pack_header(struct pack_header *hdr) static const char *pack_lockfile; -static const char *unpack(void) +static const char *unpack(int err_fd) { struct pack_header hdr; const char *hdr_err; @@ -833,6 +833,7 @@ static const char *unpack(void) memset(&child, 0, sizeof(child)); child.argv = unpacker; child.no_stdout = 1; + child.err = err_fd; child.git_cmd = 1; code = run_command(&child); if (!code) @@ -859,6 +860,7 @@ static const char *unpack(void) memset(&ip, 0, sizeof(ip)); ip.argv = keeper; ip.out = -1; + ip.err = err_fd; ip.git_cmd = 1; status = start_command(&ip); if (status) { @@ -875,6 +877,26 @@ static const char *unpack(void) } } +static const char *unpack_with_sideband(void) +{ + struct async muxer; + const char *ret; + + if (!use_sideband) + return unpack(0); + + memset(&muxer, 0, sizeof(muxer)); + muxer.proc = copy_to_sideband; + muxer.in = -1; + if (start_async(&muxer)) + return NULL; + + ret = unpack(muxer.in); + + finish_async(&muxer); + return ret; +} + static void report(struct command *commands, const char *unpack_status) { struct command *cmd; @@ -972,7 +994,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) const char *unpack_status = NULL; if (!delete_only(commands)) - unpack_status = unpack(); + unpack_status = unpack_with_sideband(); execute_commands(commands, unpack_status); if (pack_lockfile) unlink_or_warn(pack_lockfile); -- cgit v1.2.1 From 74eb32d3a49c490b0322fb9e4100b5bb08ef7f2a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 21 Sep 2012 01:38:46 -0400 Subject: receive-pack: drop "n/a" on unpacker errors The output from git push currently looks like this: $ git push dest HEAD fatal: [some message from index-pack] error: unpack failed: index-pack abnormal exit To dest ! [remote rejected] HEAD -> master (n/a (unpacker error)) That n/a is meant to be "the per-ref status is not available" but the nested parentheses just make it look ugly. Let's turn the final line into just: ! [remote rejected] HEAD -> master (unpacker error) Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 2062b141d..165a63320 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -701,7 +701,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro if (unpacker_error) { for (cmd = commands; cmd; cmd = cmd->next) - cmd->error_string = "n/a (unpacker error)"; + cmd->error_string = "unpacker error"; return; } -- cgit v1.2.1