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

[Felipe Contreras <felipe.contreras@gmail.com>]

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