[PATCH] send-email: add proper default sender

[PATCH] send-email: add proper default sender

[Felipe Contreras]

There's no point in asking this over and over if the user already
properly configured his/her name and email.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

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

 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

Re: [PATCH] send-email: add proper default sender

[Ramkumar Ramachandra]

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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

On Sun, Nov 11, 2012 at 6:12 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

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 <felipe.contreras@nokia.com>

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

% 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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

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

It should be defaulting to your regular git ident, and you just have to
hit enter, right?

I think it's probably reasonable to skip that "enter" in most cases. But
I'm not sure why we ever asked in the first place. What do people input
there if they are not taking the default?

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

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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

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.

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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

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

But we use the environment to default the field, so the distinction
doesn't make much sense to me.  Plus, it has always been the case that
you can use git without setting user.*, but instead only using the
environment. I don't see any reason not to follow that principle here,
too.

The one distinction that would make sense to me is pausing to ask when
we use "implicit" methods to look up the ident, like concatenating the
username with the hostname to get the email.

Git::ident uses "git var" to do its lookup, which will use IDENT_STRICT;
that stops most junk like empty names and bogus domains. But I think we
would want to go one step further and actually check
user_ident_sufficiently_given.  Unfortunately, that is not currently
available outside of C. You'd probably want something like:

diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..eaf324e 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -26,6 +26,12 @@ static const char *pager(int flag)
 	return pgm;
 }
 
+static const char *explicit_ident(int flag)
+{
+	git_committer_info(flag);
+	return user_ident_sufficiently_given() ? "1" : "0";
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -35,6 +41,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
 	{ "GIT_PAGER", pager },
+	{ "GIT_EXPLICIT_IDENT", explicit_ident },
 	{ "", NULL },
 };

-Peff

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

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

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

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

Re: [PATCH] send-email: add proper default sender

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yeah, I agree with pretty much everything you said in this thread
(including that environment and config are equally likely to reflect
user's wish and it does not make much sense to treat environment as
more suspicious).

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

s/one-off format-patches/one-off send-email/.  I think dying will
force them to configure their names once (so that later invocations
do not have to stop) while prompting will force them to type their
names every time, so the current behaviour is probably a false
economy.  As long as we caution users in the release notes, it
probably is OK to change the command to die.

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

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

No, it's not. Those broken names do not come from the environment, but
from our last-resort guess of the hostname. We long ago switched to
printing the name as a warning when we have made such a guess (bb1ae3f),
then more recently started rejecting them outright (8c5b1ae).

And I have proposed exactly the same behavior here: respect the
environment and the config, but do not trust the implicit guesses.

> 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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

That depends how you define environment, but fine, the point is that it happens.

> We long ago switched to
> printing the name as a warning when we have made such a guess (bb1ae3f),
> then more recently started rejecting them outright (8c5b1ae).

Right, but these would still happen:

michael <michael@michael-laptop.michael.org>

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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

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

If you have a strawman definition that does not have anything to do with
what I said in my original email, then yes, it could happen. But as I
said already, "git var" uses IDENT_STRICT and will not allow such broken
names.

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

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

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

Sorry, but that is not how things work on this project. You do not get
to cause regressions because you are too lazy to implement the feature
_you_ want in a way that does not break other people.

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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

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.

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

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.

>> > 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
<felipec@nysa.felipec.org>]

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
<felipe.contreras@gmail.com>]

4) config user

Who should the emails appear to be from? [Felipe Contreras 2nd
<felipe.contreras+2@gmail.com>]

5) GIT_COMMITTER

Who should the emails appear to be from? [Felipe Contreras 2nd
<felipe.contreras+2@gmail.com>]

Whoa, what happened there?

Well:

  $sender = $repoauthor || $repocommitter || '';
  ($repoauthor) = Git::ident_person(@repo, 'author');
  % ./git var GIT_AUTHOR_IDENT
  Felipe Contreras 2nd <felipe.contreras+2@gmail.com> 1352783223 +0100

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

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
<felipe.contreras+4@gmail.com>]

What about after my change?

6.1) GIT_AUTHOR without anything else

fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
var GIT_COMMITTER_IDENT: command returned error: 128

4) config user

From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

5) GIT_COMMITTER

From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

6) GIT_AUTHOR

From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

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 <felipec@nysa.felipec.org>

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 <felipe.contreras@gmail.com>

4) config user

From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

5) GIT_COMMITTER

From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

(as buggy as before)

6) GIT_AUTHOR

