{"thread":{"id":"32079","subject":"[PATCH] send-email: add proper default sender","startedAt":"2012-11-11T17:06:50Z","lastAt":"2012-11-16T20:11:08Z","messageCount":76,"participants":["Felipe Contreras","Ramkumar Ramachandra","Jeff King","Junio C Hamano","Erik Faye-Lund","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202893","messageId":"1352653610-2090-1-git-send-email-felipe.contreras@gmail.com","threadId":"32079","inReplyTo":null,"subject":"[PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-11T17:06:50Z","receivedAt":"2012-11-11T17:06:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no point in asking this over and over if the user already\nproperly configured his/her name and email.\n\nSigned-off-by: Felipe Contreras \n---\n\nI got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n\n git-send-email.perl | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex aea66a0..65b8328 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -748,6 +748,17 @@ if (!$force) {\n \t}\n }\n \n+my $name = Git::config('user.name');\n+my $email = Git::config('user.email');\n+\n+if (defined $email) {\n+\tif (defined $name) {\n+\t\t$sender = sprintf(\"%s <%s>\", $name, $email);\n+\t} else {\n+\t\t$sender = $email;\n+\t}\n+}\n+\n my $prompting = 0;\n if (!defined $sender) {\n \t$sender = $repoauthor || $repocommitter || '';\n-- \n1.8.0\n"},{"id":"202895","messageId":"CALkWK0kvScgtqwCLA2BPZxs5qc-ZVyZPiCu6BHrqjne-Yfr8-A@mail.gmail.com","threadId":"32079","inReplyTo":"1352653610-2090-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-11T17:12:58Z","receivedAt":"2012-11-11T17:12:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n\nUse sendemail.from. The email sender doesn't necessarily have to be the author.\n\nRam\n"},{"id":"202905","messageId":"CAMP44s1iNFcN6LFhpFTR0cuTcytOXDSof8H+bPN1_0ebGandBw@mail.gmail.com","threadId":"32079","inReplyTo":"CALkWK0kvScgtqwCLA2BPZxs5qc-ZVyZPiCu6BHrqjne-Yfr8-A@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-11T18:06:28Z","receivedAt":"2012-11-11T18:06:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Nov 11, 2012 at 6:12 PM, Ramkumar Ramachandra\n wrote:\n> Felipe Contreras wrote:\n>> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n>\n> Use sendemail.from. The email sender doesn't necessarily have to be the author.\n\nAnd when it's not the author, then sendemail.from would be used.\n\n% git config user.email felipe.contreras@nokia.com\n% git send-email master\nFrom: Felipe Contreras \n\n% git config user.email felipe.contreras@gmail.com\n% git send-email.perl master\nFrom: Felipe Contreras \n\n% git send-email --from=foo@bar.com master\nFrom: foo@bar.com\n\n% git config sendemail.from test@example.com\n% git send-email master\nFrom: test@example.com\n\nWhat do you loose with this code? Nothing. What do you gain by asking\nthe user every time: \"Who should the emails appear to be from?\", when\nthe default GIT_AUTHOR_IDENT is already fine? Nothing.\n\nThe problem with sendemail.from, is that each time the user needs to\nchange email address, it has to be done in multiple places:\nuser.email, and sendemail.from. There's no need for that.\n\nBut I screwed the patch, it should be:\n\n+if (!defined $sender) {\n+ my $name = Git::config('user.name');\n+ my $email = Git::config('user.email');\n+\n+ if (defined $email) {\n+ if (defined $name) {\n+ $sender = sprintf(\"%s <%s>\", $name, $email);\n+ } else {\n+ $sender = $email;\n+ }\n+ }\n+}\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203020","messageId":"20121112233546.GG10531@sigill.intra.peff.net","threadId":"32079","inReplyTo":"1352653610-2090-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-12T23:35:46Z","receivedAt":"2012-11-12T23:35:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:\n\n> There's no point in asking this over and over if the user already\n> properly configured his/her name and email.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> \n> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n\nIt should be defaulting to your regular git ident, and you just have to\nhit enter, right?\n\nI think it's probably reasonable to skip that \"enter\" in most cases. But\nI'm not sure why we ever asked in the first place. What do people input\nthere if they are not taking the default?\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index aea66a0..65b8328 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -748,6 +748,17 @@ if (!$force) {\n> \t}\n> }\n> \n> +my $name = Git::config('user.name');\n> +my $email = Git::config('user.email');\n> +\n> +if (defined $email) {\n> +\tif (defined $name) {\n> +\t\t$sender = sprintf(\"%s <%s>\", $name, $email);\n> +\t} else {\n> +\t\t$sender = $email;\n> +\t}\n> +}\n\nWhy not use Git::ident_person() here? It saves some code, and would also\nrespect environment variables. Or better yet...\n\n> my $prompting = 0;\n> if (!defined $sender) {\n> \t$sender = $repoauthor || $repocommitter || '';\n\nWhy not just use $repoauthor or $repocommitter, as the prompt default\nalready does?\n\n-Peff\n"},{"id":"203023","messageId":"CAMP44s0emar-C27SX-FDsUVB6Sevuy4fBFHuO2OD6xELCEjmGg@mail.gmail.com","threadId":"32079","inReplyTo":"20121112233546.GG10531@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-12T23:42:02Z","receivedAt":"2012-11-12T23:42:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 12:35 AM, Jeff King wrote:\n> On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:\n>\n>> There's no point in asking this over and over if the user already\n>> properly configured his/her name and email.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>>\n>> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n>\n> It should be defaulting to your regular git ident, and you just have to\n> hit enter, right?\n\nYes.\n\n> I think it's probably reasonable to skip that \"enter\" in most cases. But\n> I'm not sure why we ever asked in the first place. What do people input\n> there if they are not taking the default?\n\nBeats me.\n\n> Why not use Git::ident_person() here? It saves some code, and would also\n> respect environment variables. Or better yet...\n\nI assume there was a reason why that code was asking for input;\nprecisely because it would use the environment variables. For some\nreason the user might have exported GIT_AUTHOR_EMAIL, or maybe EMAIL\nis not right, or the full name config.\n\nOTOH user.name/.email configurations come clearly from the user.\n\n>> my $prompting = 0;\n>> if (!defined $sender) {\n>> $sender = $repoauthor || $repocommitter || '';\n>\n> Why not just use $repoauthor or $repocommitter, as the prompt default\n> already does?\n\nSee above.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203024","messageId":"20121113000217.GH10531@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s0emar-C27SX-FDsUVB6Sevuy4fBFHuO2OD6xELCEjmGg@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T00:02:17Z","receivedAt":"2012-11-13T00:02:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 12:42:02AM +0100, Felipe Contreras wrote:\n\n> > Why not use Git::ident_person() here? It saves some code, and would also\n> > respect environment variables. Or better yet...\n> \n> I assume there was a reason why that code was asking for input;\n> precisely because it would use the environment variables. For some\n> reason the user might have exported GIT_AUTHOR_EMAIL, or maybe EMAIL\n> is not right, or the full name config.\n> \n> OTOH user.name/.email configurations come clearly from the user.\n\nBut we use the environment to default the field, so the distinction\ndoesn't make much sense to me. Plus, it has always been the case that\nyou can use git without setting user.*, but instead only using the\nenvironment. I don't see any reason not to follow that principle here,\ntoo.\n\nThe one distinction that would make sense to me is pausing to ask when\nwe use \"implicit\" methods to look up the ident, like concatenating the\nusername with the hostname to get the email.\n\nGit::ident uses \"git var\" to do its lookup, which will use IDENT_STRICT;\nthat stops most junk like empty names and bogus domains. But I think we\nwould want to go one step further and actually check\nuser_ident_sufficiently_given. Unfortunately, that is not currently\navailable outside of C. You'd probably want something like:\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex aedbb53..eaf324e 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -26,6 +26,12 @@ static const char *pager(int flag)\n \treturn pgm;\n }\n \n+static const char *explicit_ident(int flag)\n+{\n+\tgit_committer_info(flag);\n+\treturn user_ident_sufficiently_given() ? \"1\" : \"0\";\n+}\n+\n struct git_var {\n \tconst char *name;\n \tconst char *(*read)(int);\n@@ -35,6 +41,7 @@ static struct git_var git_vars[] = {\n \t{ \"GIT_AUTHOR_IDENT\", git_author_info },\n \t{ \"GIT_EDITOR\", editor },\n \t{ \"GIT_PAGER\", pager },\n+\t{ \"GIT_EXPLICIT_IDENT\", explicit_ident },\n \t{ \"\", NULL },\n };\n\n-Peff\n"},{"id":"203025","messageId":"20121113000637.GI10531@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113000217.GH10531@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T00:06:37Z","receivedAt":"2012-11-13T00:06:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 07:02:17PM -0500, Jeff King wrote:\n\n> The one distinction that would make sense to me is pausing to ask when\n> we use \"implicit\" methods to look up the ident, like concatenating the\n> username with the hostname to get the email.\n\nBy the way, I suspect this is the answer to \"what do people type for\nthis prompt\". It is probably more about a safety on bad ident than it is\nabout people routinely updating the information. I actually think it\nwould make more sense to drop the prompt entirely and just die when the\nuser has not given us a usable ident. But maybe people who do one-off\nformat-patches would rather type their name in a prompt than set an\nenvironment variable and re-run the program.\n\n-Peff\n"},{"id":"203029","messageId":"CAMP44s16y9WSmnTdb04EMSzXVgzfYP7pSMo6qZi0HY0bjouA0w@mail.gmail.com","threadId":"32079","inReplyTo":"20121113000217.GH10531@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T00:54:59Z","receivedAt":"2012-11-13T00:54:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 1:02 AM, Jeff King wrote:\n> On Tue, Nov 13, 2012 at 12:42:02AM +0100, Felipe Contreras wrote:\n>\n>> > Why not use Git::ident_person() here? It saves some code, and would also\n>> > respect environment variables. Or better yet...\n>>\n>> I assume there was a reason why that code was asking for input;\n>> precisely because it would use the environment variables. For some\n>> reason the user might have exported GIT_AUTHOR_EMAIL, or maybe EMAIL\n>> is not right, or the full name config.\n>>\n>> OTOH user.name/.email configurations come clearly from the user.\n>\n> But we use the environment to default the field, so the distinction\n> doesn't make much sense to me. Plus, it has always been the case that\n> you can use git without setting user.*, but instead only using the\n> environment. I don't see any reason not to follow that principle here,\n> too.\n\nAnd that's why a lot of commits end up like michael\n.\n\n> The one distinction that would make sense to me is pausing to ask when\n> we use \"implicit\" methods to look up the ident, like concatenating the\n> username with the hostname to get the email.\n>\n> Git::ident uses \"git var\" to do its lookup, which will use IDENT_STRICT;\n> that stops most junk like empty names and bogus domains. But I think we\n> would want to go one step further and actually check\n> user_ident_sufficiently_given. Unfortunately, that is not currently\n> available outside of C. You'd probably want something like:\n>\n> diff --git a/builtin/var.c b/builtin/var.c\n> index aedbb53..eaf324e 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -26,6 +26,12 @@ static const char *pager(int flag)\n> return pgm;\n> }\n>\n> +static const char *explicit_ident(int flag)\n> +{\n> + git_committer_info(flag);\n> + return user_ident_sufficiently_given() ? \"1\" : \"0\";\n> +}\n> +\n> struct git_var {\n> const char *name;\n> const char *(*read)(int);\n> @@ -35,6 +41,7 @@ static struct git_var git_vars[] = {\n> { \"GIT_AUTHOR_IDENT\", git_author_info },\n> { \"GIT_EDITOR\", editor },\n> { \"GIT_PAGER\", pager },\n> + { \"GIT_EXPLICIT_IDENT\", explicit_ident },\n> { \"\", NULL },\n> };\n\nProbably. But what I really want is to stop 'git send-email' from\nasking. I think the one next step further can be done later.\n\n-- \nFelipe Contreras\n"},{"id":"203030","messageId":"7v1ufygw3y.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121113000637.GI10531@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T00:55:13Z","receivedAt":"2012-11-13T00:55:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 12, 2012 at 07:02:17PM -0500, Jeff King wrote:\n>\n>> The one distinction that would make sense to me is pausing to ask when\n>> we use \"implicit\" methods to look up the ident, like concatenating the\n>> username with the hostname to get the email.\n>\n> By the way, I suspect this is the answer to \"what do people type for\n> this prompt\". It is probably more about a safety on bad ident than it is\n> about people routinely updating the information. I actually think it\n> would make more sense to drop the prompt entirely and just die when the\n> user has not given us a usable ident.\n\nYeah, I agree with pretty much everything you said in this thread\n(including that environment and config are equally likely to reflect\nuser's wish and it does not make much sense to treat environment as\nmore suspicious).\n\n> But maybe people who do one-off format-patches would rather type\n> their name in a prompt than set an environment variable and re-run\n> the program.\n\ns/one-off format-patches/one-off send-email/. I think dying will\nforce them to configure their names once (so that later invocations\ndo not have to stop) while prompting will force them to type their\nnames every time, so the current behaviour is probably a false\neconomy. As long as we caution users in the release notes, it\nprobably is OK to change the command to die.\n"},{"id":"203036","messageId":"20121113032727.GA8387@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s16y9WSmnTdb04EMSzXVgzfYP7pSMo6qZi0HY0bjouA0w@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T03:27:27Z","receivedAt":"2012-11-13T03:27:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:\n\n> > But we use the environment to default the field, so the distinction\n> > doesn't make much sense to me. Plus, it has always been the case that\n> > you can use git without setting user.*, but instead only using the\n> > environment. I don't see any reason not to follow that principle here,\n> > too.\n> \n> And that's why a lot of commits end up like michael\n> .\n\nNo, it's not. Those broken names do not come from the environment, but\nfrom our last-resort guess of the hostname. We long ago switched to\nprinting the name as a warning when we have made such a guess (bb1ae3f),\nthen more recently started rejecting them outright (8c5b1ae).\n\nAnd I have proposed exactly the same behavior here: respect the\nenvironment and the config, but do not trust the implicit guesses.\n\n> Probably. But what I really want is to stop 'git send-email' from\n> asking. I think the one next step further can be done later.\n\nBut in the meantime you are causing a regression for anybody who expects\nGIT_AUTHOR_NAME to override user.email when running git-send-email (and\nyou have taken away the prompt that they could have used to notice and\ncorrect it).\n\n-Peff\n"},{"id":"203037","messageId":"20121113034010.GA8879@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113032727.GA8387@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T03:40:10Z","receivedAt":"2012-11-13T03:40:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 10:27:27PM -0500, Jeff King wrote:\n\n> On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:\n> \n> > > But we use the environment to default the field, so the distinction\n> > > doesn't make much sense to me. Plus, it has always been the case that\n> > > you can use git without setting user.*, but instead only using the\n> > > environment. I don't see any reason not to follow that principle here,\n> > > too.\n> > \n> > And that's why a lot of commits end up like michael\n> > .\n> \n> No, it's not. Those broken names do not come from the environment, but\n> from our last-resort guess of the hostname. We long ago switched to\n> printing the name as a warning when we have made such a guess (bb1ae3f),\n> then more recently started rejecting them outright (8c5b1ae).\n> \n> And I have proposed exactly the same behavior here: respect the\n> environment and the config, but do not trust the implicit guesses.\n\nRe-reading this, I think \"them\" at the end of the second paragraph is\nslightly unclear. Let me rephrase.\n\nThe lack of a name or the presence of an obviously bogus email address\n(e.g., with \"(none)\") is disallowed by commit. We still allow implicit\nidents on commit as long they are not obviously wrong, but show them to\nthe user so that they can notice and correct via \"commit --amend\".\n\nSo if it dies on an implicit ident, send-email would actually be taking\nan even stronger stance against bogus idents. Which makes sense, since\nthere is no \"--amend\" for fixing a broken email that has been sent.\n\n-Peff\n"},{"id":"203041","messageId":"CAMP44s0SDHVzSd-8Rq7Z1sbiQ6m0pxX+2pgx16_DoWnHeyNsNQ@mail.gmail.com","threadId":"32079","inReplyTo":"20121113032727.GA8387@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T03:55:25Z","receivedAt":"2012-11-13T03:55:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 4:27 AM, Jeff King wrote:\n> On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:\n>\n>> > But we use the environment to default the field, so the distinction\n>> > doesn't make much sense to me. Plus, it has always been the case that\n>> > you can use git without setting user.*, but instead only using the\n>> > environment. I don't see any reason not to follow that principle here,\n>> > too.\n>>\n>> And that's why a lot of commits end up like michael\n>> .\n>\n> No, it's not. Those broken names do not come from the environment, but\n> from our last-resort guess of the hostname.\n\nThat depends how you define environment, but fine, the point is that it happens.\n\n> We long ago switched to\n> printing the name as a warning when we have made such a guess (bb1ae3f),\n> then more recently started rejecting them outright (8c5b1ae).\n\nRight, but these would still happen:\n\nmichael \n\n>> Probably. But what I really want is to stop 'git send-email' from\n>> asking. I think the one next step further can be done later.\n>\n> But in the meantime you are causing a regression for anybody who expects\n> GIT_AUTHOR_NAME to override user.email when running git-send-email (and\n> you have taken away the prompt that they could have used to notice and\n> correct it).\n\nI think they can survive. If anybody like this exists.\n\n-- \nFelipe Contreras\n"},{"id":"203042","messageId":"20121113040104.GA9361@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s0SDHVzSd-8Rq7Z1sbiQ6m0pxX+2pgx16_DoWnHeyNsNQ@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T04:01:04Z","receivedAt":"2012-11-13T04:01:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 04:55:25AM +0100, Felipe Contreras wrote:\n\n> > No, it's not. Those broken names do not come from the environment, but\n> > from our last-resort guess of the hostname.\n> \n> That depends how you define environment, but fine, the point is that\n> it happens.\n\nIf you have a strawman definition that does not have anything to do with\nwhat I said in my original email, then yes, it could happen. But as I\nsaid already, \"git var\" uses IDENT_STRICT and will not allow such broken\nnames.\n\n> > We long ago switched to\n> > printing the name as a warning when we have made such a guess (bb1ae3f),\n> > then more recently started rejecting them outright (8c5b1ae).\n> \n> Right, but these would still happen:\n> \n> michael \n\nDid you read my email? I explicitly proposed that we would _not_ allow\nsend-email to use implicit email addresses constructed in that way.\n\n> > But in the meantime you are causing a regression for anybody who expects\n> > GIT_AUTHOR_NAME to override user.email when running git-send-email (and\n> > you have taken away the prompt that they could have used to notice and\n> > correct it).\n> \n> I think they can survive. If anybody like this exists.\n\nSorry, but that is not how things work on this project. You do not get\nto cause regressions because you are too lazy to implement the feature\n_you_ want in a way that does not break other people.\n\nI tried to help you by pointing you in the right direction and even\nproviding a sample \"git var\" patch. But it is not my itch to scratch.\n\n-Peff\n"},{"id":"203054","messageId":"CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com","threadId":"32079","inReplyTo":"20121113040104.GA9361@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T06:42:58Z","receivedAt":"2012-11-13T06:42:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 5:01 AM, Jeff King wrote:\n> On Tue, Nov 13, 2012 at 04:55:25AM +0100, Felipe Contreras wrote:\n>\n>> > No, it's not. Those broken names do not come from the environment, but\n>> > from our last-resort guess of the hostname.\n>>\n>> That depends how you define environment, but fine, the point is that\n>> it happens.\n>\n> If you have a strawman definition that does not have anything to do with\n> what I said in my original email, then yes, it could happen.\n\nIt happens, I've seen commits with (none) not that long ago.\n\n> But as I\n> said already, \"git var\" uses IDENT_STRICT and will not allow such broken\n> names.\n\nSince 1.7.11, sure. But not everyone is using such a recent version of\ngit, and people with fully qualified domains would still get unwanted\nbehavior.\n\n>> > We long ago switched to\n>> > printing the name as a warning when we have made such a guess (bb1ae3f),\n>> > then more recently started rejecting them outright (8c5b1ae).\n>>\n>> Right, but these would still happen:\n>>\n>> michael \n>\n> Did you read my email? I explicitly proposed that we would _not_ allow\n> send-email to use implicit email addresses constructed in that way.\n\nI'm not talking about git send-email, I'm talking about your comment\n'it has always been the case that you can use git without setting\nuser.*', which has caused issues with wrong author/commmitter names in\ncommits, and will probably continue to do so.\n\n>> > But in the meantime you are causing a regression for anybody who expects\n>> > GIT_AUTHOR_NAME to override user.email when running git-send-email (and\n>> > you have taken away the prompt that they could have used to notice and\n>> > correct it).\n>>\n>> I think they can survive. If anybody like this exists.\n>\n> Sorry, but that is not how things work on this project. You do not get\n> to cause regressions because you are too lazy to implement the feature\n> _you_ want in a way that does not break other people.\n\nThat doesn't change the fact that they would survive, and the fact\nthat those users don't actually exist.\n\nBut let's look at the current situation closely:\n\nPERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1\n\n1) No information at all\n\nfatal: empty ident name (for ) not allowed\n\n2) Full Name + full hostname\n\nWho should the emails appear to be from? [Felipe Contreras\n]\n\nThat's right, ident doesn't fail, and that's not the mail address I\nspecified, it's *implicit*.\n\n3) Full Name + EMAIL\n\nWho should the emails appear to be from? [Felipe Contreras\n]\n\n4) config user\n\nWho should the emails appear to be from? [Felipe Contreras 2nd\n]\n\n5) GIT_COMMITTER\n\nWho should the emails appear to be from? [Felipe Contreras 2nd\n]\n\nWhoa, what happened there?\n\nWell:\n\n $sender = $repoauthor || $repocommitter || '';\n ($repoauthor) = Git::ident_person(@repo, 'author');\n % ./git var GIT_AUTHOR_IDENT\n Felipe Contreras 2nd 1352783223 +0100\n\nThat's right, AUTHOR_IDENT would fall back to the default email and full name.\n\nHmm, I wonder...\n\n5.1) GIT_COMMITER without anything else\n\nfatal: empty ident name (for ) not allowed\nvar GIT_AUTHOR_IDENT: command returned error: 128\n\nWhy? Because:\n\n% PERL5LIB=~/dev/git/perl perl -e 'use Git; printf(\"%s\\n\",\nGit::ident_person(@repo, 'author'));'\nfatal: empty ident name (for ) not allowed\n\n($repoauthor) = Git::ident_person(@repo, 'author');\n($repocommitter) = Git::ident_person(@repo, 'committer');\n\nSo $repoauthor || $repocommiter is pointless.\n\n6) GIT_AUTHOR\n\nWho should the emails appear to be from? [Felipe Contreras 4th\n]\n\nWhat about after my change?\n\n6.1) GIT_AUTHOR without anything else\n\nfatal: empty ident name (for ) not allowed\nvar GIT_COMMITTER_IDENT: command returned error: 128\n\n4) config user\n\nFrom: Felipe Contreras 2nd \n\n5) GIT_COMMITTER\n\nFrom: Felipe Contreras 2nd \n\n6) GIT_AUTHOR\n\nFrom: Felipe Contreras 2nd \n\nAnd what about your proposed change?\n\n2) Full Name + full hostname\n\n./git var GIT_EXPLICIT_IDENT\n0\n\n6.1) GIT_AUTHOR without anything else\n\nEven if the previous problem was solved:\n\nexport GIT_AUTHOR_NAME='Felipe Contreras 4th'; export\nGIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'\n./git var GIT_EXPLICIT_IDENT\n0\n\nNo explicit ident? This is most certainly not what the user would expect.\n\nAnd then:\n\n5.2) GIT_COMMITTER with Full Name and full hostname\n\nexport GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export\nGIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'\n./git var GIT_EXPLICIT_IDENT\n1\n\nFrom: Felipe Contreras \n\nIt is explicit, yeah, but 'git send-email' would not be picking the\ncommitter, it would pick the author.\n\n> I tried to help you by pointing you in the right direction and even\n> providing a sample \"git var\" patch.\n\nAre you 100% sure that was the right direction?\n\nI think the right approach is more along these lines:\n\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -748,16 +748,11 @@ if (!$force) {\n }\n }\n\n-my $prompting = 0;\n if (!defined $sender) {\n $sender = $repoauthor || $repocommitter || '';\n- $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n- default => $sender,\n- valid_re => qr/\\@.*\\./, confirm_only => 1);\n- print \"Emails will be sent from: \", $sender, \"\\n\";\n- $prompting++;\n }\n\n+my $prompting = 0;\n if (!@initial_to && !defined $to_cmd) {\n my $to = ask(\"Who should the emails be sent to (if any)? \",\n default => \"\",\ndiff --git a/ident.c b/ident.c\nindex a4bf206..c73ba82 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,\n }\n\n if (strict && email == git_default_email.buf &&\n- strstr(email, \"(none)\")) {\n+ !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {\n fputs(env_hint, stderr);\n- die(\"unable to auto-detect email address (got '%s')\", email);\n+ die(\"no explicit email address\");\n }\n\n if (want_date) {\n\nWith that we get:\n\n2) Full Name + full hostname\n\nfatal: no explicit email address\n\n3) Full Name + EMAIL\n\nFrom: Felipe Contreras \n\n4) config user\n\nFrom: Felipe Contreras 2nd \n\n5) GIT_COMMITTER\n\nFrom: Felipe Contreras 2nd \n\n(as buggy as before)\n\n6) GIT_AUTHOR\n\nFrom: Felipe Contreras 4th \n\nNot only will this fix 'git send-email', but it will also fix 'git\ncommit' so that we don't end up with authors such as 'Felipe Contreras\n' ever again.\n\n> But it is not my itch to scratch.\n\nSuit yourself, it's only git users that would get hurt. I can always\nuse my own 'git send-email' (as I am doing right now).\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203057","messageId":"CAMP44s2P14tfCm6SV3A1tks-BJ-go0gDPE3aiWMST-dEL4SETA@mail.gmail.com","threadId":"32079","inReplyTo":"CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T07:18:30Z","receivedAt":"2012-11-13T07:18:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 7:42 AM, Felipe Contreras\n wrote:\n\n> 6) GIT_AUTHOR\n>\n> Who should the emails appear to be from? [Felipe Contreras 4th\n> ]\n>\n> What about after my change?\n>\n> 6.1) GIT_AUTHOR without anything else\n>\n> fatal: empty ident name (for ) not allowed\n> var GIT_COMMITTER_IDENT: command returned error: 128\n\nThis was supposed to be above (before my change).\n\n-- \nFelipe Contreras\n"},{"id":"203058","messageId":"20121113074720.GA18746@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T07:47:21Z","receivedAt":"2012-11-13T07:47:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:\n\n> >> > No, it's not. Those broken names do not come from the environment, but\n> >> > from our last-resort guess of the hostname.\n> >>\n> >> That depends how you define environment, but fine, the point is that\n> >> it happens.\n> >\n> > If you have a strawman definition that does not have anything to do with\n> > what I said in my original email, then yes, it could happen.\n> \n> It happens, I've seen commits with (none) not that long ago.\n\nThere was a bug that caused the check to fail in some cases. I fixed it\nin f20f387 (this July).\n\nBut I still don't see how that has anything to do with what send-email\ndoes or should do. That is why I said \"strawman\" above. You seem to\nthink I am saying that send-email should use the system that generated\nthose broken names, when I am saying the opposite.\n\n> > But as I said already, \"git var\" uses IDENT_STRICT and will not\n> > allow such broken names.\n> \n> Since 1.7.11, sure. But not everyone is using such a recent version of\n> git, and people with fully qualified domains would still get unwanted\n> behavior.\n\nThose people would also not be using a new version of git-send-email,\nand it will always prompt. I thought we were talking about what\nsend-email should do in future versions. Namely, loosening that safety\nvalve (the prompt) because it is inconvenient, but tightening the checks\nso that losing the safety valve is not a problem.\n\n> > Did you read my email? I explicitly proposed that we would _not_ allow\n> > send-email to use implicit email addresses constructed in that way.\n> \n> I'm not talking about git send-email, I'm talking about your comment\n> 'it has always been the case that you can use git without setting\n> user.*', which has caused issues with wrong author/commmitter names in\n> commits, and will probably continue to do so.\n\nThe second half of that sentence that you quoted above is \"...instead\nonly using the environment.\" As in, environment variables like\nGIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, and EMAIL. _Not_ implicit\ngeneration of the email from the username and hostname.\n\nI am tempted to fault myself for not communicating well, but I feel like\nI have made that point at least 3 times in this conversation now. Is\nthat the source of the confusion?\n\n> > Sorry, but that is not how things work on this project. You do not get\n> > to cause regressions because you are too lazy to implement the feature\n> > _you_ want in a way that does not break other people.\n> \n> That doesn't change the fact that they would survive, and the fact\n> that those users don't actually exist.\n\nAnd you will survive if upstream git (whether it is me today or Junio\ntomorrow) does not pick up your patch. I remember writing you a long\nemail recently about how one of the responsibilities of the maintainer\nis to balance features versus regressions. I'll not bother repeating\nmyself here.\n\nAs for whether they exist, what data do you have? Are you aware that the\ntest suite, for example, relies on setting GIT_AUTHOR_NAME but not\nhaving any user.* config? When somebody comes on the list and asks why\nevery git program in the entire system respects GIT_* environment\nvariables as an override to user.* configuration _except_ for\nsend-email, what should I say?\n\n> But let's look at the current situation closely:\n> \n> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1\n> \n> 1) No information at all\n> \n> fatal: empty ident name (for ) not allowed\n\nThat is dependent on your system. If you have a non-empty name in your\nGECOS field, and if your machine has a FQDN, it will currently work (and\nprompt).\n\n> 2) Full Name + full hostname\n> \n> Who should the emails appear to be from? [Felipe Contreras\n> ]\n> \n> That's right, ident doesn't fail, and that's not the mail address I\n> specified, it's *implicit*.\n\nRight. I never said it did. I said it currently rejected obviously bogus\nstuff (like the \".(none)\" above) due to IDENT_STRICT, but currently\nallowed implicit definitions. And I also said that if we get rid of the\nprompt, we should disallow implicit definitions like this, because the\nprompt is the safety valve on sending out mails with broken from\naddresses. I even wrote a patch that let you find out whether the ident\nwas generated implicitly.\n\n> 3) Full Name + EMAIL\n> \n> Who should the emails appear to be from? [Felipe Contreras\n> ]\n\nWhich sounds fine to me. EMAIL is considered explicit, and I have not\nseen any evidence that people are putting bogus values in their EMAIL\nvariable and complaining that it is git's fault for respecting it.\n\n> 4) config user\n> \n> Who should the emails appear to be from? [Felipe Contreras 2nd\n> ]\n\nOK.\n\n> 5) GIT_COMMITTER\n> \n> Who should the emails appear to be from? [Felipe Contreras 2nd\n> ]\n> \n> Whoa, what happened there?\n> \n> Well:\n> \n> $sender = $repoauthor || $repocommitter || '';\n> ($repoauthor) = Git::ident_person(@repo, 'author');\n> % ./git var GIT_AUTHOR_IDENT\n> Felipe Contreras 2nd 1352783223 +0100\n> \n> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n\nYeah, I find that somewhat questionable in the current behavior, and I'd\nconsider it a bug. Typically we prefer the committer ident when given a\nchoice (e.g., for writing reflog entries).\n\n> 5.1) GIT_COMMITER without anything else\n> \n> fatal: empty ident name (for ) not allowed\n> var GIT_AUTHOR_IDENT: command returned error: 128\n\nRight. Same bug as above.\n\n> So $repoauthor || $repocommiter is pointless.\n\nAgreed.\n\n> 6) GIT_AUTHOR\n> \n> Who should the emails appear to be from? [Felipe Contreras 4th\n> ]\n\nRight, that's what I'd expect.\n\n> What about after my change?\n> \n> 6.1) GIT_AUTHOR without anything else\n> \n> fatal: empty ident name (for ) not allowed\n> var GIT_COMMITTER_IDENT: command returned error: 128\n\nDoesn't that seem like a regression? It used to work.\n\n> 4) config user\n> \n> From: Felipe Contreras 2nd \n\nOK.\n\n> 5) GIT_COMMITTER\n> \n> From: Felipe Contreras 2nd \n\nOK.\n\n> 6) GIT_AUTHOR\n> \n> From: Felipe Contreras 2nd \n\nDoesn't that seem like a regression? It used to use a different address,\nand in every other git program, the environment takes precedence over\nconfig.\n\n> And what about your proposed change?\n\nLet me be clear that I sent you a \"something like this\" patch to try to\npoint you in the right direction. If it has a bug or is incomplete, that\ndoes not mean the direction is wrong, but only that I did not spend very\nmuch time on the patch.\n\n> 6.1) GIT_AUTHOR without anything else\n> \n> Even if the previous problem was solved:\n> \n> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export\n> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'\n> ./git var GIT_EXPLICIT_IDENT\n> 0\n> \n> No explicit ident? This is most certainly not what the user would expect.\n\nYes, it looks like we do not set up the explicit ident flags when\nparsing the author. So my patch is insufficient.\n\n> 5.2) GIT_COMMITTER with Full Name and full hostname\n> \n> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export\n> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'\n> ./git var GIT_EXPLICIT_IDENT\n> 1\n> \n> From: Felipe Contreras \n> \n> It is explicit, yeah, but 'git send-email' would not be picking the\n> committer, it would pick the author.\n\nYep.\n\nThe explicitness needs to be tied to the specific ident we grabbed.\nProbably adding a \"git var GIT_AUTHOR_EXPLICIT\" would be enough, or\nalternatively, adding a flag to \"git var\" to error out rather than\nreturn a non-explicit ident (this may need to adjust the error\nhandling of the \"git var\" calls from send-email).\n\n> > I tried to help you by pointing you in the right direction and even\n> > providing a sample \"git var\" patch.\n> \n> Are you 100% sure that was the right direction?\n\nI think that respecting the usual ident lookup but disallowing implicit\nidentities (either totally, or causing them to fallback to prompting) is\nthe right direction. I agree my patch was not a complete solution. I'm\nsorry if it led you astray in terms of implementation, but I also think\nI've been very clear in my text about what the behavior should be.\n\n> I think the right approach is more along these lines:\n\nI think that is moving in the right direction, but...\n\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,\n> }\n> \n> if (strict && email == git_default_email.buf &&\n> - strstr(email, \"(none)\")) {\n> + !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {\n> fputs(env_hint, stderr);\n> - die(\"unable to auto-detect email address (got '%s')\", email);\n> + die(\"no explicit email address\");\n\nI think this needs to be optional, otherwise you are breaking callers\nwho use IDENT_STRICT but are OK with the implicit ident (e.g.,\ncommit, format-patch with threading).\n\nYou can argue whether \"git commit\" should disallow such addresses, but\nthat is a separate topic from how send-email should behave.\n\n> Not only will this fix 'git send-email', but it will also fix 'git\n> commit' so that we don't end up with authors such as 'Felipe Contreras\n> ' ever again.\n\nWhile simultaneously breaking \"git commit\" for people who are happily\nusing the implicit generation. I can see the appeal of doing so; I was\ntempted to suggest it when I cleaned up IDENT_STRICT a few months back.\nBut do we have any data on how many people are currently using that\nfeature that would be annoyed by it?\n\n> > But it is not my itch to scratch.\n> \n> Suit yourself, it's only git users that would get hurt. I can always\n> use my own 'git send-email' (as I am doing right now).\n\nDon't get me wrong. I think the spirit of your patch is correct, and it\nhelps some git users. But it also hurts others. And it is not that hard\nto do it right.\n\nIt may be something I would work on myself in the future, but I have\nother things to work on at the moment, and since you are interested in\nthe topic, I thought you would be a good candidate to polish it enough\nto be suitable upstream. But instead I see a lot of push-back on what I\nconsidered to be a fairly straightforward technical comment on a\nregression.\n\nAnd now I have wasted a large chunk of the evening responding to you,\nneither accomplishing my other tasks nor polishing this topic. I do not\nmind reviewing patches or responding to discussions, nor do I consider\nthem time wasted; they are an important part of the development process.\nBut I feel like I am fighting an uphill battle just to convince you that\nregressions are bad, and that I am having to make the same points\nrepeatedly. That makes me frustrated and less excited about reviewing\nyour patches; and when I say \"it is not my itch\", that is my most polite\nway of saying \"If that is going to be your attitude, then I do not feel\nlike dealing with you anymore on this topic\".\n\n-Peff\n"},{"id":"203064","messageId":"CAMP44s1NdK9mw3Qz_sk1Zvg0gS6E+V0BuCfDutz8-8YD_App=Q@mail.gmail.com","threadId":"32079","inReplyTo":"20121113074720.GA18746@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T09:06:26Z","receivedAt":"2012-11-13T09:06:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 8:47 AM, Jeff King wrote:\n\n> But I still don't see how that has anything to do with what send-email\n> does or should do. That is why I said \"strawman\" above. You seem to\n> think I am saying that send-email should use the system that generated\n> those broken names, when I am saying the opposite.\n\nNo, I'm saying none should use that system, and that in fact 'git\ncommit' should be stricter... both should be stricter.\n\n> Those people would also not be using a new version of git-send-email,\n> and it will always prompt. I thought we were talking about what\n> send-email should do in future versions. Namely, loosening that safety\n> valve (the prompt) because it is inconvenient, but tightening the checks\n> so that losing the safety valve is not a problem.\n\nYeah, but all I'm saying is that the issue happens, you seemed to\nsuggest that it doesn't.\n\n>> I'm not talking about git send-email, I'm talking about your comment\n>> 'it has always been the case that you can use git without setting\n>> user.*', which has caused issues with wrong author/commmitter names in\n>> commits, and will probably continue to do so.\n>\n> The second half of that sentence that you quoted above is \"...instead\n> only using the environment.\" As in, environment variables like\n> GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, and EMAIL. _Not_ implicit\n> generation of the email from the username and hostname.\n\nThat sentence was *not* about 'git send-email', it was about git in\ngeneral, and 'git commit' is perfectly happy with implicit generation\nof the email from the username and hostname.\n\nI don't thin 'git commit' should do that, and I don't think 'git\nsend-email' should do that. I'm criticizing the whole approach.\n\n> I am tempted to fault myself for not communicating well, but I feel like\n> I have made that point at least 3 times in this conversation now. Is\n> that the source of the confusion?\n\nI think you are the one that is not understanding what I'm saying. But\nI don't think it matters.\n\nThis is what I'm saying; the current situation with 'git commit' is\nnot OK, _both_ 'git commit' and 'git send-email' should change.\n\n> And you will survive if upstream git (whether it is me today or Junio\n> tomorrow) does not pick up your patch.\n\nIndeed I would, but there's other people that would benefit from this\npatch. I'm sure I'm not the only person that doesn't have\nsendmail.from configured, but does have user.name/user.email, and is\nconstantly typing enter.\n\nAnd the difference is that I'm _real_, the hypothetical user that\nsends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced\notherwise if some evidence was presented that such a user is real\nthough.\n\n> I remember writing you a long\n> email recently about how one of the responsibilities of the maintainer\n> is to balance features versus regressions. I'll not bother repeating\n> myself here.\n\nAnd to balance you need to *measure*, and that means taking into\nconsideration who actually uses the features, if there's any. And it\nlooks to me this is a feature nobody uses.\n\nBut listen closely to what you said:\n\n> I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.\n\nSuppose somebody has a full name, and a fully qualified domain name,\nand he can receive mails to it directly. Such a user would not need a\ngit configuration, and would not need $EMAIL, or anything.\n\nCurrently 'git send-email' will throw 'Felipe Contreras\n' which would actually work, but is not explicit.\n\nYou are suggesting to break that use-case. You are introducing a\nregression. And this case is realistic, unlike the\nGIT_AUTHOR_NAME/EMAIL. Isn't it?\n\nI prefer to concentrate on real issues, but that's just me.\n\n> As for whether they exist, what data do you have?\n\nWhat data do _you_ have?\n\nWhen there's no evidence either way, the rational response is to don't\nbelieve. That's the default position.\n\n> Are you aware that the\n> test suite, for example, relies on setting GIT_AUTHOR_NAME but not\n> having any user.* config?\n\nWhat tests? My patch doesn't seem to break anything there:\n% make -C t t9001-send-email.sh\n# passed all 96 test(s)\n\n> When somebody comes on the list and asks why\n> every git program in the entire system respects GIT_* environment\n> variables as an override to user.* configuration _except_ for\n> send-email, what should I say?\n\nThe same thing you say when somebody comes reporting a bug: \"yeah, we\nshould probably fix that\".\n\nBut that's not going to happen. And in the unlikely event that it\ndoes, it's not going to be a major issue.\n\nIt's all about proportion. Is it possible that we all are going to die\ntomorrow because of an asteroid? Sure... but what's the point of\nworrying about it if it's not likely?\n\n>> But let's look at the current situation closely:\n>>\n>> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1\n>>\n>> 1) No information at all\n>>\n>> fatal: empty ident name (for ) not allowed\n>\n> That is dependent on your system. If you have a non-empty name in your\n> GECOS field, and if your machine has a FQDN, it will currently work (and\n> prompt).\n\nYes, that's point 2).\n\n>> 2) Full Name + full hostname\n>>\n>> Who should the emails appear to be from? [Felipe Contreras\n>> ]\n>>\n>> That's right, ident doesn't fail, and that's not the mail address I\n>> specified, it's *implicit*.\n>\n> Right. I never said it did. I said it currently rejected obviously bogus\n> stuff (like the \".(none)\" above) due to IDENT_STRICT, but currently\n> allowed implicit definitions. And I also said that if we get rid of the\n> prompt, we should disallow implicit definitions like this, because the\n> prompt is the safety valve on sending out mails with broken from\n> addresses. I even wrote a patch that let you find out whether the ident\n> was generated implicitly.\n\nCorrect.\n\n>> 3) Full Name + EMAIL\n>>\n>> Who should the emails appear to be from? [Felipe Contreras\n>> ]\n>\n> Which sounds fine to me. EMAIL is considered explicit, and I have not\n> seen any evidence that people are putting bogus values in their EMAIL\n> variable and complaining that it is git's fault for respecting it.\n\nAgreed.\n\n>> 5) GIT_COMMITTER\n>>\n>> Who should the emails appear to be from? [Felipe Contreras 2nd\n>> ]\n>>\n>> Whoa, what happened there?\n>>\n>> Well:\n>>\n>> $sender = $repoauthor || $repocommitter || '';\n>> ($repoauthor) = Git::ident_person(@repo, 'author');\n>> % ./git var GIT_AUTHOR_IDENT\n>> Felipe Contreras 2nd 1352783223 +0100\n>>\n>> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n>\n> Yeah, I find that somewhat questionable in the current behavior, and I'd\n> consider it a bug. Typically we prefer the committer ident when given a\n> choice (e.g., for writing reflog entries).\n\nYeah, but clearly the intention of the code was to use the committer\nif the author wasn't available, which is the case here.\n\n>> 5.1) GIT_COMMITER without anything else\n>>\n>> fatal: empty ident name (for ) not allowed\n>> var GIT_AUTHOR_IDENT: command returned error: 128\n>\n> Right. Same bug as above.\n\nNo, this is a different bug.\n\nThe bug above 5) is here:\n\n$sender = $repoauthor || $repocommitter || '';\n\n$repoauthor will always evaluate to true.\n\nThis one 5.1) is there:\n\n($repoauthor) = Git::ident_person(@repo, 'author');\n($repocommitter) = Git::ident_person(@repo, 'committer'); <-\n\n>> So $repoauthor || $repocommiter is pointless.\n>\n> Agreed.\n\nGood.\n\n>> 6) GIT_AUTHOR\n>>\n>> Who should the emails appear to be from? [Felipe Contreras 4th\n>> ]\n>\n> Right, that's what I'd expect.\n\nYou mean without the input question?\n\n>> What about after my change?\n>>\n>> 6.1) GIT_AUTHOR without anything else\n>>\n>> fatal: empty ident name (for ) not allowed\n>> var GIT_COMMITTER_IDENT: command returned error: 128\n>\n> Doesn't that seem like a regression? It used to work.\n\nNo, this is *before* my change.\n\nI's the same bug as 5.1):\n\n($repoauthor) = Git::ident_person(@repo, 'author'); <- here\n($repocommitter) = Git::ident_person(@repo, 'committer');\n\n>> 4) config user\n>>\n>> From: Felipe Contreras 2nd \n>\n> OK.\n>\n>> 5) GIT_COMMITTER\n>>\n>> From: Felipe Contreras 2nd \n>\n> OK.\n>\n>> 6) GIT_AUTHOR\n>>\n>> From: Felipe Contreras 2nd \n>\n> Doesn't that seem like a regression? It used to use a different address,\n> and in every other git program, the environment takes precedence over\n> config.\n\nYes, it is a regression (that won't affect anybody).\n\n>> And what about your proposed change?\n>\n> Let me be clear that I sent you a \"something like this\" patch to try to\n> point you in the right direction. If it has a bug or is incomplete, that\n> does not mean the direction is wrong, but only that I did not spend very\n> much time on the patch.\n\nIt doesn't matter, the idea was to use user_ident_sufficiently_given().\n\n>> 6.1) GIT_AUTHOR without anything else\n>>\n>> Even if the previous problem was solved:\n>>\n>> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export\n>> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'\n>> ./git var GIT_EXPLICIT_IDENT\n>> 0\n>>\n>> No explicit ident? This is most certainly not what the user would expect.\n>\n> Yes, it looks like we do not set up the explicit ident flags when\n> parsing the author. So my patch is insufficient.\n\nIndeed.\n\n>> 5.2) GIT_COMMITTER with Full Name and full hostname\n>>\n>> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export\n>> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'\n>> ./git var GIT_EXPLICIT_IDENT\n>> 1\n>>\n>> From: Felipe Contreras \n>>\n>> It is explicit, yeah, but 'git send-email' would not be picking the\n>> committer, it would pick the author.\n>\n> Yep.\n>\n> The explicitness needs to be tied to the specific ident we grabbed.\n> Probably adding a \"git var GIT_AUTHOR_EXPLICIT\" would be enough, or\n> alternatively, adding a flag to \"git var\" to error out rather than\n> return a non-explicit ident (this may need to adjust the error\n> handling of the \"git var\" calls from send-email).\n\nI think strictess should be tied to explicitness, and 'git var' should\nerror out, and not die.\n\n>> > I tried to help you by pointing you in the right direction and even\n>> > providing a sample \"git var\" patch.\n>>\n>> Are you 100% sure that was the right direction?\n>\n> I think that respecting the usual ident lookup but disallowing implicit\n> identities (either totally, or causing them to fallback to prompting) is\n> the right direction. I agree my patch was not a complete solution. I'm\n> sorry if it led you astray in terms of implementation, but I also think\n> I've been very clear in my text about what the behavior should be.\n\nI think that is orthogonal to what I'm trying accomplish.\n\n>> I think the right approach is more along these lines:\n>\n> I think that is moving in the right direction, but...\n>\n>> --- a/ident.c\n>> +++ b/ident.c\n>> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,\n>> }\n>>\n>> if (strict && email == git_default_email.buf &&\n>> - strstr(email, \"(none)\")) {\n>> + !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {\n>> fputs(env_hint, stderr);\n>> - die(\"unable to auto-detect email address (got '%s')\", email);\n>> + die(\"no explicit email address\");\n>\n> I think this needs to be optional, otherwise you are breaking callers\n> who use IDENT_STRICT but are OK with the implicit ident (e.g.,\n> commit, format-patch with threading).\n>\n> You can argue whether \"git commit\" should disallow such addresses, but\n> that is a separate topic from how send-email should behave.\n\nYes, that's exactly what I would argue.\n\n>> Not only will this fix 'git send-email', but it will also fix 'git\n>> commit' so that we don't end up with authors such as 'Felipe Contreras\n>> ' ever again.\n>\n> While simultaneously breaking \"git commit\" for people who are happily\n> using the implicit generation. I can see the appeal of doing so; I was\n> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.\n> But do we have any data on how many people are currently using that\n> feature that would be annoyed by it?\n\nNo, but it can also be considered a bug... do we have any data on how\nmany people are being affected by this? If the '(none)' commits are\nany indication of it, probably a lot. At least the ones that do have a\nfqdn.\n\n>> > But it is not my itch to scratch.\n>>\n>> Suit yourself, it's only git users that would get hurt. I can always\n>> use my own 'git send-email' (as I am doing right now).\n>\n> Don't get me wrong. I think the spirit of your patch is correct, and it\n> helps some git users. But it also hurts others. And it is not that hard\n> to do it right.\n\nAnd I disagree, I think it hurts nobody, and I think it's hard to do it right.\n\n> It may be something I would work on myself in the future, but I have\n> other things to work on at the moment, and since you are interested in\n> the topic, I thought you would be a good candidate to polish it enough\n> to be suitable upstream. But instead I see a lot of push-back on what I\n> considered to be a fairly straightforward technical comment on a\n> regression.\n\nI'm just trying to be pragmatic. I don't see the point in wasting my\ntime for people that don't exist. As I said, I don't think anybody\nwould be hit by this.\n\n> And now I have wasted a large chunk of the evening responding to you,\n> neither accomplishing my other tasks nor polishing this topic. I do not\n> mind reviewing patches or responding to discussions, nor do I consider\n> them time wasted; they are an important part of the development process.\n> But I feel like I am fighting an uphill battle just to convince you that\n> regressions are bad, and that I am having to make the same points\n> repeatedly. That makes me frustrated and less excited about reviewing\n> your patches; and when I say \"it is not my itch\", that is my most polite\n> way of saying \"If that is going to be your attitude, then I do not feel\n> like dealing with you anymore on this topic\".\n\nFixing a regression that nobody would notice is not my itch either,\nyet the patch I sent above does it, and it even fixes 'git commit'\n(IMO). But it's also not good enough.\n\nI scratched my itch with the original patch, anything after that is to\nhelp other people.\n\nI think it would be much easier to just remove the question input. The\nonly \"regression\" would be the people that have a fqdn and full name\n_and_ expect the question. But Junio suggested to just die in those\ncases, and trying to send an email that would probably fail is not\nthat different.\n\nFixing all the var and ident infrastructure seems way, *way* far from\nwhat I intended to do.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203101","messageId":"7vpq3hfpm7.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121113074720.GA18746@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T16:13:04Z","receivedAt":"2012-11-13T16:13:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:\n> ...\n>> 5) GIT_COMMITTER\n>> \n>> Who should the emails appear to be from? [Felipe Contreras 2nd\n>> ]\n>> \n>> Whoa, what happened there?\n>> \n>> Well:\n>> \n>> $sender = $repoauthor || $repocommitter || '';\n>> ($repoauthor) = Git::ident_person(@repo, 'author');\n>> % ./git var GIT_AUTHOR_IDENT\n>> Felipe Contreras 2nd 1352783223 +0100\n>> \n>> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n>\n> Yeah, I find that somewhat questionable in the current behavior, and I'd\n> consider it a bug. Typically we prefer the committer ident when given a\n> choice (e.g., for writing reflog entries).\n\nJust to make sure I follow the discussion correctly, do you mean\nthat the bug is that we pick a fuzzy AUTHOR when COMMITTER is\nspecified more concretely and we usually use COMMIITTER for this\nkind of thing in the first place but send-email does not in this\ncase (I do not see \"git var GIT_AUTHOR_IDENT\" returning value from\nthe implicit logic as a bug in this case---just making sure).\n\n>> 6.1) GIT_AUTHOR without anything else\n>> \n>> fatal: empty ident name (for ) not allowed\n>> var GIT_COMMITTER_IDENT: command returned error: 128\n>\n> Doesn't that seem like a regression? It used to work.\n\nI do not think we mind a change to favor GIT_COMMITTER setting over\nGIT_AUTHOR to be consistent with others too much, but that indeed is\na big change. People who are used to the inconsistency that\nsend-email favors AUTHOR over COMMITTER will certainly find it as a\nregression.\n\n> The explicitness needs to be tied to the specific ident we grabbed.\n> Probably adding a \"git var GIT_AUTHOR_EXPLICIT\" would be enough, or\n> alternatively, adding a flag to \"git var\" to error out rather than\n> return a non-explicit ident (this may need to adjust the error\n> handling of the \"git var\" calls from send-email).\n\nYeah, something like that would be necessary for \"git var\" users to\nmake this kind of decision.\n\n> While simultaneously breaking \"git commit\" for people who are happily\n> using the implicit generation. I can see the appeal of doing so; I was\n> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.\n> But do we have any data on how many people are currently using that\n> feature that would be annoyed by it?\n\nAs we didn't break \"git commit\" when you worked on IDENT_STRICT, I\ndo not think we have any data on it ;-)\n\n> ...\n> It may be something I would work on myself in the future, but I have\n> other things to work on at the moment, and since you are interested in\n> the topic, I thought you would be a good candidate to polish it enough\n> to be suitable upstream. But instead I see a lot of push-back on what I\n> considered to be a fairly straightforward technical comment on a\n> regression.\n> ...\n> But I feel like I am fighting an uphill battle just to convince you that\n> regressions are bad, and that I am having to make the same points\n> repeatedly. That makes me frustrated and less excited about reviewing\n> your patches; and when I say \"it is not my itch\", that is my most polite\n> way of saying \"If that is going to be your attitude, then I do not feel\n> like dealing with you anymore on this topic\".\n\nFor a change with low benefit/cost ratio like this, the best course\nof action often is to stop paying attention to the thread that has\nbecome unproductive and venomous, and resurrect the topic after\npeople forgot about it and doing it right yourself ;-).\n"},{"id":"203107","messageId":"20121113164845.GD20361@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s1NdK9mw3Qz_sk1Zvg0gS6E+V0BuCfDutz8-8YD_App=Q@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:48:45Z","receivedAt":"2012-11-13T16:48:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:\n\n> > Those people would also not be using a new version of git-send-email,\n> > and it will always prompt. I thought we were talking about what\n> > send-email should do in future versions. Namely, loosening that safety\n> > valve (the prompt) because it is inconvenient, but tightening the checks\n> > so that losing the safety valve is not a problem.\n> \n> Yeah, but all I'm saying is that the issue happens, you seemed to\n> suggest that it doesn't.\n\nWhat is \"it\"?\n\nIf it is bad guesses like \"user@host.(none)\" on unconfigured systems,\nthen I did not suggest it doesn't happen. I said that it happened with\nolder versions of git, but that it has now been fixed. Of course it will\ncontinue to happen for people on older versions of git until they\nupgrade. I cannot go back in time and fix released versions.\n\nIf it is guessing at all to end up with \"user@host.domain\" for a host\nthat does not receive mail, yes, that happens, and I have been very\nclear that it does. The safety valve is showing the ident and a warning\nto the user during the commit process.\n\nI do not see any point in discussing the former when considering future\nchanges to git. It is already disallowed by current versions of git, and\nwe are just waiting for the whole world to upgrade to a fixed version.\n\nI can see the argument for tightening the check to disallow the latter.\nBut it would also hurt real people who are relying on the feature.\nPerhaps it would make sense to adopt the implicit_ident_advice in other\ncode paths besides \"git commit\" (e.g., via \"git var).\n\n> I think you are the one that is not understanding what I'm saying. But\n> I don't think it matters.\n> \n> This is what I'm saying; the current situation with 'git commit' is\n> not OK, _both_ 'git commit' and 'git send-email' should change.\n\nPerhaps I am being dense, but this is the first time it became apparent\nto me that you are arguing for a change in \"git commit\".\n\n> Indeed I would, but there's other people that would benefit from this\n> patch. I'm sure I'm not the only person that doesn't have\n> sendmail.from configured, but does have user.name/user.email, and is\n> constantly typing enter.\n> \n> And the difference is that I'm _real_, the hypothetical user that\n> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced\n> otherwise if some evidence was presented that such a user is real\n> though.\n\nSadly we cannot poll the configuration of every user, nor expect them\nall to pay attention to this discussion on the list. So we cannot take\nthe absence of comment from such users as evidence that they do not\nexist. Instead we must use our judgement about what is being changed,\nand we tend to err on the side of keeping the status quo, since that is\nwhat the silent majority is busy _not_ complaining about.\n\nWe use the same judgement on the other side, too. Right now your\nevidence is \"1 user wants this, 0 users do not\". But we can guess that\nthere are other people who would like the intent of your patch, but did\nnot care enough or are not active enough on the list to write the patch\nthemselves or comment on this thread.\n\nIt is also very easy to me accept the status quo when it is not in\nfundamental conflict with the proposed improvement, but simply a matter\nof implementation (which it is the case for send-email stopping\nprompting in some cases, though it is not for changing the behavior of\ngit-commit).\n\n> And to balance you need to *measure*, and that means taking into\n> consideration who actually uses the features, if there's any. And it\n> looks to me this is a feature nobody uses.\n\nYou said \"measure\" and then \"it looks to me like\". What did you measure?\nDid you consider systematic bias in your measurement, like the fact that\npeople who are using the feature have no reason to come on the list and\nannounce it?\n\n> But listen closely to what you said:\n> \n> > I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.\n> \n> Suppose somebody has a full name, and a fully qualified domain name,\n> and he can receive mails to it directly. Such a user would not need a\n> git configuration, and would not need $EMAIL, or anything.\n> \n> Currently 'git send-email' will throw 'Felipe Contreras\n> ' which would actually work, but is not explicit.\n> \n> You are suggesting to break that use-case. You are introducing a\n> regression. And this case is realistic, unlike the\n> GIT_AUTHOR_NAME/EMAIL. Isn't it?\n\nYes, dying would be a regression, in that you would have to configure\nyour name via the environment and re-run rather than type it at the\nprompt. You raise a good point that for people who _could_ take the\nimplicit default, hitting \"enter\" is working fine now, and we would lose\nthat. I'd be fine with also just continuing to prompt in the implicit\ncase.\n\nBut that is a much smaller issue to me than having send-email fail to\nrespect environment variables and silently use user.*, which is what\nstarted this whole discussion. And I agree it is worth considering as a\nregression we should avoid.\n\n> I prefer to concentrate on real issues, but that's just me.\n\nTo be honest, I am confused at this point what you actually want. Do you\nthink we should take your original patch? I think its regression is too\ngreat. And I am not sure if you agree or not. You seem to be arguing\nthat the regression is not important, yet you simultaneously argue that\nwe should be making all ident more strict, which would mean that we\nshould indeed take my suggestion and use \"git var\" instead of preferring\nthe config.\n\n> > As for whether they exist, what data do you have?\n> \n> What data do _you_ have?\n> \n> When there's no evidence either way, the rational response is to don't\n> believe. That's the default position.\n\nThis is not religion. It is a software project. In the absence of data,\nthe sane thing is not to break existing users. The burden is on you to\nargue that there is no such breakage.\n\n> > Are you aware that the\n> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not\n> > having any user.* config?\n> \n> What tests? My patch doesn't seem to break anything there:\n> % make -C t t9001-send-email.sh\n> # passed all 96 test(s)\n\nMy point was that there is at least one known setup that uses only\nenvironment variables and not a config file, and that the rest of git is\nintended to work with that. It is not a test failure, but t9001.18\nreveals the fact that your change does not handle this situation; we\ncontinue to prompt under the test suite's configuration. In the proper\nfix, the test needs to be adjusted.\n\nYou don't see any test failures with your patch because we do not cover\nthe case that you regress (GIT_AUTHOR_EMAIL and user.email both set).\n\n> > When somebody comes on the list and asks why\n> > every git program in the entire system respects GIT_* environment\n> > variables as an override to user.* configuration _except_ for\n> > send-email, what should I say?\n> \n> The same thing you say when somebody comes reporting a bug: \"yeah, we\n> should probably fix that\".\n\nIt is a larger hassle for both the developer and the user to fix the bug\nafter the fact, ship a new version, and then have the user upgrade their\ngit. Why not just not introduce the bug in the first place?\n\n> It's all about proportion. Is it possible that we all are going to die\n> tomorrow because of an asteroid? Sure... but what's the point of\n> worrying about it if it's not likely?\n\nIf you want to talk about risk assessment, then the right computation is\nto compare the cost of fixing it now versus the cost of fixing it later\ntimes the probability of it happening. The development cost is probably\na little higher later (because we have to refresh ourselves on the\nissue), but the deployment cost is much higher (e.g., users whose\ndistros ship a broken version, or who have IT policy that does not let\nthem upgrade git as soon as the bug-fix ships).\n\n> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n> >\n> > Yeah, I find that somewhat questionable in the current behavior, and I'd\n> > consider it a bug. Typically we prefer the committer ident when given a\n> > choice (e.g., for writing reflog entries).\n> \n> Yeah, but clearly the intention of the code was to use the committer\n> if the author wasn't available, which is the case here.\n\nYes. It would make sense to me to respect an explicit committer over an\nimplicit author. Though the fact that author is preferred over committer\nis inconsistent with most of the rest of git. I'm undecided whether that\nshould be changed.\n\n> >> What about after my change?\n> >>\n> >> 6.1) GIT_AUTHOR without anything else\n> >>\n> >> fatal: empty ident name (for ) not allowed\n> >> var GIT_COMMITTER_IDENT: command returned error: 128\n> >\n> > Doesn't that seem like a regression? It used to work.\n> \n> No, this is *before* my change.\n> \n> I's the same bug as 5.1):\n\nAh, I see. We are loading both, whether or not they end up being used,\nand barfing prematurely on errors. That seems like a bug.\n\n> >> And what about your proposed change?\n> >\n> > Let me be clear that I sent you a \"something like this\" patch to try to\n> > point you in the right direction. If it has a bug or is incomplete, that\n> > does not mean the direction is wrong, but only that I did not spend very\n> > much time on the patch.\n> \n> It doesn't matter, the idea was to use user_ident_sufficiently_given().\n\nRight. Which it sounds like now agree with me on?\n\n> > I think that respecting the usual ident lookup but disallowing implicit\n> > identities (either totally, or causing them to fallback to prompting) is\n> > the right direction. I agree my patch was not a complete solution. I'm\n> > sorry if it led you astray in terms of implementation, but I also think\n> > I've been very clear in my text about what the behavior should be.\n> \n> I think that is orthogonal to what I'm trying accomplish.\n\nWhat is it you are trying to accomplish? Eliminating the prompt in some\ncases, as in your original patch? Dealing with implicit identities may\nbe orthogonal to your goal, but your original patch is not sufficient,\nas it reverses the precedence of config and environment variables.\nSo you can either:\n\n 1. Reimplement the environment variable lookup that ident.c does,\n leaving implicit ident logic out completely.\n\n 2. Modify ident.c and \"git var\" to let send-email reuse the logic in\n ident.c, but avoid dropping the prompt when an implicit ident is\n used.\n\nDoing (2) sounds a lot more maintainable to me in the long run.\n\n> > Don't get me wrong. I think the spirit of your patch is correct, and it\n> > helps some git users. But it also hurts others. And it is not that hard\n> > to do it right.\n> \n> And I disagree, I think it hurts nobody, and I think it's hard to do it right.\n\nI am tired of talking about this. The patch series below took me less\ntime to write than I have spent arguing with you. It was not that hard.\n\n [1/6]: ident: make user_ident_explicitly_given private\n [2/6]: ident: keep separate \"explicit\" flags for author and committer\n [3/6]: var: accept multiple variables on the command line\n [4/6]: var: provide explicit/implicit ident information\n [5/6]: Git.pm: teach \"ident\" to query explicitness\n [6/6]: send-email: do not prompt for explicit repo ident\n\n> Fixing a regression that nobody would notice is not my itch either,\n\nSorry, but it is part of your itch. The git project is at state A. With\nyour patch, we are at a regressed state B. If you then fix it, we are at\nstate C. From your perspective, it may be \"I do not want to make the fix\nto go from B to C\". But from the project's perspective, it is \"we do not\nwant to go from state A to state B; state C would be acceptable\". So\nfrom the perspective of people who do not care about your feature but do\ncare about the regression in state B, it is inextricably linked to your\nitch.\n\n> yet the patch I sent above does it, and it even fixes 'git commit'\n> (IMO). But it's also not good enough.\n\nI do not necessarily agree on \"git commit\". Moreover, I feel like it is\na separate issue. My series above _just_ implements the \"do not prompt\nwhen explicit\" behavior. It does not deal with git-commit at all, nor\ndoes it address the author/committer fallback questions. Those can\neasily go on top.\n\n-Peff\n"},{"id":"203108","messageId":"20121113164931.GA12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 1/6] ident: make user_ident_explicitly_given private","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:49:31Z","receivedAt":"2012-11-13T16:49:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no users of this global variable, as queriers\ngo through the user_ident_sufficiently_given accessor.\nLet's make it private, which will enable further\nrefactoring.\n\nSigned-off-by: Jeff King \n---\n cache.h | 4 ----\n ident.c | 6 +++++-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex dbd8018..50d9eea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,10 +1149,6 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-#define IDENT_NAME_GIVEN 01\n-#define IDENT_MAIL_GIVEN 02\n-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-extern int user_ident_explicitly_given;\n extern int user_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\ndiff --git a/ident.c b/ident.c\nindex a4bf206..733d69d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -10,7 +10,11 @@\n static struct strbuf git_default_name = STRBUF_INIT;\n static struct strbuf git_default_email = STRBUF_INIT;\n static char git_default_date[50];\n-int user_ident_explicitly_given;\n+\n+#define IDENT_NAME_GIVEN 01\n+#define IDENT_MAIL_GIVEN 02\n+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n+static int user_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203109","messageId":"20121113165203.GB12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 2/6] ident: keep separate \"explicit\" flags for author and committer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:52:03Z","receivedAt":"2012-11-13T16:52:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep track of whether the user ident was given to us\nexplicitly, or if we guessed at it from system parameters\nlike username and hostname. However, we kept only a single\nvariable. This covers the common cases (because the author\nand committer will usually come from the same explicit\nsource), but can miss two cases:\n\n 1. GIT_COMMITTER_* is set explicitly, but we fallback for\n GIT_AUTHOR. We claim the ident is explicit, even though\n the author is not.\n\n 2. GIT_AUTHOR_* is set and we ask for author ident, but\n not committer ident. We will claim the ident is\n implicit, even though it is explicit.\n\nThis patch uses two variables instead of one, updates both\nwhen we set the \"fallback\" values, and updates them\nindividually when we read from the environment.\n\nRather than keep user_ident_sufficiently_given as a\ncompatibility wrapper, we update the only two callers to\ncheck the committer_ident, which matches their intent and\nwhat was happening already.\n\nSigned-off-by: Jeff King \n---\nCase 1 made me initially think might need to check\nauthor_ident_sufficiently_given when deciding whether to show the\n\"Author:\" line during commit. But I don't think it is necessary, as we\nalready check !strcmp(author, committer); if the committer is explicit\nand the author is identical, even if it is implicit, there is no point\nin telling the user.\n\n builtin/commit.c | 4 ++--\n cache.h | 3 ++-\n ident.c | 32 +++++++++++++++++++++++++-------\n 3 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dd2ec5..d6dd3df 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n \t\t\t\tauthor_ident->buf);\n \n-\t\tif (!user_ident_sufficiently_given())\n+\t\tif (!committer_ident_sufficiently_given())\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Committer: %s\"),\n@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \t\tstrbuf_addstr(&format, \"\\n Author: \");\n \t\tstrbuf_addbuf_percentquote(&format, &author_ident);\n \t}\n-\tif (!user_ident_sufficiently_given()) {\n+\tif (!committer_ident_sufficiently_given()) {\n \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n \t\tstrbuf_addbuf_percentquote(&format, &committer_ident);\n \t\tif (advice_implicit_identity) {\ndiff --git a/cache.h b/cache.h\nindex 50d9eea..18fdd18 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,7 +1149,8 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-extern int user_ident_sufficiently_given(void);\n+extern int committer_ident_sufficiently_given(void);\n+extern int author_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\ndiff --git a/ident.c b/ident.c\nindex 733d69d..ac9672f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -14,7 +14,8 @@ static char git_default_date[50];\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-static int user_ident_explicitly_given;\n+static int committer_ident_explicitly_given;\n+static int author_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n@@ -113,7 +114,8 @@ const char *ident_default_email(void)\n \n \t\tif (email && email[0]) {\n \t\t\tstrbuf_addstr(&git_default_email, email);\n-\t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else\n \t\t\tcopy_email(xgetpwuid_self(), &git_default_email);\n \t\tstrbuf_trim(&git_default_email);\n@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)\n \n const char *git_author_info(int flag)\n {\n+\tif (getenv(\"GIT_AUTHOR_NAME\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n \t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n \t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n@@ -340,16 +346,16 @@ const char *git_author_info(int flag)\n const char *git_committer_info(int flag)\n {\n \tif (getenv(\"GIT_COMMITTER_NAME\"))\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n \n-int user_ident_sufficiently_given(void)\n+static int ident_is_sufficient(int user_ident_explicitly_given)\n {\n #ifndef WINDOWS\n \treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)\n #endif\n }\n \n+int committer_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(committer_ident_explicitly_given);\n+}\n+\n+int author_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(author_ident_explicitly_given);\n+}\n+\n int git_ident_config(const char *var, const char *value, void *data)\n {\n \tif (!strcmp(var, \"user.name\")) {\n@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_name);\n \t\tstrbuf_addstr(&git_default_name, value);\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n \n@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_email);\n \t\tstrbuf_addstr(&git_default_email, value);\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"203110","messageId":"20121113165247.GC12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 3/6] var: accept multiple variables on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:52:47Z","receivedAt":"2012-11-13T16:52:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git-var currently only accepts a single value to print. This\nis inefficient if the caller is interested in finding\nmultiple values, as they must invoke git-var multiple times.\n\nThis patch lets callers specify multiple variables, and\nprints one per line.\n\nSigned-off-by: Jeff King \n---\nThis will later let us get the \"explicit\" flag for free.\n\n Documentation/git-var.txt | 9 +++++++--\n builtin/var.c | 13 +++++++------\n t/t0007-git-var.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 43 insertions(+), 8 deletions(-)\n create mode 100755 t/t0007-git-var.sh\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 67edf58..53abba5 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -9,11 +9,16 @@ git-var - Show a git logical variable\n SYNOPSIS\n --------\n [verse]\n-'git var' ( -l | )\n+'git var' ( -l | ... )\n \n DESCRIPTION\n -----------\n-Prints a git logical variable.\n+Prints one or more git logical variables, separated by newlines.\n+\n+Note that some variables may contain newlines themselves (e.g.,\n+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output\n+when requesting multiple variables. This can be mitigated by putting any\n+such variables at the end of the list.\n \n OPTIONS\n -------\ndiff --git a/builtin/var.c b/builtin/var.c\nindex aedbb53..49b48f5 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *val = NULL;\n-\tif (argc != 2)\n+\tif (argc < 2)\n \t\tusage(var_usage);\n \n \tif (strcmp(argv[1], \"-l\") == 0) {\n@@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n-\tval = read_var(argv[1]);\n-\tif (!val)\n-\t\tusage(var_usage);\n \n-\tprintf(\"%s\\n\", val);\n+\twhile (*++argv) {\n+\t\tconst char *val = read_var(*argv);\n+\t\tif (!val)\n+\t\t\tusage(var_usage);\n+\t\tprintf(\"%s\\n\", val);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nnew file mode 100755\nindex 0000000..45a5f66\n--- /dev/null\n+++ b/t/t0007-git-var.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='basic sanity checks for git var'\n+. ./test-lib.sh\n+\n+test_expect_success 'get GIT_AUTHOR_IDENT' '\n+\ttest_tick &&\n+\techo \"A U Thor 1112911993 -0700\" >expect &&\n+\tgit var GIT_AUTHOR_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get GIT_COMMITTER_IDENT' '\n+\ttest_tick &&\n+\techo \"C O Mitter 1112912053 -0700\" >expect &&\n+\tgit var GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git var can show multiple values' '\n+\tcat >expect <<-\\EOF &&\n+\tA U Thor 1112912053 -0700\n+\tC O Mitter 1112912053 -0700\n+\tEOF\n+\tgit var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203111","messageId":"20121113165308.GD12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 4/6] var: provide explicit/implicit ident information","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:53:08Z","receivedAt":"2012-11-13T16:53:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Internally, we keep track of whether the author or committer\nident information was provided by the user, or whether it\nwas implicitly determined by the system. However, there is\ncurrently no way for external programs or scripts to get\nthis information without re-implementing the ident logic\nthemselves. Let's provide a way for them to do so.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-var.txt | 8 ++++++++\n builtin/var.c | 27 +++++++++++++++++++++++++++\n t/t0007-git-var.sh | 36 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 71 insertions(+)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 53abba5..963b8d4 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -39,9 +39,17 @@ VARIABLES\n GIT_AUTHOR_IDENT::\n The author of a piece of code.\n \n+GIT_AUTHOR_EXPLICIT::\n+ Outputs \"1\" if the author identity was provided explicitly by the\n+ user (in the config or environment), or \"0\" if it was determined\n+ implicitly from the system.\n+\n GIT_COMMITTER_IDENT::\n The person who put a piece of code into git.\n \n+GIT_COMMITTER_EXPLICIT::\n+ Like GIT_AUTHOR_EXPLICIT, but for the committer ident.\n+\n GIT_EDITOR::\n Text editor for use by git commands. The value is meant to be\n interpreted by the shell when it is used. Examples: `~/bin/vi`,\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 49b48f5..ce28101 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -26,13 +26,40 @@ static const char *pager(int flag)\n \treturn pgm;\n }\n \n+static const char *explicit_ident(const char * (*get_ident)(int),\n+\t\t\t\t int (*check_ident)(void))\n+{\n+\t/*\n+\t * Prime the \"explicit\" flag by getting the identity.\n+\t * We do not want to be strict here, because we would not want\n+\t * to die on bogus implicit values, but instead just report that they\n+\t * are not explicit.\n+\t */\n+\tget_ident(0);\n+\treturn check_ident() ? \"1\" : \"0\";\n+}\n+\n+static const char *committer_explicit(int flag)\n+{\n+\treturn explicit_ident(git_committer_info,\n+\t\t\t committer_ident_sufficiently_given);\n+}\n+\n+static const char *author_explicit(int flag)\n+{\n+\treturn explicit_ident(git_author_info,\n+\t\t\t author_ident_sufficiently_given);\n+}\n+\n struct git_var {\n \tconst char *name;\n \tconst char *(*read)(int);\n };\n static struct git_var git_vars[] = {\n \t{ \"GIT_COMMITTER_IDENT\", git_committer_info },\n+\t{ \"GIT_COMMITTER_EXPLICIT\", committer_explicit },\n \t{ \"GIT_AUTHOR_IDENT\", git_author_info },\n+\t{ \"GIT_AUTHOR_EXPLICIT\", author_explicit },\n \t{ \"GIT_EDITOR\", editor },\n \t{ \"GIT_PAGER\", pager },\n \t{ \"\", NULL },\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex 45a5f66..66b9810 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -26,4 +26,40 @@ test_expect_success 'git var can show multiple values' '\n \ttest_cmp expect actual\n '\n \n+for type in AUTHOR COMMITTER; do\n+\ttest_expect_success \"$type ident can detect implicit values\" \"\n+\t\techo 0 >expect &&\n+\t\t(\n+\t\t\tsane_unset GIT_${type}_NAME &&\n+\t\t\tsane_unset GIT_${type}_EMAIL &&\n+\t\t\tsane_unset EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t) &&\n+\t\ttest_cmp expect actual\n+\t\"\n+\n+\ttest_expect_success \"$type ident is explicit via environment\" \"\n+\t\techo 1 >expect &&\n+\t\t(\n+\t\t\tGIT_${type}_NAME='A Name' &&\n+\t\t\texport GIT_${type}_NAME &&\n+\t\t\tGIT_${type}_EMAIL='name@example.com' &&\n+\t\t\texport GIT_${type}_EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t) &&\n+\t\ttest_cmp expect actual\n+\t\"\n+\n+\ttest_expect_success \"$type ident is explicit via config\" \"\n+\t\techo 1 >expect &&\n+\t\ttest_config user.name 'A Name' &&\n+\t\ttest_config user.email 'name@example.com' &&\n+\t\t(\n+\t\t\tsane_unset GIT_${type}_NAME &&\n+\t\t\tsane_unset GIT_${type}_EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t)\n+\t\"\n+done\n+\n test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203113","messageId":"20121113165320.GE12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 5/6] Git.pm: teach \"ident\" to query explicitness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:53:20Z","receivedAt":"2012-11-13T16:53:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"\"git var\" recently learned to report on whether an ident we\nfetch from it was configured explicitly or implicitly. Let's\nmake that information available to callers of the ident\nfunction.\n\nBecause evaluating \"ident\" in an array versus scalar context\nalready has a meaning, we cannot return our extra value in a\nbackwards compatible way. Instead, we require the caller to\nadd an extra \"explicit\" flag to request the information.\nThe ident_person function, on the other hand, always returns\na scalar, so we are free to overload it in an array context.\n\nSigned-off-by: Jeff King \n---\n perl/Git.pm | 28 ++++++++++++++++++++--------\n 1 file changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 497f420..1994ec1 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -737,7 +737,7 @@ sub remote_refs {\n }\n \n \n-=item ident ( TYPE | IDENTSTR )\n+=item ident ( TYPE | IDENTSTR [, options] )\n \n =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )\n \n@@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.\n Alternatively, it can take a prepared ident string (e.g. from the commit\n object) and just parse it.\n \n+If the C option is set to 1, the returned array will contain an\n+additional boolean specifying whether the ident was configure explicitly by the\n+user.\n+\n C returns the person part of the ident - name and email;\n it can take the same arguments as C or the array returned by C.\n \n@@ -763,17 +767,22 @@ The synopsis is like:\n =cut\n \n sub ident {\n-\tmy ($self, $type) = _maybe_self(@_);\n-\tmy $identstr;\n+\tmy ($self, $type, %options) = _maybe_self(@_);\n+\tmy ($identstr, $explicit);\n \tif (lc $type eq lc 'committer' or lc $type eq lc 'author') {\n-\t\tmy @cmd = ('var', 'GIT_'.uc($type).'_IDENT');\n+\t\tmy $uc = uc($type);\n+\t\tmy @cmd = ('var', \"GIT_${uc}_IDENT\", \"GIT_${uc}_EXPLICIT\");\n \t\tunshift @cmd, $self if $self;\n-\t\t$identstr = command_oneline(@cmd);\n+\t\t($identstr, $explicit) = command(@cmd);\n \t} else {\n \t\t$identstr = $type;\n \t}\n \tif (wantarray) {\n-\t\treturn $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n+\t\tmy @ret = $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n+\t\tif ($options{explicit} && defined $explicit) {\n+\t\t\tpush @ret, $explicit if defined $explicit;\n+\t\t}\n+\t\treturn @ret;\n \t} else {\n \t\treturn $identstr;\n \t}\n@@ -781,8 +790,11 @@ sub ident {\n \n sub ident_person {\n \tmy ($self, @ident) = _maybe_self(@_);\n-\t$#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);\n-\treturn \"$ident[0] <$ident[1]>\";\n+\t$#ident == 0 and @ident = $self ?\n+\t\t\t\t $self->ident($ident[0], explicit => 1) :\n+\t\t\t\t ident($ident[0], explicit => 1);\n+\tmy $ret = \"$ident[0] <$ident[1]>\";\n+\treturn wantarray ? ($ret, @ident[3]) : $ret;\n }\n \n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"203112","messageId":"20121113165327.GF12626@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCH 6/6] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T16:53:27Z","receivedAt":"2012-11-13T16:53:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If git-send-email is configured with sendemail.from, we will\nnot prompt the user for the \"From\" address of the emails.\nIf it is not configured, we prompt the user, but provide the\nrepo author or committer as a default. Even though we\nprobably have a sensible value for the default, the prompt\nis a safety check in case git generated an incorrect\nimplicit ident string.\n\nNow that Git.pm will tell us whether the ident is implicit or\nexplicit, we can stop prompting in the explicit case, saving\nmost users from having to see the prompt at all.\n\nThe test scripts need to be adjusted to not expect a prompt\nfor the sender, since they always have the author explicitly\ndefined in the environment. Unfortunately, we cannot\nreliably test that prompting still happens in the implicit\ncase, as send-email will produce inconsistent results\ndepending on the machine config (if we cannot find a FQDN,\n\"git var\" will barf, causing us to exit early; if we can,\nsend-email will continue but prompt).\n\nSigned-off-by: Jeff King \n---\n git-send-email.perl | 22 +++++++++++++---------\n t/t9001-send-email.sh | 5 ++---\n 2 files changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5a7c29d..0c49b32 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -436,9 +436,8 @@ if (0) {\n \t}\n }\n \n-my ($repoauthor, $repocommitter);\n-($repoauthor) = Git::ident_person(@repo, 'author');\n-($repocommitter) = Git::ident_person(@repo, 'committer');\n+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');\n+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');\n \n # Verify the user input\n \n@@ -755,12 +754,17 @@ if (!$force) {\n \n my $prompting = 0;\n if (!defined $sender) {\n-\t$sender = $repoauthor || $repocommitter || '';\n-\t$sender = ask(\"Who should the emails appear to be from? [$sender] \",\n-\t default => $sender,\n-\t\t valid_re => qr/\\@.*\\./, confirm_only => 1);\n-\tprint \"Emails will be sent from: \", $sender, \"\\n\";\n-\t$prompting++;\n+\t($sender, my $explicit) =\n+\t\tdefined $repoauthor ? ($repoauthor, $author_explicit) :\n+\t\tdefined $repocommitter ? ($repocommitter, $committer_explicit) :\n+\t\t('', 0);\n+\tif (!$explicit) {\n+\t\t$sender = ask(\"Who should the emails appear to be from? [$sender] \",\n+\t\t\t default => $sender,\n+\t\t\t valid_re => qr/\\@.*\\./, confirm_only => 1);\n+\t\tprint \"Emails will be sent from: \", $sender, \"\\n\";\n+\t\t$prompting++;\n+\t}\n }\n \n if (!@initial_to && !defined $to_cmd) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 6c6af7d..c5d66cf 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -191,14 +191,13 @@ test_expect_success $PREREQ 'Show all headers' '\n \n test_expect_success $PREREQ 'Prompting works' '\n \tclean_fake_sendmail &&\n-\t(echo \"Example \"\n-\t echo \"to@example.com\"\n+\t(echo \"to@example.com\"\n \t echo \"\"\n \t) | GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$patches \\\n \t\t2>errors &&\n-\t\tgrep \"^From: Example \\$\" msgtxt1 &&\n+\t\tgrep \"^From: A U Thor \\$\" msgtxt1 &&\n \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n '\n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"203116","messageId":"20121113171445.GG20361@sigill.intra.peff.net","threadId":"32079","inReplyTo":"7vpq3hfpm7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T17:14:46Z","receivedAt":"2012-11-13T17:14:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:\n\n> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n> >\n> > Yeah, I find that somewhat questionable in the current behavior, and I'd\n> > consider it a bug. Typically we prefer the committer ident when given a\n> > choice (e.g., for writing reflog entries).\n> \n> Just to make sure I follow the discussion correctly, do you mean\n> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is\n> specified more concretely and we usually use COMMIITTER for this\n> kind of thing in the first place but send-email does not in this\n> case (I do not see \"git var GIT_AUTHOR_IDENT\" returning value from\n> the implicit logic as a bug in this case---just making sure).\n\nHaving discussed more, I think there are two questionable things:\n\n 1. Preferring author over committer\n\n 2. Failing to fall back to committer when author is implicit or bogus\n (because \"git var\" dies).\n\nI think (1) may fall into the \"this is not how I would do it today, but\nit is not worth a possible regression\" category.\n\nI think (2) might be worth fixing, though. Certainly when the author is\nbogus (by IDENT_STRICT rules), which I think was the original intent of\nthe \"$repoauthor || $repocommitter\" code. Probably when the author ident\nis implicit, though that is more hazy to me.\n\n> For a change with low benefit/cost ratio like this, the best course\n> of action often is to stop paying attention to the thread that has\n> become unproductive and venomous, and resurrect the topic after\n> people forgot about it and doing it right yourself ;-).\n\nI came to the same conclusion, but decided to simply do it right now\nwhile it was in my head. Hopefully we can progress by reviewing the\nseries I just posted.\n\n-Peff\n"},{"id":"203117","messageId":"CABPQNSZ6_jaEf+_pVon6w=zX5_FFHRHO0KoUSnS67O-j3t9n7w@mail.gmail.com","threadId":"32079","inReplyTo":"20121112233546.GG10531@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-11-13T17:20:38Z","receivedAt":"2012-11-13T17:20:38Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Nov 13, 2012 at 12:35 AM, Jeff King wrote:\n> On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:\n>\n>> There's no point in asking this over and over if the user already\n>> properly configured his/her name and email.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>>\n>> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.\n>\n> It should be defaulting to your regular git ident, and you just have to\n> hit enter, right?\n>\n> I think it's probably reasonable to skip that \"enter\" in most cases. But\n> I'm not sure why we ever asked in the first place. What do people input\n> there if they are not taking the default?\n>\n\nI usually input \"Linus Torvalds \", to\ngive my patch high priority ;-)\n"},{"id":"203119","messageId":"7vvcd9e7rt.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121113171445.GG20361@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T17:23:50Z","receivedAt":"2012-11-13T17:23:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:\n>\n>> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n>> >\n>> > Yeah, I find that somewhat questionable in the current behavior, and I'd\n>> > consider it a bug. Typically we prefer the committer ident when given a\n>> > choice (e.g., for writing reflog entries).\n>> \n>> Just to make sure I follow the discussion correctly, do you mean\n>> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is\n>> specified more concretely and we usually use COMMIITTER for this\n>> kind of thing in the first place but send-email does not in this\n>> case (I do not see \"git var GIT_AUTHOR_IDENT\" returning value from\n>> the implicit logic as a bug in this case---just making sure).\n>\n> Having discussed more, I think there are two questionable things:\n>\n> 1. Preferring author over committer\n>\n> 2. Failing to fall back to committer when author is implicit or bogus\n> (because \"git var\" dies).\n>\n> I think (1) may fall into the \"this is not how I would do it today, but\n> it is not worth a possible regression\" category.\n>\n> I think (2) might be worth fixing, though. Certainly when the author is\n> bogus (by IDENT_STRICT rules), which I think was the original intent of\n> the \"$repoauthor || $repocommitter\" code. Probably when the author ident\n> is implicit, though that is more hazy to me.\n\nI agree with both points. Thanks.\n"},{"id":"203120","messageId":"20121113172508.GA12951@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113172300.GA16241@ftbfs.org","subject":"Re: [PATCH 5/6] Git.pm: teach \"ident\" to query explicitness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T17:25:08Z","receivedAt":"2012-11-13T17:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 09:23:00AM -0800, Matt Kraai wrote:\n\n> Minor nits:\n> \n> On Tue, Nov 13, 2012 at 11:53:20AM -0500, Jeff King wrote:\n> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.\n> > Alternatively, it can take a prepared ident string (e.g. from the commit\n> > object) and just parse it.\n> > \n> > +If the C option is set to 1, the returned array will contain an\n> > +additional boolean specifying whether the ident was configure explicitly by the\n> \n> s/configure/configured/\n\nThanks.\n\n> > \tif (wantarray) {\n> > -\t\treturn $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n> > +\t\tmy @ret = $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n> > +\t\tif ($options{explicit} && defined $explicit) {\n> > +\t\t\tpush @ret, $explicit if defined $explicit;\n> \n> Isn't the test on this line redundant given that defined $explicit is\n> already guaranteed by the condition on the previous line?\n\nYes, thanks (left over from an earlier attempt that tried to avoid\nhaving $options{explicit}).\n\n-Peff\n"},{"id":"203144","messageId":"CAMP44s3sBj0iYsCLUpiouUB8PXRwLORDEyD_+dWKrSsMP+TOaw@mail.gmail.com","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-13T20:35:18Z","receivedAt":"2012-11-13T20:35:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 13, 2012 at 5:48 PM, Jeff King wrote:\n> On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:\n\n>> I think you are the one that is not understanding what I'm saying. But\n>> I don't think it matters.\n>>\n>> This is what I'm saying; the current situation with 'git commit' is\n>> not OK, _both_ 'git commit' and 'git send-email' should change.\n>\n> Perhaps I am being dense, but this is the first time it became apparent\n> to me that you are arguing for a change in \"git commit\".\n\nMiscomunication then. When you mentioned 'it has always been the case\nthat you can use git without setting\nuser.*', I directed my comments at git in general, not git send-email.\n\n>> Indeed I would, but there's other people that would benefit from this\n>> patch. I'm sure I'm not the only person that doesn't have\n>> sendmail.from configured, but does have user.name/user.email, and is\n>> constantly typing enter.\n>>\n>> And the difference is that I'm _real_, the hypothetical user that\n>> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced\n>> otherwise if some evidence was presented that such a user is real\n>> though.\n>\n> Sadly we cannot poll the configuration of every user, nor expect them\n> all to pay attention to this discussion on the list. So we cannot take\n> the absence of comment from such users as evidence that they do not\n> exist.\n\nAnd we cannot take it as evidence that these users do exist either.\n\nThe absence of evidence simply means that... we don't have evidence.\n\n> Instead we must use our judgement about what is being changed,\n> and we tend to err on the side of keeping the status quo, since that is\n> what the silent majority is busy _not_ complaining about.\n\nYes, the keyword is *tend*; this wouldn't be first or the last time\nthat a behavior change happens.\n\nWe have to be careful about these changes, but we do them.\n\n> We use the same judgement on the other side, too. Right now your\n> evidence is \"1 user wants this, 0 users do not\".\n\n1 is infinitely greater than 0.\n\n> But we can guess that\n> there are other people who would like the intent of your patch, but did\n> not care enough or are not active enough on the list to write the patch\n> themselves or comment on this thread.\n\nYes, that would be an educated guess. IMO the fact that people use\nGIT_AUTHOR_ variables to send mail is not. There's many ways to\nachieve that: sendemail.from, --from, user configuration, $EMAIL,\n--compose and From:, etc. each and every one of them much more likely\nto be used than GIT_AUTHOR_.\n\nBut I'm tired of arguing how extremely unlikely it is that such people\ndon't exist. Lets agree to disagree. Either way it doesn't matter,\nbecause nobody is proposing a patch that would affect these\nhypothetical users.\n\n>> And to balance you need to *measure*, and that means taking into\n>> consideration who actually uses the features, if there's any. And it\n>> looks to me this is a feature nobody uses.\n>\n> You said \"measure\" and then \"it looks to me like\". What did you measure?\n> Did you consider systematic bias in your measurement, like the fact that\n> people who are using the feature have no reason to come on the list and\n> announce it?\n\nmeasure != scientific measurement.\n\nI used common sense, because that's the only tool available.\n\nGIT_AUTHOR is plumbing, not very well known, it's cumbersome (you need\nto export two variables), it can be easily confused with\nGIT_COMMITTER, which wouldn't work on this case. And there's plenty of\ntools that much simpler to use, starting with 'git send-email --from',\nwhich is so user friendly it's in the --help. There's absolutely no\nreason why anybody would want to use GIT_AUTHOR.\n\nBut lets agree to disagree.\n\n>> But listen closely to what you said:\n>>\n>> > I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.\n>>\n>> Suppose somebody has a full name, and a fully qualified domain name,\n>> and he can receive mails to it directly. Such a user would not need a\n>> git configuration, and would not need $EMAIL, or anything.\n>>\n>> Currently 'git send-email' will throw 'Felipe Contreras\n>> ' which would actually work, but is not explicit.\n>>\n>> You are suggesting to break that use-case. You are introducing a\n>> regression. And this case is realistic, unlike the\n>> GIT_AUTHOR_NAME/EMAIL. Isn't it?\n>\n> Yes, dying would be a regression, in that you would have to configure\n> your name via the environment and re-run rather than type it at the\n> prompt. You raise a good point that for people who _could_ take the\n> implicit default, hitting \"enter\" is working fine now, and we would lose\n> that. I'd be fine with also just continuing to prompt in the implicit\n> case.\n>\n> But that is a much smaller issue to me than having send-email fail to\n> respect environment variables and silently use user.*, which is what\n> started this whole discussion. And I agree it is worth considering as a\n> regression we should avoid.\n\nIt might be smaller, I don't think so. A hypothetical user that was\nrelying on GIT_AUTHOR for whatever reason can switch to 'git\nsend-email --from' (which is much easier) when they notice the\nfailure, the same way somebody relying on fqdn would. The difference\nis that people with fqdn do exist, and they might be relying on this.\n\nBoth are small issues, that I agree with.\n\nBut the point is that you seem to be very adamant about _my_\nregressions, and not pay much attention about yours.\n\n>> I prefer to concentrate on real issues, but that's just me.\n>\n> To be honest, I am confused at this point what you actually want. Do you\n> think we should take your original patch? I think its regression is too\n> great. And I am not sure if you agree or not. You seem to be arguing\n> that the regression is not important, yet you simultaneously argue that\n> we should be making all ident more strict, which would mean that we\n> should indeed take my suggestion and use \"git var\" instead of preferring\n> the config.\n\nNo, I was proposing my second patch. That's why I sent a second patch.\n\nBut forget it. I don't think it's worth trying to do the right thing\nhere (make both 'git commit' and 'git send-email' be stricter). I do\nthink that would be simpler than all the required changes to var, and\nbetter, but that's not my itch atm.\n\n>> > As for whether they exist, what data do you have?\n>>\n>> What data do _you_ have?\n>>\n>> When there's no evidence either way, the rational response is to don't\n>> believe. That's the default position.\n>\n> This is not religion. It is a software project.\n\nThis has absolutely nothing to with religion, this is rationality. If\nyou don't want to make a distinction between the different levels at\nwhich we _know_ something, suit yourself.\n\n> In the absence of data,\n> the sane thing is not to break existing users.\n\nWhat you decide to do has absolutely nothing to do with what is true.\nThese hypothetical users exist, or they don't. I don't have data for\ntheir existence, you don't have data for their non-existence. In the\nabsence of data the rational response is to don't believe the claim,\nand this one is particularly clear because you cannot prove a\nnegative, so the burden of proof is clearly on your side, as it's\n*impossible* for me to prove that there are no users that use certain\nfeature, and it's very easy for you to do so, as all you need is\n*one*.\n\nPresumably you are not interested in fulfilling your burden of proof,\nwhich is fine, you can choose the safest action without proof that\nsuch users exist, but that doesn't change the fact that you don't know\nthat if they exist or not.\n\nAnd yet, you continue to say they do exist, and try to win the\nargument by means of rhetoric: 'not to break existing users'; they are\nnot *existing* users, they are hypothetical.\n\n> The burden is on you to\n> argue that there is no such breakage.\n\nThat's impossible to prove. *Nobody* can prove that people are not\nusing even the most obscure feature of git.\n\nAll you do is make educated guesses, and try to minimize the damage.\n\n>> > Are you aware that the\n>> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not\n>> > having any user.* config?\n>>\n>> What tests? My patch doesn't seem to break anything there:\n>> % make -C t t9001-send-email.sh\n>> # passed all 96 test(s)\n>\n> My point was that there is at least one known setup that uses only\n> environment variables and not a config file, and that the rest of git is\n> intended to work with that. It is not a test failure, but t9001.18\n> reveals the fact that your change does not handle this situation; we\n> continue to prompt under the test suite's configuration. In the proper\n> fix, the test needs to be adjusted.\n>\n> You don't see any test failures with your patch because we do not cover\n> the case that you regress (GIT_AUTHOR_EMAIL and user.email both set).\n\nExactly, but I was not the one that brought the test suite. My patch\nis not a problem for our current tests. So this hypothetical user\nremains evasive.\n\n>> > When somebody comes on the list and asks why\n>> > every git program in the entire system respects GIT_* environment\n>> > variables as an override to user.* configuration _except_ for\n>> > send-email, what should I say?\n>>\n>> The same thing you say when somebody comes reporting a bug: \"yeah, we\n>> should probably fix that\".\n>\n> It is a larger hassle for both the developer and the user to fix the bug\n> after the fact,\n\n*If* that happens, which in all likelihood it won't.\n\n> ship a new version, and then have the user upgrade their\n> git. Why not just not introduce the bug in the first place?\n\nBecause we are spending more time arguing about something that most\nlikely won't happen.\n\nThere's a chance the next time you sit the chair will break because of\na bad leg, but you don't engage in philosophical discussions each time\nyou are going to sit, you go ahead and sit down, and take your\nchances, and 99.9% of the time you would be fine.\n\n>> It's all about proportion. Is it possible that we all are going to die\n>> tomorrow because of an asteroid? Sure... but what's the point of\n>> worrying about it if it's not likely?\n>\n> If you want to talk about risk assessment, then the right computation is\n> to compare the cost of fixing it now versus the cost of fixing it later\n> times the probability of it happening. The development cost is probably\n> a little higher later (because we have to refresh ourselves on the\n> issue), but the deployment cost is much higher (e.g., users whose\n> distros ship a broken version, or who have IT policy that does not let\n> them upgrade git as soon as the bug-fix ships).\n\nTimes 0.01 (at best), and you get barely nothing.\n\n>> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.\n>> >\n>> > Yeah, I find that somewhat questionable in the current behavior, and I'd\n>> > consider it a bug. Typically we prefer the committer ident when given a\n>> > choice (e.g., for writing reflog entries).\n>>\n>> Yeah, but clearly the intention of the code was to use the committer\n>> if the author wasn't available, which is the case here.\n>\n> Yes. It would make sense to me to respect an explicit committer over an\n> implicit author. Though the fact that author is preferred over committer\n> is inconsistent with most of the rest of git. I'm undecided whether that\n> should be changed.\n\nIndeed. And that's something I won't worry about, because I honestly\nbelieve nobody is using neither GIT_COMMITTER, nor GIT_AUTHOR, and the\nfact that nobody has found this breakage only increases my certainty.\n\n>> >> What about after my change?\n>> >>\n>> >> 6.1) GIT_AUTHOR without anything else\n>> >>\n>> >> fatal: empty ident name (for ) not allowed\n>> >> var GIT_COMMITTER_IDENT: command returned error: 128\n>> >\n>> > Doesn't that seem like a regression? It used to work.\n>>\n>> No, this is *before* my change.\n>>\n>> I's the same bug as 5.1):\n>\n> Ah, I see. We are loading both, whether or not they end up being used,\n> and barfing prematurely on errors. That seems like a bug.\n\nYes.\n\n>> >> And what about your proposed change?\n>> >\n>> > Let me be clear that I sent you a \"something like this\" patch to try to\n>> > point you in the right direction. If it has a bug or is incomplete, that\n>> > does not mean the direction is wrong, but only that I did not spend very\n>> > much time on the patch.\n>>\n>> It doesn't matter, the idea was to use user_ident_sufficiently_given().\n>\n> Right. Which it sounds like now agree with me on?\n\nNo. I tried the idea, it didn't work.\n\nI saw your patches about\n{committer,author}_ident_sufficiently_given(), those might work, but I\ndon't think they are _needed_. They might be nice though.\n\n>> > I think that respecting the usual ident lookup but disallowing implicit\n>> > identities (either totally, or causing them to fallback to prompting) is\n>> > the right direction. I agree my patch was not a complete solution. I'm\n>> > sorry if it led you astray in terms of implementation, but I also think\n>> > I've been very clear in my text about what the behavior should be.\n>>\n>> I think that is orthogonal to what I'm trying accomplish.\n>\n> What is it you are trying to accomplish? Eliminating the prompt in some\n> cases, as in your original patch? Dealing with implicit identities may\n> be orthogonal to your goal, but your original patch is not sufficient,\n> as it reverses the precedence of config and environment variables.\n\nNo, it doesn't.\n\nWhy do you hold on to my first patch? The second patch doesn't have\nthis issue. It does change the behavior of 'git commit', yeah, but I\nthink that's a benefit.\n\nYou might disagree, but there's no point in going back to discuss the\nfirst patch.\n\n> So you can either:\n>\n> 1. Reimplement the environment variable lookup that ident.c does,\n> leaving implicit ident logic out completely.\n>\n> 2. Modify ident.c and \"git var\" to let send-email reuse the logic in\n> ident.c, but avoid dropping the prompt when an implicit ident is\n> used.\n>\n> Doing (2) sounds a lot more maintainable to me in the long run.\n\nOr:\n\n3. Change the meaning of the STRICT flag so that the values are\nexplicit, which has benefits outside 'git send-email'. Yes, this would\nchange the behavior in 'git commit' and other tools, but it's worth to\ninvestigate these changes, and most likely they would be desirable.\n\nOr:\n\n4. Just stop prompting\n\nI already sent a patch for 4. with all the details of why nobody (or\nvery few, if any) would be affected negatively.\n\n>> > Don't get me wrong. I think the spirit of your patch is correct, and it\n>> > helps some git users. But it also hurts others. And it is not that hard\n>> > to do it right.\n>>\n>> And I disagree, I think it hurts nobody, and I think it's hard to do it right.\n>\n> I am tired of talking about this. The patch series below took me less\n> time to write than I have spent arguing with you. It was not that hard.\n>\n> [1/6]: ident: make user_ident_explicitly_given private\n> [2/6]: ident: keep separate \"explicit\" flags for author and committer\n> [3/6]: var: accept multiple variables on the command line\n> [4/6]: var: provide explicit/implicit ident information\n> [5/6]: Git.pm: teach \"ident\" to query explicitness\n> [6/6]: send-email: do not prompt for explicit repo ident\n\nI think this adds a lot of code that nobody would use.\n\nThe prompt will happen only if:\n\n1) No configured user.name/user.email\n2) No specified $EMAIL\n3) No configured sendemail.from\n4) No specified --from argument\n5) A fully qualified domain name\n6) A full name in the geckos field\n\nVery, very unlikely.\n\nAnd if:\n\n* A sendmail configuration doesn't allow sending from this domain name\n* The user sees the problem in the confirmation part\n\nThen the user would see a problem anyway, so the prompt is not adding\nthat much value.\n\n>> Fixing a regression that nobody would notice is not my itch either,\n>\n> Sorry, but it is part of your itch. The git project is at state A. With\n> your patch, we are at a regressed state B.\n\nAnd that's fine.\n\n> If you then fix it, we are at\n> state C. From your perspective, it may be \"I do not want to make the fix\n> to go from B to C\". But from the project's perspective, it is \"we do not\n> want to go from state A to state B; state C would be acceptable\". So\n> from the perspective of people who do not care about your feature but do\n> care about the regression in state B, it is inextricably linked to your\n> itch.\n\nNo it's not. It's a conflict of interests. It's only linked as long as\npeople care about the \"regression\" in state B. If they realize this\n\"regression\" is irrelevant, then there wouldn't be any problem. This,\nis now clear is not going to happen. But I don't think your opinions\ndefine whether or not I have an itch.\n\n>> yet the patch I sent above does it, and it even fixes 'git commit'\n>> (IMO). But it's also not good enough.\n>\n> I do not necessarily agree on \"git commit\". Moreover, I feel like it is\n> a separate issue. My series above _just_ implements the \"do not prompt\n> when explicit\" behavior. It does not deal with git-commit at all, nor\n> does it address the author/committer fallback questions. Those can\n> easily go on top.\n\nYes, at the cost of adding a lot of code. If we end up agreeing that\nthe changes to 'git commit' are desirable (which I hope at some point\nwe will), then this code would be all for nothing.\n\nI sent another patch series that is very simple[1]. This deals with\nthe hypothetical GIT_AUTHOR users, but might hit another set of\nhypothetical users, but the damage would be very small, and the\npossibility of these users existing very low. IOW; very low risk.\n\nI want clarify that this is merely a disagreement to at which level\nshould we worry about regressions. On one side of the spectrum you\nhave projects like GNOME, who don't have any problem breaking the\nuser-experience from one release to the next, I'm not proposing\nanything like that. On the other side I think it's you, because I\ndon't recall encountering anybody with such an extreme position of\nnever introducing a regression ever if there's absolutely no evidence\nthat anybody is using certain feature. That's fine, it's your opinion,\nI respectfully disagree. Personally, I think the sweet spot is between\nthe two, far from GNOME, but not quite to the point where no\nregressions happen ever. When there's a good chance that very few\npeople will be hit, and the drawback is small, then it's OK to\nintroduce regressions. Always keeping in mind that these risk\nassessments might be wrong, and there might be more people hit than\nexpected and/or the problems were greater, but if/when that happens,\n*then* we simply deal with it.\n\nCheers.\n\n[1] http://mid.gmane.org/1352834364-2674-1-git-send-email-felipe.contreras@gmail.com\n\n-- \nFelipe Contreras\n"},{"id":"203196","messageId":"20121114164457.GA6858@elie.Belkin","threadId":"32079","inReplyTo":"20121113164931.GA12626@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] ident: make user_ident_explicitly_given private","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-14T16:44:57Z","receivedAt":"2012-11-14T16:44:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> There are no users of this global variable, as queriers\n> go through the user_ident_sufficiently_given accessor.\n> Let's make it private, which will enable further\n> refactoring.\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1149,10 +1149,6 @@ struct config_include_data {\n> #define CONFIG_INCLUDE_INIT { 0 }\n> extern int git_config_include(const char *name, const char *value, void *data);\n> \n> -#define IDENT_NAME_GIVEN 01\n> -#define IDENT_MAIL_GIVEN 02\n> -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n> -extern int user_ident_explicitly_given;\n> extern int user_ident_sufficiently_given(void);\n\nIn v1.5.6-rc0~56^2 (2008-05-04) \"user_ident_explicitly_given\" was\nintroduced as a global for communication between config, ident, and\nbuiltin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to\nusing the common wrapper user_ident_sufficiently_given(). After\nv1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,\nand the variable can finally be made static.\n\nThis patch finally does that, which is a nice way to make cache.h\neasier to read and change less often.\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n"},{"id":"203197","messageId":"20121114170148.GB6858@elie.Belkin","threadId":"32079","inReplyTo":"20121113165247.GC12626@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] var: accept multiple variables on the command line","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-14T17:01:48Z","receivedAt":"2012-11-14T17:01:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This patch lets callers specify multiple variables, and\n> prints one per line.\n\nYay!\n\n[...]\n> --- a/Documentation/git-var.txt\n> +++ b/Documentation/git-var.txt\n> @@ -9,11 +9,16 @@ git-var - Show a git logical variable\n> SYNOPSIS\n> --------\n> [verse]\n> -'git var' ( -l | )\n> +'git var' ( -l | ... )\n> \n> DESCRIPTION\n> -----------\n> -Prints a git logical variable.\n> +Prints one or more git logical variables, separated by newlines.\n> +\n> +Note that some variables may contain newlines themselves\n\nMaybe a -z option to NUL-terminate values would be useful some day.\n\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)\n> \n> int cmd_var(int argc, const char **argv, const char *prefix)\n> {\n> -\tconst char *val = NULL;\n> -\tif (argc != 2)\n> +\tif (argc < 2)\n> \t\tusage(var_usage);\n> \n> \tif (strcmp(argv[1], \"-l\") == 0) {\n\nWhat should happen if I pass \"-l\" followed by other arguments?\n\n[...]\n> --- /dev/null\n> +++ b/t/t0007-git-var.sh\n> @@ -0,0 +1,29 @@\n> +#!/bin/sh\n> +\n> +test_description='basic sanity checks for git var'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'get GIT_AUTHOR_IDENT' '\n> +\ttest_tick &&\n> +\techo \"A U Thor 1112911993 -0700\" >expect &&\n\nDo we need to hardcode the timestamp? Something like\n\n\ttest_cmp_filtered () {\n\t\texpect=$1 actual=$2 &&\n\t\tsed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP\" \\\n\t\t\t<\"$actual\" >\"$actual.filtered\" &&\n\t\ttest_cmp \"$expect\" \"$actual.filtered\"\n\t}\n\t...\n\n\t\techo \"A U Thor $timestamp\" >expect &&\n\t\tgit var GIT_AUTHOR_IDENT >actual &&\n\t\ttest_cmp_filtered expect actual\n\nshould make reordering tests a lot easier, though it has the downside\nof not being able to catch a weird bug that would make the timestamp\nout of sync with reality.\n\nHope that helps,\nJonathan\n"},{"id":"203198","messageId":"20121114170657.GC6858@elie.Belkin","threadId":"32079","inReplyTo":"20121113165308.GD12626@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] var: provide explicit/implicit ident information","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-14T17:06:57Z","receivedAt":"2012-11-14T17:06:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Internally, we keep track of whether the author or committer\n> ident information was provided by the user, or whether it\n> was implicitly determined by the system. However, there is\n> currently no way for external programs or scripts to get\n> this information\n\nWhat are the intended semantics? If my machine has /etc/mailname\nfilled out, is that an implicit identity? How about if I set the\nEMAIL envvar but not GIT_COMMITTER_EMAIL?\n\nIf external scripts are going to start using this mechanism, they will\nneed answers to these questions to support users that run into\nconfiguration problems. A few words on this in the documentation\ncould probably help.\n\nOn most machines I have the EMAIL envvar set explicitly, but in the\nrecent past I relied on /etc/mailname on some others, so I'm also\ngenuinely curious about the use case here (and too lazy to dig it up).\n\nThanks,\nJonathan\n"},{"id":"203199","messageId":"20121114171213.GD6858@elie.Belkin","threadId":"32079","inReplyTo":"20121113165320.GE12626@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] Git.pm: teach \"ident\" to query explicitness","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-14T17:12:13Z","receivedAt":"2012-11-14T17:12:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> \"git var\" recently learned to report on whether an ident we\n> fetch from it was configured explicitly or implicitly. Let's\n> make that information available to callers of the ident\n> function.\n\nSounds sensible. Quick nits:\n\n[...]\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -737,7 +737,7 @@ sub remote_refs {\n> }\n> \n> \n> -=item ident ( TYPE | IDENTSTR )\n> +=item ident ( TYPE | IDENTSTR [, options] )\n> \n> =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )\n> \n> @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.\n> Alternatively, it can take a prepared ident string (e.g. from the commit\n> object) and just parse it.\n> \n> +If the C option is set to 1, the returned array will contain an\n> +additional boolean specifying whether the ident was configure explicitly by the\n> +user.\n\ns/configure/configured/\n\nI'd suggest adding \"See GIT_COMMITTER_EXPLICIT in git-var(1) for\ndetails\" to make the semantics crystal clear. What do you think?\n\nThanks,\nJonathan\n"},{"id":"203200","messageId":"20121114171827.GE6858@elie.Belkin","threadId":"32079","inReplyTo":"20121113165327.GF12626@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-14T17:18:27Z","receivedAt":"2012-11-14T17:18:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> If git-send-email is configured with sendemail.from, we will\n> not prompt the user for the \"From\" address of the emails.\n> If it is not configured, we prompt the user, but provide the\n> repo author or committer as a default. Even though we\n> probably have a sensible value for the default, the prompt\n> is a safety check in case git generated an incorrect\n> implicit ident string.\n\nI haven't read the code carefully, but this behavior sounds sensible,\nso for what it's worth,\nAcked-by: Jonathan Nieder \n\n[...]\n> The test scripts need to be adjusted to not expect a prompt\n> for the sender, since they always have the author explicitly\n> defined in the environment. Unfortunately, we cannot\n> reliably test that prompting still happens in the implicit\n> case, as send-email will produce inconsistent results\n> depending on the machine config (if we cannot find a FQDN,\n> \"git var\" will barf, causing us to exit early;\n\nAt first this sounded like a bug to me --- how could the user keep\nworking without the sysadmin intervening?\n\nBut then I remembered that the user can set her name and email in\n.gitconfig and probably would want to in such a setup anyway.\n\nWhen someone writes such a test, I think it could check that git\neither prompts or writes a message advising to configure the user\nemail, no? Waiting until later for that seems fine to me, though.\n\nThanks,\nJonathan\n"},{"id":"203210","messageId":"20121114191104.GB3860@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114164457.GA6858@elie.Belkin","subject":"Re: [PATCH 1/6] ident: make user_ident_explicitly_given private","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T19:11:05Z","receivedAt":"2012-11-14T19:11:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 08:44:57AM -0800, Jonathan Nieder wrote:\n\n> > -#define IDENT_NAME_GIVEN 01\n> > -#define IDENT_MAIL_GIVEN 02\n> > -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n> > -extern int user_ident_explicitly_given;\n> > extern int user_ident_sufficiently_given(void);\n> \n> In v1.5.6-rc0~56^2 (2008-05-04) \"user_ident_explicitly_given\" was\n> introduced as a global for communication between config, ident, and\n> builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to\n> using the common wrapper user_ident_sufficiently_given(). After\n> v1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,\n> and the variable can finally be made static.\n\nThanks for digging up the history. I remembered trying to do this before\nduring the ident refactoring that went into v1.7.11, but it didn't work\nthen for some reason (I think it was probably an earlier iteration of\nthe series, and then I forgot to revisit it during the final one).\n\nI'll add your explanation to the commit message if I re-roll.\n\n-Peff\n"},{"id":"203212","messageId":"20121114192602.GC3860@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114170148.GB6858@elie.Belkin","subject":"Re: [PATCH 3/6] var: accept multiple variables on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T19:26:02Z","receivedAt":"2012-11-14T19:26:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 09:01:48AM -0800, Jonathan Nieder wrote:\n\n> > DESCRIPTION\n> > -----------\n> > -Prints a git logical variable.\n> > +Prints one or more git logical variables, separated by newlines.\n> > +\n> > +Note that some variables may contain newlines themselves\n> \n> Maybe a -z option to NUL-terminate values would be useful some day.\n\nYeah, I thought about that but stopped short. The intended caller in my\nseries is Git.pm, whose command() splits on newlines. Although it is\nperl...I suspect doing:\n\n local $/ = \"\\0\";\n my @entries = command(...);\n\nwould work. For ident variables, we know they don't contain a newline,\nthough.\n\n> > --- a/builtin/var.c\n> > +++ b/builtin/var.c\n> > @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)\n> > \n> > int cmd_var(int argc, const char **argv, const char *prefix)\n> > {\n> > -\tconst char *val = NULL;\n> > -\tif (argc != 2)\n> > +\tif (argc < 2)\n> > \t\tusage(var_usage);\n> > \n> > \tif (strcmp(argv[1], \"-l\") == 0) {\n> \n> What should happen if I pass \"-l\" followed by other arguments?\n\nGood catch. Probably we should just call usage() once we see \"-l\"\nand (argc > 2), which matches the previous behavior. I don't see much\npoint in listing specific variables after having listed them all.\n\nI was also tempted to convert to parse_options, but I don't think that\nreally buys us anything (we could detect the option in \"git var foo -l\nbar\", but since we are not going to do anything useful in such a case,\nthere is not much point).\n\n> > +\ttest_tick &&\n> > +\techo \"A U Thor 1112911993 -0700\" >expect &&\n> \n> Do we need to hardcode the timestamp? Something like\n> \n> \ttest_cmp_filtered () {\n> \t\texpect=$1 actual=$2 &&\n> \t\tsed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP\" \\\n> \t\t\t<\"$actual\" >\"$actual.filtered\" &&\n> \t\ttest_cmp \"$expect\" \"$actual.filtered\"\n> \t}\n\nNo, we don't have to. I was just hoping to keep the tests simple by not\ndoing any parsing trickery. The test_tick keeps it stable, but as you\nnote, it is not robust to reordering. I think it would be sufficient to\njust put $GIT_COMMITTER_DATE into the expected output.\n\nI'll fix both in a re-roll.\n\nThanks.\n\n-Peff\n"},{"id":"203215","messageId":"20121114195357.GD3860@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114170657.GC6858@elie.Belkin","subject":"Re: [PATCH 4/6] var: provide explicit/implicit ident information","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T19:53:57Z","receivedAt":"2012-11-14T19:53:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 09:06:57AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Internally, we keep track of whether the author or committer\n> > ident information was provided by the user, or whether it\n> > was implicitly determined by the system. However, there is\n> > currently no way for external programs or scripts to get\n> > this information\n> \n> What are the intended semantics? If my machine has /etc/mailname\n> filled out, is that an implicit identity? How about if I set the\n> EMAIL envvar but not GIT_COMMITTER_EMAIL?\n> \n> If external scripts are going to start using this mechanism, they will\n> need answers to these questions to support users that run into\n> configuration problems. A few words on this in the documentation\n> could probably help.\n\nThe intent is to make git's internal rules (whatever they may be)\navailable to calling scripts. But I agree those rules should be spelled\nout now that they are becoming a more visible interface. I'll update the\ndocumentation.\n\n> On most machines I have the EMAIL envvar set explicitly, but in the\n> recent past I relied on /etc/mailname on some others, so I'm also\n> genuinely curious about the use case here (and too lazy to dig it up).\n\nRight now EMAIL is explicit, but `id`@`cat /etc/mailname` is not. I\nthink changing that would be a separate issue, though (and I'm not sure\nwhether it is a good idea; my /etc/mailname is not an email address I\nwould want to use).\n\n-Peff\n"},{"id":"203216","messageId":"20121114195423.GE3860@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114171213.GD6858@elie.Belkin","subject":"Re: [PATCH 5/6] Git.pm: teach \"ident\" to query explicitness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T19:54:24Z","receivedAt":"2012-11-14T19:54:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 09:12:13AM -0800, Jonathan Nieder wrote:\n\n> > --- a/perl/Git.pm\n> > +++ b/perl/Git.pm\n> > @@ -737,7 +737,7 @@ sub remote_refs {\n> > }\n> > \n> > \n> > -=item ident ( TYPE | IDENTSTR )\n> > +=item ident ( TYPE | IDENTSTR [, options] )\n> > \n> > =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )\n> > \n> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.\n> > Alternatively, it can take a prepared ident string (e.g. from the commit\n> > object) and just parse it.\n> > \n> > +If the C option is set to 1, the returned array will contain an\n> > +additional boolean specifying whether the ident was configure explicitly by the\n> > +user.\n> \n> s/configure/configured/\n> \n> I'd suggest adding \"See GIT_COMMITTER_EXPLICIT in git-var(1) for\n> details\" to make the semantics crystal clear. What do you think?\n\nGood suggestion. Thanks.\n\n-Peff\n"},{"id":"203218","messageId":"20121114200505.GA13317@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114171827.GE6858@elie.Belkin","subject":"Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T20:05:05Z","receivedAt":"2012-11-14T20:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 09:18:27AM -0800, Jonathan Nieder wrote:\n\n> > The test scripts need to be adjusted to not expect a prompt\n> > for the sender, since they always have the author explicitly\n> > defined in the environment. Unfortunately, we cannot\n> > reliably test that prompting still happens in the implicit\n> > case, as send-email will produce inconsistent results\n> > depending on the machine config (if we cannot find a FQDN,\n> > \"git var\" will barf, causing us to exit early;\n> \n> At first this sounded like a bug to me --- how could the user keep\n> working without the sysadmin intervening?\n> \n> But then I remembered that the user can set her name and email in\n> .gitconfig and probably would want to in such a setup anyway.\n\nRight. They would already have to to make commits, for example.\n\n> When someone writes such a test, I think it could check that git\n> either prompts or writes a message advising to configure the user\n> email, no? Waiting until later for that seems fine to me, though.\n\nYes. The problem is that the behavior and output are dependent on\nfactors outside the test suite, so we would have to check that one of\nthe possible expected outcomes happens. But I think there are really\nonly two such outcomes (neglecting that the ident itself can have\narbitrary content, but we do not have to check the actual content).\n\n-Peff\n"},{"id":"203221","messageId":"20121114202616.GA23492@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121114200505.GA13317@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-14T20:26:16Z","receivedAt":"2012-11-14T20:26:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 12:05:05PM -0800, Jeff King wrote:\n\n> > When someone writes such a test, I think it could check that git\n> > either prompts or writes a message advising to configure the user\n> > email, no? Waiting until later for that seems fine to me, though.\n> \n> Yes. The problem is that the behavior and output are dependent on\n> factors outside the test suite, so we would have to check that one of\n> the possible expected outcomes happens. But I think there are really\n> only two such outcomes (neglecting that the ident itself can have\n> arbitrary content, but we do not have to check the actual content).\n\nActually, I think the simplest thing is to add a prerequisite, like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 489bc80..8d192ff 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -738,6 +738,14 @@ test_lazy_prereq UTF8_NFD_TO_NFC '\n \tesac\n '\n \n+test_lazy_prereq IMPLICIT_IDENT '\n+\tsane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tgit var GIT_AUTHOR_IDENT &&\n+\t# double check that we were not polluted by config\n+\ttest \"$(git var GIT_AUTHOR_EXPLICIT)\" = 0\n+'\n+\n # When the tests are run as root, permission tests will report that\n # things are writable when they shouldn't be.\n test -w / || test_set_prereq SANITY\n\nWe can't have one test machine that will cover all of the cases, but\ngiven that the test suite is run by many people across many machines, we\nwill get coverage (and I know that some people have machines which would\nnot pass that prereq, because I got test failure reports during the last\nident refactoring).\n\nI'll include something like that in my re-roll (and it should let us\ntest \"git commit\" more thoroughly, too).\n\n-Peff\n"},{"id":"203231","messageId":"20121115000726.GA16910@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s3sBj0iYsCLUpiouUB8PXRwLORDEyD_+dWKrSsMP+TOaw@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:07:26Z","receivedAt":"2012-11-15T00:07:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:\n\n> > Yes, dying would be a regression, in that you would have to configure\n> > your name via the environment and re-run rather than type it at the\n> > prompt. You raise a good point that for people who _could_ take the\n> > implicit default, hitting \"enter\" is working fine now, and we would lose\n> > that. I'd be fine with also just continuing to prompt in the implicit\n> > case.\n> >\n> > But that is a much smaller issue to me than having send-email fail to\n> > respect environment variables and silently use user.*, which is what\n> > started this whole discussion. And I agree it is worth considering as a\n> > regression we should avoid.\n> \n> It might be smaller, I don't think so. A hypothetical user that was\n> relying on GIT_AUTHOR for whatever reason can switch to 'git\n> send-email --from' (which is much easier) when they notice the\n> failure, the same way somebody relying on fqdn would. The difference\n> is that people with fqdn do exist, and they might be relying on this.\n> \n> Both are small issues, that I agree with.\n> \n> But the point is that you seem to be very adamant about _my_\n> regressions, and not pay much attention about yours.\n\nReally? I mentioned initially the possibility of dying instead of\nprompting. You raised the point that it would regress a certain use\ncase. And then what happened? I said above \"you raise a good point[...].\nI'd be fine with also just continuing to prompt[...]. I agree it is\nworth considering as a regression we should avoid\". And then I sent out\na patch series which does not have the regression.\n\nIn other words, my suggestion was a bad one, and once it was pointed\nout, I did not pursue it. If you want to call that \"not paying much\nattention\", feel free. But I'd much rather you point out problems in my\nactual patch series.\n\n> Why do you hold on to my first patch?\n\nBecause clearly I am confused by your behavior. You continued to argue\nthat the initial regression was not worth worrying about. Which I\nthought meant you were still interested in pursuing that patch. But if\nyou are not, then there is not even any point in discussing it. Which\nis just fine with me, as that discussion seemed to be going nowhere.\n\n> The second patch doesn't have this issue. It does change the behavior\n> of 'git commit', yeah, but I think that's a benefit.\n\nChanging \"git commit\" is even something I would entertain. It would be a\nregression for some people, but at least it buys us something (increased\nsafety against people making bogus commits and failing to notice the\nwarning). I'm undecided on whether that is worth it or not.\n\nBut when you presented it, as far as I could tell the change in behavior\nto \"git commit\" was accidental (which is why I pointed it out in\nresponse). And as it was in the middle of a discussion about whether\nregressions matter, it was not clear to me that your argument was \"I\nhave thought this through and the inconvenience is outweighted by the\nbenefit\" and not \"I did not bother to consider the implications for\nusers who are currently using this feature\".\n\nIf you want to seriously propose changing the behavior of \"git commit\",\nI think the best thing would be to make a real patch, laying out the\npros and cons in the commit message, and post it. I would not be\nsurprised if the other list participants have stopped reading our thread\nat this point, and the idea is going otherwise unnoticed.\n\n> > So you can either:\n> >\n> > 1. Reimplement the environment variable lookup that ident.c does,\n> > leaving implicit ident logic out completely.\n> >\n> > 2. Modify ident.c and \"git var\" to let send-email reuse the logic in\n> > ident.c, but avoid dropping the prompt when an implicit ident is\n> > used.\n> >\n> > Doing (2) sounds a lot more maintainable to me in the long run.\n> \n> Or:\n> \n> 3. Change the meaning of the STRICT flag so that the values are\n> explicit, which has benefits outside 'git send-email'. Yes, this would\n> change the behavior in 'git commit' and other tools, but it's worth to\n> investigate these changes, and most likely they would be desirable.\n\nYes, I think that is fine _if_ we want to change git-commit. Which is\nnot clear to me. If we don't, then it is not an option.\n\n> Or:\n> \n> 4. Just stop prompting\n> \n> I already sent a patch for 4. with all the details of why nobody (or\n> very few, if any) would be affected negatively.\n\nIf doing (2) were really hard, that might be worth considering. But it's\nnot. I already did it. So I don't see how this is an attractive option,\nunless my series is so unpalatable that we would rather accept a\nregression.\n\n> > [1/6]: ident: make user_ident_explicitly_given private\n> > [2/6]: ident: keep separate \"explicit\" flags for author and committer\n> > [3/6]: var: accept multiple variables on the command line\n> > [4/6]: var: provide explicit/implicit ident information\n> > [5/6]: Git.pm: teach \"ident\" to query explicitness\n> > [6/6]: send-email: do not prompt for explicit repo ident\n> \n> I think this adds a lot of code that nobody would use.\n\nA lot of code? It is mostly refactoring, which IMHO makes the resulting\ncode cleaner, and it increases the utility of \"git var\", and our test\ncoverage. If you have review comments, then by all means, respond to the\nseries.\n\n> > I do not necessarily agree on \"git commit\". Moreover, I feel like it is\n> > a separate issue. My series above _just_ implements the \"do not prompt\n> > when explicit\" behavior. It does not deal with git-commit at all, nor\n> > does it address the author/committer fallback questions. Those can\n> > easily go on top.\n> \n> Yes, at the cost of adding a lot of code. If we end up agreeing that\n> the changes to 'git commit' are desirable (which I hope at some point\n> we will), then this code would be all for nothing.\n\nIf we are going to change \"git commit\" immediately, then I agree there\nis not much point merging my series. But even if we do change it, will\nwe do so immediately? Will there be a deprecation period? If so, then my\nseries helps send-email in the meantime. And it's already written, so\nyou do not even have to do anything.\n\n> I want clarify that this is merely a disagreement to at which level\n> should we worry about regressions. On one side of the spectrum you\n> have projects like GNOME, who don't have any problem breaking the\n> user-experience from one release to the next, I'm not proposing\n> anything like that. On the other side I think it's you, because I\n> don't recall encountering anybody with such an extreme position of\n> never introducing a regression ever if there's absolutely no evidence\n> that anybody is using certain feature.\n\nI don't think that's a fair characterization of my position. I am fine\nwith introducing a regression if there is a large benefit to it, and\nespecially if the regression is mutually exclusive with the benefit. For\nexample, look at IDENT_STRICT. We used to allow broken email addresses\nin commits, and it was _me_ who pushed forward the change to disallow\nit. That potentially regressed people who would rather have junk in the\ncommit objects than configure their identity (e.g., because they are\ncreating commits on the backend of some automated process). But we\ndiscussed it, and the breakage was worth the increased safety for normal\nusers. We could not have it both ways, since the safety came at the\nexpense of switching the default.\n\nBut with this topic, we had a too-safe default (a safety prompt that was\nsometimes overkill). We can have our cake and eat it, too: drop the\nprompt for the overkill cases, but leave the other cases untouched. And\nthat is what I tried to do in my series. Note that this _still_\nregresses certain use cases. What if I have configured my user.email,\nbut I am expecting send-email to prompt me so I can put in some other\nrandom value. But we can't improve the prompting and leave that case\nthere; they are mutually exclusive. But IMHO, the benefit outweighs the\npossibility of breakage.\n\n-Peff\n"},{"id":"203236","messageId":"20121115003029.GA17550@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121113164845.GD20361@sigill.intra.peff.net","subject":"[PATCHv2 0/8] loosening \"sender\" prompt in send-email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:30:30Z","receivedAt":"2012-11-15T00:30:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 11:48:45AM -0500, Jeff King wrote:\n\n> [1/6]: ident: make user_ident_explicitly_given private\n> [2/6]: ident: keep separate \"explicit\" flags for author and committer\n> [3/6]: var: accept multiple variables on the command line\n> [4/6]: var: provide explicit/implicit ident information\n> [5/6]: Git.pm: teach \"ident\" to query explicitness\n> [6/6]: send-email: do not prompt for explicit repo ident\n\nHere is a re-roll of the series based on comments from Jonathan (changes\nfrom the first version noted below):\n\n [1/8]: test-lib: allow negation of prerequisites\n [2/8]: t7502: factor out autoident prerequisite\n\nThese two are new, and allow us to use the same prereq from t7502 in\nt9001.\n\n [3/8]: ident: make user_ident_explicitly_given static\n\nExpanded the commit message to explain history of the variable.\n\n [4/8]: ident: keep separate \"explicit\" flags for author and committer\n\nSame as before.\n\n [5/8]: var: accept multiple variables on the command line\n\n - Expanded tests to include \"git var -l\"\n - Use $GIT_* variables to create test \"expect\" files, which should make\n them less brittle.\n - Fixed \"git var -l foo\" to print usage instead of ignoring foo (i.e.,\n keeping the behavior the same).\n - Update usage message to match new feature.\n\n [6/8]: var: provide explicit/implicit ident information\n\nUpdated documentation to describe what \"explicit\" means. No functional\nchanges.\n\n [7/8]: Git.pm: teach \"ident\" to query explicitness\n\nImproved Git.pm documentation. No functional changes.\n\n [8/8]: send-email: do not prompt for explicit repo ident\n\nTest the implicit ident case, both when we have implicit ident and when\nwe don't. No changes to send-email.\n\n-Peff\n"},{"id":"203237","messageId":"20121115003325.GA17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 1/8] test-lib: allow negation of prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:33:25Z","receivedAt":"2012-11-15T00:33:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can set and test a prerequisite like this:\n\n test_set_prereq FOO\n test_have_prereq FOO && echo yes\n\nYou can negate the test in the shell like this:\n\n ! test_have_prereq && echo no\n\nHowever, when you are using the automatic prerequisite\nchecking in test_expect_*, there is no opportunity to use\nthe shell negation. This patch introduces the syntax \"!FOO\"\nto indicate that the test should only run if a prerequisite\nis not meant.\n\nOne alternative is to set an explicit negative prerequisite,\nlike:\n\n if system_has_foo; then\n\t test_set_prereq FOO\n else\n\t test_set_prereq NO_FOO\n fi\n\nHowever, this doesn't work for lazy prerequisites, which\nassociate a single test with a single name. We could teach\nthe lazy prereq evaluator to set both forms, but the code\nchange ends up quite similar to this one (because we still\nneed to convert NO_FOO into FOO to find the correct lazy\nscript).\n\nSigned-off-by: Jeff King \n---\nI chose \"!\" as the negation prefix because it will never conflict with\nanother prerequisite. But we could also use \"NO_\" if that's more\naesthetically pleasing. I don't have a strong preference.\n\n t/t0000-basic.sh | 32 ++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 21 ++++++++++++++++++++-\n 2 files changed, 52 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 08677df..562cf41 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -115,6 +115,38 @@ then\n \texit 1\n fi\n \n+test_lazy_prereq LAZY_TRUE true\n+havetrue=no\n+test_expect_success LAZY_TRUE 'test runs if lazy prereq is satisfied' '\n+\thavetrue=yes\n+'\n+donthavetrue=yes\n+test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '\n+\tdonthavetrue=no\n+'\n+\n+if test \"$havetrue$donthavetrue\" != yesyes\n+then\n+\tsay 'bug in test framework: lazy prerequisites do not work'\n+\texit 1\n+fi\n+\n+test_lazy_prereq LAZY_FALSE false\n+nothavefalse=no\n+test_expect_success !LAZY_FALSE 'negative lazy prereqs checked' '\n+\tnothavefalse=yes\n+'\n+havefalse=yes\n+test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '\n+\thavefalse=no\n+'\n+\n+if test \"$nothavefalse$havefalse\" != yesyes\n+then\n+\tsay 'bug in test framework: negative lazy prerequisites do not work'\n+\texit 1\n+fi\n+\n clean=no\n test_expect_success 'tests clean up after themselves' '\n \ttest_when_finished clean=yes\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8889ba5..22a4f8f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -275,6 +275,15 @@ test_have_prereq () {\n \n \tfor prerequisite\n \tdo\n+\t\tcase \"$prerequisite\" in\n+\t\t!*)\n+\t\t\tnegative_prereq=t\n+\t\t\tprerequisite=${prerequisite#!}\n+\t\t\t;;\n+\t\t*)\n+\t\t\tnegative_prereq=\n+\t\tesac\n+\n \t\tcase \" $lazily_tested_prereq \" in\n \t\t*\" $prerequisite \"*)\n \t\t\t;;\n@@ -294,10 +303,20 @@ test_have_prereq () {\n \t\ttotal_prereq=$(($total_prereq + 1))\n \t\tcase \"$satisfied_prereq\" in\n \t\t*\" $prerequisite \"*)\n+\t\t\tsatisfied_this_prereq=t\n+\t\t\t;;\n+\t\t*)\n+\t\t\tsatisfied_this_prereq=\n+\t\tesac\n+\n+\t\tcase \"$satisfied_this_prereq,$negative_prereq\" in\n+\t\tt,|,t)\n \t\t\tok_prereq=$(($ok_prereq + 1))\n \t\t\t;;\n \t\t*)\n-\t\t\t# Keep a list of missing prerequisites\n+\t\t\t# Keep a list of missing prerequisites; restore\n+\t\t\t# the negative marker if necessary.\n+\t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203238","messageId":"20121115003340.GB17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 2/8] t7502: factor out autoident prerequisite","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:33:40Z","receivedAt":"2012-11-15T00:33:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"t7502 checks the behavior of commit when we can and cannot\ndetermine a valid committer ident. Let's move that into\ntest-lib as a lazy prerequisite so other scripts can use it.\n\nSigned-off-by: Jeff King \n---\n t/t7502-commit.sh | 12 +-----------\n t/test-lib.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex deb187e..1a5cb69 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -243,16 +243,6 @@ test_expect_success 'message shows author when it is not equal to committer' '\n \t .git/COMMIT_EDITMSG\n '\n \n-test_expect_success 'setup auto-ident prerequisite' '\n-\tif (sane_unset GIT_COMMITTER_EMAIL &&\n-\t sane_unset GIT_COMMITTER_NAME &&\n-\t git var GIT_COMMITTER_IDENT); then\n-\t\ttest_set_prereq AUTOIDENT\n-\telse\n-\t\ttest_set_prereq NOAUTOIDENT\n-\tfi\n-'\n-\n test_expect_success AUTOIDENT 'message shows committer when it is automatic' '\n \n \techo >>negative &&\n@@ -271,7 +261,7 @@ echo editor started > \"$(pwd)/.git/result\"\n exit 0\n EOF\n \n-test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '\n+test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '\n \t>.git/result\n \t>expect &&\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 489bc80..0334a9e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '\n \tesac\n '\n \n+test_lazy_prereq AUTOIDENT '\n+\tsane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tgit var GIT_AUTHOR_IDENT\n+'\n+\n # When the tests are run as root, permission tests will report that\n # things are writable when they shouldn't be.\n test -w / || test_set_prereq SANITY\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203239","messageId":"20121115003405.GC17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 3/8] ident: make user_ident_explicitly_given static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:34:05Z","receivedAt":"2012-11-15T00:34:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In v1.5.6-rc0~56^2 (2008-05-04) \"user_ident_explicitly_given\"\nwas introduced as a global for communication between config,\nident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07)\nreaders switched to using the common wrapper\nuser_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18\n(2012-05-21), the var is only written in ident.c.\n\nNow we can make it static, which will enable further\nrefactoring without worrying about upsetting other code.\n\nSigned-off-by: Jeff King \nReviewed-by: Jonathan Nieder \n---\n cache.h | 4 ----\n ident.c | 6 +++++-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex dbd8018..50d9eea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,10 +1149,6 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-#define IDENT_NAME_GIVEN 01\n-#define IDENT_MAIL_GIVEN 02\n-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-extern int user_ident_explicitly_given;\n extern int user_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\ndiff --git a/ident.c b/ident.c\nindex a4bf206..733d69d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -10,7 +10,11 @@\n static struct strbuf git_default_name = STRBUF_INIT;\n static struct strbuf git_default_email = STRBUF_INIT;\n static char git_default_date[50];\n-int user_ident_explicitly_given;\n+\n+#define IDENT_NAME_GIVEN 01\n+#define IDENT_MAIL_GIVEN 02\n+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n+static int user_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203240","messageId":"20121115003413.GD17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 4/8] ident: keep separate \"explicit\" flags for author and committer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:34:13Z","receivedAt":"2012-11-15T00:34:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep track of whether the user ident was given to us\nexplicitly, or if we guessed at it from system parameters\nlike username and hostname. However, we kept only a single\nvariable. This covers the common cases (because the author\nand committer will usually come from the same explicit\nsource), but can miss two cases:\n\n 1. GIT_COMMITTER_* is set explicitly, but we fallback for\n GIT_AUTHOR. We claim the ident is explicit, even though\n the author is not.\n\n 2. GIT_AUTHOR_* is set and we ask for author ident, but\n not committer ident. We will claim the ident is\n implicit, even though it is explicit.\n\nThis patch uses two variables instead of one, updates both\nwhen we set the \"fallback\" values, and updates them\nindividually when we read from the environment.\n\nRather than keep user_ident_sufficiently_given as a\ncompatibility wrapper, we update the only two callers to\ncheck the committer_ident, which matches their intent and\nwhat was happening already.\n\nSigned-off-by: Jeff King \n---\n builtin/commit.c | 4 ++--\n cache.h | 3 ++-\n ident.c | 32 +++++++++++++++++++++++++-------\n 3 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dd2ec5..d6dd3df 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n \t\t\t\tauthor_ident->buf);\n \n-\t\tif (!user_ident_sufficiently_given())\n+\t\tif (!committer_ident_sufficiently_given())\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Committer: %s\"),\n@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \t\tstrbuf_addstr(&format, \"\\n Author: \");\n \t\tstrbuf_addbuf_percentquote(&format, &author_ident);\n \t}\n-\tif (!user_ident_sufficiently_given()) {\n+\tif (!committer_ident_sufficiently_given()) {\n \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n \t\tstrbuf_addbuf_percentquote(&format, &committer_ident);\n \t\tif (advice_implicit_identity) {\ndiff --git a/cache.h b/cache.h\nindex 50d9eea..18fdd18 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1149,7 +1149,8 @@ struct config_include_data {\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n-extern int user_ident_sufficiently_given(void);\n+extern int committer_ident_sufficiently_given(void);\n+extern int author_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\ndiff --git a/ident.c b/ident.c\nindex 733d69d..ac9672f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -14,7 +14,8 @@ static char git_default_date[50];\n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n-static int user_ident_explicitly_given;\n+static int committer_ident_explicitly_given;\n+static int author_ident_explicitly_given;\n \n #ifdef NO_GECOS_IN_PWENT\n #define get_gecos(ignored) \"&\"\n@@ -113,7 +114,8 @@ const char *ident_default_email(void)\n \n \t\tif (email && email[0]) {\n \t\t\tstrbuf_addstr(&git_default_email, email);\n-\t\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t} else\n \t\t\tcopy_email(xgetpwuid_self(), &git_default_email);\n \t\tstrbuf_trim(&git_default_email);\n@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)\n \n const char *git_author_info(int flag)\n {\n+\tif (getenv(\"GIT_AUTHOR_NAME\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n \t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n \t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n@@ -340,16 +346,16 @@ const char *git_author_info(int flag)\n const char *git_committer_info(int flag)\n {\n \tif (getenv(\"GIT_COMMITTER_NAME\"))\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n \n-int user_ident_sufficiently_given(void)\n+static int ident_is_sufficient(int user_ident_explicitly_given)\n {\n #ifndef WINDOWS\n \treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)\n #endif\n }\n \n+int committer_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(committer_ident_explicitly_given);\n+}\n+\n+int author_ident_sufficiently_given(void)\n+{\n+\treturn ident_is_sufficient(author_ident_explicitly_given);\n+}\n+\n int git_ident_config(const char *var, const char *value, void *data)\n {\n \tif (!strcmp(var, \"user.name\")) {\n@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_name);\n \t\tstrbuf_addstr(&git_default_name, value);\n-\t\tuser_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \t\treturn 0;\n \t}\n \n@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)\n \t\t\treturn config_error_nonbool(var);\n \t\tstrbuf_reset(&git_default_email);\n \t\tstrbuf_addstr(&git_default_email, value);\n-\t\tuser_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\treturn 0;\n \t}\n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"203241","messageId":"20121115003504.GE17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 5/8] var: accept multiple variables on the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:35:04Z","receivedAt":"2012-11-15T00:35:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git-var currently only accepts a single value to print. This\nis inefficient if the caller is interested in finding\nmultiple values, as they must invoke git-var multiple times.\n\nThis patch lets callers specify multiple variables, and\nprints one per line.\n\nWhile we're in the area, let's add some basic tests for \"git\nvar\", which until now was largely untested (and of course a\ntest for multiple values on top).\n\nSigned-off-by: Jeff King \n---\n Documentation/git-var.txt | 9 +++++++--\n builtin/var.c | 17 +++++++++-------\n t/t0007-git-var.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 67 insertions(+), 9 deletions(-)\n create mode 100755 t/t0007-git-var.sh\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 67edf58..53abba5 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -9,11 +9,16 @@ git-var - Show a git logical variable\n SYNOPSIS\n --------\n [verse]\n-'git var' ( -l | )\n+'git var' ( -l | ... )\n \n DESCRIPTION\n -----------\n-Prints a git logical variable.\n+Prints one or more git logical variables, separated by newlines.\n+\n+Note that some variables may contain newlines themselves (e.g.,\n+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output\n+when requesting multiple variables. This can be mitigated by putting any\n+such variables at the end of the list.\n \n OPTIONS\n -------\ndiff --git a/builtin/var.c b/builtin/var.c\nindex aedbb53..49ab300 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -5,7 +5,7 @@\n */\n #include \"builtin.h\"\n \n-static const char var_usage[] = \"git var (-l | )\";\n+static const char var_usage[] = \"git var (-l | ...)\";\n \n static const char *editor(int flag)\n {\n@@ -73,21 +73,24 @@ static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *val = NULL;\n-\tif (argc != 2)\n+\tif (argc < 2)\n \t\tusage(var_usage);\n \n \tif (strcmp(argv[1], \"-l\") == 0) {\n+\t\tif (argc > 2)\n+\t\t\tusage(var_usage);\n \t\tgit_config(show_config, NULL);\n \t\tlist_vars();\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n-\tval = read_var(argv[1]);\n-\tif (!val)\n-\t\tusage(var_usage);\n \n-\tprintf(\"%s\\n\", val);\n+\twhile (*++argv) {\n+\t\tconst char *val = read_var(*argv);\n+\t\tif (!val)\n+\t\t\tusage(var_usage);\n+\t\tprintf(\"%s\\n\", val);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nnew file mode 100755\nindex 0000000..ec5d946\n--- /dev/null\n+++ b/t/t0007-git-var.sh\n@@ -0,0 +1,50 @@\n+#!/bin/sh\n+\n+test_description='basic sanity checks for git var'\n+. ./test-lib.sh\n+\n+test_expect_success 'get GIT_AUTHOR_IDENT' '\n+\ttest_tick &&\n+\techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n+\tgit var GIT_AUTHOR_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get GIT_COMMITTER_IDENT' '\n+\ttest_tick &&\n+\techo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\" >expect &&\n+\tgit var GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# For git var -l, we check only a representative variable;\n+# testing the whole output would make our test too brittle with\n+# respect to unrelated changes in the test suite's environment.\n+test_expect_success 'git var -l lists variables' '\n+\tgit var -l >actual &&\n+\techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n+\tsed -n s/GIT_AUTHOR_IDENT=//p actual.author &&\n+\ttest_cmp expect actual.author\n+'\n+\n+test_expect_success 'git var -l lists config' '\n+\tgit var -l >actual &&\n+\techo false >expect &&\n+\tsed -n s/core\\\\.bare=//p actual.bare &&\n+\ttest_cmp expect actual.bare\n+'\n+\n+test_expect_success 'listing and asking for variables are exclusive' '\n+\ttest_must_fail git var -l GIT_COMMITTER_IDENT\n+'\n+\n+test_expect_success 'git var can show multiple values' '\n+\tcat >expect <<-EOF &&\n+\t$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\n+\t$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tEOF\n+\tgit var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203242","messageId":"20121115003550.GF17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 6/8] var: provide explicit/implicit ident information","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:35:50Z","receivedAt":"2012-11-15T00:35:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Internally, we keep track of whether the author or committer\nident information was provided by the user, or whether it\nwas implicitly determined by the system. However, there is\ncurrently no way for external programs or scripts to get\nthis information without re-implementing the ident logic\nthemselves. Let's provide a way for them to do so.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-var.txt | 12 ++++++++++++\n builtin/var.c | 27 +++++++++++++++++++++++++++\n t/t0007-git-var.sh | 36 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 75 insertions(+)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 53abba5..bb2997d 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -39,9 +39,21 @@ VARIABLES\n GIT_AUTHOR_IDENT::\n The author of a piece of code.\n \n+GIT_AUTHOR_EXPLICIT::\n+ Outputs \"1\" if the author identity was provided explicitly by the\n+ user, or \"0\" if it was determined implicitly from the system.\n+ Identity is considered explicit if it comes from one of git's config\n+ file (i.e., via the `user.*` variables), from the `$GIT_AUTHOR_*`\n+ environment variables, or from the `$EMAIL` environment variable. It\n+ is considered implicit if it was generated from system variables\n+ like the username and hostname.\n+\n GIT_COMMITTER_IDENT::\n The person who put a piece of code into git.\n \n+GIT_COMMITTER_EXPLICIT::\n+ Like GIT_AUTHOR_EXPLICIT, but for the committer ident.\n+\n GIT_EDITOR::\n Text editor for use by git commands. The value is meant to be\n interpreted by the shell when it is used. Examples: `~/bin/vi`,\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 49ab300..9503df9 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -26,13 +26,40 @@ static const char *pager(int flag)\n \treturn pgm;\n }\n \n+static const char *explicit_ident(const char * (*get_ident)(int),\n+\t\t\t\t int (*check_ident)(void))\n+{\n+\t/*\n+\t * Prime the \"explicit\" flag by getting the identity.\n+\t * We do not want to be strict here, because we would not want\n+\t * to die on bogus implicit values, but instead just report that they\n+\t * are not explicit.\n+\t */\n+\tget_ident(0);\n+\treturn check_ident() ? \"1\" : \"0\";\n+}\n+\n+static const char *committer_explicit(int flag)\n+{\n+\treturn explicit_ident(git_committer_info,\n+\t\t\t committer_ident_sufficiently_given);\n+}\n+\n+static const char *author_explicit(int flag)\n+{\n+\treturn explicit_ident(git_author_info,\n+\t\t\t author_ident_sufficiently_given);\n+}\n+\n struct git_var {\n \tconst char *name;\n \tconst char *(*read)(int);\n };\n static struct git_var git_vars[] = {\n \t{ \"GIT_COMMITTER_IDENT\", git_committer_info },\n+\t{ \"GIT_COMMITTER_EXPLICIT\", committer_explicit },\n \t{ \"GIT_AUTHOR_IDENT\", git_author_info },\n+\t{ \"GIT_AUTHOR_EXPLICIT\", author_explicit },\n \t{ \"GIT_EDITOR\", editor },\n \t{ \"GIT_PAGER\", pager },\n \t{ \"\", NULL },\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex ec5d946..47907de 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -47,4 +47,40 @@ test_expect_success 'git var can show multiple values' '\n \ttest_cmp expect actual\n '\n \n+for type in AUTHOR COMMITTER; do\n+\ttest_expect_success \"$type ident can detect implicit values\" \"\n+\t\techo 0 >expect &&\n+\t\t(\n+\t\t\tsane_unset GIT_${type}_NAME &&\n+\t\t\tsane_unset GIT_${type}_EMAIL &&\n+\t\t\tsane_unset EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t) &&\n+\t\ttest_cmp expect actual\n+\t\"\n+\n+\ttest_expect_success \"$type ident is explicit via environment\" \"\n+\t\techo 1 >expect &&\n+\t\t(\n+\t\t\tGIT_${type}_NAME='A Name' &&\n+\t\t\texport GIT_${type}_NAME &&\n+\t\t\tGIT_${type}_EMAIL='name@example.com' &&\n+\t\t\texport GIT_${type}_EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t) &&\n+\t\ttest_cmp expect actual\n+\t\"\n+\n+\ttest_expect_success \"$type ident is explicit via config\" \"\n+\t\techo 1 >expect &&\n+\t\ttest_config user.name 'A Name' &&\n+\t\ttest_config user.email 'name@example.com' &&\n+\t\t(\n+\t\t\tsane_unset GIT_${type}_NAME &&\n+\t\t\tsane_unset GIT_${type}_EMAIL &&\n+\t\t\tgit var GIT_${type}_EXPLICIT >actual\n+\t\t)\n+\t\"\n+done\n+\n test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203243","messageId":"20121115003628.GG17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 7/8] Git.pm: teach \"ident\" to query explicitness","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:36:28Z","receivedAt":"2012-11-15T00:36:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"\"git var\" recently learned to report on whether an ident we\nfetch from it was configured explicitly or implicitly. Let's\nmake that information available to callers of the ident\nfunction.\n\nBecause evaluating \"ident\" in an array versus scalar context\nalready has a meaning, we cannot return our extra value in a\nbackwards compatible way. Instead, we require the caller to\nadd an extra \"explicit\" flag to request the information.\nThe ident_person function, on the other hand, always returns\na scalar, so we are free to overload it in an array context.\n\nSigned-off-by: Jeff King \n---\n perl/Git.pm | 32 ++++++++++++++++++++++++--------\n 1 file changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 497f420..28075a9 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -737,7 +737,7 @@ sub remote_refs {\n }\n \n \n-=item ident ( TYPE | IDENTSTR )\n+=item ident ( TYPE | IDENTSTR [, options] )\n \n =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )\n \n@@ -750,8 +750,16 @@ and either returns it as a scalar string or as an array with the fields parsed.\n Alternatively, it can take a prepared ident string (e.g. from the commit\n object) and just parse it.\n \n+If the C option is set to 1, we are in an array context, and we are\n+using C to lookup the ident, the returned array will contain an\n+additional boolean element specifying whether the ident was configured\n+explicitly by the user. See the description of GIT_*_EXPLICIT in git-var(1) for\n+details.\n+\n C returns the person part of the ident - name and email;\n it can take the same arguments as C or the array returned by C.\n+In an array context, C returns both the person string and the\n+C boolean.\n \n The synopsis is like:\n \n@@ -763,17 +771,22 @@ The synopsis is like:\n =cut\n \n sub ident {\n-\tmy ($self, $type) = _maybe_self(@_);\n-\tmy $identstr;\n+\tmy ($self, $type, %options) = _maybe_self(@_);\n+\tmy ($identstr, $explicit);\n \tif (lc $type eq lc 'committer' or lc $type eq lc 'author') {\n-\t\tmy @cmd = ('var', 'GIT_'.uc($type).'_IDENT');\n+\t\tmy $uc = uc($type);\n+\t\tmy @cmd = ('var', \"GIT_${uc}_IDENT\", \"GIT_${uc}_EXPLICIT\");\n \t\tunshift @cmd, $self if $self;\n-\t\t$identstr = command_oneline(@cmd);\n+\t\t($identstr, $explicit) = command(@cmd);\n \t} else {\n \t\t$identstr = $type;\n \t}\n \tif (wantarray) {\n-\t\treturn $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n+\t\tmy @ret = $identstr =~ /^(.*) <(.*)> (\\d+ [+-]\\d{4})$/;\n+\t\tif ($options{explicit} && defined $explicit) {\n+\t\t\tpush @ret, $explicit;\n+\t\t}\n+\t\treturn @ret;\n \t} else {\n \t\treturn $identstr;\n \t}\n@@ -781,8 +794,11 @@ sub ident {\n \n sub ident_person {\n \tmy ($self, @ident) = _maybe_self(@_);\n-\t$#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);\n-\treturn \"$ident[0] <$ident[1]>\";\n+\t$#ident == 0 and @ident = $self ?\n+\t\t\t\t $self->ident($ident[0], explicit => 1) :\n+\t\t\t\t ident($ident[0], explicit => 1);\n+\tmy $ret = \"$ident[0] <$ident[1]>\";\n+\treturn wantarray ? ($ret, @ident[3]) : $ret;\n }\n \n \n-- \n1.8.0.207.gdf2154c\n"},{"id":"203244","messageId":"20121115003640.GH17819@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115003029.GA17550@sigill.intra.peff.net","subject":"[PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T00:36:40Z","receivedAt":"2012-11-15T00:36:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If git-send-email is configured with sendemail.from, we will\nnot prompt the user for the \"From\" address of the emails.\nIf it is not configured, we prompt the user, but provide the\nrepo author or committer as a default. Even though we\nprobably have a sensible value for the default, the prompt\nis a safety check in case git generated an incorrect\nimplicit ident string.\n\nNow that Git.pm will tell us whether the ident is implicit or\nexplicit, we can stop prompting in the explicit case, saving\nmost users from having to see the prompt at all.\n\nSigned-off-by: Jeff King \n---\n git-send-email.perl | 22 +++++++++++++---------\n t/t9001-send-email.sh | 36 ++++++++++++++++++++++++++++++++++--\n 2 files changed, 47 insertions(+), 11 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5a7c29d..0c49b32 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -436,9 +436,8 @@ if (0) {\n \t}\n }\n \n-my ($repoauthor, $repocommitter);\n-($repoauthor) = Git::ident_person(@repo, 'author');\n-($repocommitter) = Git::ident_person(@repo, 'committer');\n+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');\n+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');\n \n # Verify the user input\n \n@@ -755,12 +754,17 @@ if (!$force) {\n \n my $prompting = 0;\n if (!defined $sender) {\n-\t$sender = $repoauthor || $repocommitter || '';\n-\t$sender = ask(\"Who should the emails appear to be from? [$sender] \",\n-\t default => $sender,\n-\t\t valid_re => qr/\\@.*\\./, confirm_only => 1);\n-\tprint \"Emails will be sent from: \", $sender, \"\\n\";\n-\t$prompting++;\n+\t($sender, my $explicit) =\n+\t\tdefined $repoauthor ? ($repoauthor, $author_explicit) :\n+\t\tdefined $repocommitter ? ($repocommitter, $committer_explicit) :\n+\t\t('', 0);\n+\tif (!$explicit) {\n+\t\t$sender = ask(\"Who should the emails appear to be from? [$sender] \",\n+\t\t\t default => $sender,\n+\t\t\t valid_re => qr/\\@.*\\./, confirm_only => 1);\n+\t\tprint \"Emails will be sent from: \", $sender, \"\\n\";\n+\t\t$prompting++;\n+\t}\n }\n \n if (!@initial_to && !defined $to_cmd) {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 6c6af7d..0fe0b8e 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '\n \n test_expect_success $PREREQ 'Prompting works' '\n \tclean_fake_sendmail &&\n-\t(echo \"Example \"\n-\t echo \"to@example.com\"\n+\t(echo \"to@example.com\"\n \t echo \"\"\n \t) | GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$patches \\\n \t\t2>errors &&\n+\t\tgrep \"^From: A U Thor \\$\" msgtxt1 &&\n+\t\tgrep \"^To: to@example.com\\$\" msgtxt1\n+'\n+\n+test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '\n+\tclean_fake_sendmail &&\n+\t(echo \"Example \" &&\n+\t echo \"to@example.com\" &&\n+\t echo \"\"\n+\t) |\n+\t(sane_unset GIT_AUTHOR_NAME &&\n+\t sane_unset GIT_AUTHOR_EMAIL &&\n+\t sane_unset GIT_COMMITTER_NAME &&\n+\t sane_unset GIT_COMMITTER_EMAIL &&\n+\t GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t$patches \\\n+\t\t2>errors &&\n \t\tgrep \"^From: Example \\$\" msgtxt1 &&\n \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n+\t)\n+'\n+\n+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n+\tclean_fake_sendmail &&\n+\t(sane_unset GIT_AUTHOR_NAME &&\n+\t sane_unset GIT_AUTHOR_EMAIL &&\n+\t sane_unset GIT_COMMITTER_NAME &&\n+\t sane_unset GIT_COMMITTER_EMAIL &&\n+\t GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n+\t test_must_fail git send-email \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t$patches errors.out &&\n+\t\ttest_i18ngrep \"tell me who you are\" errors.out\n+\t)\n '\n \n test_expect_success $PREREQ 'tocmd works' '\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203255","messageId":"CAMP44s3vYP6zR6sKxgVSoZUySxcAuR7MioMeT1WDAnU0PtZKGA@mail.gmail.com","threadId":"32079","inReplyTo":"20121115000726.GA16910@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-15T01:41:50Z","receivedAt":"2012-11-15T01:41:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 15, 2012 at 1:07 AM, Jeff King wrote:\n> On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:\n>\n>> > Yes, dying would be a regression, in that you would have to configure\n>> > your name via the environment and re-run rather than type it at the\n>> > prompt. You raise a good point that for people who _could_ take the\n>> > implicit default, hitting \"enter\" is working fine now, and we would lose\n>> > that. I'd be fine with also just continuing to prompt in the implicit\n>> > case.\n>> >\n>> > But that is a much smaller issue to me than having send-email fail to\n>> > respect environment variables and silently use user.*, which is what\n>> > started this whole discussion. And I agree it is worth considering as a\n>> > regression we should avoid.\n>>\n>> It might be smaller, I don't think so. A hypothetical user that was\n>> relying on GIT_AUTHOR for whatever reason can switch to 'git\n>> send-email --from' (which is much easier) when they notice the\n>> failure, the same way somebody relying on fqdn would. The difference\n>> is that people with fqdn do exist, and they might be relying on this.\n>>\n>> Both are small issues, that I agree with.\n>>\n>> But the point is that you seem to be very adamant about _my_\n>> regressions, and not pay much attention about yours.\n>\n> Really? I mentioned initially the possibility of dying instead of\n> prompting. You raised the point that it would regress a certain use\n> case. And then what happened? I said above \"you raise a good point[...].\n> I'd be fine with also just continuing to prompt[...]. I agree it is\n> worth considering as a regression we should avoid\". And then I sent out\n> a patch series which does not have the regression.\n>\n> In other words, my suggestion was a bad one, and once it was pointed\n> out, I did not pursue it. If you want to call that \"not paying much\n> attention\", feel free. But I'd much rather you point out problems in my\n> actual patch series.\n\nBut that I meant that when I introduce a regression it's like I'm\nkilling all that is good and sacred about git, and when you do it's\neverything but that.\n\nYes, you sent a new patch. So did I.\n\n>> The second patch doesn't have this issue. It does change the behavior\n>> of 'git commit', yeah, but I think that's a benefit.\n>\n> Changing \"git commit\" is even something I would entertain. It would be a\n> regression for some people, but at least it buys us something (increased\n> safety against people making bogus commits and failing to notice the\n> warning). I'm undecided on whether that is worth it or not.\n>\n> But when you presented it, as far as I could tell the change in behavior\n> to \"git commit\" was accidental (which is why I pointed it out in\n> response).\n\nHow could it be accidental if I said this: \"Not only will this fix\n'git send-email', but it will also fix 'git\ncommit'\".\n\n> And as it was in the middle of a discussion about whether\n> regressions matter,\n\nThat was not the discussion at all.\n\nYou can't say that all regressions are the same, and if I say\n\"regression X doesn't matter\", that means ALL regressions don't\nmatter. That's a hasty generalization.\n\n> If you want to seriously propose changing the behavior of \"git commit\",\n> I think the best thing would be to make a real patch, laying out the\n> pros and cons in the commit message, and post it. I would not be\n> surprised if the other list participants have stopped reading our thread\n> at this point, and the idea is going otherwise unnoticed.\n\nI would, if I saw any chance in it actually going through.\n\n>> Or:\n>>\n>> 4. Just stop prompting\n>>\n>> I already sent a patch for 4. with all the details of why nobody (or\n>> very few, if any) would be affected negatively.\n>\n> If doing (2) were really hard, that might be worth considering. But it's\n> not. I already did it. So I don't see how this is an attractive option,\n> unless my series is so unpalatable that we would rather accept a\n> regression.\n\nA matter of opinion. I think that series introduces way too much code\nfor a very very small gain that eventually would probably disappear.\n\n>> > [1/6]: ident: make user_ident_explicitly_given private\n>> > [2/6]: ident: keep separate \"explicit\" flags for author and committer\n>> > [3/6]: var: accept multiple variables on the command line\n>> > [4/6]: var: provide explicit/implicit ident information\n>> > [5/6]: Git.pm: teach \"ident\" to query explicitness\n>> > [6/6]: send-email: do not prompt for explicit repo ident\n>>\n>> I think this adds a lot of code that nobody would use.\n>\n> A lot of code? It is mostly refactoring,\n\nPatch #1 and #3 are refactoring, the rest are not.\n\n> which IMHO makes the resulting\n> code cleaner, and it increases the utility of \"git var\", and our test\n> coverage. If you have review comments, then by all means, respond to the\n> series.\n\nI don't have any comments, except that I don't think all that code is\nneeded. And why would I bother commenting there, if my opinion will be\nignored?\n\n>> > I do not necessarily agree on \"git commit\". Moreover, I feel like it is\n>> > a separate issue. My series above _just_ implements the \"do not prompt\n>> > when explicit\" behavior. It does not deal with git-commit at all, nor\n>> > does it address the author/committer fallback questions. Those can\n>> > easily go on top.\n>>\n>> Yes, at the cost of adding a lot of code. If we end up agreeing that\n>> the changes to 'git commit' are desirable (which I hope at some point\n>> we will), then this code would be all for nothing.\n>\n> If we are going to change \"git commit\" immediately, then I agree there\n> is not much point merging my series. But even if we do change it, will\n> we do so immediately? Will there be a deprecation period? If so, then my\n> series helps send-email in the meantime. And it's already written, so\n> you do not even have to do anything.\n\nYes, but it still adds a lot of code.\n\n>> I want clarify that this is merely a disagreement to at which level\n>> should we worry about regressions. On one side of the spectrum you\n>> have projects like GNOME, who don't have any problem breaking the\n>> user-experience from one release to the next, I'm not proposing\n>> anything like that. On the other side I think it's you, because I\n>> don't recall encountering anybody with such an extreme position of\n>> never introducing a regression ever if there's absolutely no evidence\n>> that anybody is using certain feature.\n>\n> I don't think that's a fair characterization of my position. I am fine\n> with introducing a regression if there is a large benefit to it, and\n> especially if the regression is mutually exclusive with the benefit. For\n> example, look at IDENT_STRICT. We used to allow broken email addresses\n> in commits, and it was _me_ who pushed forward the change to disallow\n> it. That potentially regressed people who would rather have junk in the\n> commit objects than configure their identity (e.g., because they are\n> creating commits on the backend of some automated process). But we\n> discussed it, and the breakage was worth the increased safety for normal\n> users. We could not have it both ways, since the safety came at the\n> expense of switching the default.\n>\n> But with this topic, we had a too-safe default (a safety prompt that was\n> sometimes overkill). We can have our cake and eat it, too: drop the\n> prompt for the overkill cases, but leave the other cases untouched. And\n> that is what I tried to do in my series. Note that this _still_\n> regresses certain use cases. What if I have configured my user.email,\n> but I am expecting send-email to prompt me so I can put in some other\n> random value. But we can't improve the prompting and leave that case\n> there; they are mutually exclusive. But IMHO, the benefit outweighs the\n> possibility of breakage.\n\nThat's true, so we will be introducing a regression regardless.\n\nNow, if we go with this patch:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/209660\n\nInstead of your patch series, who will get hurt? Hint: I already\nanswered that question.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203258","messageId":"20121115015052.GA19131@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s3vYP6zR6sKxgVSoZUySxcAuR7MioMeT1WDAnU0PtZKGA@mail.gmail.com","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T01:50:52Z","receivedAt":"2012-11-15T01:50:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:\n\n> But that I meant that when I introduce a regression it's like I'm\n> killing all that is good and sacred about git, and when you do it's\n> everything but that.\n\nThe rhetoric in this statement is a good indication that there is\nnothing productive to come from our discussing it anymore.\n\n> > If you want to seriously propose changing the behavior of \"git commit\",\n> > I think the best thing would be to make a real patch, laying out the\n> > pros and cons in the commit message, and post it. I would not be\n> > surprised if the other list participants have stopped reading our thread\n> > at this point, and the idea is going otherwise unnoticed.\n> \n> I would, if I saw any chance in it actually going through.\n\nWell, it certainly will not go through if you do not try.\n\n-Peff\n"},{"id":"203261","messageId":"CAMP44s0d+g7bXCnOf55jZNNFS6uJ+4BDowx5uYxWBP4xA+-0zA@mail.gmail.com","threadId":"32079","inReplyTo":"20121115003640.GH17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-15T02:08:42Z","receivedAt":"2012-11-15T02:08:42Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 15, 2012 at 1:36 AM, Jeff King wrote:\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 5a7c29d..0c49b32 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -436,9 +436,8 @@ if (0) {\n> }\n> }\n>\n> -my ($repoauthor, $repocommitter);\n> -($repoauthor) = Git::ident_person(@repo, 'author');\n> -($repocommitter) = Git::ident_person(@repo, 'committer');\n> +my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');\n> +my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');\n>\n> # Verify the user input\n>\n> @@ -755,12 +754,17 @@ if (!$force) {\n>\n> my $prompting = 0;\n> if (!defined $sender) {\n> - $sender = $repoauthor || $repocommitter || '';\n> - $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n> - default => $sender,\n> - valid_re => qr/\\@.*\\./, confirm_only => 1);\n> - print \"Emails will be sent from: \", $sender, \"\\n\";\n> - $prompting++;\n> + ($sender, my $explicit) =\n> + defined $repoauthor ? ($repoauthor, $author_explicit) :\n> + defined $repocommitter ? ($repocommitter, $committer_explicit) :\n> + ('', 0);\n> + if (!$explicit) {\n> + $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n> + default => $sender,\n> + valid_re => qr/\\@.*\\./, confirm_only => 1);\n> + print \"Emails will be sent from: \", $sender, \"\\n\";\n> + $prompting++;\n> + }\n> }\n>\n> if (!@initial_to && !defined $to_cmd) {\n\nI don't think there's any need for all that, this does the trick:\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex aea66a0..503e551 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -748,16 +748,11 @@ if (!$force) {\n }\n }\n\n-my $prompting = 0;\n if (!defined $sender) {\n $sender = $repoauthor || $repocommitter || '';\n- $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n- default => $sender,\n- valid_re => qr/\\@.*\\./, confirm_only => 1);\n- print \"Emails will be sent from: \", $sender, \"\\n\";\n- $prompting++;\n }\n\n+my $prompting = 0;\n\nThis passes all the current tests and the ones you added.\n\nWhich kind of user will get the prompt with your patch, that would\nmiss it with mine?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203262","messageId":"CAMP44s0Mrdj6rr3BGuwNW1OHjks_LvymUGK5thtBqX45SRSU+Q@mail.gmail.com","threadId":"32079","inReplyTo":"20121115015052.GA19131@sigill.intra.peff.net","subject":"Re: [PATCH] send-email: add proper default sender","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-15T02:14:30Z","receivedAt":"2012-11-15T02:14:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 15, 2012 at 2:50 AM, Jeff King wrote:\n> On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:\n>\n>> But that I meant that when I introduce a regression it's like I'm\n>> killing all that is good and sacred about git, and when you do it's\n>> everything but that.\n>\n> The rhetoric in this statement is a good indication that there is\n> nothing productive to come from our discussing it anymore.\n\nThe point is still true.\n\n>> > If you want to seriously propose changing the behavior of \"git commit\",\n>> > I think the best thing would be to make a real patch, laying out the\n>> > pros and cons in the commit message, and post it. I would not be\n>> > surprised if the other list participants have stopped reading our thread\n>> > at this point, and the idea is going otherwise unnoticed.\n>>\n>> I would, if I saw any chance in it actually going through.\n>\n> Well, it certainly will not go through if you do not try.\n\nAt least I wouldn't be wasting my time.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203269","messageId":"20121115074658.GA8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003325.GA17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T07:46:58Z","receivedAt":"2012-11-15T07:46:58Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '\n\nI have a visceral nervousness when reading this code, from too much\nunpleasant experience of bash's csh-style !history expansion. Luckily\nbash does not treat ! specially in the '-o sh' mode used by tests.\n\nDoes this feature work when running a test explicitly using\n\"bash \"? That's something I do from time to time to\nfigure out whether a weird behavior is shell-specific.\n\nIf it works everywhere, this patch would help me conquer my fear of\nexclamation points in git's tests, which would be a comfort to me and\na very good thing.\n"},{"id":"203271","messageId":"20121115074955.GB8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003340.GB17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 2/8] t7502: factor out autoident prerequisite","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T07:49:55Z","receivedAt":"2012-11-15T07:49:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '\n> \tesac\n> '\n> \n> +test_lazy_prereq AUTOIDENT '\n> +\tsane_unset GIT_AUTHOR_NAME &&\n> +\tsane_unset GIT_AUTHOR_EMAIL &&\n> +\tgit var GIT_AUTHOR_IDENT\n> +'\n\nLazy prereq scripts run in a subshell, so this should be safe. Ack.\n\nThanks,\nJonathan\n"},{"id":"203272","messageId":"20121115075125.GC8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003405.GC17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 3/8] ident: make user_ident_explicitly_given static","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T07:51:25Z","receivedAt":"2012-11-15T07:51:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> In v1.5.6-rc0~56^2 (2008-05-04) \"user_ident_explicitly_given\"\n> was introduced as a global for communication between config,\n> ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07)\n> readers switched to using the common wrapper\n> user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18\n> (2012-05-21), the var is only written in ident.c.\n>\n> Now we can make it static, which will enable further\n> refactoring without worrying about upsetting other code.\n>\n> Signed-off-by: Jeff King \n\nFor what it's worth I liked the old commit message more. But I don't\nmind either.\n\nHope that helps,\nJonathan\n"},{"id":"203273","messageId":"20121115080457.GD8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003413.GD17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 4/8] ident: keep separate \"explicit\" flags for author and committer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T08:04:57Z","receivedAt":"2012-11-15T08:04:57Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> 1. GIT_COMMITTER_* is set explicitly, but we fallback for\n> GIT_AUTHOR. We claim the ident is explicit, even though\n> the author is not.\n>\n> 2. GIT_AUTHOR_* is set and we ask for author ident, but\n> not committer ident. We will claim the ident is\n> implicit, even though it is explicit.\n>\n> This patch uses two variables instead of one, updates both\n> when we set the \"fallback\" values, and updates them\n> individually when we read from the environment.\n\nNice problem description. The fixed behavior makes sense to me, for\nwhat it's worth.\n\nNot about this patch, but: in case (1), shouldn't the author fall\nback to $GIT_COMMITER_NAME?\n"},{"id":"203274","messageId":"20121115081012.GE8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003504.GE17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 5/8] var: accept multiple variables on the command line","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T08:10:12Z","receivedAt":"2012-11-15T08:10:12Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This patch lets callers specify multiple variables, and\n> prints one per line.\n[...]\n> Signed-off-by: Jeff King \n\nVery pleasantly done --- thanks. For what it's worth, assuming this\nis tested, I can't see any reason not to apply it.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"203275","messageId":"20121115081321.GF8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003628.GG17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 7/8] Git.pm: teach \"ident\" to query explicitness","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T08:13:21Z","receivedAt":"2012-11-15T08:13:21Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Signed-off-by: Jeff King \n\nFor what it's worth,\nAcked-by: Jonathan Nieder \n"},{"id":"203277","messageId":"20121115082316.GG8429@elie.Belkin","threadId":"32079","inReplyTo":"20121115003640.GH17819@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T08:23:16Z","receivedAt":"2012-11-15T08:23:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '\n> \n> test_expect_success $PREREQ 'Prompting works' '\n> \tclean_fake_sendmail &&\n> -\t(echo \"Example \"\n> -\t echo \"to@example.com\"\n> +\t(echo \"to@example.com\"\n> \t echo \"\"\n> \t) | GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n> \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> \t\t$patches \\\n> \t\t2>errors &&\n> +\t\tgrep \"^From: A U Thor \\$\" msgtxt1 &&\n> +\t\tgrep \"^To: to@example.com\\$\" msgtxt1\n> +'\n\nThe indentation seems strange here --- are the new \"grep\" lines\ncontinuations of the git send-email line?\n\nIt's probably easier to change the structure completely:\n\n\tclean_fake_sendmail &&\n\techo to@examples.com >prompt.input &&\n\techo >>prompt.input &&\n\tGIT_SEND_EMAIL_NOTTY=1 \\\n\t\tgit send-email --smtp-server=... $patches \\$\" msgtxt1 &&\n\tgrep \"^To: to@example.com\\$\" msgtxt1\n\n> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '\n> +\tclean_fake_sendmail &&\n> +\t(echo \"Example \" &&\n> +\t echo \"to@example.com\" &&\n> +\t echo \"\"\n> +\t) |\n> +\t(sane_unset GIT_AUTHOR_NAME &&\n> +\t sane_unset GIT_AUTHOR_EMAIL &&\n> +\t sane_unset GIT_COMMITTER_NAME &&\n> +\t sane_unset GIT_COMMITTER_EMAIL &&\n> +\t GIT_SEND_EMAIL_NOTTY=1 git send-email \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t$patches \\\n> +\t\t2>errors &&\n> \t\tgrep \"^From: Example \\$\" msgtxt1 &&\n> \t\tgrep \"^To: to@example.com\\$\" msgtxt1\n> +\t)\n> +'\n\nLikewise:\n\n\tclean_fake_sendmail &&\n\techo \"Example \" >prompt.in &&\n\techo to@example.com >>prompt.in\n\techo >>prompt.in &&\n\t(\n\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&\n\t\tsane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&\n\t\tGIT_SEND_EMAIL_NOTTY=1 \\\n\t\t\tgit send-email --smtp-server=... $patches \\$\" msgtxt1 &&\n\tgrep \"^To: to@example.com\\$\" msgtxt1\n\n> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '\n> +\tclean_fake_sendmail &&\n> +\t(sane_unset GIT_AUTHOR_NAME &&\n> +\t sane_unset GIT_AUTHOR_EMAIL &&\n> +\t sane_unset GIT_COMMITTER_NAME &&\n> +\t sane_unset GIT_COMMITTER_EMAIL &&\n> +\t GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&\n> +\t test_must_fail git send-email \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t$patches errors.out &&\n> +\t\ttest_i18ngrep \"tell me who you are\" errors.out\n> +\t)\n> '\n\nLikewise:\n\n\tclean_fake_sendmail &&\n\t(\n\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&\n\t\tsane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&\n\t\tGIT_SEND_EMAIL_NOTTY=1 \\\n\t\t\tgit send-email --smtp-server=... $patches err\n\t) &&\n\ttest_i18ngrep \"[Tt]ell me who you are\" err\n\nFor what it's worth, with or without such changes,\nAcked-by: Jonathan Nieder \n"},{"id":"203278","messageId":"20121115083315.GA23377@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s0d+g7bXCnOf55jZNNFS6uJ+4BDowx5uYxWBP4xA+-0zA@mail.gmail.com","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T08:33:17Z","receivedAt":"2012-11-15T08:33:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:\n\n> I don't think there's any need for all that, this does the trick:\n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index aea66a0..503e551 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -748,16 +748,11 @@ if (!$force) {\n> }\n> }\n> \n> -my $prompting = 0;\n> if (!defined $sender) {\n> $sender = $repoauthor || $repocommitter || '';\n> - $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n> - default => $sender,\n> - valid_re => qr/\\@.*\\./, confirm_only => 1);\n> - print \"Emails will be sent from: \", $sender, \"\\n\";\n> - $prompting++;\n> }\n> \n> +my $prompting = 0;\n> \n> This passes all the current tests and the ones you added.\n\nIt may pass on your system, but it will not on a system that meets the\nAUTOIDENT prerequisite (it fails the new t9001.19 on my system; I\nsuspect your system config is such that we skip t9001.19 and run\nt9001.20, whereas mine is the opposite).\n\n> Which kind of user will get the prompt with your patch, that would\n> miss it with mine?\n\nOne whose system is configured in such a way that git can produce an\nautomatic ident (i.e., has a non-blank GECOS name and a FQDN).\n\n-Peff\n"},{"id":"203280","messageId":"CAMP44s2NBGDRLUKhBTU+kNy7Fyn8T6qm3nneSbS4rrNN1oPgdw@mail.gmail.com","threadId":"32079","inReplyTo":"20121115083315.GA23377@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-15T10:28:46Z","receivedAt":"2012-11-15T10:28:46Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 15, 2012 at 9:33 AM, Jeff King wrote:\n> On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:\n>\n>> I don't think there's any need for all that, this does the trick:\n>>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index aea66a0..503e551 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -748,16 +748,11 @@ if (!$force) {\n>> }\n>> }\n>>\n>> -my $prompting = 0;\n>> if (!defined $sender) {\n>> $sender = $repoauthor || $repocommitter || '';\n>> - $sender = ask(\"Who should the emails appear to be from? [$sender] \",\n>> - default => $sender,\n>> - valid_re => qr/\\@.*\\./, confirm_only => 1);\n>> - print \"Emails will be sent from: \", $sender, \"\\n\";\n>> - $prompting++;\n>> }\n>>\n>> +my $prompting = 0;\n>>\n>> This passes all the current tests and the ones you added.\n>\n> It may pass on your system, but it will not on a system that meets the\n> AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I\n> suspect your system config is such that we skip t9001.19 and run\n> t9001.20, whereas mine is the opposite).\n\nI tried both:\n\nok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of\nPERL,AUTOIDENT)\nok 20 - broken implicit ident aborts send-email\n\nok 19 - implicit ident prompts for sender\nok 20 # skip broken implicit ident aborts send-email (missing\n!AUTOIDENT of PERL,!AUTOIDENT)\n\nHowever, it would be much easier if ident learned to check\nGIT_TEST_FAKE_HOSTNAME, or something.\n\nBut then I realized I had to run 'make' again. Yes, my patch breaks\ntest 19, but see below.\n\n>> Which kind of user will get the prompt with your patch, that would\n>> miss it with mine?\n>\n> One whose system is configured in such a way that git can produce an\n> automatic ident (i.e., has a non-blank GECOS name and a FQDN).\n\nAnd doesn't have any of the following:\n\n * configured user.name/user.email\n * specified $EMAIL\n * configured sendemail.from\n * specified --from argument\n\nVery unlikely. And then, what would be the consequences of not\nreceiving this prompt?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203281","messageId":"20121115104345.GA32465@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s2NBGDRLUKhBTU+kNy7Fyn8T6qm3nneSbS4rrNN1oPgdw@mail.gmail.com","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T10:43:59Z","receivedAt":"2012-11-15T10:43:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 11:28:46AM +0100, Felipe Contreras wrote:\n\n> I tried both:\n> \n> ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of\n> PERL,AUTOIDENT)\n> ok 20 - broken implicit ident aborts send-email\n> \n> ok 19 - implicit ident prompts for sender\n> ok 20 # skip broken implicit ident aborts send-email (missing\n> !AUTOIDENT of PERL,!AUTOIDENT)\n> \n> However, it would be much easier if ident learned to check\n> GIT_TEST_FAKE_HOSTNAME, or something.\n\nYes, it would be. It has two downsides:\n\n 1. The regular git code has to be instrumented to respect the\n variable, so it can potentially affect git in production use\n outside of the test suite. Since such code is simple, though, it is\n probably not a big risk.\n\n 2. We would not actually exercise the code paths for doing\n hostname and GECOS lookup. We do not test their resulting values,\n so the coverage is not great now, but we do at least run the code,\n which would let a run with \"--valgrind\" check it. I guess we could\n go through the motions of assembling the ident and then replace\n it at the end with the fake value.\n\nI don't have a strong opinion either way.\n\n> > One whose system is configured in such a way that git can produce an\n> > automatic ident (i.e., has a non-blank GECOS name and a FQDN).\n> \n> And doesn't have any of the following:\n> \n> * configured user.name/user.email\n> * specified $EMAIL\n> * configured sendemail.from\n> * specified --from argument\n> \n> Very unlikely.\n\nThat is certainly the opinion you have stated already. I'm not sure I\nagree. Linus, for example, was an advocate of such a configuration early\non in git's history. I don't think he still runs that way, though.\n\n> And then, what would be the consequences of not receiving this prompt?\n\nAn email would be sent with the generated identity.\n\n-Peff\n"},{"id":"203283","messageId":"20121115111334.GA1879@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115104345.GA32465@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T11:13:47Z","receivedAt":"2012-11-15T11:13:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 02:43:58AM -0800, Jeff King wrote:\n\n> > And doesn't have any of the following:\n> > \n> > * configured user.name/user.email\n> > * specified $EMAIL\n> > * configured sendemail.from\n> > * specified --from argument\n> > \n> > Very unlikely.\n> \n> That is certainly the opinion you have stated already. I'm not sure I\n> agree. Linus, for example, was an advocate of such a configuration early\n> on in git's history. I don't think he still runs that way, though.\n> \n> > And then, what would be the consequences of not receiving this prompt?\n> \n> An email would be sent with the generated identity.\n\nI suspect you did not need me to answer that question, but were setting\nit up as a rhetorical trap to mention the final confirmation, which I\nfailed to note in my response.\n\nI think a much more compelling argument/commit message for your\nsuggested patch would be:\n\n We currently prompt the user for the \"From\" address. This is an\n inconvenience in the common case that the user has configured their\n identity in the environment, but is meant as a safety check for when\n git falls back to an implicitly generated identity (which may or may\n not be valid).\n\n That safety check is not really necessary, though, as by default\n send-email will prompt the user for a final confirmation before\n sending out any message. The likelihood that a user has both bothered\n to turn off this default _and_ not configured any identity (nor\n checked that the automatic identity is valid) is rather low.\n\nI could accept that line of reasoning. I see that this argument is\nburied deep in your commit message, but I will admit to not reading your\n9-point list of conditions all that closely, as the first 7 points are,\nin my opinion, not relevant (and I had already read and disagreed with\nthem in other messages).\n\n-Peff\n"},{"id":"203284","messageId":"20121115115018.GA3437@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115111334.GA1879@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T11:50:20Z","receivedAt":"2012-11-15T11:50:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 03:13:47AM -0800, Jeff King wrote:\n\n> I think a much more compelling argument/commit message for your\n> suggested patch would be:\n> \n> We currently prompt the user for the \"From\" address. This is an\n> inconvenience in the common case that the user has configured their\n> identity in the environment, but is meant as a safety check for when\n> git falls back to an implicitly generated identity (which may or may\n> not be valid).\n> \n> That safety check is not really necessary, though, as by default\n> send-email will prompt the user for a final confirmation before\n> sending out any message. The likelihood that a user has both bothered\n> to turn off this default _and_ not configured any identity (nor\n> checked that the automatic identity is valid) is rather low.\n\nIf that is the route we want to go, then we should obviously drop my\nseries in favor of your final patch. I think it would also need a test\nupdate, no?\n\nI think a more concise commit message would help, too. I disagree with a\ngreat deal of the reasoning in your existing message, but those parts\nturn out not to be relevant. The crux of the issue is that the safety\ncheck is not necessary because there is already one (i.e., point 8 of\nyour list). Feel free to use any or all of my text above.\n\n>From my series, there were a few cleanups that might be worth salvaging.\nHere is a rundown by patch:\n\n [1/8]: test-lib: allow negation of prerequisites\n\nThis stands on its own, and is something I have wanted a few times in\nthe past. However, since there is no immediate user, I don't know if it\nis worth doing or not.\n\n [2/8]: t7502: factor out autoident prerequisite\n\nA minor cleanup and possible help to future tests, but since there are\nno other callers now, not sure if it is worth it.\n\n [3/8]: ident: make user_ident_explicitly_given static\n\nA cleanup that is worth doing, I think.\n\n [4/8]: ident: keep separate \"explicit\" flags for author and committer\n\nAnother cleanup. This is \"more correct\", in that it handles the corner\ncases I mentioned in the commit message. But no current code cares about\nthose corner cases, because the only real caller is git-commit, and this\nis a purely internal interface. I could take or leave it.\n\n [5/8]: var: accept multiple variables on the command line\n\nThe tests for this can be split out; we currently don't have \"git var\"\ntests at all, so increasing our test coverage is reasonable. The\nmultiple variables thing is potentially useful, but there are simply not\nthat many callers of \"git var\", and nobody has been asking for such a\nfeature (we could use it to save a process in git-send-email, but it is\nprobably not worth the complexity).\n\n [6/8]: var: provide explicit/implicit ident information\n [7/8]: Git.pm: teach \"ident\" to query explicitness\n\nThese two should probably be dropped. They would lock us into supporting\nthe explicit/implicit variables in \"git var\", for no immediate benefit.\n\n [8/8]: send-email: do not prompt for explicit repo ident\n\nObviously drop.\n\n> I could accept that line of reasoning. I see that this argument is\n> buried deep in your commit message, but I will admit to not reading your\n> 9-point list of conditions all that closely, as the first 7 points are,\n> in my opinion, not relevant (and I had already read and disagreed with\n> them in other messages).\n\nIf it sounds like I am blaming you here, I am to some degree. But I am\nalso blaming myself. I should have read your commit message more\ncarefully, and I'm sorry for not doing so. I hope we can both try harder\nto avoid getting side-tracked on arguing about issues that turned out\nnot to be important at all (of course, we cannot know which part of the\ndiscussion will turn out to be important, but I think there some\nobviously unproductive parts of this discussion).\n\n-Peff\n"},{"id":"203296","messageId":"20121115164228.GA18108@sigill.intra.peff.net","threadId":"32079","inReplyTo":"20121115074658.GA8429@elie.Belkin","subject":"Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:42:30Z","receivedAt":"2012-11-15T16:42:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:\n\n> > +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '\n> \n> I have a visceral nervousness when reading this code, from too much\n> unpleasant experience of bash's csh-style !history expansion. Luckily\n> bash does not treat ! specially in the '-o sh' mode used by tests.\n\nI can understand, having been bitten by it many times myself (not only\nis it counter-intuitively expanded inside double quotes, and not only\ndoes it not do what you wanted, but it does not insert the command into\nthe history, so you cannot even go back to fix it easily).\n\n> Does this feature work when running a test explicitly using\n> \"bash \"? That's something I do from time to time to\n> figure out whether a weird behavior is shell-specific.\n\nYes. You can test it yourself with \"bash t0000-basic.sh\". The reason is\nthat the \"!\" is part of history expansion, which is only enabled by\ndefault for interactive shells.\n\n> If it works everywhere, this patch would help me conquer my fear of\n> exclamation points in git's tests, which would be a comfort to me and\n> a very good thing.\n\nAs far as I know, \"!\" should be safe in a non-interactive shell script.\n\n-Peff\n"},{"id":"203297","messageId":"20121115164921.GA17173@elie.Belkin","threadId":"32079","inReplyTo":"20121115164228.GA18108@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-11-15T16:49:21Z","receivedAt":"2012-11-15T16:49:21Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Yes. You can test it yourself with \"bash t0000-basic.sh\". The reason is\n> that the \"!\" is part of history expansion, which is only enabled by\n> default for interactive shells.\n\nNice to hear. Thanks much for looking into it.\n\n> On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:\n\n>> If it works everywhere, this patch would help me conquer my fear of\n>> exclamation points in git's tests, which would be a comfort to me and\n>> a very good thing.\n"},{"id":"203299","messageId":"7vvcd6954q.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121115111334.GA1879@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-15T16:56:37Z","receivedAt":"2012-11-15T16:56:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think a much more compelling argument/commit message for your\n> suggested patch would be:\n>\n> We currently prompt the user for the \"From\" address. This is an\n> inconvenience in the common case that the user has configured their\n> identity in the environment, but is meant as a safety check for when\n> git falls back to an implicitly generated identity (which may or may\n> not be valid).\n>\n> That safety check is not really necessary, though, as by default\n> send-email will prompt the user for a final confirmation before\n> sending out any message. The likelihood that a user has both bothered\n> to turn off this default _and_ not configured any identity (nor\n> checked that the automatic identity is valid) is rather low.\n\nThis somehow reminds me of the first paragraph of f20f387 (commit:\ncheck committer identity more strictly, 2012-07-23).\n\nI never use \"send-email driving format-patch\" workflow myself, but I\nsuspect there are people among who do so who are using --compose to\ndo the cover letter of their series. Does the \"confirmation as the\nlast step\" help them, or would they have to retype their message?\n"},{"id":"203302","messageId":"20121115172845.GA20298@sigill.intra.peff.net","threadId":"32079","inReplyTo":"7vvcd6954q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T17:28:47Z","receivedAt":"2012-11-15T17:28:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 08:56:37AM -0800, Junio C Hamano wrote:\n\n> > I think a much more compelling argument/commit message for your\n> > suggested patch would be:\n> >\n> > We currently prompt the user for the \"From\" address. This is an\n> > inconvenience in the common case that the user has configured their\n> > identity in the environment, but is meant as a safety check for when\n> > git falls back to an implicitly generated identity (which may or may\n> > not be valid).\n> >\n> > That safety check is not really necessary, though, as by default\n> > send-email will prompt the user for a final confirmation before\n> > sending out any message. The likelihood that a user has both bothered\n> > to turn off this default _and_ not configured any identity (nor\n> > checked that the automatic identity is valid) is rather low.\n> \n> This somehow reminds me of the first paragraph of f20f387 (commit:\n> check committer identity more strictly, 2012-07-23).\n> \n> I never use \"send-email driving format-patch\" workflow myself, but I\n> suspect there are people among who do so who are using --compose to\n> do the cover letter of their series. Does the \"confirmation as the\n> last step\" help them, or would they have to retype their message?\n\nThat is a good question. That confirmation step does come after they\nhave typed their cover letter. However, if they are using --compose,\nthey are dumped in their editor with something like:\n\n From Jeff King # This line is ignored.\n GIT: Lines beginning in \"GIT:\" will be removed.\n GIT: Consider including an overall diffstat or table of contents\n GIT: for the patch you are writing.\n GIT:\n GIT: Clear the body content if you don't wish to send a summary.\n From: Jeff King \n Subject: \n In-Reply-To: \n\nwhich I think would count as sufficient notice of the address being\nused.\n\n-Peff\n"},{"id":"203329","messageId":"7vzk2i6s9h.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121115172845.GA20298@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-16T05:17:30Z","receivedAt":"2012-11-16T05:17:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That is a good question. That confirmation step does come after they\n> have typed their cover letter. However, if they are using --compose,\n> they are dumped in their editor with something like:\n>\n> From Jeff King # This line is ignored.\n> GIT: Lines beginning in \"GIT:\" will be removed.\n> GIT: Consider including an overall diffstat or table of contents\n> GIT: for the patch you are writing.\n> GIT:\n> GIT: Clear the body content if you don't wish to send a summary.\n> From: Jeff King \n> Subject: \n> In-Reply-To: \n>\n> which I think would count as sufficient notice of the address being\n> used.\n\nOK. Tentatively I replaced your old series with these 8 patches\nincluding the last one, as I tend to agree with the value the\nearlier clean-up in the series gives us in the longer term. As you\nand Felipe discussed, we may want to replace the last one with a\nsimpler \"don't bother asking\" patch, but I think that is more or\nless an orthogonal issue.\n\nThanks.\n"},{"id":"203359","messageId":"20121116190811.GB2310@sigill.intra.peff.net","threadId":"32079","inReplyTo":"7vzk2i6s9h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-16T19:08:11Z","receivedAt":"2012-11-16T19:08:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > That is a good question. That confirmation step does come after they\n> > have typed their cover letter. However, if they are using --compose,\n> > they are dumped in their editor with something like:\n> >\n> > From Jeff King # This line is ignored.\n> > GIT: Lines beginning in \"GIT:\" will be removed.\n> > GIT: Consider including an overall diffstat or table of contents\n> > GIT: for the patch you are writing.\n> > GIT:\n> > GIT: Clear the body content if you don't wish to send a summary.\n> > From: Jeff King \n> > Subject: \n> > In-Reply-To: \n> >\n> > which I think would count as sufficient notice of the address being\n> > used.\n> \n> OK. Tentatively I replaced your old series with these 8 patches\n> including the last one, as I tend to agree with the value the\n> earlier clean-up in the series gives us in the longer term. As you\n> and Felipe discussed, we may want to replace the last one with a\n> simpler \"don't bother asking\" patch, but I think that is more or\n> less an orthogonal issue.\n\nI'm not sure how orthogonal it is. The latter half of my series is about\nexposing the user_ident_sufficiently_given() flag. If we go with\nFelipe's patch, then that exposed information has no users, and it may\nnot be worth it (OTOH, it's possible that some third-party script may\nwant it).\n\n-Peff\n"},{"id":"203363","messageId":"CAMP44s2+0vFUwK+ATe-jDTRYG=kE=zFF4X_JAMZExgVw0Vtfgw@mail.gmail.com","threadId":"32079","inReplyTo":"20121116190811.GB2310@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-11-16T19:57:43Z","receivedAt":"2012-11-16T19:57:43Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 16, 2012 at 8:08 PM, Jeff King wrote:\n> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > That is a good question. That confirmation step does come after they\n>> > have typed their cover letter. However, if they are using --compose,\n>> > they are dumped in their editor with something like:\n>> >\n>> > From Jeff King # This line is ignored.\n>> > GIT: Lines beginning in \"GIT:\" will be removed.\n>> > GIT: Consider including an overall diffstat or table of contents\n>> > GIT: for the patch you are writing.\n>> > GIT:\n>> > GIT: Clear the body content if you don't wish to send a summary.\n>> > From: Jeff King \n>> > Subject:\n>> > In-Reply-To:\n>> >\n>> > which I think would count as sufficient notice of the address being\n>> > used.\n>>\n>> OK. Tentatively I replaced your old series with these 8 patches\n>> including the last one, as I tend to agree with the value the\n>> earlier clean-up in the series gives us in the longer term. As you\n>> and Felipe discussed, we may want to replace the last one with a\n>> simpler \"don't bother asking\" patch, but I think that is more or\n>> less an orthogonal issue.\n>\n> I'm not sure how orthogonal it is. The latter half of my series is about\n> exposing the user_ident_sufficiently_given() flag. If we go with\n> Felipe's patch, then that exposed information has no users, and it may\n> not be worth it (OTOH, it's possible that some third-party script may\n> want it).\n\nWell, who is using user_ident_sufficiently_given() in the first place?\nI think 'git commit' might be suffering from the same problem that\nprompted you to split it.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"203366","messageId":"7vzk2hl3gv.fsf@alter.siamese.dyndns.org","threadId":"32079","inReplyTo":"20121116190811.GB2310@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-16T20:04:00Z","receivedAt":"2012-11-16T20:04:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:\n> ...\n>> OK. Tentatively I replaced your old series with these 8 patches\n>> including the last one, as I tend to agree with the value the\n>> earlier clean-up in the series gives us in the longer term. As you\n>> and Felipe discussed, we may want to replace the last one with a\n>> simpler \"don't bother asking\" patch, but I think that is more or\n>> less an orthogonal issue.\n>\n> I'm not sure how orthogonal it is. The latter half of my series is about\n> exposing the user_ident_sufficiently_given() flag. If we go with\n> Felipe's patch, then that exposed information has no users, and it may\n> not be worth it (OTOH, it's possible that some third-party script may\n> want it).\n\nOh, no, you are correct. I shouldn't have said \"the last one\", but\ninstead \"the later ones\". The first two we should definitely keep,\nat least, but the other three are not so clear.\n"},{"id":"203368","messageId":"20121116201107.GA5740@sigill.intra.peff.net","threadId":"32079","inReplyTo":"CAMP44s2+0vFUwK+ATe-jDTRYG=kE=zFF4X_JAMZExgVw0Vtfgw@mail.gmail.com","subject":"Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-16T20:11:08Z","receivedAt":"2012-11-16T20:11:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 16, 2012 at 08:57:43PM +0100, Felipe Contreras wrote:\n\n> > I'm not sure how orthogonal it is. The latter half of my series is about\n> > exposing the user_ident_sufficiently_given() flag. If we go with\n> > Felipe's patch, then that exposed information has no users, and it may\n> > not be worth it (OTOH, it's possible that some third-party script may\n> > want it).\n> \n> Well, who is using user_ident_sufficiently_given() in the first place?\n> I think 'git commit' might be suffering from the same problem that\n> prompted you to split it.\n\nIt is just `git commit` now. It does not suffer from the problems that\nprompted the author/committer split:\n\n http://article.gmane.org/gmane.comp.version-control.git/209635\n\nTo expand on what I wrote there, we cannot hit case 2 because we always\nask for the committer within the same process. Case 1 is not\ninteresting, because we would only fail to show it if is identical to a\nnon-implicit committer (so even if it was implicit, we know that it is a\nsane value).\n\n-Peff\n"}]}