Skip to content

Commit 1e93c28

Browse files
gitstergitster
committed
Merge branch 'jc/transport-do-not-use-connect-twice-in-fetch' into maint
The auto-tag-following code in "git fetch" tries to reuse the same transport twice when the serving end does not cooperate and does not give tags that point to commits that are asked for as part of the primary transfer. Unfortunately, Git-aware transport helper interface is not designed to be used more than once, hence this does not work over smart-http transfer. * jc/transport-do-not-use-connect-twice-in-fetch: builtin/fetch.c: Fix a sparse warning fetch: work around "transport-take-over" hack fetch: refactor code that fetches leftover tags fetch: refactor code that prepares a transport fetch: rename file-scope global "transport" to "gtransport" t5802: add test for connect helper
2 parents 4b510c3 + 0f73f8b commit 1e93c28

File tree

4 files changed

+134
-31
lines changed

4 files changed

+134
-31
lines changed

‎builtin/fetch.c‎

Lines changed: 54 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ static int tags = TAGS_DEFAULT, unshallow;
3636
static const char *depth;
3737
static const char *upload_pack;
3838
static struct strbuf default_rla = STRBUF_INIT;
39-
static struct transport *transport;
39+
static struct transport *gtransport;
40+
static struct transport *gsecondary;
4041
static const char *submodule_prefix = "";
4142
static const char *recurse_submodules_default;
4243

@@ -95,8 +96,10 @@ static struct option builtin_fetch_options[] = {
9596

9697
static void unlock_pack(void)
9798
{
98-
if (transport)
99-
transport_unlock_pack(transport);
99+
if (gtransport)
100+
transport_unlock_pack(gtransport);
101+
if (gsecondary)
102+
transport_unlock_pack(gsecondary);
100103
}
101104

102105
static void unlock_pack_on_signal(int signo)
@@ -720,6 +723,48 @@ static int truncate_fetch_head(void)
720723
return 0;
721724
}
722725

