{"thread":{"id":"33988","subject":"[PATCH 2/2] git.txt: document GIT_TRACE_PACKET","startedAt":"2013-05-30T13:51:47Z","lastAt":"2013-06-04T07:48:04Z","messageCount":6,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"219002","messageId":"1369921908-2988-1-git-send-email-pclouds@gmail.com","threadId":"33988","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-05-30T13:51:47Z","receivedAt":"2013-05-30T13:51:47Z","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 sha1_file.c           | 10 ++++++++++\n 4 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 9e302b0..3e74440 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -832,6 +832,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 94ca1ac..9bfd76b 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 aefd80b..ce074d7 100644\n--- a/config.c\n+++ b/config.c\n@@ -675,9 +675,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/sha1_file.c b/sha1_file.c\nindex 67e815b..7b47bdc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -36,6 +36,8 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n const unsigned char null_sha1[20];\n \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,6 +1958,14 @@ 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+\t\tif (!*log_pack_access)\n+\t\t\tlog_pack_access = NULL;\n+\t\tif (!log_pack_access)\n+\t\t\treturn;\n+\t}\n+\n \tif (!log_file) {\n \t\tlog_file = fopen(log_pack_access, \"w\");\n \t\tif (!log_file) {\n-- \n1.8.2.83.gc99314b\n"},{"id":"219001","messageId":"1369921908-2988-2-git-send-email-pclouds@gmail.com","threadId":"33988","inReplyTo":"1369921908-2988-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-05-30T13:51:48Z","receivedAt":"2013-05-30T13:51:48Z","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 3e74440..12ef7a2 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -839,6 +839,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":"219285","messageId":"7vmwr7c59b.fsf@alter.siamese.dyndns.org","threadId":"33988","inReplyTo":"1369921908-2988-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":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T20:24:00Z","receivedAt":"2013-06-03T20:24:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 67e815b..7b47bdc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -36,6 +36,8 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }\n>  \n>  const unsigned char null_sha1[20];\n>  \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,6 +1958,14 @@ 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> +\t\tif (!*log_pack_access)\n> +\t\t\tlog_pack_access = NULL;\n> +\t\tif (!log_pack_access)\n> +\t\t\treturn;\n> +\t}\n\nHave you ever tested this?\n\nOnce log_pack_access goes to NULL (e.g. when it sees the empty\nstring it was initialized to), this new test will happily\ndereference NULL.\n"},{"id":"219312","messageId":"1370299959-5573-1-git-send-email-pclouds@gmail.com","threadId":"33988","inReplyTo":"7vmwr7c59b.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] 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-03T22:52:39Z","receivedAt":"2013-06-03T22:52:39Z","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 >> +     if (!*log_pack_access) {\n >> +             log_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n >> +             if (!*log_pack_access)\n >> +                     log_pack_access = NULL;\n >> +             if (!log_pack_access)\n >> +                     return;\n >> +     }\n >\n > Have you ever tested this?\n >\n > Once log_pack_access goes to NULL (e.g. when it sees the empty\n > string it was initialized to), this new test will happily\n > dereference NULL.\n\n My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it\n was set to an empty string. All cases tested now.\n\n Documentation/git.txt |  7 +++++++\n cache.h               |  3 ---\n config.c              |  3 ---\n sha1_file.c           | 10 ++++++++++\n 4 files changed, 17 insertions(+), 6 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 830ee14..e68184f 100644\n--- a/config.c\n+++ b/config.c\n@@ -675,9 +675,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/sha1_file.c b/sha1_file.c\nindex 67e815b..91c3627 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -36,6 +36,8 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n const unsigned char null_sha1[20];\n \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,6 +1958,14 @@ 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+\t\tif (log_pack_access && !*log_pack_access)\n+\t\t\tlog_pack_access = NULL;\n+\t\tif (!log_pack_access)\n+\t\t\treturn;\n+\t}\n+\n \tif (!log_file) {\n \t\tlog_file = fopen(log_pack_access, \"w\");\n \t\tif (!log_file) {\n-- \n1.8.2.83.gc99314b\n"},{"id":"219326","messageId":"7vzjv68k8b.fsf@alter.siamese.dyndns.org","threadId":"33988","inReplyTo":"1370299959-5573-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] core: use env variable instead of config var to turn on logging pack access","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-04T06:26:28Z","receivedAt":"2013-06-04T06:26:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  > Have you ever tested this?\n>  >\n>  > Once log_pack_access goes to NULL (e.g. when it sees the empty\n>  > string it was initialized to), this new test will happily\n>  > dereference NULL.\n>\n>  My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it\n>  was set to an empty string. All cases tested now.\n>\n> @@ -1956,6 +1958,14 @@ 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\nThe first time, we will see the static empty string and come into\nthis block...\n\n> +\t\tlog_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n> +\t\tif (log_pack_access && !*log_pack_access)\n> +\t\t\tlog_pack_access = NULL;\n> +\t\tif (!log_pack_access)\n> +\t\t\treturn;\n> +\t}\n\nThis may \n\n (1) not find the env-var, in which case log_pack_access becomes\n     NULL here, and the function returns.\n\n (2) find the env-var to be an empty string, in which case\n     log_pack_access becomes NULL in the next statement, and the\n     function returns.\n\n (3) find the env-var to be a non-empty string, and the function\n     does not return.\n\nAnd the next time around, (3) above may work fine; the first if()\nwill fail and we do not come in.  But in either (1) or (2), don't\nyou keep checking the environment every time you come here?\n\nOh, no, it is worse than that.  Either case you set the variable to\nNULL, so the very first \"if (!*log_pack_access)\" would dereference\nNULL.\n\nWhy not start from NULL:\n\n    static const char *log_pack_access;\n\ntreat that NULL as \"unknown\" state, and avoid running getenv over\nand over again by treating non-NULL value as \"known\"?  Perhaps like\nthis?\n\n\tif (!log_pack_access) {\n        \t/* First time: is env set? */\n                log_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n                if (!log_pack_access)\n                \tlog_pack_access = \"\";\n\t}\n        /* Now GIT_TRACE_PACK_ACCESS is known */\n        if (!*log_pack_access)\n        \treturn; /* not wanted */\n\nAs you can no longer rely on \"config is read before we do anything\nelse\" by switching to lazy env checking, your guard at the caller\nneeds to be updated, if you want to reduce unneeded call-return\noverhead:\n\n\tif (!log_pack_access || *log_pack_access)\n\t\twrite_pack_access_log(...);\n\nBut the guard is purely for performance, not correctness, because\nthe function now does the \"return no-op if we know we are not\nwanted\" itself.\n\nAlso you no longer need to have an extern variable in environment.c\n"},{"id":"219329","messageId":"CACsJy8BwKfzU0KOPpsPUb601HVpMPWiMAjctHLdpcOSUUUxfGw@mail.gmail.com","threadId":"33988","inReplyTo":"7vzjv68k8b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] core: use env variable instead of config var to turn on logging pack access","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-04T07:48:04Z","receivedAt":"2013-06-04T07:48:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 4, 2013 at 1:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n>\n>>  > Have you ever tested this?\n>>  >\n>>  > Once log_pack_access goes to NULL (e.g. when it sees the empty\n>>  > string it was initialized to), this new test will happily\n>>  > dereference NULL.\n>>\n>>  My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it\n>>  was set to an empty string. All cases tested now.\n>>\n>> @@ -1956,6 +1958,14 @@ 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>\n> The first time, we will see the static empty string and come into\n> this block...\n>\n>> +             log_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n>> +             if (log_pack_access && !*log_pack_access)\n>> +                     log_pack_access = NULL;\n>> +             if (!log_pack_access)\n>> +                     return;\n>> +     }\n>\n> This may\n>\n>  (1) not find the env-var, in which case log_pack_access becomes\n>      NULL here, and the function returns.\n>\n>  (2) find the env-var to be an empty string, in which case\n>      log_pack_access becomes NULL in the next statement, and the\n>      function returns.\n>\n>  (3) find the env-var to be a non-empty string, and the function\n>      does not return.\n>\n> And the next time around, (3) above may work fine; the first if()\n> will fail and we do not come in.  But in either (1) or (2), don't\n> you keep checking the environment every time you come here?\n>\n> Oh, no, it is worse than that.  Either case you set the variable to\n> NULL, so the very first \"if (!*log_pack_access)\" would dereference\n> NULL.\n\nYou found it out already. The only caller does this\n\nif (log_pack_access) write_pack_access_log(p, obj_offset);\n\nso in (1) and (2), write_pack_access_log() will never be called again.\nOriginally I had \"log_pack_access && !*log_pack_access\" in the first\nif(), but I dropped it because of the caller's condition. It's a bit\nfragile though. Imagine a new callsite is added.. (to be continued)\n\n> Why not start from NULL:\n>\n>     static const char *log_pack_access;\n>\n> treat that NULL as \"unknown\" state, and avoid running getenv over\n> and over again by treating non-NULL value as \"known\"?  Perhaps like\n> this?\n>\n>         if (!log_pack_access) {\n>                 /* First time: is env set? */\n>                 log_pack_access = getenv(\"GIT_TRACE_PACK_ACCESS\");\n>                 if (!log_pack_access)\n>                         log_pack_access = \"\";\n\nor set log_pack_access to SomePredefinedButUselessString here. I\nwanted to do this but couldn't because of global variable\ninitialization.\n\n>         }\n>         /* Now GIT_TRACE_PACK_ACCESS is known */\n>         if (!*log_pack_access)\n>                 return; /* not wanted */\n>\n> As you can no longer rely on \"config is read before we do anything\n> else\" by switching to lazy env checking, your guard at the caller\n> needs to be updated, if you want to reduce unneeded call-return\n> overhead:\n>\n>         if (!log_pack_access || *log_pack_access)\n>                 write_pack_access_log(...);\n\nand this turns to \"if (!log_.. || log != SomePrede...)\", no more\npeeking into log_pack_access.\n\n>\n> But the guard is purely for performance, not correctness, because\n> the function now does the \"return no-op if we know we are not\n> wanted\" itself.\n\n(continued here) ..so yeah this looks better.\n\n> Also you no longer need to have an extern variable in environment.c\n\nwill fix.\n--\nDuy\n"}]}