From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 12 Jul 2018 16:06:02 GMT
Subject: Re: [PATCH v3 06/24] multi-pack-index: load into memory
Message-ID: <1fb87ff0-5315-d96e-4671-433311746c31@gmail.com>
In-Reply-To: <xmqq36wsdy75.fsf@gitster-ct.c.googlers.com>

On 7/9/2018 3:08 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>> diff --git a/midx.h b/midx.h
>> index dbdbe9f873..2d83dd9ec1 100644
>> --- a/midx.h
>> +++ b/midx.h
>> @@ -1,6 +1,10 @@
>>   #ifndef __MIDX_H__
>>   #define __MIDX_H__
>>   
>> +struct multi_pack_index;
> I actually was quite surprised that this struct is defined in
> object-store.h and not here.  It feels the other way around.
>
> The raw_object_store needs to know that such an in-core structure
> might exist as an optional feature in an object store, but as an
> optional feature, I suspect that it has a pointer to an instance of
> multi_pack_index, instead of embedding the struct itself in it, so I
> would have expected to see an "I am only telling you that there is a
> struct with this name, but I am leaving it opaque as you do not have
> any business looking inside the struct yourself.  You only need to
> be aware of the type's existence and a pointer to it so that you can
> call helpers that know what's inside and that should be sufficient
> for your needs." decl like this in object-store.h and instead an
> actual implementation in here.

I thought it natural to include the struct definition next to the 
definition for 'struct packed_git', but I like your separation of 
concerns. Perhaps we could move the packed_git definition to packfile.h 
as well (separately). Of course, that sounds like an unnecessary churn.

Thanks,

-Stolee