{"thread":{"id":"38158","subject":"Git commit amend empty emails","startedAt":"2014-12-10T13:17:35Z","lastAt":"2014-12-11T20:26:17Z","messageCount":7,"participants":["Simon","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"253499","messageId":"548847EF.7080805@gmail.com","threadId":"38158","inReplyTo":null,"subject":"Git commit amend empty emails","fromName":"Simon","fromEmail":"simonzack@gmail.com","sentAt":"2014-12-10T13:17:35Z","receivedAt":"2014-12-10T13:17:35Z","isPatch":false,"sender":{"key":"simonzack@gmail.com","avatar":null},"body":"Git is having empty email problems I think, I'm on git v2.1.3.\n\nSteps to reproduce:\n\n     $ git init\n     Initialized empty Git repository in /tmp/test_git/.git/\n     $ echo 'test' > abc\n     $ git add --all 1 ↵\n     $ git commit --message 'test'\n     [master (root-commit) 3cc2793] test\n      1 file changed, 1 insertion(+)\n      create mode 100644 abc\n     $ echo 'test2' > abc\n     $ git commit --amend\n     fatal: Malformed ident string: 'admin <> 1418217345 +0000'\n"},{"id":"253505","messageId":"20141210153952.GA14910@peff.net","threadId":"38158","inReplyTo":"548847EF.7080805@gmail.com","subject":"Re: Git commit amend empty emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T15:39:53Z","receivedAt":"2014-12-10T15:39:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 11, 2014 at 12:17:35AM +1100, Simon wrote:\n\n> Git is having empty email problems I think, I'm on git v2.1.3.\n> \n> Steps to reproduce:\n> \n>     $ git init\n>     Initialized empty Git repository in /tmp/test_git/.git/\n>     $ echo 'test' > abc\n>     $ git add --all 1 ↵\n>     $ git commit --message 'test'\n>     [master (root-commit) 3cc2793] test\n>      1 file changed, 1 insertion(+)\n>      create mode 100644 abc\n>     $ echo 'test2' > abc\n>     $ git commit --amend\n>     fatal: Malformed ident string: 'admin <> 1418217345 +0000'\n\nYes, this looks like a regression in v2.1.0, due to my 4701026 (commit:\nuse split_ident_line to compare author/committer, 2014-05-01).  That\ncommit added a call to sane_ident_split() when preparing the commit\nmessage template, which is what is causing the error you see (and why\nyour first commit succeeds, but the latter fails).\n\nThe absolute simplest fix would be to remove the sane_ident_split call.\nThough I have a different patch prepared, which argues that we should\nnot be doing any validation in this code path at all (see below).\n\nHowever, there's something else going on. I am surprised that we allow\nempty emails at all and the code here is quite strange. The first check\non the ident format is when we feed the data to fmt_ident to generate\nthe string that goes into the commit object.  We disallow empty _names_\nthere, but not empty _emails_.  I'm not sure if this is an oversight, or\nan intentional historic compatibility thing.\n\nAnd then it gets weirder. We take the output of fmt_ident, and then feed\nthat back into split_ident_line, so that we can set the GIT_AUTHOR_*\nvariables in the environment. And that does its _own_ set of checks, via\nsane_ident_split.\n\nOnce upon a time, it relied only on split_ident_lane to report problems.\nBut Junio's e27ddb6 (split_ident_line(): make best effort when parsing\nauthor/committer line, 2012-08-31) made split_ident_line more lenient,\nand introduced sane_ident_split to cover the difference. Except that it\ndid more than that: besides checking whether the name is empty (which\nthe original split_ident_line used to do), it also complains if the\nemail is empty (which is new in that commit).\n\nSo we now notice the empty email in this code path, but the only thing\nwe do is avoid writing out the environment variables and continue. Which\nmeans that the actual string generated by fmt_ident (complete with empty\nemail) is what goes into the commit. So why are we setting the\nenvironment variables at all?\n\nIt looks like they are for the benefit of hooks, as evidenced by 7dfe8ad\n(commit: pass author/committer info to hooks, 2012-03-11). So that means\nthat we are sometimes passing totally bogus data to hooks (i.e., an\nident is good enough to make it into a commit message, but\nsane_ident_split prevents us from putting the data into the\nenvironment).\n\nSo I think e27ddb6 was a little over-anxious in adding the email\nvalidation, but nobody noticed because those checks don't actually\naffect whether or not we commit. Hooks might see bogus data, but it is\nin such a rare set of cases that nobody has noticed.\n\nHere are two patches to improve this. These are on top of the\njk/commit-date-approxidate topic, as that is where the regression was\nintroduced.\n\nThe first one fixes the regression and can stand by itself. The second\nfixes the GIT_AUTHOR problem, but AFAIK that has been there for years.\nSo it is not as urgent, but is still maint-worthy, in my opinion.\n\n  [1/2]: commit: loosen ident checks when generating template\n  [2/2]: commit: always populate GIT_AUTHOR_* variables\n\nIf we did want to truly disallow empty emails, we could do a follow-on\n3/2 that teaches fmt_ident to reject them (that is the right place\nbecause it is where the validation checks for the author go, and also\nbecause we would probably want the same validation for the committer).\n\nBut I do not think we should do that lightly. It has been this way for\nyears, and clearly at least one person is depending on it. If we're\ngoing to change it, we might want a warning/deprecation period.\n\n-Peff\n"},{"id":"253506","messageId":"20141210154209.GA20771@peff.net","threadId":"38158","inReplyTo":"20141210153952.GA14910@peff.net","subject":"[PATCH 1/2] commit: loosen ident checks when generating template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T15:42:10Z","receivedAt":"2014-12-10T15:42:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we generate the commit-message template, we try to\nreport an author or committer ident that will be of interest\nto the user: an author that does not match the committer, or\na committer that was auto-configured.\n\nWhen doing so, if we encounter what we consider to be a\nbogus ident, we immediately die. This is a bad idea, because\nour use of the idents here is purely informational.  Any\nident rules should be enforced elsewhere, because commits\nthat do not invoke the editor will not even hit this code\npath (e.g., \"git commit -mfoo\" would work, but \"git commit\"\nwould not). So at best, we are redundant with other checks,\nand at worse, we actively prevent commits that should\notherwise be allowed.\n\nWe should therefore do the minimal parsing we can to get a\nvalue and not do any validation (i.e., drop the call to\nsane_ident_split()).\n\nIn theory we could notice when even our minimal parsing\nfails to work, and do the sane thing for each check (e.g.,\nif we have an author but can't parse the committer, assume\nthey are different and print the author). But we can\nactually simplify this even further.\n\nWe know that the author and committer strings we are parsing\nhave been generated by us earlier in the program, and\ntherefore they must be parseable. We could just call\nsplit_ident_line without even checking its return value,\nknowing that it will put _something_ in the name/mail\nfields. Of course, to protect ourselves against future\nchanges to the code, it makes sense to turn this into an\nassert, so we are not surprised if our assumption fails.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d1c90db..2be5506 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -502,6 +502,12 @@ static int is_a_merge(const struct commit *current_head)\n \treturn !!(current_head->parents && current_head->parents->next);\n }\n \n+static void assert_split_ident(struct ident_split *id, const struct strbuf *buf)\n+{\n+\tif (split_ident_line(id, buf->buf, buf->len))\n+\t\tdie(\"BUG: unable to parse our own ident: %s\", buf->buf);\n+}\n+\n static void export_one(const char *var, const char *s, const char *e, int hack)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -608,13 +614,6 @@ static void determine_author_info(struct strbuf *author_ident)\n \t}\n }\n \n-static void split_ident_or_die(struct ident_split *id, const struct strbuf *buf)\n-{\n-\tif (split_ident_line(id, buf->buf, buf->len) ||\n-\t    !sane_ident_split(id))\n-\t\tdie(_(\"Malformed ident string: '%s'\"), buf->buf);\n-}\n-\n static int author_date_is_interesting(void)\n {\n \treturn author_message || force_date;\n@@ -822,8 +821,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t\t\"%s\", only_include_assumed);\n \n-\t\tsplit_ident_or_die(&ai, author_ident);\n-\t\tsplit_ident_or_die(&ci, &committer_ident);\n+\t\t/*\n+\t\t * These should never fail because we they come from our own\n+\t\t * fmt_ident. They may fail the sane_ident test, but we know\n+\t\t * that the name and mail pointers will at least be valid,\n+\t\t * which is enough for our tests and printing here.\n+\t\t */\n+\t\tassert_split_ident(&ai, author_ident);\n+\t\tassert_split_ident(&ci, &committer_ident);\n \n \t\tif (ident_cmp(&ai, &ci))\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253507","messageId":"20141210154342.GB20771@peff.net","threadId":"38158","inReplyTo":"20141210153952.GA14910@peff.net","subject":"[PATCH 2/2] commit: always populate GIT_AUTHOR_* variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T15:43:42Z","receivedAt":"2014-12-10T15:43:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To figure out the author ident for a commit, we call\ndetermine_author_info(). This function collects information\nfrom the environment, other commits (in the case of\n\"--amend\" or \"-c/-C\"), and the \"--author\" option. It then\nuses fmt_ident to generate the final ident string that goes\ninto the commit object. fmt_ident is therefore responsible\nfor any quality or validation checks on what is allowed to\ngo into a commit.\n\nBefore returning, though, we call split_ident_line on the\nresult, and feed the individual components to hooks via the\nGIT_AUTHOR_* variables. Furthermore, we do extra validation\nby feeding the split to sane_ident_split(), which is pickier\nthan fmt_ident (in particular, it will complain about an empty\nemail field).  If this parsing or validation fails, we skip\nupdating the environment variables.\n\nThis is bad, because it means that hooks may silently see a\ndifferent ident than what we are putting into the commit. We\nshould drop the extra sane_ident_split checks entirely, and\ntake whatever fmt_ident has fed us (and what will go into\nthe commit object).\n\nIf parsing fails, we should actually abort here rather than\ncontinuing (and feeding the hooks bogus data). However,\nsplit_ident_line should never fail here. The ident was just\ngenerated by fmt_ident, so we know that it's sane. We can\nuse assert_split_ident to double-check this.\n\nNote that we also teach that assertion to check that we\nfound a date (it always should, but until now, no caller\ncared whether we found a date or not). Checking the return\nvalue of sane_ident_split is enough to ensure we have the\nname/email pointers set, and checking date_begin is enough\nto know that all of the date/tz variables are set.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c | 26 +++++---------------------\n 1 file changed, 5 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2be5506..f1a9e07 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -504,7 +504,7 @@ static int is_a_merge(const struct commit *current_head)\n \n static void assert_split_ident(struct ident_split *id, const struct strbuf *buf)\n {\n-\tif (split_ident_line(id, buf->buf, buf->len))\n+\tif (split_ident_line(id, buf->buf, buf->len) || !id->date_begin)\n \t\tdie(\"BUG: unable to parse our own ident: %s\", buf->buf);\n }\n \n@@ -518,20 +518,6 @@ static void export_one(const char *var, const char *s, const char *e, int hack)\n \tstrbuf_release(&buf);\n }\n \n-static int sane_ident_split(struct ident_split *person)\n-{\n-\tif (!person->name_begin || !person->name_end ||\n-\t    person->name_begin == person->name_end)\n-\t\treturn 0; /* no human readable name */\n-\tif (!person->mail_begin || !person->mail_end ||\n-\t    person->mail_begin == person->mail_end)\n-\t\treturn 0; /* no usable mail */\n-\tif (!person->date_begin || !person->date_end ||\n-\t    !person->tz_begin || !person->tz_end)\n-\t\treturn 0;\n-\treturn 1;\n-}\n-\n static int parse_force_date(const char *in, char *out, int len)\n {\n \tif (len < 1)\n@@ -606,12 +592,10 @@ static void determine_author_info(struct strbuf *author_ident)\n \t}\n \n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));\n-\tif (!split_ident_line(&author, author_ident->buf, author_ident->len) &&\n-\t    sane_ident_split(&author)) {\n-\t\texport_one(\"GIT_AUTHOR_NAME\", author.name_begin, author.name_end, 0);\n-\t\texport_one(\"GIT_AUTHOR_EMAIL\", author.mail_begin, author.mail_end, 0);\n-\t\texport_one(\"GIT_AUTHOR_DATE\", author.date_begin, author.tz_end, '@');\n-\t}\n+\tassert_split_ident(&author, author_ident);\n+\texport_one(\"GIT_AUTHOR_NAME\", author.name_begin, author.name_end, 0);\n+\texport_one(\"GIT_AUTHOR_EMAIL\", author.mail_begin, author.mail_end, 0);\n+\texport_one(\"GIT_AUTHOR_DATE\", author.date_begin, author.tz_end, '@');\n }\n \n static int author_date_is_interesting(void)\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253525","messageId":"xmqqmw6vgx6v.fsf@gitster.dls.corp.google.com","threadId":"38158","inReplyTo":"20141210153952.GA14910@peff.net","subject":"Re: Git commit amend empty emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T18:46:16Z","receivedAt":"2014-12-10T18:46:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> However, there's something else going on. I am surprised that we allow\n> empty emails at all and the code here is quite strange. The first check\n> on the ident format is when we feed the data to fmt_ident to generate\n> the string that goes into the commit object.  We disallow empty _names_\n> there, but not empty _emails_.  I'm not sure if this is an oversight, or\n> an intentional historic compatibility thing.\n\nLooking at e27ddb6 you cited, I think we knew about historical\nmistakes that allowed an empty names, but not an empty e-mail\naddress.  We probably have tried to kill both in one stone.\n\n> Once upon a time, it relied only on split_ident_lane to report problems.\n> But Junio's e27ddb6 (split_ident_line(): make best effort when parsing\n> author/committer line, 2012-08-31) made split_ident_line more lenient,\n> and introduced sane_ident_split to cover the difference. Except that it\n> did more than that: besides checking whether the name is empty (which\n> the original split_ident_line used to do), it also complains if the\n> email is empty (which is new in that commit).\n\n> So we now notice the empty email in this code path, but the only thing\n> we do is avoid writing out the environment variables and continue. Which\n> means that the actual string generated by fmt_ident (complete with empty\n> email) is what goes into the commit. So why are we setting the\n> environment variables at all?\n\nI think that part was more underthinking than oversight.\n\nWe didn't want to abort the commit but we didn't want to contaminate\nthe environment variables with known-to-be-bad values to spread the\nproblem further.  But there is no guarantee that not exporting the\nenvironment variables would give us more comformant name and e-mail\naddress, so that thinking is flawed.\n\n> Here are two patches to improve this. These are on top of the\n> jk/commit-date-approxidate topic, as that is where the regression was\n> introduced.\n>\n> The first one fixes the regression and can stand by itself. The second\n> fixes the GIT_AUTHOR problem, but AFAIK that has been there for years.\n> So it is not as urgent, but is still maint-worthy, in my opinion.\n>\n>   [1/2]: commit: loosen ident checks when generating template\n>   [2/2]: commit: always populate GIT_AUTHOR_* variables\n>\n> If we did want to truly disallow empty emails, we could do a follow-on\n> 3/2 that teaches fmt_ident to reject them (that is the right place\n> because it is where the validation checks for the author go, and also\n> because we would probably want the same validation for the committer).\n>\n> But I do not think we should do that lightly. It has been this way for\n> years, and clearly at least one person is depending on it. If we're\n> going to change it, we might want a warning/deprecation period.\n>\n> -Peff\n"},{"id":"253529","messageId":"20141210194501.GA22969@peff.net","threadId":"38158","inReplyTo":"xmqqmw6vgx6v.fsf@gitster.dls.corp.google.com","subject":"Re: Git commit amend empty emails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T19:45:01Z","receivedAt":"2014-12-10T19:45:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 10:46:16AM -0800, Junio C Hamano wrote:\n\n> > So we now notice the empty email in this code path, but the only thing\n> > we do is avoid writing out the environment variables and continue. Which\n> > means that the actual string generated by fmt_ident (complete with empty\n> > email) is what goes into the commit. So why are we setting the\n> > environment variables at all?\n> \n> I think that part was more underthinking than oversight.\n> \n> We didn't want to abort the commit but we didn't want to contaminate\n> the environment variables with known-to-be-bad values to spread the\n> problem further.  But there is no guarantee that not exporting the\n> environment variables would give us more comformant name and e-mail\n> address, so that thinking is flawed.\n\nThat sort of makes sense, but I agree it is flawed. The real spread is\nwhen the bogus data makes it into the commit objects themselves.\n\n> > The first one fixes the regression and can stand by itself. The second\n> > fixes the GIT_AUTHOR problem, but AFAIK that has been there for years.\n> > So it is not as urgent, but is still maint-worthy, in my opinion.\n> >\n> >   [1/2]: commit: loosen ident checks when generating template\n> >   [2/2]: commit: always populate GIT_AUTHOR_* variables\n\nBy the way, as I said I built these on the original regression. They\nwill have some minor textual conflicts if you merge them up to master,\ndue to the jk/commit-author-parsing topic. Please me know if you run\ninto any trouble merging.\n\n-Peff\n"},{"id":"253577","messageId":"CAPig+cTQBgDVoR-YG-WU_mJHR_NjdA6=5Lre_n4YMFawcSRwVw@mail.gmail.com","threadId":"38158","inReplyTo":"20141210154209.GA20771@peff.net","subject":"Re: [PATCH 1/2] commit: loosen ident checks when generating template","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-11T20:26:17Z","receivedAt":"2014-12-11T20:26:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 10, 2014 at 10:42 AM, Jeff King <peff@peff.net> wrote:\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index d1c90db..2be5506 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -822,8 +821,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>                         status_printf_ln(s, GIT_COLOR_NORMAL,\n>                                         \"%s\", only_include_assumed);\n>\n> -               split_ident_or_die(&ai, author_ident);\n> -               split_ident_or_die(&ci, &committer_ident);\n> +               /*\n> +                * These should never fail because we they come from our own\n\ns/we//\n\n> +                * fmt_ident. They may fail the sane_ident test, but we know\n> +                * that the name and mail pointers will at least be valid,\n> +                * which is enough for our tests and printing here.\n> +                */\n> +               assert_split_ident(&ai, author_ident);\n> +               assert_split_ident(&ci, &committer_ident);\n>\n>                 if (ident_cmp(&ai, &ci))\n>                         status_printf_ln(s, GIT_COLOR_NORMAL,\n> --\n> 2.2.0.454.g7eca6b7\n"}]}