Thread (10 messages) 10 messages, 3 authors, 11d ago

Re: [PATCH 1/2] odb/source: generalize `reprepare()` callback

From: Toon Claes <hidden>
Date: 2026-06-30 08:18:41

Patrick Steinhardt [off-list ref] writes:
On Fri, Jun 26, 2026 at 02:10:32PM +0200, Toon Claes wrote:
quoted
quoted
diff --git a/builtin/grep.c b/builtin/grep.c
index 8080d1bf5e..7361bf071e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1361,10 +1360,8 @@ int cmd_grep(int argc,
 			struct odb_source *source;
 
 			odb_prepare_alternates(the_repository->objects);
-			for (source = the_repository->objects->sources; source; source = source->next) {
-				struct odb_source_files *files = odb_source_files_downcast(source);
-				odb_source_packed_prepare(files->packed);
+			for (source = the_repository->objects->sources; source; source = source->next)
+				odb_source_prepare(source, 0);
So you're downcasting inside the implementation by the backends itself.
That makes sense, but would it be worth to say something about that in
the commit message?
Hm. Would that provide much value? I'm probably quite a bit biased here,
but I think that it's implicit that the backends have to eventually cast
the generic structure to their own backend.

So I wouldn't really know how to clarify this. Did you have anything
specific in mind?
Ah, I'm sorry, I misread that. I thought you changed the vtable function
to do the downcasting, but you're simply changing from calling a 
`*_packed_*()` to the generic variant that goes through the vtable.

Anyhow, not worth mentioning in the commit message.
quoted
quoted
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 42c28fba0e..fa5a072488 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -15,7 +15,7 @@ static int find_pack_entry(struct odb_source_packed *store,
 {
 	struct packfile_list_entry *l;
 
-	odb_source_packed_prepare(store);
+	odb_source_prepare(&store->base, 0);
Why are you not using ODB_PREPARE_FLUSH_CACHES here? It used to do
before?
Because this was calling `odb_source_packed_prepare()` before, not
`odb_source_reprepare()`. So this was calling the non-flushing
variant.
Again, confusion on my end.
quoted
quoted
 	if (store->midx && fill_midx_entry(store->midx, oid, e))
 		return 1;
 
@@ -47,7 +47,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 	 * been added since the last time we have prepared the packfile store.
 	 */
 	if (flags & OBJECT_INFO_SECOND_READ)
-		odb_source_reprepare(source);
+		odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
I think the new code is correct, but why wasn't `packed` used here in
the past? The old odb_source_reprepare() expected a downcasted, didn't
it?
No, `odb_source_reprepare()` is the generic variant. The naming schema
is typically:

  - `odb_source_frobnicate()` for the generic variants, which receive a
    `struct odb_source` as input.

  - `odb_source_<type>_frobnitcate()` for their backend-specific
    implementations, which cast down the generic `struct odb_source` to
    their backend-specific struct.
Yeah, I understand things better now. Thanks for clarifying.

-- 
Cheers,
Toon
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help