diff mbox series

[10/11] pack-bitmap.c: record whether the result was filtered

Message ID dffdd794de3255719028a5f2c64d43048a5a5f60.1728505840.git.me@ttaylorr.com (mailing list archive)
State New
Headers show
Series pack-bitmap: convert offset to ref deltas where possible | expand

Commit Message

Taylor Blau Oct. 9, 2024, 8:31 p.m. UTC
In a following commit, we will prepare to mark objects for reuse which
are stored as deltas, but whose base object wasn't included in the
output pack (e.g., because the client already has it).

We can't, however, take advantage of that optimization when the
traversal removed objects from the result due to either (a) object
filtering, or (b) --unpacked.

That's because in either case, we can't rely on the principle that "if
an object appears in the 'haves' bitmap, that the client already has
that object", since they may not have it as a result of filtering.

Keep track of whether or not we performed any object filtering during
our traversal in preparation to actually implement thin delta
conversion.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jeff King Oct. 11, 2024, 8:35 a.m. UTC | #1
On Wed, Oct 09, 2024 at 04:31:31PM -0400, Taylor Blau wrote:

> In a following commit, we will prepare to mark objects for reuse which
> are stored as deltas, but whose base object wasn't included in the
> output pack (e.g., because the client already has it).
> 
> We can't, however, take advantage of that optimization when the
> traversal removed objects from the result due to either (a) object
> filtering, or (b) --unpacked.
> 
> That's because in either case, we can't rely on the principle that "if
> an object appears in the 'haves' bitmap, that the client already has
> that object", since they may not have it as a result of filtering.

I think this is a reasonable precaution, though it does make me wonder
if the non-reuse code paths are so careful. That is, if we have object Y
which is a delta against object X, but we know the other side _could_
have X (because it's reachable from some commit they claim to have),
would we ever send a thin delta against X?

I don't recall seeing any protections for that, though I also wouldn't
be too surprised if it somehow just works out because we never figure
out whether they have X or not. :-/

I wonder, though: should thin deltas just be turned off entirely when
filtering is in play?

Likewise for --unpacked (though in practice I think it would never be
used with --thin, as it is about generating new on-disk packs).

-Peff
diff mbox series

Patch

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 8326a20979e..67ea267ed2a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -112,6 +112,12 @@  struct bitmap_index {
 	/* "have" bitmap from the last performed walk */
 	struct bitmap *haves;
 
+	/*
+	 * Whether the last performed walk had objects removed from
+	 * 'result' due to object filtering.
+	 */
+	int filtered;
+
 	/* Version of the bitmap index */
 	unsigned int version;
 };
@@ -1684,6 +1690,8 @@  static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,
 			bitmap_unset(to_filter, pos);
 	}
 
+	bitmap_git->filtered = 1;
+
 	bitmap_free(tips);
 }
 
@@ -1776,6 +1784,8 @@  static void filter_bitmap_blob_limit(struct bitmap_index *bitmap_git,
 			bitmap_unset(to_filter, pos);
 	}
 
+	bitmap_git->filtered = 1;
+
 	bitmap_free(tips);
 }
 
@@ -1892,6 +1902,8 @@  static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 		if (has_object_pack(&eindex->objects[i]->oid))
 			bitmap_unset(result, objects_nr + i);
 	}
+
+	bitmap_git->filtered = 1;
 }
 
 struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,