From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix email parsing
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 19 Jul 2022 11:16:09 +0200

On Tue, Jul 19, 2022 at 10:55:17AM +0200, Omar Polo wrote:
> -static int
> +static const struct got_error *
>  valid_author(const char *author)
>  {
> +	const char *email = author;
> +
>  	/*
>  	 * Really dumb email address check; we're only doing this to
>  	 * avoid git's object parser breaking on commits we create.
>  	 */

The check isn't dumb anymore now, is it? :)

Actually, I think it would help to mention which git man page section
these rules come from, like you did in your initial email. The comment
could even quote the whole paragraph in case they move their docs around.

What I think is important to communicate to the reader (and to the user
who sees an error message) is that we are raising an error only because
Git enforces an email address (well, actually "<" and ">" as you found
out, but that's just an implementation detail). Otherwise, in Got we
could set GOT_AUTHOR="Flan Hacker" and it would work just fine.

> -	while (*author && *author != '<')
> +
> +	while (*author && *author != '\n' && *author != '<')
>  		author++;
> -	if (*author != '<')
> -		return 0;
> -	while (*author && *author != '@')
> +	if (*author++ != '<')
> +		goto err;

If a function does not need to free resources before returning then it
does not really need 'goto'. I don't see an advantage of having two
'goto err' cases over seeing this line appear twice:

	return got_error_fmt(GOT_ERR_COMMIT_NO_EMAIL, "%s", email);

Alternatively, this function could return of 0/1 and we could keep
raising an error in the caller.

Sorry, I missed this in my initial review. 

> +	while (*author && *author != '\n' && *author != '<' && *author != '>')
>  		author++;
> -	if (*author != '@')
> -		return 0;
> -	while (*author && *author != '>')
> -		author++;
> -	return *author == '>';
> +	if (strcmp(author, ">") != 0)
> +		goto err;
> +	return NULL;
> +
> +err:
> +	return got_error_fmt(GOT_ERR_COMMIT_NO_EMAIL, "%s", email);