[PATCH] send-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 <redacted>
---

I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.

 git-send-email.perl | 11 +++++++++++
 1 file changed, 11 insertions(+)

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;
+	}
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';

-- 
1.8.0

Felipe Contreras wrote:

I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.

Use sendemail.from.  The email sender doesn't necessarily have to be the author.

Ram

On Sun, Nov 11, 2012 at 6:12 PM, Ramkumar Ramachandra
[off-list ref] wrote:

Felipe Contreras wrote:

quoted

I got really tired of 'git send-email' always asking me from which address to send mails... that's already configured.

Use sendemail.from.  The email sender doesn't necessarily have to be the author.

And when it's not the author, then sendemail.from would be used.

% git config user.email felipe.contreras@nokia.com
% git send-email master
From: Felipe Contreras <redacted>

% git config user.email felipe.contreras@gmail.com
% git send-email.perl master
From: Felipe Contreras <redacted>

% git send-email --from=foo@bar.com master
From: foo@bar.com

% git config sendemail.from test@example.com
% git send-email master
From: test@example.com

What do you loose with this code? Nothing. What do you gain by asking
the user every time: "Who should the emails appear to be from?", when
the default GIT_AUTHOR_IDENT is already fine? Nothing.

The problem with sendemail.from, is that each time the user needs to
change email address, it has to be done in multiple places:
user.email, and sendemail.from. There's no need for that.

But I screwed the patch, it should be:

+if (!defined $sender) {
+       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;
+               }
+       }
+}

Cheers.

-- 
Felipe Contreras

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 <redacted>
---

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?

Why not use Git::ident_person() here? It saves some code, and would also
respect environment variables. Or better yet...

 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';

Why not just use $repoauthor or $repocommitter, as the prompt default
already does?

-Peff

On Tue, Nov 13, 2012 at 12:35 AM, Jeff King [off-list ref] wrote:

On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:

quoted

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 <redacted>
---

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?

Yes.

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?

Beats me.

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.

quoted

 my $prompting = 0;
 if (!defined $sender) {
      $sender = $repoauthor || $repocommitter || '';

Why not just use $repoauthor or $repocommitter, as the prompt default
already does?

See above.

Cheers.

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 12:42:02AM +0100, Felipe Contreras wrote:

quoted

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 },
 };

-Peff

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. But maybe people who do one-off
format-patches would rather type their name in a prompt than set an
environment variable and re-run the program.

-Peff

On Tue, Nov 13, 2012 at 1:02 AM, Jeff King [off-list ref] wrote:

On Tue, Nov 13, 2012 at 12:42:02AM +0100, Felipe Contreras wrote:

quoted

quoted

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.

And that's why a lot of commits end up like michael
<michael@michael-laptop.(none)>.

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.

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:

quoted

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.

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).

-Peff

On Mon, Nov 12, 2012 at 10:27:27PM -0500, Jeff King wrote:

On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:

quoted

quoted

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.

Re-reading this, I think "them" at the end of the second paragraph is
slightly unclear. Let me rephrase.

The lack of a name or the presence of an obviously bogus email address
(e.g., with "(none)") is disallowed by commit. We still allow implicit
idents on commit as long they are not obviously wrong, but show them to
the user so that they can notice and correct via "commit --amend".

So if it dies on an implicit ident, send-email would actually be taking
an even stronger stance against bogus idents. Which makes sense, since
there is no "--amend" for fixing a broken email that has been sent.

-Peff

On Tue, Nov 13, 2012 at 4:27 AM, Jeff King [off-list ref] wrote:

On Tue, Nov 13, 2012 at 01:54:59AM +0100, Felipe Contreras wrote:

quoted

quoted

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.

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 [off-list ref]

quoted

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).

I think they can survive. If anybody like this exists.

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 04:55:25AM +0100, Felipe Contreras wrote:

quoted

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. But as I
said already, "git var" uses IDENT_STRICT and will not allow such broken
names.

quoted

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 [off-list ref]

Did you read my email? I explicitly proposed that we would _not_ allow
send-email to use implicit email addresses constructed in that way.

quoted

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.

I tried to help you by pointing you in the right direction and even
providing a sample "git var" patch. But it is not my itch to scratch.

-Peff

On Tue, Nov 13, 2012 at 5:01 AM, Jeff King [off-list ref] wrote:

On Tue, Nov 13, 2012 at 04:55:25AM +0100, Felipe Contreras wrote:

quoted

quoted

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.

quoted

quoted

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 [off-list ref]

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.

quoted

quoted

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.

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
[off-list ref]]

That's right, ident doesn't fail, and that's not the mail address I
specified, it's *implicit*.

3) Full Name + EMAIL

Who should the emails appear to be from? [Felipe Contreras
[off-list ref]]

4) config user

Who should the emails appear to be from? [Felipe Contreras 2nd
[off-list ref]]

5) GIT_COMMITTER

Who should the emails appear to be from? [Felipe Contreras 2nd
[off-list ref]]

Whoa, what happened there?

Well:

  $sender = $repoauthor || $repocommitter || '';
  ($repoauthor) = Git::ident_person(@repo, 'author');
  % ./git var GIT_AUTHOR_IDENT
  Felipe Contreras 2nd [off-list ref] 1352783223 +0100

That's right, AUTHOR_IDENT would fall back to the default email and full name.

Hmm, I wonder...

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

Why? Because:

% PERL5LIB=~/dev/git/perl perl -e 'use Git; printf("%s\n",
Git::ident_person(@repo, 'author'));'
fatal: empty ident name (for <felipec@nysa.(none)>) not allowed

($repoauthor) = Git::ident_person(@repo, 'author');
($repocommitter) = Git::ident_person(@repo, 'committer');

So $repoauthor || $repocommiter is pointless.

6) GIT_AUTHOR

