Re: [PATCH v6 2/2] config: allow giving separate author and committer idents

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Feb 05 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Feb 05 2019, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +static int set_ident_internal(const char *var, const char *value,
>>> +			    struct strbuf *sb, const int flag)
>>> +{
>>> +	if (!value)
>>> +		return config_error_nonbool(var);
>>> +	strbuf_reset(sb);
>>> +	strbuf_addstr(sb, value);
>>> +	author_ident_explicitly_given |= flag;
>>> +	ident_config_given |= flag;
>>> +	return 0;
>>> +}
>>> +
>>> +static int set_ident(const char *var, const char *value)
>>> +{
>>> +	if (!strcmp(var, "author.name"))
>>> +		return set_ident_internal(var, value, &git_author_name,
>>> +					  IDENT_NAME_GIVEN);
>>> +	else if (!strcmp(var, "author.email"))
>>> +		return set_ident_internal(var, value, &git_author_email,
>>> +					  IDENT_MAIL_GIVEN);
>>> +	else if (!strcmp(var, "committer.name"))
>>> +		return set_ident_internal(var, value, &git_committer_name,
>>> +					  IDENT_NAME_GIVEN);
>>> +	else if (!strcmp(var, "committer.email"))
>>> +		return set_ident_internal(var, value, &git_committer_email,
>>> +					  IDENT_MAIL_GIVEN);
>>> +	else if (!strcmp(var, "user.name"))
>>> +		return set_ident_internal(var, value, &git_default_name,
>>> +					  IDENT_NAME_GIVEN);
>>> +	else if (!strcmp(var, "user.email"))
>>> +		return set_ident_internal(var, value, &git_default_email,
>>> +					  IDENT_MAIL_GIVEN);
>>> +	return 0;
>>> +}
>>
>> In the v5 patch from William, author_ident_explicitly_given and
>> committer_ident_explicitly_given were set separately depending on
>> what variable was given (e.g. user.name marked both, author.name
>> marked only author but not committer_ident_explicitly_given).  In
>> the original before the addition of this feature with v6, giving
>> user.name would have set both, as we can see below.
>>
>> Is this change intended?
>>
>> Or did you find that committer_ident_explicitly_given is no longer
>> useful and the variable is not used anymore?
>
> No, that's a mistake of mine when porting this over, but also clearly a
> blindspot in our tests since they all pass with this.
>
> I haven't dug (don't have time right now) to check what the effect of
> that is. William?

I did some further digging. One of the confusing things is that we've
been carrying dead code since 2012 to set this
author_ident_explicitly_given variable. We can just apply this on top of
master:

     cache.h |  1 -
     ident.c | 13 -------------
     2 files changed, 14 deletions(-)

    diff --git a/cache.h b/cache.h
    index 038e3764a9..52308bd5e4 100644
    --- a/cache.h
    +++ b/cache.h
    @@ -1631,3 +1631,2 @@ extern int ignore_untracked_cache_config;
     extern int committer_ident_sufficiently_given(void);
    -extern int author_ident_sufficiently_given(void);

    diff --git a/ident.c b/ident.c
    index 33bcf40644..95fa2370e5 100644
    --- a/ident.c
    +++ b/ident.c
    @@ -22,3 +22,2 @@ static int ident_use_config_only;
     static int committer_ident_explicitly_given;
    -static int author_ident_explicitly_given;
     static int ident_config_given;
    @@ -169,3 +168,2 @@ const char *ident_default_email(void)
     			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
    -			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
     		} else if ((email = query_user_email()) && email[0]) {
    @@ -434,6 +432,2 @@ const char *git_author_info(int flag)
     {
    -	if (getenv("GIT_AUTHOR_NAME"))
    -		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
    -	if (getenv("GIT_AUTHOR_EMAIL"))
    -		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
     	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
    @@ -470,7 +464,2 @@ int committer_ident_sufficiently_given(void)

    -int author_ident_sufficiently_given(void)
    -{
    -	return ident_is_sufficient(author_ident_explicitly_given);
    -}
    -
     int git_ident_config(const char *var, const char *value, void *data)
    @@ -488,3 +477,2 @@ int git_ident_config(const char *var, const char *value, void *data)
     		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
    -		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
     		ident_config_given |= IDENT_NAME_GIVEN;
    @@ -499,3 +487,2 @@ int git_ident_config(const char *var, const char *value, void *data)
     		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
    -		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
     		ident_config_given |= IDENT_MAIL_GIVEN;

A more complete "fix" is to entirely revert Jeff's d6991ceedc ("ident:
keep separate "explicit" flags for author and committer",
2012-11-14). As he noted in 2012
(https://public-inbox.org/git/20121128182534.GA21020@sigill.intra.peff.net/):

    I do not know if anybody will ever care about the corner cases it
    fixes, so it is really just being defensive for future code.

I also found that the bug in my v6 is easily spotted & fixed. We just
grep stderr to see if we emit the "Your name and email were configured
automatically..." message. My patch v6 introduced a bug where we'd start
emitting that.

So it seems to me that the best way forward is to produce a series
where:

 1. We revert Jeff's 2012 patch (or not, Jeff?)

 2. Fix the tests so we test that given a combo of env vars & config
    create the expected commit objects *and* either emit the advice
    about having set stuff implicitly, or not.

    These need a lot of improvement, e.g. all our tests pass if I apply
    this:

        diff --git a/ident.c b/ident.c
        index 33bcf40644..f68e3c32ea 100644
        --- a/ident.c
        +++ b/ident.c
        @@ -167,6 +167,4 @@ const char *ident_default_email(void)
                        if (email && email[0]) {
                                strbuf_addstr(&git_default_email, email);
        -                       committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
        -                       author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
                        } else if ((email = query_user_email()) && email[0]) {
                                strbuf_addstr(&git_default_email, email);

 3. This {author,committer}.{name,email} feature on top of that.

William, is that something you're intererested in carrying forward? I
can also help if you want. Sorry your first contribution to git has
turned into this mess of re-rolls, as often happens we find that when
trying to tweak something that the existing behavior doesn't have any
tests.

I think it's worth spending a bit more time here to prove to ourselves
that the changes aren't e.g. spamming users with the ident advice in
cases where we don't want that, but maybe everyone else is tired of this
and we should just take your v5 and fix the other stuff later. I'll
leave that up to you / Junio to decide.

>>>  int git_ident_config(const char *var, const char *value, void *data)
>>>  {
>>>  	if (!strcmp(var, "user.useconfigonly")) {
>>> @@ -480,29 +551,7 @@ int git_ident_config(const char *var, const char *value, void *data)
>>>  		return 0;
>>>  	}
>>>
>>> -	if (!strcmp(var, "user.name")) {
>>> -		if (!value)
>>> -			return config_error_nonbool(var);
>>> -		strbuf_reset(&git_default_name);
>>> -		strbuf_addstr(&git_default_name, value);
>>> -		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
>>> -		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
>>> -		ident_config_given |= IDENT_NAME_GIVEN;
>>> -		return 0;
>>> -	}
>>> -
>>> -	if (!strcmp(var, "user.email")) {
>>> -		if (!value)
>>> -			return config_error_nonbool(var);
>>> -		strbuf_reset(&git_default_email);
>>> -		strbuf_addstr(&git_default_email, value);
>>> -		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>>> -		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
>>> -		ident_config_given |= IDENT_MAIL_GIVEN;
>>> -		return 0;
>>> -	}
>>> -
>>> -	return 0;
>>> +	return set_ident(var, value);
>>>  }