Re: [PATCH v2] pack-objects: use streaming interface for reading large loose blobs

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2012, 15:27 UTC
Message-ID: <7vhavhforl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8Bk5=X8hQAHKXCArfq57o2tLBv0RN641N3-ws69S3GghQ@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 24 quoted lines

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

In short, what I was hinting at was to do something like this instead:

	create a large file "large4"
        BLOB=$(git hash-objects -w "large4")
        PACK=$(echo "$BLOB" | git pack-objects pack)
        create a new empty repository "check"
        move pack-$PACK.{pack,idx} to "check" repository
        in "check" repository, try to see if you can read $BLOB correctly

Show 5 quoted lines

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