# [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access 9 messages from 2013-06-09 to 2013-06-09. Participants: Nguyễn Thái Ngọc Duy, Jeff King, Eric Sunshine, Junio C Hamano. Thread: https://gitlist.dev/t/34070 ## Nguyễn Thái Ngọc Duy, 2013-06-09 05:22 Subject: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access Message-ID: <1370755369-30868-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1370755369-30868-1-git-send-email-pclouds%40gmail.com ``` 5f44324 (core: log offset pack data accesses happened - 2011-07-06) provides a way to observe pack access patterns via a config switch. Setting an environment variable looks more obvious than a config var, especially when you just need to _observe_, and more inline with other tracing knobs we have. Document it as it may be useful for remote troubleshooting. Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git.txt | 7 +++++++ cache.h | 3 --- config.c | 3 --- environment.c | 1 - sha1_file.c | 14 ++++++++++++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 68f1ee6..c760918 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -838,6 +838,13 @@ for further details. as a file path and will try to write the trace messages into it. +'GIT_TRACE_PACK_ACCESS':: + If this variable is set to a path, a file will be created at + the given path logging all accesses to any packs. For each + access, the pack file name and an offset in the pack is + recorded. This may be helpful for troubleshooting some + pack-related performance problems. + GIT_LITERAL_PATHSPECS:: Setting this variable to `1` will cause Git to treat all pathspecs literally, rather than as glob patterns. For example, diff --git a/cache.h b/cache.h index df532f8..4f41606 100644 --- a/cache.h +++ b/cache.h @@ -772,9 +772,6 @@ extern int parse_sha1_header(const char *hdr, unsigned long *sizep); /* global flag to enable extra checks when accessing packed objects */ extern int do_check_packed_object_crc; -/* for development: log offset of pack access */ -extern const char *log_pack_access; - extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type); extern int move_temp_to_file(const char *tmpfile, const char *filename); diff --git a/config.c b/config.c index 7a85ebd..d04e815 100644 --- a/config.c +++ b/config.c @@ -688,9 +688,6 @@ static int git_default_core_config(const char *var, const char *value) return 0; } - if (!strcmp(var, "core.logpackaccess")) - return git_config_string(&log_pack_access, var, value); - if (!strcmp(var, "core.autocrlf")) { if (value && !strcasecmp(value, "input")) { if (core_eol == EOL_CRLF) diff --git a/environment.c b/environment.c index e2e75c1..0cb67b2 100644 --- a/environment.c +++ b/environment.c @@ -37,7 +37,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 16 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; -const char *log_pack_access; const char *pager_program; int pager_use_color = 1; const char *editor_program; diff --git a/sha1_file.c b/sha1_file.c index b114cc9..4b23bb8 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -36,6 +36,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; } const unsigned char null_sha1[20]; +static const char *no_log_pack_access = "no_log_pack_access"; +static const char *log_pack_access; + /* * This is meant to hold a *small* number of objects that you would * want read_sha1_file() to be able to return, but yet you do not want @@ -1956,12 +1959,19 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset) { static FILE *log_file; + if (!log_pack_access) + log_pack_access = getenv("GIT_TRACE_PACK_ACCESS"); + if (!log_pack_access) + log_pack_access = no_log_pack_access; + if (log_pack_access == no_log_pack_access) + return; + if (!log_file) { log_file = fopen(log_pack_access, "w"); if (!log_file) { error("cannot open pack access log '%s' for writing: %s", log_pack_access, strerror(errno)); - log_pack_access = NULL; + log_pack_access = no_log_pack_access; return; } } @@ -1992,7 +2002,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset, int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC; int base_from_cache = 0; - if (log_pack_access) + if (log_pack_access != no_log_pack_access) write_pack_access_log(p, obj_offset); /* PHASE 1: drill down to the innermost base object */ -- 1.8.2.83.gc99314b ``` ## Nguyễn Thái Ngọc Duy, 2013-06-09 05:22 Subject: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: <1370755369-30868-2-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1370755369-30868-2-git-send-email-pclouds%40gmail.com In-Reply-To: <1370755369-30868-1-git-send-email-pclouds@gmail.com> ``` "This can help with debugging object negotiation or other protocol issues." Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index c760918..72e9045 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -845,6 +845,11 @@ for further details. recorded. This may be helpful for troubleshooting some pack-related performance problems. +'GIT_TRACE_PACKET':: + If this variable is set, it shows a trace of all packets + coming in or out of a given program. This can help with + debugging object negotiation or other protocol issues. + GIT_LITERAL_PATHSPECS:: Setting this variable to `1` will cause Git to treat all pathspecs literally, rather than as glob patterns. For example, -- 1.8.2.83.gc99314b ``` ## Jeff King, 2013-06-09 05:36 Subject: Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: <20130609053621.GA1854@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130609053621.GA1854%40sigill.intra.peff.net In-Reply-To: <1370755369-30868-2-git-send-email-pclouds@gmail.com> ``` On Sun, Jun 09, 2013 at 12:22:49PM +0700, Nguyen Thai Ngoc Duy wrote: > diff --git a/Documentation/git.txt b/Documentation/git.txt > index c760918..72e9045 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -845,6 +845,11 @@ for further details. > recorded. This may be helpful for troubleshooting some > pack-related performance problems. > > +'GIT_TRACE_PACKET':: > + If this variable is set, it shows a trace of all packets > + coming in or out of a given program. This can help with > + debugging object negotiation or other protocol issues. This is not quite true. It stops showing packets after it sees a packet starting with "PACK" (optionally with a sideband prefix). So you would miss, for example, a sideband error that came after the pack had started. So it is really only useful for looking at the ref and object negotiation phases. I know that probably sounds a bit nit-picky, but it might be worth making the distinction in case somebody is trying to track down such messages. -Peff ``` ## Nguyễn Thái Ngọc Duy, 2013-06-09 05:53 Subject: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: <1370757210-2994-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1370757210-2994-1-git-send-email-pclouds%40gmail.com In-Reply-To: <20130609053621.GA1854@sigill.intra.peff.net> ``` "This can help with debugging object negotiation or other protocol issues." Signed-off-by: Nguyễn Thái Ngọc Duy --- >> +'GIT_TRACE_PACKET':: >> +     If this variable is set, it shows a trace of all packets >> +     coming in or out of a given program. This can help with >> +     debugging object negotiation or other protocol issues. > > This is not quite true. It stops showing packets after it sees a packet > starting with "PACK" (optionally with a sideband prefix). So you would > miss, for example, a sideband error that came after the pack had > started. So it is really only useful for looking at the ref and object > negotiation phases. I blindly copied the first paragraph from bbc30f9 (add packet tracing debug code - 2011-02-24) and missed the "PACK" bit in the second one. How about this? Documentation/git.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index c760918..c10b647 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -845,6 +845,12 @@ for further details. recorded. This may be helpful for troubleshooting some pack-related performance problems. +'GIT_TRACE_PACKET':: + If this variable is set, it shows a trace of all packets + coming in or out of a given program. This can help with + debugging object negotiation or other protocol issues. Tracing + is turned off at a packet starting with "PACK". + GIT_LITERAL_PATHSPECS:: Setting this variable to `1` will cause Git to treat all pathspecs literally, rather than as glob patterns. For example, -- 1.8.2.83.gc99314b ``` ## Jeff King, 2013-06-09 05:55 Subject: Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: <20130609055509.GA17617@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130609055509.GA17617%40sigill.intra.peff.net In-Reply-To: <1370757210-2994-1-git-send-email-pclouds@gmail.com> ``` On Sun, Jun 09, 2013 at 12:53:30PM +0700, Nguyen Thai Ngoc Duy wrote: > "This can help with debugging object negotiation or other protocol > issues." > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > >> +'GIT_TRACE_PACKET':: > >> +     If this variable is set, it shows a trace of all packets > >> +     coming in or out of a given program. This can help with > >> +     debugging object negotiation or other protocol issues. > > > > This is not quite true. It stops showing packets after it sees a packet > > starting with "PACK" (optionally with a sideband prefix). So you would > > miss, for example, a sideband error that came after the pack had > > started. So it is really only useful for looking at the ref and object > > negotiation phases. > > I blindly copied the first paragraph from bbc30f9 (add packet tracing > debug code - 2011-02-24) and missed the "PACK" bit in the second one. > How about this? Hmm, what do you know, I sort-of documented it already. I think I just never guessed that anybody but people reading "git log" would actually need to use TRACE_PACKET. :) Thanks, the patch looks fine to me. -Peff ``` ## Eric Sunshine, 2013-06-09 07:17 Subject: Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ-oKdL75P%3Df-WwKLxu72UNsvZknOC4YaS8%2BU2yaZw%2BBw%40mail.gmail.com In-Reply-To: <1370755369-30868-1-git-send-email-pclouds@gmail.com> ``` On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy wrote: > 5f44324 (core: log offset pack data accesses happened - 2011-07-06) > provides a way to observe pack access patterns via a config > switch. Setting an environment variable looks more obvious than a > config var, especially when you just need to _observe_, and more > inline with other tracing knobs we have. > > Document it as it may be useful for remote troubleshooting. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Documentation/git.txt | 7 +++++++ > cache.h | 3 --- > config.c | 3 --- > environment.c | 1 - > sha1_file.c | 14 ++++++++++++-- > 5 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 68f1ee6..c760918 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -838,6 +838,13 @@ for further details. > as a file path and will try to write the trace messages > into it. > > +'GIT_TRACE_PACK_ACCESS':: > + If this variable is set to a path, a file will be created at > + the given path logging all accesses to any packs. For each > + access, the pack file name and an offset in the pack is > + recorded. This may be helpful for troubleshooting some > + pack-related performance problems. > + > GIT_LITERAL_PATHSPECS:: > Setting this variable to `1` will cause Git to treat all > pathspecs literally, rather than as glob patterns. For example, > diff --git a/cache.h b/cache.h > index df532f8..4f41606 100644 > --- a/cache.h > +++ b/cache.h > @@ -772,9 +772,6 @@ extern int parse_sha1_header(const char *hdr, unsigned long *sizep); > /* global flag to enable extra checks when accessing packed objects */ > extern int do_check_packed_object_crc; > > -/* for development: log offset of pack access */ > -extern const char *log_pack_access; > - > extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type); > > extern int move_temp_to_file(const char *tmpfile, const char *filename); > diff --git a/config.c b/config.c > index 7a85ebd..d04e815 100644 > --- a/config.c > +++ b/config.c > @@ -688,9 +688,6 @@ static int git_default_core_config(const char *var, const char *value) > return 0; > } > > - if (!strcmp(var, "core.logpackaccess")) > - return git_config_string(&log_pack_access, var, value); > - > if (!strcmp(var, "core.autocrlf")) { > if (value && !strcasecmp(value, "input")) { > if (core_eol == EOL_CRLF) > diff --git a/environment.c b/environment.c > index e2e75c1..0cb67b2 100644 > --- a/environment.c > +++ b/environment.c > @@ -37,7 +37,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; > size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; > size_t delta_base_cache_limit = 16 * 1024 * 1024; > unsigned long big_file_threshold = 512 * 1024 * 1024; > -const char *log_pack_access; > const char *pager_program; > int pager_use_color = 1; > const char *editor_program; > diff --git a/sha1_file.c b/sha1_file.c > index b114cc9..4b23bb8 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -36,6 +36,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; } > > const unsigned char null_sha1[20]; > > +static const char *no_log_pack_access = "no_log_pack_access"; > +static const char *log_pack_access; > + > /* > * This is meant to hold a *small* number of objects that you would > * want read_sha1_file() to be able to return, but yet you do not want > @@ -1956,12 +1959,19 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset) > { > static FILE *log_file; > > + if (!log_pack_access) > + log_pack_access = getenv("GIT_TRACE_PACK_ACCESS"); > + if (!log_pack_access) > + log_pack_access = no_log_pack_access; > + if (log_pack_access == no_log_pack_access) > + return; > + > if (!log_file) { > log_file = fopen(log_pack_access, "w"); > if (!log_file) { > error("cannot open pack access log '%s' for writing: %s", > log_pack_access, strerror(errno)); > - log_pack_access = NULL; > + log_pack_access = no_log_pack_access; > return; > } > } > @@ -1992,7 +2002,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset, > int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC; > int base_from_cache = 0; > > - if (log_pack_access) > + if (log_pack_access != no_log_pack_access) > write_pack_access_log(p, obj_offset); > > /* PHASE 1: drill down to the innermost base object */ > -- > 1.8.2.83.gc99314b > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Eric Sunshine, 2013-06-09 07:18 Subject: Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRK%2BmcPXpsUZSQH1hHx3E2Aq%2BS_44rV0sytLnGBHh%2B1xg%40mail.gmail.com In-Reply-To: <1370755369-30868-1-git-send-email-pclouds@gmail.com> ``` On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy wrote: > 5f44324 (core: log offset pack data accesses happened - 2011-07-06) > provides a way to observe pack access patterns via a config > switch. Setting an environment variable looks more obvious than a > config var, especially when you just need to _observe_, and more > inline with other tracing knobs we have. s/inline/in line/ (IMHO) Or: "..., and akin to other tracing knobs we have." > Document it as it may be useful for remote troubleshooting. > > Signed-off-by: Nguyễn Thái Ngọc Duy ``` ## Eric Sunshine, 2013-06-09 07:20 Subject: Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRiSqU%3DyTM05uFp7%2BbY-Q5Bn5S-dMeibV55Xb3x5j6qdA%40mail.gmail.com In-Reply-To: <1370757210-2994-1-git-send-email-pclouds@gmail.com> ``` On Sun, Jun 9, 2013 at 1:53 AM, Nguyễn Thái Ngọc Duy wrote: > "This can help with debugging object negotiation or other protocol > issues." s/"//g > Signed-off-by: Nguyễn Thái Ngọc Duy ``` ## Junio C Hamano, 2013-06-09 23:16 Subject: Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET Message-ID: <7v61xmoox5.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v61xmoox5.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130609055509.GA17617@sigill.intra.peff.net> ``` Thanks, both. Will queue both after re-reading 1/2. ```