On Tue, Jun 14, 2022 at 09:58:39AM +0200, Stefan Sperling wrote:
> > Here, got_repo_open accepts a single array of fds to work with. This
> > function copies the opened fds from repo->packs into a single array for
> > run_blame, since we need to use our existing fds here, which are split
> > up between accumfd and basefd. This cannot be inlined, since we can't
> > work directly on the repo struct in tog or got.
> > 
> > Perhaps you can think of a better way to do this for tog? I'm open to
> > suggestions. :)
> 
> So you did this because tog creates a separate repo instance for its
> blame thread, since our library code isn't thread-safe with an instance
> of struct got_repository being shared across threads.
> 
> The main thread isn't actually using this repo once the blame operation
> has started, but it has already opened the files. You want to avoid
> closing pack_fds in stop_blame() and re-opening them again in run_blame()?
> Is there a reason other than efficiency for avoiding this?
> In the log-thread case, which is similar, you have instead chosen to close
> and re-open.
> 
> I think we'll need a different name for got_repo_pack_fds_copy() if we
> end up keeping this function. Something other than "copy"; "copy" implies
> that duplicates of these file descriptors are being created, which is not
> what this function does. Perhaps got_repo_pack_fds_get()?
> /* Obtain the array of open pack file descriptors used by a repository. */
> const struct got_error *got_repo_pack_fds_get(int **, struct got_repository *);
> 

I'm still trying to completely understand this entire section of code. I
started with opening and closing in run_blame, and it crashes. Let me
walk through this part some more to see if I can get it working.

> > @@ -2311,8 +2331,12 @@ cmd_fetch(int argc, char *argv[])
> >  		goto done;
> >  	}
> >  
> > +	error = got_repo_pack_fds_open(&pack_fds);
> > +	if (error != NULL)
> > +		goto done;
> > +
> >  	if (repo_path == NULL) {
> > -		error = got_worktree_open(&worktree, cwd);
> > +		error = got_worktree_open(&worktree, cwd, pack_fds);
> >  		if (error && error->code != GOT_ERR_NOT_WORKTREE)
> >  			goto done;
> >  		else
> 
> It is unfortunate to have got_worktree_open depend on pack_fds.
> The reason is that the work tree base commit ID is being resolved
> at open() time. I will try to find a way to avoid this to allow you
> to drop all of these chunks from your diff if possible.

Ok. I'll look too, when I get back to this work.

> 
> > blob - fff58b083aa8eadb970ebd09ce593455d223fcc9
> > blob + c148b0c64ac4412a9bfc2da3e193337b4cc91238
> > --- lib/pack_create.c
> > +++ lib/pack_create.c
> > @@ -1520,21 +1520,13 @@ load_packed_tree_ids(void *arg, struct got_tree_object
> >  	if (tree == NULL) {
> >  		free(a->id);
> >  		a->id = got_object_id_dup(id);
> > -		if (a->id == NULL) {
> > -			err = got_error_from_errno("got_object_id_dup");
> > -			free(a->dpath);
> > -			a->dpath = NULL;
> > -			return err;
> > -		}
> > +		if (a->id == NULL)
> > +			return got_error_from_errno("got_object_id_dup");
> >  
> >  		free(a->dpath);
> >  		a->dpath = strdup(dpath);
> > -		if (a->dpath == NULL) {
> > -			err = got_error_from_errno("strdup");
> > -			free(a->id);
> > -			a->id = NULL;
> > -			return err;
> > -		}
> > +		if (a->dpath == NULL)
> > +			return got_error_from_errno("strdup");
> >  
> >  		a->mtime = mtime;
> >  		return NULL;
> 
> The above looks like a rebase gone wrong. This change re-introduces leaks
> that were present in an earlier version of my object-enumerate patch.
> 
> > @@ -4162,7 +4202,7 @@ draw_blame(struct tog_view *view)
> >  			width = 9;
> >  			wline = wcsdup(L"");
> >  			if (wline == NULL) {
> > -				err = got_error_from_errno("wcsdup");
> > +				err = got_error_from_errno("wcsup");
> >  				free(line);
> >  				return err;
> 
> This change does not look like it was intentional either?
> 

:( neither of those is intentional. I'll have to figure out want went
wrong here. Thanks for catching those.

-- 

Tracey Emery