From: Omar Polo <op@omarpolo.com>
Subject: improve error message with malformed `author' in got.conf
To: gameoftrees@openbsd.org
Date: Sun, 06 Feb 2022 12:44:21 +0100

Hello,

playing with the diff to add -A to commit, I noticed that the
GOT_ERR_COMMIT_NO_EMAIL error message is not correct in some
circumstances.  Let's say that I have a got.conf with

	author 'foobar'

Then, if I try to commit I get

% got ci regular-file
got: GOT_AUTHOR environment variable contains no email address; an email
address is required for compatibility with Git

which is a bit misleading since I may have a valid $GOT_AUTHOR set

% env | grep GOT_AUTHOR
GOT_AUTHOR=Omar Polo <op@omarpolo.com>

With the following diff instead I would get

% got ci regular-file
got: foobar: an email address is required for compatibility with Git

The original message is a helpful since it points to a possible cause,
but it can be misleading, while this shows the invalid value without
pointing the user to the source.  I think it's overall a little bit
better since if someone is messing with got.conf should immediately see
the issue.

what do you think?

diff 4cd2c1745cda66c359c86bea412d77af0b643a7e /home/op/w/got
blob - 16b939dba60d2210e6c75b2ffbb51a4b1f8d608a
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -596,6 +596,26 @@ import_progress(void *arg, const char *path)
 	return NULL;
 }
 
+static int
+valid_author(const char *author)
+{
+	/*
+	 * Really dumb email address check; we're only doing this to
+	 * avoid git's object parser breaking on commits we create.
+	 */
+	while (*author && *author != '<')
+		author++;
+	if (*author != '<')
+		return 0;
+	while (*author && *author != '@')
+		author++;
+	if (*author != '@')
+		return 0;
+	while (*author && *author != '>')
+		author++;
+	return *author == '>';
+}
+
 static const struct got_error *
 get_author(char **author, struct got_repository *repo,
     struct got_worktree *worktree)
@@ -653,28 +673,8 @@ get_author(char **author, struct got_repository *repo,
 	if (*author == NULL)
 		return got_error_from_errno("strdup");
 
-	/*
-	 * Really dumb email address check; we're only doing this to
-	 * avoid git's object parser breaking on commits we create.
-	 */
-	while (*got_author && *got_author != '<')
-		got_author++;
-	if (*got_author != '<') {
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-		goto done;
-	}
-	while (*got_author && *got_author != '@')
-		got_author++;
-	if (*got_author != '@') {
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-		goto done;
-	}
-	while (*got_author && *got_author != '>')
-		got_author++;
-	if (*got_author != '>')
-		err = got_error(GOT_ERR_COMMIT_NO_EMAIL);
-done:
-	if (err) {
+	if (!valid_author(*author)) {
+		err = got_error_fmt(GOT_ERR_COMMIT_NO_EMAIL, "%s", *author);
 		free(*author);
 		*author = NULL;
 	}
blob - 78a5edb08ad1bbb3feb172d3b16c537a9fe1c3cb
file + include/got_error.h
--- include/got_error.h
+++ include/got_error.h
@@ -287,9 +287,8 @@ static const struct got_error {
 	{ GOT_ERR_STAGED_PATHS, "work tree contains files with staged "
 	    "changes; these changes must be committed or unstaged first" },
 	{ GOT_ERR_PATCH_CHOICE, "invalid patch choice" },
-	{ GOT_ERR_COMMIT_NO_EMAIL,"GOT_AUTHOR environment variable contains "
-	    "no email address; an email address is required for compatibility "
-	    "with Git" },
+	{ GOT_ERR_COMMIT_NO_EMAIL, "an email address is required for "
+	    "compatibility with Git" },
 	{ GOT_ERR_TAG_EXISTS,"specified tag already exists" },
 	{ GOT_ERR_GIT_REPO_FORMAT,"unknown git repository format version" },
 	{ GOT_ERR_REBASE_REQUIRED,"specified branch must be rebased first" },
blob - 34d74d26fefb2a4461a2c489cade5dcd7ebfcd66
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -649,11 +649,9 @@ test_commit_no_email() {
 		got commit -m 'test no email' > $testroot/stdout \
 		2> $testroot/stderr)
 
-	echo -n "got: GOT_AUTHOR environment variable contains no email " \
+	echo -n "got: :flan_hacker:: an email address is required " \
 		> $testroot/stderr.expected
-	echo -n "address; an email address is required for compatibility "\
-		>> $testroot/stderr.expected
-	echo "with Git" >> $testroot/stderr.expected
+	echo "for compatibility with Git" >> $testroot/stderr.expected
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
 	if [ "$ret" != "0" ]; then