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

[Junio C Hamano <gitster@pobox.com>]

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

>  > 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.
>
>  My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it
>  was set to an empty string. All cases tested now.
>
> @@ -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) {

The first time, we will see the static empty string and come into
this block...

> +		log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
> +		if (log_pack_access && !*log_pack_access)
> +			log_pack_access = NULL;
> +		if (!log_pack_access)
> +			return;
> +	}

This may 

 (1) not find the env-var, in which case log_pack_access becomes
     NULL here, and the function returns.

 (2) find the env-var to be an empty string, in which case
     log_pack_access becomes NULL in the next statement, and the
     function returns.

 (3) find the env-var to be a non-empty string, and the function
     does not return.

And the next time around, (3) above may work fine; the first if()
will fail and we do not come in.  But in either (1) or (2), don't
you keep checking the environment every time you come here?

Oh, no, it is worse than that.  Either case you set the variable to
NULL, so the very first "if (!*log_pack_access)" would dereference
NULL.

Why not start from NULL:

    static const char *log_pack_access;

treat that NULL as "unknown" state, and avoid running getenv over
and over again by treating non-NULL value as "known"?  Perhaps like
this?

	if (!log_pack_access) {
        	/* First time: is env set? */
                log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
                if (!log_pack_access)
                	log_pack_access = "";
	}
        /* Now GIT_TRACE_PACK_ACCESS is known */
        if (!*log_pack_access)
        	return; /* not wanted */

As you can no longer rely on "config is read before we do anything
else" by switching to lazy env checking, your guard at the caller
needs to be updated, if you want to reduce unneeded call-return
overhead:

	if (!log_pack_access || *log_pack_access)
		write_pack_access_log(...);

But the guard is purely for performance, not correctness, because
the function now does the "return no-op if we know we are not
wanted" itself.

Also you no longer need to have an extern variable in environment.c