Re: [PATCH] send-email: add proper default sender
- From
Jeff King <peff@peff.net>
- Date
- Nov 13, 2012, 07:47 UTC
- Message-ID
- <20121113074720.GA18746@sigill.intra.peff.net>
- In-Reply-To
- <CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com>
On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:
Show 10 quoted lines
> >> > 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.
Show 6 quoted lines
> > 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.
Show 7 quoted lines
> > 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?
Show 6 quoted lines
> > 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?
Show 7 quoted lines
> 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).
Show 7 quoted lines
> 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.
Show 15 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 10 quoted lines
> 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.
Show 11 quoted lines
> 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...
Show 11 quoted lines
> --- 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