Who should the emails appear to be from? [Felipe Contreras 4th
[off-list ref]]

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

4) config user

From: Felipe Contreras 2nd <redacted>

5) GIT_COMMITTER

From: Felipe Contreras 2nd <redacted>

6) GIT_AUTHOR

From: Felipe Contreras 2nd <redacted>

And what about your proposed change?

2) Full Name + full hostname

./git var GIT_EXPLICIT_IDENT
0

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.

And then:

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 <redacted>

It is explicit, yeah, but 'git send-email' would not be picking the
committer, it would pick the author.

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 the right approach is more along these lines:

--- 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;
 if (!@initial_to && !defined $to_cmd) {
        my $to = ask("Who should the emails be sent to (if any)? ",
                     default => "",

diff --git a/ident.c b/ident.c
index a4bf206..c73ba82 100644
--- 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");
        }

        if (want_date) {

With that we get:

2) Full Name + full hostname

fatal: no explicit email address

3) Full Name + EMAIL

From: Felipe Contreras <redacted>

4) config user

From: Felipe Contreras 2nd <redacted>

5) GIT_COMMITTER

From: Felipe Contreras 2nd <redacted>

(as buggy as before)

6) GIT_AUTHOR

From: Felipe Contreras 4th <redacted>

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
[off-list ref]' ever again.

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).

Cheers.

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 7:42 AM, Felipe Contreras
[off-list ref] wrote:

6) GIT_AUTHOR

Who should the emails appear to be from? [Felipe Contreras 4th
[off-list ref]]

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

This was supposed to be above (before my change).

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:

quoted

quoted

quoted

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.

There was a bug that caused the check to fail in some cases. I fixed it
in f20f387 (this July).

But I still don't see how that has anything to do with what send-email
does or should do. That is why I said "strawman" above. You seem to
think I am saying that send-email should use the system that generated
those broken names, when I am saying the opposite.

quoted

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.

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.

quoted

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.

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.

I am tempted to fault myself for not communicating well, but I feel like
I have made that point at least 3 times in this conversation now. Is
that the source of the confusion?

quoted

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.

And you will survive if upstream git (whether it is me today or Junio
tomorrow) does not pick up your patch. I remember writing you a long
email recently about how one of the responsibilities of the maintainer
is to balance features versus regressions. I'll not bother repeating
myself here.

As for whether they exist, what data do you have? Are you aware that the
test suite, for example, relies on setting GIT_AUTHOR_NAME but not
having any user.* config? 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?

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
[off-list ref]]

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
[off-list ref]]

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.

4) config user

Who should the emails appear to be from? [Felipe Contreras 2nd
[off-list ref]]

OK.

5) GIT_COMMITTER

Who should the emails appear to be from? [Felipe Contreras 2nd
[off-list ref]]

Whoa, what happened there?

Well:

  $sender = $repoauthor || $repocommitter || '';
  ($repoauthor) = Git::ident_person(@repo, 'author');
  % ./git var GIT_AUTHOR_IDENT
  Felipe Contreras 2nd [off-list ref] 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.

So $repoauthor || $repocommiter is pointless.

Agreed.

6) GIT_AUTHOR

Who should the emails appear to be from? [Felipe Contreras 4th
[off-list ref]]

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 <redacted>

OK.

5) GIT_COMMITTER

From: Felipe Contreras 2nd <redacted>

OK.

6) GIT_AUTHOR

From: Felipe Contreras 2nd <redacted>

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 <redacted>

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).

quoted

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...

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
[off-list ref]' 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?

quoted

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".

-Peff

On Tue, Nov 13, 2012 at 8:47 AM, Jeff King [off-list ref] wrote:

But I still don't see how that has anything to do with what send-email
does or should do. That is why I said "strawman" above. You seem to
think I am saying that send-email should use the system that generated
those broken names, when I am saying the opposite.

No, I'm saying none should use that system, and that in fact 'git
commit' should be stricter... both should be stricter.

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.

quoted

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.

That sentence was *not* about 'git send-email', it was about git in
general, and 'git commit' is perfectly happy with implicit generation
of the email from the username and hostname.

I don't thin 'git commit' should do that, and I don't think 'git
send-email' should do that. I'm criticizing the whole approach.

I am tempted to fault myself for not communicating well, but I feel like
I have made that point at least 3 times in this conversation now. Is
that the source of the confusion?

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.

And you will survive if upstream git (whether it is me today or Junio
tomorrow) does not pick up your patch.

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.

I remember writing you a long
email recently about how one of the responsibilities of the maintainer
is to balance features versus regressions. I'll not bother repeating
myself here.

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.

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
[off-list ref]' 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?

I prefer to concentrate on real issues, but that's just me.

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".

But that's not going to happen. And in the unlikely event that it
does, it's not going to be a major issue.

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?

quoted

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).

Yes, that's point 2).

quoted

2) Full Name + full hostname

Who should the emails appear to be from? [Felipe Contreras
[off-list ref]]

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.

Correct.

quoted

3) Full Name + EMAIL

Who should the emails appear to be from? [Felipe Contreras
[off-list ref]]

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.

Agreed.

quoted

5) GIT_COMMITTER

Who should the emails appear to be from? [Felipe Contreras 2nd
[off-list ref]]

Whoa, what happened there?

Well:

  $sender = $repoauthor || $repocommitter || '';
  ($repoauthor) = Git::ident_person(@repo, 'author');
  % ./git var GIT_AUTHOR_IDENT
  Felipe Contreras 2nd [off-list ref] 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).

Yeah, but clearly the intention of the code was to use the committer
if the author wasn't available, which is the case here.

quoted

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.

No, this is a different bug.

The bug above 5) is here:

$sender = $repoauthor || $repocommitter || '';

$repoauthor will always evaluate to true.

This one 5.1) is there:

