Re: [WIP 2/2] pack-objects: support --blob-size-limit
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 2, 2017, 04:48 UTC
- Message-ID
- <xmqqmv9ryoym.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <07c7139d1cbc2b3acf73c4df14d3cf1dde955720.1496361873.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 23 quoted lines
> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt > index 8973510a4..c4257cacc 100644 > --- a/Documentation/git-pack-objects.txt > +++ b/Documentation/git-pack-objects.txt > @@ -12,7 +12,8 @@ SYNOPSIS > 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied] > [--no-reuse-delta] [--delta-base-offset] [--non-empty] > [--local] [--incremental] [--window=<n>] [--depth=<n>] > - [--revs [--unpacked | --all]] [--stdout | base-name] > + [--revs [--unpacked | --all]] > + [--stdout [--blob-size-limit=<n>] | base-name] > [--shallow] [--keep-true-parents] < object-list > > > @@ -231,6 +232,22 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle. > With this option, parents that are hidden by grafts are packed > nevertheless. > > +--blob-size-limit=<n>:: > + This option can only be used with --stdout. If specified, a blob > + of at least this size will not be packed unless a to-be-packed > + tree has that blob with a filename beginning with ".git". > + The size can be suffixed with "k", "m", or "g", and may be "0".
It would be nice if the name of the parameter hints that this is counted in bytes, e.g. "--blob-max-bytes";
Or at least s/<n>/<bytes>/.
Show 5 quoted lines
> ++ > +If specified, after printing the packfile, pack-objects will print the > +count of excluded blobs (8 bytes) . Subsequently, for each excluded blob > +in hash order, pack-objects will print its hash (20 bytes) and its size > +(8 bytes). All numbers are in network byte order.
Do we need to future-proof the output format so that we can later use 32-byte hash? The input to pack-objects (i.e. rev-list --objects) is hexadecimal text, and it may not be so bad to make this also text, e.g. "<hash> SP <length> LF". That way, you do not have to worry about byte order, hash size, or length limited to uint64.
Right now, the readers of a pack stream like unpack-objects and index-pack know to copy any "garbage" that follow a pack stream to its standard output, so older readers piped downstream of this new pack-objects will simply accept the (somewhat incomplete) pack and ignore this trailer, which is probably what we want to happen.
Show 6 quoted lines
> ++ > +If pack-objects cannot efficiently determine, for an arbitrary blob, > +that no to-be-packed tree has that blob with a filename beginning with > +".git" (for example, if bitmaps are used to calculate the objects to be > +packed), it will pack all blobs regardless of size. > +
This is a bit hard to understand, at least to me. Let me step-wise deconstruct what I think you are saying.
- A blob can appear in multiple trees, and each tree has name (i.e. pathname component) for the blob.
- Sometimes we may know _all_ trees that contain a particular blob and hence _all_ names these trees call this blob. In such a case, we can _prove_ that the blob never is used as ".git<something>" in any tree.
- We exclude a blob that is larger than the specified limit, but only when we can prove the above. If it is possible that the blob might appear as ".git<something>" in some tree, the blob is not omitted no matter its size.
And this is a very conservative way to avoid omitting something that could be one of those control files like ".gitignore".
Am I following your thought correctly?
Show 10 quoted lines
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 1062d8fe2..aaf7e92b0 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -77,6 +77,8 @@ static unsigned long cache_max_small_delta_size = 1000; > > static unsigned long window_memory_limit = 0; > > +static long blob_size_limit = -1; > +
Which is the largest, long, ssize_t, or offset_t? Perhaps intmax_t but it may be overkill?
In any case, not within the scope of this series; we assume that an individual object's size fits in ulong around here.
Show 29 quoted lines
> @@ -776,6 +778,50 @@ static const char no_split_warning[] = N_(
> "disabling bitmap writing, packs are split due to pack.packSizeLimit"
> );
>
> +struct oversized_blob {
> + struct hashmap_entry entry;
> + struct object_id oid;
> + unsigned long size;
> +};
> +struct hashmap oversized_blobs;
> +
> +static int oversized_blob_cmp_fn(const void *b1, const void *b2,
> + const void *keydata)
> +{
> + const struct object_id *oid2 = keydata ? keydata :
> + &((const struct oversized_blob *) b2)->oid;
> + return oidcmp(&((const struct oversized_blob *) b1)->oid, oid2);
> +}
> +
> +static int oversized_blob_cmp(const void *b1, const void *b2)
> +{
> + return oidcmp(&(*(const struct oversized_blob **) b1)->oid,
> + &(*(const struct oversized_blob **) b2)->oid);
> +}
> +
> +static void write_oversized_info(void)
> +{
> + struct oversized_blob **obs = xmalloc(oversized_blobs.size *
> + sizeof(*obs));Can this multiplication overflow (hint: st_mult)?
Show 10 quoted lines
> + struct hashmap_iter iter; > + struct oversized_blob *ob; > + int i = 0; > + uint64_t size_network; > + > + for (ob = hashmap_iter_first(&oversized_blobs, &iter); > + ob; > + ob = hashmap_iter_next(&iter)) > + obs[i++] = ob; > + QSORT(obs, oversized_blobs.size, oversized_blob_cmp);
This sorting is a part of external contract (i.e. "output in hash order" is documented), but is it necessary? Just wondering.
Show 8 quoted lines
> + size_network = htonll(oversized_blobs.size);
> + write_in_full(1, &size_network, sizeof(size_network));
> + for (i = 0; i < oversized_blobs.size; i++) {
> + write_in_full(1, &obs[i]->oid, sizeof(obs[i]->oid));
> + size_network = htonll(obs[i]->size);
> + write_in_full(1, &size_network, sizeof(size_network));
> + }
> +}Show 6 quoted lines
> @@ -822,7 +868,11 @@ static void write_pack_file(void)
> * If so, rewrite it like in fast-import
> */
> if (pack_to_stdout) {
> - sha1close(f, sha1, CSUM_CLOSE);
> + sha1close(f, sha1, 0);You do want to exclude the trailing thing out of the checksum, but CSUM_CLOSE would close the fd and that is why you pass 0 here. OK.
> + write_in_full(1, sha1, sizeof(sha1));
Even though we are in "pack_to_stdout" codepath, I'd prefer to consistently use f->fd, not a hardcoded 1 here. Of course, sha1close() would have freed 'f' at this point, so we need to grab the return value from the function to learn which fd is connected to our original "stdout" at this point.
Likewise for write_oversized_info(), which probably should just take "int fd" as parameter.
> + if (blob_size_limit >= 0) {
> + write_oversized_info();
> + }I do not necessarily think it is a bad idea to show this as trailing data after the pack, but when we come to this function, do we have enough information to make the "oversize" information appear before the pack data if we wanted to? I have a suspicion that it is easier to handle at the receiving end, after the capability exchange decides to use this "omit oversized ones" extension, to receive this before the packdata begins. If it is at the end, the receiver needs to spawn either unpack-objects or index-objects to cause it write to the disk, but when it does so, it has to have a FD open to read from their standard output so that the receiver can grab the "remainder" that these programs did not process. "Write to the end of the pack stream data to these programs, and process the remainder ourselves" is much harder to arrange, I'd suspect.
Show 17 quoted lines
> } else if (nr_written == nr_remaining) {
> sha1close(f, sha1, CSUM_FSYNC);
> } else {
> @@ -1069,17 +1119,58 @@ static const char no_closure_warning[] = N_(
> "disabling bitmap writing, as some objects are not being packed"
> );
>
> +/*
> + * Returns 1 and records this blob in oversized_blobs if the given blob does
> + * not meet any defined blob size limits. Blobs that appear as a tree entry
> + * whose basename begins with ".git" are never considered oversized.
> + *
> + * If the tree entry corresponding to the given blob is unknown, pass NULL as
> + * name. In this case, this function will always return 0 to avoid false
> + * positives.
> + */
> +static int oversized(const unsigned char *sha1, const char *name) {Not object_id?
Show 11 quoted lines
> + const char *last_slash, *basename; > + unsigned long size; > + unsigned int hash; > + > + if (blob_size_limit < 0 || !name) > + return 0; > + > + last_slash = strrchr(name, '/'); > + basename = last_slash ? last_slash + 1 : name; > + if (starts_with(basename, ".git")) > + return 0;
When we have two paths to a blob, and the blob is first fed to this function with one name that does not begin with ".git", we would register it to the hashmap. And then the other name that begins with ".git" is later discovered to point at the same blob, what happens? Would we need to unregister it from the hashmap elsewhere in the code?
Show 20 quoted lines
> + sha1_object_info(sha1, &size);
> + if (size < blob_size_limit)
> + return 0;
> +
> + if (!oversized_blobs.tablesize)
> + hashmap_init(&oversized_blobs, oversized_blob_cmp_fn, 0);
> + hash = sha1hash(sha1);
> + if (!hashmap_get_from_hash(&oversized_blobs, hash, sha1)) {
> + struct oversized_blob *ob = xmalloc(sizeof(*ob));
> + hashmap_entry_init(ob, hash);
> + hashcpy(ob->oid.hash, sha1);
> + ob->size = size;
> + hashmap_add(&oversized_blobs, ob);
> + }
> + return 1;
> +}
> +
> static int add_object_entry(const unsigned char *sha1, enum object_type type,
> - const char *name, int exclude)
> + const char *name, int preferred_base)This belongs to the previous step that renamed "exclude", no?
Show 29 quoted lines
> {
> struct packed_git *found_pack = NULL;
> off_t found_offset = 0;
> uint32_t index_pos;
> + struct hashmap_entry entry;
>
> - if (have_duplicate_entry(sha1, exclude, &index_pos))
> + if (have_duplicate_entry(sha1, preferred_base, &index_pos))
> return 0;
>
> - if (ignore_object(sha1, exclude, &found_pack, &found_offset)) {
> + if (ignore_object(sha1, preferred_base, &found_pack, &found_offset) ||
> + (!preferred_base && type == OBJ_BLOB && oversized(sha1, name))) {
> /* The pack is missing an object, so it will not have closure */
> if (write_bitmap_index) {
> warning(_(no_closure_warning));
> @@ -1088,8 +1179,17 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
> return 0;
> }
>
> + /*
> + * Ensure that we do not report this blob as oversized if we are going
> + * to use it, be it in the returned pack or as a preferred base
> + */
> + if (oversized_blobs.tablesize) {
> + hashmap_entry_init(&entry, sha1hash(sha1));
> + free(hashmap_remove(&oversized_blobs, &entry, sha1));
> + }
> +Ah, is this where the "unregistering" happens?
Show 6 quoted lines
> create_object_entry(sha1, type, pack_name_hash(name), > - exclude, name && no_try_delta(name), > + preferred_base, name && no_try_delta(name), > index_pos, found_pack, found_offset); > > display_progress(progress_state, nr_result);