{"thread":{"id":"14660","subject":"[PATCH] index-pack: correctly initialize appended objects","startedAt":"2008-07-24T17:32:00Z","lastAt":"2008-07-26T03:04:08Z","messageCount":15,"participants":["Johannes Schindelin","Björn Steinbrink","Nicolas Pitre","Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"84754","messageId":"alpine.DEB.1.00.0807241821440.8986@racer","threadId":"14660","inReplyTo":null,"subject":"[PATCH] index-pack: correctly initialize appended objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-24T17:32:00Z","receivedAt":"2008-07-24T17:32:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nFrom: Björn Steinbrink \n\nWhen index-pack completes a thin pack it appends objects to the pack. \nSince the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when \nresolving deltas) such an object can be pruned in case of memory\npressure.\n\nTo be able to re-read the object later, a few more fields have to be set.\n\nNoticed by Pierre Habouzit.\n\nHopefully-signed-off-by: Björn Steinbrink \nHopefully-reviewed-and-signed-off-by: Nicolas Pitre , \n\n--\n\n\tThis was probably missed in the flurry of patches, scratched \n\tpatches, and new patches.\n\n\tNico could you have a quick look? (I would ask Shawn, but I know \n\tthat he is pretty busy with real world issues.)\n\ndiff --git a/index-pack.c b/index-pack.c\nindex ac20a46..33ba8ef 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -699,6 +699,9 @@ static struct object_entry *append_obj_to_pack(\n \twrite_or_die(output_fd, header, n);\n \tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n \tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n+\tobj[0].hdr_size = n;\n+\tobj[0].type = type;\n+\tobj[0].size = size;\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n \tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n \thashcpy(obj->idx.sha1, sha1);"},{"id":"84761","messageId":"20080724180710.GA26088@atjola.homenet","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807241821440.8986@racer","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-07-24T18:07:10Z","receivedAt":"2008-07-24T18:07:10Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.07.24 18:32:00 +0100, Johannes Schindelin wrote:\n> \n> From: Björn Steinbrink \n> \n> When index-pack completes a thin pack it appends objects to the pack. \n> Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when \n> resolving deltas) such an object can be pruned in case of memory\n> pressure.\n> \n> To be able to re-read the object later, a few more fields have to be set.\n\nAh, thanks a lot! I tried to come up with a sane commit message\nyesterday but totally failed, and then after a night of sneezing, I had\nforgotten about it. And I wouldn't have managed to write an equally good\nmessage anyway ;-)\n\n> Noticed by Pierre Habouzit.\n> \n> Hopefully-signed-off-by: Björn Steinbrink \n\nSure!\n\nSigned-off-by: Björn Steinbrink \n\nThanks,\nBjörn\n"},{"id":"293740","messageId":"7vy73q4jzp.fsf@gitster.siamese.dyndns.org","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807241821440.8986@racer","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-25T05:21:14Z","receivedAt":"2008-07-25T05:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Björn Steinbrink \n>\n> When index-pack completes a thin pack it appends objects to the pack. \n> Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when \n> resolving deltas) such an object can be pruned in case of memory\n> pressure.\n>\n> To be able to re-read the object later, a few more fields have to be set.\n>\n> Noticed by Pierre Habouzit.\n>\n> Hopefully-signed-off-by: Björn Steinbrink \n> Hopefully-reviewed-and-signed-off-by: Nicolas Pitre , \n>\n> --\n> \tNico could you have a quick look? (I would ask Shawn, but I know \n> \tthat he is pretty busy with real world issues.)\n\nReading get_data_from_pack(), it does rely on hdr_size, idx.offset and\nidx.offset of the next entry to be set correctly. The function does not\nseem to use type (which the patch is also setting) nor real_type (which\nthe patch does not set).\n\nHowever, the code checks objects[nth].real_type all over the place in the\ncode. Doesn't the lack of real_type assignment in append_obj_to_pack()\naffect them in any way?\n\n> diff --git a/index-pack.c b/index-pack.c\n> index ac20a46..33ba8ef 100644\n> --- a/index-pack.c\n> +++ b/index-pack.c\n> @@ -699,6 +699,9 @@ static struct object_entry *append_obj_to_pack(\n> \twrite_or_die(output_fd, header, n);\n> \tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n> \tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n> +\tobj[0].hdr_size = n;\n> +\tobj[0].type = type;\n> +\tobj[0].size = size;\n> \tobj[1].idx.offset = obj[0].idx.offset + n;\n> \tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n"},{"id":"84856","messageId":"alpine.DEB.1.00.0807251219530.11976@eeepc-johanness","threadId":"14660","inReplyTo":"7vy73q4jzp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-25T10:24:47Z","receivedAt":"2008-07-25T10:24:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 24 Jul 2008, Junio C Hamano wrote:\n\n> The function does not seem to use type (which the patch is also setting) \n> nor real_type (which the patch does not set).\n> \n> However, the code checks objects[nth].real_type all over the place in \n> the code. Doesn't the lack of real_type assignment in \n> append_obj_to_pack() affect them in any way?\n\n>From staring at the code, I thought that real_type was set in \nresolve_delta(), but I may be wrong.\n\nThe safer thing would be to set it, but I am not quite sure if we can use \n\"type\" directly, or if type can be \"delta\" for an object that is used to \ncomplete the pack, and therefore stored as a non-delta.\n\nCiao,\nDscho\n"},{"id":"84869","messageId":"alpine.LFD.1.10.0807250742120.9968@xanadu.home","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807241821440.8986@racer","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-25T11:48:24Z","receivedAt":"2008-07-25T11:48:24Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 24 Jul 2008, Johannes Schindelin wrote:\n\n> \n> From: Björn Steinbrink \n> \n> When index-pack completes a thin pack it appends objects to the pack. \n> Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when \n> resolving deltas) such an object can be pruned in case of memory\n> pressure.\n> \n> To be able to re-read the object later, a few more fields have to be set.\n> \n> Noticed by Pierre Habouzit.\n> \n> Hopefully-signed-off-by: Björn Steinbrink \n> Hopefully-reviewed-and-signed-off-by: Nicolas Pitre , \n> \n> --\n> \n> \tThis was probably missed in the flurry of patches, scratched \n> \tpatches, and new patches.\n> \n> \tNico could you have a quick look? (I would ask Shawn, but I know \n> \tthat he is pretty busy with real world issues.)\n\nsorry, I have intermitant connectivity this week, and I'll be off the \nnet for two weeks after that.\n\nYes, this looks fine, although I'd add a comment mentioning that those \nextra fields are uninitialized in the thin pack case when objects are \nappended to the pack since they're already initialized otherwise.\n\nACK.\n\n\nNicolas\n"},{"id":"84872","messageId":"alpine.LFD.1.10.0807250751220.9968@xanadu.home","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807251219530.11976@eeepc-johanness","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-25T11:54:49Z","receivedAt":"2008-07-25T11:54:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 25 Jul 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Thu, 24 Jul 2008, Junio C Hamano wrote:\n> \n> > The function does not seem to use type (which the patch is also setting) \n> > nor real_type (which the patch does not set).\n> > \n> > However, the code checks objects[nth].real_type all over the place in \n> > the code. Doesn't the lack of real_type assignment in \n> > append_obj_to_pack() affect them in any way?\n> \n> >From staring at the code, I thought that real_type was set in \n> resolve_delta(), but I may be wrong.\n> \n> The safer thing would be to set it, but I am not quite sure if we can use \n> \"type\" directly, or if type can be \"delta\" for an object that is used to \n> complete the pack, and therefore stored as a non-delta.\n\nObjects to complete the pack are always non delta, so the type and \nreal_type should be the same. However that shouldn't matter since at \nthat point the object array is not walked anymore, at least not for \nappended objects, and therefore initializing the type at that point is \nredundant.\n\n\nNicolas\n"},{"id":"84873","messageId":"20080725115519.GA32487@atjola.homenet","threadId":"14660","inReplyTo":"7vy73q4jzp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-07-25T11:55:19Z","receivedAt":"2008-07-25T11:55:19Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:\n> Reading get_data_from_pack(), it does rely on hdr_size, idx.offset and\n> idx.offset of the next entry to be set correctly. The function does not\n> seem to use type (which the patch is also setting) nor real_type (which\n> the patch does not set).\n\ntype is used in get_base_data().\n\n> However, the code checks objects[nth].real_type all over the place in the\n> code. Doesn't the lack of real_type assignment in append_obj_to_pack()\n> affect them in any way?\n\nI had thought that resolve_delta() would set that, but it seems that we\nnever call that function like that. Hm...\n\nBjörn\n"},{"id":"84874","messageId":"20080725120102.GB32487@atjola.homenet","threadId":"14660","inReplyTo":"alpine.LFD.1.10.0807250751220.9968@xanadu.home","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-07-25T12:01:02Z","receivedAt":"2008-07-25T12:01:02Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:\n> On Fri, 25 Jul 2008, Johannes Schindelin wrote:\n> \n> > Hi,\n> > \n> > On Thu, 24 Jul 2008, Junio C Hamano wrote:\n> > \n> > > The function does not seem to use type (which the patch is also setting) \n> > > nor real_type (which the patch does not set).\n> > > \n> > > However, the code checks objects[nth].real_type all over the place in \n> > > the code. Doesn't the lack of real_type assignment in \n> > > append_obj_to_pack() affect them in any way?\n> > \n> > >From staring at the code, I thought that real_type was set in \n> > resolve_delta(), but I may be wrong.\n> > \n> > The safer thing would be to set it, but I am not quite sure if we can use \n> > \"type\" directly, or if type can be \"delta\" for an object that is used to \n> > complete the pack, and therefore stored as a non-delta.\n> \n> Objects to complete the pack are always non delta, so the type and \n> real_type should be the same. However that shouldn't matter since at \n> that point the object array is not walked anymore, at least not for \n> appended objects, and therefore initializing the type at that point is \n> redundant.\n\nIs that still true when the object has been pruned due to memory\nconstraints set by deltaBaseCacheLimit? AFAICT when reloading the data\nfor the object, we end up in get_base_data, which at least checks\nobj->type.\n\nBjörn\n"},{"id":"84875","messageId":"alpine.LFD.1.10.0807250819100.9968@xanadu.home","threadId":"14660","inReplyTo":"20080725120102.GB32487@atjola.homenet","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-25T12:24:51Z","receivedAt":"2008-07-25T12:24:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 25 Jul 2008, Björn Steinbrink wrote:\n\n> On 2008.07.25 07:54:49 -0400, Nicolas Pitre wrote:\n> > On Fri, 25 Jul 2008, Johannes Schindelin wrote:\n> > \n> > > Hi,\n> > > \n> > > On Thu, 24 Jul 2008, Junio C Hamano wrote:\n> > > \n> > > > The function does not seem to use type (which the patch is also setting) \n> > > > nor real_type (which the patch does not set).\n> > > > \n> > > > However, the code checks objects[nth].real_type all over the place in \n> > > > the code. Doesn't the lack of real_type assignment in \n> > > > append_obj_to_pack() affect them in any way?\n> > > \n> > > >From staring at the code, I thought that real_type was set in \n> > > resolve_delta(), but I may be wrong.\n> > > \n> > > The safer thing would be to set it, but I am not quite sure if we can use \n> > > \"type\" directly, or if type can be \"delta\" for an object that is used to \n> > > complete the pack, and therefore stored as a non-delta.\n> > \n> > Objects to complete the pack are always non delta, so the type and \n> > real_type should be the same. However that shouldn't matter since at \n> > that point the object array is not walked anymore, at least not for \n> > appended objects, and therefore initializing the type at that point is \n> > redundant.\n> \n> Is that still true when the object has been pruned due to memory\n> constraints set by deltaBaseCacheLimit? AFAICT when reloading the data\n> for the object, we end up in get_base_data, which at least checks\n> obj->type.\n\nyeah, true. I don't really have this new code path in my head yet.\n\nIn any case, appended objects should have type = real_type = non delta \ntype.\n\n\nNicolas\n"},{"id":"84878","messageId":"alpine.DEB.1.00.0807251513240.11976@eeepc-johanness","threadId":"14660","inReplyTo":"20080725115519.GA32487@atjola.homenet","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-25T13:15:48Z","receivedAt":"2008-07-25T13:15:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 25 Jul 2008, Björn Steinbrink wrote:\n\n> On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:\n> > Reading get_data_from_pack(), it does rely on hdr_size, idx.offset and \n> > idx.offset of the next entry to be set correctly. The function does \n> > not seem to use type (which the patch is also setting) nor real_type \n> > (which the patch does not set).\n> \n> type is used in get_base_data().\n> \n> > However, the code checks objects[nth].real_type all over the place in \n> > the code. Doesn't the lack of real_type assignment in \n> > append_obj_to_pack() affect them in any way?\n> \n> I had thought that resolve_delta() would set that, but it seems that we \n> never call that function like that. Hm...\n\nSo, let's add the comment as Nico suggested, and set real_type, too? (And \nit would be smashing if you could verify that the type is indeed correctly \nset to non-delta...)\n\nI think that setting real_type is necessary to have less surprises when \nthe code is extended in the future.\n\nThanks,\nDscho\n"},{"id":"84903","messageId":"20080725164202.GC21117@spearce.org","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807251513240.11976@eeepc-johanness","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-25T16:42:02Z","receivedAt":"2008-07-25T16:42:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Fri, 25 Jul 2008, Björn Steinbrink wrote:\n> > On 2008.07.24 22:21:14 -0700, Junio C Hamano wrote:\n> > > Reading get_data_from_pack(), it does rely on hdr_size, idx.offset and \n> > > idx.offset of the next entry to be set correctly. The function does \n> > > not seem to use type (which the patch is also setting) nor real_type \n> > > (which the patch does not set).\n> > \n> > type is used in get_base_data().\n> > \n> > > However, the code checks objects[nth].real_type all over the place in \n> > > the code. Doesn't the lack of real_type assignment in \n> > > append_obj_to_pack() affect them in any way?\n> > \n> > I had thought that resolve_delta() would set that, but it seems that we \n> > never call that function like that. Hm...\n> \n> So, let's add the comment as Nico suggested, and set real_type, too? (And \n> it would be smashing if you could verify that the type is indeed correctly \n> set to non-delta...)\n> \n> I think that setting real_type is necessary to have less surprises when \n> the code is extended in the future.\n\nThe patch looks correct, but it should set real_type too because\nI'm pretty sure we use that when we unpack the delta base again if\nit was pruned out of memory.\n\n-- \nShawn.\n"},{"id":"84908","messageId":"20080725171315.GA27285@atjola.homenet","threadId":"14660","inReplyTo":"alpine.DEB.1.00.0807251513240.11976@eeepc-johanness","subject":"[PATCH] index-pack: correctly initialize appended objects","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-07-25T17:13:15Z","receivedAt":"2008-07-25T17:13:15Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"When index-pack completes a thin pack it appends objects to the pack.\nSince the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when\nresolving deltas) such an object can be pruned in case of memory pressure.\n\nTo be able to re-read the object later, a few more fields have to be set.\n\nNoticed by Pierre Habouzit.\n\nSigned-off-by: Björn Steinbrink \nAcked-by: Nicolas Pitre \n---\n\n On 2008.07.25 15:15:48 +0200, Johannes Schindelin wrote:\n > So, let's add the comment as Nico suggested, and set real_type,\n > too?\n\n OK, I hope the comment is what was expected. My lack of knowledge\n made we wonder what to write... :-/\n\n > (And it would be smashing if you could verify that the type is\n > indeed correctly set to non-delta...)\n\n Hm, we get the object via read_sha1_file, can that return a delta? I\n would not expect it to. Sorry, never looked at those code paths\n (and don't have the time to investigate at the moment).\n\n index-pack.c | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/index-pack.c b/index-pack.c\nindex ac20a46..d757b07 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -699,6 +699,12 @@ static struct object_entry *append_obj_to_pack(\n \twrite_or_die(output_fd, header, n);\n \tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n \tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n+\t// This object comes from outside the thin pack, so we need to\n+\t// initialize the size and type fields\n+\tobj[0].hdr_size = n;\n+\tobj[0].size = size;\n+\tobj[0].type = type;\n+\tobj[0].real_type = type;\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n \tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n \thashcpy(obj->idx.sha1, sha1);\n-- \n1.6.0.rc0.14.g95f8.dirty\n"},{"id":"84910","messageId":"20080725172059.GD21117@spearce.org","threadId":"14660","inReplyTo":"20080725171315.GA27285@atjola.homenet","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-25T17:20:59Z","receivedAt":"2008-07-25T17:20:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Bjjjrn Steinbrink wrote:\n> When index-pack completes a thin pack it appends objects to the pack.\n> Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when\n> resolving deltas) such an object can be pruned in case of memory pressure.\n> \n> To be able to re-read the object later, a few more fields have to be set.\n> \n> Noticed by Pierre Habouzit.\n> \n> Signed-off-by: Björn Steinbrink \n> Acked-by: Nicolas Pitre \n\nAcked-by: Shawn O. Pearce \n\n> On 2008.07.25 15:15:48 +0200, Johannes Schindelin wrote:\n> > So, let's add the comment as Nico suggested, and set real_type,\n> > too?\n> \n> OK, I hope the comment is what was expected. My lack of knowledge\n> made we wonder what to write... :-/\n\nThe commit message makes sense to me. :)\n \n> > (And it would be smashing if you could verify that the type is\n> > indeed correctly set to non-delta...)\n> \n> Hm, we get the object via read_sha1_file, can that return a delta? I\n> would not expect it to. Sorry, never looked at those code paths\n> (and don't have the time to investigate at the moment).\n\nread_sha1_file() _never_ returns a delta. It always reutrns the\nwhole object, even if the object was stored as a delta in a pack\nsomewhere. The function is widely used within git to read an object\nfor processing, without the caller needing to worry about the types\nof compression used to store the object.\n\n\n> diff --git a/index-pack.c b/index-pack.c\n> index ac20a46..d757b07 100644\n> --- a/index-pack.c\n> +++ b/index-pack.c\n> @@ -699,6 +699,12 @@ static struct object_entry *append_obj_to_pack(\n> \twrite_or_die(output_fd, header, n);\n> \tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n> \tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n> +\t// This object comes from outside the thin pack, so we need to\n> +\t// initialize the size and type fields\n> +\tobj[0].hdr_size = n;\n> +\tobj[0].size = size;\n> +\tobj[0].type = type;\n> +\tobj[0].real_type = type;\n> \tobj[1].idx.offset = obj[0].idx.offset + n;\n> \tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n> \thashcpy(obj->idx.sha1, sha1);\n\n-- \nShawn.\n"},{"id":"84914","messageId":"7v4p6du8x8.fsf@gitster.siamese.dyndns.org","threadId":"14660","inReplyTo":"alpine.LFD.1.10.0807250751220.9968@xanadu.home","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-25T18:15:47Z","receivedAt":"2008-07-25T18:15:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Fri, 25 Jul 2008, Johannes Schindelin wrote:\n>\n>> Hi,\n>> \n>> On Thu, 24 Jul 2008, Junio C Hamano wrote:\n>> \n>> > The function does not seem to use type (which the patch is also setting) \n>> > nor real_type (which the patch does not set).\n>> > \n>> > However, the code checks objects[nth].real_type all over the place in \n>> > the code. Doesn't the lack of real_type assignment in \n>> > append_obj_to_pack() affect them in any way?\n>> \n>> >From staring at the code, I thought that real_type was set in \n>> resolve_delta(), but I may be wrong.\n>> \n>> The safer thing would be to set it, but I am not quite sure if we can use \n>> \"type\" directly, or if type can be \"delta\" for an object that is used to \n>> complete the pack, and therefore stored as a non-delta.\n>\n> Objects to complete the pack are always non delta, so the type and \n> real_type should be the same. However that shouldn't matter since at \n> that point the object array is not walked anymore, at least not for \n> appended objects, and therefore initializing the type at that point is \n> redundant.\n\nThanks. Here is what I committed.\n\ncommit 72de2883bd7d4ceda05f107826c7607c594de965\nAuthor: Björn Steinbrink \nDate: Thu Jul 24 18:32:00 2008 +0100\n\n index-pack.c: correctly initialize appended objects\n \n When index-pack completes a thin pack it appends objects to the pack.\n Since the commit 92392b4(index-pack: Honor core.deltaBaseCacheLimit when\n resolving deltas) such an object can be pruned in case of memory\n pressure, and will be read back again by get_data_from_pack(). For this\n to work, the fields in object_entry structure need to be initialized\n properly.\n \n Noticed by Pierre Habouzit.\n \n Signed-off-by: Björn Steinbrink \n Acked-by: Nicolas Pitre \n Acked-by: Shawn O. Pearce \n Signed-off-by: Junio C Hamano \n\ndiff --git a/index-pack.c b/index-pack.c\nindex c359f8c..7d5344a 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -698,6 +698,10 @@ static struct object_entry *append_obj_to_pack(\n \twrite_or_die(output_fd, header, n);\n \tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n \tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n+\tobj[0].size = size;\n+\tobj[0].hdr_size = n;\n+\tobj[0].type = type;\n+\tobj[0].real_type = type;\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n \tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n \thashcpy(obj->idx.sha1, sha1);\n"},{"id":"84994","messageId":"alpine.DEB.1.00.0807260503400.26810@eeepc-johanness","threadId":"14660","inReplyTo":"20080725171315.GA27285@atjola.homenet","subject":"Re: [PATCH] index-pack: correctly initialize appended objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-26T03:04:08Z","receivedAt":"2008-07-26T03:04:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 25 Jul 2008, Björn Steinbrink wrote:\n\n> +\t// This object comes from outside the thin pack, so we need to\n> +\t// initialize the size and type fields\n\nDo not use C++ comments in Git. Ever.\n\nCiao,\nDscho"}]}