Re: [PATCH] server-info: do not list unlinked packs

From: Jeff King <peff@peff.net>
Date: May 23, 2019, 10:24 UTC
Message-ID: <20190523102456.GA6583@sigill.intra.peff.net>
In-Reply-To: <20190523085959.4q76pokx2gy6wqq7@dcvr>

On Thu, May 23, 2019 at 08:59:59AM +0000, Eric Wong wrote:

Show 6 quoted lines

> > We never delete entries from the in-memory packed_git list; a reprepare
> > only adds to the list. You'd need to teach update_server_info() to
> > ignore packs which are no longer present (or switch to exec-ing a
> > separate update-server-info binary).
> 
> Ah, checking files_exists() and setting a bit seems sufficient.

Yes, though we do we even need to store the bit?

I.e.,

Show 18 quoted lines

> @@ -199,12 +200,16 @@ static void init_pack_info(const char *infofile, int force)
>  		 */
>  		if (!p->pack_local)
>  			continue;
> +		if (!file_exists(p->pack_name)) {
> +			p->pack_unlinked = 1;
> +			continue;
> +		}
>  		i++;
>  	}
>  	num_pack = i;
>  	info = xcalloc(num_pack, sizeof(struct pack_info *));
>  	for (i = 0, p = get_all_packs(the_repository); p; p = p->next) {
> -		if (!p->pack_local)
> +		if (!p->pack_local || p->pack_unlinked)
>  			continue;
>  		assert(i < num_pack);
>  		info[i] = xcalloc(1, sizeof(struct pack_info));

If we just check file_exists() in the second loop, then this is entirely local to update_server_info(). And other users of packed_git do not have to wonder who is responsible for setting that flag in the global list.

It does mean you'd over-allocate the array (and num_pack would have to be adjusted down to "i" after the second loop), but that's not a big deal. I do think the whole two-loop thing would be more readable if we simply grew it on the fly with ALLOC_GROW().

-Peff