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

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Jun 26, 2026 at 02:10:32PM +0200, Toon Claes wrote:
>> > 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.

>> > 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.

>> >  	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