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@xxxxxxxxxx" 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
>