From: Felipe Contreras 4th <felipe.contreras+4@gmail.com>

Not only will this fix 'git send-email', but it will also fix 'git
commit' so that we don't end up with authors such as 'Felipe Contreras
<felipec@nysa.felipec.org>' ever again.

> 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

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

On Tue, Nov 13, 2012 at 7:42 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

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

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

-- 
Felipe Contreras

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

> >> > No, it's not. Those broken names do not come from the environment, but
> >> > from our last-resort guess of the hostname.
> >>
> >> That depends how you define environment, but fine, the point is that
> >> it happens.
> >
> > If you have a strawman definition that does not have anything to do with
> > what I said in my original email, then yes, it could happen.
> 
> 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.

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

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

> > 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
> <felipec@nysa.felipec.org>]
> 
> That's right, ident doesn't fail, and that's not the mail address I
> specified, it's *implicit*.

Right. I never said it did. I said it currently rejected obviously bogus
stuff (like the ".(none)" above) due to IDENT_STRICT, but currently
allowed implicit definitions. And I also said that if we get rid of the
prompt, we should disallow implicit definitions like this, because the
prompt is the safety valve on sending out mails with broken from
addresses. I even wrote a patch that let you find out whether the ident
was generated implicitly.

> 3) Full Name + EMAIL
> 
> Who should the emails appear to be from? [Felipe Contreras
> <felipe.contreras@gmail.com>]

Which sounds fine to me. EMAIL is considered explicit, and I have not
seen any evidence that people are putting bogus values in their EMAIL
variable and complaining that it is git's fault for respecting it.

> 4) config user
> 
> Who should the emails appear to be from? [Felipe Contreras 2nd
> <felipe.contreras+2@gmail.com>]

OK.

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

Yeah, I find that somewhat questionable in the current behavior, and I'd
consider it a bug. Typically we prefer the committer ident when given a
choice (e.g., for writing reflog entries).

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

Right. Same bug as above.

> So $repoauthor || $repocommiter is pointless.

Agreed.

> 6) GIT_AUTHOR
> 
> Who should the emails appear to be from? [Felipe Contreras 4th
> <felipe.contreras+4@gmail.com>]

Right, that's what I'd expect.

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

Doesn't that seem like a regression? It used to work.

> 4) config user
> 
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

OK.

> 5) GIT_COMMITTER
> 
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

OK.

> 6) GIT_AUTHOR
> 
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>

Doesn't that seem like a regression? It used to use a different address,
and in every other git program, the environment takes precedence over
config.

> And what about your proposed change?

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

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

Yes, it looks like we do not set up the explicit ident flags when
parsing the author. So my patch is insufficient.

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

Yep.

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

> > I tried to help you by pointing you in the right direction and even
> > providing a sample "git var" patch.
> 
> Are you 100% sure that was the right direction?

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

> I think the right approach is more along these lines:

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

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

I think this needs to be optional, otherwise you are breaking callers
who use IDENT_STRICT but are OK with the implicit ident (e.g.,
commit, format-patch with threading).

You can argue whether "git commit" should disallow such addresses, but
that is a separate topic from how send-email should behave.

> Not only will this fix 'git send-email', but it will also fix 'git
> commit' so that we don't end up with authors such as 'Felipe Contreras
> <felipec@nysa.felipec.org>' ever again.

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

> > But it is not my itch to scratch.
> 
> Suit yourself, it's only git users that would get hurt. I can always
> use my own 'git send-email' (as I am doing right now).

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

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

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

-Peff

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

On Tue, Nov 13, 2012 at 8:47 AM, Jeff King <peff@peff.net> 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.

>> 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
<felipec@felipec.org>' which would actually work, but is not explicit.

You are suggesting to break that use-case. You are introducing a
regression. And this case is realistic, unlike the
GIT_AUTHOR_NAME/EMAIL. Isn't it?

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?

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

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

Correct.

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

Agreed.

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

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

>> 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'); <-

>> So $repoauthor || $repocommiter is pointless.
>
> Agreed.

Good.

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

You mean without the input question?

>> 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');

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

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

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

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

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

I think strictess should be tied to explicitness, and 'git var' should
error out, and not die.

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

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

Yes, that's exactly what I would argue.

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

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.

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

Re: [PATCH] send-email: add proper default sender

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Just to make sure I follow the discussion correctly, do you mean
that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
specified more concretely and we usually use COMMIITTER for this
kind of thing in the first place but send-email does not in this
case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
the implicit logic as a bug in this case---just making sure).

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

