Paul-Sebastian Ungureanu <[email protected]> writes:
> +static void set_env_if(const char *key, const char *value, int *given, int
> bit)
> +{
> + if ((*given & bit) || getenv(key))
> + return; /* nothing to do */
> + setenv(key, value, 0);
> + *given |= bit;
> +}
We call setenv(3) with overwrite=0 but we protect the call with a
check for existing value with getenv(3), which feels a bit like an
anti-pattern. Wouldn't the following be simpler to follow, I wonder?
if (!(*given & bit)) {
setenv(key, value, 1);
*given |= bit;
}
The only case these two may behave differently is when '*given' does
not have the 'bit' set but the environment 'key' already exists.
The proposed patch will leave 'bit' in '*given' unset, so when a
later code says "let's see if author_ident is explicitly given, and
complain otherwise", such a check will trigger and cause complaint.
On the other hand, the simplified version does not allow the
"explicitly-given" bits to be left unset, so it won't cause
complaint.
Isn't it a BUG() if *given lacks 'bit' when the corresponding
environment variable 'key' is missing? IOW, I would understand
an implementation that is more elaborate than the simplified one I
just gave above were something like
if (!(*given & bit)) {
if (getenv(key))
BUG("why does %s exist and no %x bit set???", key, bit);
setenv(key, value, 0);
*given |= bit;
}
but I do not quite understand the reasoning behind the "check either
the bit, or the environment variable" in the proposed patch.
> +void prepare_fallback_ident(const char *name, const char *email)
> +{
> + set_env_if("GIT_AUTHOR_NAME", name,
> + &author_ident_explicitly_given, IDENT_NAME_GIVEN);
> + set_env_if("GIT_AUTHOR_EMAIL", email,
> + &author_ident_explicitly_given, IDENT_MAIL_GIVEN);
> + set_env_if("GIT_COMMITTER_NAME", name,
> + &committer_ident_explicitly_given, IDENT_NAME_GIVEN);
> + set_env_if("GIT_COMMITTER_EMAIL", email,
> + &committer_ident_explicitly_given, IDENT_MAIL_GIVEN);
> +}
Introducing this function alone without a caller and without
function doc is a bit unfriendly to future callers, who must be
careful when to call it, I think. For example, they must know that
it will be a disaster if they call this before they call
git_ident_config(), right?
> +
> static int buf_cmp(const char *a_begin, const char *a_end,
> const char *b_begin, const char *b_end)
> {
