From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: Move got_opentempfd out of got_repo_open
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Wed, 15 Jun 2022 07:43:03 -0600

On Wed, Jun 15, 2022 at 01:23:38PM +0200, Stefan Sperling wrote:
> 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.

Nice catch! I did not do any searches in my tog tests. I'll add that to
the pile of things to remember. Thanks.

> 
> 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)
> 

-- 

Tracey Emery