I do not think we mind a change to favor GIT_COMMITTER setting over
GIT_AUTHOR to be consistent with others too much, but that indeed is
a big change.  People who are used to the inconsistency that
send-email favors AUTHOR over COMMITTER will certainly find it as a
regression.

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

Yeah, something like that would be necessary for "git var" users to
make this kind of decision.

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

As we didn't break "git commit" when you worked on IDENT_STRICT, I
do not think we have any data on it ;-)

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

For a change with low benefit/cost ratio like this, the best course
of action often is to stop paying attention to the thread that has
become unproductive and venomous, and resurrect the topic after
people forgot about it and doing it right yourself ;-).

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

> > 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:
> 
> > I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.
> 
> Suppose somebody has a full name, and a fully qualified domain name,
> and he can receive mails to it directly. Such a user would not need a
> git configuration, and would not need $EMAIL, or anything.
> 
> Currently 'git send-email' will throw 'Felipe Contreras
> <felipec@felipec.org>' which would actually work, but is not explicit.
> 
> You are suggesting to break that use-case. You are introducing a
> regression. And this case is realistic, unlike the
> GIT_AUTHOR_NAME/EMAIL. Isn't it?

Yes, dying would be a regression, in that you would have to configure
your name via the environment and re-run rather than type it at the
prompt. You raise a good point that for people who _could_ take the
implicit default, hitting "enter" is working fine now, and we would lose
that.  I'd be fine with also just continuing to prompt in the implicit
case.

But that is a much smaller issue to me than having send-email fail to
respect environment variables and silently use user.*, which is what
started this whole discussion. And I agree it is worth considering as a
regression we should avoid.

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

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

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

This is not religion. It is a software project. 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.

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

My point was that there is at least one known setup that uses only
environment variables and not a config file, and that the rest of git is
intended to work with that.  It is not a test failure, but t9001.18
reveals the fact that your change does not handle this situation; we
continue to prompt under the test suite's configuration. In the proper
fix, the test needs to be adjusted.

You don't see any test failures with your patch because we do not cover
the case that you regress (GIT_AUTHOR_EMAIL and user.email both set).

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

It is a larger hassle for both the developer and the user to fix the bug
after the fact, 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).

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

Yes. It would make sense to me to respect an explicit committer over an
implicit author. Though the fact that author is preferred over committer
is inconsistent with most of the rest of git. I'm undecided whether that
should be changed.

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

Ah, I see. We are loading both, whether or not they end up being used,
and barfing prematurely on errors. That seems like a bug.

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

Right. Which it sounds like now agree with me on?

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

What is it you are trying to accomplish? Eliminating the prompt in some
cases, as in your original patch? Dealing with implicit identities may
be orthogonal to your goal, but your original patch is not sufficient,
as it reverses the precedence of config and environment variables.
So you can either:

  1. Reimplement the environment variable lookup that ident.c does,
     leaving implicit ident logic out completely.

  2. Modify ident.c and "git var" to let send-email reuse the logic in
     ident.c, but avoid dropping the prompt when an implicit ident is
     used.

Doing (2) sounds a lot more maintainable to me in the long run.

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

I am tired of talking about this. The patch series below took me less
time to write than I have spent arguing with you. It was not that hard.

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

> 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

[PATCH 1/6] ident: make user_ident_explicitly_given private

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 2/6] ident: keep separate "explicit" flags for author and committer

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH 3/6] var: accept multiple variables on the command line

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH 4/6] var: provide explicit/implicit ident information

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 5/6] Git.pm: teach "ident" to query explicitness

[Jeff King]

"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 <peff@peff.net>
---
 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

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

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:

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

Having discussed more, I think there are two questionable things:

  1. Preferring author over committer

  2. Failing to fall back to committer when author is implicit or bogus
     (because "git var" dies).

I think (1) may fall into the "this is not how I would do it today, but
it is not worth a possible regression" category.

I think (2) might be worth fixing, though. Certainly when the author is
bogus (by IDENT_STRICT rules), which I think was the original intent of
the "$repoauthor || $repocommitter" code. Probably when the author ident
is implicit, though that is more hazy to me.

> For a change with low benefit/cost ratio like this, the best course
> of action often is to stop paying attention to the thread that has
> become unproductive and venomous, and resurrect the topic after
> people forgot about it and doing it right yourself ;-).

I came to the same conclusion, but decided to simply do it right now
while it was in my head. Hopefully we can progress by reviewing the
series I just posted.

