Re: [PATCH v6 2/2] config: allow giving separate author and committer idents
- From
Jeff King <peff@peff.net>
- Date
- Feb 6, 2019, 18:26 UTC
- Message-ID
- <20190206182612.GA10231@sigill.intra.peff.net>
- In-Reply-To
- <87h8dhl0zh.fsf@evledraar.gmail.com>
On Wed, Feb 06, 2019 at 10:28:34AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
> 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...
Show 7 quoted lines
> 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