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

[Jeff King <peff@peff.net>]

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.

-Peff