Re: [PATCH v12 04/26] ident: add the ability to provide a "fallback identity"

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 26, 2018, 21:21 UTC
Message-ID: <xmqqpntoq8zs.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <57a654887e652251ae966ec31b4604dc8222f9c6.1545331726.git.ungureanupaulsebastian@gmail.com>

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

Show 7 quoted lines

> +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.

Show 11 quoted lines

> +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)
>  {