Skip to content

Commit 125e389

Browse files
gitstergitster
committed
Merge branch 'xx/bundie-uri-fixes'
When bundleURI interface fetches multiple bundles, Git failed to take full advantage of all bundles and ended up slurping duplicated objects. * xx/bundie-uri-fixes: unbundle: extend object verification for fetches fetch-pack: expose fsckObjects configuration logic bundle-uri: verify oid before writing refs
2 parents 3997614 + 63d903f commit 125e389

File tree

8 files changed

+243
-14
lines changed

8 files changed

+243
-14
lines changed

‎bundle-uri.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "hashmap.h"
1212
#include "pkt-line.h"
1313
#include "config.h"
14+
#include "fetch-pack.h"
1415
#include "remote.h"
1516

1617
static struct {
@@ -375,7 +376,7 @@ static int unbundle_from_file(struct repository *r, const char *file)
375376
* the prerequisite commits.
376377
*/
377378
if ((result = unbundle(r, &header, bundle_fd, NULL,
378-
VERIFY_BUNDLE_QUIET)))
379+
VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))
379380
return 1;
380381

381382
/*
@@ -402,8 +403,7 @@ static int unbundle_from_file(struct repository *r, const char *file)
402403
refs_update_ref(get_main_ref_store(the_repository),
403404
"fetched bundle", bundle_ref.buf, oid,
404405
has_old ? &old_oid : NULL,
405-
REF_SKIP_OID_VERIFICATION,
406-
UPDATE_REFS_MSG_ON_ERR);
406+
0, UPDATE_REFS_MSG_ON_ERR);
407407
}
408408

409409
bundle_header_release(&header);

‎bundle.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,9 @@ int unbundle(struct repository *r, struct bundle_header *header,
636636
if (header->filter.choice)
637637
strvec_push(&ip.args, "--promisor=from-bundle");
638638

639+
if (flags & VERIFY_BUNDLE_FSCK)
640+
strvec_push(&ip.args, "--fsck-objects");
641+
639642
if (extra_index_pack_args) {
640643
strvec_pushv(&ip.args, extra_index_pack_args->v);
641644
strvec_clear(extra_index_pack_args);

‎bundle.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ int create_bundle(struct repository *r, const char *path,
3333
enum verify_bundle_flags {
3434
VERIFY_BUNDLE_VERBOSE = (1 << 0),
3535
VERIFY_BUNDLE_QUIET = (1 << 1),
36+
VERIFY_BUNDLE_FSCK = (1 << 2),
3637
};
3738

3839
int verify_bundle(struct repository *r, struct bundle_header *header,

‎fetch-pack.c

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -956,12 +956,7 @@ static int get_pack(struct fetch_pack_args *args,
956956
strvec_push(&cmd.args, alternate_shallow_file);
957957
}
958958

959-
if (fetch_fsck_objects >= 0
960-
? fetch_fsck_objects
961-
: transfer_fsck_objects >= 0
962-
? transfer_fsck_objects
963-
: 0)
964-
fsck_objects = 1;
959+
fsck_objects = fetch_pack_fsck_objects();
965960

966961
if (do_keep || args->from_promisor || index_pack_args || fsck_objects) {
967962
if (pack_lockfiles || fsck_objects)
@@ -2050,6 +2045,16 @@ static const struct object_id *iterate_ref_map(void *cb_data)
20502045
return &ref->old_oid;
20512046
}
20522047

2048+
int fetch_pack_fsck_objects(void)
2049+
{
2050+
fetch_pack_setup();
2051+
if (fetch_fsck_objects >= 0)
2052+
return fetch_fsck_objects;
2053+
if (transfer_fsck_objects >= 0)
2054+
return transfer_fsck_objects;
2055+
return 0;
2056+
}
2057+
20532058
struct ref *fetch_pack(struct fetch_pack_args *args,
20542059
int fd[],
20552060
const struct ref *ref,

‎fetch-pack.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,4 +101,9 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
101101
*/
102102
int report_unmatched_refs(struct ref **sought, int nr_sought);
103103

