Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects

[Nick Edelen <sirnot@gmail.com>]

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<nico@cam.org> 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 <sam@vilain.net>
>> ---
>>   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
>