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

[William Hubbs <williamh@gentoo.org>]

On Wed, Feb 06, 2019 at 01:26:12PM -0500, Jeff King wrote:
> On Wed, Feb 06, 2019 at 10:28:34AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > 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:
> > [...]
> >     @@ -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"),
> 
> Yeah, that would be OK with me. It's conceivable somebody ask "was the author
> ident sufficiently given", but given that 7 years have passed, it seems
> unlikely (and it's easy to resurrect it in the worst case).
> 
> But...
> 
> > 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 think that reintroduces some oddness. E.g., if I don't have any ident
> information set in config or the environment, and I do:
> 
>   GIT_AUTHOR_NAME=me GIT_AUTHOR_EMAIL=me@example.com git commit ...
> 
> that shouldn't count as "committer ident sufficiently given", and should
> still give a warning. So we wouldn't want to conflate them in a single
> flag (which is what d6991ceedc fixed). Curiously, though, I'm not sure
> you can trigger the problem through git-commit. It does call
> committer_ident_sufficiently_given(), but it never calls
> git_author_info(), where we set the flags!
> 
> Instead, it does its own parse via determine_author_info(), which does
> not bother to set the "explicit" flag at all. I suspect this could be
> refactored share more code with git_author_info() (which is what the
> plumbing commit-tree uses). But that's all a side note here.
> 
> There is one other call to check that the committer ident is
> sufficiently given, and that's in sequencer.c, when it prints a picked
> commit's info. That _might_ be triggerable (it doesn't call
> git_author_info() in that code path, but do_merge() does, so if the two
> happen in the same process, you'd not see the "Committer:" info line
> when you should).
> 
> So the bugs are minor and fairly unlikely. But I do think it's worth
> keeping the flags separate (even if we don't bother carrying an "author"
> one), just because it's an easy mistake to make.
> 
> An alternative view is that anybody who calls git_author_info() to
> create a commit _should_ be checking author_ident_sufficiently_given(),
> and it's a bug that they're not.
> 
> I.e., should we be doing something like this (and probably some other
> spots, too):
> 
> diff --git a/commit.c b/commit.c
> index a5333c7ac6..c99b311a48 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1419,8 +1419,11 @@ int commit_tree_extended(const char *msg, size_t msg_len,
>  	}
>  
>  	/* Person/date information */
> -	if (!author)
> +	if (!author) {
>  		author = git_author_info(IDENT_STRICT);
> +		if (!author_ident_sufficiently_given())
> +			warning("your author ident was auto-detected, etc...");
> +	}
>  	strbuf_addf(&buffer, "author %s\n", author);
>  	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
>  	if (!encoding_is_utf8)
> 
> I dunno. It seems pretty low priority, and nobody has even noticed after
> all these years. So I'm not sure if it's worth spending too much time on
> it.

Given this info (which came in while I was writing my last email), I
would rather see my v5 patch get in then we fix everything else later.

Junio, what do you think?

Thanks,

William