From 989b8c4452f63f415c276df348defce6df613696 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 27 Jun 2018 15:30:22 -0700 Subject: fetch-pack: put shallow info in output parameter Expand the transport fetch method signature, by adding an output parameter, to allow transports to return information about the refs they have fetched. Then communicate shallow status information through this mechanism instead of by modifying the input list of refs. This does require clients to sometimes generate the ref map twice: once from the list of refs provided by the remote (as is currently done) and potentially once from the new list of refs that the fetch mechanism provides. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano --- fetch-pack.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'fetch-pack.c') diff --git a/fetch-pack.c b/fetch-pack.c index a320ce9872..73890b8943 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1470,12 +1470,13 @@ static int remove_duplicates_in_refs(struct ref **ref, int nr) } static void update_shallow(struct fetch_pack_args *args, - struct ref **sought, int nr_sought, + struct ref *refs, struct shallow_info *si) { struct oid_array ref = OID_ARRAY_INIT; int *status; int i; + struct ref *r; if (args->deepen && alternate_shallow_file) { if (*alternate_shallow_file == '\0') { /* --unshallow */ @@ -1517,8 +1518,8 @@ static void update_shallow(struct fetch_pack_args *args, remove_nonexistent_theirs_shallow(si); if (!si->nr_ours && !si->nr_theirs) return; - for (i = 0; i < nr_sought; i++) - oid_array_append(&ref, &sought[i]->old_oid); + for (r = refs; r; r = r->next) + oid_array_append(&ref, &r->old_oid); si->ref = &ref; if (args->update_shallow) { @@ -1552,12 +1553,12 @@ static void update_shallow(struct fetch_pack_args *args, * remote is also shallow, check what ref is safe to update * without updating .git/shallow */ - status = xcalloc(nr_sought, sizeof(*status)); + status = xcalloc(ref.nr, sizeof(*status)); assign_shallow_commits_to_refs(si, NULL, status); if (si->nr_ours || si->nr_theirs) { - for (i = 0; i < nr_sought; i++) + for (r = refs, i = 0; r; r = r->next, i++) if (status[i]) - sought[i]->status = REF_STATUS_REJECT_SHALLOW; + r->status = REF_STATUS_REJECT_SHALLOW; } free(status); oid_array_clear(&ref); @@ -1591,7 +1592,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, &si, pack_lockfile); reprepare_packed_git(the_repository); - update_shallow(args, sought, nr_sought, &si); + update_shallow(args, ref_cpy, &si); clear_shallow_info(&si); return ref_cpy; } -- cgit 1.2.3-korg From 733020517a1baa6f4f76bb7bf48d8d8d14eecd6c Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Wed, 27 Jun 2018 15:30:23 -0700 Subject: fetch-pack: implement ref-in-want Implement ref-in-want on the client side so that when a server supports the "ref-in-want" feature, a client will send "want-ref" lines for each reference the client wants to fetch. This feature allows clients to tolerate inconsistencies that exist when a remote repository's refs change during the course of negotiation. This allows a client to request to request a particular ref without specifying the OID of the ref. This means that instead of hitting an error when a ref no longer points at the OID it did at the beginning of negotiation, negotiation can continue and the value of that ref will be sent at the termination of negotiation, just before a packfile is sent. More information on the ref-in-want feature can be found in Documentation/technical/protocol-v2.txt. Signed-off-by: Brandon Williams Signed-off-by: Junio C Hamano --- fetch-pack.c | 38 +++++++++- remote.c | 1 + remote.h | 1 + t/t5703-upload-pack-ref-in-want.sh | 149 +++++++++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 3 deletions(-) (limited to 'fetch-pack.c') diff --git a/fetch-pack.c b/fetch-pack.c index 73890b8943..0b4a9f288f 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf, static void add_wants(const struct ref *wants, struct strbuf *req_buf) { + int use_ref_in_want = server_supports_feature("fetch", "ref-in-want", 0); + for ( ; wants ; wants = wants->next) { const struct object_id *remote = &wants->old_oid; - const char *remote_hex; struct object *o; /* @@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct strbuf *req_buf) continue; } - remote_hex = oid_to_hex(remote); - packet_buf_write(req_buf, "want %s\n", remote_hex); + if (!use_ref_in_want || wants->exact_oid) + packet_buf_write(req_buf, "want %s\n", oid_to_hex(remote)); + else + packet_buf_write(req_buf, "want-ref %s\n", wants->name); } } @@ -1334,6 +1337,32 @@ static void receive_shallow_info(struct fetch_pack_args *args, args->deepen = 1; } +static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs) +{ + process_section_header(reader, "wanted-refs", 0); + while (packet_reader_read(reader) == PACKET_READ_NORMAL) { + struct object_id oid; + const char *end; + struct ref *r = NULL; + + if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ') + die("expected wanted-ref, got '%s'", reader->line); + + for (r = refs; r; r = r->next) { + if (!strcmp(end, r->name)) { + oidcpy(&r->old_oid, &oid); + break; + } + } + + if (!r) + die("unexpected wanted-ref: '%s'", reader->line); + } + + if (reader->status != PACKET_READ_DELIM) + die("error processing wanted refs: %d", reader->status); +} + enum fetch_state { FETCH_CHECK_LOCAL = 0, FETCH_SEND_REQUEST, @@ -1408,6 +1437,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, if (process_section_header(&reader, "shallow-info", 1)) receive_shallow_info(args, &reader); + if (process_section_header(&reader, "wanted-refs", 1)) + receive_wanted_refs(&reader, ref); + /* get the pack */ process_section_header(&reader, "packfile", 0); if (get_pack(args, fd, pack_lockfile)) diff --git a/remote.c b/remote.c index abe80c1397..2c2376fff5 100644 --- a/remote.c +++ b/remote.c @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs, if (refspec->exact_sha1) { ref_map = alloc_ref(name); get_oid_hex(name, &ref_map->old_oid); + ref_map->exact_oid = 1; } else { ref_map = get_remote_ref(remote_refs, name); } diff --git a/remote.h b/remote.h index 45ecc6cefa..976292152c 100644 --- a/remote.h +++ b/remote.h @@ -73,6 +73,7 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, + exact_oid:1, deletion:1; enum { diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh index 32527a59c4..a73c55a47e 100755 --- a/t/t5703-upload-pack-ref-in-want.sh +++ b/t/t5703-upload-pack-ref-in-want.sh @@ -211,6 +211,18 @@ test_expect_success 'server is initially ahead - no ref in want' ' grep "ERR upload-pack: not our ref" err ' +test_expect_success 'server is initially ahead - ref in want' ' + git -C "$REPO" config uploadpack.allowRefInWant true && + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + inconsistency master 1234567890123456789012345678901234567890 && + git -C local fetch && + + git -C "$REPO" rev-parse --verify master >expected && + git -C local rev-parse --verify refs/remotes/origin/master >actual && + test_cmp expected actual +' + test_expect_success 'server is initially behind - no ref in want' ' git -C "$REPO" config uploadpack.allowRefInWant false && rm -rf local && @@ -223,6 +235,143 @@ test_expect_success 'server is initially behind - no ref in want' ' test_cmp expected actual ' +test_expect_success 'server is initially behind - ref in want' ' + git -C "$REPO" config uploadpack.allowRefInWant true && + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + inconsistency master "master^" && + git -C local fetch && + + git -C "$REPO" rev-parse --verify "master" >expected && + git -C local rev-parse --verify refs/remotes/origin/master >actual && + test_cmp expected actual +' + +test_expect_success 'server loses a ref - ref in want' ' + git -C "$REPO" config uploadpack.allowRefInWant true && + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" && + test_must_fail git -C local fetch 2>err && + + grep "ERR unknown ref refs/heads/raster" err +' + stop_httpd +REPO="$(pwd)/repo" +LOCAL_PRISTINE="$(pwd)/local_pristine" + +# $REPO +# c(o/foo) d(o/bar) +# \ / +# b e(baz) f(master) +# \__ | __/ +# \ | / +# a +# +# $LOCAL_PRISTINE +# s32(side) +# | +# . +# . +# | +# a(master) +test_expect_success 'setup repos for fetching with ref-in-want tests' ' + ( + git init "$REPO" && + cd "$REPO" && + test_commit a && + + # Local repo with many commits (so that negotiation will take + # more than 1 request/response pair) + rm -rf "$LOCAL_PRISTINE" && + git clone "file://$REPO" "$LOCAL_PRISTINE" && + cd "$LOCAL_PRISTINE" && + git checkout -b side && + for i in $(seq 1 33); do test_commit s$i; done && + + # Add novel commits to upstream + git checkout master && + cd "$REPO" && + git checkout -b o/foo && + test_commit b && + test_commit c && + git checkout -b o/bar b && + test_commit d && + git checkout -b baz a && + test_commit e && + git checkout master && + test_commit f + ) && + git -C "$REPO" config uploadpack.allowRefInWant true && + git -C "$LOCAL_PRISTINE" config protocol.version 2 +' + +test_expect_success 'fetching with exact OID' ' + test_when_finished "rm -f log" && + + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch origin \ + $(git -C "$REPO" rev-parse d):refs/heads/actual && + + git -C "$REPO" rev-parse "d" >expected && + git -C local rev-parse refs/heads/actual >actual && + test_cmp expected actual && + grep "want $(git -C "$REPO" rev-parse d)" log +' + +test_expect_success 'fetching multiple refs' ' + test_when_finished "rm -f log" && + + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch origin master baz && + + git -C "$REPO" rev-parse "master" "baz" >expected && + git -C local rev-parse refs/remotes/origin/master refs/remotes/origin/baz >actual && + test_cmp expected actual && + grep "want-ref refs/heads/master" log && + grep "want-ref refs/heads/baz" log +' + +test_expect_success 'fetching ref and exact OID' ' + test_when_finished "rm -f log" && + + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch origin \ + master $(git -C "$REPO" rev-parse b):refs/heads/actual && + + git -C "$REPO" rev-parse "master" "b" >expected && + git -C local rev-parse refs/remotes/origin/master refs/heads/actual >actual && + test_cmp expected actual && + grep "want $(git -C "$REPO" rev-parse b)" log && + grep "want-ref refs/heads/master" log +' + +test_expect_success 'fetching with wildcard that does not match any refs' ' + test_when_finished "rm -f log" && + + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + git -C local fetch origin refs/heads/none*:refs/heads/* >out && + test_must_be_empty out +' + +test_expect_success 'fetching with wildcard that matches multiple refs' ' + test_when_finished "rm -f log" && + + rm -rf local && + cp -r "$LOCAL_PRISTINE" local && + GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch origin refs/heads/o*:refs/heads/o* && + + git -C "$REPO" rev-parse "o/foo" "o/bar" >expected && + git -C local rev-parse "o/foo" "o/bar" >actual && + test_cmp expected actual && + grep "want-ref refs/heads/o/foo" log && + grep "want-ref refs/heads/o/bar" log +' + test_done -- cgit 1.2.3-korg From cf1e7c07705eb21c30d0ee414810e7bc8fdf7d82 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Mon, 2 Jul 2018 15:08:43 -0700 Subject: fetch-pack: write shallow, then check connectivity When fetching, connectivity is checked after the shallow file is updated. There are 2 issues with this: (1) the connectivity check is only performed up to ancestors of existing refs (which is not thorough enough if we were deepening an existing ref in the first place), and (2) there is no rollback of the shallow file if the connectivity check fails. To solve (1), update the connectivity check to check the ancestry chain completely in the case of a deepening fetch by refraining from passing "--not --all" when invoking rev-list in connected.c. To solve (2), have fetch_pack() perform its own connectivity check before updating the shallow file. To support existing use cases in which "git fetch-pack" is used to download objects without much regard as to the connectivity of the resulting objects with respect to the existing repository, the connectivity check is only done if necessary (that is, the fetch is not a clone, and the fetch involves shallow/deepen functionality). "git fetch" still performs its own connectivity check, preserving correctness but sometimes performing redundant work. This redundancy is mitigated by the fact that fetch_pack() reports if it has performed a connectivity check itself, and if the transport supports connect or stateless-connect, it will bubble up that report so that "git fetch" knows not to perform the connectivity check in such a case. This was noticed when a user tried to deepen an existing repository by fetching with --no-shallow from a server that did not send all necessary objects - the connectivity check as run by "git fetch" succeeded, but a subsequent "git fsck" failed. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/fetch.c | 15 ++++++++++----- connected.c | 6 ++++-- connected.h | 7 +++++++ fetch-pack.c | 31 +++++++++++++++++++++++++++++++ fetch-pack.h | 15 +++++++++++++++ t/t5537-fetch-shallow.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ transport.c | 1 + transport.h | 11 +++++++++++ 8 files changed, 122 insertions(+), 7 deletions(-) (limited to 'fetch-pack.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index 0347cf0167..d4b2767d48 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -769,7 +769,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid) } static int store_updated_refs(const char *raw_url, const char *remote_name, - struct ref *ref_map) + int connectivity_checked, struct ref *ref_map) { FILE *fp; struct commit *commit; @@ -791,10 +791,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, else url = xstrdup("foreign"); - rm = ref_map; - if (check_connected(iterate_ref_map, &rm, NULL)) { - rc = error(_("%s did not send all necessary objects\n"), url); - goto abort; + if (!connectivity_checked) { + rm = ref_map; + if (check_connected(iterate_ref_map, &rm, NULL)) { + rc = error(_("%s did not send all necessary objects\n"), url); + goto abort; + } } prepare_format_display(ref_map); @@ -966,8 +968,11 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map, /* Update local refs based on the ref values fetched from a remote */ static int consume_refs(struct transport *transport, struct ref *ref_map) { + int connectivity_checked = transport->smart_options + ? transport->smart_options->connectivity_checked : 0; int ret = store_updated_refs(transport->url, transport->remote->name, + connectivity_checked, ref_map); transport_unlock_pack(transport); return ret; diff --git a/connected.c b/connected.c index 91feb78815..1bba888eff 100644 --- a/connected.c +++ b/connected.c @@ -58,8 +58,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data, argv_array_push(&rev_list.args, "--stdin"); if (repository_format_partial_clone) argv_array_push(&rev_list.args, "--exclude-promisor-objects"); - argv_array_push(&rev_list.args, "--not"); - argv_array_push(&rev_list.args, "--all"); + if (!opt->is_deepening_fetch) { + argv_array_push(&rev_list.args, "--not"); + argv_array_push(&rev_list.args, "--all"); + } argv_array_push(&rev_list.args, "--quiet"); if (opt->progress) argv_array_pushf(&rev_list.args, "--progress=%s", diff --git a/connected.h b/connected.h index a53f03a61a..322dc76372 100644 --- a/connected.h +++ b/connected.h @@ -38,6 +38,13 @@ struct check_connected_options { * Insert these variables into the environment of the child process. */ const char **env; + + /* + * If non-zero, check the ancestry chain completely, not stopping at + * any existing ref. This is necessary when deepening existing refs + * during a fetch. + */ + unsigned is_deepening_fetch : 1; }; #define CHECK_CONNECTED_INIT { 0 } diff --git a/fetch-pack.c b/fetch-pack.c index 0b4a9f288f..60bbffb976 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -19,6 +19,7 @@ #include "sha1-array.h" #include "oidset.h" #include "packfile.h" +#include "connected.h" static int transfer_unpack_limit = -1; static int fetch_unpack_limit = -1; @@ -1596,6 +1597,18 @@ static void update_shallow(struct fetch_pack_args *args, oid_array_clear(&ref); } +static int iterate_ref_map(void *cb_data, struct object_id *oid) +{ + struct ref **rm = cb_data; + struct ref *ref = *rm; + + if (!ref) + return -1; /* end of the list */ + *rm = ref->next; + oidcpy(oid, &ref->old_oid); + return 0; +} + struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, @@ -1624,7 +1637,25 @@ struct ref *fetch_pack(struct fetch_pack_args *args, ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, &si, pack_lockfile); reprepare_packed_git(the_repository); + + if (!args->cloning && args->deepen) { + struct check_connected_options opt = CHECK_CONNECTED_INIT; + struct ref *iterator = ref_cpy; + opt.shallow_file = alternate_shallow_file; + if (args->deepen) + opt.is_deepening_fetch = 1; + if (check_connected(iterate_ref_map, &iterator, &opt)) { + error(_("remote did not send all necessary objects")); + free_refs(ref_cpy); + ref_cpy = NULL; + rollback_lock_file(&shallow_lock); + goto cleanup; + } + args->connectivity_checked = 1; + } + update_shallow(args, ref_cpy, &si); +cleanup: clear_shallow_info(&si); return ref_cpy; } diff --git a/fetch-pack.h b/fetch-pack.h index bb45a366a8..2160be9164 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -41,6 +41,21 @@ struct fetch_pack_args { * regardless of which object flags it uses (if any). */ unsigned no_dependents:1; + + /* + * Because fetch_pack() overwrites the shallow file upon a + * successful deepening non-clone fetch, if this struct + * specifies such a fetch, fetch_pack() needs to perform a + * connectivity check before deciding if a fetch is successful + * (and overwriting the shallow file). fetch_pack() sets this + * field to 1 if such a connectivity check was performed. + * + * This is different from check_self_contained_and_connected + * in that the former allows existing objects in the + * repository to satisfy connectivity needs, whereas the + * latter doesn't. + */ + unsigned connectivity_checked:1; }; /* diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index df8d2f095a..a7afb66049 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -186,4 +186,47 @@ EOF test_cmp expect actual ' +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd + +REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo" + +test_expect_success 'shallow fetches check connectivity before writing shallow file' ' + rm -rf "$REPO" client && + + git init "$REPO" && + test_commit -C "$REPO" one && + test_commit -C "$REPO" two && + test_commit -C "$REPO" three && + + git init client && + + # Use protocol v2 to ensure that shallow information is sent exactly + # once by the server, since we are planning to manipulate it. + git -C "$REPO" config protocol.version 2 && + git -C client config protocol.version 2 && + + git -C client fetch --depth=2 "$HTTPD_URL/one_time_sed/repo" master:a_branch && + + # Craft a situation in which the server sends back an unshallow request + # with an empty packfile. This is done by refetching with a shorter + # depth (to ensure that the packfile is empty), and overwriting the + # shallow line in the response with the unshallow line we want. + printf "s/0034shallow %s/0036unshallow %s/" \ + "$(git -C "$REPO" rev-parse HEAD)" \ + "$(git -C "$REPO" rev-parse HEAD^)" \ + >"$HTTPD_ROOT_PATH/one-time-sed" && + test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \ + master:a_branch && + + # Ensure that the one-time-sed script was used. + ! test -e "$HTTPD_ROOT_PATH/one-time-sed" && + + # Ensure that the resulting repo is consistent, despite our failure to + # fetch. + git -C client fsck +' + +stop_httpd + test_done diff --git a/transport.c b/transport.c index 39d8c2fa55..fdd813f684 100644 --- a/transport.c +++ b/transport.c @@ -350,6 +350,7 @@ static int fetch_refs_via_pack(struct transport *transport, data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; + data->options.connectivity_checked = args.connectivity_checked; if (refs == NULL) ret = -1; diff --git a/transport.h b/transport.h index 3dff767a87..7a9a7fcaf3 100644 --- a/transport.h +++ b/transport.h @@ -18,6 +18,17 @@ struct git_transport_options { unsigned deepen_relative : 1; unsigned from_promisor : 1; unsigned no_dependents : 1; + + /* + * If this transport supports connect or stateless-connect, + * the corresponding field in struct fetch_pack_args is copied + * here after fetching. + * + * See the definition of connectivity_checked in struct + * fetch_pack_args for more information. + */ + unsigned connectivity_checked:1; + int depth; const char *deepen_since; const struct string_list *deepen_not; -- cgit 1.2.3-korg