104+
/*
105+
* Return true if checks for broken objects in received pack are required.
106+
*/
107+
int fetch_pack_fsck_objects(void);
108+
104109
#endif

‎t/t5558-clone-bundle-uri.sh

Lines changed: 183 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
test_description='test fetching bundles with --bundle-uri'
44

55
. ./test-lib.sh
6+
. "$TEST_DIRECTORY"/lib-bundle.sh
67

78
test_expect_success 'fail to clone from non-existent file' '
89
test_when_finished rm -rf test &&
@@ -19,10 +20,39 @@ test_expect_success 'fail to clone from non-bundle file' '
1920

2021
test_expect_success 'create bundle' '
2122
git init clone-from &&
22-
git -C clone-from checkout -b topic &&
23-
test_commit -C clone-from A &&
24-
test_commit -C clone-from B &&
25-
git -C clone-from bundle create B.bundle topic
23+
(
24+
cd clone-from &&
25+
git checkout -b topic &&
26+
27+
test_commit A &&
28+
git bundle create A.bundle topic &&
29+
30+
test_commit B &&
31+
git bundle create B.bundle topic &&
32+
33+
# Create a bundle with reference pointing to non-existent object.
34+
commit_a=$(git rev-parse A) &&
35+
commit_b=$(git rev-parse B) &&
36+
sed -e "/^$/q" -e "s/$commit_a /$commit_b /" \
37+
<A.bundle >bad-header.bundle &&
38+
convert_bundle_to_pack \
39+
<A.bundle >>bad-header.bundle &&
40+
41+
tree_b=$(git rev-parse B^{tree}) &&
42+
cat >data <<-EOF &&
43+
tree $tree_b
44+
parent $commit_b
45+
author A U Thor
46+
committer A U Thor
47+
48+
commit: this is a commit with bad emails
49+
50+
EOF
51+
bad_commit=$(git hash-object --literally -t commit -w --stdin <data) &&
52+
git branch bad $bad_commit &&
53+
git bundle create bad-object.bundle bad &&
54+
git update-ref -d refs/heads/bad
55+
)
2656
'
2757

2858
test_expect_success 'clone with path bundle' '
@@ -33,6 +63,33 @@ test_expect_success 'clone with path bundle' '
3363
test_cmp expect actual
3464
'
3565

66+
test_expect_success 'clone with bundle that has bad header' '
67+
# Write bundle ref fails, but clone can still proceed.
68+
git clone --bundle-uri="clone-from/bad-header.bundle" \
69+
clone-from clone-bad-header 2>err &&
70+
commit_b=$(git -C clone-from rev-parse B) &&
71+
test_grep "trying to write ref '\''refs/bundles/topic'\'' with nonexistent object $commit_b" err &&
72+
git -C clone-bad-header for-each-ref --format="%(refname)" >refs &&
73+
test_grep ! "refs/bundles/" refs
74+
'
75+
76+
test_expect_success 'clone with bundle that has bad object' '
77+
# Unbundle succeeds if no fsckObjects configured.
78+
git clone --bundle-uri="clone-from/bad-object.bundle" \
79+
clone-from clone-bad-object-no-fsck &&
80+
git -C clone-bad-object-no-fsck for-each-ref --format="%(refname)" >refs &&
81+
grep "refs/bundles/" refs >actual &&
82+
test_write_lines refs/bundles/bad >expect &&
83+
test_cmp expect actual &&
84+
85+
# Unbundle fails with fsckObjects set true, but clone can still proceed.
86+
git -c fetch.fsckObjects=true clone --bundle-uri="clone-from/bad-object.bundle" \
87+
clone-from clone-bad-object-fsck 2>err &&
88+
test_grep "missingEmail" err &&
89+
git -C clone-bad-object-fsck for-each-ref --format="%(refname)" >refs &&
90+
test_grep ! "refs/bundles/" refs
91+
'
92+
3693
test_expect_success 'clone with path bundle and non-default hash' '
3794
test_when_finished "rm -rf clone-path-non-default-hash" &&
3895
GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="clone-from/B.bundle" \
@@ -259,6 +316,128 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '
259316
! grep "refs/bundles/" refs
260317
'
261318