-Peff

Re: [PATCH] send-email: add proper default sender

[Erik Faye-Lund]

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

I usually input "Linus Torvalds <torvalds@linux-foundation.org>", to
give my patch high priority ;-)

Re: [PATCH] send-email: add proper default sender

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I agree with both points.  Thanks.

Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness

[Jeff King]

On Tue, Nov 13, 2012 at 09:23:00AM -0800, Matt Kraai wrote:

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

Thanks.

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

Yes, thanks (left over from an earlier attempt that tried to avoid
having $options{explicit}).

-Peff

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

On Tue, Nov 13, 2012 at 5:48 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

Re: [PATCH 1/6] ident: make user_ident_explicitly_given private

[Jonathan Nieder]

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.
[...]
> --- 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);

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 <jrnieder@gmail.com>

Re: [PATCH 3/6] var: accept multiple variables on the command line

[Jonathan Nieder]

Jeff King wrote:

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

Yay!

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

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

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

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

[...]
> --- /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 &&

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 <author@example.com> $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

Re: [PATCH 4/6] var: provide explicit/implicit ident information

[Jonathan Nieder]

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

Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness

[Jonathan Nieder]

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:

[...]
> --- 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?

Thanks,
Jonathan

Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident

[Jonathan Nieder]

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 <jrnieder@gmail.com>

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

Re: [PATCH 1/6] ident: make user_ident_explicitly_given private

[Jeff King]

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

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

Re: [PATCH 3/6] var: accept multiple variables on the command line

[Jeff King]

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

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

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

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

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

Re: [PATCH 4/6] var: provide explicit/implicit ident information

[Jeff King]

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

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

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

Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness

[Jeff King]

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

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

Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident

[Jeff King]

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

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

Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident

[Jeff King]

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

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

Re: [PATCH] send-email: add proper default sender

[Jeff King]

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

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

Really? I mentioned initially the possibility of dying instead of
prompting. You raised the point that it would regress a certain use
case. And then what happened? I said above "you raise a good point[...].
I'd be fine with also just continuing to prompt[...]. I agree it is
worth considering as a regression we should avoid". And then I sent out
a patch series which does not have the regression.

In other words, my suggestion was a bad one, and once it was pointed
out, I did not pursue it.  If you want to call that "not paying much
attention", feel free. But I'd much rather you point out problems in my
actual patch series.

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

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

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

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

[PATCHv2 0/8] loosening "sender" prompt in send-email

[Jeff King]

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

[PATCHv2 1/8] test-lib: allow negation of prerequisites

[Jeff King]

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 <peff@peff.net>
---
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

[PATCHv2 2/8] t7502: factor out autoident prerequisite

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCHv2 3/8] ident: make user_ident_explicitly_given static

[Jeff King]

In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given"
was introduced as a global for communication between config,
ident, and builtin-commit.  In v1.7.0-rc0~72^2 (2010-01-07)
readers switched to using the common wrapper
user_ident_sufficiently_given().  After v1.7.11-rc1~15^2~18
(2012-05-21), the var is only written in ident.c.

Now we can make it static, which will enable further
refactoring without worrying about upsetting other code.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

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

[Jeff King]

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 <peff@peff.net>
---
 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

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

[Jeff King]

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.

While we're in the area, let's add some basic tests for "git
var", which until now was largely untested (and of course a
test for multiple values on top).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-var.txt |  9 +++++++--
 builtin/var.c             | 17 +++++++++-------
 t/t0007-git-var.sh        | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 9 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..49ab300 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -5,7 +5,7 @@
  */
 #include "builtin.h"
 
