{"thread":{"id":"50867","subject":"[PATCH] clone: do faster object check for partial clones","startedAt":"2019-04-03T17:27:25Z","lastAt":"2019-04-22T21:31:17Z","messageCount":27,"participants":["Josh Steadmon","Jonathan Tan","Jeff King","Christian Couder","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"373062","messageId":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","threadId":"50867","inReplyTo":null,"subject":"[PATCH] clone: do faster object check for partial clones","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-03T17:27:21Z","receivedAt":"2019-04-03T17:27:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"For partial clones, doing a full connectivity check is wasteful; we skip\npromisor objects (which, for a partial clone, is all known objects), and\nexcluding them all from the connectivity check can take a significant\namount of time on large repos.\n\nAt most, we want to make sure that we get the objects referred to by any\nwanted refs. For partial clones, just check that these objects were\ntransferred.\n\nSigned-off-by: Josh Steadmon \n---\n builtin/clone.c | 6 ++++--\n connected.c | 15 +++++++++++++++\n connected.h | 8 ++++++++\n 3 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..fdbbd8942a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -657,7 +657,8 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t const char *branch_top,\n \t\t\t const char *msg,\n \t\t\t struct transport *transport,\n-\t\t\t int check_connectivity)\n+\t\t\t int check_connectivity,\n+\t\t\t int check_refs_only)\n {\n \tconst struct ref *rm = mapped_refs;\n \n@@ -666,6 +667,7 @@ static void update_remote_refs(const struct ref *refs,\n \n \t\topt.transport = transport;\n \t\topt.progress = transport->progress;\n+\t\topt.check_refs_only = !!check_refs_only;\n \n \t\tif (check_connected(iterate_ref_map, &rm, &opt))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n@@ -1224,7 +1226,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n-\t\t\t !is_local);\n+\t\t\t !is_local, filter_options.choice);\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \ndiff --git a/connected.c b/connected.c\nindex 1bba888eff..c297cdc5ab 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"object-store.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n #include \"connected.h\"\n@@ -49,6 +50,20 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tstrbuf_release(&idx_file);\n \t}\n \n+\tif (opt->check_refs_only) {\n+\t\t/*\n+\t\t * For partial clones, we don't want to walk the full commit\n+\t\t * graph because we're skipping promisor objects anyway. We\n+\t\t * should just check that objects referenced by wanted refs were\n+\t\t * transferred.\n+\t\t */\n+\t\tdo {\n+\t\t\tif (!repo_has_object_file(the_repository, &oid))\n+\t\t\t\treturn 1;\n+\t\t} while (!fn(cb_data, &oid));\n+\t\treturn 0;\n+\t}\n+\n \tif (opt->shallow_file) {\n \t\targv_array_push(&rev_list.args, \"--shallow-file\");\n \t\targv_array_push(&rev_list.args, opt->shallow_file);\ndiff --git a/connected.h b/connected.h\nindex 8d5a6b3ad6..bb4afcb301 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -46,6 +46,14 @@ struct check_connected_options {\n \t * during a fetch.\n \t */\n \tunsigned is_deepening_fetch : 1;\n+\n+\t/*\n+\t * If non-zero, only check the top-level objects referenced by the\n+\t * wanted refs (passed in as cb_data). This is useful for partial\n+\t * clones, where this can be much faster than excluding all promisor\n+\t * objects prior to walking the commit graph.\n+\t */\n+\tunsigned check_refs_only : 1;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"373066","messageId":"20190403185810.8787-1-jonathantanmy@google.com","threadId":"50867","inReplyTo":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","subject":"Re: [PATCH] clone: do faster object check for partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-03T18:58:10Z","receivedAt":"2019-04-03T18:58:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> For partial clones, doing a full connectivity check is wasteful; we skip\n> promisor objects (which, for a partial clone, is all known objects), and\n> excluding them all from the connectivity check can take a significant\n> amount of time on large repos.\n\nInstead of \"excluding them all\", I would word this as \"enumerating them\nall so that we can exclude them\" - the enumerating is the slow part, not\nthe excluding (which actually makes things faster).\n\n> +\tif (opt->check_refs_only) {\n> +\t\t/*\n> +\t\t * For partial clones, we don't want to walk the full commit\n> +\t\t * graph because we're skipping promisor objects anyway. We\n> +\t\t * should just check that objects referenced by wanted refs were\n> +\t\t * transferred.\n\nThe enumeration of promisor objects to be excluded is done through\nfor_each_packed_object() (see is_promisor_object()), not through walking\nthe commit graph. Maybe reword this comment to be similar to what I've\nsuggested for the commit message.\n\n> @@ -46,6 +46,14 @@ struct check_connected_options {\n> \t * during a fetch.\n> \t */\n> \tunsigned is_deepening_fetch : 1;\n> +\n> +\t/*\n> +\t * If non-zero, only check the top-level objects referenced by the\n> +\t * wanted refs (passed in as cb_data). This is useful for partial\n> +\t * clones, where this can be much faster than excluding all promisor\n> +\t * objects prior to walking the commit graph.\n> +\t */\n> +\tunsigned check_refs_only : 1;\n> };\n\nSame enumerating vs excluding comment as before.\n\nAside from that: thinking from scratch, we want something that tells\ncheck_connected() to avoid anything that enumerates the list of promised\nobjects, since the objects that we're checking are all promisor objects,\nand thus any outgoing links are automatically promised. I would include\nsome of this explanation in the comment, but in the interest of trying\nto avoid a bikeshedding discussion, I don't consider this necessary.\n"},{"id":"373067","messageId":"20190403194150.GA27199@sigill.intra.peff.net","threadId":"50867","inReplyTo":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","subject":"Re: [PATCH] clone: do faster object check for partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-03T19:41:50Z","receivedAt":"2019-04-03T19:41:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2019 at 10:27:21AM -0700, Josh Steadmon wrote:\n\n> For partial clones, doing a full connectivity check is wasteful; we skip\n> promisor objects (which, for a partial clone, is all known objects), and\n> excluding them all from the connectivity check can take a significant\n> amount of time on large repos.\n> \n> At most, we want to make sure that we get the objects referred to by any\n> wanted refs. For partial clones, just check that these objects were\n> transferred.\n\nThis isn't strictly true, since we could get objects from elsewhere via\n--shared or --reference. Those might not be promisor objects.\n\nShouldn't we be able to stop a traversal as soon as we see that an\nobject is in a promisor pack?\n\nI.e., here:\n\n> +\tif (opt->check_refs_only) {\n> +\t\t/*\n> +\t\t * For partial clones, we don't want to walk the full commit\n> +\t\t * graph because we're skipping promisor objects anyway. We\n> +\t\t * should just check that objects referenced by wanted refs were\n> +\t\t * transferred.\n> +\t\t */\n> +\t\tdo {\n> +\t\t\tif (!repo_has_object_file(the_repository, &oid))\n> +\t\t\t\treturn 1;\n> +\t\t} while (!fn(cb_data, &oid));\n> +\t\treturn 0;\n> +\t}\n\nfor each object where you ask \"do we have this?\" we could, for the same\ncost, ask \"do we have this in a promisor pack?\". And the answer would be\nyes for each in a partial clone.\n\nBut that would also cleanly handle --shared, etc, that I mentioned. And\nmore importantly, it would _also_ work on fetches. If I fetch from you\nand get a promisor pack, then there is no point in me enumerating every\ntree you sent. As long as you gave me a tip tree, then you have promised\nthat you'd give me all the others if I ask.\n\nSo it seems like this should be a feature of the child rev-list, to stop\nwalking the graph at any object that is in a promisor pack.\n\n-Peff\n"},{"id":"373069","messageId":"20190403205748.107979-1-jonathantanmy@google.com","threadId":"50867","inReplyTo":"20190403194150.GA27199@sigill.intra.peff.net","subject":"Re: [PATCH] clone: do faster object check for partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-03T20:57:48Z","receivedAt":"2019-04-03T20:57:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Apr 03, 2019 at 10:27:21AM -0700, Josh Steadmon wrote:\n> \n> > For partial clones, doing a full connectivity check is wasteful; we skip\n> > promisor objects (which, for a partial clone, is all known objects), and\n> > excluding them all from the connectivity check can take a significant\n> > amount of time on large repos.\n> > \n> > At most, we want to make sure that we get the objects referred to by any\n> > wanted refs. For partial clones, just check that these objects were\n> > transferred.\n> \n> This isn't strictly true, since we could get objects from elsewhere via\n> --shared or --reference. Those might not be promisor objects.\n\nI don't think local clones (which --shared or --reference implies) can\nbe partial, but the bigger point is below.\n\n> Shouldn't we be able to stop a traversal as soon as we see that an\n> object is in a promisor pack?\n> \n> I.e., here:\n> \n> > +\tif (opt->check_refs_only) {\n> > +\t\t/*\n> > +\t\t * For partial clones, we don't want to walk the full commit\n> > +\t\t * graph because we're skipping promisor objects anyway. We\n> > +\t\t * should just check that objects referenced by wanted refs were\n> > +\t\t * transferred.\n> > +\t\t */\n> > +\t\tdo {\n> > +\t\t\tif (!repo_has_object_file(the_repository, &oid))\n> > +\t\t\t\treturn 1;\n> > +\t\t} while (!fn(cb_data, &oid));\n> > +\t\treturn 0;\n> > +\t}\n> \n> for each object where you ask \"do we have this?\" we could, for the same\n> cost, ask \"do we have this in a promisor pack?\". And the answer would be\n> yes for each in a partial clone.\n> \n> But that would also cleanly handle --shared, etc, that I mentioned. And\n> more importantly, it would _also_ work on fetches. If I fetch from you\n> and get a promisor pack, then there is no point in me enumerating every\n> tree you sent. As long as you gave me a tip tree, then you have promised\n> that you'd give me all the others if I ask.\n> \n> So it seems like this should be a feature of the child rev-list, to stop\n> walking the graph at any object that is in a promisor pack.\n\nWe currently already do a less optimal version of this - we pass\n--exclude-promisor-objects to rev-list, which indeed stops traversal at\nany promisor objects (whether in a promisor pack or referenced by one).\nAs far as I know, the problem is that to do so, we currently enumerate\nall the objects in all promisor packs, and all objects that those\nobjects reference (which means we inflate them too) - so that we have an\noidset that we can check objects against.\n\nA partial solution is for is_promisor_object() to first check if the\ngiven object is in a promisor pack, avoiding generating the set of\npromisor objects until necessary. This would work in a blob:none clone\nwith the refs pointing all to commits or all to blobs, but would not\nwork in a tree:none clone (or maybe, in this case, the clone would be\nsmall enough that performance is not a concern, hmm).\n\nMaybe the ideal solution is for rev-list to check if an object is in a\npromisor pack and if --exclude-promisor-objects is active, we do not\nfollow any outgoing links.\n"},{"id":"373080","messageId":"20190404002101.GF60888@google.com","threadId":"50867","inReplyTo":"20190403205748.107979-1-jonathantanmy@google.com","subject":"Re: [PATCH] clone: do faster object check for partial clones","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-04T00:21:01Z","receivedAt":"2019-04-04T00:21:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.03 13:57, Jonathan Tan wrote:\n> > On Wed, Apr 03, 2019 at 10:27:21AM -0700, Josh Steadmon wrote:\n> > \n> > > For partial clones, doing a full connectivity check is wasteful; we skip\n> > > promisor objects (which, for a partial clone, is all known objects), and\n> > > excluding them all from the connectivity check can take a significant\n> > > amount of time on large repos.\n> > > \n> > > At most, we want to make sure that we get the objects referred to by any\n> > > wanted refs. For partial clones, just check that these objects were\n> > > transferred.\n> > \n> > This isn't strictly true, since we could get objects from elsewhere via\n> > --shared or --reference. Those might not be promisor objects.\n> \n> I don't think local clones (which --shared or --reference implies) can\n> be partial, but the bigger point is below.\n> \n> > Shouldn't we be able to stop a traversal as soon as we see that an\n> > object is in a promisor pack?\n> > \n> > I.e., here:\n> > \n> > > +\tif (opt->check_refs_only) {\n> > > +\t\t/*\n> > > +\t\t * For partial clones, we don't want to walk the full commit\n> > > +\t\t * graph because we're skipping promisor objects anyway. We\n> > > +\t\t * should just check that objects referenced by wanted refs were\n> > > +\t\t * transferred.\n> > > +\t\t */\n> > > +\t\tdo {\n> > > +\t\t\tif (!repo_has_object_file(the_repository, &oid))\n> > > +\t\t\t\treturn 1;\n> > > +\t\t} while (!fn(cb_data, &oid));\n> > > +\t\treturn 0;\n> > > +\t}\n> > \n> > for each object where you ask \"do we have this?\" we could, for the same\n> > cost, ask \"do we have this in a promisor pack?\". And the answer would be\n> > yes for each in a partial clone.\n> > \n> > But that would also cleanly handle --shared, etc, that I mentioned. And\n> > more importantly, it would _also_ work on fetches. If I fetch from you\n> > and get a promisor pack, then there is no point in me enumerating every\n> > tree you sent. As long as you gave me a tip tree, then you have promised\n> > that you'd give me all the others if I ask.\n> > \n> > So it seems like this should be a feature of the child rev-list, to stop\n> > walking the graph at any object that is in a promisor pack.\n> \n> We currently already do a less optimal version of this - we pass\n> --exclude-promisor-objects to rev-list, which indeed stops traversal at\n> any promisor objects (whether in a promisor pack or referenced by one).\n> As far as I know, the problem is that to do so, we currently enumerate\n> all the objects in all promisor packs, and all objects that those\n> objects reference (which means we inflate them too) - so that we have an\n> oidset that we can check objects against.\n> \n> A partial solution is for is_promisor_object() to first check if the\n> given object is in a promisor pack, avoiding generating the set of\n> promisor objects until necessary. This would work in a blob:none clone\n> with the refs pointing all to commits or all to blobs, but would not\n> work in a tree:none clone (or maybe, in this case, the clone would be\n> small enough that performance is not a concern, hmm).\n> \n> Maybe the ideal solution is for rev-list to check if an object is in a\n> promisor pack and if --exclude-promisor-objects is active, we do not\n> follow any outgoing links.\n\nI am not sure that this actually saves us any work vs. the original\nmethod of marking promisor objects as uninteresting. Marking the objects\nuninteresting involves calling for_each_packed_object(some_callback_fn, ...,\nFOR_EACH_OBJECT_PROMISOR_ONLY). But this is also exactly the setup that\nis_promisor_object() runs the first time it's called. In fact, it looks\nto me like the callback function used by the latter is more expensive\nthan the former. Is there some alternative to is_promisor_object() that\ndoesn't involve this object enumeration?\n\nI've tried implementing this approach anyway (diff against master is\nbelow), but I can't get a version that passes t0410-partial-clone.sh\n(cases 17 and 19). In case 17, the rev-list emits one extra line\ncorresponding to the blob for baz.t. I'm not sure why the original\nimplementation thinks that baz.t is a promisor object but my WIP version\ndoesn't.\n\nI'll keep poking at this tomorrow, I just thought I should comment on\nwhat I've found so far in case anyone can spot where I'm going wrong.\n\n\n\ndiff --git a/list-objects.c b/list-objects.c\nindex dc77361e11..1cb85f1662 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -326,6 +326,12 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,\n \t\tstruct object *obj = pending->item;\n \t\tconst char *name = pending->name;\n \t\tconst char *path = pending->path;\n+\t\ttrace_printf(\"DEBUG: Examining object: %s\\n\", oid_to_hex(&obj->oid));\n+\t\tif (ctx->revs->exclude_promisor_objects &&\n+\t\t is_promisor_object(&obj->oid)) {\n+\t\t\ttrace_printf(\"DEBUG: Skipping object: %s\\n\", oid_to_hex(&obj->oid));\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (obj->flags & (UNINTERESTING | SEEN))\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n@@ -356,6 +362,13 @@ static void do_traverse(struct traversal_context *ctx)\n \tstrbuf_init(&csp, PATH_MAX);\n \n \twhile ((commit = get_revision(ctx->revs)) != NULL) {\n+\t\ttrace_printf(\"DEBUG: Examining commit: %s\\n\", oid_to_hex(&commit->object.oid));\n+\t\tif (ctx->revs->exclude_promisor_objects &&\n+\t\t is_promisor_object(&commit->object.oid)) {\n+\t\t\ttrace_printf(\"DEBUG: Skipping commit: %s\\n\", oid_to_hex(&commit->object.oid));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/*\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..85974e941d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3067,17 +3067,6 @@ void reset_revision_walk(void)\n \tclear_object_flags(SEEN | ADDED | SHOWN);\n }\n \n-static int mark_uninteresting(const struct object_id *oid,\n-\t\t\t struct packed_git *pack,\n-\t\t\t uint32_t pos,\n-\t\t\t void *cb)\n-{\n-\tstruct rev_info *revs = cb;\n-\tstruct object *o = parse_object(revs->repo, oid);\n-\to->flags |= UNINTERESTING | SEEN;\n-\treturn 0;\n-}\n-\n define_commit_slab(indegree_slab, int);\n define_commit_slab(author_date_slab, timestamp_t);\n \n@@ -3316,11 +3305,6 @@ int prepare_revision_walk(struct rev_info *revs)\n \t (revs->limited && limiting_can_increase_treesame(revs)))\n \t\trevs->treesame.name = \"treesame\";\n \n-\tif (revs->exclude_promisor_objects) {\n-\t\tfor_each_packed_object(mark_uninteresting, revs,\n-\t\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n-\t}\n-\n \tif (revs->no_walk != REVISION_WALK_NO_WALK_UNSORTED)\n \t\tcommit_list_sort_by_date(&revs->commits);\n \tif (revs->no_walk)\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex bce02788e6..55ac29f650 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -342,7 +342,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&\n+\tGIT_TRACE=\"$(pwd)/rev_trace\" git -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&\n \t! grep $COMMIT out &&\n \t! grep $TREE out &&\n \t! grep $BLOB out &&\n"},{"id":"373082","messageId":"20190404013343.GA4409@sigill.intra.peff.net","threadId":"50867","inReplyTo":"20190403205748.107979-1-jonathantanmy@google.com","subject":"Re: [PATCH] clone: do faster object check for partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T01:33:44Z","receivedAt":"2019-04-04T01:33:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2019 at 01:57:48PM -0700, Jonathan Tan wrote:\n\n> > This isn't strictly true, since we could get objects from elsewhere via\n> > --shared or --reference. Those might not be promisor objects.\n> \n> I don't think local clones (which --shared or --reference implies) can\n> be partial, but the bigger point is below.\n\nYeah, you're right about --shared. But I don't see any reason a\n--reference clone could not be partial.\n\n> > So it seems like this should be a feature of the child rev-list, to stop\n> > walking the graph at any object that is in a promisor pack.\n> \n> We currently already do a less optimal version of this - we pass\n> --exclude-promisor-objects to rev-list, which indeed stops traversal at\n> any promisor objects (whether in a promisor pack or referenced by one).\n> As far as I know, the problem is that to do so, we currently enumerate\n> all the objects in all promisor packs, and all objects that those\n> objects reference (which means we inflate them too) - so that we have an\n> oidset that we can check objects against.\n> \n> A partial solution is for is_promisor_object() to first check if the\n> given object is in a promisor pack, avoiding generating the set of\n> promisor objects until necessary. This would work in a blob:none clone\n> with the refs pointing all to commits or all to blobs, but would not\n> work in a tree:none clone (or maybe, in this case, the clone would be\n> small enough that performance is not a concern, hmm).\n> \n> Maybe the ideal solution is for rev-list to check if an object is in a\n> promisor pack and if --exclude-promisor-objects is active, we do not\n> follow any outgoing links.\n\nI was thinking you could actually check it before even loading the\nobject. I.e., something like:\n\n struct object_info oi = OBJECT_INFO_INIT;\n\n if (!oid_object_info_extended(oid, &oi, 0) &&\n oi->whence = OI_PACKED &&\n oi->u.packed.pack->pack_promisor)) {\n /*\n\t * no point in even looking at its links,\n\t * since the promisor pack claims that we\n\t * can get anything we need later from the\n\t * remote\n\t */\n return 0; /* or whatever, depending where this goes ;) */\n } else {\n /* not a promisor object, load it and traverse as normal */\n }\n\nThat doesn't quite work as an implementation of is_promisor_object(),\nbecause it wouldn't know about items that we _don't_ have that are\npromised. But I think it could work as part of the traversal in\nlist-objects.c, since we'd just be walking down a traversal from which\nwe presumably have all the objects.\n\nI guess maybe it would be complicated if you had non-promisor objects\nthat refer indirectly to promisor ones. E.g., imagine ref A points to\nobject X, which is in a promisor pack pointing to Y (which we don't\nhave). But we also have ref B pointing to object Z which also refers to\nY, but _isn't_ in a promisor pack. I'm not sure that can actually happen\nwith the promisor mechanism, though (how did we get a Z without all of\nits objects into a non-promisor pack?).\n\nIt's also a shame that it would incur an extra object lookup, since if\nit _isn't_ in the promisor pack we'd then have to actually look it up\nagain via parse_object() or whatever. It may not be measurable though.\nIn an ideal world, we'd have an object access API that lets us open a\nhandle, ask things about it (like \"which pack is this coming from\") and\nthen load it if we want. But I don't think that needs to hold up this\nparticular topic.\n\n-Peff\n"},{"id":"373137","messageId":"9f327d6d8dc5e71eb0039aef3ac76ea16c2adab3.1554417917.git.steadmon@google.com","threadId":"50867","inReplyTo":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","subject":"[PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-04T22:53:56Z","receivedAt":"2019-04-04T22:54:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"For large repositories, enumerating the list of all promisor objects (in\norder to exclude them from a rev-list walk) can take a significant\namount of time).\n\nWhen --exclude-promisor-objects is passed to rev-list, don't enumerate\nthe promisor objects. Instead, filter them (and any children objects)\nduring the actual graph walk.\n\nRemove the mark_uninteresting() function as it's not used anywhere else.\n\nHelped-By: Jonathan Tan \nHelped-By: Jeff King \nHelped-By: Jonathan Nieder \n\nSigned-off-by: Josh Steadmon \n---\n\nRe-implemented following Jonathan & Jeff's advice (and also previously\nJonathan Nieder's, although I didn't understand it at the time). Thanks\nfor the feedback all.\n\n\n list-objects.c | 26 ++++++++++++++++++++++++++\n revision.c | 16 ----------------\n 2 files changed, 26 insertions(+), 16 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex dc77361e11..d1eaa0999e 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -30,6 +30,7 @@ static void process_blob(struct traversal_context *ctx,\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \n \tif (!ctx->revs->blob_objects)\n \t\treturn;\n@@ -37,6 +38,11 @@ static void process_blob(struct traversal_context *ctx,\n \t\tdie(\"bad blob object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n+\tif (ctx->revs->exclude_promisor_objects &&\n+\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n+\t oi.whence == OI_PACKED &&\n+\t oi.u.packed.pack->pack_promisor)\n+\t\treturn;\n \n \t/*\n \t * Pre-filter known-missing objects when explicitly requested.\n@@ -149,6 +155,7 @@ static void process_tree(struct traversal_context *ctx,\n \tint baselen = base->len;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \tint failed_parse;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \n \tif (!revs->tree_objects)\n \t\treturn;\n@@ -156,6 +163,11 @@ static void process_tree(struct traversal_context *ctx,\n \t\tdie(\"bad tree object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n+\tif (ctx->revs->exclude_promisor_objects &&\n+\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n+\t oi.whence == OI_PACKED &&\n+\t oi.u.packed.pack->pack_promisor)\n+\t\treturn;\n \n \tfailed_parse = parse_tree_gently(tree, 1);\n \tif (failed_parse) {\n@@ -318,6 +330,7 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,\n \t\t\t\t struct strbuf *base)\n {\n \tint i;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \n \tassert(base->len == 0);\n \n@@ -326,6 +339,12 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,\n \t\tstruct object *obj = pending->item;\n \t\tconst char *name = pending->name;\n \t\tconst char *path = pending->path;\n+\t\tif (ctx->revs->exclude_promisor_objects &&\n+\t\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n+\t\t oi.whence == OI_PACKED &&\n+\t\t oi.u.packed.pack->pack_promisor)\n+\t\t\tcontinue;\n+\n \t\tif (obj->flags & (UNINTERESTING | SEEN))\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n@@ -353,9 +372,16 @@ static void do_traverse(struct traversal_context *ctx)\n {\n \tstruct commit *commit;\n \tstruct strbuf csp; /* callee's scratch pad */\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \tstrbuf_init(&csp, PATH_MAX);\n \n \twhile ((commit = get_revision(ctx->revs)) != NULL) {\n+\t\tif (ctx->revs->exclude_promisor_objects &&\n+\t\t !oid_object_info_extended(the_repository, &commit->object.oid, &oi, 0) &&\n+\t\t oi.whence == OI_PACKED &&\n+\t\t oi.u.packed.pack->pack_promisor)\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..85974e941d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3067,17 +3067,6 @@ void reset_revision_walk(void)\n \tclear_object_flags(SEEN | ADDED | SHOWN);\n }\n \n-static int mark_uninteresting(const struct object_id *oid,\n-\t\t\t struct packed_git *pack,\n-\t\t\t uint32_t pos,\n-\t\t\t void *cb)\n-{\n-\tstruct rev_info *revs = cb;\n-\tstruct object *o = parse_object(revs->repo, oid);\n-\to->flags |= UNINTERESTING | SEEN;\n-\treturn 0;\n-}\n-\n define_commit_slab(indegree_slab, int);\n define_commit_slab(author_date_slab, timestamp_t);\n \n@@ -3316,11 +3305,6 @@ int prepare_revision_walk(struct rev_info *revs)\n \t (revs->limited && limiting_can_increase_treesame(revs)))\n \t\trevs->treesame.name = \"treesame\";\n \n-\tif (revs->exclude_promisor_objects) {\n-\t\tfor_each_packed_object(mark_uninteresting, revs,\n-\t\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n-\t}\n-\n \tif (revs->no_walk != REVISION_WALK_NO_WALK_UNSORTED)\n \t\tcommit_list_sort_by_date(&revs->commits);\n \tif (revs->no_walk)\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"373138","messageId":"20190404230759.GA26623@sigill.intra.peff.net","threadId":"50867","inReplyTo":"9f327d6d8dc5e71eb0039aef3ac76ea16c2adab3.1554417917.git.steadmon@google.com","subject":"Re: [PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-04T23:08:00Z","receivedAt":"2019-04-04T23:08:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 03:53:56PM -0700, Josh Steadmon wrote:\n\n> For large repositories, enumerating the list of all promisor objects (in\n> order to exclude them from a rev-list walk) can take a significant\n> amount of time).\n> \n> When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> the promisor objects. Instead, filter them (and any children objects)\n> during the actual graph walk.\n\nYeah, this is definitely the approach I was thinking of.\n\nDid you (or anybody else) have any thoughts on the case where a given\nobject is referred to both by a promisor and a non-promisor (and we\ndon't have it)? That's the \"shortcut\" I think we're taking here: we\nwould no longer realize that it's available via the promisor when we\ntraverse to it from the non-promisor. I'm just not clear on whether that\ncan ever happen.\n\n> Helped-By: Jonathan Tan \n> Helped-By: Jeff King \n> Helped-By: Jonathan Nieder \n> \n> Signed-off-by: Josh Steadmon \n\nMinor nit, but these should all be part of the same block.\n\n> diff --git a/list-objects.c b/list-objects.c\n> index dc77361e11..d1eaa0999e 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -30,6 +30,7 @@ static void process_blob(struct traversal_context *ctx,\n> \tstruct object *obj = &blob->object;\n> \tsize_t pathlen;\n> \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n> +\tstruct object_info oi = OBJECT_INFO_INIT;\n> \n> \tif (!ctx->revs->blob_objects)\n> \t\treturn;\n> @@ -37,6 +38,11 @@ static void process_blob(struct traversal_context *ctx,\n> \t\tdie(\"bad blob object\");\n> \tif (obj->flags & (UNINTERESTING | SEEN))\n> \t\treturn;\n> +\tif (ctx->revs->exclude_promisor_objects &&\n> +\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n> +\t oi.whence == OI_PACKED &&\n> +\t oi.u.packed.pack->pack_promisor)\n> +\t\treturn;\n\nThis conditional gets repeated a lot in your patch. Perhaps it's worth a\nhelper so we can say:\n\n if (skip_promisor_object(&ctx->revs, &obj->oid))\n\treturn;\n\nin each place?\n\nOne other possible small optimization: we don't look up the object\nunless the caller asked to exclude promisors, which is good. But we\ncould also keep a single flag for \"is there a promisor pack at all?\".\nWhen there isn't, we know there's no point in looking for the object.\n\nIt might not matter much in practice. The main caller here is going to\nbe check_connected(), and it only passes --exclude-promisor-objects if\nit's in a partial clone.\n\n> [...]\n\nI didn't see any tweaks to the callers, which makes sense; we're already\npassing --exclude-promisor-objects as necessary. Which means by itself,\nthis patch should be making things faster, right? Do you have timings to\nshow that off?\n\n-Peff\n"},{"id":"373152","messageId":"20190404234726.GG60888@google.com","threadId":"50867","inReplyTo":"20190404230759.GA26623@sigill.intra.peff.net","subject":"Re: [PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-04T23:47:26Z","receivedAt":"2019-04-04T23:49:13Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.04 19:08, Jeff King wrote:\n> On Thu, Apr 04, 2019 at 03:53:56PM -0700, Josh Steadmon wrote:\n> \n> > For large repositories, enumerating the list of all promisor objects (in\n> > order to exclude them from a rev-list walk) can take a significant\n> > amount of time).\n> > \n> > When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> > the promisor objects. Instead, filter them (and any children objects)\n> > during the actual graph walk.\n> \n> Yeah, this is definitely the approach I was thinking of.\n> \n> Did you (or anybody else) have any thoughts on the case where a given\n> object is referred to both by a promisor and a non-promisor (and we\n> don't have it)? That's the \"shortcut\" I think we're taking here: we\n> would no longer realize that it's available via the promisor when we\n> traverse to it from the non-promisor. I'm just not clear on whether that\n> can ever happen.\n\nI am not sure either. In process_blob() and process_tree() there are\nadditional checks for whether missing blobs/trees are promisor objects\nusing is_promisor_object()... but if we call that we undo the\nperformance gains from this change.\n\n\n> > Helped-By: Jonathan Tan \n> > Helped-By: Jeff King \n> > Helped-By: Jonathan Nieder \n> > \n> > Signed-off-by: Josh Steadmon \n> \n> Minor nit, but these should all be part of the same block.\n\nWill fix in v3.\n\n\n> > diff --git a/list-objects.c b/list-objects.c\n> > index dc77361e11..d1eaa0999e 100644\n> > --- a/list-objects.c\n> > +++ b/list-objects.c\n> > @@ -30,6 +30,7 @@ static void process_blob(struct traversal_context *ctx,\n> > \tstruct object *obj = &blob->object;\n> > \tsize_t pathlen;\n> > \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n> > +\tstruct object_info oi = OBJECT_INFO_INIT;\n> > \n> > \tif (!ctx->revs->blob_objects)\n> > \t\treturn;\n> > @@ -37,6 +38,11 @@ static void process_blob(struct traversal_context *ctx,\n> > \t\tdie(\"bad blob object\");\n> > \tif (obj->flags & (UNINTERESTING | SEEN))\n> > \t\treturn;\n> > +\tif (ctx->revs->exclude_promisor_objects &&\n> > +\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n> > +\t oi.whence == OI_PACKED &&\n> > +\t oi.u.packed.pack->pack_promisor)\n> > +\t\treturn;\n> \n> This conditional gets repeated a lot in your patch. Perhaps it's worth a\n> helper so we can say:\n> \n> if (skip_promisor_object(&ctx->revs, &obj->oid))\n> \treturn;\n> \n> in each place?\n\nWill fix in v3.\n\n\n> One other possible small optimization: we don't look up the object\n> unless the caller asked to exclude promisors, which is good. But we\n> could also keep a single flag for \"is there a promisor pack at all?\".\n> When there isn't, we know there's no point in looking for the object.\n> \n> It might not matter much in practice. The main caller here is going to\n> be check_connected(), and it only passes --exclude-promisor-objects if\n> it's in a partial clone.\n\nI'm not necessarily opposed, but I'm leaning towards the \"won't matter\nmuch\" side.\n\nWhere would such a flag live, in this case, and who would be responsible\nfor initializing it? I guess it would only matter for rev-list, so we\ncould initialize it in cmd_rev_list() if --exclude-promisor-objects is\npassed?\n\n> > [...]\n> \n> I didn't see any tweaks to the callers, which makes sense; we're already\n> passing --exclude-promisor-objects as necessary. Which means by itself,\n> this patch should be making things faster, right? Do you have timings to\n> show that off?\n\nYeah, for a partial clone of a large-ish Android repo [1], we see the\nconnectivity check go from >180s to ~7s.\n\n[1]: https://android.googlesource.com/platform/frameworks/base/\n"},{"id":"373153","messageId":"20190405000001.GA20793@sigill.intra.peff.net","threadId":"50867","inReplyTo":"20190404234726.GG60888@google.com","subject":"Re: [PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T00:00:01Z","receivedAt":"2019-04-05T00:00:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2019 at 04:47:26PM -0700, Josh Steadmon wrote:\n\n> > Did you (or anybody else) have any thoughts on the case where a given\n> > object is referred to both by a promisor and a non-promisor (and we\n> > don't have it)? That's the \"shortcut\" I think we're taking here: we\n> > would no longer realize that it's available via the promisor when we\n> > traverse to it from the non-promisor. I'm just not clear on whether that\n> > can ever happen.\n> \n> I am not sure either. In process_blob() and process_tree() there are\n> additional checks for whether missing blobs/trees are promisor objects\n> using is_promisor_object()... but if we call that we undo the\n> performance gains from this change.\n\nHmm. That might be a good outcome, though. If it never happens, we're\nfast. If it does happen, then our worst case is that we fall back to the\ncurrent slower-but-more-thorough check. (And I think that happens with\nyour patch, without us having to do anything further).\n\n> > One other possible small optimization: we don't look up the object\n> > unless the caller asked to exclude promisors, which is good. But we\n> > could also keep a single flag for \"is there a promisor pack at all?\".\n> > When there isn't, we know there's no point in looking for the object.\n> [...]\n> I'm not necessarily opposed, but I'm leaning towards the \"won't matter\n> much\" side.\n> \n> Where would such a flag live, in this case, and who would be responsible\n> for initializing it? I guess it would only matter for rev-list, so we\n> could initialize it in cmd_rev_list() if --exclude-promisor-objects is\n> passed?\n\nThe check is really something like:\n\n int have_promisor_pack() {\n\tfor (p = packed_git; p; p = p->next) {\n\t\tif (p->pack_promisor)\n\t\t\treturn 1;\n\t}\n\treturn 0;\n }\n\nThat could be lazily cached as a single bit, but it would need to be\nreset whenever we call reprepare_packed_git().\n\nLet's just punt on it for now. I'm not convinced it would actually yield\nany benefit, unless we have a partial-clone repo that doesn't have any\npromisor packs (but then, I suspect whatever un-partial'd it should\nprobably be resetting the partial flag in the config).\n\n> > I didn't see any tweaks to the callers, which makes sense; we're already\n> > passing --exclude-promisor-objects as necessary. Which means by itself,\n> > this patch should be making things faster, right? Do you have timings to\n> > show that off?\n> \n> Yeah, for a partial clone of a large-ish Android repo [1], we see the\n> connectivity check go from >180s to ~7s.\n\nThose are nice numbers. :) Worth mentioning in the commit message, I\nthink. How does it compare to your earlier patch? I'd hope they're about\nthe same.\n\n-Peff\n"},{"id":"373154","messageId":"20190405000934.GH60888@google.com","threadId":"50867","inReplyTo":"20190405000001.GA20793@sigill.intra.peff.net","subject":"Re: [PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-05T00:09:34Z","receivedAt":"2019-04-05T00:09:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.04 20:00, Jeff King wrote:\n> On Thu, Apr 04, 2019 at 04:47:26PM -0700, Josh Steadmon wrote:\n> \n> > > Did you (or anybody else) have any thoughts on the case where a given\n> > > object is referred to both by a promisor and a non-promisor (and we\n> > > don't have it)? That's the \"shortcut\" I think we're taking here: we\n> > > would no longer realize that it's available via the promisor when we\n> > > traverse to it from the non-promisor. I'm just not clear on whether that\n> > > can ever happen.\n> > \n> > I am not sure either. In process_blob() and process_tree() there are\n> > additional checks for whether missing blobs/trees are promisor objects\n> > using is_promisor_object()... but if we call that we undo the\n> > performance gains from this change.\n> \n> Hmm. That might be a good outcome, though. If it never happens, we're\n> fast. If it does happen, then our worst case is that we fall back to the\n> current slower-but-more-thorough check. (And I think that happens with\n> your patch, without us having to do anything further).\n> \n> > > One other possible small optimization: we don't look up the object\n> > > unless the caller asked to exclude promisors, which is good. But we\n> > > could also keep a single flag for \"is there a promisor pack at all?\".\n> > > When there isn't, we know there's no point in looking for the object.\n> > [...]\n> > I'm not necessarily opposed, but I'm leaning towards the \"won't matter\n> > much\" side.\n> > \n> > Where would such a flag live, in this case, and who would be responsible\n> > for initializing it? I guess it would only matter for rev-list, so we\n> > could initialize it in cmd_rev_list() if --exclude-promisor-objects is\n> > passed?\n> \n> The check is really something like:\n> \n> int have_promisor_pack() {\n> \tfor (p = packed_git; p; p = p->next) {\n> \t\tif (p->pack_promisor)\n> \t\t\treturn 1;\n> \t}\n> \treturn 0;\n> }\n> \n> That could be lazily cached as a single bit, but it would need to be\n> reset whenever we call reprepare_packed_git().\n> \n> Let's just punt on it for now. I'm not convinced it would actually yield\n> any benefit, unless we have a partial-clone repo that doesn't have any\n> promisor packs (but then, I suspect whatever un-partial'd it should\n> probably be resetting the partial flag in the config).\n> \n> > > I didn't see any tweaks to the callers, which makes sense; we're already\n> > > passing --exclude-promisor-objects as necessary. Which means by itself,\n> > > this patch should be making things faster, right? Do you have timings to\n> > > show that off?\n> > \n> > Yeah, for a partial clone of a large-ish Android repo [1], we see the\n> > connectivity check go from >180s to ~7s.\n> \n> Those are nice numbers. :) Worth mentioning in the commit message, I\n> think. How does it compare to your earlier patch? I'd hope they're about\n> the same.\n\nThanks, will include them in the v3 commit message. Unfortunately it's\nhard to compare against v1, because v1 doesn't call rev-list at all, and\nthus we don't have a good measurement in the trace / trace2 output. The\nrev-list timing has been pretty consistent at just a bit over 3 minutes,\nbut the overall clone takes anywhere from 12-20 minutes, so any\ndifference between v1 and v2 performance just gets lost in the noise. If\nI get a chance on Monday I may go back to v1 and add some timing.\n"},{"id":"373440","messageId":"20190408205934.GI60888@google.com","threadId":"50867","inReplyTo":"20190405000934.GH60888@google.com","subject":"Re: [PATCH v2] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-08T20:59:34Z","receivedAt":"2019-04-08T20:59:46Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.04 17:09, Josh Steadmon wrote:\n> On 2019.04.04 20:00, Jeff King wrote:\n> > On Thu, Apr 04, 2019 at 04:47:26PM -0700, Josh Steadmon wrote:\n> > \n> > > > Did you (or anybody else) have any thoughts on the case where a given\n> > > > object is referred to both by a promisor and a non-promisor (and we\n> > > > don't have it)? That's the \"shortcut\" I think we're taking here: we\n> > > > would no longer realize that it's available via the promisor when we\n> > > > traverse to it from the non-promisor. I'm just not clear on whether that\n> > > > can ever happen.\n> > > \n> > > I am not sure either. In process_blob() and process_tree() there are\n> > > additional checks for whether missing blobs/trees are promisor objects\n> > > using is_promisor_object()... but if we call that we undo the\n> > > performance gains from this change.\n> > \n> > Hmm. That might be a good outcome, though. If it never happens, we're\n> > fast. If it does happen, then our worst case is that we fall back to the\n> > current slower-but-more-thorough check. (And I think that happens with\n> > your patch, without us having to do anything further).\n> > \n> > > > One other possible small optimization: we don't look up the object\n> > > > unless the caller asked to exclude promisors, which is good. But we\n> > > > could also keep a single flag for \"is there a promisor pack at all?\".\n> > > > When there isn't, we know there's no point in looking for the object.\n> > > [...]\n> > > I'm not necessarily opposed, but I'm leaning towards the \"won't matter\n> > > much\" side.\n> > > \n> > > Where would such a flag live, in this case, and who would be responsible\n> > > for initializing it? I guess it would only matter for rev-list, so we\n> > > could initialize it in cmd_rev_list() if --exclude-promisor-objects is\n> > > passed?\n> > \n> > The check is really something like:\n> > \n> > int have_promisor_pack() {\n> > \tfor (p = packed_git; p; p = p->next) {\n> > \t\tif (p->pack_promisor)\n> > \t\t\treturn 1;\n> > \t}\n> > \treturn 0;\n> > }\n> > \n> > That could be lazily cached as a single bit, but it would need to be\n> > reset whenever we call reprepare_packed_git().\n> > \n> > Let's just punt on it for now. I'm not convinced it would actually yield\n> > any benefit, unless we have a partial-clone repo that doesn't have any\n> > promisor packs (but then, I suspect whatever un-partial'd it should\n> > probably be resetting the partial flag in the config).\n> > \n> > > > I didn't see any tweaks to the callers, which makes sense; we're already\n> > > > passing --exclude-promisor-objects as necessary. Which means by itself,\n> > > > this patch should be making things faster, right? Do you have timings to\n> > > > show that off?\n> > > \n> > > Yeah, for a partial clone of a large-ish Android repo [1], we see the\n> > > connectivity check go from >180s to ~7s.\n> > \n> > Those are nice numbers. :) Worth mentioning in the commit message, I\n> > think. How does it compare to your earlier patch? I'd hope they're about\n> > the same.\n> \n> Thanks, will include them in the v3 commit message. Unfortunately it's\n> hard to compare against v1, because v1 doesn't call rev-list at all, and\n> thus we don't have a good measurement in the trace / trace2 output. The\n> rev-list timing has been pretty consistent at just a bit over 3 minutes,\n> but the overall clone takes anywhere from 12-20 minutes, so any\n> difference between v1 and v2 performance just gets lost in the noise. If\n> I get a chance on Monday I may go back to v1 and add some timing.\n\nFor the record, the V1 abbreviated check takes about 5ms\n"},{"id":"373441","messageId":"9856e7fc74f51b60ae162cbed3f5c0cf8c603222.1554757275.git.steadmon@google.com","threadId":"50867","inReplyTo":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","subject":"[PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-08T21:06:04Z","receivedAt":"2019-04-08T21:06:09Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"For large repositories, enumerating the list of all promisor objects (in\norder to exclude them from a rev-list walk) can take a significant\namount of time).\n\nWhen --exclude-promisor-objects is passed to rev-list, don't enumerate\nthe promisor objects. Instead, filter them (and any children objects)\nduring the actual graph walk.\n\nRemove the mark_uninteresting() function as it's not used anywhere else.\n\nWhen testing against a large repo [1], this patch reduces the\nconnectivity check runtime from 3 minutes to ~7 seconds.\n\n[1]: https://android.googlesource.com/platform/frameworks/base/\n\nHelped-By: Jonathan Tan \nHelped-By: Jeff King \nHelped-By: Jonathan Nieder \nSigned-off-by: Josh Steadmon \n\nSigned-off-by: Josh Steadmon \n---\nChanges since V2:\n* Pulled the \"OK to skip?\" logic into a separate function.\n\nChanges since V1:\n* Switched to alternate approach, we now do the regular rev-list walk\n but skip promisor objects at walk time, rather than pre-excluding\n them.\n\nRange-diff against v2:\n1: 9f327d6d8d ! 1: 9856e7fc74 rev-list: exclude promisor objects at walk time\n @@ -10,9 +10,15 @@\n the promisor objects. Instead, filter them (and any children objects)\n during the actual graph walk.\n \n + When testing against a large repo [1], this reduces the connectivity\n + check runtime from 3 minutes to ~7 seconds.\n +\n + [1]: https://android.googlesource.com/platform/frameworks/base/\n +\n Helped-By: Jonathan Tan \n Helped-By: Jeff King \n Helped-By: Jonathan Nieder \n + Signed-off-by: Josh Steadmon \n \n \n @@ -20,78 +26,55 @@\n --- a/list-objects.c\n +++ b/list-objects.c\n @@\n - \tstruct object *obj = &blob->object;\n - \tsize_t pathlen;\n - \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n -+\tstruct object_info oi = OBJECT_INFO_INIT;\n + \tvoid *filter_data;\n + };\n \n - \tif (!ctx->revs->blob_objects)\n - \t\treturn;\n ++static int should_skip_promisor_object(const struct rev_info *revs,\n ++\t\t\t\t const struct object_id *oid)\n ++{\n ++\tstruct object_info oi = OBJECT_INFO_INIT;\n ++\treturn (revs->exclude_promisor_objects &&\n ++\t\t!oid_object_info_extended(the_repository, oid, &oi, 0) &&\n ++\t\toi.whence == OI_PACKED &&\n ++\t\toi.u.packed.pack->pack_promisor);\n ++}\n ++\n + static void process_blob(struct traversal_context *ctx,\n + \t\t\t struct blob *blob,\n + \t\t\t struct strbuf *path,\n @@\n \t\tdie(\"bad blob object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n -+\tif (ctx->revs->exclude_promisor_objects &&\n -+\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n -+\t oi.whence == OI_PACKED &&\n -+\t oi.u.packed.pack->pack_promisor)\n ++\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n +\t\treturn;\n \n \t/*\n \t * Pre-filter known-missing objects when explicitly requested.\n -@@\n - \tint baselen = base->len;\n - \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n - \tint failed_parse;\n -+\tstruct object_info oi = OBJECT_INFO_INIT;\n - \n - \tif (!revs->tree_objects)\n - \t\treturn;\n @@\n \t\tdie(\"bad tree object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n -+\tif (ctx->revs->exclude_promisor_objects &&\n -+\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n -+\t oi.whence == OI_PACKED &&\n -+\t oi.u.packed.pack->pack_promisor)\n ++\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n +\t\treturn;\n \n \tfailed_parse = parse_tree_gently(tree, 1);\n \tif (failed_parse) {\n -@@\n - \t\t\t\t struct strbuf *base)\n - {\n - \tint i;\n -+\tstruct object_info oi = OBJECT_INFO_INIT;\n - \n - \tassert(base->len == 0);\n - \n @@\n \t\tstruct object *obj = pending->item;\n \t\tconst char *name = pending->name;\n \t\tconst char *path = pending->path;\n -+\t\tif (ctx->revs->exclude_promisor_objects &&\n -+\t\t !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&\n -+\t\t oi.whence == OI_PACKED &&\n -+\t\t oi.u.packed.pack->pack_promisor)\n ++\t\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n +\t\t\tcontinue;\n +\n \t\tif (obj->flags & (UNINTERESTING | SEEN))\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n @@\n - {\n - \tstruct commit *commit;\n - \tstruct strbuf csp; /* callee's scratch pad */\n -+\tstruct object_info oi = OBJECT_INFO_INIT;\n \tstrbuf_init(&csp, PATH_MAX);\n \n \twhile ((commit = get_revision(ctx->revs)) != NULL) {\n -+\t\tif (ctx->revs->exclude_promisor_objects &&\n -+\t\t !oid_object_info_extended(the_repository, &commit->object.oid, &oi, 0) &&\n -+\t\t oi.whence == OI_PACKED &&\n -+\t\t oi.u.packed.pack->pack_promisor)\n ++\t\tif (should_skip_promisor_object(ctx->revs, &commit->object.oid))\n +\t\t\tcontinue;\n +\n \t\t/*\n\n list-objects.c | 20 ++++++++++++++++++++\n revision.c | 16 ----------------\n 2 files changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex dc77361e11..c153ee5dfb 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -22,6 +22,16 @@ struct traversal_context {\n \tvoid *filter_data;\n };\n \n+static int should_skip_promisor_object(const struct rev_info *revs,\n+\t\t\t\t const struct object_id *oid)\n+{\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n+\treturn (revs->exclude_promisor_objects &&\n+\t\t!oid_object_info_extended(the_repository, oid, &oi, 0) &&\n+\t\toi.whence == OI_PACKED &&\n+\t\toi.u.packed.pack->pack_promisor);\n+}\n+\n static void process_blob(struct traversal_context *ctx,\n \t\t\t struct blob *blob,\n \t\t\t struct strbuf *path,\n@@ -37,6 +47,8 @@ static void process_blob(struct traversal_context *ctx,\n \t\tdie(\"bad blob object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n+\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n+\t\treturn;\n \n \t/*\n \t * Pre-filter known-missing objects when explicitly requested.\n@@ -156,6 +168,8 @@ static void process_tree(struct traversal_context *ctx,\n \t\tdie(\"bad tree object\");\n \tif (obj->flags & (UNINTERESTING | SEEN))\n \t\treturn;\n+\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n+\t\treturn;\n \n \tfailed_parse = parse_tree_gently(tree, 1);\n \tif (failed_parse) {\n@@ -326,6 +340,9 @@ static void traverse_trees_and_blobs(struct traversal_context *ctx,\n \t\tstruct object *obj = pending->item;\n \t\tconst char *name = pending->name;\n \t\tconst char *path = pending->path;\n+\t\tif (should_skip_promisor_object(ctx->revs, &obj->oid))\n+\t\t\tcontinue;\n+\n \t\tif (obj->flags & (UNINTERESTING | SEEN))\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n@@ -356,6 +373,9 @@ static void do_traverse(struct traversal_context *ctx)\n \tstrbuf_init(&csp, PATH_MAX);\n \n \twhile ((commit = get_revision(ctx->revs)) != NULL) {\n+\t\tif (should_skip_promisor_object(ctx->revs, &commit->object.oid))\n+\t\t\tcontinue;\n+\n \t\t/*\n \t\t * an uninteresting boundary commit may not have its tree\n \t\t * parsed yet, but we are not going to show them anyway\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..85974e941d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3067,17 +3067,6 @@ void reset_revision_walk(void)\n \tclear_object_flags(SEEN | ADDED | SHOWN);\n }\n \n-static int mark_uninteresting(const struct object_id *oid,\n-\t\t\t struct packed_git *pack,\n-\t\t\t uint32_t pos,\n-\t\t\t void *cb)\n-{\n-\tstruct rev_info *revs = cb;\n-\tstruct object *o = parse_object(revs->repo, oid);\n-\to->flags |= UNINTERESTING | SEEN;\n-\treturn 0;\n-}\n-\n define_commit_slab(indegree_slab, int);\n define_commit_slab(author_date_slab, timestamp_t);\n \n@@ -3316,11 +3305,6 @@ int prepare_revision_walk(struct rev_info *revs)\n \t (revs->limited && limiting_can_increase_treesame(revs)))\n \t\trevs->treesame.name = \"treesame\";\n \n-\tif (revs->exclude_promisor_objects) {\n-\t\tfor_each_packed_object(mark_uninteresting, revs,\n-\t\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n-\t}\n-\n \tif (revs->no_walk != REVISION_WALK_NO_WALK_UNSORTED)\n \t\tcommit_list_sort_by_date(&revs->commits);\n \tif (revs->no_walk)\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"373444","messageId":"CAP8UFD3fWi=3rYY1LAzn8XWvHQswDZmkWj7TNicTE+qbnOGkJQ@mail.gmail.com","threadId":"50867","inReplyTo":"9856e7fc74f51b60ae162cbed3f5c0cf8c603222.1554757275.git.steadmon@google.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-08T22:23:33Z","receivedAt":"2019-04-08T22:23:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 8, 2019 at 11:46 PM Josh Steadmon wrote:\n>\n> Helped-By: Jonathan Tan \n> Helped-By: Jeff King \n> Helped-By: Jonathan Nieder \n> Signed-off-by: Josh Steadmon \n>\n> Signed-off-by: Josh Steadmon \n\nYour S-o-B is duplicated an there is a spurious line between the\nduplicated lines. Do you use an automated script/hook to add your\nS-o-B?\n"},{"id":"373445","messageId":"20190408231256.GK60888@google.com","threadId":"50867","inReplyTo":"CAP8UFD3fWi=3rYY1LAzn8XWvHQswDZmkWj7TNicTE+qbnOGkJQ@mail.gmail.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-08T23:12:56Z","receivedAt":"2019-04-08T23:13:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.09 00:23, Christian Couder wrote:\n> On Mon, Apr 8, 2019 at 11:46 PM Josh Steadmon wrote:\n> >\n> > Helped-By: Jonathan Tan \n> > Helped-By: Jeff King \n> > Helped-By: Jonathan Nieder \n> > Signed-off-by: Josh Steadmon \n> >\n> > Signed-off-by: Josh Steadmon \n> \n> Your S-o-B is duplicated an there is a spurious line between the\n> duplicated lines. Do you use an automated script/hook to add your\n> S-o-B?\n\nI only use the --signoff flag to git-format-patch. It looks like the\ncause is that I have a hook to add a Gerrit Change-Id to my commit\nmessages, and that hook added a blank line followed by Change-Id:blah\nafter my original Signed-off-by line. Then git-format-patch added\nanother Signed-off-by after the Change-Id. The Change-Id line was then\nstripped out by a wrapper around git-send-email.\n"},{"id":"373493","messageId":"xmqqo95f43dq.fsf@gitster-ct.c.googlers.com","threadId":"50867","inReplyTo":"9856e7fc74f51b60ae162cbed3f5c0cf8c603222.1554757275.git.steadmon@google.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T15:14:41Z","receivedAt":"2019-04-09T15:14:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> For large repositories, enumerating the list of all promisor objects (in\n> order to exclude them from a rev-list walk) can take a significant\n> amount of time).\n>\n> When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> the promisor objects. Instead, filter them (and any children objects)\n> during the actual graph walk.\n>\n> Remove the mark_uninteresting() function as it's not used anywhere else.\n>\n> When testing against a large repo [1], this patch reduces the\n> connectivity check runtime from 3 minutes to ~7 seconds.\n>\n> [1]: https://android.googlesource.com/platform/frameworks/base/\n>\n> Helped-By: Jonathan Tan \n> Helped-By: Jeff King \n> Helped-By: Jonathan Nieder \n> Signed-off-by: Josh Steadmon \n>\n> Signed-off-by: Josh Steadmon \n\nI've dealt with the stray double-sign-off locally, but is there\nanything else planned for v4 or later? Is this performance-only\nchange, or does it have an externally observable behaviour change\nthat we can easily add to our test suite?\n\n> list-objects.c | 20 ++++++++++++++++++++\n> revision.c | 16 ----------------\n> 2 files changed, 20 insertions(+), 16 deletions(-)\n\n"},{"id":"373495","messageId":"20190409151559.GB12627@sigill.intra.peff.net","threadId":"50867","inReplyTo":"xmqqo95f43dq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-09T15:15:59Z","receivedAt":"2019-04-09T15:16:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 10, 2019 at 12:14:41AM +0900, Junio C Hamano wrote:\n\n> Josh Steadmon writes:\n> \n> > For large repositories, enumerating the list of all promisor objects (in\n> > order to exclude them from a rev-list walk) can take a significant\n> > amount of time).\n> >\n> > When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> > the promisor objects. Instead, filter them (and any children objects)\n> > during the actual graph walk.\n> >\n> > Remove the mark_uninteresting() function as it's not used anywhere else.\n> >\n> > When testing against a large repo [1], this patch reduces the\n> > connectivity check runtime from 3 minutes to ~7 seconds.\n> >\n> > [1]: https://android.googlesource.com/platform/frameworks/base/\n> >\n> > Helped-By: Jonathan Tan \n> > Helped-By: Jeff King \n> > Helped-By: Jonathan Nieder \n> > Signed-off-by: Josh Steadmon \n> >\n> > Signed-off-by: Josh Steadmon \n> \n> I've dealt with the stray double-sign-off locally, but is there\n> anything else planned for v4 or later? Is this performance-only\n> change, or does it have an externally observable behaviour change\n> that we can easily add to our test suite?\n\nI am OK if we do not include it, but even if this is \"just\" a\nperformance-only change, we can always add to our perf regression suite.\n\n-Peff\n"},{"id":"373499","messageId":"xmqqbm1f421c.fsf@gitster-ct.c.googlers.com","threadId":"50867","inReplyTo":"20190409151559.GB12627@sigill.intra.peff.net","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T15:43:43Z","receivedAt":"2019-04-09T15:43:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 10, 2019 at 12:14:41AM +0900, Junio C Hamano wrote:\n>\n>> I've dealt with the stray double-sign-off locally, but is there\n>> anything else planned for v4 or later? Is this performance-only\n>> change, or does it have an externally observable behaviour change\n>> that we can easily add to our test suite?\n>\n> I am OK if we do not include it, but even if this is \"just\" a\n> performance-only change, we can always add to our perf regression suite.\n\nHmph, that does not say much about a possible change in behaviour in\ncorner cases you guys were discuussing near the beginning of the\nthread when an object can be reached from both a non-promisor and a\npromisor object, does it?\n\nShouldn't we at least tweak the log message to record that we were\naware of the possibility even though we couldn't readily come up\nwith a case where this optimization breaks things? I suspect that\nit would help the next person who needs to deal with a possible\nregression coming from this change to understand the problem better\nand hopefully faster.\n\n"},{"id":"373521","messageId":"20190409163535.GL60888@google.com","threadId":"50867","inReplyTo":"xmqqbm1f421c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-09T16:35:35Z","receivedAt":"2019-04-09T16:35:44Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.10 00:43, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Wed, Apr 10, 2019 at 12:14:41AM +0900, Junio C Hamano wrote:\n> >\n> >> I've dealt with the stray double-sign-off locally, but is there\n> >> anything else planned for v4 or later? Is this performance-only\n> >> change, or does it have an externally observable behaviour change\n> >> that we can easily add to our test suite?\n> >\n> > I am OK if we do not include it, but even if this is \"just\" a\n> > performance-only change, we can always add to our perf regression suite.\n> \n> Hmph, that does not say much about a possible change in behaviour in\n> corner cases you guys were discuussing near the beginning of the\n> thread when an object can be reached from both a non-promisor and a\n> promisor object, does it?\n> \n> Shouldn't we at least tweak the log message to record that we were\n> aware of the possibility even though we couldn't readily come up\n> with a case where this optimization breaks things? I suspect that\n> it would help the next person who needs to deal with a possible\n> regression coming from this change to understand the problem better\n> and hopefully faster.\n> \n\nI'll update the log message and send a v4 in a few minutes.\n"},{"id":"373524","messageId":"20190409180418.GH8796@szeder.dev","threadId":"50867","inReplyTo":"9856e7fc74f51b60ae162cbed3f5c0cf8c603222.1554757275.git.steadmon@google.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-09T18:04:18Z","receivedAt":"2019-04-09T18:04:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 08, 2019 at 02:06:04PM -0700, Josh Steadmon wrote:\n> For large repositories, enumerating the list of all promisor objects (in\n> order to exclude them from a rev-list walk) can take a significant\n> amount of time).\n> \n> When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> the promisor objects. Instead, filter them (and any children objects)\n> during the actual graph walk.\n> \n> Remove the mark_uninteresting() function as it's not used anywhere else.\n> \n> When testing against a large repo [1], this patch reduces the\n> connectivity check runtime from 3 minutes to ~7 seconds.\n\nThis patch breaks test 'repack -d does not irreversibly delete\npromisor objects' in 't0410-partial-clone.sh' when run with\nGIT_TEST_COMMIT_GRAPH=1.\n\n +rm -rf repo\n +test_create_repo repo\n +test 1 = 1\n +repo=repo\n +mkdir -p repo\n +cd repo\n +/home/travis/build/git/git/t/../git init --template=/home/travis/build/git/git/t/../templates/blt/\n Initialized empty Git repository in /home/travis/build/git/git/t/trash directory.t0410-partial-clone/repo/.git/\n +mv .git/hooks .git/hooks-disabled\n +git -C repo config core.repositoryformatversion 1\n +git -C repo config extensions.partialclone arbitrary string\n +git -C repo commit --allow-empty -m one\n [master (root-commit) 71905df] one\n Author: A U Thor \n +git -C repo commit --allow-empty -m two\n [master 202c4a3] two\n Author: A U Thor \n +git -C repo commit --allow-empty -m three\n [master 4737577] three\n Author: A U Thor \n +git -C repo commit --allow-empty -m four\n [master d6ba7e0] four\n Author: A U Thor \n +git -C repo rev-parse HEAD^^^\n +ONE=71905dfcd543b7cbb0b4b66fbd20379e67220557\n +git -C repo rev-parse HEAD^^\n +TWO=202c4a3dd9a2dac927f056abb747cce9ea2eb67b\n +git -C repo rev-parse HEAD^\n +THREE=47375779ebcca4b422e3afdd14aa37a358081297\n +pack_as_from_promisor\n +printf 202c4a3dd9a2dac927f056abb747cce9ea2eb67b\\n\n +git -C repo pack-objects .git/objects/pack/pack\n +HASH=2e675cd706e508d6c52a21d28cfcddde5ec02a06\n +\n +echo 2e675cd706e508d6c52a21d28cfcddde5ec02a06\n 2e675cd706e508d6c52a21d28cfcddde5ec02a06\n +printf 47375779ebcca4b422e3afdd14aa37a358081297\\n\n +pack_as_from_promisor\n +git -C repo pack-objects .git/objects/pack/pack\n +HASH=31f7d2797549ab9b1c425a9e60eb2030481170e5\n +\n +echo 31f7d2797549ab9b1c425a9e60eb2030481170e5\n 31f7d2797549ab9b1c425a9e60eb2030481170e5\n +delete_object repo 71905dfcd543b7cbb0b4b66fbd20379e67220557\n +sed -e s|^..|&/|\n +echo 71905dfcd543b7cbb0b4b66fbd20379e67220557\n +rm repo/.git/objects/71/905dfcd543b7cbb0b4b66fbd20379e67220557\n +repack_and_check -a 202c4a3dd9a2dac927f056abb747cce9ea2eb67b 47375779ebcca4b422e3afdd14aa37a358081297\n +rm -rf repo2\n +cp -r repo repo2\n +git -C repo2 repack -a -d\n warning: reflog of 'HEAD' references pruned commits\n warning: reflog of 'refs/heads/master' references pruned commits\n fatal: unable to read 71905dfcd543b7cbb0b4b66fbd20379e67220557\n error: last command exited with $?=128\n not ok 23 - repack -d does not irreversibly delete promisor objects\n\n\n https://travis-ci.org/git/git/jobs/517874310#L5822\n\n"},{"id":"373545","messageId":"20190409234255.GM60888@google.com","threadId":"50867","inReplyTo":"20190409180418.GH8796@szeder.dev","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-09T23:42:55Z","receivedAt":"2019-04-09T23:43:06Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.09 20:04, SZEDER Gábor wrote:\n> On Mon, Apr 08, 2019 at 02:06:04PM -0700, Josh Steadmon wrote:\n> > For large repositories, enumerating the list of all promisor objects (in\n> > order to exclude them from a rev-list walk) can take a significant\n> > amount of time).\n> > \n> > When --exclude-promisor-objects is passed to rev-list, don't enumerate\n> > the promisor objects. Instead, filter them (and any children objects)\n> > during the actual graph walk.\n> > \n> > Remove the mark_uninteresting() function as it's not used anywhere else.\n> > \n> > When testing against a large repo [1], this patch reduces the\n> > connectivity check runtime from 3 minutes to ~7 seconds.\n> \n> This patch breaks test 'repack -d does not irreversibly delete\n> promisor objects' in 't0410-partial-clone.sh' when run with\n> GIT_TEST_COMMIT_GRAPH=1.\n> \n> +rm -rf repo\n> +test_create_repo repo\n> +test 1 = 1\n> +repo=repo\n> +mkdir -p repo\n> +cd repo\n> +/home/travis/build/git/git/t/../git init --template=/home/travis/build/git/git/t/../templates/blt/\n> Initialized empty Git repository in /home/travis/build/git/git/t/trash directory.t0410-partial-clone/repo/.git/\n> +mv .git/hooks .git/hooks-disabled\n> +git -C repo config core.repositoryformatversion 1\n> +git -C repo config extensions.partialclone arbitrary string\n> +git -C repo commit --allow-empty -m one\n> [master (root-commit) 71905df] one\n> Author: A U Thor \n> +git -C repo commit --allow-empty -m two\n> [master 202c4a3] two\n> Author: A U Thor \n> +git -C repo commit --allow-empty -m three\n> [master 4737577] three\n> Author: A U Thor \n> +git -C repo commit --allow-empty -m four\n> [master d6ba7e0] four\n> Author: A U Thor \n> +git -C repo rev-parse HEAD^^^\n> +ONE=71905dfcd543b7cbb0b4b66fbd20379e67220557\n> +git -C repo rev-parse HEAD^^\n> +TWO=202c4a3dd9a2dac927f056abb747cce9ea2eb67b\n> +git -C repo rev-parse HEAD^\n> +THREE=47375779ebcca4b422e3afdd14aa37a358081297\n> +pack_as_from_promisor\n> +printf 202c4a3dd9a2dac927f056abb747cce9ea2eb67b\\n\n> +git -C repo pack-objects .git/objects/pack/pack\n> +HASH=2e675cd706e508d6c52a21d28cfcddde5ec02a06\n> +\n> +echo 2e675cd706e508d6c52a21d28cfcddde5ec02a06\n> 2e675cd706e508d6c52a21d28cfcddde5ec02a06\n> +printf 47375779ebcca4b422e3afdd14aa37a358081297\\n\n> +pack_as_from_promisor\n> +git -C repo pack-objects .git/objects/pack/pack\n> +HASH=31f7d2797549ab9b1c425a9e60eb2030481170e5\n> +\n> +echo 31f7d2797549ab9b1c425a9e60eb2030481170e5\n> 31f7d2797549ab9b1c425a9e60eb2030481170e5\n> +delete_object repo 71905dfcd543b7cbb0b4b66fbd20379e67220557\n> +sed -e s|^..|&/|\n> +echo 71905dfcd543b7cbb0b4b66fbd20379e67220557\n> +rm repo/.git/objects/71/905dfcd543b7cbb0b4b66fbd20379e67220557\n> +repack_and_check -a 202c4a3dd9a2dac927f056abb747cce9ea2eb67b 47375779ebcca4b422e3afdd14aa37a358081297\n> +rm -rf repo2\n> +cp -r repo repo2\n> +git -C repo2 repack -a -d\n> warning: reflog of 'HEAD' references pruned commits\n> warning: reflog of 'refs/heads/master' references pruned commits\n> fatal: unable to read 71905dfcd543b7cbb0b4b66fbd20379e67220557\n> error: last command exited with $?=128\n> not ok 23 - repack -d does not irreversibly delete promisor objects\n> \n> \n> https://travis-ci.org/git/git/jobs/517874310#L5822\n> \n\nThank you for catching this. I haven't yet figured out the cause. I will\nlook into this more tomorrow and then send a V4 once I've fixed it.\n"},{"id":"373625","messageId":"20190411040642.GA8418@sigill.intra.peff.net","threadId":"50867","inReplyTo":"20190409234255.GM60888@google.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T04:06:42Z","receivedAt":"2019-04-11T04:06:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 09, 2019 at 04:42:55PM -0700, Josh Steadmon wrote:\n\n> > warning: reflog of 'HEAD' references pruned commits\n> > warning: reflog of 'refs/heads/master' references pruned commits\n> > fatal: unable to read 71905dfcd543b7cbb0b4b66fbd20379e67220557\n> > error: last command exited with $?=128\n> > not ok 23 - repack -d does not irreversibly delete promisor objects\n> > \n> \n> Thank you for catching this. I haven't yet figured out the cause. I will\n> look into this more tomorrow and then send a V4 once I've fixed it.\n\nI'm concerned that this is a sign that the approach I suggested does not\nactually work everywhere. I.e., could this be a case where we have some\nnon-promisor object that points to a sub-object that is reachable from\nthe promisor pack, but not a direct tip? Before your patch we'd consider\nthat sub-object a promisor (because we enumerate all of the graph that\nwe do have and mark each such object), but afterwards we would not.\n\nAnd I wonder if that confuses pack-objects. Though I think it would\nconfuse it in the _opposite_ direction. I.e., using\n--exclude-promisor-objects would count such an object as not-a-promisor\nand would be more inclined to include it in the new pack.\n\nIt is curious that this only turns up with GIT_TEST_COMMIT_GRAPH=1, too.\nIt seems like any such problem ought to be independent of that.\n\nPuzzling...\n\n-Peff\n"},{"id":"373767","messageId":"20190412223847.GO60888@google.com","threadId":"50867","inReplyTo":"20190411040642.GA8418@sigill.intra.peff.net","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-12T22:38:47Z","receivedAt":"2019-04-12T22:39:59Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.11 00:06, Jeff King wrote:\n> On Tue, Apr 09, 2019 at 04:42:55PM -0700, Josh Steadmon wrote:\n> \n> > > warning: reflog of 'HEAD' references pruned commits\n> > > warning: reflog of 'refs/heads/master' references pruned commits\n> > > fatal: unable to read 71905dfcd543b7cbb0b4b66fbd20379e67220557\n> > > error: last command exited with $?=128\n> > > not ok 23 - repack -d does not irreversibly delete promisor objects\n> > > \n> > \n> > Thank you for catching this. I haven't yet figured out the cause. I will\n> > look into this more tomorrow and then send a V4 once I've fixed it.\n> \n> I'm concerned that this is a sign that the approach I suggested does not\n> actually work everywhere. I.e., could this be a case where we have some\n> non-promisor object that points to a sub-object that is reachable from\n> the promisor pack, but not a direct tip? Before your patch we'd consider\n> that sub-object a promisor (because we enumerate all of the graph that\n> we do have and mark each such object), but afterwards we would not.\n> \n> And I wonder if that confuses pack-objects. Though I think it would\n> confuse it in the _opposite_ direction. I.e., using\n> --exclude-promisor-objects would count such an object as not-a-promisor\n> and would be more inclined to include it in the new pack.\n> \n> It is curious that this only turns up with GIT_TEST_COMMIT_GRAPH=1, too.\n> It seems like any such problem ought to be independent of that.\n> \n> Puzzling...\n> \n> -Peff\n\nDo you think this justifies going back to the V1 approach (only checking\npresence of objects pointed to by refs when doing a partial clone)?\n"},{"id":"373777","messageId":"20190413053434.GA31884@sigill.intra.peff.net","threadId":"50867","inReplyTo":"20190412223847.GO60888@google.com","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-13T05:34:35Z","receivedAt":"2019-04-13T05:34:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 12, 2019 at 03:38:47PM -0700, Josh Steadmon wrote:\n\n> > > Thank you for catching this. I haven't yet figured out the cause. I will\n> > > look into this more tomorrow and then send a V4 once I've fixed it.\n> > \n> > I'm concerned that this is a sign that the approach I suggested does not\n> > actually work everywhere. I.e., could this be a case where we have some\n> > non-promisor object that points to a sub-object that is reachable from\n> > the promisor pack, but not a direct tip? Before your patch we'd consider\n> > that sub-object a promisor (because we enumerate all of the graph that\n> > we do have and mark each such object), but afterwards we would not.\n> > \n> > And I wonder if that confuses pack-objects. Though I think it would\n> > confuse it in the _opposite_ direction. I.e., using\n> > --exclude-promisor-objects would count such an object as not-a-promisor\n> > and would be more inclined to include it in the new pack.\n> > \n> > It is curious that this only turns up with GIT_TEST_COMMIT_GRAPH=1, too.\n> > It seems like any such problem ought to be independent of that.\n> > \n> > Puzzling...\n> \n> Do you think this justifies going back to the V1 approach (only checking\n> presence of objects pointed to by refs when doing a partial clone)?\n\nYes, I think it might. Especially coupled with your other report that\nthe V1 approach is 500ms compared to several seconds for this one. Which\nI'd guess is probably because we actually parse the ref tip objects in\nrev-list, whereas your V1 just skipped that step entirely (which is\nperfectly fine for a clone, as we'd have just hashed the objects via\nindex-pack anyway).\n\nIt might be interesting to know if the problem is indeed insurmountable\nwith the V3 approach here, or if it's simply another bug. But diving\ninto it is going to be rather tricky, and I am not volunteering to do\nit. :) So if you want to punt and go back to the more clearly correct V1\napproach, I can live with that. We can always revisit this approach\nlater (it wouldn't be necessary for the clone case after your V1, but in\ntheory it could be helping other cases, too).\n\n-Peff\n"},{"id":"374164","messageId":"20190419202652.GU60888@google.com","threadId":"50867","inReplyTo":"20190413053434.GA31884@sigill.intra.peff.net","subject":"Re: [PATCH v3] rev-list: exclude promisor objects at walk time","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-19T20:26:52Z","receivedAt":"2019-04-19T20:27:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.04.13 01:34, Jeff King wrote:\n> On Fri, Apr 12, 2019 at 03:38:47PM -0700, Josh Steadmon wrote:\n> \n> > > > Thank you for catching this. I haven't yet figured out the cause. I will\n> > > > look into this more tomorrow and then send a V4 once I've fixed it.\n> > > \n> > > I'm concerned that this is a sign that the approach I suggested does not\n> > > actually work everywhere. I.e., could this be a case where we have some\n> > > non-promisor object that points to a sub-object that is reachable from\n> > > the promisor pack, but not a direct tip? Before your patch we'd consider\n> > > that sub-object a promisor (because we enumerate all of the graph that\n> > > we do have and mark each such object), but afterwards we would not.\n> > > \n> > > And I wonder if that confuses pack-objects. Though I think it would\n> > > confuse it in the _opposite_ direction. I.e., using\n> > > --exclude-promisor-objects would count such an object as not-a-promisor\n> > > and would be more inclined to include it in the new pack.\n> > > \n> > > It is curious that this only turns up with GIT_TEST_COMMIT_GRAPH=1, too.\n> > > It seems like any such problem ought to be independent of that.\n> > > \n> > > Puzzling...\n> > \n> > Do you think this justifies going back to the V1 approach (only checking\n> > presence of objects pointed to by refs when doing a partial clone)?\n> \n> Yes, I think it might. Especially coupled with your other report that\n> the V1 approach is 500ms compared to several seconds for this one. Which\n> I'd guess is probably because we actually parse the ref tip objects in\n> rev-list, whereas your V1 just skipped that step entirely (which is\n> perfectly fine for a clone, as we'd have just hashed the objects via\n> index-pack anyway).\n> \n> It might be interesting to know if the problem is indeed insurmountable\n> with the V3 approach here, or if it's simply another bug. But diving\n> into it is going to be rather tricky, and I am not volunteering to do\n> it. :) So if you want to punt and go back to the more clearly correct V1\n> approach, I can live with that. We can always revisit this approach\n> later (it wouldn't be necessary for the clone case after your V1, but in\n> theory it could be helping other cases, too).\n> \n> -Peff\n\nI have not made any progress in figuring out the repack + commit-graph\nfailure, so I will resend V1.\n"},{"id":"374166","messageId":"b4a285e2a199ea059c165aa344d037374797fa40.1555707373.git.steadmon@google.com","threadId":"50867","inReplyTo":"6de682d5e48186970644569586fc6613763d5caa.1554312374.git.steadmon@google.com","subject":"[PATCH v4] clone: do faster object check for partial clones","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-04-19T21:00:13Z","receivedAt":"2019-04-19T21:00:19Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"For partial clones, doing a full connectivity check is wasteful; we skip\npromisor objects (which, for a partial clone, is all known objects), and\nenumerating them all to exclude them from the connectivity check can\ntake a significant amount of time on large repos.\n\nAt most, we want to make sure that we get the objects referred to by any\nwanted refs. For partial clones, just check that these objects were\ntransferred.\n\nSigned-off-by: Josh Steadmon \n---\nThis is an update of the original V1 approach (skipping the fully\nconnectivity check when doing a partial clone) with updated commit\nmessage and comments to address the review concerns.\n\n\nRange-diff against v1:\n1: 9c29e1ce8d ! 1: b4a285e2a1 clone: do faster object check for partial clones\n @@ -4,8 +4,8 @@\n \n For partial clones, doing a full connectivity check is wasteful; we skip\n promisor objects (which, for a partial clone, is all known objects), and\n - excluding them all from the connectivity check can take a significant\n - amount of time on large repos.\n + enumerating them all to exclude them from the connectivity check can\n + take a significant amount of time on large repos.\n \n At most, we want to make sure that we get the objects referred to by any\n wanted refs. For partial clones, just check that these objects were\n @@ -59,10 +59,12 @@\n \n +\tif (opt->check_refs_only) {\n +\t\t/*\n -+\t\t * For partial clones, we don't want to walk the full commit\n -+\t\t * graph because we're skipping promisor objects anyway. We\n -+\t\t * should just check that objects referenced by wanted refs were\n -+\t\t * transferred.\n ++\t\t * For partial clones, we don't want to have to do a regular\n ++\t\t * connectivity check because we have to enumerate and exclude\n ++\t\t * all promisor objects (slow), and then the connectivity check\n ++\t\t * itself becomes a no-op because in a partial clone every\n ++\t\t * object is a promisor object. Instead, just make sure we\n ++\t\t * received the objects pointed to by each wanted ref.\n +\t\t */\n +\t\tdo {\n +\t\t\tif (!repo_has_object_file(the_repository, &oid))\n @@ -86,8 +88,8 @@\n +\t/*\n +\t * If non-zero, only check the top-level objects referenced by the\n +\t * wanted refs (passed in as cb_data). This is useful for partial\n -+\t * clones, where this can be much faster than excluding all promisor\n -+\t * objects prior to walking the commit graph.\n ++\t * clones, where enumerating and excluding all promisor objects is very\n ++\t * slow and the commit-walk itself becomes a no-op.\n +\t */\n +\tunsigned check_refs_only : 1;\n };\n\n builtin/clone.c | 6 ++++--\n connected.c | 17 +++++++++++++++++\n connected.h | 8 ++++++++\n 3 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..fdbbd8942a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -657,7 +657,8 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t const char *branch_top,\n \t\t\t const char *msg,\n \t\t\t struct transport *transport,\n-\t\t\t int check_connectivity)\n+\t\t\t int check_connectivity,\n+\t\t\t int check_refs_only)\n {\n \tconst struct ref *rm = mapped_refs;\n \n@@ -666,6 +667,7 @@ static void update_remote_refs(const struct ref *refs,\n \n \t\topt.transport = transport;\n \t\topt.progress = transport->progress;\n+\t\topt.check_refs_only = !!check_refs_only;\n \n \t\tif (check_connected(iterate_ref_map, &rm, &opt))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n@@ -1224,7 +1226,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n-\t\t\t !is_local);\n+\t\t\t !is_local, filter_options.choice);\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \ndiff --git a/connected.c b/connected.c\nindex 1bba888eff..1ab481fed6 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"object-store.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n #include \"connected.h\"\n@@ -49,6 +50,22 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tstrbuf_release(&idx_file);\n \t}\n \n+\tif (opt->check_refs_only) {\n+\t\t/*\n+\t\t * For partial clones, we don't want to have to do a regular\n+\t\t * connectivity check because we have to enumerate and exclude\n+\t\t * all promisor objects (slow), and then the connectivity check\n+\t\t * itself becomes a no-op because in a partial clone every\n+\t\t * object is a promisor object. Instead, just make sure we\n+\t\t * received the objects pointed to by each wanted ref.\n+\t\t */\n+\t\tdo {\n+\t\t\tif (!repo_has_object_file(the_repository, &oid))\n+\t\t\t\treturn 1;\n+\t\t} while (!fn(cb_data, &oid));\n+\t\treturn 0;\n+\t}\n+\n \tif (opt->shallow_file) {\n \t\targv_array_push(&rev_list.args, \"--shallow-file\");\n \t\targv_array_push(&rev_list.args, opt->shallow_file);\ndiff --git a/connected.h b/connected.h\nindex 8d5a6b3ad6..ce2e7d8f2e 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -46,6 +46,14 @@ struct check_connected_options {\n \t * during a fetch.\n \t */\n \tunsigned is_deepening_fetch : 1;\n+\n+\t/*\n+\t * If non-zero, only check the top-level objects referenced by the\n+\t * wanted refs (passed in as cb_data). This is useful for partial\n+\t * clones, where enumerating and excluding all promisor objects is very\n+\t * slow and the commit-walk itself becomes a no-op.\n+\t */\n+\tunsigned check_refs_only : 1;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.21.0.593.g511ec345e18-goog\n\n"},{"id":"374272","messageId":"20190422213113.GB4728@sigill.intra.peff.net","threadId":"50867","inReplyTo":"b4a285e2a199ea059c165aa344d037374797fa40.1555707373.git.steadmon@google.com","subject":"Re: [PATCH v4] clone: do faster object check for partial clones","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-22T21:31:14Z","receivedAt":"2019-04-22T21:31:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 19, 2019 at 02:00:13PM -0700, Josh Steadmon wrote:\n\n> For partial clones, doing a full connectivity check is wasteful; we skip\n> promisor objects (which, for a partial clone, is all known objects), and\n> enumerating them all to exclude them from the connectivity check can\n> take a significant amount of time on large repos.\n> \n> At most, we want to make sure that we get the objects referred to by any\n> wanted refs. For partial clones, just check that these objects were\n> transferred.\n> \n> Signed-off-by: Josh Steadmon \n> ---\n> This is an update of the original V1 approach (skipping the fully\n> connectivity check when doing a partial clone) with updated commit\n> message and comments to address the review concerns.\n\nThis looks OK to me. Just trying to think of ways it could bite us, the\nobvious line of thinking is where \"--reference\" is used. If we tell the\nother side we already have object X, it will not be sent to us, and we\nare relying on our local non-promisor alternate to have all of the\nrequired objects. But I think this is OK:\n\n - for it to be mentioned in a ref, then the server must have been\n advertising it. Which means that it should similarly be on the hook\n for providing it to us via the promisor mechanism. That's a little\n hand-wavy, but then so is the entire promisor concept. We're\n inherently at the server's mercy, so if they're lying to us about\n what they're willing or able to provide, there's not much we can do\n anyway.\n\n - if we sent it as a \"have\" to the server, that means that our\n alternate was advertising it from a ref tip. Which means that unless\n it's corrupted, we're fine (which is no different than the\n connectivity promise we'd make for our own refs). I actually think\n that the connectivity check should \"--not\" any advertised alternate\n tips. I even have a patch to do that, but I need to polish it a\n little.\n\nSo I think this optimization will yield correct results. If we later\nfigure out a better way for rev-list to optimize this case itself, then\nwe can rip this out.\n\nI think you had dug up some numbers to put in the commit message after\nthe last discussion? Likewise, is there a t/perf test which shows this\noff (and will help us catch regressions)? If not, it might be worth\nadding one (AFAICT a simple no-blobs partial-clone would be enough).\n\n-Peff\n"}]}