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

Nguyen Thai Ngoc Duy [off-list ref] writes:

On Tue, May 15, 2012 at 2:43 AM, Junio C Hamano [off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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

quoted

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.

On Tue, May 15, 2012 at 10:27 PM, Junio C Hamano [off-list ref] wrote:

Nguyen Thai Ngoc Duy [off-list ref] writes:

quoted

On Tue, May 15, 2012 at 2:43 AM, Junio C Hamano [off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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.

quoted

quoted

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