git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] fetch-pack: optionally save packs to disk

From
Jeff King <peff@peff.net>
Date
Jun 12, 2015, 18:00 UTC
Message-ID
<20150612180005.GA9242@peff.net>
In-Reply-To
<xmqqmw05dkxz.fsf@gitster.dls.corp.google.com>
On Fri, Jun 12, 2015 at 08:07:36AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > What is the problem with the current fetch-pack implementation? Does
> > it remove a bogus packfile after download? Does it abort during
> > download when it detects a broken packfile? Does --keep not do what
> > you need?
> 
> Doesn't the incoming data still go through the fattening process,
> though?  You will not be able to inspect the byte-for-byte identical
> stream that came out of the server end whose packfile generation
> logic is suspect.
> 
> For the purpose of debugging your own new server implementation, it
> might be a better approach to capture the pack as it comes out at
> the server end, instead of doing it at the fetch-pack end as it
> comes in. But the approach to add this "dump" at the fetch-pack side
> is that it gives us a tool to diagnose problems that come from
> broken server (re)implementations by other people we cannot debug,
> i.e. "you are spewing this corrupt pack against this request; here
> is a dump we took to help you go fix your server".

When I added GIT_TRACE_PACKET long ago, I had always intended to follow-up with a GIT_TRACE_PACKFILE. The former stops tracing when we get to the binary data, but I had intended the latter to store the pure on-the-wire packfile transmission for later debugging to. I never got around to it, but I think it is something like the patch below.

With:
  GIT_TRACE_PACKET=1 GIT_TRACE_PACK=/tmp/foo.pack git clone ...

this yields a usable pack in /tmp/foo.pack (note that it only kicks in when packet-tracing is on at all; if we restructure the code a bit, we can remove that limitation).

In theory it would also work when receiving a pack via push, but I think we actually skip the pkt-line protocol there. We'd have to manually check GIT_TRACE_PACK.

Also, as a bonus, it means we do not stop tracing completely when we start to receive a sideband pack. The current GIT_TRACE_PACKET code misses any sideband-2 messages that come after we start receiving the pack.

diff --git a/pkt-line.c b/pkt-line.c
index 187a229..f82871a 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -4,20 +4,39 @@
 char packet_buffer[LARGE_PACKET_MAX];
 static const char *packet_trace_prefix = "git";
 static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
+static struct trace_key trace_pack = TRACE_KEY_INIT(PACK);
 
 void packet_trace_identity(const char *prog)
 {
 	packet_trace_prefix = xstrdup(prog);
 }
 
+static int packet_trace_pack(const char *buf, unsigned int len, int sideband)
+{
+	if (!sideband) {
+		trace_verbatim(&trace_pack, buf, len);
+		return 1;
+	} else if (len && *buf == '\1') {
+		trace_verbatim(&trace_pack, buf + 1, len - 1);
+		return 1;
+	} else {
+		/* it's another non-pack sideband */
+		return 0;
+	}
+}
+
 static void packet_trace(const char *buf, unsigned int len, int write)
 {
 	int i;
 	struct strbuf out;
+	static int in_pack, sideband;
 
 	if (!trace_want(&trace_packet))
 		return;
 
+	if (in_pack && packet_trace_pack(buf, len, sideband))
+		return;
+
 	/* +32 is just a guess for header + quoting */
 	strbuf_init(&out, len+32);
 
@@ -27,7 +46,9 @@ static void packet_trace(const char *buf, unsigned int len, int write)
 	if ((len >= 4 && starts_with(buf, "PACK")) ||
 	    (len >= 5 && starts_with(buf+1, "PACK"))) {
 		strbuf_addstr(&out, "PACK ...");
-		trace_disable(&trace_packet);
+		in_pack = 1;
+		sideband = *buf == '\1';
+		packet_trace_pack(buf, len, sideband);
 	}
 	else {
 		/* XXX we should really handle printable utf8 */
diff --git a/trace.c b/trace.c
index 3c3bd8f..7393926 100644
--- a/trace.c
+++ b/trace.c
@@ -120,6 +120,13 @@ static int prepare_trace_line(const char *file, int line,
 	return 1;
 }
 
+void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
+{
+	if (!trace_want(key))
+		return;
+	write_or_whine_pipe(get_trace_fd(key), buf, len, err_msg);
+}
+
 static void print_trace_line(struct trace_key *key, struct strbuf *buf)
 {
 	strbuf_complete_line(buf);
diff --git a/trace.h b/trace.h
index ae6a332..179b249 100644
--- a/trace.h
+++ b/trace.h
@@ -18,6 +18,7 @@ extern int trace_want(struct trace_key *key);
 extern void trace_disable(struct trace_key *key);
 extern uint64_t getnanotime(void);
 extern void trace_command_performance(const char **argv);
+extern void trace_verbatim(struct trace_key *key, const void *buf, unsigned len);
 
 #ifndef HAVE_VARIADIC_MACROS
Previous: Augie FacklerNext: Jeff King
Message 5 of 33 in “fetch-pack: optionally save packs to disk”
  1. fetch-pack: optionally save packs to diskAugie Fackler, Jun 11, 2015
  2. Johannes SixtJun 12, 2015
  3. Junio C HamanoJun 12, 2015
  4. Augie FacklerJun 12, 2015
  5. Jeff KingJun 12, 2015
  6. Jeff KingJun 12, 2015
  7. 1/3 pkt-line: simplify starts_with checks in packet tracingJeff King, Jun 12, 2015
  8. Stefan BellerJun 12, 2015
  9. 2/3 pkt-line: tighten sideband PACK check when tracingJeff King, Jun 12, 2015
  10. Stefan BellerJun 12, 2015
  11. Jeff KingJun 12, 2015
  12. Stefan BellerJun 12, 2015
  13. 3/3 pkt-line: support tracing verbatim pack contentsJeff King, Jun 12, 2015
  14. Augie FacklerJun 16, 2015
  15. Junio C HamanoJun 16, 2015
  16. Jeff KingJun 16, 2015
  17. Augie FacklerJun 16, 2015
  18. Jeff KingJun 16, 2015
  19. Jeff KingJun 16, 2015
  20. Augie FacklerJun 16, 2015
  21. Jeff KingJun 16, 2015
  22. Augie FacklerJun 16, 2015
  23. 0/3 add GIT_TRACE_STDINJeff King, Jun 16, 2015
  24. 1/3 trace: implement %p placeholder for filenamesJeff King, Jun 16, 2015
  25. 2/3 trace: add pid to each output lineJeff King, Jun 16, 2015
  26. 3/3 trace: add GIT_TRACE_STDINJeff King, Jun 16, 2015
  27. Jeff KingJun 16, 2015
  28. Jeff KingJun 16, 2015
  29. Duy NguyenJun 17, 2015
  30. Jeff KingJun 17, 2015
  31. Duy NguyenJun 18, 2015
  32. Junio C HamanoJun 26, 2015
  33. Augie FacklerJun 12, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.