($repoauthor) = Git::ident_person(@repo, 'author');
($repocommitter) = Git::ident_person(@repo, 'committer'); <-

quoted

So $repoauthor || $repocommiter is pointless.

Agreed.

Good.

quoted

6) GIT_AUTHOR

Who should the emails appear to be from? [Felipe Contreras 4th
[off-list ref]]

Right, that's what I'd expect.

You mean without the input question?

quoted

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):

($repoauthor) = Git::ident_person(@repo, 'author'); <- here
($repocommitter) = Git::ident_person(@repo, 'committer');

quoted

4) config user

From: Felipe Contreras 2nd <redacted>

OK.

quoted

5) GIT_COMMITTER

From: Felipe Contreras 2nd <redacted>

OK.

quoted

6) GIT_AUTHOR

From: Felipe Contreras 2nd <redacted>

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.

Yes, it is a regression (that won't affect anybody).

quoted

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().

quoted

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.

Indeed.

quoted

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 <redacted>

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 think strictess should be tied to explicitness, and 'git var' should
error out, and not die.

quoted

quoted

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 that is orthogonal to what I'm trying accomplish.

quoted

I think the right approach is more along these lines:

I think that is moving in the right direction, but...

quoted

--- 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.

Yes, that's exactly what I would argue.

quoted

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
[off-list ref]' 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?

No, but it can also be considered a bug... do we have any data on how
many people are being affected by this? If the '(none)' commits are
any indication of it, probably a lot. At least the ones that do have a
fqdn.

quoted

quoted

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.

And I disagree, I think it hurts nobody, and I think it's 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.

I'm just trying to be pragmatic. I don't see the point in wasting my
time for people that don't exist. As I said, I don't think anybody
would be hit by this.

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".

Fixing a regression that nobody would notice is not my itch either,
yet the patch I sent above does it, and it even fixes 'git commit'
(IMO). But it's also not good enough.

I scratched my itch with the original patch, anything after that is to
help other people.

I think it would be much easier to just remove the question input. The
only "regression" would be the people that have a fqdn and full name
_and_ expect the question. But Junio suggested to just die in those
cases, and trying to send an email that would probably fail is not
that different.

Fixing all the var and ident infrastructure seems way, *way* far from
what I intended to do.

Cheers.

-- 
Felipe Contreras

On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:

quoted

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.

What is "it"?

If it is bad guesses like "user@host.(none)" on unconfigured systems,
then I did not suggest it doesn't happen. I said that it happened with
older versions of git, but that it has now been fixed. Of course it will
continue to happen for people on older versions of git until they
upgrade. I cannot go back in time and fix released versions.

If it is guessing at all to end up with "user@host.domain" for a host
that does not receive mail, yes, that happens, and I have been very
clear that it does. The safety valve is showing the ident and a warning
to the user during the commit process.

I do not see any point in discussing the former when considering future
changes to git. It is already disallowed by current versions of git, and
we are just waiting for the whole world to upgrade to a fixed version.

I can see the argument for tightening the check to disallow the latter.
But it would also hurt real people who are relying on the feature.
Perhaps it would make sense to adopt the implicit_ident_advice in other
code paths besides "git commit" (e.g., via "git var).

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. Instead we must use our judgement about what is being changed,
and we tend to err on the side of keeping the status quo, since that is
what the silent majority is busy _not_ complaining about.

We use the same judgement on the other side, too. Right now your
evidence is "1 user wants this, 0 users do not". But we can guess that
there are other people who would like the intent of your patch, but did
not care enough or are not active enough on the list to write the patch
themselves or comment on this thread.

It is also very easy to me accept the status quo when it is not in
fundamental conflict with the proposed improvement, but simply a matter
of implementation (which it is the case for send-email stopping
prompting in some cases, though it is not for changing the behavior of
git-commit).

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:

quoted

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
[off-list ref]' 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.

quoted

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. In the absence of data,
the sane thing is not to break existing users. The burden is on you to
argue that there is no such breakage.

quoted

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).

quoted

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, ship a new version, and then have the user upgrade their
git. Why not just not introduce the bug in the first place?

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).

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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?

quoted

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.

quoted

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

Fixing a regression that nobody would notice is not my itch either,

Sorry, but it is part of your itch. The git project is at state A. With
your patch, we are at a regressed state B. 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.

-Peff

There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.

Signed-off-by: Jeff King <redacted>
---
 cache.h | 4 ----
 ident.c | 6 +++++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- 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);
 
 extern const char *git_commit_encoding;

diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c

@@ -10,7 +10,11 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"

-- 
1.8.0.207.gdf2154c

We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:

  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.

Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.

Signed-off-by: Jeff King <redacted>
---
Case 1 made me initially think might need to check
author_ident_sufficiently_given when deciding whether to show the
"Author:" line during commit.  But I don't think it is necessary, as we
already check !strcmp(author, committer); if the committer is explicit
and the author is identical, even if it is implicit, there is no point
in telling the user.

 builtin/commit.c |  4 ++--
 cache.h          |  3 ++-
 ident.c          | 32 +++++++++++++++++++++++++-------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				ident_shown++ ? "" : "\n",
 				author_ident->buf);
 
-		if (!user_ident_sufficiently_given())
+		if (!committer_ident_sufficiently_given())
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Committer: %s"),