-static const char var_usage[] = "git var (-l | <variable>)";
+static const char var_usage[] = "git var (-l | <variable>...)";
 
 static const char *editor(int flag)
 {
@@ -73,21 +73,24 @@ 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) {
+		if (argc > 2)
+			usage(var_usage);
 		git_config(show_config, NULL);
 		list_vars();
 		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..ec5d946
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+	test_tick &&
+	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	git var GIT_AUTHOR_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+	test_tick &&
+	echo "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" >expect &&
+	git var GIT_COMMITTER_IDENT >actual &&
+	test_cmp expect actual
+'
+
+# For git var -l, we check only a representative variable;
+# testing the whole output would make our test too brittle with
+# respect to unrelated changes in the test suite's environment.
+test_expect_success 'git var -l lists variables' '
+	git var -l >actual &&
+	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	sed -n s/GIT_AUTHOR_IDENT=//p <actual >actual.author &&
+	test_cmp expect actual.author
+'
+
+test_expect_success 'git var -l lists config' '
+	git var -l >actual &&
+	echo false >expect &&
+	sed -n s/core\\.bare=//p <actual >actual.bare &&
+	test_cmp expect actual.bare
+'
+
+test_expect_success 'listing and asking for variables are exclusive' '
+	test_must_fail git var -l GIT_COMMITTER_IDENT
+'
+
+test_expect_success 'git var can show multiple values' '
+	cat >expect <<-EOF &&
+	$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE
+	$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	EOF
+	git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.0.207.gdf2154c

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

[Jeff King]

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 <peff@peff.net>
---
 Documentation/git-var.txt | 12 ++++++++++++
 builtin/var.c             | 27 +++++++++++++++++++++++++++
 t/t0007-git-var.sh        | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..bb2997d 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -39,9 +39,21 @@ 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, or "0" if it was determined implicitly from the system.
+    Identity is considered explicit if it comes from one of git's config
+    file (i.e., via the `user.*` variables), from the `$GIT_AUTHOR_*`
+    environment variables, or from the `$EMAIL` environment variable. It
+    is considered implicit if it was generated from system variables
+    like the username and hostname.
+
 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 49ab300..9503df9 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 ec5d946..47907de 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -47,4 +47,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

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

[Jeff King]

"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 <peff@peff.net>
---
 perl/Git.pm | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..28075a9 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,8 +750,16 @@ 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, we are in an array context, and we are
+using C<git var> to lookup the ident, the returned array will contain an
+additional boolean element specifying whether the ident was configured
+explicitly by the user. See the description of GIT_*_EXPLICIT in git-var(1) for
+details.
+
 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>.
+In an array context, C<ident_person> returns both the person string and the
+C<explicit> boolean.
 
 The synopsis is like:
 
@@ -763,17 +771,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;
+		}
+		return @ret;
 	} else {
 		return $identstr;
 	}
@@ -781,8 +794,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

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

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-send-email.perl   | 22 +++++++++++++---------
 t/t9001-send-email.sh | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 11 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..0fe0b8e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '
 
 test_expect_success $PREREQ 'Prompting works' '
 	clean_fake_sendmail &&
-	(echo "Example <from@example.com>"
-	 echo "to@example.com"
+	(echo "to@example.com"
 	 echo ""
 	) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$patches \
 		2>errors &&
+		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
+		grep "^To: to@example.com\$" msgtxt1
+'
+
+test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '
+	clean_fake_sendmail &&
+	(echo "Example <from@example.com>" &&
+	 echo "to@example.com" &&
+	 echo ""
+	) |
+	(sane_unset GIT_AUTHOR_NAME &&
+	 sane_unset GIT_AUTHOR_EMAIL &&
+	 sane_unset GIT_COMMITTER_NAME &&
+	 sane_unset GIT_COMMITTER_EMAIL &&
+	 GIT_SEND_EMAIL_NOTTY=1 git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches \
+		2>errors &&
 		grep "^From: Example <from@example.com>\$" msgtxt1 &&
 		grep "^To: to@example.com\$" msgtxt1
+	)
+'
+
+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
+	clean_fake_sendmail &&
+	(sane_unset GIT_AUTHOR_NAME &&
+	 sane_unset GIT_AUTHOR_EMAIL &&
+	 sane_unset GIT_COMMITTER_NAME &&
+	 sane_unset GIT_COMMITTER_EMAIL &&
+	 GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
+	 test_must_fail git send-email \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches </dev/null 2>errors.out &&
+		test_i18ngrep "tell me who you are" errors.out
+	)
 '
 
 test_expect_success $PREREQ 'tocmd works' '
-- 
1.8.0.207.gdf2154c

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

But that I meant that when I introduce a regression it's like I'm
killing all that is good and sacred about git, and when you do it's
everything but that.

Yes, you sent a new patch. So did I.

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

How could it be accidental if I said this: "Not only will this fix
'git send-email', but it will also fix 'git
commit'".

> And as it was in the middle of a discussion about whether
> regressions matter,

That was not the discussion at all.

You can't say that all regressions are the same, and if I say
"regression X doesn't matter", that means ALL regressions don't
matter. That's a hasty generalization.

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

I would, if I saw any chance in it actually going through.

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

A matter of opinion. I think that series introduces way too much code
for a very very small gain that eventually would probably disappear.

>> >   [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,

Patch #1 and #3 are refactoring, the rest are not.

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

I don't have any comments, except  that I don't think all that code is
needed. And why would I bother commenting there, if my opinion will be
ignored?

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

Yes, but it still adds a lot of code.

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

That's true, so we will be introducing a regression regardless.

Now, if we go with this patch:

http://article.gmane.org/gmane.comp.version-control.git/209660

Instead of your patch series, who will get hurt? Hint: I already
answered that question.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] send-email: add proper default sender

[Jeff King]

On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:

> But that I meant that when I introduce a regression it's like I'm
> killing all that is good and sacred about git, and when you do it's
> everything but that.

The rhetoric in this statement is a good indication that there is
nothing productive to come from our discussing it anymore.

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

Well, it certainly will not go through if you do not try.

-Peff

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

[Felipe Contreras]

On Thu, Nov 15, 2012 at 1:36 AM, Jeff King <peff@peff.net> wrote:

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

I don't think there's any need for all that, this does the trick:

diff --git a/git-send-email.perl b/git-send-email.perl
index aea66a0..503e551 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -748,16 +748,11 @@ if (!$force) {
        }
 }

