On Thu, Jun 07, 2018 at 12:04:13PM -0700, Jonathan Tan wrote:
> 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:
> -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).
> - 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()).
> @@ -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
