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

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

From
AFAugie Fackler <augie@google.com>
Date
Jun 16, 2015, 15:38 UTC
Message-ID
<CAHcr6HYvVR4uTmtegWHK0h+v_aVs4JVLsSwvjthGY3pb=-Q0yQ@mail.gmail.com>
In-Reply-To
<20150612212827.GC25757@peff.net>
On Fri, Jun 12, 2015 at 5:28 PM, Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> When debugging the pack protocol, it is sometimes useful to
> store the verbatim pack that we sent or received on the
> wire. Looking at the on-disk result is often not helpful for
> a few reasons:
>
>   1. If the operation is a clone, we destroy the repo on
>      failure, leaving nothing on disk.
>
>   2. If the pack is small, we unpack it immediately, and the
>      full pack never hits the disk.
>
>   3. If we feed the pack to "index-pack --fix-thin", the
>      resulting pack has the extra delta bases added to it.
>
> 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 ;).

One bit of feedback is that it might be worth mentioning (though I don't feel strongly) that GIT_TRACE_PACK works with or without GIT_TRACE_PACKET - that wasn't immediately obvious to me, but it makes sense once I read the code.

Thanks!
Show 183 quoted lines
>
> There are a few other positive fallouts that come from
> rearranging this code:
>
>  - We currently disable the packet trace after seeing the
>    PACK header, even though we may get human-readable lines
>    on other sidebands; now we include them in the trace.
>
>  - We currently try to print "PACK ..." in the trace to
>    indicate that the packfile has started. But because we
>    disable packet tracing, we never printed this line. We
>    will now do so.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git.txt | 13 +++++++++++-
>  pkt-line.c            | 59 ++++++++++++++++++++++++++++++++++++++-------------
>  t/t5601-clone.sh      |  7 ++++++
>  trace.c               |  7 ++++++
>  trace.h               |  1 +
>  5 files changed, 71 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 45b64a7..8c44d14 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1000,9 +1000,20 @@ Unsetting the variable, or setting it to empty, "0" or
>         Enables trace messages for all packets coming in or out of a
>         given program. This can help with debugging object negotiation
>         or other protocol issues. Tracing is turned off at a packet
> -       starting with "PACK".
> +       starting with "PACK" (but see 'GIT_TRACE_PACK' below).
>         See 'GIT_TRACE' for available trace output options.
>
> +'GIT_TRACE_PACK'::
> +       Enables tracing of packfiles sent or received by a
> +       given program. Unlike other trace output, this trace is
> +       verbatim: no headers, and no quoting of binary data. You almost
> +       certainly want to direct into a file (e.g.,
> +       `GIT_TRACE_PACK=/tmp/my.pack`) rather than displaying it on the
> +       terminal or mixing it with other trace output.
> ++
> +Note that this is currently only implemented for the client side
> +of clones and fetches.
> +
>  'GIT_TRACE_PERFORMANCE'::
>         Enables performance related trace messages, e.g. total execution
>         time of each Git command.
> diff --git a/pkt-line.c b/pkt-line.c
> index e75af02..2e122c0 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -4,16 +4,51 @@
>  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) && !trace_want(&trace_pack))
> +               return;
> +
> +       if (in_pack) {
> +               if (packet_trace_pack(buf, len, sideband))
> +                       return;
> +       } else if (starts_with(buf, "PACK") || starts_with(buf, "\1PACK")) {
> +               in_pack = 1;
> +               sideband = *buf == '\1';
> +               packet_trace_pack(buf, len, sideband);
> +
> +               /*
> +                * Make a note in the human-readable trace that the pack data
> +                * started.
> +                */
> +               buf = "PACK ...";
> +               len = strlen(buf);
> +       }
>
>         if (!trace_want(&trace_packet))
>                 return;
> @@ -24,21 +59,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)
>         strbuf_addf(&out, "packet: %12s%c ",
>                     packet_trace_prefix, write ? '>' : '<');
>
> -       if (starts_with(buf, "PACK") || starts_with(buf, "\1PACK")) {
> -               strbuf_addstr(&out, "PACK ...");
> -               trace_disable(&trace_packet);
> -       }
> -       else {
> -               /* XXX we should really handle printable utf8 */
> -               for (i = 0; i < len; i++) {
> -                       /* suppress newlines */
> -                       if (buf[i] == '\n')
> -                               continue;
> -                       if (buf[i] >= 0x20 && buf[i] <= 0x7e)
> -                               strbuf_addch(&out, buf[i]);
> -                       else
> -                               strbuf_addf(&out, "\\%o", buf[i]);
> -               }
> +       /* XXX we should really handle printable utf8 */
> +       for (i = 0; i < len; i++) {
> +               /* suppress newlines */
> +               if (buf[i] == '\n')
> +                       continue;
> +               if (buf[i] >= 0x20 && buf[i] <= 0x7e)
> +                       strbuf_addch(&out, buf[i]);
> +               else
> +                       strbuf_addf(&out, "\\%o", buf[i]);
>         }
>
>         strbuf_addch(&out, '\n');
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index bfdaf75..795ece0 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -496,4 +496,11 @@ test_expect_success 'shallow clone locally' '
>         ( cd ddsstt && git fsck )
>  '
>
> +test_expect_success 'GIT_TRACE_PACK produces a usable pack' '
> +       rm -rf dst.git &&
> +       GIT_TRACE_PACK=$PWD/tmp.pack git clone --no-local --bare src dst.git &&
> +       git init --bare replay.git &&
> +       git -C replay.git index-pack -v --stdin <tmp.pack
> +'
> +
>  test_done
> 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
>
> --
> 2.4.2.752.geeb594a
Previous: Jeff KingNext: Junio C Hamano
Message 14 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.