-my $prompting = 0;
 if (!defined $sender) {
        $sender = $repoauthor || $repocommitter || '';
-       $sender = ask("Who should the emails appear to be from? [$sender] ",
-                     default => $sender,
-                     valid_re => qr/\@.*\./, confirm_only => 1);
-       print "Emails will be sent from: ", $sender, "\n";
-       $prompting++;
 }

+my $prompting = 0;

This passes all the current tests and the ones you added.

Which kind of user will get the prompt with your patch, that would
miss it with mine?

Cheers.

-- 
Felipe Contreras

Re: [PATCH] send-email: add proper default sender

[Felipe Contreras]

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

The point is still true.

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

At least I wouldn't be wasting my time.

Cheers.

-- 
Felipe Contreras

Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites

[Jonathan Nieder]

Jeff King wrote:

> +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '

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

Does this feature work when running a test explicitly using
"bash <name of test>"?  That's something I do from time to time to
figure out whether a weird behavior is shell-specific.

If it works everywhere, this patch would help me conquer my fear of
exclamation points in git's tests, which would be a comfort to me and
a very good thing.

Re: [PATCHv2 2/8] t7502: factor out autoident prerequisite

[Jonathan Nieder]

Jeff King wrote:

> --- 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
> +'

Lazy prereq scripts run in a subshell, so this should be safe.  Ack.

Thanks,
Jonathan

Re: [PATCHv2 3/8] ident: make user_ident_explicitly_given static

[Jonathan Nieder]

Jeff King wrote:

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

For what it's worth I liked the old commit message more.  But I don't
mind either.

Hope that helps,
Jonathan

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

[Jonathan Nieder]

Jeff King wrote:

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

Nice problem description.  The fixed behavior makes sense to me, for
what it's worth.

Not about this patch, but: in case (1), shouldn't the author fall
back to $GIT_COMMITER_NAME?

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

[Jonathan Nieder]

Jeff King wrote:

> This patch lets callers specify multiple variables, and
> prints one per line.
[...]
> Signed-off-by: Jeff King <peff@peff.net>

Very pleasantly done --- thanks.  For what it's worth, assuming this
is tested, I can't see any reason not to apply it.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jonathan Nieder]

Jeff King wrote:

> Signed-off-by: Jeff King <peff@peff.net>

For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jonathan Nieder]

Jeff King wrote:

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

The indentation seems strange here --- are the new "grep" lines
continuations of the git send-email line?

It's probably easier to change the structure completely:

	clean_fake_sendmail &&
	echo to@examples.com >prompt.input &&
	echo >>prompt.input &&
	GIT_SEND_EMAIL_NOTTY=1 \
		git send-email --smtp-server=... $patches <prompt.input &&
	grep "^From: A U Thor <authorident@example.com>\$" msgtxt1 &&
	grep "^To: to@example.com\$" msgtxt1

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

Likewise:

	clean_fake_sendmail &&
	echo "Example <from@example.com>" >prompt.in &&
	echo to@example.com >>prompt.in
	echo >>prompt.in &&
	(
		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
		sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
		GIT_SEND_EMAIL_NOTTY=1 \
			git send-email --smtp-server=... $patches <prompt.in
	) &&
	grep "^From: Example <from@example.com>\$" msgtxt1 &&
	grep "^To: to@example.com\$" msgtxt1

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

