Lucas Gabriel Vuotto <lucas@sexy.is> wrote:
> Hey,
> 
> My tog started dying like this
> 
> 	$ tog 
> 	tog: GOT_AUTHOR environment variable is not set
> 
> This started happening with a66bb113790d6808d3a1646844962ac4d5153dec .
> tog_worktree_status exits with an error if it doesn't manage to find an
> author. This is easily reproducible by running tog in a dirty worktree
> from a cloned repo while GOT_AUTHOR is unset.
> 
> The following patch addresses that, by setting wt_author to "". I don't
> consider the situation should be fatal for this purpouse.
> 
> btw, I think that wctx->wt_author leaks? I couldn't convince myself that
> tog_worktree_status is called only once before worktree_ctx_close.
> 
> comments? oks?

Thanks for finding this; I did test with the GOT_AUTHOR environment
variable unset, but I have `author` set in my got.git/got.conf file!

I agree it shouldn't be a fatal error; however, I think it's better to
explicitly check for the no author case. I'm not sure if it's a good
idea to attempt another malloc if get_author() has already failed with
its strdup(3) or asprintf(3) calls because of ENOMEM. So the below diffs
produce the same behaviour as your fix except we check get_author()'s
GOT_ERR_COMMIT_NO_AUTHOR failure case.

I've also removed the author validation code from get_author() for two
reasons: tog's not the place to validate the author; and you've reminded
me of my TODO note to make get_author() a library routine, but I think
the validation should be decoupled from the retrieval. So the API will
be something like:

got_author_retrieve(char **, struct got_repository *, struct got_worktree *);
got_author_validate(char *);

In any case, the first commit removes valid_author(), and the second
sets wctx->wt_author to the empty string if get_author() fails because
we were unable to find an author.


With regards to leaking wctx members, I don't think it is being leaked:
tog_worktree_status() is only called by log_thread() if need_wt_status
is set. need_wt_status is only set by open_log_view() if the log view
is opened while in a work tree and by worktree_ctx_close(), which frees
the context members. need_wt_status is immediately unset when
tog_worktree_status() is called. So the control flow is something like:

if worktree:
	need_wt_status := 1
view loop:
	if (need_wt_status):
		tog_worktree_status():
			alloc wctx
		need_wt_status := 0
	worktree_ctx_close():
		release wctx
		need_wt_status := 1

I've also ktraced tog through various routes and haven't identified any
wctx (or any other memory) leaks so I don't think it's being leaked.


-----------------------------------------------
commit 76213573e2796759cc17ba5bc1e938142bc2e810
from: Mark Jamsek <mark@jamsek.dev>
date: Wed Dec 11 02:54:11 2024 UTC
 
 tog: remove author validation code
 
 It's not the place of tog to validate such things. The author in this
 case is for display purposes only. Got will alert the user in the event
 the bad author value is set when it's needed to write to the repository.
 
 M  tog/tog.c  |  1+  34-

1 file changed, 1 insertion(+), 34 deletions(-)

diff 90f5fbaf35ecebd981177c9be514580632965552 76213573e2796759cc17ba5bc1e938142bc2e810
commit - 90f5fbaf35ecebd981177c9be514580632965552
commit + 76213573e2796759cc17ba5bc1e938142bc2e810
blob - 2ce26fea02651c5c22189e43095204197b89c0d7
blob + ee2204f2759c2e124cf57661e5b510d26831be94
--- tog/tog.c
+++ tog/tog.c
@@ -3032,33 +3032,6 @@ select_commit(struct tog_log_view_state *s)
 	}
 }
 
-static const struct got_error *
-valid_author(const char *author)
-{
-	const char *email = author;
-
-	/*
-	 * Git' expects the author (or committer) to be in the form
-	 * "name <email>", which are mostly free form (see the
-	 * "committer" description in git-fast-import(1)).  We're only
-	 * doing this to avoid git's object parser breaking on commits
-	 * we create.
-	 */
-
-	while (*author && *author != '\n' && *author != '<' && *author != '>')
-		author++;
-	if (author != email && *author == '<' && *(author - 1) != ' ')
-		return got_error_fmt(GOT_ERR_COMMIT_BAD_AUTHOR, "%s: space "
-		    "between author name and email required", email);
-	if (*author++ != '<')
-		return got_error_fmt(GOT_ERR_COMMIT_NO_EMAIL, "%s", email);
-	while (*author && *author != '\n' && *author != '<' && *author != '>')
-		author++;
-	if (strcmp(author, ">") != 0)
-		return got_error_fmt(GOT_ERR_COMMIT_NO_EMAIL, "%s", email);
-	return NULL;
-}
-
 /* lifted from got.c:652 (TODO make lib routine) */
 static const struct got_error *
 get_author(char **author, struct got_repository *repo,
@@ -3115,13 +3088,7 @@ get_author(char **author, struct got_repository *repo,
 
 	*author = strdup(got_author);
 	if (*author == NULL)
-		return got_error_from_errno("strdup");
-
-	err = valid_author(*author);
-	if (err) {
-		free(*author);
-		*author = NULL;
-	}
+		err = got_error_from_errno("strdup");
 	return err;
 }
 

-----------------------------------------------
commit 974d5f1186af30a7c2c769ed8b33bf19a1eaf901 (main)
from: Mark Jamsek <mark@jamsek.dev>
date: Wed Dec 11 02:54:31 2024 UTC
 
 tog: don't fatal error if got author can't be found
 
 Reported by Lucas: if we can't find an author in the work tree or repo
 got.conf, git's config file, or the GOT_AUTHOR environment variable, set
 the author to the empty string for display purposes rather than hit the
 user with a fatal error. Based on Lucas's patch: the only change is to
 check for GOT_ERR_COMMIT_NO_AUTHOR before strdup(3)ing the empty string.
 
 M  tog/tog.c  |  8+  2-

1 file changed, 8 insertions(+), 2 deletions(-)

diff 76213573e2796759cc17ba5bc1e938142bc2e810 974d5f1186af30a7c2c769ed8b33bf19a1eaf901
commit - 76213573e2796759cc17ba5bc1e938142bc2e810
commit + 974d5f1186af30a7c2c769ed8b33bf19a1eaf901
blob - ee2204f2759c2e124cf57661e5b510d26831be94
blob + a09591e35e09e13fb23f7c13166a95b75ab02ffc
--- tog/tog.c
+++ tog/tog.c
@@ -3211,8 +3211,14 @@ tog_worktree_status(struct tog_log_thread_args *ta)
 
 	if (wt_state != 0) {
 		err = get_author(&wctx->wt_author, ta->repo, wt);
-		if (err != NULL)
-			goto done;
+		if (err != NULL) {
+			if (err->code != GOT_ERR_COMMIT_NO_AUTHOR)
+				goto done;
+			if ((wctx->wt_author = strdup("")) == NULL) {
+				err = got_error_from_errno("strdup");
+				goto done;
+			}
+		}
 
 		wctx->wt_root = strdup(got_worktree_get_root_path(wt));
 		if (wctx->wt_root == NULL) {


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68