726+
static void set_option(struct transport *transport, const char *name, const char *value)
727+
{
728+
int r = transport_set_option(transport, name, value);
729+
if (r < 0)
730+
die(_("Option \"%s\" value \"%s\" is not valid for %s"),
731+
name, value, transport->url);
732+
if (r > 0)
733+
warning(_("Option \"%s\" is ignored for %s\n"),
734+
name, transport->url);
735+
}
736+
737+
static struct transport *prepare_transport(struct remote *remote)
738+
{
739+
struct transport *transport;
740+
transport = transport_get(remote, NULL);
741+
transport_set_verbosity(transport, verbosity, progress);
742+
if (upload_pack)
743+
set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
744+
if (keep)
745+
set_option(transport, TRANS_OPT_KEEP, "yes");
746+
if (depth)
747+
set_option(transport, TRANS_OPT_DEPTH, depth);
748+
return transport;
749+
}
750+
751+
static void backfill_tags(struct transport *transport, struct ref *ref_map)
752+
{
753+
if (transport->cannot_reuse) {
754+
gsecondary = prepare_transport(transport->remote);
755+
transport = gsecondary;
756+
}
757+
758+
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
759+
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
760+
fetch_refs(transport, ref_map);
761+
762+
if (gsecondary) {
763+
transport_disconnect(gsecondary);
764+
gsecondary = NULL;
765+
}
766+
}
767+
723768
static int do_fetch(struct transport *transport,
724769
struct refspec *refs, int ref_count)
725770
{
@@ -803,11 +848,8 @@ static int do_fetch(struct transport *transport,
803848
struct ref **tail = &ref_map;
804849
ref_map = NULL;
805850
find_non_local_tags(transport, &ref_map, &tail);
806-
if (ref_map) {
807-
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
808-
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
809-
fetch_refs(transport, ref_map);
810-
}
851+
if (ref_map)
852+
backfill_tags(transport, ref_map);
811853
free_refs(ref_map);
812854
}
813855

@@ -816,17 +858,6 @@ static int do_fetch(struct transport *transport,
816858
return retcode;
817859
}
818860

819-
static void set_option(const char *name, const char *value)
820-
{
821-
int r = transport_set_option(transport, name, value);
822-
if (r < 0)
823-
die(_("Option \"%s\" value \"%s\" is not valid for %s"),
824-
name, value, transport->url);
825-
if (r > 0)
826-
warning(_("Option \"%s\" is ignored for %s\n"),
827-
name, transport->url);
828-
}
829-
830861
static int get_one_remote_for_fetch(struct remote *remote, void *priv)
831862
{
832863
struct string_list *list = priv;
@@ -949,15 +980,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
949980
die(_("No remote repository specified. Please, specify either a URL or a\n"
950981
"remote name from which new revisions should be fetched."));
951982

952-
transport = transport_get(remote, NULL);
953-
transport_set_verbosity(transport, verbosity, progress);
954-
if (upload_pack)
955-
set_option(TRANS_OPT_UPLOADPACK, upload_pack);
956-
if (keep)
957-
set_option(TRANS_OPT_KEEP, "yes");
958-
if (depth)
959-
set_option(TRANS_OPT_DEPTH, depth);
960-
983+
gtransport = prepare_transport(remote);
961984
if (argc > 0) {
962985
int j = 0;
963986
refs = xcalloc(argc + 1, sizeof(const char *));
@@ -983,10 +1006,10 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
9831006
sigchain_push_common(unlock_pack_on_signal);
9841007
atexit(unlock_pack);
9851008
refspec = parse_fetch_refspec(ref_nr, refs);
986-
exit_code = do_fetch(transport, refspec, ref_nr);
1009+
exit_code = do_fetch(gtransport, refspec, ref_nr);
9871010
free_refspec(ref_nr, refspec);
988-
transport_disconnect(transport);
989-
transport = NULL;
1011+
transport_disconnect(gtransport);
1012+
gtransport = NULL;
9901013
return exit_code;
9911014
}
9921015

‎t/t5802-connect-helper.sh‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
#!/bin/sh
2+
3+
test_description='ext::cmd remote "connect" helper'
4+
. ./test-lib.sh
5+
6+
test_expect_success setup '
7+
test_tick &&
8+
git commit --allow-empty -m initial &&
9+
test_tick &&
10+
git commit --allow-empty -m second &&
11+
test_tick &&
12+
git commit --allow-empty -m third &&
13+
test_tick &&
14+
git tag -a -m "tip three" three &&
15+
16+
test_tick &&
17+
git commit --allow-empty -m fourth
18+
'
19+
20+
test_expect_success clone '
21+
cmd=$(echo "echo >&2 ext::sh invoked && %S .." | sed -e "s/ /% /g") &&
22+
git clone "ext::sh -c %S% ." dst &&
23+
git for-each-ref refs/heads/ refs/tags/ >expect &&
24+
(
25+
cd dst &&
26+
git config remote.origin.url "ext::sh -c $cmd" &&
27+
git for-each-ref refs/heads/ refs/tags/
28+
) >actual &&
29+
test_cmp expect actual
30+
'
31+
32+
test_expect_success 'update following tag' '
33+
test_tick &&
34+
git commit --allow-empty -m fifth &&
35+
test_tick &&
36+
git tag -a -m "tip five" five &&
37+
git for-each-ref refs/heads/ refs/tags/ >expect &&
38+
(
39+
cd dst &&
40+
git pull &&
41+
git for-each-ref refs/heads/ refs/tags/ >../actual
42+
) &&
43+
test_cmp expect actual
44+
'
45+
46+
test_expect_success 'update backfilled tag' '
47+
test_tick &&
48+
git commit --allow-empty -m sixth &&
49+
test_tick &&
50+
git tag -a -m "tip two" two three^1 &&
51+
git for-each-ref refs/heads/ refs/tags/ >expect &&
52+
(
53+
cd dst &&
54+
git pull &&
55+
git for-each-ref refs/heads/ refs/tags/ >../actual
56+
) &&
57+
test_cmp expect actual
58+
'
59+
60+
test_expect_success 'update backfilled tag without primary transfer' '
61+
test_tick &&
62+
git tag -a -m "tip one " one two^1 &&
63+
git for-each-ref refs/heads/ refs/tags/ >expect &&
64+
(
65+
cd dst &&
66+
git pull &&
67+
git for-each-ref refs/heads/ refs/tags/ >../actual
68+
) &&
69+
test_cmp expect actual
70+
'
71+
72+
test_done

‎transport.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -875,6 +875,8 @@ void transport_take_over(struct transport *transport,
875875
transport->push_refs = git_transport_push;
876876
transport->disconnect = disconnect_git;
877877
transport->smart_options = &(data->options);
878+
879+
transport->cannot_reuse = 1;
878880
}
879881

880882
static int is_local(const char *url)

‎transport.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ struct transport {
2727
*/
2828
unsigned got_remote_refs : 1;
2929

30+
/*
31+
* Transports that call take-over destroys the data specific to
32+
* the transport type while doing so, and cannot be reused.
33+
*/
34+
unsigned cannot_reuse : 1;
35+
3036
/**
3137
* Returns 0 if successful, positive if the option is not
3238
* recognized or is inapplicable, and negative if the option

0 commit comments

Comments
 (0)