Re: [PATCH] connected: always use partial clone optimization
From: Junio C Hamano <hidden>
Date: 2020-03-20 22:54:46
Jonathan Tan [off-list ref] writes:
The addition of the fast path might cause performance reductions in these cases: - If a partial clone or a fetch into a partial clone fails, Git will fruitlessly run rev-list (it is expected that everything fetched would go into promisor packs, so if that didn't happen, it is most likely that rev-list will fail too).
I agree that it is reasonable not to optimize the system for the failure case---it is pointless to fail as quick as possible ;-)
- Any connectivity checks done by receive-pack, in the (in my opinion, unlikely) event that a partial clone serves receive-pack.
Meaning "I created this repository by partially cloning, and now I want to update it by pushing into it"? I am not sure how rare such a use case is, so I won't be a good judge on this one.
quoted hunk
diff --git a/builtin/clone.c b/builtin/clone.c index 1ad26f4d8c..4b2b14ff61 100644 --- a/builtin/clone.c +++ b/builtin/clone.c@@ -672,8 +672,7 @@ static void update_remote_refs(const struct ref *refs, const char *branch_top, const char *msg, struct transport *transport, - int check_connectivity, - int check_refs_are_promisor_objects_only) + int check_connectivity) { const struct ref *rm = mapped_refs;@@ -682,8 +681,6 @@ static void update_remote_refs(const struct ref *refs, opt.transport = transport; opt.progress = transport->progress; - opt.check_refs_are_promisor_objects_only = - !!check_refs_are_promisor_objects_only; if (check_connected(iterate_ref_map, &rm, &opt)) die(_("remote did not send all necessary objects"));@@ -1275,7 +1272,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) update_remote_refs(refs, mapped_refs, remote_head_points_at, branch_top.buf, reflog_msg.buf, transport, - !is_local, filter_options.choice); + !is_local); update_head(our_head_points_at, remote_head, reflog_msg.buf);diff --git a/builtin/fetch.c b/builtin/fetch.c index bf6bab80fa..1097e1e512 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -908,13 +908,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, if (!connectivity_checked) { struct check_connected_options opt = CHECK_CONNECTED_INIT; - if (filter_options.choice) - /* - * Since a filter is specified, objects indirectly - * referenced by refs are allowed to be absent. - */ - opt.check_refs_are_promisor_objects_only = 1; - rm = ref_map; if (check_connected(iterate_ref_map, &rm, &opt)) { rc = error(_("%s did not send all necessary objects\n"), url);diff --git a/connected.c b/connected.c index 7e9bd1bc62..846f2e4eef 100644 --- a/connected.c +++ b/connected.c@@ -52,7 +52,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data, strbuf_release(&idx_file); } - if (opt->check_refs_are_promisor_objects_only) { + if (has_promisor_remote()) {
Earlier we would have this bit on only when filter_options.choice was non-NULL but this allows us to take the branch as long as we are lazily populated. Makes sense.
quoted hunk
@@ -71,13 +71,18 @@ int check_connected(oid_iterate_fn fn, void *cb_data, if (find_pack_entry_one(oid.hash, p)) goto promisor_pack_found; } - return 1; + /* + * Fallback to rev-list with oid and the rest of the + * object IDs provided by fn. + */ + goto no_promisor_pack_found;
OK. This is the "fallback" thing you mentioned in the log message. Makes sense.
promisor_pack_found:
;
} while (!fn(cb_data, &oid));
return 0;
}
+no_promisor_pack_found:
if (opt->shallow_file) {
argv_array_push(&rev_list.args, "--shallow-file");
argv_array_push(&rev_list.args, opt->shallow_file);