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

From: Jeff King <peff@peff.net>
Date: Oct 23, 2019, 05:02 UTC
Message-ID: <20191023050206.GA29779@sigill.intra.peff.net>
In-Reply-To: <20191022232847.5212-1-prarit@redhat.com>

On Tue, Oct 22, 2019 at 07:28:47PM -0400, Prarit Bhargava wrote:

Show 7 quoted lines

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

Show 11 quoted lines

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

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

Show 5 quoted lines

> +	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?

Show 7 quoted lines

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

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

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

-Peff