Re: [PATCH 3/3] pkt-line: support tracing verbatim pack contents

From: Jeff King <peff@peff.net>
Date: Jun 16, 2015, 17:10 UTC
Message-ID: <20150616171005.GB18667@peff.net>
In-Reply-To: <CAHcr6HYvVR4uTmtegWHK0h+v_aVs4JVLsSwvjthGY3pb=-Q0yQ@mail.gmail.com>

On Tue, Jun 16, 2015 at 11:38:41AM -0400, Augie Fackler wrote:

Show 7 quoted lines

> > We already have a GIT_TRACE_PACKET mechanism for tracing
> > packets. Let's extend it with GIT_TRACE_PACK to dump the
> > verbatim packfile.
> 
> FWIW, this also works for me - I have no preference between my patches
> and Jeff's. I suspect yours are much better given that you have a clue
> about git internals ;).

I like mine better than yours, if only because it hooks into the existing tracing mechanism. But I am sad that neither of our proposals works for tracing pushed packs (something that is useful for the opposite situation: you have a sane git server, and some unknown misbehaving _client_ is sending you bogus packs).

Here's a rough cut at the "trace stdin" idea I mentioned earlier (which is essentially an internal "tee"). You can collect the incoming pack like:

  GIT_TRACE=1 \
  GIT_TRACE_STDIN=/tmp/foo.pack \
  GIT_TRACE_STDIN_FOR=index-pack \
  git fetch ...

The "STDIN_FOR" hack is there because otherwise you get the stdin for multiple processes intermingled. I think a cleaner way to do it would be to allow something like:

  GIT_TRACE_STDIN=/tmp/stdin.%p

and replace "%p" with the PID of the tracing process.

I like this approach because it works everywhere (for pushed packs, but also for any other git commands you may want to debug). The downside versus the other patches is that to replay the index-pack command, you need to collect both its stdin and its arguments (which you can get from the GIT_TRACE output). On the other hand, it gives you a more realistic replay of what happened (e.g., if there is a bug triggered by the --pack-header code).

Here is the patch:

---

diff --git a/git.c b/git.c
index 44374b1..d58866e 100644
--- a/git.c
+++ b/git.c
@@ -616,6 +616,80 @@ static void restore_sigpipe_to_default(void)
 	signal(SIGPIPE, SIG_DFL);
 }
 
+static int copy_stdin(int in, int out, void *data)
+{
+	struct trace_key *key = data;
+	while (1) {
+		char buf[8192];
+		ssize_t len = xread(in, buf, sizeof(buf));
+		if (!len)
+			break;
+		if (len < 0) {
+			warning("error reading stdin trace: %s",
+				strerror(errno));
+			break;
+		}
+
+		trace_verbatim(key, buf, len);
+		if (write_in_full(out, buf, len) < 0) {
+			warning("error writing stdin trace: %s",
+				strerror(errno));
+			break;
+		}
+	}
+	close(in);
+	close(out);
+	return 0;
+}
+
+static int trace_stdin_for(const char *cmd)
+{
+	int ret = 1;
+	const char *x = getenv("GIT_TRACE_STDIN_FOR");
+
+	if (x) {
+		struct string_list items = STRING_LIST_INIT_DUP;
+		string_list_split(&items, x, ':', -1);
+		ret = unsorted_string_list_has_string(&items, cmd);
+		string_list_clear(&items, 0);
+	}
+
+	return ret;
+}
+
+static void trace_stdin(const char *cmd)
+{
+	static struct trace_key key = TRACE_KEY_INIT(STDIN);
+	static struct async async;
+
+	if (!trace_stdin_for(cmd) || !trace_want(&key))
+		return;
+
+	memset(&async, 0, sizeof(async));
+	async.proc = copy_stdin;
+	async.data = &key;
+	async.in = dup(0);
+	async.out = -1;
+
+	if (async.in < 0 || start_async(&async) < 0) {
+		warning("unable to trace stdin: %s", strerror(errno));
+		return ;
+	}
+
+	/*
+	 * At this point we've handed stdin off to the async process,
+	 * so there we are past the point of no return.
+	 */
+	if (dup2(async.out, 0))
+		die_errno("unable to redirect stdin from async process");
+	close(async.out);
+
+	/*
+	 * leak async; we would know to finish_async() only when we are
+	 * exiting, and there is no point then
+	 */
+}
+
 int main(int argc, char **av)
 {
 	const char **argv = (const char **) av;
@@ -674,6 +748,8 @@ int main(int argc, char **av)
 	}
 	cmd = argv[0];
 
+	trace_stdin(cmd);
+
 	/*
 	 * We use PATH to find git commands, but we prepend some higher
 	 * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH