{"thread":{"id":"34070","subject":"[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access","startedAt":"2013-06-09T05:22:48Z","lastAt":"2013-06-09T23:16:38Z","messageCount":9,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"219882","messageId":"1370755369-30868-1-git-send-email-pclouds@gmail.com","threadId":"34070","inReplyTo":null,"subject":"[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-09T05:22:48Z","receivedAt":"2013-06-09T05:22:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"5f44324 (core: log offset pack data accesses happened - 2011-07-06)\nprovides a way to observe pack access patterns via a config\nswitch. Setting an environment variable looks more obvious than a\nconfig var, especially when you just need to _observe_, and more\ninline with other tracing knobs we have.\n\nDocument it as it may be useful for remote troubleshooting.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git.txt |  7 +++++++\n cache.h               |  3 ---\n config.c              |  3 ---\n environment.c         |  1 -\n sha1_file.c           | 14 ++++++++++++--\n 5 files changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 68f1ee6..c760918 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -838,6 +838,13 @@ for further details.\n \tas a file path and will try to write the trace messages\n \tinto it.\n \n+'GIT_TRACE_PACK_ACCESS'::\n+\tIf this variable is set to a path, a file will be created at\n+\tthe given path logging all accesses to any packs. For each\n+\taccess, the pack file name and an offset in the pack is\n+\trecorded. This may be helpful for troubleshooting some\n+\tpack-related performance problems.\n+\n GIT_LITERAL_PATHSPECS::\n \tSetting this variable to `1` will cause Git to treat all\n \tpathspecs literally, rather than as glob patterns. For example,\ndiff --git a/cache.h b/cache.h\nindex df532f8..4f41606 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -772,9 +772,6 @@ extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\n /* global flag to enable extra checks when accessing packed objects */\n extern int do_check_packed_object_crc;\n \n-/* for development: log offset of pack access */\n-extern const char *log_pack_access;\n-\n extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n \n extern int move_temp_to_file(const char *tmpfile, const char *filename);\ndiff --git a/config.c b/config.c\nindex 7a85ebd..d04e815 100644\n--- a/config.c\n+++ b/config.c\n@@ -688,9 +688,6 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.logpackaccess\"))\n-\t\treturn git_config_string(&log_pack_access, var, value);\n-\n \tif (!strcmp(var, \"core.autocrlf\")) {\n \t\tif (value && !strcasecmp(value, \"input\")) {\n \t\t\tif (core_eol == EOL_CRLF)\ndiff --git a/environment.c b/environment.c\nindex e2e75c1..0cb67b2 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -37,7 +37,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 16 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n-const char *log_pack_access;\n const char *pager_program;\n int pager_use_color = 1;\n const char *editor_program;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b114cc9..4b23bb8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -36,6 +36,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n const unsigned char null_sha1[20];\n \n+static const char *no_log_pack_access = \"no_log_pack_access\";\n+static const char *log_pack_access;\n+\n /*\n  * This is meant to hold a *small* number of objects that you would\n  * want read_sha1_file() to be able to return, but yet you do not want\n@@ -1956,12 +1959,19 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)\n {\n \tstatic FILE *log_file;\n \n+\tif (!log_pack_access)\n+\t\tlog_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n+\tif (!log_pack_access)\n+\t\tlog_pack_access = no_log_pack_access;\n+\tif (log_pack_access == no_log_pack_access)\n+\t\treturn;\n+\n \tif (!log_file) {\n \t\tlog_file = fopen(log_pack_access, \"w\");\n \t\tif (!log_file) {\n \t\t\terror(\"cannot open pack access log '%s' for writing: %s\",\n \t\t\t      log_pack_access, strerror(errno));\n-\t\t\tlog_pack_access = NULL;\n+\t\t\tlog_pack_access = no_log_pack_access;\n \t\t\treturn;\n \t\t}\n \t}\n@@ -1992,7 +2002,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \tint delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;\n \tint base_from_cache = 0;\n \n-\tif (log_pack_access)\n+\tif (log_pack_access != no_log_pack_access)\n \t\twrite_pack_access_log(p, obj_offset);\n \n \t/* PHASE 1: drill down to the innermost base object */\n-- \n1.8.2.83.gc99314b\n"},{"id":"219884","messageId":"1370755369-30868-2-git-send-email-pclouds@gmail.com","threadId":"34070","inReplyTo":"1370755369-30868-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-09T05:22:49Z","receivedAt":"2013-06-09T05:22:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"This can help with debugging object negotiation or other protocol\nissues.\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git.txt | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex c760918..72e9045 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -845,6 +845,11 @@ for further details.\n \trecorded. This may be helpful for troubleshooting some\n \tpack-related performance problems.\n \n+'GIT_TRACE_PACKET'::\n+\tIf this variable is set, it shows a trace of all packets\n+\tcoming in or out of a given program. This can help with\n+\tdebugging object negotiation or other protocol issues.\n+\n GIT_LITERAL_PATHSPECS::\n \tSetting this variable to `1` will cause Git to treat all\n \tpathspecs literally, rather than as glob patterns. For example,\n-- \n1.8.2.83.gc99314b\n"},{"id":"219886","messageId":"20130609053621.GA1854@sigill.intra.peff.net","threadId":"34070","inReplyTo":"1370755369-30868-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-09T05:36:21Z","receivedAt":"2013-06-09T05:36:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 09, 2013 at 12:22:49PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index c760918..72e9045 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -845,6 +845,11 @@ for further details.\n>  \trecorded. This may be helpful for troubleshooting some\n>  \tpack-related performance problems.\n>  \n> +'GIT_TRACE_PACKET'::\n> +\tIf this variable is set, it shows a trace of all packets\n> +\tcoming in or out of a given program. This can help with\n> +\tdebugging object negotiation or other protocol issues.\n\nThis is not quite true. It stops showing packets after it sees a packet\nstarting with \"PACK\" (optionally with a sideband prefix). So you would\nmiss, for example, a sideband error that came after the pack had\nstarted. So it is really only useful for looking at the ref and object\nnegotiation phases.\n\nI know that probably sounds a bit nit-picky, but it might be worth\nmaking the distinction in case somebody is trying to track down such\nmessages.\n\n-Peff\n"},{"id":"219887","messageId":"1370757210-2994-1-git-send-email-pclouds@gmail.com","threadId":"34070","inReplyTo":"20130609053621.GA1854@sigill.intra.peff.net","subject":"[PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-09T05:53:30Z","receivedAt":"2013-06-09T05:53:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"This can help with debugging object negotiation or other protocol\nissues.\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n >> +'GIT_TRACE_PACKET'::\n >> +     If this variable is set, it shows a trace of all packets\n >> +     coming in or out of a given program. This can help with\n >> +     debugging object negotiation or other protocol issues.\n >\n > This is not quite true. It stops showing packets after it sees a packet\n > starting with \"PACK\" (optionally with a sideband prefix). So you would\n > miss, for example, a sideband error that came after the pack had\n > started. So it is really only useful for looking at the ref and object\n > negotiation phases.\n\n I blindly copied the first paragraph from bbc30f9 (add packet tracing\n debug code - 2011-02-24) and missed the \"PACK\" bit in the second one.\n How about this?\n\n Documentation/git.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex c760918..c10b647 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -845,6 +845,12 @@ for further details.\n \trecorded. This may be helpful for troubleshooting some\n \tpack-related performance problems.\n \n+'GIT_TRACE_PACKET'::\n+\tIf this variable is set, it shows a trace of all packets\n+\tcoming in or out of a given program. This can help with\n+\tdebugging object negotiation or other protocol issues. Tracing\n+\tis turned off at a packet starting with \"PACK\".\n+\n GIT_LITERAL_PATHSPECS::\n \tSetting this variable to `1` will cause Git to treat all\n \tpathspecs literally, rather than as glob patterns. For example,\n-- \n1.8.2.83.gc99314b\n"},{"id":"219888","messageId":"20130609055509.GA17617@sigill.intra.peff.net","threadId":"34070","inReplyTo":"1370757210-2994-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-09T05:55:10Z","receivedAt":"2013-06-09T05:55:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 09, 2013 at 12:53:30PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> \"This can help with debugging object negotiation or other protocol\n> issues.\"\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  >> +'GIT_TRACE_PACKET'::\n>  >> +     If this variable is set, it shows a trace of all packets\n>  >> +     coming in or out of a given program. This can help with\n>  >> +     debugging object negotiation or other protocol issues.\n>  >\n>  > This is not quite true. It stops showing packets after it sees a packet\n>  > starting with \"PACK\" (optionally with a sideband prefix). So you would\n>  > miss, for example, a sideband error that came after the pack had\n>  > started. So it is really only useful for looking at the ref and object\n>  > negotiation phases.\n> \n>  I blindly copied the first paragraph from bbc30f9 (add packet tracing\n>  debug code - 2011-02-24) and missed the \"PACK\" bit in the second one.\n>  How about this?\n\nHmm, what do you know, I sort-of documented it already. I think I just\nnever guessed that anybody but people reading \"git log\" would actually\nneed to use TRACE_PACKET. :)\n\nThanks, the patch looks fine to me.\n\n-Peff\n"},{"id":"219937","messageId":"CAPig+cQ-oKdL75P=f-WwKLxu72UNsvZknOC4YaS8+U2yaZw+Bw@mail.gmail.com","threadId":"34070","inReplyTo":"1370755369-30868-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-06-09T07:17:09Z","receivedAt":"2013-06-09T07:17:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> 5f44324 (core: log offset pack data accesses happened - 2011-07-06)\n> provides a way to observe pack access patterns via a config\n> switch. Setting an environment variable looks more obvious than a\n> config var, especially when you just need to _observe_, and more\n> inline with other tracing knobs we have.\n>\n> Document it as it may be useful for remote troubleshooting.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Documentation/git.txt |  7 +++++++\n>  cache.h               |  3 ---\n>  config.c              |  3 ---\n>  environment.c         |  1 -\n>  sha1_file.c           | 14 ++++++++++++--\n>  5 files changed, 19 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 68f1ee6..c760918 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -838,6 +838,13 @@ for further details.\n>         as a file path and will try to write the trace messages\n>         into it.\n>\n> +'GIT_TRACE_PACK_ACCESS'::\n> +       If this variable is set to a path, a file will be created at\n> +       the given path logging all accesses to any packs. For each\n> +       access, the pack file name and an offset in the pack is\n> +       recorded. This may be helpful for troubleshooting some\n> +       pack-related performance problems.\n> +\n>  GIT_LITERAL_PATHSPECS::\n>         Setting this variable to `1` will cause Git to treat all\n>         pathspecs literally, rather than as glob patterns. For example,\n> diff --git a/cache.h b/cache.h\n> index df532f8..4f41606 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -772,9 +772,6 @@ extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\n>  /* global flag to enable extra checks when accessing packed objects */\n>  extern int do_check_packed_object_crc;\n>\n> -/* for development: log offset of pack access */\n> -extern const char *log_pack_access;\n> -\n>  extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n>\n>  extern int move_temp_to_file(const char *tmpfile, const char *filename);\n> diff --git a/config.c b/config.c\n> index 7a85ebd..d04e815 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -688,9 +688,6 @@ static int git_default_core_config(const char *var, const char *value)\n>                 return 0;\n>         }\n>\n> -       if (!strcmp(var, \"core.logpackaccess\"))\n> -               return git_config_string(&log_pack_access, var, value);\n> -\n>         if (!strcmp(var, \"core.autocrlf\")) {\n>                 if (value && !strcasecmp(value, \"input\")) {\n>                         if (core_eol == EOL_CRLF)\n> diff --git a/environment.c b/environment.c\n> index e2e75c1..0cb67b2 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -37,7 +37,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n>  size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n>  size_t delta_base_cache_limit = 16 * 1024 * 1024;\n>  unsigned long big_file_threshold = 512 * 1024 * 1024;\n> -const char *log_pack_access;\n>  const char *pager_program;\n>  int pager_use_color = 1;\n>  const char *editor_program;\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b114cc9..4b23bb8 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -36,6 +36,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }\n>\n>  const unsigned char null_sha1[20];\n>\n> +static const char *no_log_pack_access = \"no_log_pack_access\";\n> +static const char *log_pack_access;\n> +\n>  /*\n>   * This is meant to hold a *small* number of objects that you would\n>   * want read_sha1_file() to be able to return, but yet you do not want\n> @@ -1956,12 +1959,19 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)\n>  {\n>         static FILE *log_file;\n>\n> +       if (!log_pack_access)\n> +               log_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n> +       if (!log_pack_access)\n> +               log_pack_access = no_log_pack_access;\n> +       if (log_pack_access == no_log_pack_access)\n> +               return;\n> +\n>         if (!log_file) {\n>                 log_file = fopen(log_pack_access, \"w\");\n>                 if (!log_file) {\n>                         error(\"cannot open pack access log '%s' for writing: %s\",\n>                               log_pack_access, strerror(errno));\n> -                       log_pack_access = NULL;\n> +                       log_pack_access = no_log_pack_access;\n>                         return;\n>                 }\n>         }\n> @@ -1992,7 +2002,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n>         int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;\n>         int base_from_cache = 0;\n>\n> -       if (log_pack_access)\n> +       if (log_pack_access != no_log_pack_access)\n>                 write_pack_access_log(p, obj_offset);\n>\n>         /* PHASE 1: drill down to the innermost base object */\n> --\n> 1.8.2.83.gc99314b\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"219938","messageId":"CAPig+cRK+mcPXpsUZSQH1hHx3E2Aq+S_44rV0sytLnGBHh+1xg@mail.gmail.com","threadId":"34070","inReplyTo":"1370755369-30868-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-06-09T07:18:04Z","receivedAt":"2013-06-09T07:18:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> 5f44324 (core: log offset pack data accesses happened - 2011-07-06)\n> provides a way to observe pack access patterns via a config\n> switch. Setting an environment variable looks more obvious than a\n> config var, especially when you just need to _observe_, and more\n> inline with other tracing knobs we have.\n\ns/inline/in line/ (IMHO)\n\nOr: \"..., and akin to other tracing knobs we have.\"\n\n> Document it as it may be useful for remote troubleshooting.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"},{"id":"219939","messageId":"CAPig+cRiSqU=yTM05uFp7+bY-Q5Bn5S-dMeibV55Xb3x5j6qdA@mail.gmail.com","threadId":"34070","inReplyTo":"1370757210-2994-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-06-09T07:20:42Z","receivedAt":"2013-06-09T07:20:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jun 9, 2013 at 1:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> \"This can help with debugging object negotiation or other protocol\n> issues.\"\n\ns/\"//g\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"},{"id":"220206","messageId":"7v61xmoox5.fsf@alter.siamese.dyndns.org","threadId":"34070","inReplyTo":"20130609055509.GA17617@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-09T23:16:38Z","receivedAt":"2013-06-09T23:16:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both.  Will queue both after re-reading 1/2.\n"}]}