Likewise:

	clean_fake_sendmail &&
	(
		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
		sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
		GIT_SEND_EMAIL_NOTTY=1 \
			git send-email --smtp-server=... $patches </dev/null 2>err
	) &&
	test_i18ngrep "[Tt]ell me who you are" err

For what it's worth, with or without such changes,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jeff King]

On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:

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

It may pass on your system, but it will not on a system that meets the
AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
suspect your system config is such that we skip t9001.19 and run
t9001.20, whereas mine is the opposite).

> Which kind of user will get the prompt with your patch, that would
> miss it with mine?

One whose system is configured in such a way that git can produce an
automatic ident (i.e., has a non-blank GECOS name and a FQDN).

-Peff

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

[Felipe Contreras]

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

I tried both:

ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of
PERL,AUTOIDENT)
ok 20 - broken implicit ident aborts send-email

ok 19 - implicit ident prompts for sender
ok 20 # skip broken implicit ident aborts send-email (missing
!AUTOIDENT of PERL,!AUTOIDENT)

However, it would be much easier if ident learned to check
GIT_TEST_FAKE_HOSTNAME, or something.

But then I realized I had to run 'make' again. Yes, my patch breaks
test 19, but see below.

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

And doesn't have any of the following:

 * configured user.name/user.email
 * specified $EMAIL
 * configured sendemail.from
 * specified --from argument

Very unlikely. And then, what would be the consequences of not
receiving this prompt?

Cheers.

-- 
Felipe Contreras

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

[Jeff King]

On Thu, Nov 15, 2012 at 11:28:46AM +0100, Felipe Contreras wrote:

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

Yes, it would be. It has two downsides:

  1. The regular git code has to be instrumented to respect the
     variable, so it can potentially affect git in production use
     outside of the test suite. Since such code is simple, though, it is
     probably not a big risk.

  2. We would not actually exercise the code paths for doing
     hostname and GECOS lookup. We do not test their resulting values,
     so the coverage is not great now, but we do at least run the code,
     which would let a run with "--valgrind" check it. I guess we could
     go through the motions of assembling the ident and then replace
     it at the end with the fake value.

I don't have a strong opinion either way.

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

That is certainly the opinion you have stated already. I'm not sure I
agree. Linus, for example, was an advocate of such a configuration early
on in git's history. I don't think he still runs that way, though.

> And then, what would be the consequences of not receiving this prompt?

An email would be sent with the generated identity.

-Peff

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

[Jeff King]

On Thu, Nov 15, 2012 at 02:43:58AM -0800, Jeff King wrote:

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

I suspect you did not need me to answer that question, but were setting
it up as a rhetorical trap to mention the final confirmation, which I
failed to note in my response.

I think a much more compelling argument/commit message for your
suggested patch would be:

  We currently prompt the user for the "From" address. This is an
  inconvenience in the common case that the user has configured their
  identity in the environment, but is meant as a safety check for when
  git falls back to an implicitly generated identity (which may or may
  not be valid).

  That safety check is not really necessary, though, as by default
  send-email will prompt the user for a final confirmation before
  sending out any message. The likelihood that a user has both bothered
  to turn off this default _and_ not configured any identity (nor
  checked that the automatic identity is valid) is rather low.

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

-Peff

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

[Jeff King]

On Thu, Nov 15, 2012 at 03:13:47AM -0800, Jeff King wrote:

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

If that is the route we want to go, then we should obviously drop my
series in favor of your final patch. I think it would also need a test
update, no?

I think a more concise commit message would help, too. I disagree with a
great deal of the reasoning in your existing message, but those parts
turn out not to be relevant. The crux of the issue is that the safety
check is not necessary because there is already one (i.e., point 8 of
your list).  Feel free to use any or all of my text above.

>From my series, there were a few cleanups that might be worth salvaging.
Here is a rundown by patch:

  [1/8]: test-lib: allow negation of prerequisites

This stands on its own, and is something I have wanted a few times in
the past. However, since there is no immediate user, I don't know if it
is worth doing or not.

  [2/8]: t7502: factor out autoident prerequisite

A minor cleanup and possible help to future tests, but since there are
no other callers now, not sure if it is worth it.

  [3/8]: ident: make user_ident_explicitly_given static

A cleanup that is worth doing, I think.

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

Another cleanup.  This is "more correct", in that it handles the corner
cases I mentioned in the commit message. But no current code cares about
those corner cases, because the only real caller is git-commit, and this
is a purely internal interface. I could take or leave it.

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

