From: Mark Jamsek Subject: Re: Make missing worktree author non-fatal in tog To: Lucas Gabriel Vuotto Cc: gameoftrees@openbsd.org Date: Wed, 11 Dec 2024 14:25:03 +1100 Lucas Gabriel Vuotto 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 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 ", 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 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68