319+
test_expect_success 'negotiation: bundle with part of wanted commits' '
320+
test_when_finished "rm -f trace*.txt" &&
321+
GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
322+
git clone --no-local --bundle-uri="clone-from/A.bundle" \
323+
clone-from nego-bundle-part &&
324+
git -C nego-bundle-part for-each-ref --format="%(refname)" >refs &&
325+
grep "refs/bundles/" refs >actual &&
326+
test_write_lines refs/bundles/topic >expect &&
327+
test_cmp expect actual &&
328+
# Ensure that refs/bundles/topic are sent as "have".
329+
tip=$(git -C clone-from rev-parse A) &&
330+
test_grep "clone> have $tip" trace-packet.txt
331+
'
332+
333+
test_expect_success 'negotiation: bundle with all wanted commits' '
334+
test_when_finished "rm -f trace*.txt" &&
335+
GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
336+
git clone --no-local --single-branch --branch=topic --no-tags \
337+
--bundle-uri="clone-from/B.bundle" \
338+
clone-from nego-bundle-all &&
339+
git -C nego-bundle-all for-each-ref --format="%(refname)" >refs &&
340+
grep "refs/bundles/" refs >actual &&
341+
test_write_lines refs/bundles/topic >expect &&
342+
test_cmp expect actual &&
343+
# We already have all needed commits so no "want" needed.
344+
test_grep ! "clone> want " trace-packet.txt
345+
'
346+
347+
test_expect_success 'negotiation: bundle list (no heuristic)' '
348+
test_when_finished "rm -f trace*.txt" &&
349+
cat >bundle-list <<-EOF &&
350+
[bundle]
351+
version = 1
352+
mode = all
353+
354+
[bundle "bundle-1"]
355+
uri = file://$(pwd)/clone-from/bundle-1.bundle
356+
357+
[bundle "bundle-2"]
358+
uri = file://$(pwd)/clone-from/bundle-2.bundle
359+
EOF
360+
361+
GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
362+
git clone --no-local --bundle-uri="file://$(pwd)/bundle-list" \
363+
clone-from nego-bundle-list-no-heuristic &&
364+
365+
git -C nego-bundle-list-no-heuristic for-each-ref --format="%(refname)" >refs &&
366+
grep "refs/bundles/" refs >actual &&
367+
cat >expect <<-\EOF &&
368+
refs/bundles/base
369+
refs/bundles/left
370+
EOF
371+
test_cmp expect actual &&
372+
tip=$(git -C nego-bundle-list-no-heuristic rev-parse refs/bundles/left) &&
373+
test_grep "clone> have $tip" trace-packet.txt
374+
'
375+
376+
test_expect_success 'negotiation: bundle list (creationToken)' '
377+
test_when_finished "rm -f trace*.txt" &&
378+
cat >bundle-list <<-EOF &&
379+
[bundle]
380+
version = 1
381+
mode = all
382+
heuristic = creationToken
383+
384+
[bundle "bundle-1"]
385+
uri = file://$(pwd)/clone-from/bundle-1.bundle
386+
creationToken = 1
387+
388+
[bundle "bundle-2"]
389+
uri = file://$(pwd)/clone-from/bundle-2.bundle
390+
creationToken = 2
391+
EOF
392+
393+
GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
394+
git clone --no-local --bundle-uri="file://$(pwd)/bundle-list" \
395+
clone-from nego-bundle-list-heuristic &&
396+
397+
git -C nego-bundle-list-heuristic for-each-ref --format="%(refname)" >refs &&
398+
grep "refs/bundles/" refs >actual &&
399+
cat >expect <<-\EOF &&
400+
refs/bundles/base
401+
refs/bundles/left
402+
EOF
403+
test_cmp expect actual &&
404+
tip=$(git -C nego-bundle-list-heuristic rev-parse refs/bundles/left) &&
405+
test_grep "clone> have $tip" trace-packet.txt
406+
'
407+
408+
test_expect_success 'negotiation: bundle list with all wanted commits' '
409+
test_when_finished "rm -f trace*.txt" &&
410+
cat >bundle-list <<-EOF &&
411+
[bundle]
412+
version = 1
413+
mode = all
414+
heuristic = creationToken
415+
416+
[bundle "bundle-1"]
417+
uri = file://$(pwd)/clone-from/bundle-1.bundle
418+
creationToken = 1
419+
420+
[bundle "bundle-2"]
421+
uri = file://$(pwd)/clone-from/bundle-2.bundle
422+
creationToken = 2
423+
EOF
424+
425+
GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
426+
git clone --no-local --single-branch --branch=left --no-tags \
427+
--bundle-uri="file://$(pwd)/bundle-list" \
428+
clone-from nego-bundle-list-all &&
429+
430+
git -C nego-bundle-list-all for-each-ref --format="%(refname)" >refs &&
431+
grep "refs/bundles/" refs >actual &&
432+
cat >expect <<-\EOF &&
433+
refs/bundles/base
434+
refs/bundles/left
435+
EOF
436+
test_cmp expect actual &&
437+
# We already have all needed commits so no "want" needed.
438+
test_grep ! "clone> want " trace-packet.txt
439+
'
440+
262441
#########################################################################
263442
# HTTP tests begin here
264443

