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

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.

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