# [PATCH 08/13] prevent try_delta from using objects not in pack 8 messages from 2007-04-05 to 2007-04-06. Participants: Dana How, Junio C Hamano, Nicolas Pitre. Thread: https://gitlist.dev/t/7537 ## Dana How, 2007-04-05 22:35 Subject: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: <56b7f5510704051535ya98e86eu7f786f118c4fe229@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704051535ya98e86eu7f786f118c4fe229%40mail.gmail.com ``` --- builtin-pack-objects.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) -- Dana L. How danahow@gmail.com +1 650 804 5991 cell From 7d510f82b1e6acccc25596052abf3c5f1961ebcc Mon Sep 17 00:00:00 2001 From: Dana How Date: Thu, 5 Apr 2007 14:04:59 -0700 Subject: [PATCH 08/13] prevent try_delta from using objects not in pack --- builtin-pack-objects.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 9eb5fd6..37b0150 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1251,6 +1251,10 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, if (trg_entry->type != src_entry->type) return -1; + /* Don't try deltas involving already/non written objects */ + if (trg_entry->no_write || src_entry->no_write) + return -1; + /* We do not compute delta to *create* objects we are not * going to pack. */ -- 1.5.1.rc2.18.g9c88-dirty ``` ## Junio C Hamano, 2007-04-05 23:04 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: <7v8xd6mni1.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v8xd6mni1.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <56b7f5510704051535ya98e86eu7f786f118c4fe229@mail.gmail.com> ``` "Dana How" writes: > Subject: [PATCH 08/13] prevent try_delta from using objects not in pack > > --- > builtin-pack-objects.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c > index 9eb5fd6..37b0150 100644 > --- a/builtin-pack-objects.c > +++ b/builtin-pack-objects.c > @@ -1251,6 +1251,10 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, > if (trg_entry->type != src_entry->type) > return -1; > > + /* Don't try deltas involving already/non written objects */ > + if (trg_entry->no_write || src_entry->no_write) > + return -1; > + > /* We do not compute delta to *create* objects we are not > * going to pack. > */ I think trg_entry->no_write case should return -1 (meaning: doesn't delta with this source, nor any other sources you would throw at try_delta()), but shouldn't src_entry->no_write case return 0 (meaning: does not delta with this source, but do not give up -- other source candidate you have might be usable)? ``` ## Nicolas Pitre, 2007-04-06 01:02 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704052100420.28181%40xanadu.home In-Reply-To: <56b7f5510704051535ya98e86eu7f786f118c4fe229@mail.gmail.com> ``` What is the purpose of this patch? The try_delta() function is called with all objects before any object is written to a pack to find out how to deltify objects upfront. Nicolas ``` ## Dana How, 2007-04-06 02:28 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: <56b7f5510704051928l5d1f5a02yccc8e57c222e5f64@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704051928l5d1f5a02yccc8e57c222e5f64%40mail.gmail.com In-Reply-To: ``` On 4/5/07, Nicolas Pitre wrote: > What is the purpose of this patch? > > The try_delta() function is called with all objects before any object is > written to a pack to find out how to deltify objects upfront. I set no_write for 2 different reasons in the patchset. (1) When the blob is too big (--blob-limit) and will never be written. (2) When the blob has been written to a previous, finished pack. You're correct that this patch will never see condition (2). I think my repository statistics are a little unusual. Perhaps I'm getting ahead of myself here, but I also wanted to experiment with writing all blobs to one set of packs, and all trees, commits, and tags to another set (but probably just one small pack). I would use no_write for that and it would matter here. But I haven't run any experiments yet so I don't know if it makes any difference. I have seen related discussions on the mailing list so I know this isn't a new idea. -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Nicolas Pitre, 2007-04-06 03:27 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704052320560.28181%40xanadu.home In-Reply-To: <56b7f5510704051928l5d1f5a02yccc8e57c222e5f64@mail.gmail.com> ``` On Thu, 5 Apr 2007, Dana How wrote: > On 4/5/07, Nicolas Pitre wrote: > > What is the purpose of this patch? > > > > The try_delta() function is called with all objects before any object is > > written to a pack to find out how to deltify objects upfront. > > I set no_write for 2 different reasons in the patchset. > (1) When the blob is too big (--blob-limit) and will never be written. > (2) When the blob has been written to a previous, finished pack. > > You're correct that this patch will never see condition (2). Given that I proposed another way for big blobs in my previous email, then (1) should not be needed either. > I think my repository statistics are a little unusual. > Perhaps I'm getting ahead of myself here, > but I also wanted to experiment with writing all blobs to one set > of packs, and all trees, commits, and tags to another set > (but probably just one small pack). > I would use no_write for that and it would matter here. Again you should simply _not_ add objects you don't want to the list instead of adding them and marking them as unwanted. Nicolas ``` ## Dana How, 2007-04-06 03:47 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: <56b7f5510704052047r30166e53kffbdd8bf71885264@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704052047r30166e53kffbdd8bf71885264%40mail.gmail.com In-Reply-To: ``` On 4/5/07, Nicolas Pitre wrote: > On Thu, 5 Apr 2007, Dana How wrote: > > On 4/5/07, Nicolas Pitre wrote: > > > What is the purpose of this patch? > > > > > > The try_delta() function is called with all objects before any object is > > > written to a pack to find out how to deltify objects upfront. > > > > I set no_write for 2 different reasons in the patchset. > > (1) When the blob is too big (--blob-limit) and will never be written. > > (2) When the blob has been written to a previous, finished pack. > > > > You're correct that this patch will never see condition (2). > > Given that I proposed another way for big blobs in my previous email, > then (1) should not be needed either. > > > I think my repository statistics are a little unusual. > > Perhaps I'm getting ahead of myself here, > > but I also wanted to experiment with writing all blobs to one set > > of packs, and all trees, commits, and tags to another set > > (but probably just one small pack). > > I would use no_write for that and it would matter here. > > Again you should simply _not_ add objects you don't want to the list > instead of adding them and marking them as unwanted. Agreed, the marking just required less code change at the time. I would like not to have to add nr_skipped and nr_actual. Currently we have get_object_list -> traverse_commit_list -> show_{commit,object} -> add_object_entry , which is all called way before get_object_details -> check_object -> sha1_object_info . Can I safely move the sha1_object_info calll earlier into add_object_entry so I will know the size for pruning? In your other email you mention memory consumption due to object_entry. This structure could benefit somewhat from some attention beyond removing my no_write.(e.g. int->short for .depth and .delta_limit; int->enum for .preferred_base). Perhaps for later. Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Nicolas Pitre, 2007-04-06 14:59 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704060859230.28181%40xanadu.home In-Reply-To: <56b7f5510704052047r30166e53kffbdd8bf71885264@mail.gmail.com> ``` On Thu, 5 Apr 2007, Dana How wrote: > Currently we have get_object_list -> traverse_commit_list -> > show_{commit,object} -> add_object_entry , which is all > called way before get_object_details -> check_object -> sha1_object_info . > Can I safely move the sha1_object_info calll earlier into > add_object_entry so I will know the size for pruning? I think so, yes. And actually this call to sha1_object_info() has been bothering me for a while. There are currently two ways to get the list of objects to pack: one is from stdin where objects are listed with their SHA1's, the other one uses the internal revision walking code. In the later case we _already_ have the information that sha1_object_info() later provides making it rather wasteful by forcing a second object header parsing. However... aren't you more interested in the _compressed_ blob size than its raw size? > In your other email you mention memory consumption due to object_entry. > This structure could benefit somewhat from some attention beyond removing > my no_write.(e.g. int->short for .depth and .delta_limit; int->enum > for .preferred_base). Perhaps for later. Sure. Nicolas ``` ## Dana How, 2007-04-06 18:17 Subject: Re: [PATCH 08/13] prevent try_delta from using objects not in pack Message-ID: <56b7f5510704061117q2a833904g130246c164dd8e3f@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704061117q2a833904g130246c164dd8e3f%40mail.gmail.com In-Reply-To: ``` On 4/6/07, Nicolas Pitre wrote: > On Thu, 5 Apr 2007, Dana How wrote: > > Currently we have get_object_list -> traverse_commit_list -> > > show_{commit,object} -> add_object_entry , which is all > > called way before get_object_details -> check_object -> sha1_object_info . > > Can I safely move the sha1_object_info calll earlier into > > add_object_entry so I will know the size for pruning? > > I think so, yes. And actually this call to sha1_object_info() has been > bothering me for a while. > > There are currently two ways to get the list of objects to pack: one is > from stdin where objects are listed with their SHA1's, the other one > uses the internal revision walking code. In the later case we _already_ > have the information that sha1_object_info() later provides making it > rather wasteful by forcing a second object header parsing. Perhaps I'll add the early call (or actually, re-use the info if I can find it), and change the second call to only happen if the size is zero, since that's how it starts out in the current add_object_entry. > However... aren't you more interested in the _compressed_ blob size than > its raw size? Yes! So I am leaning towards disposing of --blob-limit anyway, which was in part a poor response to a bug in --pack-limit. Please see the other email I sent out so far today. Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ```