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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2012, 16:13 UTC
Message-ID: <7vpq3hfpm7.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20121113074720.GA18746@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 14 quoted lines

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