@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_sufficiently_given()) {
+	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {

diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h

@@ -1149,7 +1149,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;

diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c

@@ -14,7 +14,8 @@ static char git_default_date[50];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"

@@ -113,7 +114,8 @@ const char *ident_default_email(void)
 
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
-			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
 			copy_email(xgetpwuid_self(), &git_default_email);
 		strbuf_trim(&git_default_email);

@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
 
 const char *git_author_info(int flag)
 {
+	if (getenv("GIT_AUTHOR_NAME"))
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+	if (getenv("GIT_AUTHOR_EMAIL"))
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),

@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
 const char *git_committer_info(int flag)
 {
 	if (getenv("GIT_COMMITTER_NAME"))
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
 
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
 {
 #ifndef WINDOWS
 	return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);

@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
 #endif
 }
 
+int committer_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(author_ident_explicitly_given);
+}
+
 int git_ident_config(const char *var, const char *value, void *data)
 {
 	if (!strcmp(var, "user.name")) {

@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_name);
 		strbuf_addstr(&git_default_name, value);
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
 

@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_email);
 		strbuf_addstr(&git_default_email, value);
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
 

-- 
1.8.0.207.gdf2154c

Git-var currently only accepts a single value to print. This
is inefficient if the caller is interested in finding
multiple values, as they must invoke git-var multiple times.

This patch lets callers specify multiple variables, and
prints one per line.

Signed-off-by: Jeff King <redacted>
---
This will later let us get the "explicit" flag for free.

 Documentation/git-var.txt |  9 +++++++--
 builtin/var.c             | 13 +++++++------
 t/t0007-git-var.sh        | 29 +++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100755 t/t0007-git-var.sh

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 67edf58..53abba5 100644
--- 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 (e.g.,
+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output
+when requesting multiple variables. This can be mitigated by putting any
+such variables at the end of the list.
 
 OPTIONS
 -------

diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..49b48f5 100644
--- 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) {

@@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 	git_config(git_default_config, NULL);
-	val = read_var(argv[1]);
-	if (!val)
-		usage(var_usage);
 
-	printf("%s\n", val);
+	while (*++argv) {
+		const char *val = read_var(*argv);
+		if (!val)
+			usage(var_usage);
+		printf("%s\n", val);
+	}
 
 	return 0;
 }

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 0000000..45a5f66
--- /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 &&
+	git var GIT_AUTHOR_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+	test_tick &&
+	echo "C O Mitter <committer@example.com> 1112912053 -0700" >expect &&
+	git var GIT_COMMITTER_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git var can show multiple values' '
+	cat >expect <<-\EOF &&
+	A U Thor <author@example.com> 1112912053 -0700
+	C O Mitter <committer@example.com> 1112912053 -0700
+	EOF
+	git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.8.0.207.gdf2154c

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 without re-implementing the ident logic
themselves. Let's provide a way for them to do so.

Signed-off-by: Jeff King <redacted>
---
 Documentation/git-var.txt |  8 ++++++++
 builtin/var.c             | 27 +++++++++++++++++++++++++++
 t/t0007-git-var.sh        | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..963b8d4 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt

@@ -39,9 +39,17 @@ VARIABLES
 GIT_AUTHOR_IDENT::
     The author of a piece of code.
 
+GIT_AUTHOR_EXPLICIT::
+    Outputs "1" if the author identity was provided explicitly by the
+    user (in the config or environment), or "0" if it was determined
+    implicitly from the system.
+
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_COMMITTER_EXPLICIT::
+    Like GIT_AUTHOR_EXPLICIT, but for the committer ident.
+
 GIT_EDITOR::
     Text editor for use by git commands.  The value is meant to be
     interpreted by the shell when it is used.  Examples: `~/bin/vi`,

diff --git a/builtin/var.c b/builtin/var.c
index 49b48f5..ce28101 100644
--- a/builtin/var.c
+++ b/builtin/var.c

@@ -26,13 +26,40 @@ static const char *pager(int flag)
 	return pgm;
 }
 
+static const char *explicit_ident(const char * (*get_ident)(int),
+				  int (*check_ident)(void))
+{
+	/*
+	 * Prime the "explicit" flag by getting the identity.
+	 * We do not want to be strict here, because we would not want
+	 * to die on bogus implicit values, but instead just report that they
+	 * are not explicit.
+	 */
+	get_ident(0);
+	return check_ident() ? "1" : "0";
+}
+
+static const char *committer_explicit(int flag)
+{
+	return explicit_ident(git_committer_info,
+			      committer_ident_sufficiently_given);
+}
+
+static const char *author_explicit(int flag)
+{
+	return explicit_ident(git_author_info,
+			      author_ident_sufficiently_given);
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
 };
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
+	{ "GIT_COMMITTER_EXPLICIT", committer_explicit },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_AUTHOR_EXPLICIT", author_explicit },
 	{ "GIT_EDITOR", editor },
 	{ "GIT_PAGER", pager },
 	{ "", NULL },

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 45a5f66..66b9810 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh

@@ -26,4 +26,40 @@ test_expect_success 'git var can show multiple values' '
 	test_cmp expect actual
 '
 
+for type in AUTHOR COMMITTER; do
+	test_expect_success "$type ident can detect implicit values" "
+		echo 0 >expect &&
+		(
+			sane_unset GIT_${type}_NAME &&
+			sane_unset GIT_${type}_EMAIL &&
+			sane_unset EMAIL &&
+			git var GIT_${type}_EXPLICIT >actual
+		) &&
+		test_cmp expect actual
+	"
+
+	test_expect_success "$type ident is explicit via environment" "
+		echo 1 >expect &&
+		(
+			GIT_${type}_NAME='A Name' &&
+			export GIT_${type}_NAME &&
+			GIT_${type}_EMAIL='name@example.com' &&
+			export GIT_${type}_EMAIL &&
+			git var GIT_${type}_EXPLICIT >actual
+		) &&
+		test_cmp expect actual
+	"
+
+	test_expect_success "$type ident is explicit via config" "
+		echo 1 >expect &&
+		test_config user.name 'A Name' &&
+		test_config user.email 'name@example.com' &&
+		(
+			sane_unset GIT_${type}_NAME &&
+			sane_unset GIT_${type}_EMAIL &&
+			git var GIT_${type}_EXPLICIT >actual
+		)
+	"
+done
+
 test_done

-- 
1.8.0.207.gdf2154c

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.

