From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Wed, 16 May 2012 07:09:32 GMT
Subject: Re: [PATCH v2] pack-objects: use streaming interface for reading large loose blobs
Message-ID: <CACsJy8DzdFORUMy7p_eVotr=HdkMX10uXy25H=05TBDjOmi4yw@mail.gmail.com>
In-Reply-To: <7vhavhforl.fsf@alter.siamese.dyndns.org>

On Tue, May 15, 2012 at 10:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Tue, May 15, 2012 at 2:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> git usually streams large blobs directly to packs. But there are cases
>>>> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
>>>> index 55ed955..7fbd2e1 100755
>>>> --- a/t/t1050-large.sh
>>>> +++ b/t/t1050-large.sh
>>>> @@ -134,6 +134,22 @@ test_expect_success 'repack' '
>>>>       git repack -ad
>>>>  '
>>>>
>>>> +test_expect_success 'pack-objects with large loose object' '
>>>> +     echo Z | dd of=large4 bs=1k seek=2000 &&
>>>> +     OBJ=9f36d94e145816ec642592c09cc8e601d83af157 &&
>>>> +     P=.git/objects/9f/36d94e145816ec642592c09cc8e601d83af157 &&
>>>
>>> I do not think you need these hardcoded constants; you will run
>>> hash-object later, no?
>>>
>>> Also, relying on $P to exist after hash-object -w returns is somewhat
>>> flaky, no?
>>
>> I need it to be a loose object to test this code path.
>
> No you don't.  You only need it to be something istream_read() will read
> from, iow, it could come from a base representation in a packfile.

No, an in-pack object will set to_reuse to 1, which goes a completely
different code path. write_large_blob_data() is only called when
to_reuse == 0.

>>> In any case, the patch when applied on top of cd07cc5 (Update draft
>>> release notes to 1.7.11 (11th batch), 2012-05-11) does not pass this part
>>> of the test on my box.
>>
>> Interesting. It passes for me (same base). I assume rm failed?
>
> No, reading the resulting pack dies with an error message that says the
> object could not be read at offset 12, implying that the pack writer wrote
> something bogus.

I'm still unable to reproduce that. But I think I found the problem.
In streaming code path, I set datalen = <uncompressed size> but
write_object() returns "hdrlen + (wrong) datalen". Patches will come a
couple of hours from now.
-- 
Duy