Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's username

[Prarit Bhargava <prarit@redhat.com>]

On 10/23/19 1:02 AM, Jeff King wrote:
> On Tue, Oct 22, 2019 at 07:28:47PM -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 "%aU"|"%au" option that outputs the author's email username.
> 
> Like others, this seems potentially useful even if I probably wouldn't
> use it myself. Another more complicated way to think of it would be to
> give a list of domains to omit (so if 90% of the committers are
> @redhat.com, we can skip that, but the one-off contributor from another
> domain gets their fully qualified name.>
> But that's a lot more complicated. I don't mind doing the easy thing
> now, and even if we later grew the more complicated thing, I wouldn't be
> sad to still have this easy one as an option.

FWIW, I went through the exact same thought process as you did and came to the
same conclusion.

> 
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char part,
>>  		strbuf_add(sb, mail, maillen);
>>  		return placeholder_len;
>>  	}
>> +	if (part == 'u' || part == 'U') {	/* username */
>> +		maillen = strstr(s.mail_begin, "@") - s.mail_begin;
>> +		strbuf_add(sb, mail, maillen);
>> +		return placeholder_len;
>> +	}
> 
> What happens if the email doesn't have an "@"? I think you'd either end
> up printing a bunch of extra cruft (because you're not limiting the
> search for "@" to the boundaries from split_ident_line) or you'd
> crash (if there's no "@" at all, and you'd get a huge maillen).
> 
> There's also no need to use the slower strstr() when looking for a
> single character. So perhaps:
> 
>   const char *at = memchr(mail, '@', maillen);
>   if (at)
>           maillen = at - mail;
>   strbuf_add(sb, mail, maillen);

TBH I had assumed that the email address was RFC2822 compliant.  Thanks for the
code.  I've incorporated it into v2.

> 
>> +test_expect_success 'log pretty %an %ae %au' '
> 
> As others noted, this could cover %aU, too (which is broken; you need to
> handle 'U' alongside 'E' and 'N' earlier in format_person_part()).

Whups.  Thanks for the pointer.  Also fixed in v2.

> 
>> +	git checkout -b anaeau &&
>> +	test_commit anaeau_test anaeau_test_file &&
>> +	git log --pretty="%an" > actual &&
>> +	git log --pretty="%ae" >> actual &&
>> +	git log --pretty="%au" >> actual &&
> 
> Maybe:
> 
>   git log --pretty="%an %ae %au"
> 
> or
> 
>   git log --pretty="%an%n%ae%n%au"
> 
> which is shorter and runs more efficiently?
> 
>> +	git log > full &&
>> +	name=$(cat full | grep "^Author: " | awk -F "Author: " " { print \$2 } " | awk -F " <" " { print \$1 } ") &&
>> +	email=$(cat full | grep "^Author: " | awk -F "<" " { print \$2 } " | awk -F ">" " { print \$1 } ") &&
>> +	username=$(cat full | grep "^Author: " | awk -F "<" " { print \$2 } " | awk -F ">" " { print \$1 } " | awk -F "@" " { print \$1 } " ) &&
>> +	echo "${name}" > expect &&
>> +	echo "${email}" >> expect &&
>> +	echo "${username}" >> expect &&
> 
> These values come from our hard-coded test setup, so it would be more
> readable to just expect those:
> 
>   {
> 	echo "$GIT_AUTHOR_NAME" &&
> 	echo "$GIT_AUTHOR_EMAIL" &&
> 	echo "$GIT_AUTHOR_EMAIL" | sed "s/@.*//"
>   } >expect

Added to v2 (along with Brian's suggestion to test %aE, %aN, and %aL).

> 
> For the last one, also I wouldn't be upset to see test-lib.sh do
> something like:
> 
>   TEST_AUTHOR_USERNAME=author
>   TEST_AUTHOR_DOMAIN=example.com
>   GIT_AUTHOR_NAME=$TEST_AUTHOR_USERNAME@$TEST_AUTHOR_DOMAIN

I like this suggestion a lot.  I'm incorporating into v2 as well as similar
changes for the COMMITTER fields.

P.

> 
> to let tests like this pick out the individual values if they want.
> 
> -Peff
>