Now that Git.pm will tell us whether the ident is implicit or
explicit, we can stop prompting in the explicit case, saving
most users from having to see the prompt at all.

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; if we can,
send-email will continue but prompt).

Signed-off-by: Jeff King <redacted>
---
 git-send-email.perl   | 22 +++++++++++++---------
 t/t9001-send-email.sh |  5 ++---
 2 files changed, 15 insertions(+), 12 deletions(-)

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) {

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6c6af7d..c5d66cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh

@@ -191,14 +191,13 @@ 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: Example <from@example.com>\$" msgtxt1 &&
+		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
 		grep "^To: to@example.com\$" msgtxt1
 '
 

-- 
1.8.0.207.gdf2154c

"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.

Because evaluating "ident" in an array versus scalar context
already has a meaning, we cannot return our extra value in a
backwards compatible way. Instead, we require the caller to
add an extra "explicit" flag to request the information.
The ident_person function, on the other hand, always returns
a scalar, so we are free to overload it in an array context.

Signed-off-by: Jeff King <redacted>
---
 perl/Git.pm | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..1994ec1 100644
--- 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.
+
 C<ident_person> returns the person part of the ident - name and email;
 it can take the same arguments as C<ident> or the array returned by C<ident>.
 

@@ -763,17 +767,22 @@ The synopsis is like:
 =cut
 
 sub ident {
-	my ($self, $type) = _maybe_self(@_);
-	my $identstr;
+	my ($self, $type, %options) = _maybe_self(@_);
+	my ($identstr, $explicit);
 	if (lc $type eq lc 'committer' or lc $type eq lc 'author') {
-		my @cmd = ('var', 'GIT_'.uc($type).'_IDENT');
+		my $uc = uc($type);
+		my @cmd = ('var', "GIT_${uc}_IDENT", "GIT_${uc}_EXPLICIT");
 		unshift @cmd, $self if $self;
-		$identstr = command_oneline(@cmd);
+		($identstr, $explicit) = command(@cmd);
 	} else {
 		$identstr = $type;
 	}
 	if (wantarray) {
-		return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+		my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+		if ($options{explicit} && defined $explicit) {
+			push @ret, $explicit if defined $explicit;
+		}
+		return @ret;
 	} else {
 		return $identstr;
 	}

@@ -781,8 +790,11 @@ sub ident {
 
 sub ident_person {
 	my ($self, @ident) = _maybe_self(@_);
-	$#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);
-	return "$ident[0] <$ident[1]>";
+	$#ident == 0 and @ident = $self ?
+				  $self->ident($ident[0], explicit => 1) :
+				  ident($ident[0], explicit => 1);
+	my $ret = "$ident[0] <$ident[1]>";
+	return wantarray ? ($ret, @ident[3]) : $ret;
 }
 
 

-- 
1.8.0.207.gdf2154c

On Tue, Nov 13, 2012 at 12:35 AM, Jeff King [off-list ref] wrote:

On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:

quoted

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 <redacted>
---

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?

I usually input "Linus Torvalds [off-list ref]", to
give my patch high priority ;-)

On Tue, Nov 13, 2012 at 5:48 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:

quoted

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".

Miscomunication then. When you mentioned 'it has always been the case
that you can use git without setting
user.*', I directed my comments at git in general, not git send-email.

quoted

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 we cannot take it as evidence that these users do exist either.

The absence of evidence simply means that... we don't have evidence.

Instead we must use our judgement about what is being changed,
and we tend to err on the side of keeping the status quo, since that is
what the silent majority is busy _not_ complaining about.

Yes, the keyword is *tend*; this wouldn't be first or the last time
that a behavior change happens.

We have to be careful about these changes, but we do them.

We use the same judgement on the other side, too. Right now your
evidence is "1 user wants this, 0 users do not".

1 is infinitely greater than 0.

But we can guess that
there are other people who would like the intent of your patch, but did
not care enough or are not active enough on the list to write the patch
themselves or comment on this thread.

Yes, that would be an educated guess. IMO the fact that people use
GIT_AUTHOR_ variables to send mail is not. There's many ways to
achieve that: sendemail.from, --from, user configuration, $EMAIL,
--compose and From:, etc. each and every one of them much more likely
to be used than GIT_AUTHOR_.

But I'm tired of arguing how extremely unlikely it is that such people
don't exist. Lets agree to disagree. Either way it doesn't matter,
because nobody is proposing a patch that would affect these
hypothetical users.

quoted

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?

measure != scientific measurement.

I used common sense, because that's the only tool available.

GIT_AUTHOR is plumbing, not very well known, it's cumbersome (you need
to export two variables), it can be easily confused with
GIT_COMMITTER, which wouldn't work on this case. And there's plenty of
tools that much simpler to use, starting with 'git send-email --from',
which is so user friendly it's in the --help. There's absolutely no
reason why anybody would want to use GIT_AUTHOR.

But lets agree to disagree.

quoted

But listen closely to what you said:

quoted

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
[off-list ref]' 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.

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.

quoted

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.

No, I was proposing my second patch. That's why I sent a second patch.

But forget it. I don't think it's worth trying to do the right thing
here (make both 'git commit' and 'git send-email' be stricter). I do
think that would be simpler than all the required changes to var, and
better, but that's not my itch atm.

quoted

quoted

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.

This has absolutely nothing to with religion, this is rationality. If
you don't want to make a distinction between the different levels at
which we _know_ something, suit yourself.

In the absence of data,
the sane thing is not to break existing users.

What you decide to do has absolutely nothing to do with what is true.
These hypothetical users exist, or they don't. I don't have data for
their existence, you don't have data for their non-existence. In the
absence of data the rational response is to don't believe the claim,
and this one is particularly clear because you cannot prove a
negative, so the burden of proof is clearly on your side, as it's
*impossible* for me to prove that there are no users that use certain
feature, and it's very easy for you to do so, as all you need is
*one*.

Presumably you are not interested in fulfilling your burden of proof,
which is fine, you can choose the safest action without proof that
such users exist, but that doesn't change the fact that you don't know
that if they exist or not.

And yet, you continue to say they do exist, and try to win the
argument by means of rhetoric: 'not to break existing users'; they are
not *existing* users, they are hypothetical.

The burden is on you to
argue that there is no such breakage.

That's impossible to prove. *Nobody* can prove that people are not
using even the most obscure feature of git.

All you do is make educated guesses, and try to minimize the damage.

quoted

quoted

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).

