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

[Jeff King <peff@peff.net>]

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.

-Peff