From: Bj

> From: Bj

Reading get_data_from_pack(), it does rely on hdr_size, idx.offset and

idx.offset of the next entry to be set correctly.  The function does not

seem to use type (which the patch is also setting) nor real_type (which

the patch does not set).
However, the code checks objects[nth].real_type all over the place in the

code.  Doesn't the lack of real_type assignment in append_obj_to_pack()

--

I had thought that resolve_delta() would set that, but it seems that we

never call that function like that. Hm...
Björn

--

Hi,
On Fri, 25 Jul 2008, Bj

When index-pack completes a thin pack it appends objects to the pack.

Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when

resolving deltas) such an object can be pruned in case of memory pressure.
To be able to re-read the object later, a few more fields have to be set.
Noticed by Pierre Habouzit.
Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>

Acked-by: Nicolas Pitre <nico@cam.org>

---
    On 2008.07.25 15:15:48 +0200, Johannes Schindelin wrote:

    > So, let's add the comment as Nico suggested, and set real_type,

    > too?
    OK, I hope the comment is what was expected. My lack of knowledge

    made we wonder what to write... :-/
    > (And it would be smashing if you could verify that the type is

    > indeed correctly set to non-delta...)
    Hm, we get the object via read_sha1_file, can that return a delta? I

    would not expect it to.  Sorry, never looked at those code paths

    (and don't have the time to investigate at the moment).
 index-pack.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/index-pack.c b/index-pack.c

index ac20a46..d757b07 100644

--- a/index-pack.c

+++ b/index-pack.c

@@ -699,6 +699,12 @@ static struct object_entry *append_obj_to_pack(

 	write_or_die(output_fd, header, n);

 	obj[0].idx.crc32 = crc32(0, Z_NULL, 0);

 	obj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);

+	// This object comes from outside the thin pack, so we need to

+	// initialize the size and type fields

+	obj[0].hdr_size = n;

+	obj[0].size = size;

+	obj[0].type = type;

+	obj[0].real_type = type;

 	obj[1].idx.offset = obj[0].idx.offset + n;

 	obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);

 	hashcpy(obj->idx.sha1, sha1);

--

1.6.0.rc0.14.g95f8.dirty

--

Hi,
On Fri, 25 Jul 2008, Bj

read_sha1_file() _never_ returns a delta.  It always reutrns the

whole object, even if the object was stored as a delta in a pack

somewhere.  The function is widely used within git to read an object

for processing, without the caller needing to worry about the types
--

Shawn.

--

The patch looks correct, but it should set real_type too because

I'm pretty sure we use that when we unpack the delta base again if

it was pruned out of memory.
--

Shawn.

--

Hi,
From staring at the code, I thought that real_type was set in

resolve_delta(), but I may be wrong.
The safer thing would be to set it, but I am not quite sure if we can use

"type" directly, or if type can be "delta" for an object that is used to

complete the pack, and therefore stored as a non-delta.
Ciao,

Dscho

--

Objects to complete the pack are always non delta, so the type and

real_type should be the same.  However that shouldn't matter since at

that point the object array is not walked anymore, at least not for

appended objects, and therefore initializing the type at that point is

redundant.
Nicolas

--

Thanks.  Here is what I committed.
commit 72de2883bd7d4ceda05f107826c7607c594de965

Author: Björn Steinbrink <B.Steinbrink@gmx.de>

Date:   Thu Jul 24 18:32:00 2008 +0100
    index-pack.c: correctly initialize appended objects
    When index-pack completes a thin pack it appends objects to the pack.

    Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when

    resolving deltas) such an object can be pruned in case of memory

    pressure, and will be read back again by get_data_from_pack().  For this

    to work, the fields in object_entry structure need to be initialized

    properly.
    Noticed by Pierre Habouzit.
    Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>

    Acked-by: Nicolas Pitre <nico@cam.org>

    Acked-by: Shawn O. Pearce <spearce@spearce.org>

    Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/index-pack.c b/index-pack.c

index c359f8c..7d5344a 100644

--- a/index-pack.c

+++ b/index-pack.c

@@ -698,6 +698,10 @@ static struct object_entry *append_obj_to_pack(

 	write_or_die(output_fd, header, n);

 	obj[0].idx.crc32 = crc32(0, Z_NULL, 0);

 	obj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);

+	obj[0].size = size;

+	obj[0].hdr_size = n;

+	obj[0].type = type;

+	obj[0].real_type = type;

 	obj[1].idx.offset = obj[0].idx.offset + n;

 	obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);

 	hashcpy(obj->idx.sha1, sha1);

--

Is that still true when the object has been pruned due to memory

constraints set by deltaBaseCacheLimit? AFAICT when reloading the data

for the object, we end up in get_base_data, which at least checks

obj->type.
Björn

--

On Fri, 25 Jul 2008, Bj

Ah, thanks a lot! I tried to come up with a sane commit message

yesterday but totally failed, and then after a night of sneezing, I had

forgotten about it. And I wouldn't have managed to write an equally good
Sure!
Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
Thanks,

Björn

--