Re: [PATCH v4 2/2] http-fetch: redact url on die() message

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 9, 2021, 01:54 UTC
Message-ID: <211109.86ilx2dr5n.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqpmrnfmiv.fsf@gitster.g>

On Fri, Oct 29 2021, Junio C Hamano wrote:

Show 13 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +			if (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
>>> +				die("Unable to get pack file %s\n%s", preq->url,
>>> +				    curl_errorstr);
>>
>> small nit: arrange if's from "if (cheap || expensive)", i.e. no need for
>> getenv() if !nurl, but maybe compilers are smart enough for that...
>
> They typically do not see what happens inside git_env_bool() while
> compling this compilation unit, and cannot tell if the programmer
> wanted to call it first for its side effects, hence they cannot
> swap them safely.

*nod*, but since that function is just:
    
    int git_env_bool(const char *k, int def)
    {
            const char *v = getenv(k);
            return v ? git_config_bool(k, v) : def;
    }

I was hedging and pondering if some compilers were smart enough these days to optimize things like that.

I.e. in this case getenv() is a simple C library function, the env variable is constant, and we do a boolean test of it before calling git_config_bool().

So a sufficiently smart compiler could turn that into:

     /* global, probably something iterated over env already */
    static int __have_seen_GIT_TRACE_REDACT = 0;
    ...

    if ((!__have_seen_GIT_TRACE_REDACT || !nurl) ||
        (__have_seen_GIT_TRACE_REDACT && git_env_bool_without_v_bool_check(...)))

But probably not, since it wolud need quite a bit of C library cooperation/hooks...