From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: avoid extra tree requests after got-read-pack enumeration
To: gameoftrees@openbsd.org
Date: Tue, 14 Jun 2022 16:20:56 -0600

On Wed, Jun 15, 2022 at 12:08:22AM +0200, Stefan Sperling wrote:
> On Tue, Jun 14, 2022 at 02:46:24PM +0200, Stefan Sperling wrote:
> > Profiling got-read-pack shows a lot of individual tree requests
> > being sent by gotadmin pack -a, even if all required commits and
> > trees are present in the enumeration pack file.
> > This happens because gotadmin will always walk the entire history
> > again, the slow way, in order to look for any missing objects.
> > 
> > To fix this, make got-read-pack indicate whether it managed to locate
> > all required objects in its pack file, and only try loading more objects
> > if enumeration from the pack file was incomplete.
> > 
> > Tags need to be loaded in any case because got-read-pack will never
> > send tags while enumerating objects. This could still be optimized later.
> > 
> > ok?
> 
> I committed this patch after an off-list ok from op@
> 
> Turns out this exposed a bug elsewhere, and 'got send' is currently
> broken again as a result. Fix below.
> 
> ok?

Yes, this fixed it. ok

> 
> -----------------------------------------------
> commit 949fed073a0669d44791832ce3c26e7f3243ebe3 (fix-send)
> from: Stefan Sperling <stsp@stsp.name>
> date: Tue Jun 14 22:05:30 2022 UTC
>  
>  fix a bug in got_privsep_send_object_idlist() exposed by recent changes
>  
>  The old code did not work correctly if only a single object Id was to
>  be sent to got-read-pack. Make got-read-pack error out if the list
>  of commits for object enumeration is empty to catch this problem if
>  it occurs again.
>  
>  Found by the send_basic test, which was failing with GOT_TEST_PACK=1
>  
> diff 9ae64ce0617bf9bfc70ad80119b0a4f3553fc286 5a67f59023bb8af94a46a31b99794f705cbf0e50
> blob - f7736221c3c02ffd4e1d422bbe6510131dfc9e74
> blob + 708d939b13209d4284366e2f64c33fc1cf476ae5
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -3119,21 +3119,21 @@ got_privsep_send_object_idlist(struct imsgbuf *ibuf,
>  {
>  	const struct got_error *err = NULL;
>  	struct got_object_id *idlist[GOT_IMSG_OBJ_ID_LIST_MAX_NIDS];
> -	int i, j = 0;
> +	int i, queued = 0;
>  
>  	for (i = 0; i < nids; i++) {
> -		j = i % nitems(idlist);
> -		idlist[j] = ids[i];
> -		if (j >= nitems(idlist) - 1) {
> -			err = send_idlist(ibuf, idlist, j + 1);
> +		idlist[i % nitems(idlist)] = ids[i];
> +		queued++;
> +		if (queued >= nitems(idlist) - 1) {
> +			err = send_idlist(ibuf, idlist, queued);
>  			if (err)
>  				return err;
> -			j = 0;
> +			queued = 0;
>  		}
>  	}
>  
> -	if (j > 0) {
> -		err = send_idlist(ibuf, idlist, j + 1);
> +	if (queued > 0) {
> +		err = send_idlist(ibuf, idlist, queued);
>  		if (err)
>  			return err;
>  	}
> blob - 4b46e10d580fcd5a865ba785169787d0463eb2b0
> blob + c852d54f14a6f88d30757457e124d1ba29d63063
> --- libexec/got-read-pack/got-read-pack.c
> +++ libexec/got-read-pack/got-read-pack.c
> @@ -1388,6 +1388,11 @@ enumeration_request(struct imsg *imsg, struct imsgbuf 
>  	if (err)
>  		goto done;
>  
> +	if (STAILQ_EMPTY(&commit_ids)) {
> +		err = got_error(GOT_ERR_PRIVSEP_MSG);
> +		goto done;
> +	}
> +
>  	err = recv_object_ids(idset, ibuf);
>  	if (err)
>  		goto done;
> 

-- 

Tracey Emery