{"thread":{"id":"46094","subject":"[WIP 0/2] Modifying pack-objects to support --blob-size-limit","startedAt":"2017-06-02T00:15:15Z","lastAt":"2017-06-15T21:03:15Z","messageCount":17,"participants":["Jonathan Tan","Junio C Hamano","Jeff King","Jonathan Nieder","Jeff Hostetler"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"321359","messageId":"cover.1496361873.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":null,"subject":"[WIP 0/2] Modifying pack-objects to support --blob-size-limit","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T00:14:53Z","receivedAt":"2017-06-02T00:15:15Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I took a look at modifying builtin/pack-objects.c to support excluding\noversized blobs and reporting the exclusions that it has performed.\nHere's a work in progress - it might be of aid to others who are working\non a similar feature and/or are modifying pack-objects for something\nelse.\n\nThe way I used to obtain a blob's size seems inefficient and redundant\nwith check_object() - if you have a suggestion to improve this, I'm\ninterested.\n\nThis is similar to [1] except that this reports blobs and their sizes\n(after the packfile is printed), and is slightly more comprehensive in\nthat the read_object_list_from_stdin() codepath is also covered in\naddition to the get_object_list() codepath. (Although, to be clear,\nupload-pack always passes \"--revs\" and thus only needs the\nget_object_list() codepath).\n\n[1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffhost@microsoft.com/\n\nJonathan Tan (2):\n pack-objects: rename want_.* to ignore_.*\n pack-objects: support --blob-size-limit\n\n Documentation/git-pack-objects.txt | 19 ++++-\n builtin/pack-objects.c | 163 ++++++++++++++++++++++++++++++-------\n t/t5300-pack-object.sh | 71 ++++++++++++++++\n 3 files changed, 223 insertions(+), 30 deletions(-)\n\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321360","messageId":"5e8aebf2726481ef63838291c32e07439289d922.1496361873.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":"cover.1496361873.git.jonathantanmy@google.com","subject":"[WIP 1/2] pack-objects: rename want_.* to ignore_.*","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T00:14:54Z","receivedAt":"2017-06-02T00:15:16Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, pack-objects conflates the concepts of \"ignoring an object\"\nand \"including it in to_pack\". This is fine for now, but a subsequent\ncommit will introduce the concept of an object that cannot be completely\nignored, but should not be included in to_pack either. To separate these\nconcepts, restrict want_found_object() and want_object_in_pack() to only\nindicate if the object is to be ignored. This is done by renaming these\nmethods and swapping the meanings of the return values 0 and 1.\n\nWe also take the opportunity to use the terminology \"preferred_base\"\ninstead of \"excluded\" in these methods. It is true that preferred bases\nare not included in the final packfile generation, but at this point in\nthe code, there is no exclusion taking place - on the contrary, if\nsomething is \"excluded\", it is in fact guaranteed to be in to_pack.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/pack-objects.c | 50 +++++++++++++++++++++++++-------------------------\n 1 file changed, 25 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 80439047a..1062d8fe2 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -946,12 +946,12 @@ static int have_duplicate_entry(const unsigned char *sha1,\n \treturn 1;\n }\n \n-static int want_found_object(int exclude, struct packed_git *p)\n+static int ignore_found_object(int preferred_base, struct packed_git *p)\n {\n-\tif (exclude)\n-\t\treturn 1;\n-\tif (incremental)\n+\tif (preferred_base)\n \t\treturn 0;\n+\tif (incremental)\n+\t\treturn 1;\n \n \t/*\n \t * When asked to do --local (do not include an object that appears in a\n@@ -969,19 +969,19 @@ static int want_found_object(int exclude, struct packed_git *p)\n \t */\n \tif (!ignore_packed_keep &&\n \t (!local || !have_non_local_packs))\n-\t\treturn 1;\n+\t\treturn 0;\n \n \tif (local && !p->pack_local)\n-\t\treturn 0;\n+\t\treturn 1;\n \tif (ignore_packed_keep && p->pack_local && p->pack_keep)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \t/* we don't know yet; keep looking for more packs */\n \treturn -1;\n }\n \n /*\n- * Check whether we want the object in the pack (e.g., we do not want\n+ * Check whether we should ignore this object (e.g., we do not want\n * objects found in non-local stores if the \"--local\" option was used).\n *\n * If the caller already knows an existing pack it wants to take the object\n@@ -989,16 +989,16 @@ static int want_found_object(int exclude, struct packed_git *p)\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const unsigned char *sha1,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int ignore_object(const unsigned char *sha1,\n+\t\t\t int preferred_base,\n+\t\t\t struct packed_git **found_pack,\n+\t\t\t off_t *found_offset)\n {\n \tstruct mru_entry *entry;\n-\tint want;\n+\tint ignore;\n \n-\tif (!exclude && local && has_loose_object_nonlocal(sha1))\n-\t\treturn 0;\n+\tif (!preferred_base && local && has_loose_object_nonlocal(sha1))\n+\t\treturn 1;\n \n \t/*\n \t * If we already know the pack object lives in, start checks from that\n@@ -1006,9 +1006,9 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(exclude, *found_pack);\n-\t\tif (want != -1)\n-\t\t\treturn want;\n+\t\tignore = ignore_found_object(preferred_base, *found_pack);\n+\t\tif (ignore != -1)\n+\t\t\treturn ignore;\n \t}\n \n \tfor (entry = packed_git_mru->head; entry; entry = entry->next) {\n@@ -1027,15 +1027,15 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t\t*found_offset = offset;\n \t\t\t\t*found_pack = p;\n \t\t\t}\n-\t\t\twant = want_found_object(exclude, p);\n-\t\t\tif (!exclude && want > 0)\n+\t\t\tignore = ignore_found_object(preferred_base, p);\n+\t\t\tif (!preferred_base && ignore > 0)\n \t\t\t\tmru_mark(packed_git_mru, entry);\n-\t\t\tif (want != -1)\n-\t\t\t\treturn want;\n+\t\t\tif (ignore != -1)\n+\t\t\t\treturn ignore;\n \t\t}\n \t}\n \n-\treturn 1;\n+\treturn 0;\n }\n \n static void create_object_entry(const unsigned char *sha1,\n@@ -1079,7 +1079,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \tif (have_duplicate_entry(sha1, exclude, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset)) {\n+\tif (ignore_object(sha1, exclude, &found_pack, &found_offset)) {\n \t\t/* The pack is missing an object, so it will not have closure */\n \t\tif (write_bitmap_index) {\n \t\t\twarning(_(no_closure_warning));\n@@ -1106,7 +1106,7 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,\n \tif (have_duplicate_entry(sha1, 0, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, 0, &pack, &offset))\n+\tif (ignore_object(sha1, 0, &pack, &offset))\n \t\treturn 0;\n \n \tcreate_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321361","messageId":"07c7139d1cbc2b3acf73c4df14d3cf1dde955720.1496361873.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":"cover.1496361873.git.jonathantanmy@google.com","subject":"[WIP 2/2] pack-objects: support --blob-size-limit","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T00:14:55Z","receivedAt":"2017-06-02T00:15:19Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As part of an effort to improve Git support for very large repositories\nin which clients typically have only a subset of all version-controlled\nblobs, teach pack-objects to support --blob-size-limit, packing only\nblobs below that size limit unless the blob corresponds to a file whose\nname starts with \".git\". upload-pack will eventually be taught to use\nthis new parameter if needed to exclude certain blobs during a fetch or\nclone, potentially drastically reducing network consumption when serving\nthese very large repositories.\n\nSince any excluded blob should not act as a delta base, I did this by\navoiding such oversized blobs from ever going into the \"to_pack\" data\nstructure, which contains both preferred bases (which do not go into the\nfinal packfile but can act as delta bases) and the objects to be packed\n(which go into the final packfile and also can act as delta bases). To\nthat end, add_object_entry() has been modified to exclude the\nappropriate non-preferred-base objects. (Preferred bases, regardless of\nsize, still enter \"to_pack\" - they are something that the client\nindicates that it has, not something that the server needs to serve, so\nno exclusion occurs.) Any such exclusions are recorded and sent after\nthe packfile.\n\nI have taken the opportunity to rename the \"exclude\" parameter to\n\"preferred_base\" to avoid confusion between the concept of \"excluding an\nobject from to_pack\" and \"including an object in to_pack which will be\nexcluded in the final pack generation\".\n\nThe other method in which objects are added to \"to_pack\",\nadd_object_entry_from_bitmap(), has not been modified - thus packing in\nthe presence of bitmaps still packs all blobs regardless of size. See\nthe documentation update in this commit for the rationale.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-pack-objects.txt | 19 +++++-\n builtin/pack-objects.c | 115 +++++++++++++++++++++++++++++++++++--\n t/t5300-pack-object.sh | 71 +++++++++++++++++++++++\n 3 files changed, 199 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 8973510a4..c4257cacc 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -12,7 +12,8 @@ SYNOPSIS\n 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied]\n \t[--no-reuse-delta] [--delta-base-offset] [--non-empty]\n \t[--local] [--incremental] [--window=] [--depth=]\n-\t[--revs [--unpacked | --all]] [--stdout | base-name]\n+\t[--revs [--unpacked | --all]]\n+\t[--stdout [--blob-size-limit=] | base-name]\n \t[--shallow] [--keep-true-parents] < object-list\n \n \n@@ -231,6 +232,22 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.\n \tWith this option, parents that are hidden by grafts are packed\n \tnevertheless.\n \n+--blob-size-limit=::\n+\tThis option can only be used with --stdout. If specified, a blob\n+\tof at least this size will not be packed unless a to-be-packed\n+\ttree has that blob with a filename beginning with \".git\".\n+\tThe size can be suffixed with \"k\", \"m\", or \"g\", and may be \"0\".\n++\n+If specified, after printing the packfile, pack-objects will print the\n+count of excluded blobs (8 bytes) . Subsequently, for each excluded blob\n+in hash order, pack-objects will print its hash (20 bytes) and its size\n+(8 bytes). All numbers are in network byte order.\n++\n+If pack-objects cannot efficiently determine, for an arbitrary blob,\n+that no to-be-packed tree has that blob with a filename beginning with\n+\".git\" (for example, if bitmaps are used to calculate the objects to be\n+packed), it will pack all blobs regardless of size.\n+\n SEE ALSO\n --------\n linkgit:git-rev-list[1]\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1062d8fe2..aaf7e92b0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -77,6 +77,8 @@ static unsigned long cache_max_small_delta_size = 1000;\n \n static unsigned long window_memory_limit = 0;\n \n+static long blob_size_limit = -1;\n+\n /*\n * stats\n */\n@@ -776,6 +778,50 @@ static const char no_split_warning[] = N_(\n \"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n );\n \n+struct oversized_blob {\n+\tstruct hashmap_entry entry;\n+\tstruct object_id oid;\n+\tunsigned long size;\n+};\n+struct hashmap oversized_blobs;\n+\n+static int oversized_blob_cmp_fn(const void *b1, const void *b2,\n+\t\t\t\t const void *keydata)\n+{\n+\tconst struct object_id *oid2 = keydata ? keydata : \n+\t\t&((const struct oversized_blob *) b2)->oid;\n+\treturn oidcmp(&((const struct oversized_blob *) b1)->oid, oid2);\n+}\n+\n+static int oversized_blob_cmp(const void *b1, const void *b2)\n+{\n+\treturn oidcmp(&(*(const struct oversized_blob **) b1)->oid,\n+\t\t &(*(const struct oversized_blob **) b2)->oid);\n+}\n+\n+static void write_oversized_info(void)\n+{\n+\tstruct oversized_blob **obs = xmalloc(oversized_blobs.size *\n+\t\t\t\t\t sizeof(*obs));\n+\tstruct hashmap_iter iter;\n+\tstruct oversized_blob *ob;\n+\tint i = 0;\n+\tuint64_t size_network;\n+\n+\tfor (ob = hashmap_iter_first(&oversized_blobs, &iter);\n+\t ob;\n+\t ob = hashmap_iter_next(&iter))\n+\t\tobs[i++] = ob;\n+\tQSORT(obs, oversized_blobs.size, oversized_blob_cmp);\n+\tsize_network = htonll(oversized_blobs.size);\n+\twrite_in_full(1, &size_network, sizeof(size_network));\n+\tfor (i = 0; i < oversized_blobs.size; i++) {\n+\t\twrite_in_full(1, &obs[i]->oid, sizeof(obs[i]->oid));\n+\t\tsize_network = htonll(obs[i]->size);\n+\t\twrite_in_full(1, &size_network, sizeof(size_network));\n+\t}\n+}\n+\n static void write_pack_file(void)\n {\n \tuint32_t i = 0, j;\n@@ -822,7 +868,11 @@ static void write_pack_file(void)\n \t\t * If so, rewrite it like in fast-import\n \t\t */\n \t\tif (pack_to_stdout) {\n-\t\t\tsha1close(f, sha1, CSUM_CLOSE);\n+\t\t\tsha1close(f, sha1, 0);\n+\t\t\twrite_in_full(1, sha1, sizeof(sha1));\n+\t\t\tif (blob_size_limit >= 0) {\n+\t\t\t\twrite_oversized_info();\n+\t\t\t}\n \t\t} else if (nr_written == nr_remaining) {\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n \t\t} else {\n@@ -1069,17 +1119,58 @@ static const char no_closure_warning[] = N_(\n \"disabling bitmap writing, as some objects are not being packed\"\n );\n \n+/*\n+ * Returns 1 and records this blob in oversized_blobs if the given blob does\n+ * not meet any defined blob size limits. Blobs that appear as a tree entry\n+ * whose basename begins with \".git\" are never considered oversized.\n+ *\n+ * If the tree entry corresponding to the given blob is unknown, pass NULL as\n+ * name. In this case, this function will always return 0 to avoid false\n+ * positives.\n+ */\n+static int oversized(const unsigned char *sha1, const char *name) {\n+\tconst char *last_slash, *basename;\n+\tunsigned long size;\n+\tunsigned int hash;\n+\n+\tif (blob_size_limit < 0 || !name)\n+\t\treturn 0;\n+\n+\tlast_slash = strrchr(name, '/');\n+\tbasename = last_slash ? last_slash + 1 : name;\n+\tif (starts_with(basename, \".git\"))\n+\t\treturn 0;\n+\n+\tsha1_object_info(sha1, &size);\n+\tif (size < blob_size_limit)\n+\t\treturn 0;\n+\t\n+\tif (!oversized_blobs.tablesize)\n+\t\thashmap_init(&oversized_blobs, oversized_blob_cmp_fn, 0);\n+\thash = sha1hash(sha1);\n+\tif (!hashmap_get_from_hash(&oversized_blobs, hash, sha1)) {\n+\t\tstruct oversized_blob *ob = xmalloc(sizeof(*ob));\n+\t\thashmap_entry_init(ob, hash);\n+\t\thashcpy(ob->oid.hash, sha1);\n+\t\tob->size = size;\n+\t\thashmap_add(&oversized_blobs, ob);\n+\t}\n+\treturn 1;\n+}\n+\n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n-\t\t\t const char *name, int exclude)\n+\t\t\t const char *name, int preferred_base)\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n \tuint32_t index_pos;\n+\tstruct hashmap_entry entry;\n \n-\tif (have_duplicate_entry(sha1, exclude, &index_pos))\n+\tif (have_duplicate_entry(sha1, preferred_base, &index_pos))\n \t\treturn 0;\n \n-\tif (ignore_object(sha1, exclude, &found_pack, &found_offset)) {\n+\tif (ignore_object(sha1, preferred_base, &found_pack, &found_offset) ||\n+\t (!preferred_base && type == OBJ_BLOB && oversized(sha1, name))) {\n \t\t/* The pack is missing an object, so it will not have closure */\n \t\tif (write_bitmap_index) {\n \t\t\twarning(_(no_closure_warning));\n@@ -1088,8 +1179,17 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\treturn 0;\n \t}\n \n+\t/*\n+\t * Ensure that we do not report this blob as oversized if we are going\n+\t * to use it, be it in the returned pack or as a preferred base\n+\t */\n+\tif (oversized_blobs.tablesize) {\n+\t\thashmap_entry_init(&entry, sha1hash(sha1));\n+\t\tfree(hashmap_remove(&oversized_blobs, &entry, sha1));\n+\t}\n+\n \tcreate_object_entry(sha1, type, pack_name_hash(name),\n-\t\t\t exclude, name && no_try_delta(name),\n+\t\t\t preferred_base, name && no_try_delta(name),\n \t\t\t index_pos, found_pack, found_offset);\n \n \tdisplay_progress(progress_state, nr_result);\n@@ -2947,6 +3047,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n \t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index together with the pack index\")),\n+\t\tOPT_MAGNITUDE(0, \"blob-size-limit\", &blob_size_limit,\n+\t\t\t N_(\"exclude and report blobs not smaller than this limit\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3022,6 +3124,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tdie(\"--keep-unreachable and --unpack-unreachable are incompatible.\");\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n+\t\n+\tif (blob_size_limit >= 0 && !pack_to_stdout)\n+\t\tdie(\"if --blob-size-limit is specified, --stdout must also be specified\");\n \n \t/*\n \t * \"soft\" reasons not to use bitmaps - for on-disk repack by default we want\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 43a672c34..d12280cbf 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -421,6 +421,77 @@ test_expect_success 'index-pack works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n+unpack() {\n+\tperl -e '$/ = undef; $input = <>; print unpack(\"H2\" x length($input), $input)'\n+}\n+\n+rcut() {\n+\tperl -e '$/ = undef; $_ = <>; s/.{'$1'}$//s; print $_'\n+}\n+\n+test_expect_success '--blob-size-limit works with multiple excluded' '\n+\trm -rf server &&\n+\tgit init server &&\n+\tprintf a > server/a &&\n+\tprintf b > server/b &&\n+\tprintf c-very-long-file > server/c &&\n+\tprintf d-very-long-file > server/d &&\n+\tgit -C server add a b c d &&\n+\tgit -C server commit -m x &&\n+\n+\tgit -C server rev-parse HEAD >objects &&\n+\tgit -C server pack-objects --revs --stdout --blob-size-limit=10 out &&\n+\tunpack actual &&\n+\n+\t# Check that the excluded hashes are included, sorted, at the end\n+\tif [ $(git hash-object server/c) \\< $(git hash-object server/d) ]\n+\tthen\n+\t\tprintf \"0000000000000002%s0000000000000010%s0000000000000010$\" \\\n+\t\t\t$(git hash-object server/c) \\\n+\t\t\t$(git hash-object server/d) >expect\n+\telse\n+\t\tprintf \"0000000000000002%s0000000000000010%s0000000000000010$\" \\\n+\t\t\t$(git hash-object server/d) \\\n+\t\t\t$(git hash-object server/c) >expect\n+\tfi &&\n+\tgrep $(cat expect) actual &&\n+\n+\t# Ensure that only the small blobs are in the packfile\n+\trcut 64 my.pack &&\n+\tgit index-pack my.pack &&\n+\tgit verify-pack -v my.idx >objectlist &&\n+\tgrep $(git hash-object server/a) objectlist &&\n+\tgrep $(git hash-object server/b) objectlist &&\n+\t! grep $(git hash-object server/c) objectlist &&\n+\t! grep $(git hash-object server/d) objectlist\n+'\n+\n+test_expect_success '--blob-size-limit never excludes special files' '\n+\trm -rf server &&\n+\tgit init server &&\n+\tprintf a-very-long-file > server/a &&\n+\tprintf a-very-long-file > server/.git-a &&\n+\tprintf b-very-long-file > server/b &&\n+\tgit -C server add a .git-a b &&\n+\tgit -C server commit -m x &&\n+\n+\tgit -C server rev-parse HEAD >objects &&\n+\tgit -C server pack-objects --revs --stdout --blob-size-limit=10 out &&\n+\tunpack actual &&\n+\n+\t# Check that the excluded hash is included at the end\n+\tprintf \"0000000000000001%s0000000000000010$\" \\\n+\t\t$(git hash-object server/b) >expect &&\n+\tgrep $(cat expect) actual &&\n+\n+\t# Ensure that the .git-a blob is in the packfile, despite also\n+\t# appearing as a non-.git file\n+\trcut 36 my.pack &&\n+\tgit index-pack my.pack &&\n+\tgit verify-pack -v my.idx >objectlist &&\n+\tgrep $(git hash-object server/a) objectlist\n+'\n+\n #\n # WARNING!\n #\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321382","messageId":"xmqqwp8vyrv1.fsf@gitster.mtv.corp.google.com","threadId":"46094","inReplyTo":"5e8aebf2726481ef63838291c32e07439289d922.1496361873.git.jonathantanmy@google.com","subject":"Re: [WIP 1/2] pack-objects: rename want_.* to ignore_.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T03:45:54Z","receivedAt":"2017-06-02T03:46:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Currently, pack-objects conflates the concepts of \"ignoring an object\"\n> and \"including it in to_pack\".\n\nHmph, that statement is a hard to read and agree to. I thought an\nignored object that is not going to be packed is one that won't hit\nto_pack? \n\nI agree that \"including to to_pack\" and \"actually appearing in the\nresulting pack\" are three different things, though. Preferred base\nobjects that are used when constructing a thin pack are thrown into\nto_pack because they need to participate in the delta base selection\ncomputation, but by definition they shouldn't be contained in the\nresulting pack (hence, nr_result is not incremented for them).\n\n> This is fine for now, but a subsequent\n> commit will introduce the concept of an object that cannot be completely\n> ignored, but should not be included in to_pack either. To separate these\n> concepts, restrict want_found_object() and want_object_in_pack() to only\n> indicate if the object is to be ignored. This is done by renaming these\n> methods and swapping the meanings of the return values 0 and 1.\n\nI am a bit confused by your reasoning. I guess it will become\nclearer if I knew exactly what you mean by \"ignoring\". It is not\nlike \"pretend as if it didn't exist in the rev-list --objects output\nwe are working off of\".\n\n> We also take the opportunity to use the terminology \"preferred_base\"\n> instead of \"excluded\" in these methods. It is true that preferred bases\n> are not included in the final packfile generation, but at this point in\n> the code, there is no exclusion taking place - on the contrary, if\n> something is \"excluded\", it is in fact guaranteed to be in to_pack.\n\nThis one I can understand.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/pack-objects.c | 50 +++++++++++++++++++++++++-------------------------\n> 1 file changed, 25 insertions(+), 25 deletions(-)\n\nWithout understanding why this change is desirable, I can tell that\noverall this does not change the behaviour, which is good ;-).\n\n"},{"id":"321384","messageId":"xmqqmv9ryoym.fsf@gitster.mtv.corp.google.com","threadId":"46094","inReplyTo":"07c7139d1cbc2b3acf73c4df14d3cf1dde955720.1496361873.git.jonathantanmy@google.com","subject":"Re: [WIP 2/2] pack-objects: support --blob-size-limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T04:48:33Z","receivedAt":"2017-06-02T04:48:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n> index 8973510a4..c4257cacc 100644\n> --- a/Documentation/git-pack-objects.txt\n> +++ b/Documentation/git-pack-objects.txt\n> @@ -12,7 +12,8 @@ SYNOPSIS\n> 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied]\n> \t[--no-reuse-delta] [--delta-base-offset] [--non-empty]\n> \t[--local] [--incremental] [--window=] [--depth=]\n> -\t[--revs [--unpacked | --all]] [--stdout | base-name]\n> +\t[--revs [--unpacked | --all]]\n> +\t[--stdout [--blob-size-limit=] | base-name]\n> \t[--shallow] [--keep-true-parents] < object-list\n> \n> \n> @@ -231,6 +232,22 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.\n> \tWith this option, parents that are hidden by grafts are packed\n> \tnevertheless.\n> \n> +--blob-size-limit=::\n> +\tThis option can only be used with --stdout. If specified, a blob\n> +\tof at least this size will not be packed unless a to-be-packed\n> +\ttree has that blob with a filename beginning with \".git\".\n> +\tThe size can be suffixed with \"k\", \"m\", or \"g\", and may be \"0\".\n\nIt would be nice if the name of the parameter hints that this is\ncounted in bytes, e.g. \"--blob-max-bytes\"; \n\nOr at least s///.\n\n> ++\n> +If specified, after printing the packfile, pack-objects will print the\n> +count of excluded blobs (8 bytes) . Subsequently, for each excluded blob\n> +in hash order, pack-objects will print its hash (20 bytes) and its size\n> +(8 bytes). All numbers are in network byte order.\n\nDo we need to future-proof the output format so that we can later\nuse 32-byte hash? The input to pack-objects (i.e. rev-list --objects)\nis hexadecimal text, and it may not be so bad to make this also\ntext, e.g. \" SP LF\". That way, you do not have to\nworry about byte order, hash size, or length limited to uint64.\n\nRight now, the readers of a pack stream like unpack-objects and\nindex-pack know to copy any \"garbage\" that follow a pack stream to\nits standard output, so older readers piped downstream of this new\npack-objects will simply accept the (somewhat incomplete) pack and\nignore this trailer, which is probably what we want to happen.\n\n> ++\n> +If pack-objects cannot efficiently determine, for an arbitrary blob,\n> +that no to-be-packed tree has that blob with a filename beginning with\n> +\".git\" (for example, if bitmaps are used to calculate the objects to be\n> +packed), it will pack all blobs regardless of size.\n> +\n\nThis is a bit hard to understand, at least to me. Let me step-wise\ndeconstruct what I think you are saying.\n\n - A blob can appear in multiple trees, and each tree has name\n (i.e. pathname component) for the blob.\n\n - Sometimes we may know _all_ trees that contain a particular\n blob and hence _all_ names these trees call this blob. In such a\n case, we can _prove_ that the blob never is used as \".git\"\n in any tree.\n\n - We exclude a blob that is larger than the specified limit, but\n only when we can prove the above. If it is possible that the\n blob might appear as \".git\" in some tree, the blob is\n not omitted no matter its size.\n\nAnd this is a very conservative way to avoid omitting something that\ncould be one of those control files like \".gitignore\".\n\nAm I following your thought correctly?\n\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 1062d8fe2..aaf7e92b0 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -77,6 +77,8 @@ static unsigned long cache_max_small_delta_size = 1000;\n> \n> static unsigned long window_memory_limit = 0;\n> \n> +static long blob_size_limit = -1;\n> +\n\nWhich is the largest, long, ssize_t, or offset_t? Perhaps intmax_t\nbut it may be overkill?\n\nIn any case, not within the scope of this series; we assume that an\nindividual object's size fits in ulong around here.\n\n> @@ -776,6 +778,50 @@ static const char no_split_warning[] = N_(\n> \"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n> );\n> \n> +struct oversized_blob {\n> +\tstruct hashmap_entry entry;\n> +\tstruct object_id oid;\n> +\tunsigned long size;\n> +};\n> +struct hashmap oversized_blobs;\n> +\n> +static int oversized_blob_cmp_fn(const void *b1, const void *b2,\n> +\t\t\t\t const void *keydata)\n> +{\n> +\tconst struct object_id *oid2 = keydata ? keydata : \n> +\t\t&((const struct oversized_blob *) b2)->oid;\n> +\treturn oidcmp(&((const struct oversized_blob *) b1)->oid, oid2);\n> +}\n> +\n> +static int oversized_blob_cmp(const void *b1, const void *b2)\n> +{\n> +\treturn oidcmp(&(*(const struct oversized_blob **) b1)->oid,\n> +\t\t &(*(const struct oversized_blob **) b2)->oid);\n> +}\n> +\n> +static void write_oversized_info(void)\n> +{\n> +\tstruct oversized_blob **obs = xmalloc(oversized_blobs.size *\n> +\t\t\t\t\t sizeof(*obs));\n\nCan this multiplication overflow (hint: st_mult)?\n\n> +\tstruct hashmap_iter iter;\n> +\tstruct oversized_blob *ob;\n> +\tint i = 0;\n> +\tuint64_t size_network;\n> +\n> +\tfor (ob = hashmap_iter_first(&oversized_blobs, &iter);\n> +\t ob;\n> +\t ob = hashmap_iter_next(&iter))\n> +\t\tobs[i++] = ob;\n> +\tQSORT(obs, oversized_blobs.size, oversized_blob_cmp);\n\nThis sorting is a part of external contract (i.e. \"output in hash\norder\" is documented), but is it necessary? Just wondering.\n\n> +\tsize_network = htonll(oversized_blobs.size);\n> +\twrite_in_full(1, &size_network, sizeof(size_network));\n> +\tfor (i = 0; i < oversized_blobs.size; i++) {\n> +\t\twrite_in_full(1, &obs[i]->oid, sizeof(obs[i]->oid));\n> +\t\tsize_network = htonll(obs[i]->size);\n> +\t\twrite_in_full(1, &size_network, sizeof(size_network));\n> +\t}\n> +}\n\n> @@ -822,7 +868,11 @@ static void write_pack_file(void)\n> \t\t * If so, rewrite it like in fast-import\n> \t\t */\n> \t\tif (pack_to_stdout) {\n> -\t\t\tsha1close(f, sha1, CSUM_CLOSE);\n> +\t\t\tsha1close(f, sha1, 0);\n\nYou do want to exclude the trailing thing out of the checksum, but\nCSUM_CLOSE would close the fd and that is why you pass 0 here. OK.\n\n> +\t\t\twrite_in_full(1, sha1, sizeof(sha1));\n\nEven though we are in \"pack_to_stdout\" codepath, I'd prefer to\nconsistently use f->fd, not a hardcoded 1 here. Of course,\nsha1close() would have freed 'f' at this point, so we need to grab\nthe return value from the function to learn which fd is connected to\nour original \"stdout\" at this point.\n\nLikewise for write_oversized_info(), which probably should just take\n\"int fd\" as parameter.\n\n> +\t\t\tif (blob_size_limit >= 0) {\n> +\t\t\t\twrite_oversized_info();\n> +\t\t\t}\n\nI do not necessarily think it is a bad idea to show this as trailing\ndata after the pack, but when we come to this function, do we have\nenough information to make the \"oversize\" information appear before\nthe pack data if we wanted to? I have a suspicion that it is easier\nto handle at the receiving end, after the capability exchange\ndecides to use this \"omit oversized ones\" extension, to receive this\nbefore the packdata begins. If it is at the end, the receiver needs\nto spawn either unpack-objects or index-objects to cause it write to\nthe disk, but when it does so, it has to have a FD open to read from\ntheir standard output so that the receiver can grab the \"remainder\"\nthat these programs did not process. \"Write to the end of the pack\nstream data to these programs, and process the remainder ourselves\"\nis much harder to arrange, I'd suspect.\n\n> \t\t} else if (nr_written == nr_remaining) {\n> \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n> \t\t} else {\n> @@ -1069,17 +1119,58 @@ static const char no_closure_warning[] = N_(\n> \"disabling bitmap writing, as some objects are not being packed\"\n> );\n> \n> +/*\n> + * Returns 1 and records this blob in oversized_blobs if the given blob does\n> + * not meet any defined blob size limits. Blobs that appear as a tree entry\n> + * whose basename begins with \".git\" are never considered oversized.\n> + *\n> + * If the tree entry corresponding to the given blob is unknown, pass NULL as\n> + * name. In this case, this function will always return 0 to avoid false\n> + * positives.\n> + */\n> +static int oversized(const unsigned char *sha1, const char *name) {\n\nNot object_id?\n\n> +\tconst char *last_slash, *basename;\n> +\tunsigned long size;\n> +\tunsigned int hash;\n> +\n> +\tif (blob_size_limit < 0 || !name)\n> +\t\treturn 0;\n> +\n> +\tlast_slash = strrchr(name, '/');\n> +\tbasename = last_slash ? last_slash + 1 : name;\n> +\tif (starts_with(basename, \".git\"))\n> +\t\treturn 0;\n\nWhen we have two paths to a blob, and the blob is first fed to this\nfunction with one name that does not begin with \".git\", we would\nregister it to the hashmap. And then the other name that begins\nwith \".git\" is later discovered to point at the same blob, what\nhappens? Would we need to unregister it from the hashmap elsewhere\nin the code?\n\n> +\tsha1_object_info(sha1, &size);\n> +\tif (size < blob_size_limit)\n> +\t\treturn 0;\n> +\t\n> +\tif (!oversized_blobs.tablesize)\n> +\t\thashmap_init(&oversized_blobs, oversized_blob_cmp_fn, 0);\n> +\thash = sha1hash(sha1);\n> +\tif (!hashmap_get_from_hash(&oversized_blobs, hash, sha1)) {\n> +\t\tstruct oversized_blob *ob = xmalloc(sizeof(*ob));\n> +\t\thashmap_entry_init(ob, hash);\n> +\t\thashcpy(ob->oid.hash, sha1);\n> +\t\tob->size = size;\n> +\t\thashmap_add(&oversized_blobs, ob);\n> +\t}\n> +\treturn 1;\n> +}\n> +\n> static int add_object_entry(const unsigned char *sha1, enum object_type type,\n> -\t\t\t const char *name, int exclude)\n> +\t\t\t const char *name, int preferred_base)\n\nThis belongs to the previous step that renamed \"exclude\", no?\n\n> {\n> \tstruct packed_git *found_pack = NULL;\n> \toff_t found_offset = 0;\n> \tuint32_t index_pos;\n> +\tstruct hashmap_entry entry;\n> \n> -\tif (have_duplicate_entry(sha1, exclude, &index_pos))\n> +\tif (have_duplicate_entry(sha1, preferred_base, &index_pos))\n> \t\treturn 0;\n> \n> -\tif (ignore_object(sha1, exclude, &found_pack, &found_offset)) {\n> +\tif (ignore_object(sha1, preferred_base, &found_pack, &found_offset) ||\n> +\t (!preferred_base && type == OBJ_BLOB && oversized(sha1, name))) {\n> \t\t/* The pack is missing an object, so it will not have closure */\n> \t\tif (write_bitmap_index) {\n> \t\t\twarning(_(no_closure_warning));\n> @@ -1088,8 +1179,17 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n> \t\treturn 0;\n> \t}\n> \n> +\t/*\n> +\t * Ensure that we do not report this blob as oversized if we are going\n> +\t * to use it, be it in the returned pack or as a preferred base\n> +\t */\n> +\tif (oversized_blobs.tablesize) {\n> +\t\thashmap_entry_init(&entry, sha1hash(sha1));\n> +\t\tfree(hashmap_remove(&oversized_blobs, &entry, sha1));\n> +\t}\n> +\n\nAh, is this where the \"unregistering\" happens?\n\n> \tcreate_object_entry(sha1, type, pack_name_hash(name),\n> -\t\t\t exclude, name && no_try_delta(name),\n> +\t\t\t preferred_base, name && no_try_delta(name),\n> \t\t\t index_pos, found_pack, found_offset);\n> \n> \tdisplay_progress(progress_state, nr_result);\n"},{"id":"321426","messageId":"cover.1496432147.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":"cover.1496361873.git.jonathantanmy@google.com","subject":"[WIP v2 0/2] Modifying pack objects to support --blob-max-bytes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T19:38:43Z","receivedAt":"2017-06-02T19:38:56Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here's a new version addressing Junio's comments.\n\n> Hmph, that statement is a hard to read and agree to. I thought an\n> ignored object that is not going to be packed is one that won't hit\n> to_pack?\n\nThat is true currently, but will not be the full truth once the 2nd\npatch is applied. I have expanded the commit message to explain the\nplan in more detail.\n\n> It would be nice if the name of the parameter hints that this is\n> counted in bytes, e.g. \"--blob-max-bytes\"; \n\nDone. Originally I wanted to avoid terms like \"max\" because this\ndisallowed the user from requesting that even 0-byte blobs not be sent,\nbut realized that I probably shouldn't bother about that. :-)\n\n> Do we need to future-proof the output format so that we can later\n> use 32-byte hash? The input to pack-objects (i.e. rev-list --objects)\n> is hexadecimal text, and it may not be so bad to make this also\n> text, e.g. \" SP LF\". That way, you do not have to\n> worry about byte order, hash size, or length limited to uint64.\n\nThe reason for using binary is for the convenience of the client to\navoid another conversion before storing it to disk (and also network\nsavings). In a large repo, I think this list will be quite large. I\nrealized that I didn't mention anything about this in the commit\nmessage, so I have added an explanation.\n\nI think this is sufficiently future-proof in that the format of this\nhash matches the format of the hashes used in the objects in the\npackfile. As for object size being limited to uint64, I think the\nbenefits of the space savings (in using binary) outweigh the small risk\nthat our files will get larger than that before we upgrade our protocol\n:-P\n\n>> ++\n>> +If pack-objects cannot efficiently determine, for an arbitrary blob,\n>> +that no to-be-packed tree has that blob with a filename beginning with\n>> +\".git\" (for example, if bitmaps are used to calculate the objects to be\n>> +packed), it will pack all blobs regardless of size.\n>> +\n> This is a bit hard to understand, at least to me. Let me step-wise\n> deconstruct what I think you are saying.\n> \n> - A blob can appear in multiple trees, and each tree has name\n> (i.e. pathname component) for the blob.\n> \n> - Sometimes we may know _all_ trees that contain a particular\n> blob and hence _all_ names these trees call this blob. In such a\n> case, we can _prove_ that the blob never is used as \".git\"\n> in any tree.\n> \n> - We exclude a blob that is larger than the specified limit, but\n> only when we can prove the above. If it is possible that the\n> blob might appear as \".git\" in some tree, the blob is\n> not omitted no matter its size.\n> \n> And this is a very conservative way to avoid omitting something that\n> could be one of those control files like \".gitignore\".\n> \n> Am I following your thought correctly?\n\nWe only need to prove for *to-be-packed* trees (the trees that\npack-objects are about to pack in the packfile(s) that it is printing),\nbut otherwise, you are correct. I was trying to be thorough in saying\nthat \"if we don't have bitmaps, we know if a blob's name starts with\n'.git', so we can do filtering by size, but if we have bitmaps, we don't\nhave any relevant name information\" - any rewording suggestions are\nappreciated.\n\n> Can this multiplication overflow (hint: st_mult)?\n\nThanks - used st_mult.\n\n> This sorting is a part of external contract (i.e. \"output in hash\n> order\" is documented), but is it necessary? Just wondering.\n\nIt is convenient for the client because the client can then store it\ndirectly and binary search when accessing it. (Added a note about\nconvenience to the commit message.)\n\n> You do want to exclude the trailing thing out of the checksum, but\n> CSUM_CLOSE would close the fd and that is why you pass 0 here. OK.\n>\n> Even though we are in \"pack_to_stdout\" codepath, I'd prefer to\n> consistently use f->fd, not a hardcoded 1 here. Of course,\n> sha1close() would have freed 'f' at this point, so we need to grab\n> the return value from the function to learn which fd is connected to\n> our original \"stdout\" at this point.\n> \n> Likewise for write_oversized_info(), which probably should just take\n> \"int fd\" as parameter.\n\nDone for write_oversized_info(), although now that we are moving the\ninvocation of write_oversized_info() out of write_pack_file(), this\nmight not be so important. (For the rest, I have reverted the code\nbecause we are now printing the excluded hashes before the packfile\ninstead of after.)\n\n> I do not necessarily think it is a bad idea to show this as trailing\n> data after the pack, but when we come to this function, do we have\n> enough information to make the \"oversize\" information appear before\n> the pack data if we wanted to? I have a suspicion that it is easier\n> to handle at the receiving end, after the capability exchange\n> decides to use this \"omit oversized ones\" extension, to receive this\n> before the packdata begins. If it is at the end, the receiver needs\n> to spawn either unpack-objects or index-objects to cause it write to\n> the disk, but when it does so, it has to have a FD open to read from\n> their standard output so that the receiver can grab the \"remainder\"\n> that these programs did not process. \"Write to the end of the pack\n> stream data to these programs, and process the remainder ourselves\"\n> is much harder to arrange, I'd suspect.\n\nYes, we can write at the beginning - done.\n\n> > +/*\n> > + * Returns 1 and records this blob in oversized_blobs if the given blob does\n> > + * not meet any defined blob size limits. Blobs that appear as a tree entry\n> > + * whose basename begins with \".git\" are never considered oversized.\n> > + *\n> > + * If the tree entry corresponding to the given blob is unknown, pass NULL as\n> > + * name. In this case, this function will always return 0 to avoid false\n> > + * positives.\n> > + */\n> > +static int oversized(const unsigned char *sha1, const char *name) { \n>\n> Not object_id?\n\nThis would require a \"(const struct object_id *) sha1\" cast in its\ncaller that potentially violates alignment constraints, as far as I\nknow, so I used \"const unsigned char *\" for now. If the object_id\nconversion doesn't include this file in the near future, I'll try to\nsend out a patch for this file.\n\n> When we have two paths to a blob, and the blob is first fed to this\n> function with one name that does not begin with \".git\", we would\n> register it to the hashmap. And then the other name that begins\n> with \".git\" is later discovered to point at the same blob, what\n> happens? Would we need to unregister it from the hashmap elsewhere\n> in the code?\n[snip]\n> Ah, is this where the \"unregistering\" happens?\n\nYes.\n\n> > static int add_object_entry(const unsigned char *sha1, enum object_type type,\n> > -\t\t\t const char *name, int exclude)\n> > +\t\t\t const char *name, int preferred_base) \n> \n> This belongs to the previous step that renamed \"exclude\", no?\n\nYes - done.\n\nJonathan Tan (2):\n pack-objects: rename want_.* to ignore_.*\n pack-objects: support --blob-max-bytes\n\n Documentation/git-pack-objects.txt | 19 ++++-\n builtin/pack-objects.c | 159 ++++++++++++++++++++++++++++++-------\n t/t5300-pack-object.sh | 71 +++++++++++++++++\n 3 files changed, 220 insertions(+), 29 deletions(-)\n\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321427","messageId":"efea86d5542d56a34211230b36c1e3e1b856570c.1496432147.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":"cover.1496432147.git.jonathantanmy@google.com","subject":"[WIP v2 1/2] pack-objects: rename want_.* to ignore_.*","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T19:38:44Z","receivedAt":"2017-06-02T19:39:00Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, in pack_objects, add_object_entry() distinguishes between 2\ntypes of non-preferred-base objects:\n\n (1) objects that should not be in \"to_pack\" because an option like\n --local or --honor-pack-keep is set\n (2) objects that should be in \"to_pack\"\n\nA subsequent commit will teach pack-objects to exclude certain objects\n(specifically, blobs that are larger than a user-given threshold and are\nnot potentially a Git special file [1]), but unlike in (1) above, these\nexclusions need to be reported. So now we have 3 types of\nnon-preferred-base objects:\n\n (a) objects that should not be in \"to_pack\" and should not be reported\n because an option like --local or --honor-pack-keep is set\n (b) objects that should not be in \"to_pack\" and should be reported\n because they are blobs that are oversized and non-Git-special\n (c) objects that should be in \"to_pack\"\n\nadd_object_entry() will be taught to distinguish between these 3 types\nby the following:\n\n - renaming want_found_object() and want_object_in_pack() to ignore_.*\n to make it clear that they only check for (a) (this commit)\n - adding a new function to check for (b) and using it within\n add_object_entry() (a subsequent commit)\n\nThe alternative would be to retain the names want_found_object() and\nwant_object_in_pack() and have them handle both the cases (a) and (b),\nbut that would result in more complicated code.\n\nWe also take the opportunity to use the terminology \"preferred_base\"\ninstead of \"excluded\" in these methods. It is true that preferred bases\nare not included in the final packfile generation, but at this point in\nthe code, there is no exclusion taking place - on the contrary, if\nsomething is \"excluded\", it is in fact guaranteed to be in to_pack.\n\n[1] For the purposes of pack_objects, a blob is a Git special file if it\n appears in a to-be-packed tree with a filename beginning with\n \".git\".\n\nSigned-off-by: Jonathan Tan \n---\n builtin/pack-objects.c | 56 +++++++++++++++++++++++++-------------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 80439047a..730fa3d85 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -946,12 +946,12 @@ static int have_duplicate_entry(const unsigned char *sha1,\n \treturn 1;\n }\n \n-static int want_found_object(int exclude, struct packed_git *p)\n+static int ignore_found_object(int preferred_base, struct packed_git *p)\n {\n-\tif (exclude)\n-\t\treturn 1;\n-\tif (incremental)\n+\tif (preferred_base)\n \t\treturn 0;\n+\tif (incremental)\n+\t\treturn 1;\n \n \t/*\n \t * When asked to do --local (do not include an object that appears in a\n@@ -969,19 +969,19 @@ static int want_found_object(int exclude, struct packed_git *p)\n \t */\n \tif (!ignore_packed_keep &&\n \t (!local || !have_non_local_packs))\n-\t\treturn 1;\n+\t\treturn 0;\n \n \tif (local && !p->pack_local)\n-\t\treturn 0;\n+\t\treturn 1;\n \tif (ignore_packed_keep && p->pack_local && p->pack_keep)\n-\t\treturn 0;\n+\t\treturn 1;\n \n \t/* we don't know yet; keep looking for more packs */\n \treturn -1;\n }\n \n /*\n- * Check whether we want the object in the pack (e.g., we do not want\n+ * Check whether we should ignore this object (e.g., we do not want\n * objects found in non-local stores if the \"--local\" option was used).\n *\n * If the caller already knows an existing pack it wants to take the object\n@@ -989,16 +989,16 @@ static int want_found_object(int exclude, struct packed_git *p)\n * function finds if there is any pack that has the object and returns the pack\n * and its offset in these variables.\n */\n-static int want_object_in_pack(const unsigned char *sha1,\n-\t\t\t int exclude,\n-\t\t\t struct packed_git **found_pack,\n-\t\t\t off_t *found_offset)\n+static int ignore_object(const unsigned char *sha1,\n+\t\t\t int preferred_base,\n+\t\t\t struct packed_git **found_pack,\n+\t\t\t off_t *found_offset)\n {\n \tstruct mru_entry *entry;\n-\tint want;\n+\tint ignore;\n \n-\tif (!exclude && local && has_loose_object_nonlocal(sha1))\n-\t\treturn 0;\n+\tif (!preferred_base && local && has_loose_object_nonlocal(sha1))\n+\t\treturn 1;\n \n \t/*\n \t * If we already know the pack object lives in, start checks from that\n@@ -1006,9 +1006,9 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t * are present we will determine the answer right now.\n \t */\n \tif (*found_pack) {\n-\t\twant = want_found_object(exclude, *found_pack);\n-\t\tif (want != -1)\n-\t\t\treturn want;\n+\t\tignore = ignore_found_object(preferred_base, *found_pack);\n+\t\tif (ignore != -1)\n+\t\t\treturn ignore;\n \t}\n \n \tfor (entry = packed_git_mru->head; entry; entry = entry->next) {\n@@ -1027,15 +1027,15 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t\t*found_offset = offset;\n \t\t\t\t*found_pack = p;\n \t\t\t}\n-\t\t\twant = want_found_object(exclude, p);\n-\t\t\tif (!exclude && want > 0)\n+\t\t\tignore = ignore_found_object(preferred_base, p);\n+\t\t\tif (!preferred_base && ignore > 0)\n \t\t\t\tmru_mark(packed_git_mru, entry);\n-\t\t\tif (want != -1)\n-\t\t\t\treturn want;\n+\t\t\tif (ignore != -1)\n+\t\t\t\treturn ignore;\n \t\t}\n \t}\n \n-\treturn 1;\n+\treturn 0;\n }\n \n static void create_object_entry(const unsigned char *sha1,\n@@ -1070,16 +1070,16 @@ static const char no_closure_warning[] = N_(\n );\n \n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n-\t\t\t const char *name, int exclude)\n+\t\t\t const char *name, int preferred_base)\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n \tuint32_t index_pos;\n \n-\tif (have_duplicate_entry(sha1, exclude, &index_pos))\n+\tif (have_duplicate_entry(sha1, preferred_base, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset)) {\n+\tif (ignore_object(sha1, preferred_base, &found_pack, &found_offset)) {\n \t\t/* The pack is missing an object, so it will not have closure */\n \t\tif (write_bitmap_index) {\n \t\t\twarning(_(no_closure_warning));\n@@ -1089,7 +1089,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t}\n \n \tcreate_object_entry(sha1, type, pack_name_hash(name),\n-\t\t\t exclude, name && no_try_delta(name),\n+\t\t\t preferred_base, name && no_try_delta(name),\n \t\t\t index_pos, found_pack, found_offset);\n \n \tdisplay_progress(progress_state, nr_result);\n@@ -1106,7 +1106,7 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,\n \tif (have_duplicate_entry(sha1, 0, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, 0, &pack, &offset))\n+\tif (ignore_object(sha1, 0, &pack, &offset))\n \t\treturn 0;\n \n \tcreate_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321428","messageId":"6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com","threadId":"46094","inReplyTo":"cover.1496432147.git.jonathantanmy@google.com","subject":"[WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-02T19:38:45Z","receivedAt":"2017-06-02T19:39:05Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As part of an effort to improve Git support for very large repositories\nin which clients typically have only a subset of all version-controlled\nblobs, teach pack-objects to support --blob-max-bytes, packing only\nblobs not exceeding that size unless the blob corresponds to a file\nwhose name starts with \".git\". upload-pack will eventually be taught to\nuse this new parameter if needed to exclude certain blobs during a fetch\nor clone, potentially drastically reducing network consumption when\nserving these very large repositories.\n\nSince any excluded blob should not act as a delta base, I did this by\navoiding such oversized blobs from ever going into the \"to_pack\" data\nstructure, which contains both preferred bases (which do not go into the\nfinal packfile but can act as delta bases) and the objects to be packed\n(which go into the final packfile and also can act as delta bases). To\nthat end, add_object_entry() has been modified to exclude the\nappropriate non-preferred-base objects. (Preferred bases, regardless of\nsize, still enter \"to_pack\" - they are something that the client\nindicates that it has, not something that the server needs to serve, so\nno exclusion occurs.)\n\nIn addition, any such exclusions are recorded and sent before the\npackfile. These are recorded in binary format and in hash order, in a\nformat convenient for a client to use if stored directly to disk.\n\nThe other method in which objects are added to \"to_pack\",\nadd_object_entry_from_bitmap(), has not been modified - thus packing in\nthe presence of bitmaps still packs all blobs regardless of size. See\nthe documentation update in this commit for the rationale.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-pack-objects.txt | 19 ++++++-\n builtin/pack-objects.c | 105 ++++++++++++++++++++++++++++++++++++-\n t/t5300-pack-object.sh | 71 +++++++++++++++++++++++++\n 3 files changed, 193 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 8973510a4..23ff8b5be 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -12,7 +12,8 @@ SYNOPSIS\n 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied]\n \t[--no-reuse-delta] [--delta-base-offset] [--non-empty]\n \t[--local] [--incremental] [--window=] [--depth=]\n-\t[--revs [--unpacked | --all]] [--stdout | base-name]\n+\t[--revs [--unpacked | --all]]\n+\t[--stdout [--blob-max-bytes=] | base-name]\n \t[--shallow] [--keep-true-parents] < object-list\n \n \n@@ -231,6 +232,22 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.\n \tWith this option, parents that are hidden by grafts are packed\n \tnevertheless.\n \n+--blob-max-bytes=::\n+\tThis option can only be used with --stdout. If specified, a blob\n+\tlarger than this will not be packed unless a to-be-packed tree\n+\thas that blob with a filename beginning with \".git\". The size\n+\tcan be suffixed with \"k\", \"m\", or \"g\", and may be \"0\".\n++\n+If specified, after printing the packfile, pack-objects will print the\n+count of excluded blobs (8 bytes) . Subsequently, for each excluded blob\n+in hash order, pack-objects will print its hash (20 bytes) and its size\n+(8 bytes). All numbers are in network byte order.\n++\n+If pack-objects cannot efficiently determine, for an arbitrary blob,\n+that no to-be-packed tree has that blob with a filename beginning with\n+\".git\" (for example, if bitmaps are used to calculate the objects to be\n+packed), it will pack all blobs regardless of size.\n+\n SEE ALSO\n --------\n linkgit:git-rev-list[1]\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 730fa3d85..46578c05b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -77,6 +77,8 @@ static unsigned long cache_max_small_delta_size = 1000;\n \n static unsigned long window_memory_limit = 0;\n \n+static long blob_max_bytes = -1;\n+\n /*\n * stats\n */\n@@ -1069,17 +1071,73 @@ static const char no_closure_warning[] = N_(\n \"disabling bitmap writing, as some objects are not being packed\"\n );\n \n+struct oversized_blob {\n+\tstruct hashmap_entry entry;\n+\tstruct object_id oid;\n+\tunsigned long size;\n+};\n+struct hashmap oversized_blobs;\n+\n+static int oversized_blob_cmp_fn(const void *b1, const void *b2,\n+\t\t\t\t const void *keydata)\n+{\n+\tconst struct object_id *oid2 = keydata ? keydata :\n+\t\t&((const struct oversized_blob *) b2)->oid;\n+\treturn oidcmp(&((const struct oversized_blob *) b1)->oid, oid2);\n+}\n+\n+/*\n+ * Returns 1 and records this blob in oversized_blobs if the given blob does\n+ * not meet any defined blob size limits. Blobs that appear as a tree entry\n+ * whose basename begins with \".git\" are never considered oversized.\n+ *\n+ * If the tree entry corresponding to the given blob is unknown, pass NULL as\n+ * name. In this case, this function will always return 0 to avoid false\n+ * positives.\n+ */\n+static int oversized(const unsigned char *sha1, const char *name) {\n+\tconst char *last_slash, *basename;\n+\tunsigned long size;\n+\tunsigned int hash;\n+\n+\tif (blob_max_bytes < 0 || !name)\n+\t\treturn 0;\n+\n+\tlast_slash = strrchr(name, '/');\n+\tbasename = last_slash ? last_slash + 1 : name;\n+\tif (starts_with(basename, \".git\"))\n+\t\treturn 0;\n+\n+\tsha1_object_info(sha1, &size);\n+\tif (size <= blob_max_bytes)\n+\t\treturn 0;\n+\n+\tif (!oversized_blobs.tablesize)\n+\t\thashmap_init(&oversized_blobs, oversized_blob_cmp_fn, 0);\n+\thash = sha1hash(sha1);\n+\tif (!hashmap_get_from_hash(&oversized_blobs, hash, sha1)) {\n+\t\tstruct oversized_blob *ob = xmalloc(sizeof(*ob));\n+\t\thashmap_entry_init(ob, hash);\n+\t\thashcpy(ob->oid.hash, sha1);\n+\t\tob->size = size;\n+\t\thashmap_add(&oversized_blobs, ob);\n+\t}\n+\treturn 1;\n+}\n+\n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\t\t const char *name, int preferred_base)\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n \tuint32_t index_pos;\n+\tstruct hashmap_entry entry;\n \n \tif (have_duplicate_entry(sha1, preferred_base, &index_pos))\n \t\treturn 0;\n \n-\tif (ignore_object(sha1, preferred_base, &found_pack, &found_offset)) {\n+\tif (ignore_object(sha1, preferred_base, &found_pack, &found_offset) ||\n+\t (!preferred_base && type == OBJ_BLOB && oversized(sha1, name))) {\n \t\t/* The pack is missing an object, so it will not have closure */\n \t\tif (write_bitmap_index) {\n \t\t\twarning(_(no_closure_warning));\n@@ -1088,6 +1146,15 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\treturn 0;\n \t}\n \n+\t/*\n+\t * Ensure that we do not report this blob as oversized if we are going\n+\t * to use it, be it in the returned pack or as a preferred base\n+\t */\n+\tif (oversized_blobs.tablesize) {\n+\t\thashmap_entry_init(&entry, sha1hash(sha1));\n+\t\tfree(hashmap_remove(&oversized_blobs, &entry, sha1));\n+\t}\n+\n \tcreate_object_entry(sha1, type, pack_name_hash(name),\n \t\t\t preferred_base, name && no_try_delta(name),\n \t\t\t index_pos, found_pack, found_offset);\n@@ -2864,6 +2931,35 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n+static int oversized_blob_cmp(const void *b1, const void *b2)\n+{\n+\treturn oidcmp(&(*(const struct oversized_blob **) b1)->oid,\n+\t\t &(*(const struct oversized_blob **) b2)->oid);\n+}\n+\n+static void write_oversized_info(int fd)\n+{\n+\tstruct oversized_blob **obs = xmalloc(st_mult(oversized_blobs.size,\n+\t\t\t\t\t\t sizeof(*obs)));\n+\tstruct hashmap_iter iter;\n+\tstruct oversized_blob *ob;\n+\tint i = 0;\n+\tuint64_t size_network;\n+\n+\tfor (ob = hashmap_iter_first(&oversized_blobs, &iter);\n+\t ob;\n+\t ob = hashmap_iter_next(&iter))\n+\t\tobs[i++] = ob;\n+\tQSORT(obs, oversized_blobs.size, oversized_blob_cmp);\n+\tsize_network = htonll(oversized_blobs.size);\n+\twrite_in_full(fd, &size_network, sizeof(size_network));\n+\tfor (i = 0; i < oversized_blobs.size; i++) {\n+\t\twrite_in_full(fd, &obs[i]->oid, sizeof(obs[i]->oid));\n+\t\tsize_network = htonll(obs[i]->size);\n+\t\twrite_in_full(fd, &size_network, sizeof(size_network));\n+\t}\n+}\n+\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -2947,6 +3043,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n \t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n \t\t\t N_(\"write a bitmap index together with the pack index\")),\n+\t\tOPT_MAGNITUDE(0, \"blob-max-bytes\", &blob_max_bytes,\n+\t\t\t N_(\"exclude and report blobs larger than this\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3023,6 +3121,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n+\tif (blob_max_bytes >= 0 && !pack_to_stdout)\n+\t\tdie(\"if --blob-max-bytes is specified, --stdout must also be specified\");\n+\n \t/*\n \t * \"soft\" reasons not to use bitmaps - for on-disk repack by default we want\n \t *\n@@ -3089,6 +3190,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n+\tif (blob_max_bytes >= 0)\n+\t\twrite_oversized_info(1);\n \twrite_pack_file();\n \tif (progress)\n \t\tfprintf(stderr, \"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 43a672c34..987327784 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -421,6 +421,77 @@ test_expect_success 'index-pack works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n+unpack() {\n+\tperl -e '$/ = undef; $input = <>; print unpack(\"H2\" x length($input), $input)'\n+}\n+\n+lcut() {\n+\tperl -e '$/ = undef; $_ = <>; s/^.{'$1'}//s; print $_'\n+}\n+\n+test_expect_success '--blob-size-limit works with multiple excluded' '\n+\trm -rf server &&\n+\tgit init server &&\n+\tprintf a > server/a &&\n+\tprintf b > server/b &&\n+\tprintf c-very-long-file > server/c &&\n+\tprintf d-very-long-file > server/d &&\n+\tgit -C server add a b c d &&\n+\tgit -C server commit -m x &&\n+\n+\tgit -C server rev-parse HEAD >objects &&\n+\tgit -C server pack-objects --revs --stdout --blob-max-bytes=10 out &&\n+\tunpack actual &&\n+\n+\t# Check that the excluded hashes are included, sorted, at the beginning\n+\tif [ $(git hash-object server/c) \\< $(git hash-object server/d) ]\n+\tthen\n+\t\tprintf \"^0000000000000002%s0000000000000010%s0000000000000010\" \\\n+\t\t\t$(git hash-object server/c) \\\n+\t\t\t$(git hash-object server/d) >expect\n+\telse\n+\t\tprintf \"^0000000000000002%s0000000000000010%s0000000000000010\" \\\n+\t\t\t$(git hash-object server/d) \\\n+\t\t\t$(git hash-object server/c) >expect\n+\tfi &&\n+\tgrep $(cat expect) actual &&\n+\n+\t# Ensure that only the small blobs are in the packfile\n+\tlcut 64 my.pack &&\n+\tgit index-pack my.pack &&\n+\tgit verify-pack -v my.idx >objectlist &&\n+\tgrep $(git hash-object server/a) objectlist &&\n+\tgrep $(git hash-object server/b) objectlist &&\n+\t! grep $(git hash-object server/c) objectlist &&\n+\t! grep $(git hash-object server/d) objectlist\n+'\n+\n+test_expect_success '--blob-size-limit never excludes special files' '\n+\trm -rf server &&\n+\tgit init server &&\n+\tprintf a-very-long-file > server/a &&\n+\tprintf a-very-long-file > server/.git-a &&\n+\tprintf b-very-long-file > server/b &&\n+\tgit -C server add a .git-a b &&\n+\tgit -C server commit -m x &&\n+\n+\tgit -C server rev-parse HEAD >objects &&\n+\tgit -C server pack-objects --revs --stdout --blob-max-bytes=10 out &&\n+\tunpack actual &&\n+\n+\t# Check that the excluded hash is included at the beginning\n+\tprintf \"^0000000000000001%s0000000000000010\" \\\n+\t\t$(git hash-object server/b) >expect &&\n+\tgrep $(cat expect) actual &&\n+\n+\t# Ensure that the .git-a blob is in the packfile, despite also\n+\t# appearing as a non-.git file\n+\tlcut 36 my.pack &&\n+\tgit index-pack my.pack &&\n+\tgit verify-pack -v my.idx >objectlist &&\n+\tgrep $(git hash-object server/a) objectlist\n+'\n+\n #\n # WARNING!\n #\n-- \n2.13.0.506.g27d5fe0cd-goog\n\n"},{"id":"321449","messageId":"20170602221645.nsz6r6tgfndulc6c@sigill.intra.peff.net","threadId":"46094","inReplyTo":"cover.1496432147.git.jonathantanmy@google.com","subject":"Re: [WIP v2 0/2] Modifying pack objects to support --blob-max-bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T22:16:45Z","receivedAt":"2017-06-02T22:16:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 12:38:43PM -0700, Jonathan Tan wrote:\n\n> > Do we need to future-proof the output format so that we can later\n> > use 32-byte hash? The input to pack-objects (i.e. rev-list --objects)\n> > is hexadecimal text, and it may not be so bad to make this also\n> > text, e.g. \" SP LF\". That way, you do not have to\n> > worry about byte order, hash size, or length limited to uint64.\n> \n> The reason for using binary is for the convenience of the client to\n> avoid another conversion before storing it to disk (and also network\n> savings). In a large repo, I think this list will be quite large. I\n> realized that I didn't mention anything about this in the commit\n> message, so I have added an explanation.\n> \n> I think this is sufficiently future-proof in that the format of this\n> hash matches the format of the hashes used in the objects in the\n> packfile. As for object size being limited to uint64, I think the\n> benefits of the space savings (in using binary) outweigh the small risk\n> that our files will get larger than that before we upgrade our protocol\n> :-P\n\nThe rest of the pack code uses a varint encoding which is generally\nmuch smaller than a uint64 for most files, but can handle arbitrary\nsizes.\n\nThe one thing it loses is that you wouldn't have a fixed-size record, so\nif you were planning to dump this directly to disk and binary-search it,\nthat won't work. OTOH, you could make pseudo-pack-entries and just\nindex them along with the rest of the objects in the pack .idx.\n\nThe one subtle thing there is that the pseudo-entry would have to say\n\"this is my sha1\". And then we'd end up repeating that sha1 in the .idx\nfile. So it's optimal on the network but wastes 20 bytes on disk (unless\nindex-pack throws away the in-pack sha1s as it indexes, which is\ncertainly an option).\n\n> > Can this multiplication overflow (hint: st_mult)?\n> \n> Thanks - used st_mult.\n\nActually, I think this is a good candidate for ALLOC_ARRAY().\n\n> > This sorting is a part of external contract (i.e. \"output in hash\n> > order\" is documented), but is it necessary? Just wondering.\n> \n> It is convenient for the client because the client can then store it\n> directly and binary search when accessing it. (Added a note about\n> convenience to the commit message.)\n\nIn general the Git client doesn't trust the pack data coming from a\nremote, and you can't corrupt a client by sending it bogus data. Either\nthe client computes it from scratch (e.g., the sha1s of each object) or\nthe client will reject nonsense (missing bases, refs pointing to objects\nthat aren't sent, etc).\n\nI know this feature implies a certain amount of trust (after all, the\nserver could claim that it omitted any sha1 it likes), but we should\nprobably still be as strict as possible that what the other side is\nsending makes sense. In this case, we should probably hashcmp() each\nentry with the last and make sure they're strictly increasing (no\nout-of-order and no duplicates).\n\n-Peff\n"},{"id":"321451","messageId":"20170602222640.u6vni5tdpjp3sayt@sigill.intra.peff.net","threadId":"46094","inReplyTo":"6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-02T22:26:40Z","receivedAt":"2017-06-02T22:26:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 12:38:45PM -0700, Jonathan Tan wrote:\n\n> +--blob-max-bytes=::\n> +\tThis option can only be used with --stdout. If specified, a blob\n> +\tlarger than this will not be packed unless a to-be-packed tree\n> +\thas that blob with a filename beginning with \".git\". The size\n> +\tcan be suffixed with \"k\", \"m\", or \"g\", and may be \"0\".\n> ++\n> +If specified, after printing the packfile, pack-objects will print the\n> +count of excluded blobs (8 bytes) . Subsequently, for each excluded blob\n> +in hash order, pack-objects will print its hash (20 bytes) and its size\n> +(8 bytes). All numbers are in network byte order.\n> ++\n> +If pack-objects cannot efficiently determine, for an arbitrary blob,\n> +that no to-be-packed tree has that blob with a filename beginning with\n> +\".git\" (for example, if bitmaps are used to calculate the objects to be\n> +packed), it will pack all blobs regardless of size.\n\nHmm. So this feature can't be used with bitmaps at all? That seems like\na big downside, as we still have to walk the whole graph to come up with\nthe list of blobs (even if we're sending them). That's 30-40 seconds of\nCPU per clone on torvalds/linux. I imagine it's much worse on\nrepositories big enough to need a full GVFS-style \"don't send any blobs\"\napproach.\n\nWe have a name-hash cache extension in the bitmap file, but it doesn't\ncarry enough information to deduce the .git-ness of a file. I don't\nthink it would be too hard to add a \"flags\" extension, and give a single\nbit to \"this is a .git file\".\n\nI do also wonder if the two features would need to be separated for a\nGVFS-style solution. If you're not just avoiding large blobs but trying\nto get a narrow clone, you don't want the .git files from the\nuninteresting parts of the tree. You want to get no blobs at all, and\nthen fault them in as they become relevant due to user action.\n\n-Peff\n"},{"id":"321458","messageId":"20170602232508.GA21733@aiede.mtv.corp.google.com","threadId":"46094","inReplyTo":"20170602222640.u6vni5tdpjp3sayt@sigill.intra.peff.net","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-02T23:25:08Z","receivedAt":"2017-06-02T23:26:05Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Jun 02, 2017 at 12:38:45PM -0700, Jonathan Tan wrote:\n\n>> +--blob-max-bytes=::\n>> +\tThis option can only be used with --stdout. If specified, a blob\n>> +\tlarger than this will not be packed unless a to-be-packed tree\n>> +\thas that blob with a filename beginning with \".git\". The size\n>> +\tcan be suffixed with \"k\", \"m\", or \"g\", and may be \"0\".\n>> ++\n>> +If specified, after printing the packfile, pack-objects will print the\n>> +count of excluded blobs (8 bytes) . Subsequently, for each excluded blob\n>> +in hash order, pack-objects will print its hash (20 bytes) and its size\n>> +(8 bytes). All numbers are in network byte order.\n>> ++\n>> +If pack-objects cannot efficiently determine, for an arbitrary blob,\n>> +that no to-be-packed tree has that blob with a filename beginning with\n>> +\".git\" (for example, if bitmaps are used to calculate the objects to be\n>> +packed), it will pack all blobs regardless of size.\n>\n> Hmm. So this feature can't be used with bitmaps at all? That seems like\n> a big downside, as we still have to walk the whole graph to come up with\n> the list of blobs (even if we're sending them). That's 30-40 seconds of\n> CPU per clone on torvalds/linux. I imagine it's much worse on\n> repositories big enough to need a full GVFS-style \"don't send any blobs\"\n> approach.\n>\n> We have a name-hash cache extension in the bitmap file, but it doesn't\n> carry enough information to deduce the .git-ness of a file. I don't\n> think it would be too hard to add a \"flags\" extension, and give a single\n> bit to \"this is a .git file\".\n\nA nicer approach IMHO is to include an extra bitmap, like the existing\nobject-type bitmaps (see the dump_bitmap calls in\nbitmap_writer_finish). This would would represent the set of all\n.git* blobs in the pack.\n\n[...]\n> If you're not just avoiding large blobs but trying\n> to get a narrow clone, you don't want the .git files from the\n> uninteresting parts of the tree.\n\nThis is something I've wondered about, too. Part of the story is that\nwe haven't started omitting trees, so there is already O(number of\ntrees) objects being sent and some additional small blobs for .git*\nspecials doesn't make it much worse. Sending all .git* blobs keeps\nthings simple since the server doesn't have to infer which .git* blobs\nare relevant to this client.\n\nLonger term, we will likely want to allow clients to request omission\nof some trees, too. Omitting the corresponding .git* files becomes\nmore straightforward at that point.\n\nDoes that make sense?\n\nJonathan\n"},{"id":"321490","messageId":"xmqqinkcws2x.fsf@gitster.mtv.corp.google.com","threadId":"46094","inReplyTo":"20170602222640.u6vni5tdpjp3sayt@sigill.intra.peff.net","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-03T23:48:38Z","receivedAt":"2017-06-03T23:48:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do also wonder if the two features would need to be separated for a\n> GVFS-style solution. If you're not just avoiding large blobs but trying\n> to get a narrow clone, you don't want the .git files from the\n> uninteresting parts of the tree. You want to get no blobs at all, and\n> then fault them in as they become relevant due to user action.\n\nThanks; I didn't think of this one while reviewing the overall design,\nbut I agree that this is something important to think about.\n"},{"id":"321539","messageId":"20170605103523.190c9f16@twelve2.svl.corp.google.com","threadId":"46094","inReplyTo":"20170602221645.nsz6r6tgfndulc6c@sigill.intra.peff.net","subject":"Re: [WIP v2 0/2] Modifying pack objects to support --blob-max-bytes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-05T17:35:23Z","receivedAt":"2017-06-05T17:35:39Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for your comments.\n\nOn Fri, 2 Jun 2017 18:16:45 -0400\nJeff King wrote:\n\n> On Fri, Jun 02, 2017 at 12:38:43PM -0700, Jonathan Tan wrote:\n> \n> > > Do we need to future-proof the output format so that we can later\n> > > use 32-byte hash? The input to pack-objects (i.e. rev-list --objects)\n> > > is hexadecimal text, and it may not be so bad to make this also\n> > > text, e.g. \" SP LF\". That way, you do not have to\n> > > worry about byte order, hash size, or length limited to uint64.\n> > \n> > The reason for using binary is for the convenience of the client to\n> > avoid another conversion before storing it to disk (and also network\n> > savings). In a large repo, I think this list will be quite large. I\n> > realized that I didn't mention anything about this in the commit\n> > message, so I have added an explanation.\n> > \n> > I think this is sufficiently future-proof in that the format of this\n> > hash matches the format of the hashes used in the objects in the\n> > packfile. As for object size being limited to uint64, I think the\n> > benefits of the space savings (in using binary) outweigh the small risk\n> > that our files will get larger than that before we upgrade our protocol\n> > :-P\n> \n> The rest of the pack code uses a varint encoding which is generally\n> much smaller than a uint64 for most files, but can handle arbitrary\n> sizes.\n> \n> The one thing it loses is that you wouldn't have a fixed-size record, so\n> if you were planning to dump this directly to disk and binary-search it,\n> that won't work. OTOH, you could make pseudo-pack-entries and just\n> index them along with the rest of the objects in the pack .idx.\n> \n> The one subtle thing there is that the pseudo-entry would have to say\n> \"this is my sha1\". And then we'd end up repeating that sha1 in the .idx\n> file. So it's optimal on the network but wastes 20 bytes on disk (unless\n> index-pack throws away the in-pack sha1s as it indexes, which is\n> certainly an option).\n\nIf we end up going with the varint approach (which seems reasonable),\nmaybe the client could just expand the varints into uint64s so that it\nhas a binary-searchable file. I think it's better to keep this list\nseparate from the pack .idx file (there has been some discussion on this\n- [1] and its replies).\n\n[1] https://public-inbox.org/git/777ab8f2-c31a-d07b-ffe3-f8333f408ea1@jeffhostetler.com/\n\n> > > Can this multiplication overflow (hint: st_mult)?\n> > \n> > Thanks - used st_mult.\n> \n> Actually, I think this is a good candidate for ALLOC_ARRAY().\n\nThanks - I've made this change in my local version.\n\n> > > This sorting is a part of external contract (i.e. \"output in hash\n> > > order\" is documented), but is it necessary? Just wondering.\n> > \n> > It is convenient for the client because the client can then store it\n> > directly and binary search when accessing it. (Added a note about\n> > convenience to the commit message.)\n> \n> In general the Git client doesn't trust the pack data coming from a\n> remote, and you can't corrupt a client by sending it bogus data. Either\n> the client computes it from scratch (e.g., the sha1s of each object) or\n> the client will reject nonsense (missing bases, refs pointing to objects\n> that aren't sent, etc).\n> \n> I know this feature implies a certain amount of trust (after all, the\n> server could claim that it omitted any sha1 it likes), but we should\n> probably still be as strict as possible that what the other side is\n> sending makes sense. In this case, we should probably hashcmp() each\n> entry with the last and make sure they're strictly increasing (no\n> out-of-order and no duplicates).\n\nGood point.\n"},{"id":"321665","messageId":"20170607094641.pibxlk5nbyii5p63@sigill.intra.peff.net","threadId":"46094","inReplyTo":"20170605103523.190c9f16@twelve2.svl.corp.google.com","subject":"Re: [WIP v2 0/2] Modifying pack objects to support --blob-max-bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-07T09:46:41Z","receivedAt":"2017-06-07T09:47:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 05, 2017 at 10:35:23AM -0700, Jonathan Tan wrote:\n\n> > The rest of the pack code uses a varint encoding which is generally\n> > much smaller than a uint64 for most files, but can handle arbitrary\n> > sizes.\n> > \n> > The one thing it loses is that you wouldn't have a fixed-size record, so\n> > if you were planning to dump this directly to disk and binary-search it,\n> > that won't work. OTOH, you could make pseudo-pack-entries and just\n> > index them along with the rest of the objects in the pack .idx.\n> > \n> > The one subtle thing there is that the pseudo-entry would have to say\n> > \"this is my sha1\". And then we'd end up repeating that sha1 in the .idx\n> > file. So it's optimal on the network but wastes 20 bytes on disk (unless\n> > index-pack throws away the in-pack sha1s as it indexes, which is\n> > certainly an option).\n> \n> If we end up going with the varint approach (which seems reasonable),\n> maybe the client could just expand the varints into uint64s so that it\n> has a binary-searchable file. I think it's better to keep this list\n> separate from the pack .idx file (there has been some discussion on this\n> - [1] and its replies).\n> \n> [1] https://public-inbox.org/git/777ab8f2-c31a-d07b-ffe3-f8333f408ea1@jeffhostetler.com/\n\nOK. If we're keeping it separate anyway, then I agree that just\nexpanding the varints is a good solution. And we don't have to care too\nmuch about the local storage, because it can be changed out later\nwithout touching the on-the-wire protocol.\n\n-Peff\n"},{"id":"321667","messageId":"20170607094440.vg5ogy4bp5kg53s3@sigill.intra.peff.net","threadId":"46094","inReplyTo":"20170602232508.GA21733@aiede.mtv.corp.google.com","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-07T09:44:40Z","receivedAt":"2017-06-07T09:50:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 02, 2017 at 04:25:08PM -0700, Jonathan Nieder wrote:\n\n> > We have a name-hash cache extension in the bitmap file, but it doesn't\n> > carry enough information to deduce the .git-ness of a file. I don't\n> > think it would be too hard to add a \"flags\" extension, and give a single\n> > bit to \"this is a .git file\".\n> \n> A nicer approach IMHO is to include an extra bitmap, like the existing\n> object-type bitmaps (see the dump_bitmap calls in\n> bitmap_writer_finish). This would would represent the set of all\n> .git* blobs in the pack.\n\nYeah, it could be stored as a bitmap, which would be slightly smaller\n(since it would be mostly 0's). I think either way it would need an\nextension flag in the header to signal its presence.\n\nOlder versions of Git are OK with having flags they don't understand. I\nknow JGit used to complain about seeing a bitmap with unknown flags, but\nI'm not sure if that is still the case.\n\n> > If you're not just avoiding large blobs but trying\n> > to get a narrow clone, you don't want the .git files from the\n> > uninteresting parts of the tree.\n> \n> This is something I've wondered about, too. Part of the story is that\n> we haven't started omitting trees, so there is already O(number of\n> trees) objects being sent and some additional small blobs for .git*\n> specials doesn't make it much worse. Sending all .git* blobs keeps\n> things simple since the server doesn't have to infer which .git* blobs\n> are relevant to this client.\n> \n> Longer term, we will likely want to allow clients to request omission\n> of some trees, too. Omitting the corresponding .git* files becomes\n> more straightforward at that point.\n> \n> Does that make sense?\n\nYeah, I agree we'd want to avoid the trees, too, in that case.\n\n-Peff\n"},{"id":"322384","messageId":"14b1fbb0-d4fb-1dd3-936c-deea6d20a99b@jeffhostetler.com","threadId":"46094","inReplyTo":"20170602222640.u6vni5tdpjp3sayt@sigill.intra.peff.net","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-15T20:28:24Z","receivedAt":"2017-06-15T20:28:31Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 6/2/2017 6:26 PM, Jeff King wrote:\n> On Fri, Jun 02, 2017 at 12:38:45PM -0700, Jonathan Tan wrote:\n...\n> We have a name-hash cache extension in the bitmap file, but it doesn't\n> carry enough information to deduce the .git-ness of a file. I don't\n> think it would be too hard to add a \"flags\" extension, and give a single\n> bit to \"this is a .git file\".\n> \n> I do also wonder if the two features would need to be separated for a\n> GVFS-style solution. If you're not just avoiding large blobs but trying\n> to get a narrow clone, you don't want the .git files from the\n> uninteresting parts of the tree. You want to get no blobs at all, and\n> then fault them in as they become relevant due to user action.\n> \n> -Peff\n> \n\nI agree with Peff here. I've been working on my partial/narrow/sparse\nclone/fetch ideas since my original RFC and have come to the conclusion\nthat the server can do the size limiting efficiently, but we should\nleave the pathname filtering to the client. That is, let the client\nget the commits and trees and then locally apply pattern matching,\nwhether that be a sparse-checkout definition or simple \".git*\"\nmatching and then make a later request for the \"blobs of interest\".\n\nWhether we \"fault-in\" the missing blobs or have a \"fetch-blobs\"\ncommand (like fetch-pack) to get them is open to debate.\n\nThere are concerns about the size of the requested blob-id list in a\nfetch-blobs approach, but I think there are ways to say I need all\nof the blobs referenced by the directory /foo in commit xxxx (and\noptionally, that aren't present in directory /foo in commit yyyy\nor in the range yyyy..xxxx). (handwave)\n\nJeff\n"},{"id":"322393","messageId":"20170615140305.4f79f5f0@twelve2.svl.corp.google.com","threadId":"46094","inReplyTo":"14b1fbb0-d4fb-1dd3-936c-deea6d20a99b@jeffhostetler.com","subject":"Re: [WIP v2 2/2] pack-objects: support --blob-max-bytes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-15T21:03:05Z","receivedAt":"2017-06-15T21:03:15Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 15 Jun 2017 16:28:24 -0400\nJeff Hostetler wrote:\n\n> I agree with Peff here. I've been working on my partial/narrow/sparse\n> clone/fetch ideas since my original RFC and have come to the conclusion\n> that the server can do the size limiting efficiently, but we should\n> leave the pathname filtering to the client. That is, let the client\n> get the commits and trees and then locally apply pattern matching,\n> whether that be a sparse-checkout definition or simple \".git*\"\n> matching and then make a later request for the \"blobs of interest\".\n\nThis means that the client would need to inflate all the trees it\nreceived, but that might not be that bad.\n\n> Whether we \"fault-in\" the missing blobs or have a \"fetch-blobs\"\n> command (like fetch-pack) to get them is open to debate.\n> \n> There are concerns about the size of the requested blob-id list in a\n> fetch-blobs approach, but I think there are ways to say I need all\n> of the blobs referenced by the directory /foo in commit xxxx (and\n> optionally, that aren't present in directory /foo in commit yyyy\n> or in the range yyyy..xxxx). (handwave)\n\nUnless the server no longer has the relevant commit (e.g. because a\nbranch was rewound), but even then, even if we only sent blob hashes,\nthe list would be probably much smaller than the downloaded pack anyway,\nso things might still be OK.\n"}]}