From: Nick Edelen Date: Sun, 07 Jun 2009 13:25:26 GMT Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: In-Reply-To: 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 >