Exactly, but I was not the one that brought the test suite. My patch
is not a problem for our current tests. So this hypothetical user
remains evasive.

quoted

quoted

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,

*If* that happens, which in all likelihood it won't.

ship a new version, and then have the user upgrade their
git. Why not just not introduce the bug in the first place?

Because we are spending more time arguing about something that most
likely won't happen.

There's a chance the next time you sit the chair will break because of
a bad leg, but you don't engage in philosophical discussions each time
you are going to sit, you go ahead and sit down, and take your
chances, and 99.9% of the time you would be fine.

quoted

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).

Times 0.01 (at best), and you get barely nothing.

quoted

quoted

quoted

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.

Indeed. And that's something I won't worry about, because I honestly
believe nobody is using neither GIT_COMMITTER, nor GIT_AUTHOR, and the
fact that nobody has found this breakage only increases my certainty.

quoted

quoted

quoted

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.

Yes.

quoted

quoted

quoted

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?

No. I tried the idea, it didn't work.

I saw your patches about
{committer,author}_ident_sufficiently_given(), those might work, but I
don't think they are _needed_. They might be nice though.

quoted

quoted

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.

No, it doesn't.

Why do you hold on to my first patch? The second patch doesn't have
this issue. It does change the behavior of 'git commit', yeah, but I
think that's a benefit.

You might disagree, but there's no point in going back to discuss the
first patch.

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.

quoted

quoted

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

I think this adds a lot of code that nobody would use.

The prompt will happen only if:

1) No configured user.name/user.email
2) No specified $EMAIL
3) No configured sendemail.from
4) No specified --from argument
5) A fully qualified domain name
6) A full name in the geckos field

Very, very unlikely.

And if:

* A sendmail configuration doesn't allow sending from this domain name
* The user sees the problem in the confirmation part

Then the user would see a problem anyway, so the prompt is not adding
that much value.

quoted

Fixing a regression that nobody would notice is not my itch either,

Sorry, but it is part of your itch. The git project is at state A. With
your patch, we are at a regressed state B.

And that's fine.

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.

No it's not. It's a conflict of interests. It's only linked as long as
people care about the "regression" in state B. If they realize this
"regression" is irrelevant, then there wouldn't be any problem. This,
is now clear is not going to happen. But I don't think your opinions
define whether or not I have an itch.

quoted

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, 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 sent another patch series that is very simple[1]. This deals with
the hypothetical GIT_AUTHOR users, but might hit another set of
hypothetical users, but the damage would be very small, and the
possibility of these users existing very low. IOW; very low risk.

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. That's fine, it's your opinion,
I respectfully disagree. Personally, I think the sweet spot is between
the two, far from GNOME, but not quite to the point where no
regressions happen ever. When there's a good chance that very few
people will be hit, and the drawback is small, then it's OK to
introduce regressions. Always keeping in mind that these risk
assessments might be wrong, and there might be more people hit than
expected and/or the problems were greater, but if/when that happens,
*then* we simply deal with it.

Cheers.

[1] http://mid.gmane.org/1352834364-2674-1-git-send-email-felipe.contreras@gmail.com

-- 
Felipe Contreras

Jeff King wrote:

There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.

[...]

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.

This patch finally does that, which is a nice way to make cache.h
easier to read and change less often.

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Jeff King wrote:

This patch lets callers specify multiple variables, and
prints one per line.

Yay!

[...]

Maybe a -z option to NUL-terminate values would be useful some day.

What should happen if I pass "-l" followed by other arguments?

[...]

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"
	}
	...

		echo "A U Thor [off-list ref] $timestamp" >expect &&
		git var GIT_AUTHOR_IDENT >actual &&
		test_cmp_filtered expect actual

should make reordering tests a lot easier, though it has the downside
of not being able to catch a weird bug that would make the timestamp
out of sync with reality.

Hope that helps,
Jonathan

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.

On most machines I have the EMAIL envvar set explicitly, but in the
recent past I relied on /etc/mailname on some others, so I'm also
genuinely curious about the use case here (and too lazy to dig it up).

Thanks,
Jonathan

Jeff King wrote:

"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.

Sounds sensible.  Quick nits:

[...]

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?

Thanks,
Jonathan

Jeff King wrote:

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.

I haven't read the code carefully, but this behavior sounds sensible,
so for what it's worth,
Acked-by: Jonathan Nieder <redacted>

[...]

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.

Thanks,
Jonathan

On Wed, Nov 14, 2012 at 08:44:57AM -0800, Jonathan Nieder wrote:

quoted

-#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.

Thanks for digging up the history. I remembered trying to do this before
during the ident refactoring that went into v1.7.11, but it didn't work
then for some reason (I think it was probably an earlier iteration of
the series, and then I forgot to revisit it during the final one).

I'll add your explanation to the commit message if I re-roll.

-Peff

On Wed, Nov 14, 2012 at 09:01:48AM -0800, Jonathan Nieder wrote:

quoted

 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.

Yeah, I thought about that but stopped short. The intended caller in my
series is Git.pm, whose command() splits on newlines. Although it is
perl...I suspect doing:

  local $/ = "\0";
  my @entries = command(...);

would work. For ident variables, we know they don't contain a newline,
though.