‎t/t5607-clone-bundle.sh

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,41 @@ test_expect_success 'fetch SHA-1 from bundle' '
139139
git fetch --no-tags foo/tip.bundle "$(cat hash)"
140140
'
141141

142+
test_expect_success 'clone bundle with different fsckObjects configurations' '
143+
test_create_repo bundle-fsck &&
144+
(
145+
cd bundle-fsck &&
146+
test_commit A &&
147+
commit_a=$(git rev-parse A) &&
148+
tree_a=$(git rev-parse A^{tree}) &&
149+
cat >data <<-EOF &&
150+
tree $tree_a
151+
parent $commit_a
152+
author A U Thor
153+
committer A U Thor
154+
155+
commit: this is a commit with bad emails
156+
157+
EOF
158+
bad_commit=$(git hash-object --literally -t commit -w --stdin <data) &&
159+
git branch bad $bad_commit &&
160+
git bundle create bad.bundle bad
161+
) &&
162+
163+
git clone bundle-fsck/bad.bundle bundle-no-fsck &&
164+
165+
git -c fetch.fsckObjects=false -c transfer.fsckObjects=true \
166+
clone bundle-fsck/bad.bundle bundle-fetch-no-fsck &&
167+
168+
test_must_fail git -c fetch.fsckObjects=true \
169+
clone bundle-fsck/bad.bundle bundle-fetch-fsck 2>err &&
170+
test_grep "missingEmail" err &&
171+
172+
test_must_fail git -c transfer.fsckObjects=true \
173+
clone bundle-fsck/bad.bundle bundle-transfer-fsck 2>err &&
174+
test_grep "missingEmail" err
175+
'
176+
142177
test_expect_success 'git bundle uses expected default format' '
143178
git bundle create bundle HEAD^.. &&
144179
cat >expect <<-EOF &&

‎transport.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
186186
if (!data->get_refs_from_bundle_called)
187187
get_refs_from_bundle_inner(transport);
188188
ret = unbundle(the_repository, &data->header, data->fd,
189-
&extra_index_pack_args, 0);
189+
&extra_index_pack_args,
190+
fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
190191
transport->hash_algo = data->header.hash_algo;
191192
return ret;
192193
}

0 commit comments

Comments
 (0)