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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 27, 2018, 21:24 UTC
Message-ID: <nycvar.QRO.7.76.6.1812272221380.45@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqpntoq8zs.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 26 Dec 2018, Junio C Hamano wrote:

Show 21 quoted lines

> Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> 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.

Indeed, this is the case where your version would actually do the wrong thing. Imagine that GIT_AUTHOR_NAME is set already. Your code would *override* it. But that is not what we want to do here. We want to *fall back* if there is no already-configured value.

And of course we won't set the `given` bit if we don't fall back here; that should be done somewhere else, where that environment variable (that we *refuse* to overwrite) is *actually* used.

Ciao, Dscho

Show 48 quoted lines

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