Re: [PATCH 1/2] pack-bitmap: remove bitmap_git global variable

From: Jeff King <peff@peff.net>
Date: Jun 9, 2018, 06:04 UTC
Message-ID: <20180609060437.GA30224@sigill.intra.peff.net>
In-Reply-To: <d4d4fec265c1c53e14da465904d93e559ff2c17d.1528397984.git.jonathantanmy@google.com>

On Thu, Jun 07, 2018 at 12:04:13PM -0700, Jonathan Tan wrote:

Show 10 quoted lines

> Remove the bitmap_git global variable. Instead, generate on demand an
> instance of struct bitmap_index for code that needs to access it.
> 
> This allows us significant control over the lifetime of instances of
> struct bitmap_index. In particular, packs can now be closed without
> worrying if an unnecessarily long-lived "pack" field in struct
> bitmap_index still points to it.
> 
> The bitmap API is also clearer in that we need to first obtain a struct
> bitmap_index, then we use it.

I think this is the right direction, and overall it looks pretty good. There's one call that gave me pause:

Show 6 quoted lines

> -int prepare_bitmap_git(void)
> +struct bitmap_index *prepare_bitmap_git(void)
>  {
> -	if (bitmap_git.loaded)
> -		return 0;
> +	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));

This function used to be idempotent, so any code which wanted to use the global bitmap_git could call it "just in case". After your patch, it's not. I think it's probably OK, since such functions would generally now take a bitmap_git argument and use that (e.g., rebuild_existing_bitmaps works that way after your patch).

Show 8 quoted lines

> -	if (!open_pack_bitmap())
> -		return load_pack_bitmap();
> +	if (!open_pack_bitmap(bitmap_git) && !load_pack_bitmap(bitmap_git))
> +		return bitmap_git;
>  
> -	return -1;
> +	return NULL;
>  }

We probably need to free(bitmap_git) before returning NULL here (this is still in prepare_bitmap_git()).

Show 15 quoted lines

> @@ -662,12 +686,11 @@ int prepare_bitmap_walk(struct rev_info *revs)
>  	struct bitmap *wants_bitmap = NULL;
>  	struct bitmap *haves_bitmap = NULL;
>  
> -	if (!bitmap_git.loaded) {
> -		/* try to open a bitmapped pack, but don't parse it yet
> -		 * because we may not need to use it */
> -		if (open_pack_bitmap() < 0)
> -			return -1;
> -	}
> +	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
> +	/* try to open a bitmapped pack, but don't parse it yet
> +	 * because we may not need to use it */
> +	if (open_pack_bitmap(bitmap_git) < 0)
> +		return NULL;

Ditto here (and probably other error returns lower in the function, but I didn't go through it carefully).

-Peff