quoted

--- 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?

Good catch. Probably we should just call usage() once we see "-l"
and (argc > 2), which matches the previous behavior. I don't see much
point in listing specific variables after having listed them all.

I was also tempted to convert to parse_options, but I don't think that
really buys us anything (we could detect the option in "git var foo -l
bar", but since we are not going to do anything useful in such a case,
there is not much point).

quoted

+	test_tick &&
+	echo "A U Thor [off-list ref] 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"
	}

No, we don't have to. I was just hoping to keep the tests simple by not
doing any parsing trickery. The test_tick keeps it stable, but as you
note, it is not robust to reordering. I think it would be sufficient to
just put $GIT_COMMITTER_DATE into the expected output.

I'll fix both in a re-roll.

Thanks.

-Peff

On Wed, Nov 14, 2012 at 09:06:57AM -0800, Jonathan Nieder wrote:

Jeff King wrote:

quoted

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.

The intent is to make git's internal rules (whatever they may be)
available to calling scripts. But I agree those rules should be spelled
out now that they are becoming a more visible interface. I'll update the
documentation.

On most machines I have the EMAIL envvar set explicitly, but in the
recent past I relied on /etc/mailname on some others, so I'm also
genuinely curious about the use case here (and too lazy to dig it up).

Right now EMAIL is explicit, but `id`@`cat /etc/mailname` is not. I
think changing that would be a separate issue, though (and I'm not sure
whether it is a good idea; my /etc/mailname is not an email address I
would want to use).

-Peff

On Wed, Nov 14, 2012 at 09:12:13AM -0800, Jonathan Nieder wrote:

quoted

--- 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?

Good suggestion. Thanks.

-Peff

On Wed, Nov 14, 2012 at 09:18:27AM -0800, Jonathan Nieder wrote:

quoted

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.

Right. They would already have to to make commits, for example.

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).

-Peff

On Wed, Nov 14, 2012 at 12:05:05PM -0800, Jeff King wrote:

quoted

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).

Actually, I think the simplest thing is to add a prerequisite, like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 489bc80..8d192ff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -738,6 +738,14 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 	esac
 '
 
+test_lazy_prereq IMPLICIT_IDENT '
+	sane_unset GIT_AUTHOR_NAME &&
+	sane_unset GIT_AUTHOR_EMAIL &&
+	git var GIT_AUTHOR_IDENT &&
+	# double check that we were not polluted by config
+	test "$(git var GIT_AUTHOR_EXPLICIT)" = 0
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY

We can't have one test machine that will cover all of the cases, but
given that the test suite is run by many people across many machines, we
will get coverage (and I know that some people have machines which would
not pass that prereq, because I got test failure reports during the last
ident refactoring).

I'll include something like that in my re-roll (and it should let us
test "git commit" more thoroughly, too).

-Peff

On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:

quoted

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.

Why do you hold on to my first patch?

Because clearly I am confused by your behavior. You continued to argue
that the initial regression was not worth worrying about. Which I
thought meant you were still interested in pursuing that patch. But if
you are not, then there is not even any point in discussing it.  Which
is just fine with me, as that discussion seemed to be going nowhere.

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). And as it was in the middle of a discussion about whether
regressions matter, it was not clear to me that your argument was "I
have thought this through and the inconvenience is outweighted by the
benefit" and not "I did not bother to consider the implications for
users who are currently using this feature".

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.

quoted

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.

Yes, I think that is fine _if_ we want to change git-commit. Which is
not clear to me. If we don't, then it is not an option.

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.

quoted

  [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, which IMHO makes the resulting
code cleaner, and it increases the utility of "git var", and our test
coverage. If you have review comments, then by all means, respond to the
series.

quoted

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.

-Peff

On Tue, Nov 13, 2012 at 11:48:45AM -0500, Jeff King wrote:

  [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

Here is a re-roll of the series based on comments from Jonathan (changes
from the first version noted below):

  [1/8]: test-lib: allow negation of prerequisites
  [2/8]: t7502: factor out autoident prerequisite

These two are new, and allow us to use the same prereq from t7502 in
t9001.

  [3/8]: ident: make user_ident_explicitly_given static

Expanded the commit message to explain history of the variable.

  [4/8]: ident: keep separate "explicit" flags for author and committer

Same as before.

  [5/8]: var: accept multiple variables on the command line

 - Expanded tests to include "git var -l"
 - Use $GIT_* variables to create test "expect" files, which should make
   them less brittle.
 - Fixed "git var -l foo" to print usage instead of ignoring foo (i.e.,
   keeping the behavior the same).
 - Update usage message to match new feature.

  [6/8]: var: provide explicit/implicit ident information

Updated documentation to describe what "explicit" means. No functional
changes.

  [7/8]: Git.pm: teach "ident" to query explicitness

Improved Git.pm documentation. No functional changes.

  [8/8]: send-email: do not prompt for explicit repo ident

Test the implicit ident case, both when we have implicit ident and when
we don't. No changes to send-email.

-Peff

You can set and test a prerequisite like this:

  test_set_prereq FOO
  test_have_prereq FOO && echo yes

You can negate the test in the shell like this:

  ! test_have_prereq && echo no

However, when you are using the automatic prerequisite
checking in test_expect_*, there is no opportunity to use
the shell negation.  This patch introduces the syntax "!FOO"
to indicate that the test should only run if a prerequisite
is not meant.

One alternative is to set an explicit negative prerequisite,
like:

  if system_has_foo; then
	  test_set_prereq FOO
  else
	  test_set_prereq NO_FOO
  fi

However, this doesn't work for lazy prerequisites, which
associate a single test with a single name. We could teach
the lazy prereq evaluator to set both forms, but the code
change ends up quite similar to this one (because we still
need to convert NO_FOO into FOO to find the correct lazy
script).

Signed-off-by: Jeff King <redacted>
---
I chose "!" as the negation prefix because it will never conflict with
another prerequisite. But we could also use "NO_" if that's more
aesthetically pleasing. I don't have a strong preference.

 t/t0000-basic.sh        | 32 ++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh | 21 ++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 08677df..562cf41 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -115,6 +115,38 @@ then
 	exit 1
 fi
 
+test_lazy_prereq LAZY_TRUE true
+havetrue=no
+test_expect_success LAZY_TRUE 'test runs if lazy prereq is satisfied' '
+	havetrue=yes
+'
+donthavetrue=yes
+test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
+	donthavetrue=no
+'
+
+if test "$havetrue$donthavetrue" != yesyes
+then
+	say 'bug in test framework: lazy prerequisites do not work'
+	exit 1
+fi
+
+test_lazy_prereq LAZY_FALSE false
+nothavefalse=no
+test_expect_success !LAZY_FALSE 'negative lazy prereqs checked' '
+	nothavefalse=yes
+'
+havefalse=yes
+test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
+	havefalse=no
+'
+
+if test "$nothavefalse$havefalse" != yesyes
+then
+	say 'bug in test framework: negative lazy prerequisites do not work'
+	exit 1
+fi
+
 clean=no
 test_expect_success 'tests clean up after themselves' '
 	test_when_finished clean=yes

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8889ba5..22a4f8f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -275,6 +275,15 @@ test_have_prereq () {
 
 	for prerequisite
 	do
+		case "$prerequisite" in
+		!*)
+			negative_prereq=t
+			prerequisite=${prerequisite#!}
+			;;
+		*)
+			negative_prereq=
+		esac
+
 		case " $lazily_tested_prereq " in
 		*" $prerequisite "*)
 			;;

@@ -294,10 +303,20 @@ test_have_prereq () {
 		total_prereq=$(($total_prereq + 1))
 		case "$satisfied_prereq" in
 		*" $prerequisite "*)
+			satisfied_this_prereq=t
+			;;
+		*)
+			satisfied_this_prereq=
+		esac
+
+		case "$satisfied_this_prereq,$negative_prereq" in
+		t,|,t)
 			ok_prereq=$(($ok_prereq + 1))
 			;;
 		*)