The tests for this can be split out; we currently don't have "git var"
tests at all, so increasing our test coverage is reasonable. The
multiple variables thing is potentially useful, but there are simply not
that many callers of "git var", and nobody has been asking for such a
feature (we could use it to save a process in git-send-email, but it is
probably not worth the complexity).

  [6/8]: var: provide explicit/implicit ident information
  [7/8]: Git.pm: teach "ident" to query explicitness

These two should probably be dropped. They would lock us into supporting
the explicit/implicit variables in "git var", for no immediate benefit.

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

Obviously drop.

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

If it sounds like I am blaming you here, I am to some degree. But I am
also blaming myself. I should have read your commit message more
carefully, and I'm sorry for not doing so. I hope we can both try harder
to avoid getting side-tracked on arguing about issues that turned out
not to be important at all (of course, we cannot know which part of the
discussion will turn out to be important, but I think there some
obviously unproductive parts of this discussion).

-Peff

Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites

[Jeff King]

On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:

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

I can understand, having been bitten by it many times myself (not only
is it counter-intuitively expanded inside double quotes, and not only
does it not do what you wanted, but it does not insert the command into
the history, so you cannot even go back to fix it easily).

> Does this feature work when running a test explicitly using
> "bash <name of test>"?  That's something I do from time to time to
> figure out whether a weird behavior is shell-specific.

Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
that the "!" is part of history expansion, which is only enabled by
default for interactive shells.

> If it works everywhere, this patch would help me conquer my fear of
> exclamation points in git's tests, which would be a comfort to me and
> a very good thing.

As far as I know, "!" should be safe in a non-interactive shell script.

-Peff

Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites

[Jonathan Nieder]

Jeff King wrote:

> Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
> that the "!" is part of history expansion, which is only enabled by
> default for interactive shells.

Nice to hear.  Thanks much for looking into it.

> On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:

>> If it works everywhere, this patch would help me conquer my fear of
>> exclamation points in git's tests, which would be a comfort to me and
>> a very good thing.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

This somehow reminds me of the first paragraph of f20f387 (commit:
check committer identity more strictly, 2012-07-23).

I never use "send-email driving format-patch" workflow myself, but I
suspect there are people among who do so who are using --compose to
do the cover letter of their series.  Does the "confirmation as the
last step" help them, or would they have to retype their message?

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

[Jeff King]

On Thu, Nov 15, 2012 at 08:56:37AM -0800, Junio C Hamano wrote:

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

That is a good question. That confirmation step does come after they
have typed their cover letter. However, if they are using --compose,
they are dumped in their editor with something like:

  From Jeff King <peff@peff.net> # This line is ignored.
  GIT: Lines beginning in "GIT:" will be removed.
  GIT: Consider including an overall diffstat or table of contents
  GIT: for the patch you are writing.
  GIT:
  GIT: Clear the body content if you don't wish to send a summary.
  From: Jeff King <peff@peff.net>
  Subject: 
  In-Reply-To: 

which I think would count as sufficient notice of the address being
used.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

OK.  Tentatively I replaced your old series with these 8 patches
including the last one, as I tend to agree with the value the
earlier clean-up in the series gives us in the longer term.  As you
and Felipe discussed, we may want to replace the last one with a
simpler "don't bother asking" patch, but I think that is more or
less an orthogonal issue.

Thanks.

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

[Jeff King]

On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:

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

I'm not sure how orthogonal it is. The latter half of my series is about
exposing the user_ident_sufficiently_given() flag. If we go with
Felipe's patch, then that exposed information has no users, and it may
not be worth it (OTOH, it's possible that some third-party script may
want it).

-Peff

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

[Felipe Contreras]

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

Well, who is using user_ident_sufficiently_given() in the first place?
I think 'git commit' might be suffering from the same problem that
prompted you to split it.

Cheers.

-- 
Felipe Contreras

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Oh, no, you are correct.  I shouldn't have said "the last one", but
instead "the later ones".  The first two we should definitely keep,
at least, but the other three are not so clear.

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

[Jeff King]

On Fri, Nov 16, 2012 at 08:57:43PM +0100, Felipe Contreras wrote:

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

It is just `git commit` now. It does not suffer from the problems that
prompted the author/committer split:

  http://article.gmane.org/gmane.comp.version-control.git/209635

To expand on what I wrote there, we cannot hit case 2 because we always
ask for the committer within the same process. Case 1 is not
interesting, because we would only fail to show it if is identical to a
non-implicit committer (so even if it was implicit, we know that it is a
sane value).

-Peff