Jeff King <peff@xxxxxxxx> writes: > However, there's something else going on. I am surprised that we allow > empty emails at all and the code here is quite strange. The first check > on the ident format is when we feed the data to fmt_ident to generate > the string that goes into the commit object. We disallow empty _names_ > there, but not empty _emails_. I'm not sure if this is an oversight, or > an intentional historic compatibility thing. Looking at e27ddb6 you cited, I think we knew about historical mistakes that allowed an empty names, but not an empty e-mail address. We probably have tried to kill both in one stone. > Once upon a time, it relied only on split_ident_lane to report problems. > But Junio's e27ddb6 (split_ident_line(): make best effort when parsing > author/committer line, 2012-08-31) made split_ident_line more lenient, > and introduced sane_ident_split to cover the difference. Except that it > did more than that: besides checking whether the name is empty (which > the original split_ident_line used to do), it also complains if the > email is empty (which is new in that commit). > So we now notice the empty email in this code path, but the only thing > we do is avoid writing out the environment variables and continue. Which > means that the actual string generated by fmt_ident (complete with empty > email) is what goes into the commit. So why are we setting the > environment variables at all? I think that part was more underthinking than oversight. We didn't want to abort the commit but we didn't want to contaminate the environment variables with known-to-be-bad values to spread the problem further. But there is no guarantee that not exporting the environment variables would give us more comformant name and e-mail address, so that thinking is flawed. > Here are two patches to improve this. These are on top of the > jk/commit-date-approxidate topic, as that is where the regression was > introduced. > > The first one fixes the regression and can stand by itself. The second > fixes the GIT_AUTHOR problem, but AFAIK that has been there for years. > So it is not as urgent, but is still maint-worthy, in my opinion. > > [1/2]: commit: loosen ident checks when generating template > [2/2]: commit: always populate GIT_AUTHOR_* variables > > If we did want to truly disallow empty emails, we could do a follow-on > 3/2 that teaches fmt_ident to reject them (that is the right place > because it is where the validation checks for the author go, and also > because we would probably want the same validation for the committer). > > But I do not think we should do that lightly. It has been this way for > years, and clearly at least one person is depending on it. If we're > going to change it, we might want a warning/deprecation period. > > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html