[PATCH] index-pack: correctly initialize appended objects

[PATCH] index-pack: correctly initialize appended objects

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1259 bytes --]


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

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.

Hopefully-signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
Hopefully-reviewed-and-signed-off-by: Nicolas Pitre <nico@cam.org>, 

--

	This was probably missed in the flurry of patches, scratched 
	patches, and new patches.

	Nico could you have a quick look?  (I would ask Shawn, but I know 
	that he is pretty busy with real world issues.)

diff --git a/index-pack.c b/index-pack.c
index ac20a46..33ba8ef 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -699,6 +699,9 @@ 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].hdr_size = n;
+	obj[0].type = type;
+	obj[0].size = size;
 	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);

Re: [PATCH] index-pack: correctly initialize appended objects

[Björn Steinbrink]

On 2008.07.24 18:32:00 +0100, Johannes Schindelin wrote:
> 
> From: Björn Steinbrink <B.Steinbrink@gmx.de>
> 
> 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.

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
message anyway ;-)

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

Sure!

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

Thanks,
Björn

Re: [PATCH] index-pack: correctly initialize appended objects

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> From: Björn Steinbrink <B.Steinbrink@gmx.de>
>
> 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.
>
> Hopefully-signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> Hopefully-reviewed-and-signed-off-by: Nicolas Pitre <nico@cam.org>, 
>
> --
> 	Nico could you have a quick look?  (I would ask Shawn, but I know 
> 	that he is pretty busy with real world issues.)

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()
affect them in  any way?

> diff --git a/index-pack.c b/index-pack.c
> index ac20a46..33ba8ef 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -699,6 +699,9 @@ 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].hdr_size = n;
> +	obj[0].type = type;
> +	obj[0].size = size;
>  	obj[1].idx.offset = obj[0].idx.offset + n;
>  	obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);

Re: [PATCH] index-pack: correctly initialize appended objects

[Johannes Schindelin]

Hi,

On Thu, 24 Jul 2008, Junio C Hamano wrote:

> 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() affect them in any way?

>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

Re: [PATCH] index-pack: correctly initialize appended objects

[Nicolas Pitre]

On Fri, 25 Jul 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Thu, 24 Jul 2008, Junio C Hamano wrote:
> 
> > 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() affect them in any way?
> 
> >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.

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

Re: [PATCH] index-pack: correctly initialize appended objects

[Björn Steinbrink]

On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:
> On Fri, 25 Jul 2008, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Thu, 24 Jul 2008, Junio C Hamano wrote:
> > 
> > > 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() affect them in any way?
> > 
> > >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.
> 
> 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.

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

Re: [PATCH] index-pack: correctly initialize appended objects

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1618 bytes --]

On Fri, 25 Jul 2008, Björn Steinbrink wrote:

> On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:
> > On Fri, 25 Jul 2008, Johannes Schindelin wrote:
> > 
> > > Hi,
> > > 
> > > On Thu, 24 Jul 2008, Junio C Hamano wrote:
> > > 
> > > > 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() affect them in any way?
> > > 
> > > >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.
> > 
> > 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.
> 
> 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.

yeah, true.  I don't really have this new code path in my head yet.

In any case, appended objects should have type = real_type = non delta 
type.


Nicolas

Re: [PATCH] index-pack: correctly initialize appended objects

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Fri, 25 Jul 2008, Johannes Schindelin wrote:
>
>> Hi,
>> 
>> On Thu, 24 Jul 2008, Junio C Hamano wrote:
>> 
>> > 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() affect them in any way?
>> 
>> >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.
>
> 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.

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);

Re: [PATCH] index-pack: correctly initialize appended objects

[Björn Steinbrink]

On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:
> 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).

type is used in get_base_data().

> 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()
> affect them in  any way?

I had thought that resolve_delta() would set that, but it seems that we
never call that function like that. Hm...

Björn

Re: [PATCH] index-pack: correctly initialize appended objects

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1011 bytes --]

Hi,

On Fri, 25 Jul 2008, Björn Steinbrink wrote:

> On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:
> > 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).
> 
> type is used in get_base_data().
> 
> > 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() affect them in any way?
> 
> I had thought that resolve_delta() would set that, but it seems that we 
> never call that function like that. Hm...

So, let's add the comment as Nico suggested, and set real_type, too?  (And 
it would be smashing if you could verify that the type is indeed correctly 
set to non-delta...)

I think that setting real_type is necessary to have less surprises when 
the code is extended in the future.

Thanks,
Dscho

Re: [PATCH] index-pack: correctly initialize appended objects

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Fri, 25 Jul 2008, Björn Steinbrink wrote:
> > On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:
> > > 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).
> > 
> > type is used in get_base_data().
> > 
> > > 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() affect them in any way?
> > 
> > I had thought that resolve_delta() would set that, but it seems that we 
> > never call that function like that. Hm...
> 
> So, let's add the comment as Nico suggested, and set real_type, too?  (And 
> it would be smashing if you could verify that the type is indeed correctly 
> set to non-delta...)
> 
> I think that setting real_type is necessary to have less surprises when 
> the code is extended in the future.

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.

[PATCH] index-pack: correctly initialize appended objects

[Björn Steinbrink]

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

Re: [PATCH] index-pack: correctly initialize appended objects

[Shawn O. Pearce]

Bjjjrn Steinbrink <B.Steinbrink@gmx.de> wrote:
> 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>

Acked-by: Shawn O. Pearce <spearce@spearce.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... :-/

The commit message makes sense to me.  :)
 
>     > (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).

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
of compression used to store the object.


> 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);

-- 
Shawn.

Re: [PATCH] index-pack: correctly initialize appended objects

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 213 bytes --]

Hi,

On Fri, 25 Jul 2008, Björn Steinbrink wrote:

> +	// This object comes from outside the thin pack, so we need to
> +	// initialize the size and type fields

Do not use C++ comments in Git.  Ever.

Ciao,
Dscho

Re: [PATCH] index-pack: correctly initialize appended objects

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1140 bytes --]

On Thu, 24 Jul 2008, Johannes Schindelin wrote:

> 
> From: Björn Steinbrink <B.Steinbrink@gmx.de>
> 
> 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.
> 
> Hopefully-signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> Hopefully-reviewed-and-signed-off-by: Nicolas Pitre <nico@cam.org>, 
> 
> --
> 
> 	This was probably missed in the flurry of patches, scratched 
> 	patches, and new patches.
> 
> 	Nico could you have a quick look?  (I would ask Shawn, but I know 
> 	that he is pretty busy with real world issues.)

sorry, I have intermitant connectivity this week, and I'll be off the 
net for two weeks after that.

Yes, this looks fine, although I'd add a comment mentioning that those 
extra fields are uninitialized in the thin pack case when objects are 
appended to the pack since they're already initialized otherwise.

ACK.


Nicolas