-			# Keep a list of missing prerequisites
+			# Keep a list of missing prerequisites; restore
+			# the negative marker if necessary.
+			prerequisite=${negative_prereq:+!}$prerequisite
 			if test -z "$missing_prereq"
 			then
 				missing_prereq=$prerequisite

-- 
1.8.0.207.gdf2154c

t7502 checks the behavior of commit when we can and cannot
determine a valid committer ident. Let's move that into
test-lib as a lazy prerequisite so other scripts can use it.

Signed-off-by: Jeff King <redacted>
---
 t/t7502-commit.sh | 12 +-----------
 t/test-lib.sh     |  6 ++++++
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..1a5cb69 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -243,16 +243,6 @@ test_expect_success 'message shows author when it is not equal to committer' '
 	  .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'setup auto-ident prerequisite' '
-	if (sane_unset GIT_COMMITTER_EMAIL &&
-	    sane_unset GIT_COMMITTER_NAME &&
-	    git var GIT_COMMITTER_IDENT); then
-		test_set_prereq AUTOIDENT
-	else
-		test_set_prereq NOAUTOIDENT
-	fi
-'
-
 test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
 
 	echo >>negative &&

@@ -271,7 +261,7 @@ echo editor started > "$(pwd)/.git/result"
 exit 0
 EOF
 
-test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '
+test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
 	>.git/result
 	>expect &&
 

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 489bc80..0334a9e 100644
--- 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
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY

-- 
1.8.0.207.gdf2154c

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 <redacted>
Reviewed-by: Jonathan Nieder <redacted>
---
 cache.h | 4 ----
 ident.c | 6 +++++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- 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);
 
 extern const char *git_commit_encoding;

diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c

@@ -10,7 +10,11 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"

-- 
1.8.0.207.gdf2154c

We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:

  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.

Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.

Signed-off-by: Jeff King <redacted>
---
 builtin/commit.c |  4 ++--
 cache.h          |  3 ++-
 ident.c          | 32 +++++++++++++++++++++++++-------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				ident_shown++ ? "" : "\n",
 				author_ident->buf);
 
-		if (!user_ident_sufficiently_given())
+		if (!committer_ident_sufficiently_given())
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Committer: %s"),

@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_sufficiently_given()) {
+	if (!committer_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {

diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h

@@ -1149,7 +1149,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;

diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c

@@ -14,7 +14,8 @@ static char git_default_date[50];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"

@@ -113,7 +114,8 @@ const char *ident_default_email(void)
 
 		if (email && email[0]) {
 			strbuf_addstr(&git_default_email, email);
-			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
 			copy_email(xgetpwuid_self(), &git_default_email);
 		strbuf_trim(&git_default_email);

@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
 
 const char *git_author_info(int flag)
 {
+	if (getenv("GIT_AUTHOR_NAME"))
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+	if (getenv("GIT_AUTHOR_EMAIL"))
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
 			 getenv("GIT_AUTHOR_DATE"),

@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
 const char *git_committer_info(int flag)
 {
 	if (getenv("GIT_COMMITTER_NAME"))
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
 
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
 {
 #ifndef WINDOWS
 	return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);

@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
 #endif
 }
 
+int committer_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+	return ident_is_sufficient(author_ident_explicitly_given);
+}
+
 int git_ident_config(const char *var, const char *value, void *data)
 {
 	if (!strcmp(var, "user.name")) {

@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_name);
 		strbuf_addstr(&git_default_name, value);
-		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
 

@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
 			return config_error_nonbool(var);
 		strbuf_reset(&git_default_email);
 		strbuf_addstr(&git_default_email, value);
-		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}