patchsend-email: add proper default sender

> There's no point in asking this over and over if the user already
> properly configured his/her name and email.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> 
> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index aea66a0..65b8328 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -748,6 +748,17 @@ if (!$force) {
>  	}
>  }
>  
> +my $name = Git::config('user.name');
> +my $email = Git::config('user.email');
> +
> +if (defined $email) {
> +	if (defined $name) {
> +		$sender = sprintf("%s <%s>", $name, $email);
> +	} else {
> +		$sender = $email;
> +	}
> +}

> On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:
>
>> There's no point in asking this over and over if the user already
>> properly configured his/her name and email.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>
>> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.
>
> It should be defaulting to your regular git ident, and you just have to
> hit enter, right?

>>  my $prompting = 0;
>>  if (!defined $sender) {
>>       $sender = $repoauthor || $repocommitter || '';
>
> Why not just use $repoauthor or $repocommitter, as the prompt default
> already does?

> > Why not use Git::ident_person() here? It saves some code, and would also
> > respect environment variables. Or better yet...
> 
> I assume there was a reason why that code was asking for input;
> precisely because it would use the environment variables. For some
> reason the user might have exported GIT_AUTHOR_EMAIL, or maybe EMAIL
> is not right, or the full name config.
> 
> OTOH user.name/.email configurations come clearly from the user.

> On Mon, Nov 12, 2012 at 07:02:17PM -0500, Jeff King wrote:
>
>> The one distinction that would make sense to me is pausing to ask when
>> we use "implicit" methods to look up the ident, like concatenating the
>> username with the hostname to get the email.
>
> By the way, I suspect this is the answer to "what do people type for
> this prompt". It is probably more about a safety on bad ident than it is
> about people routinely updating the information. 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.

> On Tue, Nov 13, 2012 at 12:42:02AM +0100, Felipe Contreras wrote:
>
>> > Why not use Git::ident_person() here? It saves some code, and would also
>> > respect environment variables. Or better yet...
>>
>> I assume there was a reason why that code was asking for input;
>> precisely because it would use the environment variables. For some
>> reason the user might have exported GIT_AUTHOR_EMAIL, or maybe EMAIL
>> is not right, or the full name config.
>>
>> OTOH user.name/.email configurations come clearly from the user.
>
> But we use the environment to default the field, so the distinction
> doesn't make much sense to me.  Plus, it has always been the case that
> you can use git without setting user.*, but instead only using the
> environment. I don't see any reason not to follow that principle here,
> too.

> The one distinction that would make sense to me is pausing to ask when
> we use "implicit" methods to look up the ident, like concatenating the
> username with the hostname to get the email.
>
> Git::ident uses "git var" to do its lookup, which will use IDENT_STRICT;
> that stops most junk like empty names and bogus domains. But I think we
> would want to go one step further and actually check
> user_ident_sufficiently_given.  Unfortunately, that is not currently
> available outside of C. You'd probably want something like:
>
> diff --git a/builtin/var.c b/builtin/var.c
> index aedbb53..eaf324e 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -26,6 +26,12 @@ static const char *pager(int flag)
>         return pgm;
>  }
>
> +static const char *explicit_ident(int flag)
> +{
> +       git_committer_info(flag);
> +       return user_ident_sufficiently_given() ? "1" : "0";
> +}
> +
>  struct git_var {
>         const char *name;
>         const char *(*read)(int);
> @@ -35,6 +41,7 @@ static struct git_var git_vars[] = {
>         { "GIT_AUTHOR_IDENT",   git_author_info },
>         { "GIT_EDITOR", editor },
>         { "GIT_PAGER", pager },
> +       { "GIT_EXPLICIT_IDENT", explicit_ident },
>         { "", NULL },
>  };

> > But we use the environment to default the field, so the distinction
> > doesn't make much sense to me.  Plus, it has always been the case that
> > you can use git without setting user.*, but instead only using the
> > environment. I don't see any reason not to follow that principle here,
> > too.
> 
> And that's why a lot of commits end up like michael
> <michael@michael-laptop.(none)>.

> On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:
> 
> > > But we use the environment to default the field, so the distinction
> > > doesn't make much sense to me.  Plus, it has always been the case that
> > > you can use git without setting user.*, but instead only using the
> > > environment. I don't see any reason not to follow that principle here,
> > > too.
> > 
> > And that's why a lot of commits end up like michael
> > <michael@michael-laptop.(none)>.
> 
> No, it's not. Those broken names do not come from the environment, but
> from our last-resort guess of the hostname. We long ago switched to
> printing the name as a warning when we have made such a guess (bb1ae3f),
> then more recently started rejecting them outright (8c5b1ae).
> 
> And I have proposed exactly the same behavior here: respect the
> environment and the config, but do not trust the implicit guesses.

> On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:
>
>> > But we use the environment to default the field, so the distinction
>> > doesn't make much sense to me.  Plus, it has always been the case that
>> > you can use git without setting user.*, but instead only using the
>> > environment. I don't see any reason not to follow that principle here,
>> > too.
>>
>> And that's why a lot of commits end up like michael
>> <michael@michael-laptop.(none)>.
>
> No, it's not. Those broken names do not come from the environment, but
> from our last-resort guess of the hostname.

>> Probably. But what I really want is to stop 'git send-email' from
>> asking. I think the one next step further can be done later.
>
> But in the meantime you are causing a regression for anybody who expects
> GIT_AUTHOR_NAME to override user.email when running git-send-email (and
> you have taken away the prompt that they could have used to notice and
> correct it).

> > No, it's not. Those broken names do not come from the environment, but
> > from our last-resort guess of the hostname.
> 
> That depends how you define environment, but fine, the point is that
> it happens.

> > We long ago switched to
> > printing the name as a warning when we have made such a guess (bb1ae3f),
> > then more recently started rejecting them outright (8c5b1ae).
> 
> Right, but these would still happen:
> 
> michael <michael@michael-laptop.michael.org>

> > But in the meantime you are causing a regression for anybody who expects
> > GIT_AUTHOR_NAME to override user.email when running git-send-email (and
> > you have taken away the prompt that they could have used to notice and
> > correct it).
> 
> I think they can survive. If anybody like this exists.

> On Tue, Nov 13, 2012 at 04:55:25AM +0100, Felipe Contreras wrote:
>
>> > No, it's not. Those broken names do not come from the environment, but
>> > from our last-resort guess of the hostname.
>>
>> That depends how you define environment, but fine, the point is that
>> it happens.
>
> If you have a strawman definition that does not have anything to do with
> what I said in my original email, then yes, it could happen.

>> > We long ago switched to
>> > printing the name as a warning when we have made such a guess (bb1ae3f),
>> > then more recently started rejecting them outright (8c5b1ae).
>>
>> Right, but these would still happen:
>>
>> michael <michael@michael-laptop.michael.org>
>
> Did you read my email? I explicitly proposed that we would _not_ allow
> send-email to use implicit email addresses constructed in that way.

>> > But in the meantime you are causing a regression for anybody who expects
>> > GIT_AUTHOR_NAME to override user.email when running git-send-email (and
>> > you have taken away the prompt that they could have used to notice and
>> > correct it).
>>
>> I think they can survive. If anybody like this exists.
>
> Sorry, but that is not how things work on this project. You do not get
> to cause regressions because you are too lazy to implement the feature
> _you_ want in a way that does not break other people.

> 6) GIT_AUTHOR
>
> Who should the emails appear to be from? [Felipe Contreras 4th
> <felipe.contreras+4@gmail.com>]
>
> What about after my change?
>
> 6.1) GIT_AUTHOR without anything else
>
> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
> var GIT_COMMITTER_IDENT: command returned error: 128

> >> > No, it's not. Those broken names do not come from the environment, but
> >> > from our last-resort guess of the hostname.
> >>
> >> That depends how you define environment, but fine, the point is that
> >> it happens.
> >
> > If you have a strawman definition that does not have anything to do with
> > what I said in my original email, then yes, it could happen.
> 
> It happens, I've seen commits with (none) not that long ago.

