Re: [PATCH v2] pretty: add "%aL"|"%al|%cL|%cl" option to output local-part of email addresses

[Prarit Bhargava <prarit@redhat.com>]

On 10/24/19 12:16 PM, Jeff King wrote:
> On Thu, Oct 24, 2019 at 08:53:32AM -0400, Prarit Bhargava wrote:
>
>> In many projects the number of contributors is low enough that users know
>> each other and the full email address doesn't need to be displayed.
>> Displaying only the author's username saves a lot of columns on the screen.
>> For example displaying "prarit" instead of "prarit@redhat.com" saves 11
>> columns.
>>
>> Add a "%aL"|"%al|%cL|%cl" option that output the local-part of an email
>> address.
>>
>> Also add tests for "%ae","%an", "%ce", and "%cn".
>
> Thanks, this is looking better, but I think there are still a few minor
> bits to address.
>

Thanks :)

>> Changes in v2:
>> - Changed option to 'L' based on https://www.ietf.org/rfc/rfc2822.txt
>>   definition of 'local-part' of email addresses.
>> - added additional information to documentation for %cL and %cl
>> - added mailmap output test
>> - modified code to use mailmap output for "L" option
>> - modified code to check if email address field has '@' symbol
>> - modified tests based on input from Peff
>
> This change list is very welcome, but it should generally go after the
> "---", so that it's not part of the commit message (i.e., it is for
> people reading this email and reviewing now, but people reading "git
> log" later would not know or care about v1).
>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: Junio C Hamano <gitster@pobox.com>
>> Cc: brian m. carlson <sandals@crustytoothpaste.net>
>> Cc: Jeff King <peff@peff.net>
>
> Likewise we do not generally use "cc:" trailers in this project.
>
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index b87e2e83e6d0..9a1f900f114a 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -163,6 +163,11 @@ The placeholders are:
>>  '%ae':: author email
>>  '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
>>  	or linkgit:git-blame[1])
>> +'%al':: author local-part (the portion of the email address preceding the '@'
>> +	symbol)
>
> I'm not sure if it is worth saying something like "preceding the @
> symbol, or the whole address if no @ symbol". It's a pretty rare case,
> I'd think, and it does clutter up the wording. So just a thought.
>

After adding the text I've decided to keep it the same.  As you pointed out it
ended up being too long.

>> +'%aL':: author local-part (the portion of the email address preceding the '@'
>> +	symbol, respecting .mailmap, see linkgit:git-shortlog[1] or
>> +	linkgit:git-blame[1])
>
> This description gets pretty long. I wonder if we can simplify by
> referring to earlier formats, which would also make clear to the user
> the relationship between the formats. Perhaps:
>
>   '%aL':: author local-part (see '%al'), respecting .mailmap (see '%aE')
>
> And ditto for %cL.

I have added this for v3.

<snip>

> I didn't think about this before, but...surely we're testing %an, etc
> already?
>
> Indeed, it looks like t6006 already covers that. Maybe you should be
> adding to that test? I note that it just hardcodes "author@example.com"
> in the expectation. I'd be OK with either following the lead there, or
> doing a separate preparatory patch to use $GIT_AUTHOR_EMAIL, etc.
>

I've done separate preparatory patches for both t6006 and t4203 for v3.

>> +	{
>> +		echo "${GIT_AUTHOR_NAME}" &&
>> +		echo "${GIT_AUTHOR_EMAIL}" &&
>> +		echo "${TEST_AUTHOR_LOCALNAME}"
>> +		echo "${GIT_AUTHOR_NAME}" &&
>> +		echo "${GIT_AUTHOR_EMAIL}" &&
>> +		echo "${TEST_AUTHOR_LOCALNAME}"
>> +	} > expect &&
>
> The expectation for %aE is the same as for %ae. So we're not really
> testing that we actually applied the mailmap. It looks like t4203 has
> some tests for %aE; you'd probably want to check %aL there.
>
>> +test_expect_success 'log pretty %cn %ce %cl %cN %cE %cL' '
>
> Likewise, both of the spots I mentioned cover the committer formats,
> too.
>
>> +TEST_AUTHOR_LOCALNAME=author
>> +TEST_AUTHOR_DOMAIN=example.com
>> +GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}
>
> If you follow the lead of t6006 and just hard-code these, then this hunk
> can go away. But if you do keep it, note that...
>

I implemented the suggested tests in both test files in v3.

>>  export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT
>> +export TEST_AUTHOR_LOCALNAME TEST_AUTHOR_DOMAIN
>>  export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
>> +export TEST_COMMITTER_LOCALNAME TEST_COMMITTER_DOMAIN
>
> These lines are unnecessary. We have to export GIT_AUTHOR_EMAIL, etc, so
> that the child git-commit process can read it. But there's no need to do
> so for TEST_*, which are meant to be used by the test scripts
> themselves.
>

Removed in v3.  I will post after running the test suite.

Thanks,

P.