From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Move got_opentempfd out of got_repo_open
To: gameoftrees@openbsd.org
Date: Wed, 15 Jun 2022 13:23:38 +0200

On Wed, Jun 15, 2022 at 12:45:38PM +0200, Stefan Sperling wrote:
> On Tue, Jun 14, 2022 at 02:47:37PM -0600, Tracey Emery wrote:
> > On Tue, Jun 14, 2022 at 07:34:24AM -0600, Tracey Emery wrote:
> > 
> > I think this is fairly sane now.
> > ok?
> 
> Ok by me.
> 
> This also seems to fix gotweb? Has gotweb been broken since
> commit 57160834 due to the lack of a wpath pledge promise?

Your patch introduces a regression in tog.

To reproduce, open tog, type / to begin a search, and search for
a string that won't match anything. Now while "searching..." is
displayed, cancel the search with backspace. The expected behaviour,
as without your patch, is that the search is cancelled and the
display goes back to showing nothing or a reference name in place
of "searching...". With your patch applied, "loading..." will be
displayed instead and won't disappear.

The patch below fixes this, on top of your patch.
I don't understand how the interaction with the search feature results
in the behaviour described above. But management of the pack fds for
the log thread is definitely incorrect in your version of the diff.

I've also noticed that packfd array pointers aren't being reset to
NULL after got_repo_pack_fds_close() in many (all?) other cases.
Instead there is a pointless pack_fds = NULL at the end of
got_repo_pack_fds_close() which doesn't do anything useful since
the caller needs to update its copy of the pointer instead.
This problem is not fixed here.

I am still fine with your original patch going in. We can more
easily fix these issues on top.

diff bdd930398cd4aed88e9241b6b0a1eba915362566 /home/stsp/src/got
blob - 9018516f932ee548ec87c8bc079aee935759ede9
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -338,6 +338,7 @@ struct tog_log_thread_args {
 	const char *in_repo_path;
 	struct got_object_id *start_id;
 	struct got_repository *repo;
+	int *pack_fds;
 	int log_complete;
 	sig_atomic_t *quit;
 	struct commit_queue_entry **first_displayed_entry;
@@ -2162,6 +2163,14 @@ stop_log_thread(struct tog_log_view_state *s)
 		s->thread_args.repo = NULL;
 	}
 
+	if (s->thread_args.pack_fds) {
+		const struct got_error *pack_err =
+		    got_repo_pack_fds_close(s->thread_args.pack_fds);
+		if (err == NULL)
+			err = pack_err;
+		s->thread_args.pack_fds = NULL;
+	}
+
 	if (s->thread_args.graph) {
 		got_commit_graph_close(s->thread_args.graph);
 		s->thread_args.graph = NULL;
@@ -2318,7 +2327,6 @@ open_log_view(struct tog_view *view, struct got_object
 	struct got_repository *thread_repo = NULL;
 	struct got_commit_graph *thread_graph = NULL;
 	int errcode;
-	int *pack_fds = NULL;
 
 	if (in_repo_path != s->in_repo_path) {
 		free(s->in_repo_path);
@@ -2372,11 +2380,13 @@ open_log_view(struct tog_view *view, struct got_object
 	view->search_start = search_start_log_view;
 	view->search_next = search_next_log_view;
 
-	err = got_repo_pack_fds_open(&pack_fds);
-	if (err)
-		goto done;
+	if (s->thread_args.pack_fds == NULL) {
+		err = got_repo_pack_fds_open(&s->thread_args.pack_fds);
+		if (err)
+			goto done;
+	}
 	err = got_repo_open(&thread_repo, got_repo_get_path(repo), NULL,
-	    pack_fds);
+	    s->thread_args.pack_fds);
 	if (err)
 		goto done;
 	err = got_commit_graph_open(&thread_graph, s->in_repo_path,
@@ -2413,12 +2423,6 @@ open_log_view(struct tog_view *view, struct got_object
 	s->thread_args.search_next_done = &view->search_next_done;
 	s->thread_args.regex = &view->regex;
 done:
-	if (pack_fds) {
-		const struct got_error *pack_err =
-		    got_repo_pack_fds_close(pack_fds);
-		if (err == NULL)
-			err = pack_err;
-	}
 	if (err)
 		close_log_view(view);
 	return err;
@@ -2454,7 +2458,6 @@ input_log_view(struct tog_view **new_view, struct tog_
 	struct tog_view *ref_view = NULL;
 	struct commit_queue_entry *entry;
 	int begin_x = 0, n, nscroll = view->nlines - 1;
-	int *pack_fds = NULL;
 
 	if (s->thread_args.load_all) {
 		if (ch == KEY_BACKSPACE)
@@ -2655,16 +2658,11 @@ input_log_view(struct tog_view **new_view, struct tog_
 		} else /* 'B' */
 			s->log_branches = !s->log_branches;
 
-		err = got_repo_pack_fds_open(&pack_fds);
-		if (err)
-			return err;
 		err = got_repo_open(&s->thread_args.repo,
-		    got_repo_get_path(s->repo), NULL, pack_fds);
+		    got_repo_get_path(s->repo), NULL,
+		    s->thread_args.pack_fds);
 		if (err)
 			return err;
-		err = got_repo_pack_fds_close(pack_fds);
-		if (err)
-			return err;
 		tog_free_refs();
 		err = tog_load_refs(s->repo, 0);
 		if (err)