> > But as I said already, "git var" uses IDENT_STRICT and will not
> > allow such broken names.
> 
> Since 1.7.11, sure. But not everyone is using such a recent version of
> git, and people with fully qualified domains would still get unwanted
> behavior.

> > Did you read my email? I explicitly proposed that we would _not_ allow
> > send-email to use implicit email addresses constructed in that way.
> 
> I'm not talking about git send-email, I'm talking about your comment
> 'it has always been the case that you can use git without setting
> user.*', which has caused issues with wrong author/commmitter names in
> commits, and will probably continue to do so.

> > Sorry, but that is not how things work on this project. You do not get
> > to cause regressions because you are too lazy to implement the feature
> > _you_ want in a way that does not break other people.
> 
> That doesn't change the fact that they would survive, and the fact
> that those users don't actually exist.

> But let's look at the current situation closely:
> 
> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1
> 
> 1) No information at all
> 
> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed

> 2) Full Name + full hostname
> 
> Who should the emails appear to be from? [Felipe Contreras
> <felipec@nysa.felipec.org>]
> 
> That's right, ident doesn't fail, and that's not the mail address I
> specified, it's *implicit*.

> 5) GIT_COMMITTER
> 
> Who should the emails appear to be from? [Felipe Contreras 2nd
> <felipe.contreras+2@gmail.com>]
> 
> Whoa, what happened there?
> 
> Well:
> 
>   $sender = $repoauthor || $repocommitter || '';
>   ($repoauthor) = Git::ident_person(@repo, 'author');
>   % ./git var GIT_AUTHOR_IDENT
>   Felipe Contreras 2nd <felipe.contreras+2@gmail.com> 1352783223 +0100
> 
> That's right, AUTHOR_IDENT would fall back to the default email and full name.

> What about after my change?
> 
> 6.1) GIT_AUTHOR without anything else
> 
> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
> var GIT_COMMITTER_IDENT: command returned error: 128

> 6.1) GIT_AUTHOR without anything else
> 
> Even if the previous problem was solved:
> 
> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export
> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'
> ./git var GIT_EXPLICIT_IDENT
> 0
> 
> No explicit ident? This is most certainly not what the user would expect.

> 5.2) GIT_COMMITTER with Full Name and full hostname
> 
> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export
> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'
> ./git var GIT_EXPLICIT_IDENT
> 1
> 
> From: Felipe Contreras <felipec@nysa.felipec.org>
> 
> It is explicit, yeah, but 'git send-email' would not be picking the
> committer, it would pick the author.

> --- a/ident.c
> +++ b/ident.c
> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,
>         }
> 
>         if (strict && email == git_default_email.buf &&
> -           strstr(email, "(none)")) {
> +               !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {
>                 fputs(env_hint, stderr);
> -               die("unable to auto-detect email address (got '%s')", email);
> +               die("no explicit email address");

> Those people would also not be using a new version of git-send-email,
> and it will always prompt. I thought we were talking about what
> send-email should do in future versions. Namely, loosening that safety
> valve (the prompt) because it is inconvenient, but tightening the checks
> so that losing the safety valve is not a problem.

>> I'm not talking about git send-email, I'm talking about your comment
>> 'it has always been the case that you can use git without setting
>> user.*', which has caused issues with wrong author/commmitter names in
>> commits, and will probably continue to do so.
>
> The second half of that sentence that you quoted above is "...instead
> only using the environment." As in, environment variables like
> GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, and EMAIL. _Not_ implicit
> generation of the email from the username and hostname.

>> But let's look at the current situation closely:
>>
>> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1
>>
>> 1) No information at all
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>
> That is dependent on your system. If you have a non-empty name in your
> GECOS field, and if your machine has a FQDN, it will currently work (and
> prompt).

>> 2) Full Name + full hostname
>>
>> Who should the emails appear to be from? [Felipe Contreras
>> <felipec@nysa.felipec.org>]
>>
>> That's right, ident doesn't fail, and that's not the mail address I
>> specified, it's *implicit*.
>
> Right. I never said it did. I said it currently rejected obviously bogus
> stuff (like the ".(none)" above) due to IDENT_STRICT, but currently
> allowed implicit definitions. And I also said that if we get rid of the
> prompt, we should disallow implicit definitions like this, because the
> prompt is the safety valve on sending out mails with broken from
> addresses. I even wrote a patch that let you find out whether the ident
> was generated implicitly.

>> 3) Full Name + EMAIL
>>
>> Who should the emails appear to be from? [Felipe Contreras
>> <felipe.contreras@gmail.com>]
>
> Which sounds fine to me. EMAIL is considered explicit, and I have not
> seen any evidence that people are putting bogus values in their EMAIL
> variable and complaining that it is git's fault for respecting it.

>> 5) GIT_COMMITTER
>>
>> Who should the emails appear to be from? [Felipe Contreras 2nd
>> <felipe.contreras+2@gmail.com>]
>>
>> Whoa, what happened there?
>>
>> Well:
>>
>>   $sender = $repoauthor || $repocommitter || '';
>>   ($repoauthor) = Git::ident_person(@repo, 'author');
>>   % ./git var GIT_AUTHOR_IDENT
>>   Felipe Contreras 2nd <felipe.contreras+2@gmail.com> 1352783223 +0100
>>
>> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>
> Yeah, I find that somewhat questionable in the current behavior, and I'd
> consider it a bug. Typically we prefer the committer ident when given a
> choice (e.g., for writing reflog entries).

>> 5.1) GIT_COMMITER without anything else
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> var GIT_AUTHOR_IDENT: command returned error: 128
>
> Right. Same bug as above.

>> 6) GIT_AUTHOR
>>
>> Who should the emails appear to be from? [Felipe Contreras 4th
>> <felipe.contreras+4@gmail.com>]
>
> Right, that's what I'd expect.

>> What about after my change?
>>
>> 6.1) GIT_AUTHOR without anything else
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> var GIT_COMMITTER_IDENT: command returned error: 128
>
> Doesn't that seem like a regression? It used to work.

>> 4) config user
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> OK.
>
>> 5) GIT_COMMITTER
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> OK.
>
>> 6) GIT_AUTHOR
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> Doesn't that seem like a regression? It used to use a different address,
> and in every other git program, the environment takes precedence over
> config.

>> And what about your proposed change?
>
> Let me be clear that I sent you a "something like this" patch to try to
> point you in the right direction. If it has a bug or is incomplete, that
> does not mean the direction is wrong, but only that I did not spend very
> much time on the patch.

>> 6.1) GIT_AUTHOR without anything else
>>
>> Even if the previous problem was solved:
>>
>> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export
>> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'
>> ./git var GIT_EXPLICIT_IDENT
>> 0
>>
>> No explicit ident? This is most certainly not what the user would expect.
>
> Yes, it looks like we do not set up the explicit ident flags when
> parsing the author. So my patch is insufficient.

>> 5.2) GIT_COMMITTER with Full Name and full hostname
>>
>> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export
>> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'
>> ./git var GIT_EXPLICIT_IDENT
>> 1
>>
>> From: Felipe Contreras <felipec@nysa.felipec.org>
>>
>> It is explicit, yeah, but 'git send-email' would not be picking the
>> committer, it would pick the author.
>
> Yep.
>
> The explicitness needs to be tied to the specific ident we grabbed.
> Probably adding a "git var GIT_AUTHOR_EXPLICIT" would be enough, or
> alternatively, adding a flag to "git var" to error out rather than
> return a non-explicit ident (this may need to adjust the error
> handling of the "git var" calls from send-email).

>> > I tried to help you by pointing you in the right direction and even
>> > providing a sample "git var" patch.
>>
>> Are you 100% sure that was the right direction?
>
> I think that respecting the usual ident lookup but disallowing implicit
> identities (either totally, or causing them to fallback to prompting) is
> the right direction.  I agree my patch was not a complete solution. I'm
> sorry if it led you astray in terms of implementation, but I also think
> I've been very clear in my text about what the behavior should be.

