From: Nick Edelen Date: Sun, 07 Jun 2009 13:31:57 GMT Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: In-Reply-To: by the way, since I'm on the list now anyway: is there any reason why you guys are calling mark_parents_uninteresting() after add_parents_to_list() in limit_list() (revision.c), considering the latter does everything the former does? sorta trivial, but it's been bugging me... On Sun, Jun 7, 2009 at 3:25 PM, Nick Edelen wrote: > hi guys, > > I wasn't aware that Sam was going to upload this, otherwise I would've > cleaned it up a bit.  not a problem though; thanks for taking the time > to do this Sam. > > @Johannes: > the real reason I'm using start_async at all is because of shallow > commit grafts.  we could potentially add an interface to pack-objects > to allow it to accept those types of commit grafts if you wanted. > like, in addition to the --not flag we could add a --shallow flag or > something...  it'd actually be pretty easy to implement, but I don't > know if you guys'd want that in pack-objects. > > @Nicolas: > I'm using the --revs flag in pack-objects, which causes it to use > get_object_list().  you'll notice, regardless of whether --thin is > set, this function still calls >        mark_edges_uninteresting(revs.commits, &revs, show_edge); > which sets uninteresting objects as preferred bases, which I'd think > would create a thin pack.  I could be wrong though... > > as I mentioned in the comment and above, it's an easy fix, but even > then I wasn't sure what to do with commit grafts.  as use_thin_pack > seemed to be predominantly set on shallow interactions, I just didn't > bother seperating the cases 'normal but thick pack' and 'shallow > stuff'. > > (btw, I have a really cool idea for shallow/narrow/lazy interaction if > you have the time.  it basically uses 'fantom' placeholder objects to > cover for missing blobs, so a clone/fetch would get all commits/trees > but only retrieve blobs when a user specifies.  I'll get a proof of > concept done after this rev-cache project). > > thank you both for looking over this though. > >  - Nick > > On Fri, Jun 5, 2009 at 6:51 PM, Nicolas Pitre wrote: >> On Fri, 5 Jun 2009, sam@vilain.net wrote: >> >>> instead of using the internal revision walker and piping object refs >>> to pack-objects this patch passes only the revs to pack-objects, which >>> in turn handles both enumeration and packing. >>> >>> Signed-off-by: Sam Vilain >>> --- >>>   Submitted on behalf of Nick in order to get wider feedback on this. >>>   This version passes the test suite. >>> >>>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++--------- >>>  1 files changed, 45 insertions(+), 9 deletions(-) >>> >>> diff --git a/upload-pack.c b/upload-pack.c >>> index edc7861..7eda8fd 100644 >>> --- a/upload-pack.c >>> +++ b/upload-pack.c >>> @@ -155,13 +155,27 @@ static void create_pack_file(void) >>>       const char *argv[10]; >>>       int arg = 0; >>> >>> -     rev_list.proc = do_rev_list; >>> -     /* .data is just a boolean: any non-NULL value will do */ >>> -     rev_list.data = create_full_pack ? &rev_list : NULL; >>> -     if (start_async(&rev_list)) >>> -             die("git upload-pack: unable to fork git-rev-list"); >>> - >>> -     argv[arg++] = "pack-objects"; >>> +     /* sending rev params to pack-objects directly is great, but unfortunately pack-objects >>> +      * has no way of turning off thin pack generation.  this would be a relatively simple >>> +      * addition, but as we also have to deal with shallow grafts and all it's simplest to >>> +      * just resort to piping object refs. >>> +      */ >> >> What's that?  Where did you get that? >> >> The way to not generate a thin pack is to not specify --thin to >> pack-objects.  If you get a thin pack without specifying --thin then >> this is a bug that needs to be fixed first. >> >>> +     if (!use_thin_pack) { >>> +             rev_list.proc = do_rev_list; >>> +             /* .data is just a boolean: any non-NULL value will do */ >>> +             rev_list.data = create_full_pack ? &rev_list : NULL; >>> +             if (start_async(&rev_list)) >>> +                     die("git upload-pack: unable to fork git-rev-list"); >>> + >>> +             argv[arg++] = "pack-objects"; >>> +     } else { >>> +             argv[arg++] = "pack-objects"; >>> +             argv[arg++] = "--revs"; >>> +             argv[arg++] = "--include-tag"; >> >> Shouldn't this be specified only if corresponding capability was >> provided by the client? >> >> >> Nicolas >> >