[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 Documentation/git.txt |  7 +++++++
 cache.h               |  3 ---
 config.c              |  3 ---
 sha1_file.c           | 10 ++++++++++
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9e302b0..3e74440 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -832,6 +832,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 94ca1ac..9bfd76b 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 aefd80b..ce074d7 100644
--- a/config.c
+++ b/config.c
@@ -675,9 +675,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/sha1_file.c b/sha1_file.c
index 67e815b..7b47bdc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,6 +36,8 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
 
+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,6 +1958,14 @@ 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 = NULL;
+		if (!log_pack_access)
+			return;
+	}
+
 	if (!log_file) {
 		log_file = fopen(log_pack_access, "w");
 		if (!log_file) {
-- 
1.8.2.83.gc99314b

Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/sha1_file.c b/sha1_file.c
> index 67e815b..7b47bdc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -36,6 +36,8 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }
>  
>  const unsigned char null_sha1[20];
>  
> +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,6 +1958,14 @@ 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 = NULL;
> +		if (!log_pack_access)
> +			return;
> +	}

Have you ever tested this?

Once log_pack_access goes to NULL (e.g. when it sees the empty
string it was initialized to), this new test will happily
dereference NULL.

[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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

[PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Nguyễn Thái Ngọc Duy]

"This can help with debugging object negotiation or other protocol
issues."

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Jeff King]

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

[PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Nguyễn Thái Ngọc Duy]

"This can help with debugging object negotiation or other protocol
issues."

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 >> +'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

Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Jeff King]

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 <pclouds@gmail.com>
> ---
>  >> +'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

Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[Eric Sunshine]

On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>
> ---
>  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

Re: [PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

[Eric Sunshine]

On Sun, Jun 9, 2013 at 1:22 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>

Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Eric Sunshine]

On Sun, Jun 9, 2013 at 1:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> "This can help with debugging object negotiation or other protocol
> issues."

s/"//g

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Re: [PATCH 2/2] git.txt: document GIT_TRACE_PACKET

[Junio C Hamano]

Thanks, both.  Will queue both after re-reading 1/2.