Re: [PATCH 02/10] pack-objects: add --partial-by-size=n --partial-special

> On Wed, Mar 08, 2017 at 05:37:57PM +0000, Jeff Hostetler wrote:
>
>> From: Jeff Hostetler <git@jeffhostetler.com>
>>
>> Teach pack-objects to omit blobs from the generated packfile.
>>
>> When the --partial-by-size=n[kmg] argument is used, only blobs
>> smaller than the requested size are included.  When n is zero,
>> no blobs are included.
>>
>> When the --partial-special argument is used, git special files,
>> such as ".gitattributes" and ".gitignores" are included.
>>
>> When both are given, the union of two are included.
>
> I understand why one would want to do:
>
>   --partial-by-size=100 --partial-special
>
> and get the union. The first one restricts, and the second one adds back
> in. But I don't understand why "--partial-special" by itself makes any
> sense. Wouldn't we already be including all blobs, and it would be a
> noop?

>
>
> Also, I was thinking a bit on Junio's comment elsewhere on whether
> read_object_list_from_stdin() should do the same limiting. I think the
> answer is "probably not", because whoever is generating that object list
> can cull the set. You could do it today with something like:
>
>   git rev-list --objects HEAD |
>   git cat-file --batch-check='%(objectsize) %(objecttype) %(objectname) %(rest)' |
>   perl -lne 's/^(\d+) (\S+) //; print if $2 ne "blob" || $1 < 100' |
>   git pack-objects
>
> But if we are going to add this --partial-by-size for the pack-objects
> traversal, shouldn't we just add it to rev-list? Then:
>
>   git rev-list --objects --partial-by-size=100 --partial-special |
>   git pack-objects
>
> works, and you should get it in the pack-objects basically for free (I
> think you'd have to allow through the "--partial" arguments on stdin,
> and make sure the rev-list implementation is done via
> traverse_commit_list).
>
> As a bonus, I suspect it would make the --partial-special path-handling
> easier, because you'd see each tree entry rather than the fully
> constructed path (so no more monkeying around with "/").

>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index 7e052bb..2df2f49 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -77,6 +77,10 @@ static unsigned long cache_max_small_delta_size = 1000;
>>
>>  static unsigned long window_memory_limit = 0;
>>
>> +static signed long partial_by_size = -1;
>
> I would have expected this to be an off_t, though I think
> OPT_MAGNITUDE() forces you into "unsigned long". I guess it is nothing
> new for Git; we use "unsigned long" for single object sizes elsewhere,
> so systems with a 32-bit long are out of luck anyway until we fix that.
>
> The signed "long" here is unfortunate, as it limits us to 2G on such
> systems. Maybe it is not worth worrying too much about. The "big object"
> threshold is usually around 500MB. I think the failure behavior is not
> great, though (asking for "3G" would go negative and effectively be
> ignored).
>
> I think handling all cases would involve swapping out OPT_MAGNITUDE()
> for a special callback that writes the "yes, the user set this" bit in a
> separate variable.