From: Junio C Hamano <hidden> Date: 2016-08-11 17:46:18
Johannes Schindelin [off-list ref] writes:
From: Björn Steinbrink <redacted>
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 [off-list ref]
Hopefully-reviewed-and-signed-off-by: Nicolas Pitre [off-list ref],
--
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?
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:02
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
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:02
On Fri, 25 Jul 2008, Johannes Schindelin wrote:
Hi,
On Thu, 24 Jul 2008, Junio C Hamano wrote:
quoted
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?
quoted
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
From: Björn Steinbrink <hidden> Date: 2016-06-15 22:45:02
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
From: Björn Steinbrink <hidden> Date: 2016-06-15 22:45:02
On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:
On Fri, 25 Jul 2008, Johannes Schindelin wrote:
quoted
Hi,
On Thu, 24 Jul 2008, Junio C Hamano wrote:
quoted
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?
quoted
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
From: Nicolas Pitre <hidden> Date: 2016-06-15 22:45:02
On Fri, 25 Jul 2008, Björn Steinbrink wrote:
On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:
quoted
On Fri, 25 Jul 2008, Johannes Schindelin wrote:
quoted
Hi,
On Thu, 24 Jul 2008, Junio C Hamano wrote:
quoted
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?
quoted
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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:02
Hi,
On Fri, 25 Jul 2008, Björn Steinbrink wrote:
On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:
quoted
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().
quoted
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
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:02
Johannes Schindelin [off-list ref] wrote:
On Fri, 25 Jul 2008, Björn Steinbrink wrote:
quoted
On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:
quoted
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().
quoted
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.
From: Björn Steinbrink <hidden> Date: 2016-06-15 22:45:02
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 <redacted>
Acked-by: Nicolas Pitre <redacted>
---
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(-)
@@ -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);
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:02
Bjjjrn Steinbrink [off-list ref] 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 <redacted>
Acked-by: Nicolas Pitre <redacted>
Acked-by: Shawn O. Pearce <redacted>
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.
@@ -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);