Re: [PATCH v2 5/9] pack-bitmap: introduce bitmap_walk_contains()
- From
Philip Oakley <philipoakley@iee.email>
- Date
- Oct 19, 2019, 20:15 UTC
- Message-ID
- <c1764a23-15d1-cbef-a675-0f4242bc41f4@iee.email>
- In-Reply-To
- <CAP8UFD2dLPguCnUhTu+F4SNPxFZ7YT5io0qXkVwvSNP-sA8Jxg@mail.gmail.com>
Hi Christian,
On 19/10/2019 19:55, Christian Couder wrote:
Show 30 quoted lines
> Hi Philip,
>
> On Sat, Oct 19, 2019 at 5:25 PM Philip Oakley <philipoakley@iee.email> wrote:
>> Hi Christian,
>> can I check one thing?
> Yeah, sure! Thanks for taking a look at my patches!
>
>> On 19/10/2019 11:35, Christian Couder wrote:
>>> +int bitmap_walk_contains(struct bitmap_index *bitmap_git,
>>> + struct bitmap *bitmap, const struct object_id *oid)
>>> +{
>>> + int idx;
>> Excuse my ignorance here...
>>
>> For the case on Windows (int/long 32 bit), is this return value
>> guaranteed to be less than 2GiB, i.e. not a memory offset?
>>
>> I'm just thinking ahead to the resolution of the 4GiB file limit issue
>> on Git-for-Windows (https://github.com/git-for-windows/git/pull/2179)
> I understand your concern, unfortunately, below we have:
>
> idx = bitmap_position(bitmap_git, oid);
>
> and bitmap_position() returns an int at least since 3ae5fa0768
> (pack-bitmap: remove bitmap_git global variable, 2018-06-07)
>
> So I think the fix would be much more involved than just changing the
> type of the idx variable. It would likely involve modifying
> bitmap_position(), and thus would probably best be addressed in a
> separate patch series.So, IIUC it is mem-sized, so I should at least note it and pay attention to it for my >4G series, which like you say is "much more involved than just"...
The patch to flip over all the affected locations is a bit humongous (big), plus it's a bit of a moving target...
Show 7 quoted lines
>>> + >>> + if (!bitmap) >>> + return 0; >>> + >>> + idx = bitmap_position(bitmap_git, oid); >>> + return idx >= 0 && bitmap_get(bitmap, idx); >>> +}
Philip