{"thread":{"id":"46235","subject":"[PATCH 0/3] WIP list-objects and pack-objects for partial clone","startedAt":"2017-06-22T20:36:26Z","lastAt":"2017-06-28T17:55:06Z","messageCount":12,"participants":["Jeff Hostetler","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"322987","messageId":"20170622203615.34135-1-git@jeffhostetler.com","threadId":"46235","inReplyTo":null,"subject":"[PATCH 0/3] WIP list-objects and pack-objects for partial clone","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-22T20:36:12Z","receivedAt":"2017-06-22T20:36:26Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nThis WIP is a follow up to earlier patches to teach pack-objects\nto omit large blobs from packfiles. This doesn't attempt to solve\nthe whole end-to-end problem of partial/sparse clone/fetch or that\nof the client operating with missing blobs. This WIP is for now\nlimited to building the packfile with omitted blobs and hopefully\ncan mesh nicely with Jonathan Tan's work in [3].\n\nIt supports filtering by size while always including blobs associated\nwith \".git*\" paths, something we both proposed in [1] and [3].\n\nThe approach here differs from [1] and [3] in that it extends\ntraverse_commit_list() to allow custom blob filtering using a new\ncallback provided by pack-objects. This should make it easier to\ndo other filters laters. Part of this based upon Peff's suggestion\nabout rev-list in [2]. I have not updated the rev-list command,\nbut rather the routines in list-objects.c that it calls. Jonathan's\nideas in [3] to build and send the omitted blobs list means that I\nthink we need pack-objects.c manage the filter-proc used here.\n\nI considered, but omitted from this version, ideas to allow the\nfilter-proc to know of the process_tree() boundaries which might\nlet pack-objects filter by sub-tree (think sparse-checkout) as\nsuggested in [4] and various replies.\n\n[1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffhost@microsoft.com/\n[2] https://public-inbox.org/git/20170309073117.g3br5btsfwntcdpe@sigill.intra.peff.net/\n[3] https://public-inbox.org/git/cover.1496361873.git.jonathantanmy@google.com/\n[4] https://public-inbox.org/git/20170602232508.GA21733@aiede.mtv.corp.google.com/\n\n\nJeff Hostetler (3):\n list-objects: add filter_blob to traverse_commit_list\n pack-objects: WIP add max-blob-size filtering\n pack-objects: add t5317 to test max-blob-size\n\n builtin/pack-objects.c | 76 +++++++++++++++++++++++++++++++++-\n list-objects.c | 39 +++++++++++++++--\n list-objects.h | 8 ++++\n t/t5317-pack-objects-blob-filtering.sh | 68 ++++++++++++++++++++++++++++++\n 4 files changed, 186 insertions(+), 5 deletions(-)\n create mode 100644 t/t5317-pack-objects-blob-filtering.sh\n\n-- \n2.9.3\n\n"},{"id":"322988","messageId":"20170622203615.34135-3-git@jeffhostetler.com","threadId":"46235","inReplyTo":"20170622203615.34135-1-git@jeffhostetler.com","subject":"[PATCH 2/3] pack-objects: WIP add max-blob-size filtering","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-22T20:36:14Z","receivedAt":"2017-06-22T20:36:28Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nTeach pack-objects command to accept --max-blob-size= argument\nand use a traverse_commit_list filter-proc to omit unwanted blobs\nfrom the resulting packfile.\n\nThis filter-proc always includes special files matching \".git*\"\n(such as \".gitignore\") and blobs smaller than . is a\nmagnitude value and accepts [kmg] suffixes. A value of zero\ncan be used to omit all blobs (except for special files).\n\nThere are 2 placeholder TODOs in this code to talk about building\nan omitted-blob list for the client.\n\nSigned-off-by: Jeff Hostetler \n---\n builtin/pack-objects.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 75 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 50e01aa..cdcd4d6 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 signed long max_blob_size = -1;\n+\n /*\n * stats\n */\n@@ -2519,6 +2521,7 @@ static void read_object_list_from_stdin(void)\n }\n \n #define OBJECT_ADDED (1u<<20)\n+#define BLOB_OMITTED (1u<<21)\n \n static void show_commit(struct commit *commit, void *data)\n {\n@@ -2536,6 +2539,70 @@ static void show_object(struct object *obj, const char *name, void *data)\n \tobj->flags |= OBJECT_ADDED;\n }\n \n+/*\n+ * Filter blobs by pathname or size.\n+ * Return 1 to mark the blob SEEN so that it will not be reported again.\n+ * Return 0 to allow it to be presented again.\n+ */\n+static int filter_blob(\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *entryname,\n+\tvoid *data)\n+{\n+\tassert(obj->type == OBJ_BLOB);\n+\tassert((obj->flags & SEEN) == 0);\n+\tassert((obj->flags & OBJECT_ADDED) == 0);\n+\tassert(max_blob_size >= 0);\n+\n+\t/*\n+\t * Always include blobs for special files of the form \".git*\".\n+\t */\n+\tif ((strncmp(entryname, \".git\", 4) == 0) && entryname[4]) {\n+\t\tif (obj->flags & BLOB_OMITTED) {\n+\t\t\t/*\n+\t\t\t * TODO\n+\t\t\t * TODO Remove this blob from the omitted blob list.\n+\t\t\t * TODO\n+\t\t\t */\n+\t\t\tobj->flags &= ~BLOB_OMITTED;\n+\t\t}\n+\t\tshow_object(obj, pathname, data);\n+\t\treturn 1;\n+\t}\n+\n+\t/*\n+\t * We already know the blob is too big because it was previously\n+\t * omitted. We still don't want it yet. DO NOT mark it SEEN\n+\t * in case it is associated with a \".git*\" path in another tree\n+\t * or commit.\n+\t */\n+\tif (obj->flags & BLOB_OMITTED)\n+\t\treturn 0;\n+\n+\t/*\n+\t * We only want blobs that are LESS THAN the maximum.\n+\t * This allows zero to mean NO BLOBS.\n+\t */\n+\tif (max_blob_size > 0) {\n+\t\tunsigned long s;\n+\t\tenum object_type t = sha1_object_info(obj->oid.hash, &s);\n+\t\tassert(t == OBJ_BLOB);\n+\t\tif (s < max_blob_size) {\n+\t\t\tshow_object(obj, pathname, data);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * TODO\n+\t * TODO (Provisionally) add this blob to the omitted blob list.\n+\t * TODO\n+\t */\n+\tobj->flags |= BLOB_OMITTED;\n+\treturn 0;\n+}\n+\n static void show_edge(struct commit *commit)\n {\n \tadd_preferred_base(commit->object.oid.hash);\n@@ -2800,7 +2867,12 @@ static void get_object_list(int ac, const char **av)\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \tmark_edges_uninteresting(&revs, show_edge);\n-\ttraverse_commit_list(&revs, show_commit, show_object, NULL);\n+\n+\tif (max_blob_size == -1)\n+\t\ttraverse_commit_list(&revs, show_commit, show_object, NULL);\n+\telse\n+\t\ttraverse_commit_list_filtered(&revs, show_commit, show_object,\n+\t\t\tfilter_blob, NULL);\n \n \tif (unpack_unreachable_expiration) {\n \t\trevs.ignore_missing_links = 1;\n@@ -2936,6 +3008,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, \"max-blob-size\", (unsigned long *)&max_blob_size,\n+\t\t\t\t\t N_(\"omit large blobs from packfile\")),\n \t\tOPT_END(),\n \t};\n \n-- \n2.9.3\n\n"},{"id":"322989","messageId":"20170622203615.34135-4-git@jeffhostetler.com","threadId":"46235","inReplyTo":"20170622203615.34135-1-git@jeffhostetler.com","subject":"[PATCH 3/3] pack-objects: add t5317 to test max-blob-size","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-22T20:36:15Z","receivedAt":"2017-06-22T20:36:30Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nSigned-off-by: Jeff Hostetler \n---\n t/t5317-pack-objects-blob-filtering.sh | 68 ++++++++++++++++++++++++++++++++++\n 1 file changed, 68 insertions(+)\n create mode 100644 t/t5317-pack-objects-blob-filtering.sh\n\ndiff --git a/t/t5317-pack-objects-blob-filtering.sh b/t/t5317-pack-objects-blob-filtering.sh\nnew file mode 100644\nindex 0000000..58124ab\n--- /dev/null\n+++ b/t/t5317-pack-objects-blob-filtering.sh\n@@ -0,0 +1,68 @@\n+#!/bin/sh\n+\n+test_description='pack-objects blob filtering'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tprintf \"%10s\" X >x10.txt &&\n+\tprintf \"%100s\" X >x100.txt &&\n+\tprintf \"%1000s\" X >x1000.txt &&\n+\tgit add *.txt &&\n+\tgit commit -m txt\n+'\n+\n+test_expect_success 'all blobs' '\n+\ttest_when_finished \"rm -f *.pack *.idx\" &&\n+\tgit pack-objects --revs --thin --stdout >z.pack <<-EOF &&\n+\tmaster\n+\n+\tEOF\n+\tgit index-pack z.pack &&\n+\ttest 3 = $(git verify-pack -v z.pack | grep blob | wc -l)\n+'\n+\n+test_expect_success 'no blobs (max equals 0)' '\n+\ttest_when_finished \"rm -f *.pack *.idx\" &&\n+\tgit pack-objects --revs --thin --stdout --max-blob-size=0 >z.pack <<-EOF &&\n+\tmaster\n+\n+\tEOF\n+\tgit index-pack z.pack &&\n+\ttest 0 = $(git verify-pack -v z.pack | grep blob | wc -l)\n+'\n+\n+test_expect_success 'small 20 blobs' '\n+\ttest_when_finished \"rm -f *.pack *.idx\" &&\n+\tgit pack-objects --revs --thin --stdout --max-blob-size=20 >z.pack <<-EOF &&\n+\tmaster\n+\n+\tEOF\n+\tgit index-pack z.pack &&\n+\ttest 1 = $(git verify-pack -v z.pack | grep blob | wc -l)\n+'\n+\n+test_expect_success 'small 200 blobs' '\n+\ttest_when_finished \"rm -f *.pack *.idx\" &&\n+\tgit pack-objects --revs --thin --stdout --max-blob-size=200 >z.pack <<-EOF &&\n+\tmaster\n+\n+\tEOF\n+\tgit index-pack z.pack &&\n+\ttest 2 = $(git verify-pack -v z.pack | grep blob | wc -l)\n+'\n+\n+test_expect_success 'special files always present' '\n+\ttest_when_finished \"rm -f *.pack *.idx\" &&\n+\tcp x1000.txt .gitignore &&\n+\tgit add .gitignore &&\n+\tgit commit -m \"add ignores\" &&\n+\tgit pack-objects --revs --stdout --max-blob-size=0 >z.pack <<-EOF &&\n+\tmaster\n+\n+\tEOF\n+\tgit index-pack z.pack &&\n+\ttest 1 = $(git verify-pack -v z.pack | grep blob | wc -l)\n+'\n+\n+test_done\n-- \n2.9.3\n\n"},{"id":"322990","messageId":"20170622203615.34135-2-git@jeffhostetler.com","threadId":"46235","inReplyTo":"20170622203615.34135-1-git@jeffhostetler.com","subject":"[PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-22T20:36:13Z","receivedAt":"2017-06-22T20:36:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nIn preparation for partial/sparse clone/fetch where the\nserver is allowed to omit large/all blobs from the packfile,\nteach traverse_commit_list() to take a blob filter-proc that\ncontrols when blobs are shown and marked as SEEN.\n\nNormally, traverse_commit_list() always marks visited blobs\nas SEEN, so that the show_object() callback will never see\nduplicates. Since a single blob OID may be referenced by\nmultiple pathnames, we may not be able to decide if a blob\nshould be excluded until later pathnames have been traversed.\nWith the filter-proc, the automatic deduping is disabled.\n\nSigned-off-by: Jeff Hostetler \n---\n list-objects.c | 39 +++++++++++++++++++++++++++++++++++----\n list-objects.h | 8 ++++++++\n 2 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex f3ca6aa..c9ca81c 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,6 +11,7 @@\n static void process_blob(struct rev_info *revs,\n \t\t\t struct blob *blob,\n \t\t\t show_object_fn show,\n+\t\t\t filter_blob_fn filter_blob,\n \t\t\t struct strbuf *path,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n@@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,\n \t\tdie(\"bad blob object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n-\tobj->flags |= SEEN;\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tshow(obj, path->buf, cb_data);\n+\tif (!filter_blob) {\n+\t\t/*\n+\t\t * Normal processing is to imediately dedup blobs\n+\t\t * during commit traversal, regardless of how many\n+\t\t * times it appears in a single or multiple commits,\n+\t\t * so we always set SEEN.\n+\t\t */\n+\t\tobj->flags |= SEEN;\n+\t\tshow(obj, path->buf, cb_data);\n+\t} else {\n+\t\t/*\n+\t\t * Use the filter-proc to decide whether to show\n+\t\t * the blob. We only set SEEN if requested. For\n+\t\t * example, this could be used to omit a specific\n+\t\t * blob until it appears with a \".git*\" entryname.\n+\t\t */\n+\t\tif (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))\n+\t\t\tobj->flags |= SEEN;\n+\t}\n \tstrbuf_setlen(path, pathlen);\n }\n \n@@ -67,6 +85,7 @@ static void process_gitlink(struct rev_info *revs,\n static void process_tree(struct rev_info *revs,\n \t\t\t struct tree *tree,\n \t\t\t show_object_fn show,\n+\t\t\t filter_blob_fn filter_blob,\n \t\t\t struct strbuf *base,\n \t\t\t const char *name,\n \t\t\t void *cb_data)\n@@ -111,7 +130,7 @@ static void process_tree(struct rev_info *revs,\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tprocess_tree(revs,\n \t\t\t\t lookup_tree(entry.oid->hash),\n-\t\t\t\t show, base, entry.path,\n+\t\t\t\t show, filter_blob, base, entry.path,\n \t\t\t\t cb_data);\n \t\telse if (S_ISGITLINK(entry.mode))\n \t\t\tprocess_gitlink(revs, entry.oid->hash,\n@@ -120,7 +139,7 @@ static void process_tree(struct rev_info *revs,\n \t\telse\n \t\t\tprocess_blob(revs,\n \t\t\t\t lookup_blob(entry.oid->hash),\n-\t\t\t\t show, base, entry.path,\n+\t\t\t\t show, filter_blob, base, entry.path,\n \t\t\t\t cb_data);\n \t}\n \tstrbuf_setlen(base, baselen);\n@@ -188,6 +207,16 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t\t show_object_fn show_object,\n \t\t\t void *data)\n {\n+\ttraverse_commit_list_filtered(revs, show_commit, show_object, NULL, data);\n+}\n+\n+void traverse_commit_list_filtered(\n+\tstruct rev_info *revs,\n+\tshow_commit_fn show_commit,\n+\tshow_object_fn show_object,\n+\tfilter_blob_fn filter_blob,\n+\tvoid *data)\n+{\n \tint i;\n \tstruct commit *commit;\n \tstruct strbuf base;\n@@ -218,11 +247,13 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t\tpath = \"\";\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tprocess_tree(revs, (struct tree *)obj, show_object,\n+\t\t\t\t filter_blob,\n \t\t\t\t &base, path, data);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n \t\t\tprocess_blob(revs, (struct blob *)obj, show_object,\n+\t\t\t\t filter_blob,\n \t\t\t\t &base, path, data);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/list-objects.h b/list-objects.h\nindex 0cebf85..7f823aa 100644\n--- a/list-objects.h\n+++ b/list-objects.h\n@@ -3,7 +3,15 @@\n \n typedef void (*show_commit_fn)(struct commit *, void *);\n typedef void (*show_object_fn)(struct object *, const char *, void *);\n+typedef int (*filter_blob_fn)(struct object *, const char *, const char *, void *);\n+\n void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);\n+void traverse_commit_list_filtered(\n+ struct rev_info *revs,\n+ show_commit_fn show_commit,\n+ show_object_fn show_object,\n+ filter_blob_fn filter_blob,\n+ void *data);\n \n typedef void (*show_edge_fn)(struct commit *);\n void mark_edges_uninteresting(struct rev_info *, show_edge_fn);\n-- \n2.9.3\n\n"},{"id":"323002","messageId":"20170622144526.3f5813f5@twelve2.svl.corp.google.com","threadId":"46235","inReplyTo":"20170622203615.34135-2-git@jeffhostetler.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-22T21:45:26Z","receivedAt":"2017-06-22T21:45:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 22 Jun 2017 20:36:13 +0000\nJeff Hostetler wrote:\n\n> From: Jeff Hostetler \n> \n> In preparation for partial/sparse clone/fetch where the\n> server is allowed to omit large/all blobs from the packfile,\n> teach traverse_commit_list() to take a blob filter-proc that\n> controls when blobs are shown and marked as SEEN.\n> \n> Normally, traverse_commit_list() always marks visited blobs\n> as SEEN, so that the show_object() callback will never see\n> duplicates. Since a single blob OID may be referenced by\n> multiple pathnames, we may not be able to decide if a blob\n> should be excluded until later pathnames have been traversed.\n> With the filter-proc, the automatic deduping is disabled.\n\nComparing this approach (this patch and the next one) against mine [1],\nI see that this has the advantage of (in pack-objects) avoiding the\ninvocation of add_preferred_base_object() on blobs that are filtered\nout, avoiding polluting the \"to_pack\" data structure with information\nthat we do not need.\n\nBut it does add an extra place where blobs are filtered out (besides\nadd_object_entry()), which makes it harder for the reader to keep track\nof what's going on. I took a brief look to see if filtering could be\neliminated from add_object_entry(), but that function is called from\nmany places, so I couldn't tell.\n\nAnyway, I think both approaches will work (this patch's and mine [1]).\n\n[1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n\n[snip]\n\n> static void process_blob(struct rev_info *revs,\n> \t\t\t struct blob *blob,\n> \t\t\t show_object_fn show,\n> +\t\t\t filter_blob_fn filter_blob,\n> \t\t\t struct strbuf *path,\n> \t\t\t const char *name,\n> \t\t\t void *cb_data)\n> @@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,\n> \t\tdie(\"bad blob object\");\n> \tif (obj->flags & (UNINTERESTING | SEEN))\n> \t\treturn;\n> -\tobj->flags |= SEEN;\n> \n> \tpathlen = path->len;\n> \tstrbuf_addstr(path, name);\n> -\tshow(obj, path->buf, cb_data);\n> +\tif (!filter_blob) {\n> +\t\t/*\n> +\t\t * Normal processing is to imediately dedup blobs\n> +\t\t * during commit traversal, regardless of how many\n> +\t\t * times it appears in a single or multiple commits,\n> +\t\t * so we always set SEEN.\n> +\t\t */\n> +\t\tobj->flags |= SEEN;\n> +\t\tshow(obj, path->buf, cb_data);\n> +\t} else {\n> +\t\t/*\n> +\t\t * Use the filter-proc to decide whether to show\n> +\t\t * the blob. We only set SEEN if requested. For\n> +\t\t * example, this could be used to omit a specific\n> +\t\t * blob until it appears with a \".git*\" entryname.\n> +\t\t */\n> +\t\tif (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))\n> +\t\t\tobj->flags |= SEEN;\n> +\t}\n> \tstrbuf_setlen(path, pathlen);\n> }\n\nAfter looking at this code again, I wonder if we should explicitly\ndocument that SEEN will be set on the object before show(), and that\nshow() is allowed to unset SEEN if it wants traversal to process that\nobject again. That seems to accomplish what we want to accomplish\nwithout this expansion of the API.\n\n(This does mean that the next patch will need to call strrchr() itself,\nsince show() does not provide the basename of the file name.)\n\nHaving said that, if we keep with the current approach, I think there\nshould be a show() call after the invocation to filter_blob(). That is\nmuch less surprising to me, and also allows us to remove some\nshow_object() invocations in the next patch.\n\n> +void traverse_commit_list_filtered(\n> +\tstruct rev_info *revs,\n> +\tshow_commit_fn show_commit,\n> +\tshow_object_fn show_object,\n> +\tfilter_blob_fn filter_blob,\n> +\tvoid *data)\n> +{\n> \tint i;\n> \tstruct commit *commit;\n> \tstruct strbuf base;\n\nGit style is to indent to the location after the first \"(\", I believe.\nLikewise in the header file below.\n\n> typedef void (*show_commit_fn)(struct commit *, void *);\n> typedef void (*show_object_fn)(struct object *, const char *, void *);\n> +typedef int (*filter_blob_fn)(struct object *, const char *, const char *, void *);\n\nCan this have parameter names added (especially both the const char *\nones, otherwise indistinguishable) and, if necessary, documented?\n"},{"id":"323004","messageId":"20170622145440.7609fa8b@twelve2.svl.corp.google.com","threadId":"46235","inReplyTo":"20170622203615.34135-3-git@jeffhostetler.com","subject":"Re: [PATCH 2/3] pack-objects: WIP add max-blob-size filtering","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-22T21:54:40Z","receivedAt":"2017-06-22T21:54:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 22 Jun 2017 20:36:14 +0000\nJeff Hostetler wrote:\n\n> +static signed long max_blob_size = -1;\n\nFYI Junio suggested \"blob-max-bytes\" when he looked at my patch [1].\n\n[1] https://public-inbox.org/git/xmqqmv9ryoym.fsf@gitster.mtv.corp.google.com/\n\n[snip]\n\n> +/*\n> + * Filter blobs by pathname or size.\n> + * Return 1 to mark the blob SEEN so that it will not be reported again.\n> + * Return 0 to allow it to be presented again.\n> + */\n> +static int filter_blob(\n> +\tstruct object *obj,\n> +\tconst char *pathname,\n> +\tconst char *entryname,\n> +\tvoid *data)\n> +{\n> +\tassert(obj->type == OBJ_BLOB);\n> +\tassert((obj->flags & SEEN) == 0);\n> +\tassert((obj->flags & OBJECT_ADDED) == 0);\n> +\tassert(max_blob_size >= 0);\n> +\n> +\t/*\n> +\t * Always include blobs for special files of the form \".git*\".\n> +\t */\n> +\tif ((strncmp(entryname, \".git\", 4) == 0) && entryname[4]) {\n> +\t\tif (obj->flags & BLOB_OMITTED) {\n> +\t\t\t/*\n> +\t\t\t * TODO\n> +\t\t\t * TODO Remove this blob from the omitted blob list.\n> +\t\t\t * TODO\n> +\t\t\t */\n> +\t\t\tobj->flags &= ~BLOB_OMITTED;\n> +\t\t}\n> +\t\tshow_object(obj, pathname, data);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\t/*\n> +\t * We already know the blob is too big because it was previously\n> +\t * omitted. We still don't want it yet. DO NOT mark it SEEN\n> +\t * in case it is associated with a \".git*\" path in another tree\n> +\t * or commit.\n> +\t */\n> +\tif (obj->flags & BLOB_OMITTED)\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * We only want blobs that are LESS THAN the maximum.\n> +\t * This allows zero to mean NO BLOBS.\n\nThat is not what maximum means...\n\nThis is the reason why I originally called it \"limit\", but after some\nreflection, I decided that it is no big deal to always send zero-sized\nblobs. The client must be able to handle the presence of blobs anyway\n(because it will receive the \".git\" ones), and excluding all blobs\nregardless of size does not remove the necessity of obtaining their\nsizes, since we need those sizes to put in the omitted blob list.\n\n> +\t */\n> +\tif (max_blob_size > 0) {\n> +\t\tunsigned long s;\n> +\t\tenum object_type t = sha1_object_info(obj->oid.hash, &s);\n> +\t\tassert(t == OBJ_BLOB);\n> +\t\tif (s < max_blob_size) {\n> +\t\t\tshow_object(obj, pathname, data);\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * TODO\n> +\t * TODO (Provisionally) add this blob to the omitted blob list.\n> +\t * TODO\n\nAs for the omitted blob list itself, you can see from my patch [2] that I\nused a hashmap, but the decorate.h functionality might work too (I\nhaven't looked into that in detail though).\n\n[2] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n"},{"id":"323008","messageId":"20170622151028.07827b50@twelve2.svl.corp.google.com","threadId":"46235","inReplyTo":"20170622144526.3f5813f5@twelve2.svl.corp.google.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-06-22T22:10:28Z","receivedAt":"2017-06-22T22:10:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 22 Jun 2017 14:45:26 -0700\nJonathan Tan wrote:\n\n> On Thu, 22 Jun 2017 20:36:13 +0000\n> Jeff Hostetler wrote:\n> \n> > From: Jeff Hostetler \n> > \n> > In preparation for partial/sparse clone/fetch where the\n> > server is allowed to omit large/all blobs from the packfile,\n> > teach traverse_commit_list() to take a blob filter-proc that\n> > controls when blobs are shown and marked as SEEN.\n> > \n> > Normally, traverse_commit_list() always marks visited blobs\n> > as SEEN, so that the show_object() callback will never see\n> > duplicates. Since a single blob OID may be referenced by\n> > multiple pathnames, we may not be able to decide if a blob\n> > should be excluded until later pathnames have been traversed.\n> > With the filter-proc, the automatic deduping is disabled.\n> \n> Comparing this approach (this patch and the next one) against mine [1],\n> I see that this has the advantage of (in pack-objects) avoiding the\n> invocation of add_preferred_base_object() on blobs that are filtered\n> out, avoiding polluting the \"to_pack\" data structure with information\n> that we do not need.\n> \n> But it does add an extra place where blobs are filtered out (besides\n> add_object_entry()), which makes it harder for the reader to keep track\n> of what's going on. I took a brief look to see if filtering could be\n> eliminated from add_object_entry(), but that function is called from\n> many places, so I couldn't tell.\n> \n> Anyway, I think both approaches will work (this patch's and mine [1]).\n> \n> [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n\nAlso it should be mentioned somewhere that this does not cover the\nbitmap case - a similar discussion should be included in one of the\npatches, like I did in [1].\n\nAnd looking back at my original cover letter [2], I wrote:\n\n> This is similar to [1] except that this\n[...]\n> is slightly more comprehensive in\n> that the read_object_list_from_stdin() codepath is also covered in\n> addition to the get_object_list() codepath. (Although, to be clear,\n> upload-pack always passes \"--revs\" and thus only needs the\n> get_object_list() codepath).\n\n(The [1] in the quote above refers to one of Jeff Hostetler's patches,\n[QUOTE 1].)\n\nThe same issue applies to this patch (since\nread_object_list_from_stdin() invokes add_object_entry() directly\nwithout going through the traversal mechanism) and probably warrants at\nleast some description in one of the commit messages.\n\n[1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n[2] https://public-inbox.org/git/cover.1496361873.git.jonathantanmy@google.com/\n\n[QUOTE 1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffhost@microsoft.com/\n"},{"id":"323009","messageId":"xmqqvannhdpv.fsf@gitster.mtv.corp.google.com","threadId":"46235","inReplyTo":"20170622145440.7609fa8b@twelve2.svl.corp.google.com","subject":"Re: [PATCH 2/3] pack-objects: WIP add max-blob-size filtering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T22:14:36Z","receivedAt":"2017-06-22T22:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> On Thu, 22 Jun 2017 20:36:14 +0000\n> Jeff Hostetler wrote:\n>\n>> +static signed long max_blob_size = -1;\n>\n> FYI Junio suggested \"blob-max-bytes\" when he looked at my patch [1].\n>\n> [1] https://public-inbox.org/git/xmqqmv9ryoym.fsf@gitster.mtv.corp.google.com/\n\nTo give credit to where credit is due, I learned this from akpm.\n\nWhen you are tempted to say \"size\", \"length\", \"weight\", etc., if you\nthink of a way to phrase it with a more concrete unit, you'd end up\nwith a much less ambiguous name. People can imagine max_blob_size\nis counted in kilobytes, or megabytes, or whatever. There is no\nroom in max_blob_bytes to be interpreted in multiple ways.\n"},{"id":"323116","messageId":"72257781-5a2c-cdd6-3fb3-7c5e6d883653@jeffhostetler.com","threadId":"46235","inReplyTo":"20170622151028.07827b50@twelve2.svl.corp.google.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-23T17:16:30Z","receivedAt":"2017-06-23T17:16:38Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 6/22/2017 6:10 PM, Jonathan Tan wrote:\n> On Thu, 22 Jun 2017 14:45:26 -0700\n> Jonathan Tan wrote:\n> \n>> On Thu, 22 Jun 2017 20:36:13 +0000\n>> Jeff Hostetler wrote:\n>>\n>>> From: Jeff Hostetler \n>>>\n>>> In preparation for partial/sparse clone/fetch where the\n>>> server is allowed to omit large/all blobs from the packfile,\n>>> teach traverse_commit_list() to take a blob filter-proc that\n>>> controls when blobs are shown and marked as SEEN.\n>>>\n>>> Normally, traverse_commit_list() always marks visited blobs\n>>> as SEEN, so that the show_object() callback will never see\n>>> duplicates. Since a single blob OID may be referenced by\n>>> multiple pathnames, we may not be able to decide if a blob\n>>> should be excluded until later pathnames have been traversed.\n>>> With the filter-proc, the automatic deduping is disabled.\n>>\n>> Comparing this approach (this patch and the next one) against mine [1],\n>> I see that this has the advantage of (in pack-objects) avoiding the\n>> invocation of add_preferred_base_object() on blobs that are filtered\n>> out, avoiding polluting the \"to_pack\" data structure with information\n>> that we do not need.\n>>\n>> But it does add an extra place where blobs are filtered out (besides\n>> add_object_entry()), which makes it harder for the reader to keep track\n>> of what's going on. I took a brief look to see if filtering could be\n>> eliminated from add_object_entry(), but that function is called from\n>> many places, so I couldn't tell.\n>>\n>> Anyway, I think both approaches will work (this patch's and mine [1]).\n>>\n>> [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n> \n> Also it should be mentioned somewhere that this does not cover the\n> bitmap case - a similar discussion should be included in one of the\n> patches, like I did in [1].\n> \n> And looking back at my original cover letter [2], I wrote:\n> \n>> This is similar to [1] except that this\n> [...]\n>> is slightly more comprehensive in\n>> that the read_object_list_from_stdin() codepath is also covered in\n>> addition to the get_object_list() codepath. (Although, to be clear,\n>> upload-pack always passes \"--revs\" and thus only needs the\n>> get_object_list() codepath).\n> \n> (The [1] in the quote above refers to one of Jeff Hostetler's patches,\n> [QUOTE 1].)\n> \n> The same issue applies to this patch (since\n> read_object_list_from_stdin() invokes add_object_entry() directly\n> without going through the traversal mechanism) and probably warrants at\n> least some description in one of the commit messages.\n> \n> [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/\n> [2] https://public-inbox.org/git/cover.1496361873.git.jonathantanmy@google.com/\n> \n> [QUOTE 1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffhost@microsoft.com/\n> \n\n\nThanks for the quick feedback. I'll dig into each of these comments\nas I work on my next draft.\n\nJeff\n"},{"id":"323429","messageId":"xmqqy3scw06y.fsf@gitster.mtv.corp.google.com","threadId":"46235","inReplyTo":"20170622203615.34135-2-git@jeffhostetler.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T16:23:33Z","receivedAt":"2017-06-28T16:23:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> diff --git a/list-objects.c b/list-objects.c\n> index f3ca6aa..c9ca81c 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,\n> \t\tdie(\"bad blob object\");\n> \tif (obj->flags & (UNINTERESTING | SEEN))\n> \t\treturn;\n> -\tobj->flags |= SEEN;\n> \n> \tpathlen = path->len;\n> \tstrbuf_addstr(path, name);\n> -\tshow(obj, path->buf, cb_data);\n> +\tif (!filter_blob) {\n> +\t\t/*\n> +\t\t * Normal processing is to imediately dedup blobs\n> +\t\t * during commit traversal, regardless of how many\n> +\t\t * times it appears in a single or multiple commits,\n> +\t\t * so we always set SEEN.\n> +\t\t */\n> +\t\tobj->flags |= SEEN;\n> +\t\tshow(obj, path->buf, cb_data);\n> +\t} else {\n> +\t\t/*\n> +\t\t * Use the filter-proc to decide whether to show\n> +\t\t * the blob. We only set SEEN if requested. For\n> +\t\t * example, this could be used to omit a specific\n> +\t\t * blob until it appears with a \".git*\" entryname.\n> +\t\t */\n> +\t\tif (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))\n> +\t\t\tobj->flags |= SEEN;\n> +\t}\n\nThis somehow looks a bit surprising organization and division of\nresponsibility. I would have expected\n\n\tif (!filter_blob || \n\t filter_blob(obj, path->buf, &path->buf[pathlen], cb_data) {\n\t\tobj->flags |= SEEN;\n\t\tshow(obj, path->buf, cb_data);\n\t}\n\ni.e. making the filter function responsible for only making a\ndecision to include or exclude, not giving it a chance to decide to\n\"show\" anything different.\n\n> @@ -67,6 +85,7 @@ static void process_gitlink(struct rev_info *revs,\n> static void process_tree(struct rev_info *revs,\n> \t\t\t struct tree *tree,\n> \t\t\t show_object_fn show,\n> +\t\t\t filter_blob_fn filter_blob,\n> \t\t\t struct strbuf *base,\n> \t\t\t const char *name,\n> \t\t\t void *cb_data)\n> @@ -111,7 +130,7 @@ static void process_tree(struct rev_info *revs,\n> \t\tif (S_ISDIR(entry.mode))\n> \t\t\tprocess_tree(revs,\n> \t\t\t\t lookup_tree(entry.oid->hash),\n> -\t\t\t\t show, base, entry.path,\n> +\t\t\t\t show, filter_blob, base, entry.path,\n> \t\t\t\t cb_data);\n> \t\telse if (S_ISGITLINK(entry.mode))\n> \t\t\tprocess_gitlink(revs, entry.oid->hash,\n\nI wonder if we'll need filter_tree_fn in the future in this\ncodepath. When somebody wants to do a \"narrow fetch/clone\", would\nthe approach taken by this series, i.e. decide not to show certain\nobjects during the \"rev-list --objects\" traversal, a good precedent\nto follow? Would this approach be a good foundation to build on\nsuch a future?\n\nThanks.\n"},{"id":"323432","messageId":"e2216ab8-5af7-4edd-16aa-f84a45e0cbd7@jeffhostetler.com","threadId":"46235","inReplyTo":"xmqqy3scw06y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-06-28T17:13:22Z","receivedAt":"2017-06-28T17:13:28Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 6/28/2017 12:23 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> diff --git a/list-objects.c b/list-objects.c\n>> index f3ca6aa..c9ca81c 100644\n>> --- a/list-objects.c\n>> +++ b/list-objects.c\n>> @@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,\n>> \t\tdie(\"bad blob object\");\n>> \tif (obj->flags & (UNINTERESTING | SEEN))\n>> \t\treturn;\n>> -\tobj->flags |= SEEN;\n>> \n>> \tpathlen = path->len;\n>> \tstrbuf_addstr(path, name);\n>> -\tshow(obj, path->buf, cb_data);\n>> +\tif (!filter_blob) {\n>> +\t\t/*\n>> +\t\t * Normal processing is to imediately dedup blobs\n>> +\t\t * during commit traversal, regardless of how many\n>> +\t\t * times it appears in a single or multiple commits,\n>> +\t\t * so we always set SEEN.\n>> +\t\t */\n>> +\t\tobj->flags |= SEEN;\n>> +\t\tshow(obj, path->buf, cb_data);\n>> +\t} else {\n>> +\t\t/*\n>> +\t\t * Use the filter-proc to decide whether to show\n>> +\t\t * the blob. We only set SEEN if requested. For\n>> +\t\t * example, this could be used to omit a specific\n>> +\t\t * blob until it appears with a \".git*\" entryname.\n>> +\t\t */\n>> +\t\tif (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))\n>> +\t\t\tobj->flags |= SEEN;\n>> +\t}\n> \n> This somehow looks a bit surprising organization and division of\n> responsibility. I would have expected\n> \n> \tif (!filter_blob ||\n> \t filter_blob(obj, path->buf, &path->buf[pathlen], cb_data) {\n> \t\tobj->flags |= SEEN;\n> \t\tshow(obj, path->buf, cb_data);\n> \t}\n> \n> i.e. making the filter function responsible for only making a\n> decision to include or exclude, not giving it a chance to decide to\n> \"show\" anything different.\n\nYes, my logic was a little confusing there. Jonathan Tan said\nsomething similar the other day. I have a new version that I'm\nworking on now that looks like this:\n\n\tlist_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW;\n\t...\n\tif (filter)\n\t\tr = filter(obj, path->buf, ...\n\tif (r & LOFR_MARK_SEEN)\n\t\tobj->flags |= SEEN;\n\tif (r & LOFR_SHOW)\n\t\tshow(obj, path->buf, cb_data);\n\nI'm generalizing it a little to let the filter return 2 flags:\n() SEEN to indicate that the filter doesn't want to see it again\n() SHOW to include the object in the result.\nThese let filters do \"hard\" and \"provisional\" omits. (This will\nmake more sense later when I get my patch cleaned up.)\n\n\n>> @@ -67,6 +85,7 @@ static void process_gitlink(struct rev_info *revs,\n>> static void process_tree(struct rev_info *revs,\n>> \t\t\t struct tree *tree,\n>> \t\t\t show_object_fn show,\n>> +\t\t\t filter_blob_fn filter_blob,\n>> \t\t\t struct strbuf *base,\n>> \t\t\t const char *name,\n>> \t\t\t void *cb_data)\n>> @@ -111,7 +130,7 @@ static void process_tree(struct rev_info *revs,\n>> \t\tif (S_ISDIR(entry.mode))\n>> \t\t\tprocess_tree(revs,\n>> \t\t\t\t lookup_tree(entry.oid->hash),\n>> -\t\t\t\t show, base, entry.path,\n>> +\t\t\t\t show, filter_blob, base, entry.path,\n>> \t\t\t\t cb_data);\n>> \t\telse if (S_ISGITLINK(entry.mode))\n>> \t\t\tprocess_gitlink(revs, entry.oid->hash,\n> \n> I wonder if we'll need filter_tree_fn in the future in this\n> codepath. When somebody wants to do a \"narrow fetch/clone\", would\n> the approach taken by this series, i.e. decide not to show certain\n> objects during the \"rev-list --objects\" traversal, a good precedent\n> to follow? Would this approach be a good foundation to build on\n> such a future?\n\nYes, I'm including similar logic inside process_tree() to allow that\nand let the filter know about entering and leaving each tree. So we\nonly need one filter-proc to handle a particular strategy and it will\nhandle both tree and blob objects.\n\nI want to be able to use this mechanism to do narrow clone/fetch\nusing such a filter-proc and a sparse-checkout-like spec.\n\nThanks,\nJeff\n\n\n"},{"id":"323436","messageId":"xmqqa84svvyy.fsf@gitster.mtv.corp.google.com","threadId":"46235","inReplyTo":"e2216ab8-5af7-4edd-16aa-f84a45e0cbd7@jeffhostetler.com","subject":"Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T17:54:45Z","receivedAt":"2017-06-28T17:55:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> Yes, my logic was a little confusing there. Jonathan Tan said\n> something similar the other day. I have a new version that I'm\n> working on now that looks like this:\n>\n> \tlist_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW;\n> \t...\n> \tif (filter)\n> \t\tr = filter(obj, path->buf, ...\n> \tif (r & LOFR_MARK_SEEN)\n> \t\tobj->flags |= SEEN;\n> \tif (r & LOFR_SHOW)\n> \t\tshow(obj, path->buf, cb_data);\n>\n> I'm generalizing it a little to let the filter return 2 flags:\n> () SEEN to indicate that the filter doesn't want to see it again\n> () SHOW to include the object in the result.\n> These let filters do \"hard\" and \"provisional\" omits. (This will\n> make more sense later when I get my patch cleaned up.)\n\nIt is not immediately obvious to me, especially without seeing the\nactual patch, why MARK_SEEN is needed. Especially given that I\nthink a call to show() must set obj->flags |= SEEN anyway to avoid\nduplicate output, with or without the objects-filter mechanism.\n\nBut that question can and should wait.\n\n> Yes, I'm including similar logic inside process_tree() to allow that\n> and let the filter know about entering and leaving each tree. So we\n> only need one filter-proc to handle a particular strategy and it will\n> handle both tree and blob objects.\n>\n> I want to be able to use this mechanism to do narrow clone/fetch\n> using such a filter-proc and a sparse-checkout-like spec.\n\nGood to know ;-).\n"}]}