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@xxxxxxxxxxxxxxxxxxxxx/):
> >
> > 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@xxxxxxxxxxx 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
