threads / patch / 14660

patchindex-pack: correctly initialize appended objects

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

## tl;dr

15 messages between Jul 24, 2008 and Jul 26, 2008. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Johannes Schindelin· Jul 24, 2008, 17:32 UTC · lore
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.)
Show changes to index-pack.c +3 −0
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);
Björn Steinbrink· Jul 24, 2008, 18:07 UTC · re: Johannes Schindelin · lore

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

On 2008.07.24 18:32:00 +0100, Johannes Schindelin wrote:
Show 9 quoted lines
> 
> 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

Junio C Hamano· Jul 25, 2008, 05:21 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 17 quoted lines
> 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?

Show 13 quoted lines
> 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);
Johannes Schindelin· Jul 25, 2008, 10:24 UTC · re: Junio C Hamano · lore

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

Hi,
On Thu, 24 Jul 2008, Junio C Hamano wrote:
Show 6 quoted lines
> 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

Nicolas Pitre· Jul 25, 2008, 11:54 UTC · re: Johannes Schindelin · lore

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

On Fri, 25 Jul 2008, Johannes Schindelin wrote:
Show 17 quoted lines
> 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
Björn Steinbrink· Jul 25, 2008, 12:01 UTC · re: Nicolas Pitre · lore

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

On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:
Show 25 quoted lines
> 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
Nicolas Pitre· Jul 25, 2008, 12:24 UTC · re: Björn Steinbrink · lore

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

On Fri, 25 Jul 2008, Björn Steinbrink wrote:
Show 31 quoted lines
> 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
Junio C Hamano· Jul 25, 2008, 18:15 UTC · re: Nicolas Pitre · lore

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

Nicolas Pitre <nico@cam.org> writes:
Show 25 quoted lines
> 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>
Show changes to index-pack.c +4 −0
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);
Björn Steinbrink· Jul 25, 2008, 11:55 UTC · re: Junio C Hamano · lore

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

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
Johannes Schindelin· Jul 25, 2008, 13:15 UTC · re: Björn Steinbrink · lore

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

Hi,
On Fri, 25 Jul 2008, Björn Steinbrink wrote:
Show 14 quoted lines
> 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

Shawn O. Pearce· Jul 25, 2008, 16:42 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 22 quoted lines
> 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.
Björn Steinbrink· Jul 25, 2008, 17:13 UTC · re: Johannes Schindelin · lore

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(-)
Show changes to index-pack.c +6 −0
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
Shawn O. Pearce· Jul 25, 2008, 17:20 UTC · re: Björn Steinbrink · lore

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

Bjjjrn Steinbrink <B.Steinbrink@gmx.de> wrote:
Show 10 quoted lines
> 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>
Show 6 quoted lines
>     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.  :)
Show 6 quoted lines
>     > (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.

Show 17 quoted lines
> 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.
Johannes Schindelin· Jul 26, 2008, 03:04 UTC · re: Björn Steinbrink · lore

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

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

Nicolas Pitre· Jul 25, 2008, 11:48 UTC · re: Johannes Schindelin · lore

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

On Thu, 24 Jul 2008, Johannes Schindelin wrote:
Show 22 quoted lines
> 
> 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

← back to recent threads