Re: [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps

[Justin Tobler <jltobler@gmail.com>]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> When opening a bitmap for a repository we perform two steps:
> 
>   - We first look for a multi-pack index bitmap in any of the object
>     sources connected to the repository.
> 
>   - We then look for a packfile bitmap in any of the packfiles of any of
>     the object sources.

So IIUC, we generally stop searching for a bitmap once we find one.

> Both of these steps thus iterate through object sources themselves, one
> via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
> layout makes it hard to introduce a way to open the bitmap of one
> specific object source, which is functionality that we'll require in a
> subsequent commit.
> 
> Reverse the loop so that we instead loop through all sources in the
> outer loop, and then for each source we try to load its bitmap via
> either the multi-pack index or via a packfile.

Conceptually, I think this is a lot easier to follow too which is nice.

> Note that this changes the precedence of bitmaps in one specific edge
> case: when an earlier object source only has a packfile bitmap, but a
> later source has a multi-pack index bitmap, we now pick the packfile
> bitmap of the earlier source. Previously, a multi-pack index bitmap from
> any source would have taken precedence over all packfile bitmaps. Given
> that object sources are ordered such that the local source comes first,
> this arguably is an improvement, as we now prefer local bitmaps over
> bitmaps in alternates. Furthermore, we already warn about repositories
> that have multiple bitmaps, so this setup is broken and thus arguably
> not worth worrying about too much.

I agree that the change in bitmap precedent is probably not a big deal.
Having multiple bitmaps in a repository is already something we warn
against so I think this should be fine.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pack-bitmap.c | 65 ++++++++++++++++++++++++++---------------------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index eda38a5433..0e3e18a557 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
>  	return 0;
>  }
>  
> -static int open_pack_bitmap(struct repository *r,
> -			    struct bitmap_index *bitmap_git)
> +static int open_bitmap_for_source(struct odb_source_packed *source,
> +				  struct bitmap_index *bitmap_git)
>  {
> -	struct packed_git *p;
> +	struct multi_pack_index *midx = get_multi_pack_index(source);
> +	struct packfile_list_entry *e;
>  	int ret = -1;
>  
> -	repo_for_each_pack(r, p) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
> -			ret = 0;
> -			/*
> -			 * The only reason to keep looking is to report
> -			 * duplicates.
> -			 */
> -			if (!trace2_is_enabled())
> -				break;
> -		}
> +	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> +		ret = 0;

Ok, open_midx_bitmap_1() returns 0 if it find a MIDX and -1 otherwise.
Probably just a matter of preference, but I think writing out like below
is a little bit easier on the eyes:

  if (midx)
    ret = open_midx_bitmap_1(bitmap_git, midx);

it might just be that I find the return values a bit confusing though.
Maybe we could instead use `found` like a bit later in this patch.

> +
> +	for (e = packfile_store_get_packs(source); e; e = e->next) {
> +		/*
> +		 * When tracing is enabled we want to keep looking to report
> +		 * duplicates even if we have already found a bitmap.
> +		 */
> +		if (!ret && !trace2_is_enabled())
> +			break;

So if have already found a bitmap from the MIDX and tracing is not
enabled, we don't continue searching for bitmaps in this source. 

> +
> +		if (open_pack_bitmap_1(bitmap_git, e->pack))
> +			continue;
> +		ret = 0;
>  	}
>  
>  	return ret;
>  }
>  
> -static int open_midx_bitmap(struct repository *r,
> -			    struct bitmap_index *bitmap_git)
> +static int open_bitmap(struct repository *r,
> +		       struct bitmap_index *bitmap_git)
>  {
>  	struct odb_source *source;
> -	int ret = -1;
> +	int found = 0;
>  
>  	assert(!bitmap_git->map);
>  
>  	odb_prepare_alternates(r->objects);
>  	for (source = r->objects->sources; source; source = source->next) {
>  		struct odb_source_files *files = odb_source_files_downcast(source);
> -		struct multi_pack_index *midx = get_multi_pack_index(files->packed);
> -		if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> -			ret = 0;
> -	}
> -	return ret;
> -}
> -
> -static int open_bitmap(struct repository *r,
> -		       struct bitmap_index *bitmap_git)
> -{
> -	int found;
>  
> -	assert(!bitmap_git->map);
> +		found |= !open_bitmap_for_source(files->packed, bitmap_git);
>  
> -	found = !open_midx_bitmap(r, bitmap_git);
> -
> -	/*
> -	 * these will all be skipped if we opened a midx bitmap; but run it
> -	 * anyway if tracing is enabled to report the duplicates
> -	 */
> -	if (!found || trace2_is_enabled())
> -		found |= !open_pack_bitmap(r, bitmap_git);
> +		/*
> +		 * The only reason to keep looking after having found a bitmap
> +		 * is to report duplicates.
> +		 */
> +		if (found && !trace2_is_enabled())
> +			break;
> +	}

Ok, we only advance to the next source if tracing is enabled to print
warnings for multiple bitmaps. Makes sense.

Overall I quite like the direction of this patch.

-Justin