{"thread":{"id":"30609","subject":"t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","startedAt":"2012-05-24T12:01:57Z","lastAt":"2012-05-25T00:34:57Z","messageCount":16,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"192060","messageId":"4FBE2335.2090903@jpk.com","threadId":"30609","inReplyTo":null,"subject":"t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Michael Haggerty","fromEmail":"haggerty@jpk.com","sentAt":"2012-05-24T12:01:57Z","receivedAt":"2012-05-24T12:01:57Z","isPatch":false,"sender":{"key":"haggerty@jpk.com","avatar":null},"body":"On my setup, the above commit causes 12 tests in t4014 to fail.  For \nexample, test 25:\n\n> expecting success:\n>         check_threading expect.thread --thread master\n>\n> --- expect.thread       2012-05-24 11:57:26.841337237 +0000\n> +++ actual      2012-05-24 11:57:26.853337255 +0000\n> @@ -1,10 +1,10 @@\n>  ---\n> -Message-Id: <0>\n> +Message-Id: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n>  ---\n> -Message-Id: <1>\n> -In-Reply-To: <0>\n> -References: <0>\n> +Message-Id: <fd00575a8382ce27c62b83730a40bcff1dc2f25f.1337860646.git.mhagger@michael.(none)>\n> +In-Reply-To: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n> +References: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n>  ---\n> -Message-Id: <2>\n> -In-Reply-To: <0>\n> -References: <0>\n> +Message-Id: <18ed22aae56367787c36a882bd61281e07994f11.1337860646.git.mhagger@michael.(none)>\n> +In-Reply-To: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n> +References: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n> not ok - 25 thread\n\nBeyond bisecting, I haven't looked into this problem further.  Let me \nknow if you need more info.\n\nMichael\n\n-- \nMichael Haggerty\nHead of Software Development\n\nJPK Instruments AG\nBouchéstr. 12\n12435 Berlin, Germany\n\ntel:  +49 30 5331 12070\nfax:  +49 30 5331 22555\nmail: haggerty@jpk.com\nweb:  www.jpk.com, www.nanobioviews.net\n\nAufsichtsrat: Dr. Franz-Ferdinand von Falkenhausen (Vorsitzender)\nVorstand:     Torsten Jähnke, Frank Pelzer, Jörn Kamps, René Grünberg\n\nJPKinstruments Aktiengesellschaft\nAmtsgericht Berlin-Charlottenburg\nHRB 75513\n"},{"id":"192086","messageId":"20120524171640.GB3161@sigill.intra.peff.net","threadId":"30609","inReplyTo":"4FBE2335.2090903@jpk.com","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T17:16:40Z","receivedAt":"2012-05-24T17:16:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 02:01:57PM +0200, Michael Haggerty wrote:\n\n> On my setup, the above commit causes 12 tests in t4014 to fail.  For\n> example, test 25:\n> \n> >-Message-Id: <0>\n> >+Message-Id: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n\nThanks for the report. I know exactly what the issue is, as it came up\nin the discussion of the original series. 43ae9f47ab stopped using\ngit_committer_info (which looks at $GIT_COMMITTER_EMAIL) for the end of\nthe message-id and started using the default-generated email directly.\n\nNobody should care, because either:\n\n  1. The defaults set up a reasonable hostname for your machine.\n\n  2. They do not, but you adjust it by setting user.email. Otherwise,\n     your author ident would have this bogus email in it.\n\nThe only setup that _would_ care is if the generated default is bogus\nand you set $GIT_COMMITTER_EMAIL in the environment and relied on that\nto get a sane value. Which is exactly what the test environment does.\n\nThe question is, is what it is doing sane and something we should care\nabout? Or is the test broken (it fails to parse the message-id that\ncontains \".(none)\", but I am not even sure that is intentional and not\nsimply lazy regex writing in the test).\n\nI suspect that is not especially sane, but at the same time, it is not\nhard to support. The patch below (on top of jk/ident-gecos-strbuf)\nshould fix it.\n\n-- >8 --\nSubject: format-patch: use GIT_COMMITTER_EMAIL when making message ids\n\nBefore commit 43ae9f4, we generated the tail of a message id\nby calling git_committer_info and parsing the email out of\nthe result. 43ae9f4 changed to use ident_default_email\ndirectly, so we didn't have to bother with parsing. As a\nside effect, it meant we no longer used GIT_COMMITTER_EMAIL\nat all.\n\nIn general, this is probably reasonable behavior. Either the\ndefault email is sane on your system, or you are using\nuser.email to provide something sane. The exception is if\nyou rely on GIT_COMMITTER_EMAIL being set all the time to\noverride the bogus generated email.\n\nThis is unlikely to match anybody's real-life setup, but we\ndo use it in the test environment. And furthermore, it's\nwhat we have always done, and the change in 43ae9f4 was\nabout cleaning up, not fixing any bug; we should be\nconservative and keep the behavior identical.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that we check the environment outside of the usual strbuf_trim that\nhappens to the default email. And outside of fmt_ident, which trims\nwhitespace, as well. So compared to the state before this series,\nsomething like:\n\n  GIT_COMMITTER_EMAIL=\"$(printf 'foo@bar\\n')\" git format-patch ...\n\nis now broken. It also strikes me as a little ugly that this code path\nneeds to care about $GIT_COMMITTER_EMAIL at all. I can rework the ident\ninterface to provide a more sanitized broken-down version of the ident\nif we care.\n\n builtin/log.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8010a40..3f1883c 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -739,8 +739,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n static void gen_message_id(struct rev_info *info, char *base)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *email = getenv(\"GIT_COMMITTER_EMAIL\");\n+\tif (!email)\n+\t\temail = ident_default_email();\n \tstrbuf_addf(&buf, \"%s.%lu.git.%s\", base,\n-\t\t    (unsigned long) time(NULL), ident_default_email());\n+\t\t    (unsigned long) time(NULL), email);\n \tinfo->message_id = strbuf_detach(&buf, NULL);\n }\n \n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192105","messageId":"7vtxz5wdg8.fsf@alter.siamese.dyndns.org","threadId":"30609","inReplyTo":"20120524171640.GB3161@sigill.intra.peff.net","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-24T20:07:03Z","receivedAt":"2012-05-24T20:07:03Z","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> On Thu, May 24, 2012 at 02:01:57PM +0200, Michael Haggerty wrote:\n>\n>> On my setup, the above commit causes 12 tests in t4014 to fail.  For\n>> example, test 25:\n>> \n>> >-Message-Id: <0>\n>> >+Message-Id: <1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n>\n> Thanks for the report. I know exactly what the issue is, as it came up\n> in the discussion of the original series. 43ae9f47ab stopped using\n> git_committer_info (which looks at $GIT_COMMITTER_EMAIL) for the end of\n> the message-id and started using the default-generated email directly.\n>\n> Nobody should care, because either:\n>\n>   1. The defaults set up a reasonable hostname for your machine.\n>\n>   2. They do not, but you adjust it by setting user.email. Otherwise,\n>      your author ident would have this bogus email in it.\n>\n> The only setup that _would_ care is if the generated default is bogus\n> and you set $GIT_COMMITTER_EMAIL in the environment and relied on that\n> to get a sane value. Which is exactly what the test environment does.\n\nOr they worked to create their series in a good machine, pull it down to\nanother machine during their lunch break, and run format-patch to send it\nout after the final eyeballing.  Perhaps they are not supposed to be\nworking on the project in question during the day at work, so the work\nmachine does not have user.email set up correctly yet.\n\n> The question is, is what it is doing sane and something we should care\n> about? Or is the test broken (it fails to parse the message-id that\n> contains \".(none)\", but I am not even sure that is intentional and not\n> simply lazy regex writing in the test).\n\nI doubt that it was carefully written to try to exclude \".(none)\".\n\nIt somewhat curious---it seems to want to grab everything after \"<\" up to\nthe first occurrence of \">\"---why isn't this pattern matching?\n\n> This is unlikely to match anybody's real-life setup, but we do use it in\n> the test environment. And furthermore, it's what we have always done,\n> and the change in 43ae9f4 was about cleaning up, not fixing any bug; we\n> should be conservative and keep the behavior identical.\n\nI agree with the last sentence. I wouldn't be surprised if somebody was\nindexing patch e-mails on the list, keyed with their Message-Id: field,\nand using the key as something more than just a random \"uniqueness\" token.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Note that we check the environment outside of the usual strbuf_trim that\n> happens to the default email. And outside of fmt_ident, which trims\n> whitespace, as well. So compared to the state before this series,\n> something like:\n>\n>   GIT_COMMITTER_EMAIL=\"$(printf 'foo@bar\\n')\" git format-patch ...\n>\n> is now broken.\n\nTrue, and setting GIT_COMMITTER_EMAIL to that value will not break other\nuses because they trim.  But as we discussed before, we do not strongly\nstop users from deliberately adding unnecessary whitespaces around names\nand e-mails that they themselves configure (i.e. user.email and these\nenvironment variables), so it probably is not a huge deal.\n\n> It also strikes me as a little ugly that this code path\n> needs to care about $GIT_COMMITTER_EMAIL at all.\n\nDo you mean \"why committer and not author\"?  It primarily is because we\nwant to see \"who is this person who wants a unique token tied to his\nidentity\" and author and committer ident are both equally reasonable\nchoices.  But we have picked to use committer in these cases long time\nago.\n\nIf you mean \"why environment and not an API call?\", then I would have to\nagree.  ident_committer_email() call, that returns a sanitized version,\nwould have been a natural way to write this, if it were available.\n\n> I can rework the ident interface to provide a more sanitized broken-down\n> version of the ident if we care.\n\nThat is a healthy thing to do in the longer term, I would guess.\n\n>  builtin/log.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8010a40..3f1883c 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -739,8 +739,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n>  static void gen_message_id(struct rev_info *info, char *base)\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *email = getenv(\"GIT_COMMITTER_EMAIL\");\n> +\tif (!email)\n> +\t\temail = ident_default_email();\n>  \tstrbuf_addf(&buf, \"%s.%lu.git.%s\", base,\n> -\t\t    (unsigned long) time(NULL), ident_default_email());\n> +\t\t    (unsigned long) time(NULL), email);\n>  \tinfo->message_id = strbuf_detach(&buf, NULL);\n>  }\n"},{"id":"192107","messageId":"20120524201553.GA19990@sigill.intra.peff.net","threadId":"30609","inReplyTo":"7vtxz5wdg8.fsf@alter.siamese.dyndns.org","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T20:15:53Z","receivedAt":"2012-05-24T20:15:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 01:07:03PM -0700, Junio C Hamano wrote:\n\n> > The only setup that _would_ care is if the generated default is bogus\n> > and you set $GIT_COMMITTER_EMAIL in the environment and relied on that\n> > to get a sane value. Which is exactly what the test environment does.\n> \n> Or they worked to create their series in a good machine, pull it down to\n> another machine during their lunch break, and run format-patch to send it\n> out after the final eyeballing.  Perhaps they are not supposed to be\n> working on the project in question during the day at work, so the work\n> machine does not have user.email set up correctly yet.\n\nTrue. Although the chances that they have set GIT_COMMITTER_EMAIL in\ntheir environment seem unlikely in that case. In other words, it was\nbroken before, and it is broken now.\n\n> > The question is, is what it is doing sane and something we should care\n> > about? Or is the test broken (it fails to parse the message-id that\n> > contains \".(none)\", but I am not even sure that is intentional and not\n> > simply lazy regex writing in the test).\n> \n> I doubt that it was carefully written to try to exclude \".(none)\".\n> \n> It somewhat curious---it seems to want to grab everything after \"<\" up to\n> the first occurrence of \">\"---why isn't this pattern matching?\n\nI think it is even grosser than that. We follow-up that match with a\nsearch-and-replace using the message-ids we have found as regular\nexpressions, but we do not bother to quote them. So the ()\nmetacharacters get interpreted as regular expressions. I suspect\nsomething like this would fix it:\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b473b6d..3171c06 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -251,7 +251,7 @@ check_threading () {\n \t\t}\n \t\tif ($printing) {\n \t\t\t$h{$1}=$i++ if (/<([^>]+)>/ and !exists $h{$1});\n-\t\t\tfor $k (keys %h) {s/$k/$h{$k}/};\n+\t\t\tfor $k (keys %h) {s/\\Q$k\\E/$h{$k}/};\n \t\t\tprint;\n \t\t}\n \t\tprint \"---\\n\" if /^From /i;\n\n> > It also strikes me as a little ugly that this code path\n> > needs to care about $GIT_COMMITTER_EMAIL at all.\n> \n> Do you mean \"why committer and not author\"?  It primarily is because we\n> want to see \"who is this person who wants a unique token tied to his\n> identity\" and author and committer ident are both equally reasonable\n> choices.  But we have picked to use committer in these cases long time\n> ago.\n> \n> If you mean \"why environment and not an API call?\", then I would have to\n> agree.  ident_committer_email() call, that returns a sanitized version,\n> would have been a natural way to write this, if it were available.\n\nI meant the latter. There is no such call, but I can make one. Let me\nsee how awkward it is.\n\n-Peff\n"},{"id":"192120","messageId":"4FBE9EF3.9020902@alum.mit.edu","threadId":"30609","inReplyTo":"20120524171640.GB3161@sigill.intra.peff.net","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-24T20:49:55Z","receivedAt":"2012-05-24T20:49:55Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/24/2012 07:16 PM, Jeff King wrote:\n> On Thu, May 24, 2012 at 02:01:57PM +0200, Michael Haggerty wrote:\n>\n>> On my setup, the above commit causes 12 tests in t4014 to fail.  For\n>> example, test 25:\n>>\n>>> -Message-Id:<0>\n>>> +Message-Id:<1135adfeed86678c55e1aad7c568046ee8215660.1337860646.git.mhagger@michael.(none)>\n>\n> Thanks for the report. I know exactly what the issue is, as it came up\n> in the discussion of the original series. 43ae9f47ab stopped using\n> git_committer_info (which looks at $GIT_COMMITTER_EMAIL) for the end of\n> the message-id and started using the default-generated email directly.\n>\n> Nobody should care, because either:\n>\n>    1. The defaults set up a reasonable hostname for your machine.\n>\n>    2. They do not, but you adjust it by setting user.email. Otherwise,\n>       your author ident would have this bogus email in it.\n\nI'm trying hard not to get sucked into this topic (I just want the test \nsuite to work again!) but I infer that the reason for the failure in my \nsetup is that I have a global user.name but no global user.email \nconfigured.  I want git to remind me to configure user.email at the \nrepository level so that I can set my work email address for proprietary \nprojects and my personal email for open-source projects.\n\nIgnorant idea: since this test is executed in a test repo, would it help \nto set a dummy user.name and user.email at the test repository level \nusing \"git config\", perhaps as part of the standard test repo setup?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"192122","messageId":"20120524210227.GB21329@sigill.intra.peff.net","threadId":"30609","inReplyTo":"4FBE9EF3.9020902@alum.mit.edu","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T21:02:27Z","receivedAt":"2012-05-24T21:02:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 10:49:55PM +0200, Michael Haggerty wrote:\n\n> >Nobody should care, because either:\n> >\n> >   1. The defaults set up a reasonable hostname for your machine.\n> >\n> >   2. They do not, but you adjust it by setting user.email. Otherwise,\n> >      your author ident would have this bogus email in it.\n> \n> I'm trying hard not to get sucked into this topic (I just want the\n> test suite to work again!) but I infer that the reason for the\n> failure in my setup is that I have a global user.name but no global\n> user.email configured.\n\nNo, not at all. The problem is that the test suite does not look in your\n.gitconfig at all (nor should it), but rather than providing its own\nsensible gitconfig, it relies on the environment variables. Your\npersonal setup should not be relevant; only the fact that your machine\nhappens to not have a fully qualified hostname.\n\n> I want git to remind me to configure user.email at the repository\n> level so that I can set my work email address for proprietary projects\n> and my personal email for open-source projects.\n\nWhat you're doing is sane. However, I suspect that format-patch silently\ngenerates bogus message-ids when you do not have your user.email set\n(and it always has). I wonder if it is worth having it barf when\n\"(none)\" is in the email.\n\nFor that matter, I really wonder if the \"(none)\" fallback even makes\nsense these days. We encourage people to set up user.*. But I guess it\nhelps people on remote servers which write reflogs during a push; it is\nbetter to write junk into the reflog than to fail the push.\n\n> Ignorant idea: since this test is executed in a test repo, would it\n> help to set a dummy user.name and user.email at the test repository\n> level using \"git config\", perhaps as part of the standard test repo\n> setup?\n\nYes, that would solve your test failure. On the other hand, not having\nit has spurred more discussion of this situation, so maybe there is some\ngood in having it that way.\n\nAt any rate, there is a slight complication, because tests sometimes\nmake their own sub-repositories. A more sensible solution would be to\nput it in $HOME/.gitconfig, but that is also complicated. The tests have\n$HOME set to the repository directory, so the extra cruft there would\ncause some tests to fail (and splitting them into two directories would\nlikewise cause other tests to fail). That can be remedied, of course,\nbut it is definitely not a one-line fix.\n\nI think at this point I favor just fixing your bug, either with the\npatch I just posted, or a slightly nicer series refactoring fmt_ident to\nhandle this situation better (which I am working up right now).\n\n-Peff\n"},{"id":"192133","messageId":"20120524232515.GA11054@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524201553.GA19990@sigill.intra.peff.net","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:25:15Z","receivedAt":"2012-05-24T23:25:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 04:15:53PM -0400, Jeff King wrote:\n\n> > If you mean \"why environment and not an API call?\", then I would have to\n> > agree.  ident_committer_email() call, that returns a sanitized version,\n> > would have been a natural way to write this, if it were available.\n> \n> I meant the latter. There is no such call, but I can make one. Let me\n> see how awkward it is.\n\nHere it is.\n\n  [1/7]: ident: refactor empty ident error message\n\nThis one is only tangentially related. I was going to touch the message\nmore in patch 3, but decided not to (details in that patch).\n\n  [2/7]: ident: refactor NO_DATE flag in fmt_ident\n  [3/7]: ident: let callers omit name with fmt_indent\n  [4/7]: format-patch: use GIT_COMMITTER_EMAIL in message ids\n\nThese ones should fix Michael's failing test and restore the original\nbehavior.\n\n  [5/7]: ident: rename IDENT_ERROR_ON_NO_NAME to IDENT_STRICT\n  [6/7]: ident: reject bogus email addresses with IDENT_STRICT\n  [7/7]: format-patch: do not use bogus email addresses in message ids\n\nThese ones prevent bogus message ids from being generated at all\n(which is an improvement over the previous state).\n\n-Peff\n"},{"id":"192134","messageId":"20120524232632.GA11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 1/7] ident: refactor empty ident error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:26:32Z","receivedAt":"2012-05-24T23:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's on point in printing the name, since it is by\ndefinition the empty string if we have reached this code\npath. Instead, let's be more clear that we are complaining\nabout the empty name, but still show the email address that\nit is attached to (since that may provide some context to\nthe user).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs I mentioned in the cover letter, this one is optional. But I think it\nmakes sense.\n\n ident.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ident.c b/ident.c\nindex e279039..f5160e1 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -281,7 +281,7 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tif (error_on_no_name) {\n \t\t\tif (name == git_default_name.buf)\n \t\t\t\tfputs(env_hint, stderr);\n-\t\t\tdie(\"empty ident %s <%s> not allowed\", name, email);\n+\t\t\tdie(\"empty ident name (for <%s>) not allowed\", email);\n \t\t}\n \t\tpw = xgetpwuid_self();\n \t\tname = pw->pw_name;\n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192135","messageId":"20120524232650.GB11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 2/7] ident: refactor NO_DATE flag in fmt_ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:26:50Z","receivedAt":"2012-05-24T23:26:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As a short-hand, we extract this flag into the local\nvariable \"name_addr_only\". It's more accurate to simply\nnegate this and refer to it as \"want_date\", which will be\nless confusing when we add more NO_* flags.\n\nWhile we're touching this part of the code, let's move the\ncall to ident_default_date() only when we are actually going\nto use it, not when we have NO_DATE set, or when we get a\ndate from the environment.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust refactoring for the next patch...\n\n ident.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex f5160e1..59beef2 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -268,7 +268,7 @@ const char *fmt_ident(const char *name, const char *email,\n \tstatic struct strbuf ident = STRBUF_INIT;\n \tchar date[50];\n \tint error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);\n-\tint name_addr_only = (flag & IDENT_NO_DATE);\n+\tint want_date = !(flag & IDENT_NO_DATE);\n \n \tif (!name)\n \t\tname = ident_default_name();\n@@ -287,10 +287,13 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tname = pw->pw_name;\n \t}\n \n-\tstrcpy(date, ident_default_date());\n-\tif (!name_addr_only && date_str && date_str[0]) {\n-\t\tif (parse_date(date_str, date, sizeof(date)) < 0)\n-\t\t\tdie(\"invalid date format: %s\", date_str);\n+\tif (want_date) {\n+\t\tif (date_str && date_str[0]) {\n+\t\t\tif (parse_date(date_str, date, sizeof(date)) < 0)\n+\t\t\t\tdie(\"invalid date format: %s\", date_str);\n+\t\t}\n+\t\telse\n+\t\t\tstrcpy(date, ident_default_date());\n \t}\n \n \tstrbuf_reset(&ident);\n@@ -298,7 +301,7 @@ const char *fmt_ident(const char *name, const char *email,\n \tstrbuf_addstr(&ident, \" <\");\n \tstrbuf_addstr_without_crud(&ident, email);\n \tstrbuf_addch(&ident, '>');\n-\tif (!name_addr_only) {\n+\tif (want_date) {\n \t\tstrbuf_addch(&ident, ' ');\n \t\tstrbuf_addstr_without_crud(&ident, date);\n \t}\n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192136","messageId":"20120524232723.GC11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 3/7] ident: let callers omit name with fmt_indent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:27:24Z","receivedAt":"2012-05-24T23:27:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most callers want to see all of \"$name <$email> $date\", but\na few want only limited parts, omitting the date, or even\nthe name. We already have IDENT_NO_DATE to handle the date\npart, but there's not a good option for getting just the\nemail. Callers have to done one of:\n\n  1. Call ident_default_email; this does not respect\n     environment variables, nor does it promise to trim\n     whitespace or other crud from the result.\n\n  2. Call git_{committer,author}_info; this returns the name\n     and email, leaving the caller to parse out the wanted\n     bits.\n\nThis patch adds IDENT_NO_NAME; it stops short of adding\nIDENT_NO_EMAIL, as no callers want it (nor are likely to),\nand it complicates the error handling of the function.\n\nWhen no name is requested, the angle brackets (<>) around\nthe email address are also omitted.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI worked this up with IDENT_NO_EMAIL, too, but it really just made the\nfunction harder to read. And I doubt any callers are going to want it.\n\n cache.h |  1 +\n ident.c | 14 +++++++++-----\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0ac0d57..4a76c23 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -889,6 +889,7 @@ enum date_mode parse_date_format(const char *format);\n \n #define IDENT_ERROR_ON_NO_NAME 1\n #define IDENT_NO_DATE\t       2\n+#define IDENT_NO_NAME\t       4\n extern const char *git_author_info(int);\n extern const char *git_committer_info(int);\n extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);\ndiff --git a/ident.c b/ident.c\nindex 59beef2..8b5080d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -269,13 +269,14 @@ const char *fmt_ident(const char *name, const char *email,\n \tchar date[50];\n \tint error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);\n \tint want_date = !(flag & IDENT_NO_DATE);\n+\tint want_name = !(flag & IDENT_NO_NAME);\n \n-\tif (!name)\n+\tif (want_name && !name)\n \t\tname = ident_default_name();\n \tif (!email)\n \t\temail = ident_default_email();\n \n-\tif (!*name) {\n+\tif (want_name && !*name) {\n \t\tstruct passwd *pw;\n \n \t\tif (error_on_no_name) {\n@@ -297,10 +298,13 @@ const char *fmt_ident(const char *name, const char *email,\n \t}\n \n \tstrbuf_reset(&ident);\n-\tstrbuf_addstr_without_crud(&ident, name);\n-\tstrbuf_addstr(&ident, \" <\");\n+\tif (want_name) {\n+\t\tstrbuf_addstr_without_crud(&ident, name);\n+\t\tstrbuf_addstr(&ident, \" <\");\n+\t}\n \tstrbuf_addstr_without_crud(&ident, email);\n-\tstrbuf_addch(&ident, '>');\n+\tif (want_name)\n+\t\t\tstrbuf_addch(&ident, '>');\n \tif (want_date) {\n \t\tstrbuf_addch(&ident, ' ');\n \t\tstrbuf_addstr_without_crud(&ident, date);\n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192137","messageId":"20120524232825.GD11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 4/7] format-patch: use GIT_COMMITTER_EMAIL in message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:28:25Z","receivedAt":"2012-05-24T23:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Before commit 43ae9f4, we generated the tail of a message id\nby calling git_committer_info and parsing the email out of\nthe result. 43ae9f4 changed to use ident_default_email\ndirectly, so we didn't have to bother with parsing. As a\nside effect, it meant we no longer used GIT_COMMITTER_EMAIL\nat all.\n\nIn general, this is probably reasonable behavior. Either the\ndefault email is sane on your system, or you are using\nuser.email to provide something sane. The exception is if\nyou rely on GIT_COMMITTER_EMAIL being set all the time to\noverride the bogus generated email.\n\nThis is unlikely to match anybody's real-life setup, but we\ndo use it in the test environment. And furthermore, it's\nwhat we have always done, and the change in 43ae9f4 was\nabout cleaning up, not fixing any bug; we should be\nconservative and keep the behavior identical.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one should fix Michael's test failure. Let me know if it doesn't.\n\nArguably the call to ident_default_email() in http-push.c should be\nconverted in the same way. I'm unclear on how that value is actually\nused, so it may not matter at all.\n\n builtin/log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8010a40..4538309 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -740,7 +740,8 @@ static void gen_message_id(struct rev_info *info, char *base)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstrbuf_addf(&buf, \"%s.%lu.git.%s\", base,\n-\t\t    (unsigned long) time(NULL), ident_default_email());\n+\t\t    (unsigned long) time(NULL),\n+\t\t    git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE));\n \tinfo->message_id = strbuf_detach(&buf, NULL);\n }\n \n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192138","messageId":"20120524232840.GE11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 5/7] ident: rename IDENT_ERROR_ON_NO_NAME to IDENT_STRICT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:28:40Z","receivedAt":"2012-05-24T23:28:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Callers who ask for ERROR_ON_NO_NAME are not so much\nconcerned that the name will be blank (because, after all,\nwe will fall back to using the username), but rather it is a\ncheck to make sure that low-quality identities do not end up\nin things like commit messages or emails (whereas it is OK\nfor them to end up in things like reflogs).\n\nWhen future commits add more quality checks on the identity,\neach of these callers would want to use those checks, too.\nRather than modify each of them later to add a new flag,\nlet's refactor the flag.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c | 3 +--\n builtin/log.c    | 2 +-\n builtin/merge.c  | 4 ++--\n builtin/tag.c    | 2 +-\n builtin/var.c    | 4 ++--\n cache.h          | 2 +-\n commit.c         | 4 ++--\n gpg-interface.c  | 2 +-\n ident.c          | 6 +++---\n 9 files changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a2ec73d..f43eaaf 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -526,8 +526,7 @@ static void determine_author_info(struct strbuf *author_ident)\n \n \tif (force_date)\n \t\tdate = force_date;\n-\tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n-\t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\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\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);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4538309..d86bca3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1147,7 +1147,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (do_signoff) {\n \t\tconst char *committer;\n \t\tconst char *endpos;\n-\t\tcommitter = git_committer_info(IDENT_ERROR_ON_NO_NAME);\n+\t\tcommitter = git_committer_info(IDENT_STRICT);\n \t\tendpos = strchr(committer, '>');\n \t\tif (!endpos)\n \t\t\tdie(_(\"bogus committer info %s\"), committer);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 470fc57..dd50a0c 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1447,7 +1447,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\trefresh_cache(REFRESH_QUIET);\n \t\tif (allow_trivial && !fast_forward_only) {\n \t\t\t/* See if it is really trivial. */\n-\t\t\tgit_committer_info(IDENT_ERROR_ON_NO_NAME);\n+\t\t\tgit_committer_info(IDENT_STRICT);\n \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n \t\t\tif (!read_tree_trivial(common->item->object.sha1,\n \t\t\t\t\t       head_commit->object.sha1,\n@@ -1490,7 +1490,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n \n \t/* We are going to make a new commit. */\n-\tgit_committer_info(IDENT_ERROR_ON_NO_NAME);\n+\tgit_committer_info(IDENT_STRICT);\n \n \t/*\n \t * At this point, we need a real merge.  No matter what strategy\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 4fb6bd7..7b1be85 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -332,7 +332,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t  sha1_to_hex(object),\n \t\t\t  typename(type),\n \t\t\t  tag,\n-\t\t\t  git_committer_info(IDENT_ERROR_ON_NO_NAME));\n+\t\t\t  git_committer_info(IDENT_STRICT));\n \n \tif (header_len > sizeof(header_buf) - 1)\n \t\tdie(_(\"tag header too big.\"));\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 99d068a..aedbb53 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -11,7 +11,7 @@ static const char *editor(int flag)\n {\n \tconst char *pgm = git_editor();\n \n-\tif (!pgm && flag & IDENT_ERROR_ON_NO_NAME)\n+\tif (!pgm && flag & IDENT_STRICT)\n \t\tdie(\"Terminal is dumb, but EDITOR unset\");\n \n \treturn pgm;\n@@ -55,7 +55,7 @@ static const char *read_var(const char *var)\n \tval = NULL;\n \tfor (ptr = git_vars; ptr->read; ptr++) {\n \t\tif (strcmp(var, ptr->name) == 0) {\n-\t\t\tval = ptr->read(IDENT_ERROR_ON_NO_NAME);\n+\t\t\tval = ptr->read(IDENT_STRICT);\n \t\t\tbreak;\n \t\t}\n \t}\ndiff --git a/cache.h b/cache.h\nindex 4a76c23..06413e1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -887,7 +887,7 @@ unsigned long approxidate_careful(const char *, int *);\n unsigned long approxidate_relative(const char *date, const struct timeval *now);\n enum date_mode parse_date_format(const char *format);\n \n-#define IDENT_ERROR_ON_NO_NAME 1\n+#define IDENT_STRICT\t       1\n #define IDENT_NO_DATE\t       2\n #define IDENT_NO_NAME\t       4\n extern const char *git_author_info(int);\ndiff --git a/commit.c b/commit.c\nindex 9ed36c7..8248a99 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1154,9 +1154,9 @@ int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,\n \n \t/* Person/date information */\n \tif (!author)\n-\t\tauthor = git_author_info(IDENT_ERROR_ON_NO_NAME);\n+\t\tauthor = git_author_info(IDENT_STRICT);\n \tstrbuf_addf(&buffer, \"author %s\\n\", author);\n-\tstrbuf_addf(&buffer, \"committer %s\\n\", git_committer_info(IDENT_ERROR_ON_NO_NAME));\n+\tstrbuf_addf(&buffer, \"committer %s\\n\", git_committer_info(IDENT_STRICT));\n \tif (!encoding_is_utf8)\n \t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n \ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ab64a..0863c61 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -30,7 +30,7 @@ const char *get_signing_key(void)\n {\n \tif (configured_signing_key)\n \t\treturn configured_signing_key;\n-\treturn git_committer_info(IDENT_ERROR_ON_NO_NAME|IDENT_NO_DATE);\n+\treturn git_committer_info(IDENT_STRICT|IDENT_NO_DATE);\n }\n \n /*\ndiff --git a/ident.c b/ident.c\nindex 8b5080d..c42258f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -267,7 +267,7 @@ const char *fmt_ident(const char *name, const char *email,\n {\n \tstatic struct strbuf ident = STRBUF_INIT;\n \tchar date[50];\n-\tint error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);\n+\tint strict = (flag & IDENT_STRICT);\n \tint want_date = !(flag & IDENT_NO_DATE);\n \tint want_name = !(flag & IDENT_NO_NAME);\n \n@@ -279,7 +279,7 @@ const char *fmt_ident(const char *name, const char *email,\n \tif (want_name && !*name) {\n \t\tstruct passwd *pw;\n \n-\t\tif (error_on_no_name) {\n+\t\tif (strict) {\n \t\t\tif (name == git_default_name.buf)\n \t\t\t\tfputs(env_hint, stderr);\n \t\t\tdie(\"empty ident name (for <%s>) not allowed\", email);\n@@ -314,7 +314,7 @@ const char *fmt_ident(const char *name, const char *email,\n \n const char *fmt_name(const char *name, const char *email)\n {\n-\treturn fmt_ident(name, email, NULL, IDENT_ERROR_ON_NO_NAME | IDENT_NO_DATE);\n+\treturn fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);\n }\n \n const char *git_author_info(int flag)\n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192139","messageId":"20120524233237.GF11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 6/7] ident: reject bogus email addresses with IDENT_STRICT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:32:37Z","receivedAt":"2012-05-24T23:32:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we come up with a hostname like \"foo.(none)\" because the\nuser's machine is not fully qualified, we should reject this\nin strict mode (e.g., when we are making a commit object),\njust as we reject an empty gecos username.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that in conjunction with the previous patch, you can no longer \"git\ncommit\" with such a bogus address. I think this is a good thing.\n\nHowever, it's possible some old-timers might disagree. I remember Linus\na long time ago mentioning that using the machine-name in the committer\nline was a _feature_. These days he seems to set user.email to a real\naddress, though (and I think that is sane these days, because other\ntools really want to do use identities as more than just a token. E.g.,\nemail tools, gpg-signing, etc).\n\n ident.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/ident.c b/ident.c\nindex c42258f..ca098d9 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -288,6 +288,12 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tname = pw->pw_name;\n \t}\n \n+\tif (strict && email == git_default_email.buf &&\n+\t    !strstr(email, \"(none)\")) {\n+\t\tfputs(env_hint, stderr);\n+\t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n+\t}\n+\n \tif (want_date) {\n \t\tif (date_str && date_str[0]) {\n \t\t\tif (parse_date(date_str, date, sizeof(date)) < 0)\n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192140","messageId":"20120524233251.GG11084@sigill.intra.peff.net","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"[PATCH 7/7] format-patch: do not use bogus email addresses in message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T23:32:52Z","receivedAt":"2012-05-24T23:32:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can ask git_committer_info to be strict about coming up\nwith an email, which will die automatically on a poorly\nconfigured machine. This is better than letting invalid\nmessage-ids into the wild.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d86bca3..906dca4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -741,7 +741,7 @@ static void gen_message_id(struct rev_info *info, char *base)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstrbuf_addf(&buf, \"%s.%lu.git.%s\", base,\n \t\t    (unsigned long) time(NULL),\n-\t\t    git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE));\n+\t\t    git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE|IDENT_STRICT));\n \tinfo->message_id = strbuf_detach(&buf, NULL);\n }\n \n-- \n1.7.10.1.25.g7031a0f\n"},{"id":"192142","messageId":"7vipflunps.fsf@alter.siamese.dyndns.org","threadId":"30609","inReplyTo":"20120524232515.GA11054@sigill.intra.peff.net","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-25T00:08:15Z","receivedAt":"2012-05-25T00:08:15Z","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> On Thu, May 24, 2012 at 04:15:53PM -0400, Jeff King wrote:\n>\n>> > If you mean \"why environment and not an API call?\", then I would have to\n>> > agree.  ident_committer_email() call, that returns a sanitized version,\n>> > would have been a natural way to write this, if it were available.\n>> \n>> I meant the latter. There is no such call, but I can make one. Let me\n>> see how awkward it is.\n>\n> Here it is.\n>\n>   [1/7]: ident: refactor empty ident error message\n>\n> This one is only tangentially related. I was going to touch the message\n> more in patch 3, but decided not to (details in that patch).\n>\n>   [2/7]: ident: refactor NO_DATE flag in fmt_ident\n>   [3/7]: ident: let callers omit name with fmt_indent\n>   [4/7]: format-patch: use GIT_COMMITTER_EMAIL in message ids\n>\n> These ones should fix Michael's failing test and restore the original\n> behavior.\n>\n>   [5/7]: ident: rename IDENT_ERROR_ON_NO_NAME to IDENT_STRICT\n>   [6/7]: ident: reject bogus email addresses with IDENT_STRICT\n>   [7/7]: format-patch: do not use bogus email addresses in message ids\n>\n> These ones prevent bogus message ids from being generated at all\n> (which is an improvement over the previous state).\n\nAll looked pretty straightforward and cleanly done.\n\nWe might want to further tighten 6/7 to verify user-supplied (i.e. non\ndefault) e-mail for sanity, as I agree with the comment below --- lines of\nthat patch.\n\nAlso the check might want to be further tightened in the RFC 822/2822/5322\nsense, but getting it correct will open a huge can of worms; I think the\ncheck in 6/7 is a good place to stop, at least for now.\n\nThanks.\n"},{"id":"192144","messageId":"20120525003457.GA11300@sigill.intra.peff.net","threadId":"30609","inReplyTo":"7vipflunps.fsf@alter.siamese.dyndns.org","subject":"Re: t4014 broken by 43ae9f47ab: format-patch: use default email for generating message ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-25T00:34:57Z","receivedAt":"2012-05-25T00:34:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 05:08:15PM -0700, Junio C Hamano wrote:\n\n> >   [5/7]: ident: rename IDENT_ERROR_ON_NO_NAME to IDENT_STRICT\n> >   [6/7]: ident: reject bogus email addresses with IDENT_STRICT\n> >   [7/7]: format-patch: do not use bogus email addresses in message ids\n> >\n> > These ones prevent bogus message ids from being generated at all\n> > (which is an improvement over the previous state).\n> \n> All looked pretty straightforward and cleanly done.\n> \n> We might want to further tighten 6/7 to verify user-supplied (i.e. non\n> default) e-mail for sanity, as I agree with the comment below --- lines of\n> that patch.\n\nYeah. I'd be fine with more tightening, but I wanted to just catch the\nmost common uncontroversial problem in the initial round. We can build\non top later if we want.\n\n> Also the check might want to be further tightened in the RFC 822/2822/5322\n> sense, but getting it correct will open a huge can of worms; I think the\n> check in 6/7 is a good place to stop, at least for now.\n\nYeah, I considered that, but got nervous thinking about the same can of\nworms. It is not like people are complaining now, so I'd rather leave\nit.\n\n-Peff\n"}]}