>> I think the right approach is more along these lines:
>
> I think that is moving in the right direction, but...
>
>> --- a/ident.c
>> +++ b/ident.c
>> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,
>>         }
>>
>>         if (strict && email == git_default_email.buf &&
>> -           strstr(email, "(none)")) {
>> +               !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {
>>                 fputs(env_hint, stderr);
>> -               die("unable to auto-detect email address (got '%s')", email);
>> +               die("no explicit email address");
>
> I think this needs to be optional, otherwise you are breaking callers
> who use IDENT_STRICT but are OK with the implicit ident (e.g.,
> commit, format-patch with threading).
>
> You can argue whether "git commit" should disallow such addresses, but
> that is a separate topic from how send-email should behave.

>> Not only will this fix 'git send-email', but it will also fix 'git
>> commit' so that we don't end up with authors such as 'Felipe Contreras
>> <felipec@nysa.felipec.org>' ever again.
>
> While simultaneously breaking "git commit" for people who are happily
> using the implicit generation. I can see the appeal of doing so; I was
> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.
> But do we have any data on how many people are currently using that
> feature that would be annoyed by it?

>> > But it is not my itch to scratch.
>>
>> Suit yourself, it's only git users that would get hurt. I can always
>> use my own 'git send-email' (as I am doing right now).
>
> Don't get me wrong. I think the spirit of your patch is correct, and it
> helps some git users. But it also hurts others. And it is not that hard
> to do it right.

> It may be something I would work on myself in the future, but I have
> other things to work on at the moment, and since you are interested in
> the topic, I thought you would be a good candidate to polish it enough
> to be suitable upstream. But instead I see a lot of push-back on what I
> considered to be a fairly straightforward technical comment on a
> regression.

> And now I have wasted a large chunk of the evening responding to you,
> neither accomplishing my other tasks nor polishing this topic. I do not
> mind reviewing patches or responding to discussions, nor do I consider
> them time wasted; they are an important part of the development process.
> But I feel like I am fighting an uphill battle just to convince you that
> regressions are bad, and that I am having to make the same points
> repeatedly.  That makes me frustrated and less excited about reviewing
> your patches; and when I say "it is not my itch", that is my most polite
> way of saying "If that is going to be your attitude, then I do not feel
> like dealing with you anymore on this topic".

> > Those people would also not be using a new version of git-send-email,
> > and it will always prompt. I thought we were talking about what
> > send-email should do in future versions. Namely, loosening that safety
> > valve (the prompt) because it is inconvenient, but tightening the checks
> > so that losing the safety valve is not a problem.
> 
> Yeah, but all I'm saying is that the issue happens, you seemed to
> suggest that it doesn't.

> I think you are the one that is not understanding what I'm saying. But
> I don't think it matters.
> 
> This is what I'm saying; the current situation with 'git commit' is
> not OK, _both_ 'git commit' and 'git send-email' should change.

> Indeed I would, but there's other people that would benefit from this
> patch. I'm sure I'm not the only person that doesn't have
> sendmail.from configured, but does have user.name/user.email, and is
> constantly typing enter.
> 
> And the difference is that I'm _real_, the hypothetical user that
> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
> otherwise if some evidence was presented that such a user is real
> though.

> But listen closely to what you said:
> 
> > 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.
> 
> Suppose somebody has a full name, and a fully qualified domain name,
> and he can receive mails to it directly. Such a user would not need a
> git configuration, and would not need $EMAIL, or anything.
> 
> Currently 'git send-email' will throw 'Felipe Contreras
> <felipec@felipec.org>' which would actually work, but is not explicit.
> 
> You are suggesting to break that use-case. You are introducing a
> regression. And this case is realistic, unlike the
> GIT_AUTHOR_NAME/EMAIL. Isn't it?

> > As for whether they exist, what data do you have?
> 
> What data do _you_ have?
> 
> When there's no evidence either way, the rational response is to don't
> believe. That's the default position.

> > Are you aware that the
> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not
> > having any user.* config?
> 
> What tests?  My patch doesn't seem to break anything there:
> % make -C t t9001-send-email.sh
> # passed all 96 test(s)

> > When somebody comes on the list and asks why
> > every git program in the entire system respects GIT_* environment
> > variables as an override to user.* configuration _except_ for
> > send-email, what should I say?
> 
> The same thing you say when somebody comes reporting a bug: "yeah, we
> should probably fix that".

> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
> >
> > Yeah, I find that somewhat questionable in the current behavior, and I'd
> > consider it a bug. Typically we prefer the committer ident when given a
> > choice (e.g., for writing reflog entries).
> 
> Yeah, but clearly the intention of the code was to use the committer
> if the author wasn't available, which is the case here.

> >> What about after my change?
> >>
> >> 6.1) GIT_AUTHOR without anything else
> >>
> >> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
> >> var GIT_COMMITTER_IDENT: command returned error: 128
> >
> > Doesn't that seem like a regression? It used to work.
> 
> No, this is *before* my change.
> 
> I's the same bug as 5.1):

> >> And what about your proposed change?
> >
> > Let me be clear that I sent you a "something like this" patch to try to
> > point you in the right direction. If it has a bug or is incomplete, that
> > does not mean the direction is wrong, but only that I did not spend very
> > much time on the patch.
> 
> It doesn't matter, the idea was to use user_ident_sufficiently_given().

> > I think that respecting the usual ident lookup but disallowing implicit
> > identities (either totally, or causing them to fallback to prompting) is
> > the right direction.  I agree my patch was not a complete solution. I'm
> > sorry if it led you astray in terms of implementation, but I also think
> > I've been very clear in my text about what the behavior should be.
> 
> I think that is orthogonal to what I'm trying accomplish.

> > Don't get me wrong. I think the spirit of your patch is correct, and it
> > helps some git users. But it also hurts others. And it is not that hard
> > to do it right.
> 
> And I disagree, I think it hurts nobody, and I think it's hard to do it right.

> --- a/cache.h
> +++ b/cache.h
> @@ -1149,10 +1149,6 @@ struct config_include_data {
>  #define CONFIG_INCLUDE_INIT { 0 }
>  extern int git_config_include(const char *name, const char *value, void *data);
>  
> -#define IDENT_NAME_GIVEN 01
> -#define IDENT_MAIL_GIVEN 02
> -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> -extern int user_ident_explicitly_given;
>  extern int user_ident_sufficiently_given(void);

> > -#define IDENT_NAME_GIVEN 01
> > -#define IDENT_MAIL_GIVEN 02
> > -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> > -extern int user_ident_explicitly_given;
> >  extern int user_ident_sufficiently_given(void);
> 
> In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given" was
> introduced as a global for communication between config, ident, and
> builtin-commit.  In v1.7.0-rc0~72^2 (2010-01-07) readers switched to
> using the common wrapper user_ident_sufficiently_given().  After
> v1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,
> and the variable can finally be made static.

> --- a/Documentation/git-var.txt
> +++ b/Documentation/git-var.txt
> @@ -9,11 +9,16 @@ git-var - Show a git logical variable
>  SYNOPSIS
>  --------
>  [verse]
> -'git var' ( -l | <variable> )
> +'git var' ( -l | <variable>... )
>  
>  DESCRIPTION
>  -----------
> -Prints a git logical variable.
> +Prints one or more git logical variables, separated by newlines.
> +
> +Note that some variables may contain newlines themselves

> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
>  
>  int cmd_var(int argc, const char **argv, const char *prefix)
>  {
> -	const char *val = NULL;
> -	if (argc != 2)
> +	if (argc < 2)
>  		usage(var_usage);
>  
>  	if (strcmp(argv[1], "-l") == 0) {

> --- /dev/null
> +++ b/t/t0007-git-var.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +
> +test_description='basic sanity checks for git var'
> +. ./test-lib.sh
> +
> +test_expect_success 'get GIT_AUTHOR_IDENT' '
> +	test_tick &&
> +	echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&

> >  DESCRIPTION
> >  -----------
> > -Prints a git logical variable.
> > +Prints one or more git logical variables, separated by newlines.
> > +
> > +Note that some variables may contain newlines themselves
> 
> Maybe a -z option to NUL-terminate values would be useful some day.

> > --- a/builtin/var.c
> > +++ b/builtin/var.c
> > @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
> >  
> >  int cmd_var(int argc, const char **argv, const char *prefix)
> >  {
> > -	const char *val = NULL;
> > -	if (argc != 2)
> > +	if (argc < 2)
> >  		usage(var_usage);
> >  
> >  	if (strcmp(argv[1], "-l") == 0) {
> 
> What should happen if I pass "-l" followed by other arguments?

> > +	test_tick &&
> > +	echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&
> 
> Do we need to hardcode the timestamp?  Something like
> 
> 	test_cmp_filtered () {
> 		expect=$1 actual=$2 &&
> 		sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP" \
> 			<"$actual" >"$actual.filtered" &&
> 		test_cmp "$expect" "$actual.filtered"
> 	}

> Internally, we keep track of whether the author or committer
> ident information was provided by the user, or whether it
> was implicitly determined by the system. However, there is
> currently no way for external programs or scripts to get
> this information

> Jeff King wrote:
> 
> > Internally, we keep track of whether the author or committer
> > ident information was provided by the user, or whether it
> > was implicitly determined by the system. However, there is
> > currently no way for external programs or scripts to get
> > this information
> 
> What are the intended semantics?  If my machine has /etc/mailname
> filled out, is that an implicit identity?  How about if I set the
> EMAIL envvar but not GIT_COMMITTER_EMAIL?
> 
> If external scripts are going to start using this mechanism, they will
> need answers to these questions to support users that run into
> configuration problems.  A few words on this in the documentation
> could probably help.

> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -737,7 +737,7 @@ sub remote_refs {
>  }
>  
>  
> -=item ident ( TYPE | IDENTSTR )
> +=item ident ( TYPE | IDENTSTR [, options] )
>  
>  =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
>  
> @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
>  Alternatively, it can take a prepared ident string (e.g. from the commit
>  object) and just parse it.
>  
> +If the C<explicit> option is set to 1, the returned array will contain an
> +additional boolean specifying whether the ident was configure explicitly by the
> +user.

> > --- a/perl/Git.pm
> > +++ b/perl/Git.pm
> > @@ -737,7 +737,7 @@ sub remote_refs {
> >  }
> >  
> >  
> > -=item ident ( TYPE | IDENTSTR )
> > +=item ident ( TYPE | IDENTSTR [, options] )
> >  
> >  =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
> >  
> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
> >  Alternatively, it can take a prepared ident string (e.g. from the commit
> >  object) and just parse it.
> >  
> > +If the C<explicit> option is set to 1, the returned array will contain an
> > +additional boolean specifying whether the ident was configure explicitly by the
> > +user.
> 
> s/configure/configured/
> 
> I'd suggest adding "See GIT_COMMITTER_EXPLICIT in git-var(1) for
> details" to make the semantics crystal clear.  What do you think?

> If git-send-email is configured with sendemail.from, we will
> not prompt the user for the "From" address of the emails.
> If it is not configured, we prompt the user, but provide the
> repo author or committer as a default.  Even though we
> probably have a sensible value for the default, the prompt
> is a safety check in case git generated an incorrect
> implicit ident string.

> The test scripts need to be adjusted to not expect a prompt
> for the sender, since they always have the author explicitly
> defined in the environment. Unfortunately, we cannot
> reliably test that prompting still happens in the implicit
> case, as send-email will produce inconsistent results
> depending on the machine config (if we cannot find a FQDN,
> "git var" will barf, causing us to exit early;

> > The test scripts need to be adjusted to not expect a prompt
> > for the sender, since they always have the author explicitly
> > defined in the environment. Unfortunately, we cannot
> > reliably test that prompting still happens in the implicit
> > case, as send-email will produce inconsistent results
> > depending on the machine config (if we cannot find a FQDN,
> > "git var" will barf, causing us to exit early;
> 
> At first this sounded like a bug to me --- how could the user keep
> working without the sysadmin intervening?
> 
> But then I remembered that the user can set her name and email in
> .gitconfig and probably would want to in such a setup anyway.

> > When someone writes such a test, I think it could check that git
> > either prompts or writes a message advising to configure the user
> > email, no?  Waiting until later for that seems fine to me, though.
> 
> Yes. The problem is that the behavior and output are dependent on
> factors outside the test suite, so we would have to check that one of
> the possible expected outcomes happens. But I think there are really
> only two such outcomes (neglecting that the ident itself can have
> arbitrary content, but we do not have to check the actual content).

>> I think you are the one that is not understanding what I'm saying. But
>> I don't think it matters.
>>
>> This is what I'm saying; the current situation with 'git commit' is
>> not OK, _both_ 'git commit' and 'git send-email' should change.
>
> Perhaps I am being dense, but this is the first time it became apparent
> to me that you are arguing for a change in "git commit".

>> Indeed I would, but there's other people that would benefit from this
>> patch. I'm sure I'm not the only person that doesn't have
>> sendmail.from configured, but does have user.name/user.email, and is
>> constantly typing enter.
>>
>> And the difference is that I'm _real_, the hypothetical user that
>> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
>> otherwise if some evidence was presented that such a user is real
>> though.
>
> Sadly we cannot poll the configuration of every user, nor expect them
> all to pay attention to this discussion on the list. So we cannot take
> the absence of comment from such users as evidence that they do not
> exist.

>> And to balance you need to *measure*, and that means taking into
>> consideration who actually uses the features, if there's any. And it
>> looks to me this is a feature nobody uses.
>
> You said "measure" and then "it looks to me like". What did you measure?
> Did you consider systematic bias in your measurement, like the fact that
> people who are using the feature have no reason to come on the list and
> announce it?

>> But listen closely to what you said:
>>
>> > 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.
>>
>> Suppose somebody has a full name, and a fully qualified domain name,
>> and he can receive mails to it directly. Such a user would not need a
>> git configuration, and would not need $EMAIL, or anything.
>>
>> Currently 'git send-email' will throw 'Felipe Contreras
>> <felipec@felipec.org>' which would actually work, but is not explicit.
>>
>> You are suggesting to break that use-case. You are introducing a
>> regression. And this case is realistic, unlike the
>> GIT_AUTHOR_NAME/EMAIL. Isn't it?
>
> Yes, dying would be a regression, in that you would have to configure
> your name via the environment and re-run rather than type it at the
> prompt. You raise a good point that for people who _could_ take the
> implicit default, hitting "enter" is working fine now, and we would lose
> that.  I'd be fine with also just continuing to prompt in the implicit
> case.
>
> But that is a much smaller issue to me than having send-email fail to
> respect environment variables and silently use user.*, which is what
> started this whole discussion. And I agree it is worth considering as a
> regression we should avoid.

>> I prefer to concentrate on real issues, but that's just me.
>
> To be honest, I am confused at this point what you actually want. Do you
> think we should take your original patch?  I think its regression is too
> great. And I am not sure if you agree or not. You seem to be arguing
> that the regression is not important, yet you simultaneously argue that
> we should be making all ident more strict, which would mean that we
> should indeed take my suggestion and use "git var" instead of preferring
> the config.

>> > As for whether they exist, what data do you have?
>>
>> What data do _you_ have?
>>
>> When there's no evidence either way, the rational response is to don't
>> believe. That's the default position.
>
> This is not religion. It is a software project.

>> > Are you aware that the
>> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not
>> > having any user.* config?
>>
>> What tests?  My patch doesn't seem to break anything there:
>> % make -C t t9001-send-email.sh
>> # passed all 96 test(s)
>
> My point was that there is at least one known setup that uses only
> environment variables and not a config file, and that the rest of git is
> intended to work with that.  It is not a test failure, but t9001.18
> reveals the fact that your change does not handle this situation; we
> continue to prompt under the test suite's configuration. In the proper
> fix, the test needs to be adjusted.
>
> You don't see any test failures with your patch because we do not cover
> the case that you regress (GIT_AUTHOR_EMAIL and user.email both set).

>> > When somebody comes on the list and asks why
>> > every git program in the entire system respects GIT_* environment
>> > variables as an override to user.* configuration _except_ for
>> > send-email, what should I say?
>>
>> The same thing you say when somebody comes reporting a bug: "yeah, we
>> should probably fix that".
>
> It is a larger hassle for both the developer and the user to fix the bug
> after the fact,

>> It's all about proportion. Is it possible that we all are going to die
>> tomorrow because of an asteroid? Sure... but what's the point of
>> worrying about it if it's not likely?
>
> If you want to talk about risk assessment, then the right computation is
> to compare the cost of fixing it now versus the cost of fixing it later
> times the probability of it happening. The development cost is probably
> a little higher later (because we have to refresh ourselves on the
> issue), but the deployment cost is much higher (e.g., users whose
> distros ship a broken version, or who have IT policy that does not let
> them upgrade git as soon as the bug-fix ships).

>> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>> >
>> > Yeah, I find that somewhat questionable in the current behavior, and I'd
>> > consider it a bug. Typically we prefer the committer ident when given a
>> > choice (e.g., for writing reflog entries).
>>
>> Yeah, but clearly the intention of the code was to use the committer
>> if the author wasn't available, which is the case here.
>
> Yes. It would make sense to me to respect an explicit committer over an
> implicit author. Though the fact that author is preferred over committer
> is inconsistent with most of the rest of git. I'm undecided whether that
> should be changed.

>> >> What about after my change?
>> >>
>> >> 6.1) GIT_AUTHOR without anything else
>> >>
>> >> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> >> var GIT_COMMITTER_IDENT: command returned error: 128
>> >
>> > Doesn't that seem like a regression? It used to work.
>>
>> No, this is *before* my change.
>>
>> I's the same bug as 5.1):
>
> Ah, I see. We are loading both, whether or not they end up being used,
> and barfing prematurely on errors. That seems like a bug.

>> >> And what about your proposed change?
>> >
>> > Let me be clear that I sent you a "something like this" patch to try to
>> > point you in the right direction. If it has a bug or is incomplete, that
>> > does not mean the direction is wrong, but only that I did not spend very
>> > much time on the patch.
>>
>> It doesn't matter, the idea was to use user_ident_sufficiently_given().
>
> Right. Which it sounds like now agree with me on?

>> > I think that respecting the usual ident lookup but disallowing implicit
>> > identities (either totally, or causing them to fallback to prompting) is
>> > the right direction.  I agree my patch was not a complete solution. I'm
>> > sorry if it led you astray in terms of implementation, but I also think
>> > I've been very clear in my text about what the behavior should be.
>>
>> I think that is orthogonal to what I'm trying accomplish.
>
> What is it you are trying to accomplish? Eliminating the prompt in some
> cases, as in your original patch? Dealing with implicit identities may
> be orthogonal to your goal, but your original patch is not sufficient,
> as it reverses the precedence of config and environment variables.

> So you can either:
>
>   1. Reimplement the environment variable lookup that ident.c does,
>      leaving implicit ident logic out completely.
>
>   2. Modify ident.c and "git var" to let send-email reuse the logic in
>      ident.c, but avoid dropping the prompt when an implicit ident is
>      used.
>
> Doing (2) sounds a lot more maintainable to me in the long run.

>> > Don't get me wrong. I think the spirit of your patch is correct, and it
>> > helps some git users. But it also hurts others. And it is not that hard
>> > to do it right.
>>
>> And I disagree, I think it hurts nobody, and I think it's hard to do it right.
>
> I am tired of talking about this. The patch series below took me less
> time to write than I have spent arguing with you. It was not that hard.
>
>   [1/6]: ident: make user_ident_explicitly_given private
>   [2/6]: ident: keep separate "explicit" flags for author and committer
>   [3/6]: var: accept multiple variables on the command line
>   [4/6]: var: provide explicit/implicit ident information
>   [5/6]: Git.pm: teach "ident" to query explicitness
>   [6/6]: send-email: do not prompt for explicit repo ident

> If you then fix it, we are at
> state C. From your perspective, it may be "I do not want to make the fix
> to go from B to C". But from the project's perspective, it is "we do not
> want to go from state A to state B; state C would be acceptable". So
> from the perspective of people who do not care about your feature but do
> care about the regression in state B, it is inextricably linked to your
> itch.

>> yet the patch I sent above does it, and it even fixes 'git commit'
>> (IMO). But it's also not good enough.
>
> I do not necessarily agree on "git commit". Moreover, I feel like it is
> a separate issue. My series above _just_ implements the "do not prompt
> when explicit" behavior. It does not deal with git-commit at all, nor
> does it address the author/committer fallback questions. Those can
> easily go on top.

> > Yes, dying would be a regression, in that you would have to configure
> > your name via the environment and re-run rather than type it at the
> > prompt. You raise a good point that for people who _could_ take the
> > implicit default, hitting "enter" is working fine now, and we would lose
> > that.  I'd be fine with also just continuing to prompt in the implicit
> > case.
> >
> > But that is a much smaller issue to me than having send-email fail to
> > respect environment variables and silently use user.*, which is what
> > started this whole discussion. And I agree it is worth considering as a
> > regression we should avoid.
> 
> It might be smaller, I don't think so. A hypothetical user that was
> relying on GIT_AUTHOR for whatever reason can switch to 'git
> send-email --from' (which is much easier) when they notice the
> failure, the same way somebody relying on fqdn would. The difference
> is that people with fqdn do exist, and they might be relying on this.
> 
> Both are small issues, that I agree with.
> 
> But the point is that you seem to be very adamant about _my_
> regressions, and not pay much attention about yours.

> > So you can either:
> >
> >   1. Reimplement the environment variable lookup that ident.c does,
> >      leaving implicit ident logic out completely.
> >
> >   2. Modify ident.c and "git var" to let send-email reuse the logic in
> >      ident.c, but avoid dropping the prompt when an implicit ident is
> >      used.
> >
> > Doing (2) sounds a lot more maintainable to me in the long run.
> 
> Or:
> 
> 3. Change the meaning of the STRICT flag so that the values are
> explicit, which has benefits outside 'git send-email'. Yes, this would
> change the behavior in 'git commit' and other tools, but it's worth to
> investigate these changes, and most likely they would be desirable.

> Or:
> 
> 4. Just stop prompting
> 
> I already sent a patch for 4. with all the details of why nobody (or
> very few, if any) would be affected negatively.

> >   [1/6]: ident: make user_ident_explicitly_given private
> >   [2/6]: ident: keep separate "explicit" flags for author and committer
> >   [3/6]: var: accept multiple variables on the command line
> >   [4/6]: var: provide explicit/implicit ident information
> >   [5/6]: Git.pm: teach "ident" to query explicitness
> >   [6/6]: send-email: do not prompt for explicit repo ident
> 
> I think this adds a lot of code that nobody would use.

> > I do not necessarily agree on "git commit". Moreover, I feel like it is
> > a separate issue. My series above _just_ implements the "do not prompt
> > when explicit" behavior. It does not deal with git-commit at all, nor
> > does it address the author/committer fallback questions. Those can
> > easily go on top.
> 
> Yes, at the cost of adding a lot of code. If we end up agreeing that
> the changes to 'git commit' are desirable (which I hope at some point
> we will), then this code would be all for nothing.

> I want clarify that this is merely a disagreement to at which level
> should we worry about regressions. On one side of the spectrum you
> have projects like GNOME, who don't have any problem breaking the
> user-experience from one release to the next, I'm not proposing
> anything like that. On the other side I think it's you, because I
> don't recall encountering anybody with such an extreme position of
> never introducing a regression ever if there's absolutely no evidence
> that anybody is using certain feature.

> On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:
>
>> > Yes, dying would be a regression, in that you would have to configure
>> > your name via the environment and re-run rather than type it at the
>> > prompt. You raise a good point that for people who _could_ take the
>> > implicit default, hitting "enter" is working fine now, and we would lose
>> > that.  I'd be fine with also just continuing to prompt in the implicit
>> > case.
>> >
>> > But that is a much smaller issue to me than having send-email fail to
>> > respect environment variables and silently use user.*, which is what
>> > started this whole discussion. And I agree it is worth considering as a
>> > regression we should avoid.
>>
>> It might be smaller, I don't think so. A hypothetical user that was
>> relying on GIT_AUTHOR for whatever reason can switch to 'git
>> send-email --from' (which is much easier) when they notice the
>> failure, the same way somebody relying on fqdn would. The difference
>> is that people with fqdn do exist, and they might be relying on this.
>>
>> Both are small issues, that I agree with.
>>
>> But the point is that you seem to be very adamant about _my_
>> regressions, and not pay much attention about yours.
>
> Really? I mentioned initially the possibility of dying instead of
> prompting. You raised the point that it would regress a certain use
> case. And then what happened? I said above "you raise a good point[...].
> I'd be fine with also just continuing to prompt[...]. I agree it is
> worth considering as a regression we should avoid". And then I sent out
> a patch series which does not have the regression.
>
> In other words, my suggestion was a bad one, and once it was pointed
> out, I did not pursue it.  If you want to call that "not paying much
> attention", feel free. But I'd much rather you point out problems in my
> actual patch series.

>> The second patch doesn't have this issue. It does change the behavior
>> of 'git commit', yeah, but I think that's a benefit.
>
> Changing "git commit" is even something I would entertain. It would be a
> regression for some people, but at least it buys us something (increased
> safety against people making bogus commits and failing to notice the
> warning). I'm undecided on whether that is worth it or not.
>
> But when you presented it, as far as I could tell the change in behavior
> to "git commit" was accidental (which is why I pointed it out in
> response).

> If you want to seriously propose changing the behavior of "git commit",
> I think the best thing would be to make a real patch, laying out the
> pros and cons in the commit message, and post it. I would not be
> surprised if the other list participants have stopped reading our thread
> at this point, and the idea is going otherwise unnoticed.

>> Or:
>>
>> 4. Just stop prompting
>>
>> I already sent a patch for 4. with all the details of why nobody (or
>> very few, if any) would be affected negatively.
>
> If doing (2) were really hard, that might be worth considering. But it's
> not. I already did it. So I don't see how this is an attractive option,
> unless my series is so unpalatable that we would rather accept a
> regression.

>> >   [1/6]: ident: make user_ident_explicitly_given private
>> >   [2/6]: ident: keep separate "explicit" flags for author and committer
>> >   [3/6]: var: accept multiple variables on the command line
>> >   [4/6]: var: provide explicit/implicit ident information
>> >   [5/6]: Git.pm: teach "ident" to query explicitness
>> >   [6/6]: send-email: do not prompt for explicit repo ident
>>
>> I think this adds a lot of code that nobody would use.
>
> A lot of code? It is mostly refactoring,

>> > I do not necessarily agree on "git commit". Moreover, I feel like it is
>> > a separate issue. My series above _just_ implements the "do not prompt
>> > when explicit" behavior. It does not deal with git-commit at all, nor
>> > does it address the author/committer fallback questions. Those can
>> > easily go on top.
>>
>> Yes, at the cost of adding a lot of code. If we end up agreeing that
>> the changes to 'git commit' are desirable (which I hope at some point
>> we will), then this code would be all for nothing.
>
> If we are going to change "git commit" immediately, then I agree there
> is not much point merging my series. But even if we do change it, will
> we do so immediately? Will there be a deprecation period? If so, then my
> series helps send-email in the meantime. And it's already written, so
> you do not even have to do anything.

>> I want clarify that this is merely a disagreement to at which level
>> should we worry about regressions. On one side of the spectrum you
>> have projects like GNOME, who don't have any problem breaking the
>> user-experience from one release to the next, I'm not proposing
>> anything like that. On the other side I think it's you, because I
>> don't recall encountering anybody with such an extreme position of
>> never introducing a regression ever if there's absolutely no evidence
>> that anybody is using certain feature.
>
> I don't think that's a fair characterization of my position. I am fine
> with introducing a regression if there is a large benefit to it, and
> especially if the regression is mutually exclusive with the benefit. For
> example, look at IDENT_STRICT. We used to allow broken email addresses
> in commits, and it was _me_ who pushed forward the change to disallow
> it. That potentially regressed people who would rather have junk in the
> commit objects than configure their identity (e.g., because they are
> creating commits on the backend of some automated process). But we
> discussed it, and the breakage was worth the increased safety for normal
> users. We could not have it both ways, since the safety came at the
> expense of switching the default.
>
> But with this topic, we had a too-safe default (a safety prompt that was
> sometimes overkill). We can have our cake and eat it, too: drop the
> prompt for the overkill cases, but leave the other cases untouched. And
> that is what I tried to do in my series.  Note that this _still_
> regresses certain use cases. What if I have configured my user.email,
> but I am expecting send-email to prompt me so I can put in some other
> random value. But we can't improve the prompting and leave that case
> there; they are mutually exclusive. But IMHO, the benefit outweighs the
> possibility of breakage.

> > If you want to seriously propose changing the behavior of "git commit",
> > I think the best thing would be to make a real patch, laying out the
> > pros and cons in the commit message, and post it. I would not be
> > surprised if the other list participants have stopped reading our thread
> > at this point, and the idea is going otherwise unnoticed.
> 
> I would, if I saw any chance in it actually going through.

> On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:
>
>> But that I meant that when I introduce a regression it's like I'm
>> killing all that is good and sacred about git, and when you do it's
>> everything but that.
>
> The rhetoric in this statement is a good indication that there is
> nothing productive to come from our discussing it anymore.

>> > If you want to seriously propose changing the behavior of "git commit",
>> > I think the best thing would be to make a real patch, laying out the
>> > pros and cons in the commit message, and post it. I would not be
>> > surprised if the other list participants have stopped reading our thread
>> > at this point, and the idea is going otherwise unnoticed.
>>
>> I would, if I saw any chance in it actually going through.
>
> Well, it certainly will not go through if you do not try.

>   [1/6]: ident: make user_ident_explicitly_given private
>   [2/6]: ident: keep separate "explicit" flags for author and committer
>   [3/6]: var: accept multiple variables on the command line
>   [4/6]: var: provide explicit/implicit ident information
>   [5/6]: Git.pm: teach "ident" to query explicitness
>   [6/6]: send-email: do not prompt for explicit repo ident

> > +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
> 
> I have a visceral nervousness when reading this code, from too much
> unpleasant experience of bash's csh-style !history expansion.  Luckily
> bash does not treat ! specially in the '-o sh' mode used by tests.

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
>  	esac
>  '
>  
> +test_lazy_prereq AUTOIDENT '
> +	sane_unset GIT_AUTHOR_NAME &&
> +	sane_unset GIT_AUTHOR_EMAIL &&
> +	git var GIT_AUTHOR_IDENT
> +'

> In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given"
> was introduced as a global for communication between config,
> ident, and builtin-commit.  In v1.7.0-rc0~72^2 (2010-01-07)
> readers switched to using the common wrapper
> user_ident_sufficiently_given().  After v1.7.11-rc1~15^2~18
> (2012-05-21), the var is only written in ident.c.
>
> Now we can make it static, which will enable further
> refactoring without worrying about upsetting other code.
>
> Signed-off-by: Jeff King <peff@peff.net>

>   1. GIT_COMMITTER_* is set explicitly, but we fallback for
>      GIT_AUTHOR. We claim the ident is explicit, even though
>      the author is not.
>
>   2. GIT_AUTHOR_* is set and we ask for author ident, but
>      not committer ident. We will claim the ident is
>      implicit, even though it is explicit.
>
> This patch uses two variables instead of one, updates both
> when we set the "fallback" values, and updates them
> individually when we read from the environment.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5a7c29d..0c49b32 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -436,9 +436,8 @@ if (0) {
>         }
>  }
>
> -my ($repoauthor, $repocommitter);
> -($repoauthor) = Git::ident_person(@repo, 'author');
> -($repocommitter) = Git::ident_person(@repo, 'committer');
> +my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
> +my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');
>
>  # Verify the user input
>
> @@ -755,12 +754,17 @@ if (!$force) {
>
>  my $prompting = 0;
>  if (!defined $sender) {
> -       $sender = $repoauthor || $repocommitter || '';
> -       $sender = ask("Who should the emails appear to be from? [$sender] ",
> -                     default => $sender,
> -                     valid_re => qr/\@.*\./, confirm_only => 1);
> -       print "Emails will be sent from: ", $sender, "\n";
> -       $prompting++;
> +       ($sender, my $explicit) =
> +               defined $repoauthor ? ($repoauthor, $author_explicit) :
> +               defined $repocommitter ? ($repocommitter, $committer_explicit) :
> +               ('', 0);
> +       if (!$explicit) {
> +               $sender = ask("Who should the emails appear to be from? [$sender] ",
> +                             default => $sender,
> +                             valid_re => qr/\@.*\./, confirm_only => 1);
> +               print "Emails will be sent from: ", $sender, "\n";
> +               $prompting++;
> +       }
>  }
>
>  if (!@initial_to && !defined $to_cmd) {

> I don't think there's any need for all that, this does the trick:
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index aea66a0..503e551 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -748,16 +748,11 @@ if (!$force) {
>         }
>  }
> 
> -my $prompting = 0;
>  if (!defined $sender) {
>         $sender = $repoauthor || $repocommitter || '';
> -       $sender = ask("Who should the emails appear to be from? [$sender] ",
> -                     default => $sender,
> -                     valid_re => qr/\@.*\./, confirm_only => 1);
> -       print "Emails will be sent from: ", $sender, "\n";
> -       $prompting++;
>  }
> 
> +my $prompting = 0;
> 
> This passes all the current tests and the ones you added.

> On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:
>
>> I don't think there's any need for all that, this does the trick:
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index aea66a0..503e551 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -748,16 +748,11 @@ if (!$force) {
>>         }
>>  }
>>
>> -my $prompting = 0;
>>  if (!defined $sender) {
>>         $sender = $repoauthor || $repocommitter || '';
>> -       $sender = ask("Who should the emails appear to be from? [$sender] ",
>> -                     default => $sender,
>> -                     valid_re => qr/\@.*\./, confirm_only => 1);
>> -       print "Emails will be sent from: ", $sender, "\n";
>> -       $prompting++;
>>  }
>>
>> +my $prompting = 0;
>>
>> This passes all the current tests and the ones you added.
>
> It may pass on your system, but it will not on a system that meets the
> AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
> suspect your system config is such that we skip t9001.19 and run
> t9001.20, whereas mine is the opposite).

>> Which kind of user will get the prompt with your patch, that would
>> miss it with mine?
>
> One whose system is configured in such a way that git can produce an
> automatic ident (i.e., has a non-blank GECOS name and a FQDN).

> I tried both:
> 
> ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of
> PERL,AUTOIDENT)
> ok 20 - broken implicit ident aborts send-email
> 
> ok 19 - implicit ident prompts for sender
> ok 20 # skip broken implicit ident aborts send-email (missing
> !AUTOIDENT of PERL,!AUTOIDENT)
> 
> However, it would be much easier if ident learned to check
> GIT_TEST_FAKE_HOSTNAME, or something.

> > One whose system is configured in such a way that git can produce an
> > automatic ident (i.e., has a non-blank GECOS name and a FQDN).
> 
> And doesn't have any of the following:
> 
>  * configured user.name/user.email
>  * specified $EMAIL
>  * configured sendemail.from
>  * specified --from argument
> 
> Very unlikely.

> > And doesn't have any of the following:
> > 
> >  * configured user.name/user.email
> >  * specified $EMAIL
> >  * configured sendemail.from
> >  * specified --from argument
> > 
> > Very unlikely.
> 
> That is certainly the opinion you have stated already. I'm not sure I
> agree. Linus, for example, was an advocate of such a configuration early
> on in git's history. I don't think he still runs that way, though.
> 
> > And then, what would be the consequences of not receiving this prompt?
> 
> An email would be sent with the generated identity.

> I think a much more compelling argument/commit message for your
> suggested patch would be:
> 
>   We currently prompt the user for the "From" address. This is an
>   inconvenience in the common case that the user has configured their
>   identity in the environment, but is meant as a safety check for when
>   git falls back to an implicitly generated identity (which may or may
>   not be valid).
> 
>   That safety check is not really necessary, though, as by default
>   send-email will prompt the user for a final confirmation before
>   sending out any message. The likelihood that a user has both bothered
>   to turn off this default _and_ not configured any identity (nor
>   checked that the automatic identity is valid) is rather low.

> I could accept that line of reasoning.  I see that this argument is
> buried deep in your commit message, but I will admit to not reading your
> 9-point list of conditions all that closely, as the first 7 points are,
> in my opinion, not relevant (and I had already read and disagreed with
> them in other messages).

> I think a much more compelling argument/commit message for your
> suggested patch would be:
>
>   We currently prompt the user for the "From" address. This is an
>   inconvenience in the common case that the user has configured their
>   identity in the environment, but is meant as a safety check for when
>   git falls back to an implicitly generated identity (which may or may
>   not be valid).
>
>   That safety check is not really necessary, though, as by default
>   send-email will prompt the user for a final confirmation before
>   sending out any message. The likelihood that a user has both bothered
>   to turn off this default _and_ not configured any identity (nor
>   checked that the automatic identity is valid) is rather low.

> > I think a much more compelling argument/commit message for your
> > suggested patch would be:
> >
> >   We currently prompt the user for the "From" address. This is an
> >   inconvenience in the common case that the user has configured their
> >   identity in the environment, but is meant as a safety check for when
> >   git falls back to an implicitly generated identity (which may or may
> >   not be valid).
> >
> >   That safety check is not really necessary, though, as by default
> >   send-email will prompt the user for a final confirmation before
> >   sending out any message. The likelihood that a user has both bothered
> >   to turn off this default _and_ not configured any identity (nor
> >   checked that the automatic identity is valid) is rather low.
> 
> This somehow reminds me of the first paragraph of f20f387 (commit:
> check committer identity more strictly, 2012-07-23).
> 
> I never use "send-email driving format-patch" workflow myself, but I
> suspect there are people among who do so who are using --compose to
> do the cover letter of their series.  Does the "confirmation as the
> last step" help them, or would they have to retype their message?

> That is a good question. That confirmation step does come after they
> have typed their cover letter. However, if they are using --compose,
> they are dumped in their editor with something like:
>
>   From Jeff King <peff@peff.net> # This line is ignored.
>   GIT: Lines beginning in "GIT:" will be removed.
>   GIT: Consider including an overall diffstat or table of contents
>   GIT: for the patch you are writing.
>   GIT:
>   GIT: Clear the body content if you don't wish to send a summary.
>   From: Jeff King <peff@peff.net>
>   Subject: 
>   In-Reply-To: 
>
> which I think would count as sufficient notice of the address being
> used.

> Jeff King <peff@peff.net> writes:
> 
> > That is a good question. That confirmation step does come after they
> > have typed their cover letter. However, if they are using --compose,
> > they are dumped in their editor with something like:
> >
> >   From Jeff King <peff@peff.net> # This line is ignored.
> >   GIT: Lines beginning in "GIT:" will be removed.
> >   GIT: Consider including an overall diffstat or table of contents
> >   GIT: for the patch you are writing.
> >   GIT:
> >   GIT: Clear the body content if you don't wish to send a summary.
> >   From: Jeff King <peff@peff.net>
> >   Subject: 
> >   In-Reply-To: 
> >
> > which I think would count as sufficient notice of the address being
> > used.
> 
> OK.  Tentatively I replaced your old series with these 8 patches
> including the last one, as I tend to agree with the value the
> earlier clean-up in the series gives us in the longer term.  As you
> and Felipe discussed, we may want to replace the last one with a
> simpler "don't bother asking" patch, but I think that is more or
> less an orthogonal issue.

> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > That is a good question. That confirmation step does come after they
>> > have typed their cover letter. However, if they are using --compose,
>> > they are dumped in their editor with something like:
>> >
>> >   From Jeff King <peff@peff.net> # This line is ignored.
>> >   GIT: Lines beginning in "GIT:" will be removed.
>> >   GIT: Consider including an overall diffstat or table of contents
>> >   GIT: for the patch you are writing.
>> >   GIT:
>> >   GIT: Clear the body content if you don't wish to send a summary.
>> >   From: Jeff King <peff@peff.net>
>> >   Subject:
>> >   In-Reply-To:
>> >
>> > which I think would count as sufficient notice of the address being
>> > used.
>>
>> OK.  Tentatively I replaced your old series with these 8 patches
>> including the last one, as I tend to agree with the value the
>> earlier clean-up in the series gives us in the longer term.  As you
>> and Felipe discussed, we may want to replace the last one with a
>> simpler "don't bother asking" patch, but I think that is more or
>> less an orthogonal issue.
>
> I'm not sure how orthogonal it is. The latter half of my series is about
> exposing the user_ident_sufficiently_given() flag. If we go with
> Felipe's patch, then that exposed information has no users, and it may
> not be worth it (OTOH, it's possible that some third-party script may
> want it).

> > I'm not sure how orthogonal it is. The latter half of my series is about
> > exposing the user_ident_sufficiently_given() flag. If we go with
> > Felipe's patch, then that exposed information has no users, and it may
> > not be worth it (OTOH, it's possible that some third-party script may
> > want it).
> 
> Well, who is using user_ident_sufficiently_given() in the first place?
> I think 'git commit' might be suffering from the same problem that
> prompted you to split it.

> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:
> ...
>> OK.  Tentatively I replaced your old series with these 8 patches
>> including the last one, as I tend to agree with the value the
>> earlier clean-up in the series gives us in the longer term.  As you
>> and Felipe discussed, we may want to replace the last one with a
>> simpler "don't bother asking" patch, but I think that is more or
>> less an orthogonal issue.
>
> I'm not sure how orthogonal it is. The latter half of my series is about
> exposing the user_ident_sufficiently_given() flag. If we go with
> Felipe's patch, then that exposed information has no users, and it may
> not be worth it (OTOH, it's possible that some third-party script may
> want it).

> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '
>  
>  test_expect_success $PREREQ 'Prompting works' '
>  	clean_fake_sendmail &&
> -	(echo "Example <from@example.com>"
> -	 echo "to@example.com"
> +	(echo "to@example.com"
>  	 echo ""
>  	) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
>  		--smtp-server="$(pwd)/fake.sendmail" \
>  		$patches \
>  		2>errors &&
> +		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
> +		grep "^To: to@example.com\$" msgtxt1
> +'

> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '
> +	clean_fake_sendmail &&
> +	(echo "Example <from@example.com>" &&
> +	 echo "to@example.com" &&
> +	 echo ""
> +	) |
> +	(sane_unset GIT_AUTHOR_NAME &&
> +	 sane_unset GIT_AUTHOR_EMAIL &&
> +	 sane_unset GIT_COMMITTER_NAME &&
> +	 sane_unset GIT_COMMITTER_EMAIL &&
> +	 GIT_SEND_EMAIL_NOTTY=1 git send-email \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$patches \
> +		2>errors &&
>  		grep "^From: Example <from@example.com>\$" msgtxt1 &&
>  		grep "^To: to@example.com\$" msgtxt1
> +	)
> +'

> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
> +	clean_fake_sendmail &&
> +	(sane_unset GIT_AUTHOR_NAME &&
> +	 sane_unset GIT_AUTHOR_EMAIL &&
> +	 sane_unset GIT_COMMITTER_NAME &&
> +	 sane_unset GIT_COMMITTER_EMAIL &&
> +	 GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
> +	 test_must_fail git send-email \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$patches </dev/null 2>errors.out &&
> +		test_i18ngrep "tell me who you are" errors.out
> +	)
>  '

> On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:
> ...
>> 5) GIT_COMMITTER
>> 
>> Who should the emails appear to be from? [Felipe Contreras 2nd
>> <felipe.contreras+2@gmail.com>]
>> 
>> Whoa, what happened there?
>> 
>> Well:
>> 
>>   $sender = $repoauthor || $repocommitter || '';
>>   ($repoauthor) = Git::ident_person(@repo, 'author');
>>   % ./git var GIT_AUTHOR_IDENT
>>   Felipe Contreras 2nd <felipe.contreras+2@gmail.com> 1352783223 +0100
>> 
>> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>
> Yeah, I find that somewhat questionable in the current behavior, and I'd
> consider it a bug. Typically we prefer the committer ident when given a
> choice (e.g., for writing reflog entries).

>> 6.1) GIT_AUTHOR without anything else
>> 
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> var GIT_COMMITTER_IDENT: command returned error: 128
>
> Doesn't that seem like a regression? It used to work.

> The explicitness needs to be tied to the specific ident we grabbed.
> Probably adding a "git var GIT_AUTHOR_EXPLICIT" would be enough, or
> alternatively, adding a flag to "git var" to error out rather than
> return a non-explicit ident (this may need to adjust the error
> handling of the "git var" calls from send-email).

> While simultaneously breaking "git commit" for people who are happily
> using the implicit generation. I can see the appeal of doing so; I was
> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.
> But do we have any data on how many people are currently using that
> feature that would be annoyed by it?

> ...
> It may be something I would work on myself in the future, but I have
> other things to work on at the moment, and since you are interested in
> the topic, I thought you would be a good candidate to polish it enough
> to be suitable upstream. But instead I see a lot of push-back on what I
> considered to be a fairly straightforward technical comment on a
> regression.
> ...
> But I feel like I am fighting an uphill battle just to convince you that
> regressions are bad, and that I am having to make the same points
> repeatedly.  That makes me frustrated and less excited about reviewing
> your patches; and when I say "it is not my itch", that is my most polite
> way of saying "If that is going to be your attitude, then I do not feel
> like dealing with you anymore on this topic".

> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
> >
> > Yeah, I find that somewhat questionable in the current behavior, and I'd
> > consider it a bug. Typically we prefer the committer ident when given a
> > choice (e.g., for writing reflog entries).
> 
> Just to make sure I follow the discussion correctly, do you mean
> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
> specified more concretely and we usually use COMMIITTER for this
> kind of thing in the first place but send-email does not in this
> case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
> the implicit logic as a bug in this case---just making sure).

> On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:
>
>> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>> >
>> > Yeah, I find that somewhat questionable in the current behavior, and I'd
>> > consider it a bug. Typically we prefer the committer ident when given a
>> > choice (e.g., for writing reflog entries).
>> 
>> Just to make sure I follow the discussion correctly, do you mean
>> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
>> specified more concretely and we usually use COMMIITTER for this
>> kind of thing in the first place but send-email does not in this
>> case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
>> the implicit logic as a bug in this case---just making sure).
>
> Having discussed more, I think there are two questionable things:
>
>   1. Preferring author over committer
>
>   2. Failing to fall back to committer when author is implicit or bogus
>      (because "git var" dies).
>
> I think (1) may fall into the "this is not how I would do it today, but
> it is not worth a possible regression" category.
>
> I think (2) might be worth fixing, though. Certainly when the author is
> bogus (by IDENT_STRICT rules), which I think was the original intent of
> the "$repoauthor || $repocommitter" code. Probably when the author ident
> is implicit, though that is more hazy to me.

> On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:
>
>> There's no point in asking this over and over if the user already
>> properly configured his/her name and email.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>
>> I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.
>
> It should be defaulting to your regular git ident, and you just have to
> hit enter, right?
>
> I think it's probably reasonable to skip that "enter" in most cases. But
> I'm not sure why we ever asked in the first place. What do people input
> there if they are not taking the default?
>

> Minor nits:
> 
> On Tue, Nov 13, 2012 at 11:53:20AM -0500, Jeff King wrote:
> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
> >  Alternatively, it can take a prepared ident string (e.g. from the commit
> >  object) and just parse it.
> >  
> > +If the C<explicit> option is set to 1, the returned array will contain an
> > +additional boolean specifying whether the ident was configure explicitly by the
> 
> s/configure/configured/

> >  	if (wantarray) {
> > -		return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > +		my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > +		if ($options{explicit} && defined $explicit) {
> > +			push @ret, $explicit if defined $explicit;
> 
> Isn't the test on this line redundant given that defined $explicit is
> already guaranteed by the condition on the previous line?