{"thread":{"id":"26592","subject":"[PATCH/RFC 0/8] refactor trace code","startedAt":"2011-02-24T14:23:09Z","lastAt":"2011-03-08T20:59:19Z","messageCount":32,"participants":["Jeff King","Christian Couder","Andreas Ericsson","Erik Faye-Lund","Junio C Hamano","Jonathan Nieder","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"162141","messageId":"20110224142308.GA15356@sigill.intra.peff.net","threadId":"26592","inReplyTo":null,"subject":"[PATCH/RFC 0/8] refactor trace code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:23:09Z","receivedAt":"2011-02-24T14:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few weeks ago in:\n\n  http://article.gmane.org/gmane.comp.version-control.git/165496\n\nwe discussed the possibility of removing the repo setup trace lines. I\nmentioned that I had also recently done some debug code that would not\nbe appropriate to show all the time with GIT_TRACE. So here is a series\nthat jumbles it all together: refactors the trace code to handle\nmultiple environment variables, adds packet-tracing code on its own\nvariable, and puts the repo setup traces on their own variable.\n\nWith this, you can do:\n\n  GIT_TRACE=1 \\\n  GIT_TRACE_PACKET=/tmp/packet.dump \\\n  GIT_TRACE_SETUP=0 \\\n  git whatever\n\nThere may be a few other places where it is worth either pushing some\ntraces to their variable (I think the notes merge code has some\ndebugging traces in it), or places where existing debugging code could\nbe enabled (e.g., there is some diffcore debugging code that I sometimes\nturn on; it is not even compiled by default, but this would give a nice\nway of pushing that decision to runtime).\n\nI have mixed feelings on adding a bunch of debugging code. On the one\nhand, it's mostly dead code that nobody runs. On the other hand, it is\nsometimes really helpful to be able to tell a user \"run with this\nenvironment variable and tell us what it says\" without having to get\nthem to apply a custom patch.\n\n  [1/8]: compat: provide a fallback va_copy definition\n  [2/8]: strbuf: add strbuf_addv\n  [3/8]: trace: add trace_vprintf\n  [4/8]: trace: refactor to support multiple env variables\n  [5/8]: trace: factor out \"do we want to trace\" logic\n  [6/8]: trace: add trace_strbuf\n  [7/8]: add packet tracing debug code\n  [8/8]: trace: give repo_setup trace its own key\n\n-Peff\n"},{"id":"162142","messageId":"20110224142647.GA15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[RFC/PATCH 1/8] compat: provide a fallback va_copy definition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:26:47Z","receivedAt":"2011-02-24T14:26:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"va_copy is C99. Prior to this, the usual procedure was to\nsimply copy the va_list by assignment.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe have avoided using va_copy many times in the past, which has led to a\nbunch of cut-and-paste. From everything I found searching the web,\nimplementations have historically either provided va_copy or just let\nyour code assume that simple assignment of worked. I couldn't find any\nmention of any other alternatives.\n\nSo my guess is that this will be sufficient, but I we won't really know\nfor sure until somebody reports a problem. :(\n\n git-compat-util.h |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9c23622..00d41e4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -535,6 +535,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define fstat_is_reliable() 1\n #endif\n \n+#ifndef va_copy\n+#define va_copy(dst,src) (dst) = (src)\n+#endif\n+\n /*\n  * Preserves errno, prints a message, but gives no warning for ENOENT.\n  * Always returns the return value of unlink(2).\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162143","messageId":"20110224142711.GB15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 2/8] strbuf: add strbuf_addv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:27:11Z","receivedAt":"2011-02-24T14:27:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a variable-args function, the code for writing into a\nstrbuf is non-trivial. We ended up cutting and pasting it in\nseveral places because there was no vprintf-style function\nfor strbufs (which in turn was held up by a lack of\nva_copy).\n\nNow that we have a fallback va_copy, we can add strbuf_addv,\nthe strbuf equivalent of vsprintf. And we can clean up the\ncut and paste mess.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsck.c            |   14 +-------------\n merge-recursive.c |   15 +--------------\n strbuf.c          |   25 +++++++++++++++----------\n strbuf.h          |    1 +\n trace.c           |   32 ++++++--------------------------\n 5 files changed, 24 insertions(+), 63 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 3d05d4a..398b137 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -347,26 +347,14 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n int fsck_error_function(struct object *obj, int type, const char *fmt, ...)\n {\n \tva_list ap;\n-\tint len;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstrbuf_addf(&sb, \"object %s:\", obj->sha1?sha1_to_hex(obj->sha1):\"(null)\");\n \n \tva_start(ap, fmt);\n-\tlen = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);\n+\tstrbuf_addv(&sb, fmt, ap);\n \tva_end(ap);\n \n-\tif (len < 0)\n-\t\tlen = 0;\n-\tif (len >= strbuf_avail(&sb)) {\n-\t\tstrbuf_grow(&sb, len + 2);\n-\t\tva_start(ap, fmt);\n-\t\tlen = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);\n-\t\tva_end(ap);\n-\t\tif (len >= strbuf_avail(&sb))\n-\t\t\tdie(\"this should not happen, your snprintf is broken\");\n-\t}\n-\n \terror(\"%s\", sb.buf);\n \tstrbuf_release(&sb);\n \treturn 1;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 16c2dbe..b418d5b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -137,7 +137,6 @@ static void flush_output(struct merge_options *o)\n __attribute__((format (printf, 3, 4)))\n static void output(struct merge_options *o, int v, const char *fmt, ...)\n {\n-\tint len;\n \tva_list ap;\n \n \tif (!show(o, v))\n@@ -148,21 +147,9 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)\n \tstrbuf_setlen(&o->obuf, o->obuf.len + o->call_depth * 2);\n \n \tva_start(ap, fmt);\n-\tlen = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);\n+\tstrbuf_addv(&o->obuf, fmt, ap);\n \tva_end(ap);\n \n-\tif (len < 0)\n-\t\tlen = 0;\n-\tif (len >= strbuf_avail(&o->obuf)) {\n-\t\tstrbuf_grow(&o->obuf, len + 2);\n-\t\tva_start(ap, fmt);\n-\t\tlen = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);\n-\t\tva_end(ap);\n-\t\tif (len >= strbuf_avail(&o->obuf)) {\n-\t\t\tdie(\"this should not happen, your snprintf is broken\");\n-\t\t}\n-\t}\n-\tstrbuf_setlen(&o->obuf, o->obuf.len + len);\n \tstrbuf_add(&o->obuf, \"\\n\", 1);\n \tif (!o->buffer_output)\n \t\tflush_output(o);\ndiff --git a/strbuf.c b/strbuf.c\nindex 07e8883..7a2e17f 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -195,24 +195,29 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)\n \n void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n {\n-\tint len;\n \tva_list ap;\n+\tva_start(ap, fmt);\n+\tstrbuf_addv(sb, fmt, ap);\n+\tva_end(ap);\n+}\n+\n+void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)\n+{\n+\tint len;\n+\tva_list cp;\n \n \tif (!strbuf_avail(sb))\n \t\tstrbuf_grow(sb, 64);\n-\tva_start(ap, fmt);\n-\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n-\tva_end(ap);\n+\tva_copy(cp, ap);\n+\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);\n+\tva_end(cp);\n \tif (len < 0)\n-\t\tdie(\"your vsnprintf is broken\");\n+\t\tdie(\"BUG: your vsnprintf is broken (returned -1)\");\n \tif (len > strbuf_avail(sb)) {\n \t\tstrbuf_grow(sb, len);\n-\t\tva_start(ap, fmt);\n \t\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n-\t\tva_end(ap);\n-\t\tif (len > strbuf_avail(sb)) {\n-\t\t\tdie(\"this should not happen, your snprintf is broken\");\n-\t\t}\n+\t\tif (len > strbuf_avail(sb))\n+\t\t\tdie(\"BUG: your vsnprintf is broken (insatiable)\");\n \t}\n \tstrbuf_setlen(sb, sb->len + len);\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex 675a91f..65b44a0 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -120,6 +120,7 @@ extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *\n \n __attribute__((format (printf,2,3)))\n extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n /* XXX: if read fails, any partial read is undone */\ndiff --git a/trace.c b/trace.c\nindex 35d388d..745543c 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -64,28 +64,18 @@ static const char err_msg[] = \"Could not trace into fd given by \"\n \n void trace_printf(const char *fmt, ...)\n {\n-\tstruct strbuf buf;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tva_list ap;\n-\tint fd, len, need_close = 0;\n+\tint fd, need_close = 0;\n \n \tfd = get_trace_fd(&need_close);\n \tif (!fd)\n \t\treturn;\n \n \tset_try_to_free_routine(NULL);\t/* is never reset */\n-\tstrbuf_init(&buf, 64);\n \tva_start(ap, fmt);\n-\tlen = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);\n+\tstrbuf_addv(&buf, fmt, ap);\n \tva_end(ap);\n-\tif (len >= strbuf_avail(&buf)) {\n-\t\tstrbuf_grow(&buf, len - strbuf_avail(&buf) + 128);\n-\t\tva_start(ap, fmt);\n-\t\tlen = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);\n-\t\tva_end(ap);\n-\t\tif (len >= strbuf_avail(&buf))\n-\t\t\tdie(\"broken vsnprintf\");\n-\t}\n-\tstrbuf_setlen(&buf, len);\n \n \twrite_or_whine_pipe(fd, buf.buf, buf.len, err_msg);\n \tstrbuf_release(&buf);\n@@ -96,28 +86,18 @@ void trace_printf(const char *fmt, ...)\n \n void trace_argv_printf(const char **argv, const char *fmt, ...)\n {\n-\tstruct strbuf buf;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tva_list ap;\n-\tint fd, len, need_close = 0;\n+\tint fd, need_close = 0;\n \n \tfd = get_trace_fd(&need_close);\n \tif (!fd)\n \t\treturn;\n \n \tset_try_to_free_routine(NULL);\t/* is never reset */\n-\tstrbuf_init(&buf, 64);\n \tva_start(ap, fmt);\n-\tlen = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);\n+\tstrbuf_addv(&buf, fmt, ap);\n \tva_end(ap);\n-\tif (len >= strbuf_avail(&buf)) {\n-\t\tstrbuf_grow(&buf, len - strbuf_avail(&buf) + 128);\n-\t\tva_start(ap, fmt);\n-\t\tlen = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);\n-\t\tva_end(ap);\n-\t\tif (len >= strbuf_avail(&buf))\n-\t\t\tdie(\"broken vsnprintf\");\n-\t}\n-\tstrbuf_setlen(&buf, len);\n \n \tsq_quote_argv(&buf, argv, 0);\n \tstrbuf_addch(&buf, '\\n');\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162144","messageId":"20110224142814.GC15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 3/8] trace: add trace_vprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:28:15Z","receivedAt":"2011-02-24T14:28:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a necessary cleanup to adding new types of trace\nfunctions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |    1 +\n trace.c |   14 +++++++++-----\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4a758ba..08b23b2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1067,6 +1067,7 @@ extern void alloc_report(void);\n /* trace.c */\n __attribute__((format (printf, 1, 2)))\n extern void trace_printf(const char *format, ...);\n+extern void trace_vprintf(const char *format, va_list ap);\n __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n extern void trace_repo_setup(const char *prefix);\ndiff --git a/trace.c b/trace.c\nindex 745543c..6ad7788 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -62,10 +62,9 @@ static int get_trace_fd(int *need_close)\n static const char err_msg[] = \"Could not trace into fd given by \"\n \t\"GIT_TRACE environment variable\";\n \n-void trace_printf(const char *fmt, ...)\n+void trace_vprintf(const char *fmt, va_list ap)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tva_list ap;\n \tint fd, need_close = 0;\n \n \tfd = get_trace_fd(&need_close);\n@@ -73,10 +72,7 @@ void trace_printf(const char *fmt, ...)\n \t\treturn;\n \n \tset_try_to_free_routine(NULL);\t/* is never reset */\n-\tva_start(ap, fmt);\n \tstrbuf_addv(&buf, fmt, ap);\n-\tva_end(ap);\n-\n \twrite_or_whine_pipe(fd, buf.buf, buf.len, err_msg);\n \tstrbuf_release(&buf);\n \n@@ -84,6 +80,14 @@ void trace_printf(const char *fmt, ...)\n \t\tclose(fd);\n }\n \n+void trace_printf(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\ttrace_vprintf(fmt, ap);\n+\tva_end(ap);\n+}\n+\n void trace_argv_printf(const char **argv, const char *fmt, ...)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162145","messageId":"20110224142841.GD15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:28:41Z","receivedAt":"2011-02-24T14:28:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now you turn all tracing off and on with GIT_TRACE. To\nsupport new types of tracing without forcing the user to see\nall of them, we will soon support turning each tracing area\non with GIT_TRACE_*.\n\nThis patch lays the groundwork by providing an interface\nwhich does not assume GIT_TRACE. However, we still maintain\nthe trace_printf interface so that existing callers do not\nneed to be refactored.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |    2 +-\n trace.c |   28 ++++++++++++++++++----------\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 08b23b2..2ab1bf9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1067,7 +1067,7 @@ extern void alloc_report(void);\n /* trace.c */\n __attribute__((format (printf, 1, 2)))\n extern void trace_printf(const char *format, ...);\n-extern void trace_vprintf(const char *format, va_list ap);\n+extern void trace_vprintf(const char *key, const char *format, va_list ap);\n __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n extern void trace_repo_setup(const char *prefix);\ndiff --git a/trace.c b/trace.c\nindex 6ad7788..a74f00e 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -25,10 +25,10 @@\n #include \"cache.h\"\n #include \"quote.h\"\n \n-/* Get a trace file descriptor from GIT_TRACE env variable. */\n-static int get_trace_fd(int *need_close)\n+/* Get a trace file descriptor from \"key\" env variable. */\n+static int get_trace_fd(const char *key, int *need_close)\n {\n-\tchar *trace = getenv(\"GIT_TRACE\");\n+\tchar *trace = getenv(key);\n \n \tif (!trace || !strcmp(trace, \"\") ||\n \t    !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n@@ -50,10 +50,10 @@ static int get_trace_fd(int *need_close)\n \t\treturn fd;\n \t}\n \n-\tfprintf(stderr, \"What does '%s' for GIT_TRACE mean?\\n\", trace);\n+\tfprintf(stderr, \"What does '%s' for %s mean?\\n\", trace, key);\n \tfprintf(stderr, \"If you want to trace into a file, \"\n-\t\t\"then please set GIT_TRACE to an absolute pathname \"\n-\t\t\"(starting with /).\\n\");\n+\t\t\"then please set %s to an absolute pathname \"\n+\t\t\"(starting with /).\\n\", key);\n \tfprintf(stderr, \"Defaulting to tracing on stderr...\\n\");\n \n \treturn STDERR_FILENO;\n@@ -62,12 +62,12 @@ static int get_trace_fd(int *need_close)\n static const char err_msg[] = \"Could not trace into fd given by \"\n \t\"GIT_TRACE environment variable\";\n \n-void trace_vprintf(const char *fmt, va_list ap)\n+void trace_vprintf(const char *key, const char *fmt, va_list ap)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd, need_close = 0;\n \n-\tfd = get_trace_fd(&need_close);\n+\tfd = get_trace_fd(key, &need_close);\n \tif (!fd)\n \t\treturn;\n \n@@ -80,11 +80,19 @@ void trace_vprintf(const char *fmt, va_list ap)\n \t\tclose(fd);\n }\n \n+void trace_printf_key(const char *key, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\ttrace_vprintf(key, fmt, ap);\n+\tva_end(ap);\n+}\n+\n void trace_printf(const char *fmt, ...)\n {\n \tva_list ap;\n \tva_start(ap, fmt);\n-\ttrace_vprintf(fmt, ap);\n+\ttrace_vprintf(\"GIT_TRACE\", fmt, ap);\n \tva_end(ap);\n }\n \n@@ -94,7 +102,7 @@ void trace_argv_printf(const char **argv, const char *fmt, ...)\n \tva_list ap;\n \tint fd, need_close = 0;\n \n-\tfd = get_trace_fd(&need_close);\n+\tfd = get_trace_fd(\"GIT_TRACE\", &need_close);\n \tif (!fd)\n \t\treturn;\n \n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162146","messageId":"20110224142859.GE15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 5/8] trace: factor out \"do we want to trace\" logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:28:59Z","receivedAt":"2011-02-24T14:28:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As we add more tracing areas, this will avoid repeated code.\n\nTechnically, trace_printf already checks this and will avoid\nprinting if the trace key is not set. However, callers may\nwant to find out early whether or not tracing is enabled so\nthey can avoid doing work in the common non-trace case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |    1 +\n trace.c |   14 +++++++++++---\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 2ab1bf9..211d7bb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1071,6 +1071,7 @@ extern void trace_vprintf(const char *key, const char *format, va_list ap);\n __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n extern void trace_repo_setup(const char *prefix);\n+extern int trace_want(const char *key);\n \n /* convert.c */\n /* returns 1 if *dst was used */\ndiff --git a/trace.c b/trace.c\nindex a74f00e..f00d98f 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -148,10 +148,8 @@ void trace_repo_setup(const char *prefix)\n {\n \tconst char *git_work_tree;\n \tchar cwd[PATH_MAX];\n-\tchar *trace = getenv(\"GIT_TRACE\");\n \n-\tif (!trace || !strcmp(trace, \"\") ||\n-\t    !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n+\tif (!trace_want(\"GIT_TRACE\"))\n \t\treturn;\n \n \tif (!getcwd(cwd, PATH_MAX))\n@@ -168,3 +166,13 @@ void trace_repo_setup(const char *prefix)\n \ttrace_printf(\"setup: cwd: %s\\n\", quote_crnl(cwd));\n \ttrace_printf(\"setup: prefix: %s\\n\", quote_crnl(prefix));\n }\n+\n+int trace_want(const char *key)\n+{\n+\tconst char *trace = getenv(key);\n+\n+\tif (!trace || !strcmp(trace, \"\") ||\n+\t    !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n+\t\treturn 0;\n+\treturn 1;\n+}\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162147","messageId":"20110224142950.GF15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 6/8] trace: add trace_strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:29:50Z","receivedAt":"2011-02-24T14:29:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you happen to have a strbuf, it is a little more readable\nand a little more efficient to be able to print it directly\ninstead of jamming it through the trace_printf interface.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |    1 +\n trace.c |   23 ++++++++++++++++-------\n 2 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 211d7bb..3978112 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1072,6 +1072,7 @@ __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n extern void trace_repo_setup(const char *prefix);\n extern int trace_want(const char *key);\n+extern void trace_strbuf(const char *key, const struct strbuf *buf);\n \n /* convert.c */\n /* returns 1 if *dst was used */\ndiff --git a/trace.c b/trace.c\nindex f00d98f..3f9f287 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -65,19 +65,14 @@ static const char err_msg[] = \"Could not trace into fd given by \"\n void trace_vprintf(const char *key, const char *fmt, va_list ap)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint fd, need_close = 0;\n \n-\tfd = get_trace_fd(key, &need_close);\n-\tif (!fd)\n+\tif (!trace_want(key))\n \t\treturn;\n \n \tset_try_to_free_routine(NULL);\t/* is never reset */\n \tstrbuf_addv(&buf, fmt, ap);\n-\twrite_or_whine_pipe(fd, buf.buf, buf.len, err_msg);\n+\ttrace_strbuf(key, &buf);\n \tstrbuf_release(&buf);\n-\n-\tif (need_close)\n-\t\tclose(fd);\n }\n \n void trace_printf_key(const char *key, const char *fmt, ...)\n@@ -96,6 +91,20 @@ void trace_printf(const char *fmt, ...)\n \tva_end(ap);\n }\n \n+void trace_strbuf(const char *key, const struct strbuf *buf)\n+{\n+\tint fd, need_close = 0;\n+\n+\tfd = get_trace_fd(key, &need_close);\n+\tif (!fd)\n+\t\treturn;\n+\n+\twrite_or_whine_pipe(fd, buf->buf, buf->len, err_msg);\n+\n+\tif (need_close)\n+\t\tclose(fd);\n+}\n+\n void trace_argv_printf(const char **argv, const char *fmt, ...)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162148","messageId":"20110224143019.GG15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 7/8] add packet tracing debug code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:30:19Z","receivedAt":"2011-02-24T14:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This shows a trace of all packets coming in or out of a given\nprogram. This can help with debugging object negotiation or\nother protocol issues.\n\nTo keep the code changes simple, we operate at the lowest\nlevel, meaning we don't necessarily understand what's in the\npackets. The one exception is a packet starting with \"PACK\",\nwhich causes us to skip that packet and turn off tracing\n(since the gigantic pack data will not be interesting to\nread, at least not in the trace format).\n\nWe show both written and read packets. In the local case,\nthis may mean you will see packets twice (written by the\nsender and read by the receiver). However, for cases where\nthe other end is remote, this allows you to see the full\nconversation.\n\nPacket tracing can be enabled with GIT_TRACE_PACKET=<foo>,\nwhere <foo> takes the same arguments as GIT_TRACE.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c        |    1 +\n builtin/fetch-pack.c   |    2 +\n builtin/fetch.c        |    2 +\n builtin/push.c         |    1 +\n builtin/receive-pack.c |    2 +\n cache.h                |    2 +\n pkt-line.c             |   55 +++++++++++++++++++++++++++++++++++++++++++++++-\n upload-pack.c          |    1 +\n 8 files changed, 65 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 60d9a64..38b4b71 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -383,6 +383,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tjunk_pid = getpid();\n \n+\tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t     builtin_clone_usage, 0);\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex b999413..272bc38 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -804,6 +804,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tchar **pack_lockfile_ptr = NULL;\n \tstruct child_process *conn;\n \n+\tpacket_trace_identity(\"fetch-pack\");\n+\n \tnr_heads = 0;\n \theads = NULL;\n \tfor (i = 1; i < argc; i++) {\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 357f3cd..e94e001 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -906,6 +906,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote;\n \tint result = 0;\n \n+\tpacket_trace_identity(\"fetch\");\n+\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\ndiff --git a/builtin/push.c b/builtin/push.c\nindex e655eb7..26171ff 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -228,6 +228,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tpacket_trace_identity(\"push\");\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 760817d..5fa4be8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -778,6 +778,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tchar *dir = NULL;\n \tstruct command *commands;\n \n+\tpacket_trace_identity(\"receive-pack\");\n+\n \targv++;\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = *argv++;\ndiff --git a/cache.h b/cache.h\nindex 3978112..2e59aae 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1074,6 +1074,8 @@ extern void trace_repo_setup(const char *prefix);\n extern int trace_want(const char *key);\n extern void trace_strbuf(const char *key, const struct strbuf *buf);\n \n+void packet_trace_identity(const char *prog);\n+\n /* convert.c */\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 295ba2b..cd1bd26 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -1,6 +1,51 @@\n #include \"cache.h\"\n #include \"pkt-line.h\"\n \n+const char *packet_trace_prefix = \"git\";\n+static const char trace_key[] = \"GIT_TRACE_PACKET\";\n+\n+void packet_trace_identity(const char *prog)\n+{\n+\tpacket_trace_prefix = xstrdup(prog);\n+}\n+\n+static void packet_trace(const char *buf, unsigned int len, int write)\n+{\n+\tint i;\n+\tstruct strbuf out;\n+\n+\tif (!trace_want(trace_key))\n+\t\treturn;\n+\n+\t/* +32 is just a guess for header + quoting */\n+\tstrbuf_init(&out, len+32);\n+\n+\tstrbuf_addf(&out, \"packet: %12s%c \",\n+\t\t    packet_trace_prefix, write ? '>' : '<');\n+\n+\tif ((len >= 4 && !prefixcmp(buf, \"PACK\")) ||\n+\t    (len >= 5 && !prefixcmp(buf+1, \"PACK\"))) {\n+\t\tstrbuf_addstr(&out, \"PACK ...\");\n+\t\tunsetenv(trace_key);\n+\t}\n+\telse {\n+\t\t/* XXX we should really handle printable utf8 */\n+\t\tfor (i = 0; i < len; i++) {\n+\t\t\t/* suppress newlines */\n+\t\t\tif (buf[i] == '\\n')\n+\t\t\t\tcontinue;\n+\t\t\tif (buf[i] >= 0x20 && buf[i] <= 0x7e)\n+\t\t\t\tstrbuf_addch(&out, buf[i]);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(&out, \"\\\\%o\", buf[i]);\n+\t\t}\n+\t}\n+\n+\tstrbuf_addch(&out, '\\n');\n+\ttrace_strbuf(trace_key, &out);\n+\tstrbuf_release(&out);\n+}\n+\n /*\n  * Write a packetized stream, where each line is preceded by\n  * its length (including the header) as a 4-byte hex number.\n@@ -39,11 +84,13 @@ ssize_t safe_write(int fd, const void *buf, ssize_t n)\n  */\n void packet_flush(int fd)\n {\n+\tpacket_trace(\"0000\", 4, 1);\n \tsafe_write(fd, \"0000\", 4);\n }\n \n void packet_buf_flush(struct strbuf *buf)\n {\n+\tpacket_trace(\"0000\", 4, 1);\n \tstrbuf_add(buf, \"0000\", 4);\n }\n \n@@ -62,6 +109,7 @@ static unsigned format_packet(const char *fmt, va_list args)\n \tbuffer[1] = hex(n >> 8);\n \tbuffer[2] = hex(n >> 4);\n \tbuffer[3] = hex(n);\n+\tpacket_trace(buffer+4, n-4, 1);\n \treturn n;\n }\n \n@@ -130,13 +178,16 @@ int packet_read_line(int fd, char *buffer, unsigned size)\n \tlen = packet_length(linelen);\n \tif (len < 0)\n \t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n-\tif (!len)\n+\tif (!len) {\n+\t\tpacket_trace(\"0000\", 4, 0);\n \t\treturn 0;\n+\t}\n \tlen -= 4;\n \tif (len >= size)\n \t\tdie(\"protocol error: bad line length %d\", len);\n \tsafe_read(fd, buffer, len);\n \tbuffer[len] = 0;\n+\tpacket_trace(buffer, len, 0);\n \treturn len;\n }\n \n@@ -153,6 +204,7 @@ int packet_get_line(struct strbuf *out,\n \tif (!len) {\n \t\t*src_buf += 4;\n \t\t*src_len -= 4;\n+\t\tpacket_trace(\"0000\", 4, 0);\n \t\treturn 0;\n \t}\n \tif (*src_len < len)\n@@ -165,5 +217,6 @@ int packet_get_line(struct strbuf *out,\n \tstrbuf_add(out, *src_buf, len);\n \t*src_buf += len;\n \t*src_len -= len;\n+\tpacket_trace(out->buf, out->len, 0);\n \treturn len;\n }\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b40a43f..0c87bc0 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -682,6 +682,7 @@ int main(int argc, char **argv)\n \tint i;\n \tint strict = 0;\n \n+\tpacket_trace_identity(\"upload-pack\");\n \tgit_extract_argv0_path(argv[0]);\n \tread_replace_refs = 0;\n \n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162149","messageId":"20110224143030.GH15477@sigill.intra.peff.net","threadId":"26592","inReplyTo":"20110224142308.GA15356@sigill.intra.peff.net","subject":"[PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T14:30:30Z","receivedAt":"2011-02-24T14:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You no longer get this output with GIT_TRACE=1; instead, you\ncan do GIT_TRACE_SETUP=1.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n trace.c |   11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/trace.c b/trace.c\nindex 3f9f287..262260f 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -155,10 +155,11 @@ static const char *quote_crnl(const char *path)\n /* FIXME: move prefix to startup_info struct and get rid of this arg */\n void trace_repo_setup(const char *prefix)\n {\n+\tstatic const char *key = \"GIT_TRACE_SETUP\";\n \tconst char *git_work_tree;\n \tchar cwd[PATH_MAX];\n \n-\tif (!trace_want(\"GIT_TRACE\"))\n+\tif (!trace_want(key))\n \t\treturn;\n \n \tif (!getcwd(cwd, PATH_MAX))\n@@ -170,10 +171,10 @@ void trace_repo_setup(const char *prefix)\n \tif (!prefix)\n \t\tprefix = \"(null)\";\n \n-\ttrace_printf(\"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n-\ttrace_printf(\"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n-\ttrace_printf(\"setup: cwd: %s\\n\", quote_crnl(cwd));\n-\ttrace_printf(\"setup: prefix: %s\\n\", quote_crnl(prefix));\n+\ttrace_printf_key(key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n+\ttrace_printf_key(key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n+\ttrace_printf_key(key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n+\ttrace_printf_key(key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n }\n \n int trace_want(const char *key)\n-- \n1.7.2.5.25.g3bb93\n"},{"id":"162150","messageId":"AANLkTi=cBkH5=fmH-k=Sid_4Pwzoa9A4T9z8dcMAjirL@mail.gmail.com","threadId":"26592","inReplyTo":"20110224142711.GB15477@sigill.intra.peff.net","subject":"Re: [PATCH 2/8] strbuf: add strbuf_addv","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-02-24T15:05:13Z","receivedAt":"2011-02-24T15:05:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Thu, Feb 24, 2011 at 3:27 PM, Jeff King <peff@peff.net> wrote:\n> +void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap)\n> +{\n> +       int len;\n> +       va_list cp;\n>\n>        if (!strbuf_avail(sb))\n>                strbuf_grow(sb, 64);\n> -       va_start(ap, fmt);\n> -       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n> -       va_end(ap);\n> +       va_copy(cp, ap);\n> +       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);\n> +       va_end(cp);\n>        if (len < 0)\n> -               die(\"your vsnprintf is broken\");\n> +               die(\"BUG: your vsnprintf is broken (returned -1)\");\n\nMinor nit: why not:\n\n +               die(\"BUG: your vsnprintf is broken (returned %d)\", len);\n\nThanks,\nChristian.\n"},{"id":"162152","messageId":"20110224150736.GA24346@sigill.intra.peff.net","threadId":"26592","inReplyTo":"AANLkTi=cBkH5=fmH-k=Sid_4Pwzoa9A4T9z8dcMAjirL@mail.gmail.com","subject":"Re: [PATCH 2/8] strbuf: add strbuf_addv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T15:07:36Z","receivedAt":"2011-02-24T15:07:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 24, 2011 at 04:05:13PM +0100, Christian Couder wrote:\n\n> > +       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);\n> > +       va_end(cp);\n> >        if (len < 0)\n> > -               die(\"your vsnprintf is broken\");\n> > +               die(\"BUG: your vsnprintf is broken (returned -1)\");\n> \n> Minor nit: why not:\n> \n>  +               die(\"BUG: your vsnprintf is broken (returned %d)\", len);\n\nNo reason. I'll switch to yours in my re-roll.\n\n-Peff\n"},{"id":"162153","messageId":"AANLkTinUa8kg9mZaqV2ZAz9fc==FSA+ja-5W3ydqqYOL@mail.gmail.com","threadId":"26592","inReplyTo":"20110224142859.GE15477@sigill.intra.peff.net","subject":"Re: [PATCH 5/8] trace: factor out \"do we want to trace\" logic","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-02-24T15:07:44Z","receivedAt":"2011-02-24T15:07:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 24, 2011 at 3:28 PM, Jeff King <peff@peff.net> wrote:\n\n> +int trace_want(const char *key)\n> +{\n> +       const char *trace = getenv(key);\n> +\n> +       if (!trace || !strcmp(trace, \"\") ||\n> +           !strcmp(trace, \"0\") || !strcasecmp(trace, \"false\"))\n> +               return 0;\n> +       return 1;\n> +}\n\nOther minor nit: calling it \"trace_wanted()\" or \"want_trace()\" looks\nbetter to me.\n\nThanks,\nChristian.\n"},{"id":"162164","messageId":"AANLkTin7NBx5NVRkHpgymcZfnaZaJehMMpPv=zD+cKtV@mail.gmail.com","threadId":"26592","inReplyTo":"20110224142647.GA15477@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/8] compat: provide a fallback va_copy definition","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-24T15:57:04Z","receivedAt":"2011-02-24T15:57:04Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Feb 24, 2011 at 3:26 PM, Jeff King <peff@peff.net> wrote:\n> va_copy is C99. Prior to this, the usual procedure was to\n> simply copy the va_list by assignment.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> We have avoided using va_copy many times in the past, which has led to a\n> bunch of cut-and-paste. From everything I found searching the web,\n> implementations have historically either provided va_copy or just let\n> your code assume that simple assignment of worked. I couldn't find any\n> mention of any other alternatives.\n>\n> So my guess is that this will be sufficient, but I we won't really know\n> for sure until somebody reports a problem. :(\n>\n>  git-compat-util.h |    4 ++++\n>  1 files changed, 4 insertions(+), 0 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 9c23622..00d41e4 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -535,6 +535,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n>  #define fstat_is_reliable() 1\n>  #endif\n>\n> +#ifndef va_copy\n> +#define va_copy(dst,src) (dst) = (src)\n> +#endif\n> +\n\nWouldn't it be even more portable to fall back on use __va_copy (if\npresent), as suggested by Junio in\n<7vbpip86q5.fsf@alter.siamese.dyndns.org>? He also suggested using\nmemcpy instead of assignment in the same e-mail, due to a\nrecommendation in the Autoconf manual.\n\nThere's already a va_copy fall-back in compat/msvc.h, perhaps this\nshould be removed?\n"},{"id":"162157","messageId":"4D668076.7020000@op5.se","threadId":"26592","inReplyTo":"20110224143030.GH15477@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2011-02-24T15:59:50Z","receivedAt":"2011-02-24T15:59:50Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 02/24/2011 03:30 PM, Jeff King wrote:\n> You no longer get this output with GIT_TRACE=1; instead, you\n> can do GIT_TRACE_SETUP=1.\n> \n\nIt would be beneficial if GIT_TRACE still turned on tracing globally\nso one doesn't have to know all flags to start tracing for errors.\n\nI also imagine running GIT_TRACE=1 <command> as the first step would\nbe quite useful for when one's not entirely certain where the problem\nlies and then use the specific tracing flag when trying to fix it.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"162161","messageId":"20110224160555.GB30275@sigill.intra.peff.net","threadId":"26592","inReplyTo":"4D668076.7020000@op5.se","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T16:05:55Z","receivedAt":"2011-02-24T16:05:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 24, 2011 at 04:59:50PM +0100, Andreas Ericsson wrote:\n\n> On 02/24/2011 03:30 PM, Jeff King wrote:\n> > You no longer get this output with GIT_TRACE=1; instead, you\n> > can do GIT_TRACE_SETUP=1.\n> > \n> \n> It would be beneficial if GIT_TRACE still turned on tracing globally\n> so one doesn't have to know all flags to start tracing for errors.\n> \n> I also imagine running GIT_TRACE=1 <command> as the first step would\n> be quite useful for when one's not entirely certain where the problem\n> lies and then use the specific tracing flag when trying to fix it.\n\nYeah, I considered that GIT_TRACE should become \"trace everything\" and\nthe existing traces split up into specific keys. I had a few concerns:\n\n  - Having this splitting mechanism gives us room to add debugging code\n    that is probably not interesting, but may be if you are working on a\n    specific problem, without having to worry about spamming people.\n    Which means GIT_TRACE may eventually become unreadably verbose.\n\n    Maybe GIT_TRACE should include some traces, but not others (like\n    packet debugging, or giant diff state dumps).\n\n  - I didn't want to change the meaning of GIT_TRACE too much, because\n    it is advice that everybody knows (in docs, mailing list archives,\n    etc).\n\nBut those are not strong objections. Just giving the rationale I used\nwhile writing the patches. I'd be curious what others think.\n\n-Peff\n"},{"id":"162176","messageId":"7vsjvd1e9r.fsf@alter.siamese.dyndns.org","threadId":"26592","inReplyTo":"20110224142841.GD15477@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-24T18:25:04Z","receivedAt":"2011-02-24T18:25:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Right now you turn all tracing off and on with GIT_TRACE. To\n> support new types of tracing without forcing the user to see\n> all of them, we will soon support turning each tracing area\n> on with GIT_TRACE_*.\n>\n> This patch lays the groundwork by providing an interface\n> which does not assume GIT_TRACE. However, we still maintain\n> the trace_printf interface so that existing callers do not\n> need to be refactored.\n\nOne thing I found missing in the patch description is a statement of the\noverall design and expected usage.  It appears to me that the design is\nbuilt around the idea to give each named/namable area to be traceable its\nown trace output file descriptor, so that different kinds of trace events\nare sent to different files.\n\nI however expect that majority of \"trace only areas A and B\" users would\nwant to see logs of events from these two areas in the same stream to see\nthem in the order they happened.  Perhaps you are envisioning that these\nusers would use redirection from the command line to send GIT_TRACE_A and\nGIT_TRACE_B to the same place; that probably needs to be spelled out more\nexplicitly somewhere in the documentation, as that would be a more common\nthing to do.\n\nI think your [7/8] is kind of strange when viewed in that light.  Imagine\nwhat would happen if you gave separate GIT_TRACE_* to each packet class,\ninstead of giving them a single umbrella variable GIT_TRACE_PACKET.  If\nthe user wants to see them all in a single stream, the same redirection\nyou would use to unify GIT_TRACE_A and GIT_TRACE_B can be used.\n\nInstead, you have packet class prefix in the output so that later the\ndifferent kinds of packet events can be sifted out from the unified\noutput, even though they are forced to go to the same output stream.  In a\nsense, you have two-tier classification system for traceable events (the\ntop layer that can be separated or merged at the file descriptor level,\nand the bottom layer that can only be separated by looking at the prefix).\n\nIs this necessarily a good thing (not a rhetorical question)?\n\nTo put it another and opposite way, I wonder if it would be better to\ninstead use a single output stream named by GIT_TRACE and add trace event\nclass prefix to the output for classes like SETUP and PACKET (again, not a\nrhetorical question).\n\nAlso instead of wasting environment variable names, it might be a more\ncompact design from the user's point of view if we took a list of trace\nevent classes in a single environment variable, e.g.\n\n\tGIT_TRACE_CLASS=setup,packet \\\n        GIT_TRACE=/tmp/tr \\\n        git push\n\nI dunno.\n"},{"id":"162178","messageId":"20110224190258.GA4318@sigill.intra.peff.net","threadId":"26592","inReplyTo":"7vsjvd1e9r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T19:02:58Z","receivedAt":"2011-02-24T19:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 24, 2011 at 10:25:04AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Right now you turn all tracing off and on with GIT_TRACE. To\n> > support new types of tracing without forcing the user to see\n> > all of them, we will soon support turning each tracing area\n> > on with GIT_TRACE_*.\n> >\n> > This patch lays the groundwork by providing an interface\n> > which does not assume GIT_TRACE. However, we still maintain\n> > the trace_printf interface so that existing callers do not\n> > need to be refactored.\n> \n> One thing I found missing in the patch description is a statement of the\n> overall design and expected usage.  It appears to me that the design is\n> built around the idea to give each named/namable area to be traceable its\n> own trace output file descriptor, so that different kinds of trace events\n> are sent to different files.\n\nThe intent was that you _can_ do that if you want, but you can also just\ndump them all to the same fd (or the same file). Each trace statement is\ntotally independent and written via write(), so there is no problem\npushing them all to the same place. They'll arrive in chronological\norder.\n\n> I however expect that majority of \"trace only areas A and B\" users would\n> want to see logs of events from these two areas in the same stream to see\n> them in the order they happened.\n\nRight. And you can do GIT_TRACE_A=1 GIT_TRACE_B=1 if you want that (or\nput them both to fd 5, or /tmp/foo.out, or whatever).\n\n> Perhaps you are envisioning that these users would use redirection\n> from the command line to send GIT_TRACE_A and GIT_TRACE_B to the same\n> place; that probably needs to be spelled out more explicitly somewhere\n> in the documentation, as that would be a more common thing to do.\n\nYeah, sorry. There is no documentation at all with this series yet. For\nthis round I was mainly trying to see how people felt about expanding\nthe trace functions at all.\n\n> I think your [7/8] is kind of strange when viewed in that light.  Imagine\n> what would happen if you gave separate GIT_TRACE_* to each packet class,\n> instead of giving them a single umbrella variable GIT_TRACE_PACKET.  If\n> the user wants to see them all in a single stream, the same redirection\n> you would use to unify GIT_TRACE_A and GIT_TRACE_B can be used.\n\nYeah, you could break it down by packet type, though I don't know why\nanyone would want to (unless perhaps dumping the whole pack to some\nalternate location than the rest of it).\n\n> Instead, you have packet class prefix in the output so that later the\n> different kinds of packet events can be sifted out from the unified\n> output, even though they are forced to go to the same output stream.  In a\n> sense, you have two-tier classification system for traceable events (the\n> top layer that can be separated or merged at the file descriptor level,\n> and the bottom layer that can only be separated by looking at the prefix).\n> \n> Is this necessarily a good thing (not a rhetorical question)?\n\nI don't see it as a problem. As a developer, I want to throw in some\ntrace statements that are logically related to me. So I give them a name\nand put in the trace statements. The user can now have those statements\nturned on or off, and if on, can tell them where to go. No, they can't\nsay \"I want statement X but not statement Y\" or \"I want packet X and not\npacket Y\". But what is the right degree of resolution? Too coarse, and\nthey get a few traces they don't want. Too fine, and the syntax for\naddressing each individual trace statement gets cumbersome.  At some\npoint, you trust the developer's notion of a logical unit to be\nsensible.\n\nSo I guess I'm not sure what your complaint is. You don't like the\nGIT_TRACE_FOO versus GIT_TRACE_BAR syntax?  Or you disagree with the\nlogical unit I chose for packet traces? If the latter, then I welcome\nimprovement patches.\n\n> To put it another and opposite way, I wonder if it would be better to\n> instead use a single output stream named by GIT_TRACE and add trace event\n> class prefix to the output for classes like SETUP and PACKET (again, not a\n> rhetorical question).\n\nThat was actually my original design (that I didn't submit to the list).\nBut I rejected it because:\n\n  1. Even though you generally want to see several trace types in\n     chronological order, you might want the flexibility of putting\n     different traces in different locations. The current packet code\n     doesn't dump the pack at all, assuming it is uninteresting binary\n     goo (because for my purposes, the logical unit was the\n     negotiation). But let's say for example that you _do_ want to dump\n     the pack. If I have (just making up names):\n\n        GIT_TRACE=/tmp/foo\n        GIT_TRACE_CLASS=packet,incoming-packfile\n\n     then you get everything in /tmp/foo, and you are stuck sorting the\n     trace lines from the binary goo yourself. It's more natural to do:\n\n       GIT_TRACE_PACKET=1 ;# send to stderr\n       GIT_TRACE_INCOMING_PACKFILE=/tmp/dump.pack\n\n  2. Multiple variables made invoking a little more cumbersome. If the\n     trace variables are reasonable logical units, you probably only want\n     to see one at a time. If you're interested in debugging packet\n     traces, you probably don't care about seeing the exec-tracing. It's\n     useless cruft. So you do:\n\n       GIT_TRACE_PACKET=<whatever> git ...\n\n     which to my mind is much simpler than\n\n       GIT_TRACE=<whatever> GIT_TRACE_CLASS=packet git ...\n\n     It's less typing, and conceptually simpler. And I say this not just\n     hypothetically, but because I had originally implemented it the\n     other way and found it annoying.\n\n     Now I do recognize that I am optimizing for one case I envision as\n     the common case, and it's a trade-off. If your <whatever> is long,\n     then putting multiple facilities to the same place is more\n     cumbersome:\n\n       GIT_TRACE_FOO=<whatever> GIT_TRACE_BAR=<whatever>\n\n     but in my experience, <whatever> was usually \"1\".\n\nYou could do some hybrid like:\n\n  GIT_TRACE=packet,exec:/path/to/dumpfile\n\nbut I didn't see any point in getting very complex with parsing.\n\n> Also instead of wasting environment variable names, it might be a more\n> compact design from the user's point of view if we took a list of trace\n> event classes in a single environment variable, e.g.\n> \n> \tGIT_TRACE_CLASS=setup,packet \\\n>         GIT_TRACE=/tmp/tr \\\n>         git push\n> \n> I dunno.\n\nI think I covered that pretty well above, but you lose the flexibility\nof pushing different trace types to different places if you want to.\n\n-Peff\n"},{"id":"162185","messageId":"7vbp211alu.fsf@alter.siamese.dyndns.org","threadId":"26592","inReplyTo":"20110224190258.GA4318@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-24T19:44:13Z","receivedAt":"2011-02-24T19:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Feb 24, 2011 at 10:25:04AM -0800, Junio C Hamano wrote:\n> ...\n> So I guess I'm not sure what your complaint is.\n\nThere was no complaint. I wanted to see the reasoning behind the choices\nyou made spelled out (and later copied into developer docs).\n\n> I think I covered that pretty well above, but you lose the flexibility\n> of pushing different trace types to different places if you want to.\n\nThat statement illustrates the inflexibility that all packet traces going\nto a same place rather clearly, doesn't it, though?  That \"two-tier\" thing\nwas my primary concern.\n"},{"id":"162186","messageId":"20110224194828.GA5616@sigill.intra.peff.net","threadId":"26592","inReplyTo":"7vbp211alu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-24T19:48:28Z","receivedAt":"2011-02-24T19:48:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 24, 2011 at 11:44:13AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, Feb 24, 2011 at 10:25:04AM -0800, Junio C Hamano wrote:\n> > ...\n> > So I guess I'm not sure what your complaint is.\n> \n> There was no complaint. I wanted to see the reasoning behind the choices\n> you made spelled out (and later copied into developer docs).\n\nOh. :) I'll write something up to go into Documentation/technical for my\nre-roll. And something should clearly go in Documentation/git.txt for\nusers, too.\n\n> > I think I covered that pretty well above, but you lose the flexibility\n> > of pushing different trace types to different places if you want to.\n> \n> That statement illustrates the inflexibility that all packet traces going\n> to a same place rather clearly, doesn't it, though?  That \"two-tier\" thing\n> was my primary concern.\n\nSorry, I don't quite understand what you're saying here.\n\n-Peff\n"},{"id":"162188","messageId":"AANLkTikHdLqC2=Uw7D_0uJ7m9LuvcUFmVUkC9qAGWiue@mail.gmail.com","threadId":"26592","inReplyTo":"20110224160555.GB30275@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-02-24T20:01:58Z","receivedAt":"2011-02-24T20:01:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 24, 2011 at 5:05 PM, Jeff King <peff@peff.net> wrote:\n>\n> But those are not strong objections. Just giving the rationale I used\n> while writing the patches. I'd be curious what others think.\n\nI think the design is nice!\n\nThanks,\nChristian.\n"},{"id":"162189","messageId":"20110224203317.GC17412@elie","threadId":"26592","inReplyTo":"20110224142647.GA15477@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 1/8] compat: provide a fallback va_copy definition","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T20:33:17Z","receivedAt":"2011-02-24T20:33:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> So my guess is that this will be sufficient, but I we won't really know\n> for sure until somebody reports a problem. :(\n\nSounds like a good approach to me.\n\nThe POSIX rationale hints that some historical implementations used\narrays of size 1.  Assignment would error out on such an\nimplementation, which is not a big deal (since we could switch to\nmemcpy then).  I would be more worried about a historical\nimplementation using dynamic allocation with va_list being a pointer\nbut I haven't heard of any.\n\n> +#ifndef va_copy\n> +#define va_copy(dst,src) (dst) = (src)\n> +#endif\n\nThe following (as Erik mentinoed) might be a nice cleanup on top.  The\nduplicate va_copy definition is just redundant rather than causing\ncompilation errors because it comes before git-compat-util's\nifndef-guarded one.\n\ndiff --git a/compat/msvc.h b/compat/msvc.h\nindex 023aba0..a33b01c 100644\n--- a/compat/msvc.h\n+++ b/compat/msvc.h\n@@ -9,7 +9,6 @@\n #define inline __inline\n #define __inline__ __inline\n #define __attribute__(x)\n-#define va_copy(dst, src)     ((dst) = (src))\n #define strncasecmp  _strnicmp\n #define ftruncate    _chsize\n \n"},{"id":"162191","messageId":"7v39nd17im.fsf@alter.siamese.dyndns.org","threadId":"26592","inReplyTo":"20110224194828.GA5616@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-24T20:50:57Z","receivedAt":"2011-02-24T20:50:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > I think I covered that pretty well above, but you lose the flexibility\n>> > of pushing different trace types to different places if you want to.\n>> \n>> That statement illustrates the inflexibility that all packet traces going\n>> to a same place rather clearly, doesn't it, though?  That \"two-tier\" thing\n>> was my primary concern.\n>\n> Sorry, I don't quite understand what you're saying here.\n\nI was contrasting these two and called that \"two-tier thing\":\n\n - GIT_TRACE_SETUP and GIT_TRACE_PACKET can be used to give you\n   flexibility of sending their output to the same location or to\n   different destination.  Nice and flexibile.\n\n - Even though the trace identities like \"clone\", \"fetch-pack\", etc. are\n   clearly marked in the code, you cannot take advantage of the marking\n   and send the output from these packet sources to different destination,\n   because GIT_TRACE_PACKET is just one and single variable.\n\nClearer?\n\nNot that I think the same flexibility in the latter is absolutely\nnecessary, but it somewhat feels inconsistent to have these two classes.\n"},{"id":"162192","messageId":"20110224205128.GD17412@elie","threadId":"26592","inReplyTo":"20110224142711.GB15477@sigill.intra.peff.net","subject":"Re: [PATCH 2/8] strbuf: add strbuf_addv","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T20:51:28Z","receivedAt":"2011-02-24T20:51:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> In a variable-args function, the code for writing into a\n> strbuf is non-trivial. We ended up cutting and pasting it in\n> several places because there was no vprintf-style function\n> for strbufs (which in turn was held up by a lack of\n> va_copy).\n>\n> Now that we have a fallback va_copy, we can add strbuf_addv,\n> the strbuf equivalent of vsprintf.\n\n\"strbuf_vaddf\" would follow the vsnprintf scheme a little better.\n(I think strbuf_addv is just as intuitive, though.)\n\nThanks, I was looking for something like this.\n\nAnd apparently it has been a wish for 2 years now. :)  See\nhttp://thread.gmane.org/gmane.comp.version-control.git/75250/focus=76396\nfor amusement.\n\n>  5 files changed, 24 insertions(+), 63 deletions(-)\n\nNice.\n\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -120,6 +120,7 @@ extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *\n>  \n>  __attribute__((format (printf,2,3)))\n>  extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n> +extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);\n\n\"__attribute__(format (printf,2,0))\" could tell gcc to check that the format\nstring is well formed and give it a chance to carp if you try\n\n\tstatic inline void foo(struct strbuf *sb, const char *fmt, ...)\n\t{\n\t\tva_list ap;\n\n\t\tva_start(ap, fmt);\n\t\tstrbuf_addv(sb, fmt, ap);\n\t\tva_end(ap);\n\t}\n\n\t...\n\t\tfoo(some_user_supplied_string);\n---\ndiff --git a/strbuf.h b/strbuf.h\nindex 65b44a0..0c788d5 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -120,6 +120,7 @@ extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *\n \n __attribute__((format (printf,2,3)))\n extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+__attribute__((format (printf,2,0)))\n extern void strbuf_addv(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n"},{"id":"162193","messageId":"20110224210836.GE17412@elie","threadId":"26592","inReplyTo":"20110224142814.GC15477@sigill.intra.peff.net","subject":"Re: [PATCH 3/8] trace: add trace_vprintf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T21:08:36Z","receivedAt":"2011-02-24T21:08:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1067,6 +1067,7 @@ extern void alloc_report(void);\n>  /* trace.c */\n>  __attribute__((format (printf, 1, 2)))\n>  extern void trace_printf(const char *format, ...);\n> +extern void trace_vprintf(const char *format, va_list ap);\n\nMicronit.\n\nThough now that I think of it, the format(printf, n, 0) attribute\ndoesn't seem to be used anywhere in git currently.  Is it unportable?\n---\ndiff --git a/cache.h b/cache.h\nindex 08b23b2..288d4d0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1067,6 +1067,7 @@ extern void alloc_report(void);\n /* trace.c */\n __attribute__((format (printf, 1, 2)))\n extern void trace_printf(const char *format, ...);\n+__attribute__((format (printf, 1, 0)))\n extern void trace_vprintf(const char *format, va_list ap);\n __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n"},{"id":"162195","messageId":"20110224212358.GF17412@elie","threadId":"26592","inReplyTo":"20110224142841.GD15477@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] trace: refactor to support multiple env variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T21:23:59Z","receivedAt":"2011-02-24T21:23:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> However, we still maintain\n> the trace_printf interface so that existing callers do not\n> need to be refactored.\n\nHmm, so trace_printf means \"trace exec, notes_merge, and\nRUNTIME_PREFIX\"?\n"},{"id":"162196","messageId":"20110224214412.GG17412@elie","threadId":"26592","inReplyTo":"20110224143019.GG15477@sigill.intra.peff.net","subject":"Re: [PATCH 7/8] add packet tracing debug code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T21:44:12Z","receivedAt":"2011-02-24T21:44:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This shows a trace of all packets coming in or out of a given\n> program. This can help with debugging object negotiation or\n> other protocol issues.\n\nSounds handy.\n\n> Packet tracing can be enabled with GIT_TRACE_PACKET=<foo>,\n> where <foo> takes the same arguments as GIT_TRACE.\n[...]\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -383,6 +383,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \n>  \tjunk_pid = getpid();\n>  \n> +\tpacket_trace_identity(\"clone\");\n\nMy first thought on reading this was \"identity operation\" (= pass-through),\nwhich left me a little confused.\n\nMaybe \"packet_trace_set_identity\" (or ...set_prefix) would be a little\nclearer?\n\n> +++ b/pkt-line.c\n> @@ -1,6 +1,51 @@\n[...]\n> +\tif ((len >= 4 && !prefixcmp(buf, \"PACK\")) ||\n> +\t    (len >= 5 && !prefixcmp(buf+1, \"PACK\"))) {\n> +\t\tstrbuf_addstr(&out, \"PACK ...\");\n> +\t\tunsetenv(trace_key);\n> +\t}\n> +\telse {\n\nStyle: should be cuddled, I think.\n\n> +\t\t/* XXX we should really handle printable utf8 */\n> +\t\tfor (i = 0; i < len; i++) {\n\nMaybe using is_utf8, extended to take a length argument?\n(Assuming it's okay if non-ASCII is escaped in packets containing\nnon-utf8 parts.)\n\n[...]\n> @@ -39,11 +84,13 @@ ssize_t safe_write(int fd, const void *buf, ssize_t n)\n>   */\n>  void packet_flush(int fd)\n>  {\n> +\tpacket_trace(\"0000\", 4, 1);\n>  \tsafe_write(fd, \"0000\", 4);\n\n... I wonder if it would make sense to do\n\n static void packet_writebuf(int fd, const char *buf, size_t buflen)\n {\n\tpacket_trace(buf, buflen, 1);\n\tsafe_write(fd, buf, buflen);\n }\n\nNah, that would include the size noise for even nonempty packets,\nwhich your implementation conveniently omits.\n\nThanks, looks useful.\n"},{"id":"162197","messageId":"20110224214959.GH17412@elie","threadId":"26592","inReplyTo":"20110224143030.GH15477@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-24T21:49:59Z","receivedAt":"2011-02-24T21:49:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> You no longer get this output with GIT_TRACE=1; instead, you\n> can do GIT_TRACE_SETUP=1.\n\nThank you. :)\n\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -155,10 +155,11 @@ static const char *quote_crnl(const char *path)\n>  /* FIXME: move prefix to startup_info struct and get rid of this arg */\n>  void trace_repo_setup(const char *prefix)\n>  {\n> +\tstatic const char *key = \"GIT_TRACE_SETUP\";\n\nMicronit:\n\n\tstatic const char key[] = ...\n\n> @@ -170,10 +171,10 @@ void trace_repo_setup(const char *prefix)\n>  \tif (!prefix)\n>  \t\tprefix = \"(null)\";\n>  \n> -\ttrace_printf(\"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n> -\ttrace_printf(\"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n> -\ttrace_printf(\"setup: cwd: %s\\n\", quote_crnl(cwd));\n> -\ttrace_printf(\"setup: prefix: %s\\n\", quote_crnl(prefix));\n> +\ttrace_printf_key(key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n> +\ttrace_printf_key(key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n> +\ttrace_printf_key(key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n> +\ttrace_printf_key(key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n\nI wonder if it would make sense for this to be\n\n\ttrace_printf(\"setup\", \"git_dir: %s\\n\", ...);\n\nand:\n\n - automatically prefix each line with the key instead of \"trace:\"\n - enable or redirect based on the content of the GIT_TRACE_$(uc $key)\n   variable\n\nBut what you have here already works, so:\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"162207","messageId":"AANLkTimC2gv4GjA6YT7H9KuPcwOApkPxRcbDEOvvv9M8@mail.gmail.com","threadId":"26592","inReplyTo":"20110224214959.GH17412@elie","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-25T06:38:25Z","receivedAt":"2011-02-25T06:38:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 25, 2011 at 4:49 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Jeff King wrote:\n>\n>> You no longer get this output with GIT_TRACE=1; instead, you\n>> can do GIT_TRACE_SETUP=1.\n\nMinor note. GIT_TRACE warning in test-lib.sh will need improvement for\nchecking GIT_TRACE_*\n\n>> @@ -170,10 +171,10 @@ void trace_repo_setup(const char *prefix)\n>>       if (!prefix)\n>>               prefix = \"(null)\";\n>>\n>> -     trace_printf(\"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n>> -     trace_printf(\"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n>> -     trace_printf(\"setup: cwd: %s\\n\", quote_crnl(cwd));\n>> -     trace_printf(\"setup: prefix: %s\\n\", quote_crnl(prefix));\n>> +     trace_printf_key(key, \"setup: git_dir: %s\\n\", quote_crnl(get_git_dir()));\n>> +     trace_printf_key(key, \"setup: worktree: %s\\n\", quote_crnl(git_work_tree));\n>> +     trace_printf_key(key, \"setup: cwd: %s\\n\", quote_crnl(cwd));\n>> +     trace_printf_key(key, \"setup: prefix: %s\\n\", quote_crnl(prefix));\n>\n> I wonder if it would make sense for this to be\n>\n>        trace_printf(\"setup\", \"git_dir: %s\\n\", ...);\n>\n> and:\n>\n>  - automatically prefix each line with the key instead of \"trace:\"\n>  - enable or redirect based on the content of the GIT_TRACE_$(uc $key)\n>   variable\n\nYeah I think it's nice to have key = \"setup\", then env name becomes\nGIT_TRACE_$(uc $key) and prepend \"$key: \" in all trace messages.\n-- \nDuy\n"},{"id":"162294","messageId":"7v7hcn8a9r.fsf@alter.siamese.dyndns.org","threadId":"26592","inReplyTo":"20110224143030.GH15477@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] trace: give repo_setup trace its own key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-26T08:34:08Z","receivedAt":"2011-02-26T08:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> You no longer get this output with GIT_TRACE=1; instead, you\n> can do GIT_TRACE_SETUP=1.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nHence you would need this on top?\n\n t/t1510-repo-setup.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 15101d5..ec50a9a 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -57,7 +57,7 @@ test_repo () {\n \t\t\texport GIT_WORK_TREE\n \t\tfi &&\n \t\trm -f trace &&\n-\t\tGIT_TRACE=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n+\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n \t\tgrep '^setup: ' trace >result &&\n \t\ttest_cmp expected result\n \t)\n"},{"id":"162968","messageId":"20110308083344.GD26516@elie","threadId":"26592","inReplyTo":"AANLkTin7NBx5NVRkHpgymcZfnaZaJehMMpPv=zD+cKtV@mail.gmail.com","subject":"[PATCH jk/strbuf-vaddf] compat: fall back on __va_copy if available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-08T08:33:44Z","receivedAt":"2011-03-08T08:33:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Since an obvious implementation of va_list is to make it a pointer\ninto the stack frame, implementing va_copy as \"dst = src\" will work on\nmany systems.  Platforms that use something different (e.g., a size-1\narray of structs, to be assigned with *(dst) = *(src)) will need some\nother compatibility macro, though.\n\nLuckily, as the glibc manual hints, such systems tend to provide the\n__va_copy macro (introduced in GCC in March, 1997).  By using that if\nit is available, we can cover our bases pretty well.\n\nDiscovered by building with CC=\"gcc -std=c89\" on an amd64 machine:\n\n $ make CC=c89 strbuf.o\n [...]\n strbuf.c: In function ‘strbuf_vaddf’:\n strbuf.c:211:2: error: incompatible types when assigning to type ‘va_list’ from type ‘struct __va_list_tag *’\n make: *** [strbuf.o] Error 1\n\nExplained-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nErik Faye-Lund wrote:\n\n> Wouldn't it be even more portable to fall back on use __va_copy (if\n> present), as suggested by Junio in\n> <7vbpip86q5.fsf@alter.siamese.dyndns.org>?\n\nYes, it seems so.\n\n git-compat-util.h |   11 ++++++++++-\n 1 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 00d41e4..f4cb0a9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -536,7 +536,16 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #endif\n \n #ifndef va_copy\n-#define va_copy(dst,src) (dst) = (src)\n+/*\n+ * Since an obvious implementation of va_list would be to make it a\n+ * pointer into the stack frame, a simple assignment will work on\n+ * many systems.  But let's try to be more portable.\n+ */\n+#ifdef __va_copy\n+#define va_copy(dst, src) __va_copy(dst, src)\n+#else\n+#define va_copy(dst, src) ((dst) = (src))\n+#endif\n #endif\n \n /*\n-- \n1.7.4.1\n"},{"id":"163011","messageId":"7v62rte5nb.fsf@alter.siamese.dyndns.org","threadId":"26592","inReplyTo":"20110308083344.GD26516@elie","subject":"Re: [PATCH jk/strbuf-vaddf] compat: fall back on __va_copy if available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-08T20:09:12Z","receivedAt":"2011-03-08T20:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Discovered by building with CC=\"gcc -std=c89\" on an amd64 machine:\n>\n>  $ make CC=c89 strbuf.o\n>  [...]\n\n>  strbuf.c: In function ‘strbuf_vaddf’:\n\nThis is a tangent but when you quote from compiler output could you be in\nC locale so that it would be easier for everybody to get the same result?\nIt will have the added benefit that we won't have to deal with these fancy\nmatching quotes I personally hate to see in commit log messages.\n\nThanks, the patch looks good.\n"},{"id":"163017","messageId":"20110308205918.GC26471@elie","threadId":"26592","inReplyTo":"7v62rte5nb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH jk/strbuf-vaddf] compat: fall back on __va_copy if available","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-08T20:59:19Z","receivedAt":"2011-03-08T20:59:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> This is a tangent but when you quote from compiler output could you be in\n> C locale so that it would be easier for everybody to get the same result?\n\nSure, that makes sense.\n\n[...]\n> Thanks, the patch looks good.\n\nThanks for looking it over.\n"}]}