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 <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan committed
Jul 2, 2018 at 15:08 UTC
cf1e7c07705eb21c30d0ee414810e7bc8fdf7d82
8 files changed
+122
-7
builtin/fetch.c
+10
-5
@@ -769,7 +769,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)
769
}
770
771
static int store_updated_refs(const char *raw_url, const char *remote_name,
772
- struct ref *ref_map)
772
+ int connectivity_checked, struct ref *ref_map)
773
{
774
FILE *fp;
775
struct commit *commit;
@@ -791,10 +791,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
791
else
792
url = xstrdup("foreign");
793
794
- rm = ref_map;
795
- if (check_connected(iterate_ref_map, &rm, NULL)) {
796
- rc = error(_("%s did not send all necessary objects\n"), url);
797
- goto abort;
794
+ if (!connectivity_checked) {
795
+ rm = ref_map;
796
+ if (check_connected(iterate_ref_map, &rm, NULL)) {
797
+ rc = error(_("%s did not send all necessary objects\n"), url);
798
+ goto abort;
799
+ }
800
}
801
802
prepare_format_display(ref_map);
@@ -966,8 +968,11 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map,
968
/* Update local refs based on the ref values fetched from a remote */
969
static int consume_refs(struct transport *transport, struct ref *ref_map)
970
{
971
+ int connectivity_checked = transport->smart_options
972
+ ? transport->smart_options->connectivity_checked : 0;
973
int ret = store_updated_refs(transport->url,
974
transport->remote->name,
975
+ connectivity_checked,
976
ref_map);
977
transport_unlock_pack(transport);
978
return ret;
connected.c
+4
-2
@@ -58,8 +58,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
58
argv_array_push(&rev_list.args, "--stdin");
59
if (repository_format_partial_clone)
60
argv_array_push(&rev_list.args, "--exclude-promisor-objects");
61
- argv_array_push(&rev_list.args, "--not");
62
- argv_array_push(&rev_list.args, "--all");
61
+ if (!opt->is_deepening_fetch) {
62
+ argv_array_push(&rev_list.args, "--not");
63
+ argv_array_push(&rev_list.args, "--all");
64
+ }
65
argv_array_push(&rev_list.args, "--quiet");
66
if (opt->progress)
67
argv_array_pushf(&rev_list.args, "--progress=%s",
connected.h
+7
@@ -38,6 +38,13 @@ struct check_connected_options {
38
* Insert these variables into the environment of the child process.
39
*/
40
const char **env;
41
+
42
+ /*
43
+ * If non-zero, check the ancestry chain completely, not stopping at
44
+ * any existing ref. This is necessary when deepening existing refs
45
+ * during a fetch.
46
+ */
47
+ unsigned is_deepening_fetch : 1;
48
};
49
50
#define CHECK_CONNECTED_INIT { 0 }
fetch-pack.c
+31
@@ -19,6 +19,7 @@
19
#include "sha1-array.h"
20
#include "oidset.h"
21
#include "packfile.h"
22
+#include "connected.h"
23
24
static int transfer_unpack_limit = -1;
25
static int fetch_unpack_limit = -1;
@@ -1596,6 +1597,18 @@ static void update_shallow(struct fetch_pack_args *args,
1597
oid_array_clear(&ref);
1598
}
1599
1600
+static int iterate_ref_map(void *cb_data, struct object_id *oid)
1601
+{
1602
+ struct ref **rm = cb_data;
1603
+ struct ref *ref = *rm;
1604
+
1605
+ if (!ref)
1606
+ return -1; /* end of the list */
1607
+ *rm = ref->next;
1608
+ oidcpy(oid, &ref->old_oid);
1609
+ return 0;
1610
+}
1611
+
1612
struct ref *fetch_pack(struct fetch_pack_args *args,
1613
int fd[], struct child_process *conn,
1614
const struct ref *ref,
@@ -1624,7 +1637,25 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
1637
ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
1638
&si, pack_lockfile);
1639
reprepare_packed_git(the_repository);
1640
+
1641
+ if (!args->cloning && args->deepen) {
1642
+ struct check_connected_options opt = CHECK_CONNECTED_INIT;
1643
+ struct ref *iterator = ref_cpy;
1644
+ opt.shallow_file = alternate_shallow_file;
1645
+ if (args->deepen)
1646
+ opt.is_deepening_fetch = 1;
1647
+ if (check_connected(iterate_ref_map, &iterator, &opt)) {
1648
+ error(_("remote did not send all necessary objects"));
1649
+ free_refs(ref_cpy);
1650
+ ref_cpy = NULL;
1651
+ rollback_lock_file(&shallow_lock);
1652
+ goto cleanup;
1653
+ }
1654
+ args->connectivity_checked = 1;
1655
+ }
1656
+
1657
update_shallow(args, ref_cpy, &si);
1658
+cleanup:
1659
clear_shallow_info(&si);
1660
return ref_cpy;
1661
}
fetch-pack.h
+15
@@ -41,6 +41,21 @@ struct fetch_pack_args {
41
* regardless of which object flags it uses (if any).
42
*/
43
unsigned no_dependents:1;
44
+
45
+ /*
46
+ * Because fetch_pack() overwrites the shallow file upon a
47
+ * successful deepening non-clone fetch, if this struct
48
+ * specifies such a fetch, fetch_pack() needs to perform a
49
+ * connectivity check before deciding if a fetch is successful
50
+ * (and overwriting the shallow file). fetch_pack() sets this
51
+ * field to 1 if such a connectivity check was performed.
52
+ *
53
+ * This is different from check_self_contained_and_connected
54
+ * in that the former allows existing objects in the
55
+ * repository to satisfy connectivity needs, whereas the
56
+ * latter doesn't.
57
+ */
58
+ unsigned connectivity_checked:1;
59
};
60
61
/*
t/t5537-fetch-shallow.sh
+43
@@ -186,4 +186,47 @@ EOF
186
test_cmp expect actual
187
'
188
189
+. "$TEST_DIRECTORY"/lib-httpd.sh
190
+start_httpd
191
+
192
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
193
+
194
+test_expect_success 'shallow fetches check connectivity before writing shallow file' '
195
+ rm -rf "$REPO" client &&
196
+
197
+ git init "$REPO" &&
198
+ test_commit -C "$REPO" one &&
199
+ test_commit -C "$REPO" two &&
200
+ test_commit -C "$REPO" three &&
201
+
202
+ git init client &&
203
+
204
+ # Use protocol v2 to ensure that shallow information is sent exactly
205
+ # once by the server, since we are planning to manipulate it.
206
+ git -C "$REPO" config protocol.version 2 &&
207
+ git -C client config protocol.version 2 &&
208
+
209
+ git -C client fetch --depth=2 "$HTTPD_URL/one_time_sed/repo" master:a_branch &&
210
+
211
+ # Craft a situation in which the server sends back an unshallow request
212
+ # with an empty packfile. This is done by refetching with a shorter
213
+ # depth (to ensure that the packfile is empty), and overwriting the
214
+ # shallow line in the response with the unshallow line we want.
215
+ printf "s/0034shallow %s/0036unshallow %s/" \
216
+ "$(git -C "$REPO" rev-parse HEAD)" \
217
+ "$(git -C "$REPO" rev-parse HEAD^)" \
218
+ >"$HTTPD_ROOT_PATH/one-time-sed" &&
219
+ test_must_fail git -C client fetch --depth=1 "$HTTPD_URL/one_time_sed/repo" \
220
+ master:a_branch &&
221
+
222
+ # Ensure that the one-time-sed script was used.
223
+ ! test -e "$HTTPD_ROOT_PATH/one-time-sed" &&
224
+
225
+ # Ensure that the resulting repo is consistent, despite our failure to
226
+ # fetch.
227
+ git -C client fsck
228
+'
229
+
230
+stop_httpd
231
+
232
test_done
transport.c
+1
@@ -350,6 +350,7 @@ static int fetch_refs_via_pack(struct transport *transport,
350
data->got_remote_heads = 0;
351
data->options.self_contained_and_connected =
352
args.self_contained_and_connected;
353
+ data->options.connectivity_checked = args.connectivity_checked;
354
355
if (refs == NULL)
356
ret = -1;
transport.h
+11
@@ -18,6 +18,17 @@ struct git_transport_options {
18
unsigned deepen_relative : 1;
19
unsigned from_promisor : 1;
20
unsigned no_dependents : 1;
21
+
22
+ /*
23
+ * If this transport supports connect or stateless-connect,
24
+ * the corresponding field in struct fetch_pack_args is copied
25
+ * here after fetching.
26
+ *
27
+ * See the definition of connectivity_checked in struct
28
+ * fetch_pack_args for more information.
29
+ */
30
+ unsigned connectivity_checked:1;
31
+
32
int depth;
33
const char *deepen_since;
34
const struct string_list *deepen_not;