Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> Hi Junio,
>
> On Wed, 26 Dec 2018, Junio C Hamano wrote:
>
>> Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx> 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.
OK, so the designed calling sequence of the new "prepare fallback
ident" is that any process that wants to use fallback ident must
call the "prepare" function _before_ making a call to any other
functions (IOW, it is a BUG() if things like git_committer_info() is
called before prepare_fallback_ident() gets called). Under that
condition, you are absolutely right that the two implementation
behaves differently.
That indeed indicates that this is unfriendly to future callers,
which was the main issue I had with the patch. A comment before the
prepare_fallback_ident() function to explain that would have helped.
Also the first condition checking bit in *given does not help---it
is quite misleading. It would have helped if it were
static void set_env_if( ... )
{
if (*given & bit)
BUG("%s was checked before prepare_fallback got called", key);
if (getenv(key))
return; /* nothing to do */
setenv(key, value, 1);
*given |= bit;
}
Because (author|committer)_ident_explicitly_given would never say
"yes, already" if the setting of fallback MUST be done before using
other API functions in ident.c, it we were to have a check for that
condition, it would be testing for a BUG(). And I wouldn't have
been confused by the code while reviewing.
>> > +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?
As you can see from this "For example,...", the review comment (and
the "simplified but does the wrong thing" version) was written under
an assumption different from the expected calling sequence the
posted version makes. What future writers of callers that use the
fallback ident feature must know is (unlike the above) that they
must call the "prepare" before they call git_ident_config() or
anything in ident.c. A comment before this function that explain
how and when in the program flow this is designed to be used is
needed.
Thanks for a clarification.
