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

From: Jeff King <peff@peff.net>
Date: Mar 9, 2017, 07:31 UTC
Message-ID: <20170309073117.g3br5btsfwntcdpe@sigill.intra.peff.net>
In-Reply-To: <1488994685-37403-3-git-send-email-jeffhost@microsoft.com>

On Wed, Mar 08, 2017 at 05:37:57PM +0000, Jeff Hostetler wrote:

Show 12 quoted lines

> 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 "/").

Show 9 quoted lines

> 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