From: Brandon Williams <hidden> Date: 2018-06-05 18:00:43
This series adds the ref-in-want feature which was originally proposed
by Jonathan Tan
(https://public-inbox.org/git/cover.1485381677.git.jonathantanmy@google.com/).
Back when ref-in-want was first discussed it was decided that we should
first solve the issue of moving to a new wire format and find a way to
limit the ref-advertisement before moving forward with ref-in-want. Now
that protocol version 2 is a reality, and that refs can be filtered on
the server side, we can revisit ref-in-want.
This version of ref-in-want is a bit more restrictive than what Jonathan
originally proposed (only full ref names are allowed instead of globs
and OIDs), but it is meant to accomplish the same goal (solve the issues
of refs changing during negotiation).
Brandon Williams (8):
test-pkt-line: add unpack-sideband subcommand
upload-pack: implement ref-in-want
upload-pack: test negotiation with changing repository
fetch: refactor the population of peer ref OIDs
fetch: refactor fetch_refs into two functions
fetch: refactor to make function args narrower
fetch-pack: put shallow info in output parameter
fetch-pack: implement ref-in-want
Documentation/config.txt | 4 +
Documentation/technical/protocol-v2.txt | 28 ++-
builtin/clone.c | 4 +-
builtin/fetch.c | 126 +++++++-----
fetch-object.c | 2 +-
fetch-pack.c | 52 +++--
remote.c | 1 +
remote.h | 1 +
t/helper/test-pkt-line.c | 37 ++++
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +
t/lib-httpd/one-time-sed.sh | 16 ++
t/t5703-upload-pack-ref-in-want.sh | 245 ++++++++++++++++++++++++
transport-helper.c | 6 +-
transport-internal.h | 9 +-
transport.c | 34 +++-
transport.h | 3 +-
upload-pack.c | 64 +++++++
18 files changed, 564 insertions(+), 77 deletions(-)
create mode 100644 t/lib-httpd/one-time-sed.sh
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
--
2.17.1.1185.g55be947832-goog
From: Brandon Williams <hidden> Date: 2018-06-05 17:52:08
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +++
t/lib-httpd/one-time-sed.sh | 16 ++++++
t/t5703-upload-pack-ref-in-want.sh | 92 ++++++++++++++++++++++++++++++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have commit for' 'check_output'+."$TEST_DIRECTORY"/lib-httpd.sh+start_httpd++REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"+LOCAL_PRISTINE="$(pwd)/local_pristine"++test_expect_success'setup repos for change-while-negotiating test''+(+gitinit"$REPO"&&+cd"$REPO"&&+>.git/git-daemon-export-ok&&+test_commitm1&&+gittag-dm1&&++# Local repo with many commits (so that negotiation will take+# more than 1 request/response pair)+gitclone"http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo""$LOCAL_PRISTINE"&&+cd"$LOCAL_PRISTINE"&&+gitcheckout-bside&&+foriin$(seq133);dotest_commits$i;done&&++# Add novel commits to upstream+gitcheckoutmaster&&+cd"$REPO"&&+test_commitm2&&+test_commitm3&&+gittag-dm2m3+)&&+git-C"$LOCAL_PRISTINE"remoteset-urlorigin"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"&&+git-C"$LOCAL_PRISTINE"configprotocol.version2+'++inconsistency(){+# Simulate that the server initially reports $2 as the ref+# corresponding to $1, and after that, $1 as the ref corresponding to+# $1. This corresponds to the real-life situation where the server's+# repository appears to change during negotiation, for example, when+# different servers in a load-balancing arrangement serve (stateless)+# RPCs during a single negotiation.+printf"s/%s/%s/"\+$(git-C"$REPO"rev-parse$1|tr-d"\n")\+$(git-C"$REPO"rev-parse$2|tr-d"\n")\+>"$HTTPD_ROOT_PATH/one-time-sed"+}++test_expect_success'server is initially ahead - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+test_must_failgit-Clocalfetch2>err&&+grep"ERR upload-pack: not our ref"err+'++test_expect_failure'server is initially ahead - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verifymaster>expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_success'server is initially behind - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master^">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_failure'server is initially behind - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++stop_httpd+ test_done
From: Brandon Williams <hidden> Date: 2018-06-05 17:52:10
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,if(!update_head_ok)check_not_current_branch(ref_map);-for(rm=ref_map;rm;rm=rm->next){-if(rm->peer_ref){-structstring_list_item*peer_item=-string_list_lookup(&existing_refs,-rm->peer_ref->name);-if(peer_item){-structobject_id*old_oid=peer_item->util;-oidcpy(&rm->peer_ref->old_oid,old_oid);-}-}-}-if(tags==TAGS_DEFAULT&&autotags)transport_set_option(transport,TRANS_OPT_FOLLOWTAGS,"1");if(prune){
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,}cleanup:-string_list_clear(&existing_refs,1);returnretcode;}
From: Brandon Williams <hidden> Date: 2018-06-05 17:52:15
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 52 ++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 26 deletions(-)
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,if(refmap.nr)fetch_refspec=&refmap;else-fetch_refspec=&transport->remote->fetch;+fetch_refspec=&remote->fetch;for(i=0;i<fetch_refspec->nr;i++)get_fetch_map(ref_map,&fetch_refspec->items[i],&oref_tail,1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,die("--refmap option is only meaningful with command-line refspec(s).");}else{/* Use the defaults */-structremote*remote=transport->remote;structbranch*branch=branch_get(NULL);inthas_merge=branch_has_merge_config(branch);if(remote&&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,/* also fetch all tags */get_fetch_map(remote_refs,tag_refspec,&tail,0);elseif(tags==TAGS_DEFAULT&&*autotags)-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);/* Now append any refs to be updated opportunistically: */*tail=orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,structref*ref_map;intautotags=(transport->remote->fetch_tags==1);intretcode=0;+conststructref*remote_refs;+structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){if(transport->remote->fetch_tags==2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,gotocleanup;}-ref_map=get_ref_map(transport,rs,tags,&autotags);+if(rs->nr)+refspec_ref_prefixes(rs,&ref_prefixes);+elseif(transport->remote&&transport->remote->fetch.nr)+refspec_ref_prefixes(&transport->remote->fetch,&ref_prefixes);++if(ref_prefixes.argc&&+(tags==TAGS_SET||(tags==TAGS_DEFAULT&&!rs->nr))){+argv_array_push(&ref_prefixes,"refs/tags/");+}++remote_refs=transport_get_remote_refs(transport,&ref_prefixes);+argv_array_clear(&ref_prefixes);++ref_map=get_ref_map(transport->remote,remote_refs,rs,+tags,&autotags);if(!update_head_ok)check_not_current_branch(ref_map);
@@ -1184,7 +1184,7 @@ static int do_fetch(struct transport *transport,if(tags==TAGS_DEFAULT&&autotags){structref**tail=&ref_map;ref_map=NULL;-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);if(ref_map)backfill_tags(transport,ref_map);free_refs(ref_map);
From: Brandon Williams <hidden> Date: 2018-06-05 17:58:51
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.
Signed-off-by: Brandon Williams <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in want' 'grep"ERR upload-pack: not our ref"err'-test_expect_failure'server is initially ahead - ref in want''+test_expect_success'server is initially ahead - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in want' 'test_cmpexpectedactual'-test_expect_failure'server is initially behind - ref in want''+test_expect_success'server is initially behind - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
From: Brandon Williams <hidden> Date: 2018-06-05 17:59:03
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 <redacted>
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 23 +++++++++++++++++++----
fetch-object.c | 2 +-
fetch-pack.c | 17 +++++++++--------
transport-helper.c | 6 ++++--
transport-internal.h | 9 ++++++++-
transport.c | 34 ++++++++++++++++++++++++++++------
transport.h | 3 ++-
8 files changed, 73 insertions(+), 25 deletions(-)
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,intautotags=(transport->remote->fetch_tags==1);intretcode=0;conststructref*remote_refs;+structref*new_remote_refs=NULL;structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){
@@ -1172,7 +1175,19 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){++if(fetch_refs(transport,ref_map,&new_remote_refs)){+free_refs(ref_map);+retcode=1;+gotocleanup;+}+if(new_remote_refs){+free_refs(ref_map);+ref_map=get_ref_map(transport->remote,new_remote_refs,rs,+tags,&autotags);+free_refs(new_remote_refs);+}+if(consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
@@ -151,7 +151,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,}staticintfetch_refs_from_bundle(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){structbundle_transport_data*data=transport->data;returnunbundle(&data->header,data->fd,
@@ -287,7 +288,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus}staticintfetch_refs_via_pack(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){intret=0;structgit_transport_data*data=transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport,if(report_unmatched_refs(to_fetch,nr_heads))ret=-1;+if(fetched_refs)+*fetched_refs=refs;+else+free_refs(refs);+free_refs(refs_tmp);-free_refs(refs);free(dest);returnret;}
@@ -1215,19 +1221,31 @@ const struct ref *transport_get_remote_refs(struct transport *transport,returntransport->remote_refs;}-inttransport_fetch_refs(structtransport*transport,structref*refs)+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs){intrc;intnr_heads=0,nr_alloc=0,nr_refs=0;structref**heads=NULL;+structref*nop_head=NULL,**nop_tail=&nop_head;structref*rm;for(rm=refs;rm;rm=rm->next){nr_refs++;if(rm->peer_ref&&!is_null_oid(&rm->old_oid)&&-!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid))+!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid)){+/*+*Theseneedtobereportedasfetched,butwedonnot+*actuallyneedtofetchthem.+*/+if(fetched_refs){+structref*nop_ref=copy_ref(rm);+*nop_tail=nop_ref;+nop_tail=&nop_ref->next;+}continue;+}ALLOC_GROW(heads,nr_heads+1,nr_alloc);heads[nr_heads++]=rm;}
@@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)heads[nr_heads++]=rm;}-rc=transport->vtable->fetch(transport,nr_heads,heads);+rc=transport->vtable->fetch(transport,nr_heads,heads,fetched_refs);+if(fetched_refs&&nop_head){+*nop_tail=*fetched_refs;+*fetched_refs=nop_head;+}free(heads);returnrc;
@@ -218,7 +218,8 @@ int transport_push(struct transport *connection,conststructref*transport_get_remote_refs(structtransport*transport,conststructargv_array*ref_prefixes);-inttransport_fetch_refs(structtransport*transport,structref*refs);+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs);voidtransport_unlock_pack(structtransport*transport);inttransport_disconnect(structtransport*transport);char*transport_anonymize_url(constchar*url);
From: Brandon Williams <hidden> Date: 2018-06-05 17:59:40
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)){+if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
From: Brandon Williams <hidden> Date: 2018-06-05 18:00:14
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
From: Brandon Williams <hidden> Date: 2018-06-05 18:00:17
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 4 +
Documentation/technical/protocol-v2.txt | 28 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 64 ++++++++++
4 files changed, 248 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,10 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,21 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server than the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -379,6 +392,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.++ * Always begins with the section header "wanted-refs"++ * The server will send a ref listing ("<oid> <refname>") for+ each reference requested using 'want-ref' lines.++ * Ther server MUST NOT send any refs which were not requested+ using 'want-ref' lines.+ packfile section * This section is only included if the client has sent 'want' lines in its request and either requested that no more
@@ -1210,6 +1243,8 @@ static void process_args(struct packet_reader *request,/* process want */if(parse_want(arg))continue;+if(allow_ref_in_want&&parse_want_ref(arg,&data->wanted_refs))+continue;/* process have line */if(parse_have(arg,&data->haves))continue;
@@ -1352,6 +1387,24 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)returnret;}+staticvoidsend_wanted_ref_info(structupload_pack_data*data)+{+conststructstring_list_item*item;++if(!data->wanted_refs.nr)+return;++packet_write_fmt(1,"wanted-refs\n");++for_each_string_list_item(item,&data->wanted_refs){+packet_write_fmt(1,"%s %s\n",+oid_to_hex(item->util),+item->string);+}++packet_delim(1);+}+staticvoidsend_shallow_info(structupload_pack_data*data){/* No shallow info needs to be sent */
From: Ramsay Jones <hidden> Date: 2018-06-05 19:11:37
On 05/06/18 18:51, Brandon Williams wrote:
quoted hunk
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 4 +
Documentation/technical/protocol-v2.txt | 28 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 64 ++++++++++
4 files changed, 248 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,10 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,21 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server than the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command.
+
I think it makes sense to elaborate a bit on what this is for. Having
read this series through, and to make sure I understood this, maybe
something like this:
This feature is intended for the benefit of load-balanced servers
which may not have the same view of what SHA-1s their refs point to,
but are guaranteed to never advertise a reference that another server
serving the request doesn't know about.
I.e. from what I can tell this gives no benefits for someone using a
monolithic git server, except insofar as there would be a slight
decrease in network traffic if the average length of refs is less than
the length of a SHA-1.
That's fair enough, just something we should prominently say.
It does have the "disadvantage", if you can call it that, that it's
introducing a race condition between when we read the ref advertisement
and are promised XYZ refs, but may actually get ABC, but I can't think
of a reason anyone would care about this in practice.
The reason I'm saying "another server [...] doesn't know about" above is
that 2/8 has this:
if (read_ref(arg, &oid))
die("unknown ref %s", arg);
Doesn't that mean that if server A in your pool advertises master, next
& pu, and you then go and fetch from server B advertising master & next,
but not "pu" that the clone will die?
Presumably at Google you either have something to ensure a consistent
view, e.g. only advertise refs by name older than N seconds, or globally
update ref name but not their contents, and don't allow deleting refs
(or give them the same treatment).
But that, and again, I may have misunderstood this whole thing,
significantly reduces the utility of this feature for anyone "in the
wild" since nothing shipped with "git" gives you that feature.
The naïve way to do slave mirroring with stock git is to have a
post-receive hook that pushes to your mirrors in a for-loop, or has them
fetch from the master in a loop, and then round-robin LB those
servers. Due to the "die on nonexisting" semantics in this extension
that'll result in failed clones.
So I think we should either be really vocal about that caveat, or
perhaps think of how we could make that configurable, e.g. what happens
if the server says "sorry, don't know about that one", and carries on
with the rest it does know about?
Is there a way for client & server to gracefully recover from that?
E.g. send "master" & "next" now, and when I pull again in a few seconds
I get the new "pu"?
Also, as a digression isn't that a problem shared with protocol v2 in
general? I.e. without this extension isn't it going to make another
connection to the naïve LB'd mirroring setup described above and find
that SHA-1s as well as refs don't match?
BREAK.
Also is if this E-Mail wasn't long enough, on a completely different
topic, in an earlier discussion in
https://public-inbox.org/git/87inaje1uv.fsf@evledraar.gmail.com/ I noted
that it would be neat-o to have optional wildmatch/pcre etc. matching
for the use case you're not caring about here (and I don't expect you
to, you're solving a different problem).
But let's say I want to add that after this, and being unfamiliar with
the protocol v2 conventions. Would that be a whole new
ref-in-want-wildmatch-prefix capability with a new
want-ref-wildmatch-prefix verb, or is there some less verbose way we can
anticipate that use-case and internally version / advertise
sub-capabilities?
I don't know if that makes any sense, and would be fine with just a
ref-in-want-wildmatch-prefix if that's the way to do it. I just think
it's inevitable that we'll have such a thing eventually, so it's worth
thinking about how such a future extension fits in.
From: Brandon Williams <hidden> Date: 2018-06-06 21:32:19
On 06/05, Ævar Arnfjörð Bjarmason wrote:
On Tue, Jun 05 2018, Brandon Williams wrote:
quoted
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command.
+
I think it makes sense to elaborate a bit on what this is for. Having
read this series through, and to make sure I understood this, maybe
something like this:
This feature is intended for the benefit of load-balanced servers
which may not have the same view of what SHA-1s their refs point to,
but are guaranteed to never advertise a reference that another server
serving the request doesn't know about.
I.e. from what I can tell this gives no benefits for someone using a
monolithic git server, except insofar as there would be a slight
decrease in network traffic if the average length of refs is less than
the length of a SHA-1.
Yeah I agree that the motivation should probably be spelled out more,
thanks for the suggestion.
That's fair enough, just something we should prominently say.
It does have the "disadvantage", if you can call it that, that it's
introducing a race condition between when we read the ref advertisement
and are promised XYZ refs, but may actually get ABC, but I can't think
of a reason anyone would care about this in practice.
The reason I'm saying "another server [...] doesn't know about" above is
that 2/8 has this:
if (read_ref(arg, &oid))
die("unknown ref %s", arg);
Doesn't that mean that if server A in your pool advertises master, next
& pu, and you then go and fetch from server B advertising master & next,
but not "pu" that the clone will die?
Presumably at Google you either have something to ensure a consistent
view, e.g. only advertise refs by name older than N seconds, or globally
update ref name but not their contents, and don't allow deleting refs
(or give them the same treatment).
But that, and again, I may have misunderstood this whole thing,
significantly reduces the utility of this feature for anyone "in the
wild" since nothing shipped with "git" gives you that feature.
The naïve way to do slave mirroring with stock git is to have a
post-receive hook that pushes to your mirrors in a for-loop, or has them
fetch from the master in a loop, and then round-robin LB those
servers. Due to the "die on nonexisting" semantics in this extension
that'll result in failed clones.
So I think we should either be really vocal about that caveat, or
perhaps think of how we could make that configurable, e.g. what happens
if the server says "sorry, don't know about that one", and carries on
with the rest it does know about?
Jonathan actually pointed this out to me earlier and I think the best
way to deal with this is to just ignore the refs that the server doesn't
know about instead of dying here. I mean its no worse than what we
already have and we shouldn't hit this case too often. And that way the
fetch can still proceed.
Is there a way for client & server to gracefully recover from that?
E.g. send "master" & "next" now, and when I pull again in a few seconds
I get the new "pu"?
I think in this case the client would just need to wait for some amount
of replication delay and attempt fetching at a later point.
Also, as a digression isn't that a problem shared with protocol v2 in
general? I.e. without this extension isn't it going to make another
connection to the naïve LB'd mirroring setup described above and find
that SHA-1s as well as refs don't match?
This is actually an issue with fetch using either v2 or v0. Unless I'm
misunderstanding what you're asking here.
BREAK.
Also is if this E-Mail wasn't long enough, on a completely different
topic, in an earlier discussion in
https://public-inbox.org/git/87inaje1uv.fsf@evledraar.gmail.com/ I noted
that it would be neat-o to have optional wildmatch/pcre etc. matching
for the use case you're not caring about here (and I don't expect you
to, you're solving a different problem).
But let's say I want to add that after this, and being unfamiliar with
the protocol v2 conventions. Would that be a whole new
ref-in-want-wildmatch-prefix capability with a new
want-ref-wildmatch-prefix verb, or is there some less verbose way we can
anticipate that use-case and internally version / advertise
sub-capabilities?
I don't know if that makes any sense, and would be fine with just a
ref-in-want-wildmatch-prefix if that's the way to do it. I just think
it's inevitable that we'll have such a thing eventually, so it's worth
thinking about how such a future extension fits in.
Yes back when introducing the server-side ref filtering in ls-refs we
originally talked about included wildmatch or other forms of pattern
matching. We opted to not over complicate things and favored prefix
matching because it didn't bake in some subset of globbing or regex and
it was easier to compute on the server side.
Anyway back to your question. Yes if at some point in the future we
wanted to add in wildmatch/pcre to the protocol for ls-refs or for
ref-in-want then it could be added as a feature or capability. I don't
think it would require adding a whole new verb (it probably would for
the ls-refs case since the verb used there is "ref-prefix") but the
capability could mean that the "want-ref" verb now understands wildmatch
patterns in addition to fully qualified refs.
--
Brandon Williams
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command.
+
I think it makes sense to elaborate a bit on what this is for. Having
read this series through, and to make sure I understood this, maybe
something like this:
This feature is intended for the benefit of load-balanced servers
which may not have the same view of what SHA-1s their refs point to,
but are guaranteed to never advertise a reference that another server
serving the request doesn't know about.
I.e. from what I can tell this gives no benefits for someone using a
monolithic git server, except insofar as there would be a slight
decrease in network traffic if the average length of refs is less than
the length of a SHA-1.
Yeah I agree that the motivation should probably be spelled out more,
thanks for the suggestion.
quoted
That's fair enough, just something we should prominently say.
It does have the "disadvantage", if you can call it that, that it's
introducing a race condition between when we read the ref advertisement
and are promised XYZ refs, but may actually get ABC, but I can't think
of a reason anyone would care about this in practice.
The reason I'm saying "another server [...] doesn't know about" above is
that 2/8 has this:
if (read_ref(arg, &oid))
die("unknown ref %s", arg);
Doesn't that mean that if server A in your pool advertises master, next
& pu, and you then go and fetch from server B advertising master & next,
but not "pu" that the clone will die?
Presumably at Google you either have something to ensure a consistent
view, e.g. only advertise refs by name older than N seconds, or globally
update ref name but not their contents, and don't allow deleting refs
(or give them the same treatment).
But that, and again, I may have misunderstood this whole thing,
significantly reduces the utility of this feature for anyone "in the
wild" since nothing shipped with "git" gives you that feature.
The naïve way to do slave mirroring with stock git is to have a
post-receive hook that pushes to your mirrors in a for-loop, or has them
fetch from the master in a loop, and then round-robin LB those
servers. Due to the "die on nonexisting" semantics in this extension
that'll result in failed clones.
So I think we should either be really vocal about that caveat, or
perhaps think of how we could make that configurable, e.g. what happens
if the server says "sorry, don't know about that one", and carries on
with the rest it does know about?
Jonathan actually pointed this out to me earlier and I think the best
way to deal with this is to just ignore the refs that the server doesn't
know about instead of dying here. I mean its no worse than what we
already have and we shouldn't hit this case too often. And that way the
fetch can still proceed.
quoted
Is there a way for client & server to gracefully recover from that?
E.g. send "master" & "next" now, and when I pull again in a few seconds
I get the new "pu"?
I think in this case the client would just need to wait for some amount
of replication delay and attempt fetching at a later point.
quoted
Also, as a digression isn't that a problem shared with protocol v2 in
general? I.e. without this extension isn't it going to make another
connection to the naïve LB'd mirroring setup described above and find
that SHA-1s as well as refs don't match?
This is actually an issue with fetch using either v2 or v0. Unless I'm
misunderstanding what you're asking here.
Isn't the whole dialog in v1 guaranteed to be with one server from
intial ref advertisement to the client saying have/want, or is that just
with ssh?
In any case the reason the above is an issue here is because you're
getting the advertisement from a different server than you're
negotiating the pack with, right?
quoted
BREAK.
Also is if this E-Mail wasn't long enough, on a completely different
topic, in an earlier discussion in
https://public-inbox.org/git/87inaje1uv.fsf@evledraar.gmail.com/ I noted
that it would be neat-o to have optional wildmatch/pcre etc. matching
for the use case you're not caring about here (and I don't expect you
to, you're solving a different problem).
But let's say I want to add that after this, and being unfamiliar with
the protocol v2 conventions. Would that be a whole new
ref-in-want-wildmatch-prefix capability with a new
want-ref-wildmatch-prefix verb, or is there some less verbose way we can
anticipate that use-case and internally version / advertise
sub-capabilities?
I don't know if that makes any sense, and would be fine with just a
ref-in-want-wildmatch-prefix if that's the way to do it. I just think
it's inevitable that we'll have such a thing eventually, so it's worth
thinking about how such a future extension fits in.
Yes back when introducing the server-side ref filtering in ls-refs we
originally talked about included wildmatch or other forms of pattern
matching. We opted to not over complicate things and favored prefix
matching because it didn't bake in some subset of globbing or regex and
it was easier to compute on the server side.
Anyway back to your question. Yes if at some point in the future we
wanted to add in wildmatch/pcre to the protocol for ls-refs or for
ref-in-want then it could be added as a feature or capability. I don't
think it would require adding a whole new verb (it probably would for
the ls-refs case since the verb used there is "ref-prefix") but the
capability could mean that the "want-ref" verb now understands wildmatch
patterns in addition to fully qualified refs.
Probably still makes sense to have it be a different verb since some
things in wildmatch / regex are metachars but may be valid in ref names.
Thanks!
From: Brandon Williams <hidden> Date: 2018-06-06 22:45:42
On 06/07, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jun 06 2018, Brandon Williams wrote:
quoted
On 06/05, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, Jun 05 2018, Brandon Williams wrote:
quoted
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command.
+
I think it makes sense to elaborate a bit on what this is for. Having
read this series through, and to make sure I understood this, maybe
something like this:
This feature is intended for the benefit of load-balanced servers
which may not have the same view of what SHA-1s their refs point to,
but are guaranteed to never advertise a reference that another server
serving the request doesn't know about.
I.e. from what I can tell this gives no benefits for someone using a
monolithic git server, except insofar as there would be a slight
decrease in network traffic if the average length of refs is less than
the length of a SHA-1.
Yeah I agree that the motivation should probably be spelled out more,
thanks for the suggestion.
quoted
That's fair enough, just something we should prominently say.
It does have the "disadvantage", if you can call it that, that it's
introducing a race condition between when we read the ref advertisement
and are promised XYZ refs, but may actually get ABC, but I can't think
of a reason anyone would care about this in practice.
The reason I'm saying "another server [...] doesn't know about" above is
that 2/8 has this:
if (read_ref(arg, &oid))
die("unknown ref %s", arg);
Doesn't that mean that if server A in your pool advertises master, next
& pu, and you then go and fetch from server B advertising master & next,
but not "pu" that the clone will die?
Presumably at Google you either have something to ensure a consistent
view, e.g. only advertise refs by name older than N seconds, or globally
update ref name but not their contents, and don't allow deleting refs
(or give them the same treatment).
But that, and again, I may have misunderstood this whole thing,
significantly reduces the utility of this feature for anyone "in the
wild" since nothing shipped with "git" gives you that feature.
The naïve way to do slave mirroring with stock git is to have a
post-receive hook that pushes to your mirrors in a for-loop, or has them
fetch from the master in a loop, and then round-robin LB those
servers. Due to the "die on nonexisting" semantics in this extension
that'll result in failed clones.
So I think we should either be really vocal about that caveat, or
perhaps think of how we could make that configurable, e.g. what happens
if the server says "sorry, don't know about that one", and carries on
with the rest it does know about?
Jonathan actually pointed this out to me earlier and I think the best
way to deal with this is to just ignore the refs that the server doesn't
know about instead of dying here. I mean its no worse than what we
already have and we shouldn't hit this case too often. And that way the
fetch can still proceed.
quoted
Is there a way for client & server to gracefully recover from that?
E.g. send "master" & "next" now, and when I pull again in a few seconds
I get the new "pu"?
I think in this case the client would just need to wait for some amount
of replication delay and attempt fetching at a later point.
quoted
Also, as a digression isn't that a problem shared with protocol v2 in
general? I.e. without this extension isn't it going to make another
connection to the naïve LB'd mirroring setup described above and find
that SHA-1s as well as refs don't match?
This is actually an issue with fetch using either v2 or v0. Unless I'm
misunderstanding what you're asking here.
Isn't the whole dialog in v1 guaranteed to be with one server from
intial ref advertisement to the client saying have/want, or is that just
with ssh?
That's only guaranteed with statefull connections (git:// and ssh://),
http:// has this issue because its stateless.
In any case the reason the above is an issue here is because you're
getting the advertisement from a different server than you're
negotiating the pack with, right?
Yes correct, or even a different server on each negotiation round-trip.
quoted
quoted
BREAK.
Also is if this E-Mail wasn't long enough, on a completely different
topic, in an earlier discussion in
https://public-inbox.org/git/87inaje1uv.fsf@evledraar.gmail.com/ I noted
that it would be neat-o to have optional wildmatch/pcre etc. matching
for the use case you're not caring about here (and I don't expect you
to, you're solving a different problem).
But let's say I want to add that after this, and being unfamiliar with
the protocol v2 conventions. Would that be a whole new
ref-in-want-wildmatch-prefix capability with a new
want-ref-wildmatch-prefix verb, or is there some less verbose way we can
anticipate that use-case and internally version / advertise
sub-capabilities?
I don't know if that makes any sense, and would be fine with just a
ref-in-want-wildmatch-prefix if that's the way to do it. I just think
it's inevitable that we'll have such a thing eventually, so it's worth
thinking about how such a future extension fits in.
Yes back when introducing the server-side ref filtering in ls-refs we
originally talked about included wildmatch or other forms of pattern
matching. We opted to not over complicate things and favored prefix
matching because it didn't bake in some subset of globbing or regex and
it was easier to compute on the server side.
Anyway back to your question. Yes if at some point in the future we
wanted to add in wildmatch/pcre to the protocol for ls-refs or for
ref-in-want then it could be added as a feature or capability. I don't
think it would require adding a whole new verb (it probably would for
the ls-refs case since the verb used there is "ref-prefix") but the
capability could mean that the "want-ref" verb now understands wildmatch
patterns in addition to fully qualified refs.
Probably still makes sense to have it be a different verb since some
things in wildmatch / regex are metachars but may be valid in ref names.
Yeah we can leave that up to the designer of such a feature ;)
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:37
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:38
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 7 ++
Documentation/technical/protocol-v2.txt | 29 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 64 ++++++++++
4 files changed, 252 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,22 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server. A server should ignore any "want-ref <ref>" lines where+ <ref> doesn't exist on the server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -379,6 +393,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.++ * Always begins with the section header "wanted-refs"++ * The server will send a ref listing ("<oid> <refname>") for+ each reference requested using 'want-ref' lines.++ * The server MUST NOT send any refs which were not requested+ using 'want-ref' lines.+ packfile section * This section is only included if the client has sent 'want' lines in its request and either requested that no more
@@ -1185,6 +1192,32 @@ static int parse_want(const char *line)return0;}+staticintparse_want_ref(constchar*line,structstring_list*wanted_refs)+{+constchar*arg;+if(skip_prefix(line,"want-ref ",&arg)){+structobject_idoid;+structstring_list_item*item;+structobject*o;++if(read_ref(arg,&oid))+return1;++item=string_list_append(wanted_refs,arg);+item->util=oiddup(&oid);++o=parse_object_or_die(&oid,arg);+if(!(o->flags&WANTED)){+o->flags|=WANTED;+add_object_array(o,NULL,&want_obj);+}++return1;+}++return0;+}+staticintparse_have(constchar*line,structoid_array*haves){constchar*arg;
@@ -1210,6 +1243,8 @@ static void process_args(struct packet_reader *request,/* process want */if(parse_want(arg))continue;+if(allow_ref_in_want&&parse_want_ref(arg,&data->wanted_refs))+continue;/* process have line */if(parse_have(arg,&data->haves))continue;
@@ -1352,6 +1387,24 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)returnret;}+staticvoidsend_wanted_ref_info(structupload_pack_data*data)+{+conststructstring_list_item*item;++if(!data->wanted_refs.nr)+return;++packet_write_fmt(1,"wanted-refs\n");++for_each_string_list_item(item,&data->wanted_refs){+packet_write_fmt(1,"%s %s\n",+oid_to_hex(item->util),+item->string);+}++packet_delim(1);+}+staticvoidsend_shallow_info(structupload_pack_data*data){/* No shallow info needs to be sent */
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:44
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,if(!update_head_ok)check_not_current_branch(ref_map);-for(rm=ref_map;rm;rm=rm->next){-if(rm->peer_ref){-structstring_list_item*peer_item=-string_list_lookup(&existing_refs,-rm->peer_ref->name);-if(peer_item){-structobject_id*old_oid=peer_item->util;-oidcpy(&rm->peer_ref->old_oid,old_oid);-}-}-}-if(tags==TAGS_DEFAULT&&autotags)transport_set_option(transport,TRANS_OPT_FOLLOWTAGS,"1");if(prune){
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,}cleanup:-string_list_clear(&existing_refs,1);returnretcode;}
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:46
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +++
t/lib-httpd/one-time-sed.sh | 16 ++++++
t/t5703-upload-pack-ref-in-want.sh | 92 ++++++++++++++++++++++++++++++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have commit for' 'check_output'+."$TEST_DIRECTORY"/lib-httpd.sh+start_httpd++REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"+LOCAL_PRISTINE="$(pwd)/local_pristine"++test_expect_success'setup repos for change-while-negotiating test''+(+gitinit"$REPO"&&+cd"$REPO"&&+>.git/git-daemon-export-ok&&+test_commitm1&&+gittag-dm1&&++# Local repo with many commits (so that negotiation will take+# more than 1 request/response pair)+gitclone"http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo""$LOCAL_PRISTINE"&&+cd"$LOCAL_PRISTINE"&&+gitcheckout-bside&&+foriin$(seq133);dotest_commits$i;done&&++# Add novel commits to upstream+gitcheckoutmaster&&+cd"$REPO"&&+test_commitm2&&+test_commitm3&&+gittag-dm2m3+)&&+git-C"$LOCAL_PRISTINE"remoteset-urlorigin"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"&&+git-C"$LOCAL_PRISTINE"configprotocol.version2+'++inconsistency(){+# Simulate that the server initially reports $2 as the ref+# corresponding to $1, and after that, $1 as the ref corresponding to+# $1. This corresponds to the real-life situation where the server's+# repository appears to change during negotiation, for example, when+# different servers in a load-balancing arrangement serve (stateless)+# RPCs during a single negotiation.+printf"s/%s/%s/"\+$(git-C"$REPO"rev-parse$1|tr-d"\n")\+$(git-C"$REPO"rev-parse$2|tr-d"\n")\+>"$HTTPD_ROOT_PATH/one-time-sed"+}++test_expect_success'server is initially ahead - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+test_must_failgit-Clocalfetch2>err&&+grep"ERR upload-pack: not our ref"err+'++test_expect_failure'server is initially ahead - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verifymaster>expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_success'server is initially behind - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master^">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_failure'server is initially behind - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++stop_httpd+ test_done
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:50
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)){+if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:54
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.
Signed-off-by: Brandon Williams <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in want' 'grep"ERR upload-pack: not our ref"err'-test_expect_failure'server is initially ahead - ref in want''+test_expect_success'server is initially ahead - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in want' 'test_cmpexpectedactual'-test_expect_failure'server is initially behind - ref in want''+test_expect_success'server is initially behind - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:56
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 <redacted>
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 23 +++++++++++++++++++----
fetch-object.c | 2 +-
fetch-pack.c | 17 +++++++++--------
transport-helper.c | 6 ++++--
transport-internal.h | 9 ++++++++-
transport.c | 34 ++++++++++++++++++++++++++++------
transport.h | 3 ++-
8 files changed, 73 insertions(+), 25 deletions(-)
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,intautotags=(transport->remote->fetch_tags==1);intretcode=0;conststructref*remote_refs;+structref*new_remote_refs=NULL;structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){
@@ -1172,7 +1175,19 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){++if(fetch_refs(transport,ref_map,&new_remote_refs)){+free_refs(ref_map);+retcode=1;+gotocleanup;+}+if(new_remote_refs){+free_refs(ref_map);+ref_map=get_ref_map(transport->remote,new_remote_refs,rs,+tags,&autotags);+free_refs(new_remote_refs);+}+if(consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
@@ -151,7 +151,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,}staticintfetch_refs_from_bundle(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){structbundle_transport_data*data=transport->data;returnunbundle(&data->header,data->fd,
@@ -287,7 +288,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus}staticintfetch_refs_via_pack(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){intret=0;structgit_transport_data*data=transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport,if(report_unmatched_refs(to_fetch,nr_heads))ret=-1;+if(fetched_refs)+*fetched_refs=refs;+else+free_refs(refs);+free_refs(refs_tmp);-free_refs(refs);free(dest);returnret;}
@@ -1215,19 +1221,31 @@ const struct ref *transport_get_remote_refs(struct transport *transport,returntransport->remote_refs;}-inttransport_fetch_refs(structtransport*transport,structref*refs)+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs){intrc;intnr_heads=0,nr_alloc=0,nr_refs=0;structref**heads=NULL;+structref*nop_head=NULL,**nop_tail=&nop_head;structref*rm;for(rm=refs;rm;rm=rm->next){nr_refs++;if(rm->peer_ref&&!is_null_oid(&rm->old_oid)&&-!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid))+!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid)){+/*+*Theseneedtobereportedasfetched,butwedonnot+*actuallyneedtofetchthem.+*/+if(fetched_refs){+structref*nop_ref=copy_ref(rm);+*nop_tail=nop_ref;+nop_tail=&nop_ref->next;+}continue;+}ALLOC_GROW(heads,nr_heads+1,nr_alloc);heads[nr_heads++]=rm;}
@@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)heads[nr_heads++]=rm;}-rc=transport->vtable->fetch(transport,nr_heads,heads);+rc=transport->vtable->fetch(transport,nr_heads,heads,fetched_refs);+if(fetched_refs&&nop_head){+*nop_tail=*fetched_refs;+*fetched_refs=nop_head;+}free(heads);returnrc;
@@ -218,7 +218,8 @@ int transport_push(struct transport *connection,conststructref*transport_get_remote_refs(structtransport*transport,conststructargv_array*ref_prefixes);-inttransport_fetch_refs(structtransport*transport,structref*refs);+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs);voidtransport_unlock_pack(structtransport*transport);inttransport_disconnect(structtransport*transport);char*transport_anonymize_url(constchar*url);
From: Brandon Williams <hidden> Date: 2018-06-13 21:39:59
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 52 ++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 26 deletions(-)
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,if(refmap.nr)fetch_refspec=&refmap;else-fetch_refspec=&transport->remote->fetch;+fetch_refspec=&remote->fetch;for(i=0;i<fetch_refspec->nr;i++)get_fetch_map(ref_map,&fetch_refspec->items[i],&oref_tail,1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,die("--refmap option is only meaningful with command-line refspec(s).");}else{/* Use the defaults */-structremote*remote=transport->remote;structbranch*branch=branch_get(NULL);inthas_merge=branch_has_merge_config(branch);if(remote&&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,/* also fetch all tags */get_fetch_map(remote_refs,tag_refspec,&tail,0);elseif(tags==TAGS_DEFAULT&&*autotags)-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);/* Now append any refs to be updated opportunistically: */*tail=orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,structref*ref_map;intautotags=(transport->remote->fetch_tags==1);intretcode=0;+conststructref*remote_refs;+structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){if(transport->remote->fetch_tags==2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,gotocleanup;}-ref_map=get_ref_map(transport,rs,tags,&autotags);+if(rs->nr)+refspec_ref_prefixes(rs,&ref_prefixes);+elseif(transport->remote&&transport->remote->fetch.nr)+refspec_ref_prefixes(&transport->remote->fetch,&ref_prefixes);++if(ref_prefixes.argc&&+(tags==TAGS_SET||(tags==TAGS_DEFAULT&&!rs->nr))){+argv_array_push(&ref_prefixes,"refs/tags/");+}++remote_refs=transport_get_remote_refs(transport,&ref_prefixes);+argv_array_clear(&ref_prefixes);++ref_map=get_ref_map(transport->remote,remote_refs,rs,+tags,&autotags);if(!update_head_ok)check_not_current_branch(ref_map);
@@ -1184,7 +1184,7 @@ static int do_fetch(struct transport *transport,if(tags==TAGS_DEFAULT&&autotags){structref**tail=&ref_map;ref_map=NULL;-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);if(ref_map)backfill_tags(transport,ref_map);free_refs(ref_map);
From: Stefan Beller <hidden> Date: 2018-06-14 18:10:02
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
quoted hunk
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
s/sind/side/ ?
What values for band are possible?
e.g. band==4 would also just write to fd=1;
but I suspect we don't want that, yet.
So maybe
band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
die("unexpected side band %d", band)
fd = band;
instead?
From: Stefan Beller <hidden> Date: 2018-06-14 18:40:55
Hi Brandon,
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
I stopped reading when stumbling upon 'vulnerability to failure' a I
found it hard to read. A quick search turns out this is insider slang
of civil engineers. :)
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
... and the repository is not replicated evenly to all servers, yet.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
This paragraph makes it sound as if it can be combined technically,
i.e.
client:
want 01234...
want-ref master
.. usual back and forth + pack..
server:
wanted-ref: master 2345..
What happens if the client "wants" a sha1 that is advertised,
but happens to be the same as a wanted-ref?
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.
Instead of saying who benefits, can we also say what the feature is about?
Didn't someone mention on the first round of this series, that technically
ref-in-want also provides smaller net work load as refs usually are shorter
than oids (specifically as oids will grow in the hash transisition plan later)?
Is that worth mentioning?
When using this feature is a ref advertisement still needed?
quoted hunk
+
url.<base>.insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,22 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server. A server should ignore any "want-ref <ref>" lines where+ <ref> doesn't exist on the server.
Are patterns allowed?, e.g. I might want refs/tags/* at all times.
@@ -379,6 +393,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.
Is it possible to fetch non-fast-forwarded refs this way? Or specifcially
refs that were reset to an older point in history such that no pack file
is needed to transfer; would we transfer an empty pack and then
the wanted-refs section for that use case?
From: Brandon Williams <hidden> Date: 2018-06-14 18:52:28
On 06/14, Stefan Beller wrote:
Hi Brandon,
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
quoted
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
... and the repository is not replicated evenly to all servers, yet.
I'll update the commit msg to also include this.
quoted
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
This paragraph makes it sound as if it can be combined technically,
i.e.
client:
want 01234...
want-ref master
.. usual back and forth + pack..
server:
wanted-ref: master 2345..
What happens if the client "wants" a sha1 that is advertised,
but happens to be the same as a wanted-ref?
This would be fine, same as sending a want line with the same sha1 lots
of times. Though there would still be a wanted-ref section from the
server for the wanted-ref.
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.
Instead of saying who benefits, can we also say what the feature is about?
Didn't someone mention on the first round of this series, that technically
ref-in-want also provides smaller net work load as refs usually are shorter
than oids (specifically as oids will grow in the hash transisition plan later)?
Is that worth mentioning?
Well I basically just took this from what a previous reviewer thought it
should say. I think what you have listed here isn't really a big
benefit of using ref-in-want, its the issue with load-balanced servers
that this is trying to solve.
When using this feature is a ref advertisement still needed?
Maybe in the future no, but as of right now the code is structured to
still request a ref advertisement.
quoted
+
url.<base>.insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,22 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server. A server should ignore any "want-ref <ref>" lines where+ <ref> doesn't exist on the server.
Are patterns allowed?, e.g. I might want refs/tags/* at all times.
Nope, "Where <ref> is the full name of a ref". We can maybe allow this
at a later point in time.
@@ -379,6 +393,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.
Is it possible to fetch non-fast-forwarded refs this way? Or specifcially
refs that were reset to an older point in history such that no pack file
is needed to transfer; would we transfer an empty pack and then
the wanted-refs section for that use case?
Yeah there are cases where an empty packfile would be sent like you've
described.
From: Brandon Williams <hidden> Date: 2018-06-14 19:21:56
On 06/14, Stefan Beller wrote:
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
quoted
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
What values for band are possible?
e.g. band==4 would also just write to fd=1;
but I suspect we don't want that, yet.
So maybe
band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
die("unexpected side band %d", band)
fd = band;
instead?
Yeah that's must cleaner logic.
--
Brandon Williams
From: Stefan Beller <hidden> Date: 2018-06-14 19:23:24
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
From: Stefan Beller <hidden> Date: 2018-06-14 19:32:58
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
This patch and the two prior refactoring patches are
Reviewed-by: Stefan Beller <redacted>
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
+ }
The server is documented as MUST NOT send additional refs,
which is fine here, as we'd have no way of storing them anyway.
Do we want to issue a warning, though?
if (!r) /* never break'd */
warning ("server send unexpected line '%s'", reader.line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
+ }
The server is documented as MUST NOT send additional refs,
which is fine here, as we'd have no way of storing them anyway.
Do we want to issue a warning, though?
if (!r) /* never break'd */
warning ("server send unexpected line '%s'", reader.line);
Depends, does this warning help out the end user or do you think it
would confuse users to see this and still have their fetch succeed?
From: Jonathan Tan <hidden> Date: 2018-06-14 23:59:57
quoted hunk
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport, int autotags = (transport->remote->fetch_tags == 1); int retcode = 0; const struct ref *remote_refs;+ struct ref *new_remote_refs = NULL;
Above, you use the name "updated_remote_refs" - it's probably better to
standardize on one. I think "updated" is better.
(The transport calling it "fetched_refs" is fine, because that's what
they are from the perspective of the transport. From the perspective of
fetch-pack, it is indeed a new or updated set of remote refs.)
Here, if we got updated remote refs, we need to regenerate ref_map,
since it is the source of truth.
Maybe add a comment in the "if (new_remote_refs)" block explaining this
- something like: Regenerate ref_map using the updated remote refs,
because the transport would place shallow (and other) information
there.
- for (i = 0; i < nr_sought; i++)
+ for (r = refs; r; r = r->next, i++)
if (status[i])
- sought[i]->status = REF_STATUS_REJECT_SHALLOW;
+ r->status = REF_STATUS_REJECT_SHALLOW;
You use i here without initializing it to 0. t5703 also fails with this
patch - probably related to this, but I didn't check.
If you initialize i here, I don't think you need to initialize it to 0
at the top of this function.
From: Jonathan Tan <hidden> Date: 2018-06-15 19:05:06
(replying to the original since my e-mail is about design)
This version of ref-in-want is a bit more restrictive than what Jonathan
originally proposed (only full ref names are allowed instead of globs
and OIDs), but it is meant to accomplish the same goal (solve the issues
of refs changing during negotiation).
One question remains: are we planning to expand this feature (e.g. to
support patterns ending in *, or to support any pattern that can appear
on the LHS of a refspec), and if yes, are we OK with having 2 or more
versions of the service in the wild, each having different pattern
support?
Supporting patterns would mean that we would possibly be able to
eliminate the ls-refs step, thus saving at least a RTT. (Originally I
thought that supporting patterns would also allow us to tolerate refs
being removed during the fetch process, but I see that this is already
handled by the server ignoring "want-ref <ref>" wherein <ref> doesn't
exist on the server.)
However, after some in-office discussion, I see that eliminating the
ls-refs step means that we lose some optimizations that can only be done
when we see that we already have a sought remote ref. For example, in a
repo like this:
A
|
O
|
O B C
|/ /
O O
|/
O
in which we have rarely-updated branches that we still want to fetch
(e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
refs/changes/* branch), having the ref advertisement first means that we
can omit them from our "want" or "want-ref" list. But not having them
means that we send "want-ref refs/tags/*" to the server, and during
negotiation inform the server of our master branch (A), and since the
server knows of a common ancestor of all our wants (A, B, C), it will
terminate the negotiation and send the objects specific to branches B
and C even though it didn't need to.
So maybe we still need to keep the ls-refs step around, and thus, this
design of only accepting exact refs is perhaps good enough for now.
From: Brandon Williams <hidden> Date: 2018-06-19 17:32:56
On 06/15, Jonathan Tan wrote:
(replying to the original since my e-mail is about design)
quoted
This version of ref-in-want is a bit more restrictive than what Jonathan
originally proposed (only full ref names are allowed instead of globs
and OIDs), but it is meant to accomplish the same goal (solve the issues
of refs changing during negotiation).
One question remains: are we planning to expand this feature (e.g. to
support patterns ending in *, or to support any pattern that can appear
on the LHS of a refspec), and if yes, are we OK with having 2 or more
versions of the service in the wild, each having different pattern
support?
Supporting patterns would mean that we would possibly be able to
eliminate the ls-refs step, thus saving at least a RTT. (Originally I
thought that supporting patterns would also allow us to tolerate refs
being removed during the fetch process, but I see that this is already
handled by the server ignoring "want-ref <ref>" wherein <ref> doesn't
exist on the server.)
However, after some in-office discussion, I see that eliminating the
ls-refs step means that we lose some optimizations that can only be done
when we see that we already have a sought remote ref. For example, in a
repo like this:
A
|
O
|
O B C
|/ /
O O
|/
O
in which we have rarely-updated branches that we still want to fetch
(e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
refs/changes/* branch), having the ref advertisement first means that we
can omit them from our "want" or "want-ref" list. But not having them
means that we send "want-ref refs/tags/*" to the server, and during
negotiation inform the server of our master branch (A), and since the
server knows of a common ancestor of all our wants (A, B, C), it will
terminate the negotiation and send the objects specific to branches B
and C even though it didn't need to.
So maybe we still need to keep the ls-refs step around, and thus, this
design of only accepting exact refs is perhaps good enough for now.
I think that taking a smaller step first it probably better. This is
something that we've done in the past with the shallow features and
later capabilities were added to add different ways to request shallow
fetches.
That being said, if we find that this feature doesn't work as-is and
needs the extra complexity of patterns from the start then they should
be added. But it doesn't seem like there's a concrete reason at the
moment.
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2018-06-19 17:42:05
On 06/14, Jonathan Tan wrote:
quoted
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport, int autotags = (transport->remote->fetch_tags == 1); int retcode = 0; const struct ref *remote_refs;+ struct ref *new_remote_refs = NULL;
Above, you use the name "updated_remote_refs" - it's probably better to
standardize on one. I think "updated" is better.
Good catch I'll update the variable name.
(The transport calling it "fetched_refs" is fine, because that's what
they are from the perspective of the transport. From the perspective of
fetch-pack, it is indeed a new or updated set of remote refs.)
Here, if we got updated remote refs, we need to regenerate ref_map,
since it is the source of truth.
Maybe add a comment in the "if (new_remote_refs)" block explaining this
- something like: Regenerate ref_map using the updated remote refs,
because the transport would place shallow (and other) information
there.
That's probably a good idea to give future readers more context into why
this is happening.
quoted
- for (i = 0; i < nr_sought; i++)
+ for (r = refs; r; r = r->next, i++)
if (status[i])
- sought[i]->status = REF_STATUS_REJECT_SHALLOW;
+ r->status = REF_STATUS_REJECT_SHALLOW;
You use i here without initializing it to 0. t5703 also fails with this
patch - probably related to this, but I didn't check.
Oh yeah that's definitely a bug, thanks for catching that.
If you initialize i here, I don't think you need to initialize it to 0
at the top of this function.
From: Jonathan Tan <hidden> Date: 2018-06-19 19:23:36
[snip]
quoted
in which we have rarely-updated branches that we still want to fetch
(e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
refs/changes/* branch), having the ref advertisement first means that we
can omit them from our "want" or "want-ref" list. But not having them
means that we send "want-ref refs/tags/*" to the server, and during
negotiation inform the server of our master branch (A), and since the
server knows of a common ancestor of all our wants (A, B, C), it will
terminate the negotiation and send the objects specific to branches B
and C even though it didn't need to.
So maybe we still need to keep the ls-refs step around, and thus, this
design of only accepting exact refs is perhaps good enough for now.
I think that taking a smaller step first it probably better. This is
something that we've done in the past with the shallow features and
later capabilities were added to add different ways to request shallow
fetches.
I think we're agreeing that the smaller step first is better.
That being said, if we find that this feature doesn't work as-is and
needs the extra complexity of patterns from the start then they should
be added.
I agree (although I would be OK too if we decide to do the small
exact-name step now and then the pattern step later guarded by a
capability, as long as the project understood that multiple support
levels would then exist in the wild).
But it doesn't seem like there's a concrete reason at the
moment.
From: Brandon Williams <hidden> Date: 2018-06-19 23:16:49
On 06/15, Jonathan Tan wrote:
Supporting patterns would mean that we would possibly be able to
eliminate the ls-refs step, thus saving at least a RTT. (Originally I
thought that supporting patterns would also allow us to tolerate refs
being removed during the fetch process, but I see that this is already
handled by the server ignoring "want-ref <ref>" wherein <ref> doesn't
exist on the server.)
What's your opinion on this? Should we keep it how it is in v2 of the
series where the server ignores refs it doesn't know about or revert to
what v1 of the series did and have it be a hard error?
I've gone back and forth on what I think we should do so I'd like to
hear at least one more opinion :)
--
Brandon Williams
From: Jonathan Tan <hidden> Date: 2018-06-19 23:38:30
On 06/15, Jonathan Tan wrote:
quoted
Supporting patterns would mean that we would possibly be able to
eliminate the ls-refs step, thus saving at least a RTT. (Originally I
thought that supporting patterns would also allow us to tolerate refs
being removed during the fetch process, but I see that this is already
handled by the server ignoring "want-ref <ref>" wherein <ref> doesn't
exist on the server.)
What's your opinion on this? Should we keep it how it is in v2 of the
series where the server ignores refs it doesn't know about or revert to
what v1 of the series did and have it be a hard error?
I think it should be like in v2 - the server should ignore "want-ref
<ref>" lines for refs it doesn't know about. And, after more thought, I
think that the client should die if "fetch <exact-ref-name>" was not
fulfilled, and ignore if a ref in "fetch <ref-with-wildcard>" was not
fulfilled.
The advantage of doing that is that we make the protocol a bit more
tolerant to adverse conditions (e.g. a rapidly changing repository or an
eventually consistent load-balancing setup), while having little-to-no
effect on regular conditions.
The disadvantage is that there is now one additional place where a
failure can silently occur, but I think that this is a minor
disadvantage. A naive script using "git fetch", in my mind, would assume
that refs/heads/exact exists if "fetch
refs/heads/exact:refs/heads/exact" succeeds, but would not assume that
refs/heads/wildcard-something exists if "fetch
refs/heads/wildcard*:refs/heads/wildcard*" succeeds, which fits in
nicely with the die/ignore behavior I outlined above.
From: Brandon Williams <hidden> Date: 2018-06-20 21:32:50
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
@@ -48,6 +49,36 @@ static void unpack(void)}}+staticvoidunpack_sideband(void)+{+structpacket_readerreader;+packet_reader_init(&reader,0,NULL,0,+PACKET_READ_GENTLE_ON_EOF|+PACKET_READ_CHOMP_NEWLINE);++while(packet_reader_read(&reader)!=PACKET_READ_EOF){+intband;+intfd;++switch(reader.status){+casePACKET_READ_EOF:+break;+casePACKET_READ_NORMAL:+band=reader.line[0]&0xff;+if(band<1||band>2)+die("unexpected side band %d",band);+fd=band;++write_or_die(fd,reader.line+1,reader.pktlen-1);+break;+casePACKET_READ_FLUSH:+return;+casePACKET_READ_DELIM:+break;+}+}+}+intcmd_main(intargc,constchar**argv){if(argc<2)
From: Brandon Williams <hidden> Date: 2018-06-20 21:32:52
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 7 ++
Documentation/technical/protocol-v2.txt | 28 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 64 ++++++++++
4 files changed, 251 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,21 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -379,6 +392,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.++ * Always begins with the section header "wanted-refs"++ * The server will send a ref listing ("<oid> <refname>") for+ each reference requested using 'want-ref' lines.++ * The server MUST NOT send any refs which were not requested+ using 'want-ref' lines.+ packfile section * This section is only included if the client has sent 'want' lines in its request and either requested that no more
@@ -1210,6 +1243,8 @@ static void process_args(struct packet_reader *request,/* process want */if(parse_want(arg))continue;+if(allow_ref_in_want&&parse_want_ref(arg,&data->wanted_refs))+continue;/* process have line */if(parse_have(arg,&data->haves))continue;
@@ -1352,6 +1387,24 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)returnret;}+staticvoidsend_wanted_ref_info(structupload_pack_data*data)+{+conststructstring_list_item*item;++if(!data->wanted_refs.nr)+return;++packet_write_fmt(1,"wanted-refs\n");++for_each_string_list_item(item,&data->wanted_refs){+packet_write_fmt(1,"%s %s\n",+oid_to_hex(item->util),+item->string);+}++packet_delim(1);+}+staticvoidsend_shallow_info(structupload_pack_data*data){/* No shallow info needs to be sent */
From: Brandon Williams <hidden> Date: 2018-06-20 21:32:53
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +++
t/lib-httpd/one-time-sed.sh | 16 ++++++
t/t5703-upload-pack-ref-in-want.sh | 92 ++++++++++++++++++++++++++++++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have commit for' 'check_output'+."$TEST_DIRECTORY"/lib-httpd.sh+start_httpd++REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"+LOCAL_PRISTINE="$(pwd)/local_pristine"++test_expect_success'setup repos for change-while-negotiating test''+(+gitinit"$REPO"&&+cd"$REPO"&&+>.git/git-daemon-export-ok&&+test_commitm1&&+gittag-dm1&&++# Local repo with many commits (so that negotiation will take+# more than 1 request/response pair)+gitclone"http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo""$LOCAL_PRISTINE"&&+cd"$LOCAL_PRISTINE"&&+gitcheckout-bside&&+foriin$(seq133);dotest_commits$i;done&&++# Add novel commits to upstream+gitcheckoutmaster&&+cd"$REPO"&&+test_commitm2&&+test_commitm3&&+gittag-dm2m3+)&&+git-C"$LOCAL_PRISTINE"remoteset-urlorigin"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"&&+git-C"$LOCAL_PRISTINE"configprotocol.version2+'++inconsistency(){+# Simulate that the server initially reports $2 as the ref+# corresponding to $1, and after that, $1 as the ref corresponding to+# $1. This corresponds to the real-life situation where the server's+# repository appears to change during negotiation, for example, when+# different servers in a load-balancing arrangement serve (stateless)+# RPCs during a single negotiation.+printf"s/%s/%s/"\+$(git-C"$REPO"rev-parse$1|tr-d"\n")\+$(git-C"$REPO"rev-parse$2|tr-d"\n")\+>"$HTTPD_ROOT_PATH/one-time-sed"+}++test_expect_success'server is initially ahead - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+test_must_failgit-Clocalfetch2>err&&+grep"ERR upload-pack: not our ref"err+'++test_expect_failure'server is initially ahead - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verifymaster>expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_success'server is initially behind - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master^">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_failure'server is initially behind - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++stop_httpd+ test_done
From: Brandon Williams <hidden> Date: 2018-06-20 21:32:57
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,if(!update_head_ok)check_not_current_branch(ref_map);-for(rm=ref_map;rm;rm=rm->next){-if(rm->peer_ref){-structstring_list_item*peer_item=-string_list_lookup(&existing_refs,-rm->peer_ref->name);-if(peer_item){-structobject_id*old_oid=peer_item->util;-oidcpy(&rm->peer_ref->old_oid,old_oid);-}-}-}-if(tags==TAGS_DEFAULT&&autotags)transport_set_option(transport,TRANS_OPT_FOLLOWTAGS,"1");if(prune){
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,}cleanup:-string_list_clear(&existing_refs,1);returnretcode;}
From: Brandon Williams <hidden> Date: 2018-06-20 21:32:58
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)){+if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
From: Brandon Williams <hidden> Date: 2018-06-20 21:33:01
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 52 ++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 26 deletions(-)
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,if(refmap.nr)fetch_refspec=&refmap;else-fetch_refspec=&transport->remote->fetch;+fetch_refspec=&remote->fetch;for(i=0;i<fetch_refspec->nr;i++)get_fetch_map(ref_map,&fetch_refspec->items[i],&oref_tail,1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,die("--refmap option is only meaningful with command-line refspec(s).");}else{/* Use the defaults */-structremote*remote=transport->remote;structbranch*branch=branch_get(NULL);inthas_merge=branch_has_merge_config(branch);if(remote&&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,/* also fetch all tags */get_fetch_map(remote_refs,tag_refspec,&tail,0);elseif(tags==TAGS_DEFAULT&&*autotags)-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);/* Now append any refs to be updated opportunistically: */*tail=orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,structref*ref_map;intautotags=(transport->remote->fetch_tags==1);intretcode=0;+conststructref*remote_refs;+structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){if(transport->remote->fetch_tags==2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,gotocleanup;}-ref_map=get_ref_map(transport,rs,tags,&autotags);+if(rs->nr)+refspec_ref_prefixes(rs,&ref_prefixes);+elseif(transport->remote&&transport->remote->fetch.nr)+refspec_ref_prefixes(&transport->remote->fetch,&ref_prefixes);++if(ref_prefixes.argc&&+(tags==TAGS_SET||(tags==TAGS_DEFAULT&&!rs->nr))){+argv_array_push(&ref_prefixes,"refs/tags/");+}++remote_refs=transport_get_remote_refs(transport,&ref_prefixes);+argv_array_clear(&ref_prefixes);++ref_map=get_ref_map(transport->remote,remote_refs,rs,+tags,&autotags);if(!update_head_ok)check_not_current_branch(ref_map);
@@ -1184,7 +1184,7 @@ static int do_fetch(struct transport *transport,if(tags==TAGS_DEFAULT&&autotags){structref**tail=&ref_map;ref_map=NULL;-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);if(ref_map)backfill_tags(transport,ref_map);free_refs(ref_map);
From: Brandon Williams <hidden> Date: 2018-06-20 21:33:05
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.
Signed-off-by: Brandon Williams <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in want' 'grep"ERR upload-pack: not our ref"err'-test_expect_failure'server is initially ahead - ref in want''+test_expect_success'server is initially ahead - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in want' 'test_cmpexpectedactual'-test_expect_failure'server is initially behind - ref in want''+test_expect_success'server is initially behind - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
From: Brandon Williams <hidden> Date: 2018-06-20 21:33:09
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 <redacted>
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28 ++++++++++++++++++++++++----
fetch-object.c | 2 +-
fetch-pack.c | 17 +++++++++--------
transport-helper.c | 6 ++++--
transport-internal.h | 9 ++++++++-
transport.c | 34 ++++++++++++++++++++++++++++------
transport.h | 3 ++-
8 files changed, 78 insertions(+), 25 deletions(-)
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,intautotags=(transport->remote->fetch_tags==1);intretcode=0;conststructref*remote_refs;+structref*updated_remote_refs=NULL;structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){
@@ -1172,7 +1175,24 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){++if(fetch_refs(transport,ref_map,&updated_remote_refs)){+free_refs(ref_map);+retcode=1;+gotocleanup;+}+if(updated_remote_refs){+/*+*Regenerateref_mapusingtheupdatedremoterefs.Thisis+*toaccountforadditionalinformationwhichmaybeprovided+*bythetransport(e.g.shallowinfo).+*/+free_refs(ref_map);+ref_map=get_ref_map(transport->remote,updated_remote_refs,rs,+tags,&autotags);+free_refs(updated_remote_refs);+}+if(consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
@@ -151,7 +151,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,}staticintfetch_refs_from_bundle(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){structbundle_transport_data*data=transport->data;returnunbundle(&data->header,data->fd,
@@ -287,7 +288,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus}staticintfetch_refs_via_pack(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){intret=0;structgit_transport_data*data=transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport,if(report_unmatched_refs(to_fetch,nr_heads))ret=-1;+if(fetched_refs)+*fetched_refs=refs;+else+free_refs(refs);+free_refs(refs_tmp);-free_refs(refs);free(dest);returnret;}
@@ -1215,19 +1221,31 @@ const struct ref *transport_get_remote_refs(struct transport *transport,returntransport->remote_refs;}-inttransport_fetch_refs(structtransport*transport,structref*refs)+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs){intrc;intnr_heads=0,nr_alloc=0,nr_refs=0;structref**heads=NULL;+structref*nop_head=NULL,**nop_tail=&nop_head;structref*rm;for(rm=refs;rm;rm=rm->next){nr_refs++;if(rm->peer_ref&&!is_null_oid(&rm->old_oid)&&-!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid))+!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid)){+/*+*Theseneedtobereportedasfetched,butwedon't+*actuallyneedtofetchthem.+*/+if(fetched_refs){+structref*nop_ref=copy_ref(rm);+*nop_tail=nop_ref;+nop_tail=&nop_ref->next;+}continue;+}ALLOC_GROW(heads,nr_heads+1,nr_alloc);heads[nr_heads++]=rm;}
@@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)heads[nr_heads++]=rm;}-rc=transport->vtable->fetch(transport,nr_heads,heads);+rc=transport->vtable->fetch(transport,nr_heads,heads,fetched_refs);+if(fetched_refs&&nop_head){+*nop_tail=*fetched_refs;+*fetched_refs=nop_head;+}free(heads);returnrc;
@@ -218,7 +218,8 @@ int transport_push(struct transport *connection,conststructref*transport_get_remote_refs(structtransport*transport,conststructargv_array*ref_prefixes);-inttransport_fetch_refs(structtransport*transport,structref*refs);+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs);voidtransport_unlock_pack(structtransport*transport);inttransport_disconnect(structtransport*transport);char*transport_anonymize_url(constchar*url);
From: Brandon Williams <hidden> Date: 2018-06-20 21:33:14
Changes in v3:
* Discussion seemed to settle on keeping the simplified version of
ref-in-want where the "want-ref" line only accepts full ref names. If
we want to we can add patterns at a later time.
* Reverted back to v1's behavior where requesting a ref that doesn't
exists is a hard error on the server. I went back and forth many
times on what the right thing to do here is and decided that a hard
error works much cleaner for the time being.
* Some typos.
Brandon Williams (8):
test-pkt-line: add unpack-sideband subcommand
upload-pack: implement ref-in-want
upload-pack: test negotiation with changing repository
fetch: refactor the population of peer ref OIDs
fetch: refactor fetch_refs into two functions
fetch: refactor to make function args narrower
fetch-pack: put shallow info in output parameter
fetch-pack: implement ref-in-want
Documentation/config.txt | 7 +
Documentation/technical/protocol-v2.txt | 28 ++-
builtin/clone.c | 4 +-
builtin/fetch.c | 131 ++++++++-----
fetch-object.c | 2 +-
fetch-pack.c | 52 +++--
remote.c | 1 +
remote.h | 1 +
t/helper/test-pkt-line.c | 33 ++++
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +
t/lib-httpd/one-time-sed.sh | 16 ++
t/t5703-upload-pack-ref-in-want.sh | 245 ++++++++++++++++++++++++
transport-helper.c | 6 +-
transport-internal.h | 9 +-
transport.c | 34 +++-
transport.h | 3 +-
upload-pack.c | 64 +++++++
18 files changed, 568 insertions(+), 77 deletions(-)
create mode 100644 t/lib-httpd/one-time-sed.sh
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
--
2.18.0.rc1.244.gcf134e6275-goog
From: Jonathan Nieder <hidden> Date: 2018-06-22 21:12:43
Hi,
Brandon Williams wrote:
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
Neat. It appears that this writes sideband channel 1 (packfile data)
to stdout, sideband channel 2 (progress) to stderr, and sideband
channel 3 (errors) cause the helper to fail. It would have been nice
if a comment or the commit message said that, but it's no reason to
reroll --- the code is clear enough.
I think this is for write_or_die. Makes sense (well, in the same way
as any of the other functions that ended up in cache.h instead of a
more thought-through place do).
The old #includes were problematic, since the caller cannot count on
git-compat-util.h to be the first include of pkt-line.h. See
Documentation/CodingGuidelines "The first #include" for more on this
subject.
[...]
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(&reader, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read(&reader) != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
reader.line[0] is a char. This promotes it to an 'int' and then ANDs
against 0xff, which would ensure it is a positive value. In other
words, this does the same thing as
band = (int) (unsigned char) reader.line[0];
but more concisely.
More importantly, it matches what recv_sideband does. Good.
Reviewed-by: Jonathan Nieder <redacted>
Thanks.
From: Jonathan Nieder <hidden> Date: 2018-06-22 21:26:28
Hi,
Brandon Williams wrote:
[Subject: fetch: refactor fetch_refs into two functions]
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
It's hard to understand the context for this patch based on this
description alone. E.g. is fetch_refs too long to follow? Or are we
about to expand it? Or are we going to use the factored-out subpart
for something else?
[...]
quoted hunk
--- a/builtin/fetch.c+++ b/builtin/fetch.c
@@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)intret=quickfetch(ref_map);if(ret)ret=transport_fetch_refs(transport,ref_map);-if(!ret)-ret|=store_updated_refs(transport->url,-transport->remote->name,-ref_map);+if(ret)+transport_unlock_pack(transport);+returnret;+}
Paraphrasing the old code:
try quickfetch
if that fails, we have to fetch for real
both of the above "lock" a pack using a .keep file to
avoid races
if the fetch succeeded, now update the refs
finally, "unlock" the pack by rm-ing the .keep file
Paraphrasing the new code:
try quickfetch
if that fails, we have to fetch for real
both of the above "lock" a pack using a .keep file to
avoid races
if the fetch failed, "unlock" the pack by rm-ing the
.keep file.
Do I understand correctly that this is preparation for changing the
'update refs' step?
As a minor nit, I think this would be easier to read if we treat
the unlock_pack as a destructor. Something like this:
int ret = quickfetch(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow
* the caller time to update refs to reference the new
* objects.
*/
return 0;
transport_unlock_pack(transport);
return ret;
+
+static int consume_refs(struct transport *transport, struct ref *ref_map)
The name consume_refs doesn't make it clear to me what this function
is going to do. Maybe a function comment can help.
I.e. either the name or a comment would tell me that this is going to
update local refs based on the ref values that the remote end told us.
The rest of the patch looks good.
Thanks and hope that helps,
Jonathan
From: Jonathan Nieder <hidden> Date: 2018-06-22 21:42:46
Brandon Williams wrote:
quoted hunk
--- a/builtin/fetch.c+++ b/builtin/fetch.c
@@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)intret=quickfetch(ref_map);if(ret)ret=transport_fetch_refs(transport,ref_map);-if(!ret)-ret|=store_updated_refs(transport->url,-transport->remote->name,-ref_map);+if(ret)+transport_unlock_pack(transport);+returnret;+}++staticintconsume_refs(structtransport*transport,structref*ref_map)+{+intret=store_updated_refs(transport->url,+transport->remote->name,+ref_map);transport_unlock_pack(transport);returnret;}
[...]
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
Ah, I missed something in my previous reply.
If transport_fetch_refs succeeds and store_updated_refs fails, then in
the old code, transport_unlock_pack would clean up by removing the no
longer needed .keep file. In the new code, that's consume_refs's
responsibility, which I find much nicer. It's probably worth
mentioning that in the commit message as well.
Thanks again,
Jonathan
From: Jonathan Nieder <hidden> Date: 2018-06-22 22:29:09
Hi,
Brandon Williams wrote:
On 06/14, Stefan Beller wrote:
quoted
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams [off-list ref] wrote:
quoted
quoted
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
+ }
The server is documented as MUST NOT send additional refs,
which is fine here, as we'd have no way of storing them anyway.
Do we want to issue a warning, though?
if (!r) /* never break'd */
warning ("server send unexpected line '%s'", reader.line);
Depends, does this warning help out the end user or do you think it
would confuse users to see this and still have their fetch succeed?
I think we'd want to error out instead of warning. That keeps the
spec simple and that way, server implementors will notice early if
they are doing something that clients aren't going to understand
anyway, which would benefit users.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2018-06-22 23:01:25
Hi,
Brandon Williams wrote:
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.
Signed-off-by: Brandon Williams <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
This commit message doesn't tell me what ref-in-want is or is for. Could
it include
A. a pointer to Documentation/technical/protocol-v2.txt, or
B. an example illustrating the effect e.g. using GIT_TRACE_PACKET
or both?
[...]
Not about this patch: it's kind of confusing that the iterator is called
'wants' even though it points into the middle of the list. I would even
be tempted to do
const struct ref *want;
for (want = wants; want; want = want->next) {
It wouldn't make sense to do in this patch, though.
[...]
Stefan mentioned that the spec says
* The server MUST NOT send any refs which were not requested
using 'want-ref' lines.
Can client enforce that? If not, can the spec say SHOULD NOT for the
server and add a MUST describing appropriate client behavior?
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
The spec says
* This section is only included if the client has requested a
ref using a 'want-ref' line and if a packfile section is also
included in the response.
What should happen if the client already has all the relevant objects
(or in other words if there is no packfile to send in the packfile
section)? Is the idea that the client should already have known that
based on the ref advertisement? What if ref values change to put us
in that state between the ls-refs and fetch steps?
[...]
quoted hunk
--- 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;
Sensible. The alternative would be that we check whether the
refname is oid-shaped at want-ref generation time, which would be
unnecessarily complicated.
[...]
quoted hunk
--- a/remote.h+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
Not about this patch: why is this in remote.h instead of ref.h?
From: Jonathan Tan <hidden> Date: 2018-06-25 17:41:04
+ wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs"
Add a period at the end to be consistent with the others.
+ * The server will send a ref listing ("<oid> <refname>") for
+ each reference requested using 'want-ref' lines.
+
+ * The server MUST NOT send any refs which were not requested
+ using 'want-ref' lines.
We might want tag following refs to be included here in the future, but
at that time, I think we can amend this to say that if include-tag-ref
is sent by the user, the server may send additional refs, otherwise the
server must not do so. So this is fine.
The documentation states that the "wanted-refs" section is only sent if
there is at least one "want-ref" from the client, and each "want-ref"
causes one entry to be added to data->wanted_refs, so this is correct.
Thanks - besides adding the period in the documentation, this patch
looks good to me.
From: Jonathan Tan <hidden> Date: 2018-06-25 17:45:21
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
get_ref_map() is only called in one place in builtin/fetch.c, and that
place is in do_fetch(), so moving functionality from do_fetch() to
get_ref_map() is perfectly fine, and also allows tightening of the scope
of the existing_refs variable.
Reviewed-by: Jonathan Tan <redacted>
update_shallow() now takes in a linked list of refs instead of an array.
I see that the translation of this function is straightforward -
occasionally, we need to iterate through the linked list and count up
from 0 at the same time, but that is not a problem.
struct shallow_info *si)
{
struct oid_array ref = OID_ARRAY_INIT;
int *status;
- int i;
+ int i = 0;
Remove the " = 0" - I've verified that it does not need to be there, and
it might inhibit useful "unintialized variable" warnings if others were
to change the code later.
Optional: I would also remove this declaration and declare "int i;" in
each of the blocks that need it.
quoted hunk
static int fetch_refs_via_pack(struct transport *transport,
- int nr_heads, struct ref **to_fetch)
+ int nr_heads, struct ref **to_fetch,
+ struct ref **fetched_refs)
{
int ret = 0;
struct git_transport_data *data = transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport, if (report_unmatched_refs(to_fetch, nr_heads)) ret = -1;+ if (fetched_refs)+ *fetched_refs = refs;+ else+ free_refs(refs);+ free_refs(refs_tmp);- free_refs(refs); free(dest); return ret; }
Instead of just freeing the linked list, we return it if requested by
the client. This makes sense.
quoted hunk
-int transport_fetch_refs(struct transport *transport, struct ref *refs)
+int transport_fetch_refs(struct transport *transport, struct ref *refs,
+ struct ref **fetched_refs)
{
int rc;
int nr_heads = 0, nr_alloc = 0, nr_refs = 0;
struct ref **heads = NULL;
+ struct ref *nop_head = NULL, **nop_tail = &nop_head;
struct ref *rm;
for (rm = refs; rm; rm = rm->next) {
nr_refs++;
if (rm->peer_ref &&
!is_null_oid(&rm->old_oid) &&
- !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid))
+ !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {
+ /*
+ * These need to be reported as fetched, but we don't
+ * actually need to fetch them.
+ */
+ if (fetched_refs) {
+ struct ref *nop_ref = copy_ref(rm);
+ *nop_tail = nop_ref;
+ nop_tail = &nop_ref->next;
+ }
continue;
+ }
ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
heads[nr_heads++] = rm;
}
And sometimes, even if we are merely simulating the fetching of refs, we
still need to report those refs in fetched_refs. This is correct.
I also see that t5703 now passes.
Besides enabling the writing of subsequent patches, I see that this also
makes the API clearer in that the input refs to transport_fetch_refs()
are not overloaded to output shallow information. Other than the " = 0"
change above, this patch looks good to me.
From: Brandon Williams <hidden> Date: 2018-06-25 18:08:55
On 06/22, Jonathan Nieder wrote:
Hi,
Brandon Williams wrote:
quoted
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.
Signed-off-by: Brandon Williams <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
This commit message doesn't tell me what ref-in-want is or is for. Could
it include
A. a pointer to Documentation/technical/protocol-v2.txt, or
B. an example illustrating the effect e.g. using GIT_TRACE_PACKET
or both?
Yeah I can imporve the message here.
quoted
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
Stefan mentioned that the spec says
* The server MUST NOT send any refs which were not requested
using 'want-ref' lines.
Can client enforce that? If not, can the spec say SHOULD NOT for the
server and add a MUST describing appropriate client behavior?
Yeah I can update the docs in an earlier patch.
quoted
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
The spec says
* This section is only included if the client has requested a
ref using a 'want-ref' line and if a packfile section is also
included in the response.
What should happen if the client already has all the relevant objects
(or in other words if there is no packfile to send in the packfile
section)? Is the idea that the client should already have known that
based on the ref advertisement? What if ref values change to put us
in that state between the ls-refs and fetch steps?
I believe the current functionality is that if all wants are already
satisfied by all haves then an empty packfile is sent, so that would
fall under that case.
--
Brandon Williams
update_shallow() now takes in a linked list of refs instead of an array.
I see that the translation of this function is straightforward -
occasionally, we need to iterate through the linked list and count up
from 0 at the same time, but that is not a problem.
quoted
struct shallow_info *si)
{
struct oid_array ref = OID_ARRAY_INIT;
int *status;
- int i;
+ int i = 0;
Remove the " = 0" - I've verified that it does not need to be there, and
it might inhibit useful "unintialized variable" warnings if others were
to change the code later.
Optional: I would also remove this declaration and declare "int i;" in
each of the blocks that need it.
quoted
static int fetch_refs_via_pack(struct transport *transport,
- int nr_heads, struct ref **to_fetch)
+ int nr_heads, struct ref **to_fetch,
+ struct ref **fetched_refs)
{
int ret = 0;
struct git_transport_data *data = transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport, if (report_unmatched_refs(to_fetch, nr_heads)) ret = -1;+ if (fetched_refs)+ *fetched_refs = refs;+ else+ free_refs(refs);+ free_refs(refs_tmp);- free_refs(refs); free(dest); return ret; }
Instead of just freeing the linked list, we return it if requested by
the client. This makes sense.
quoted
-int transport_fetch_refs(struct transport *transport, struct ref *refs)
+int transport_fetch_refs(struct transport *transport, struct ref *refs,
+ struct ref **fetched_refs)
{
int rc;
int nr_heads = 0, nr_alloc = 0, nr_refs = 0;
struct ref **heads = NULL;
+ struct ref *nop_head = NULL, **nop_tail = &nop_head;
struct ref *rm;
for (rm = refs; rm; rm = rm->next) {
nr_refs++;
if (rm->peer_ref &&
!is_null_oid(&rm->old_oid) &&
- !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid))
+ !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {
+ /*
+ * These need to be reported as fetched, but we don't
+ * actually need to fetch them.
+ */
+ if (fetched_refs) {
+ struct ref *nop_ref = copy_ref(rm);
+ *nop_tail = nop_ref;
+ nop_tail = &nop_ref->next;
+ }
continue;
+ }
ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
heads[nr_heads++] = rm;
}
And sometimes, even if we are merely simulating the fetching of refs, we
still need to report those refs in fetched_refs. This is correct.
I also see that t5703 now passes.
Besides enabling the writing of subsequent patches, I see that this also
makes the API clearer in that the input refs to transport_fetch_refs()
are not overloaded to output shallow information. Other than the " = 0"
change above, this patch looks good to me.
Perfect, I'll just drop the " = 0" part (making the diff slightly
smaller)
--
Brandon Williams
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:43
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
@@ -48,6 +49,36 @@ static void unpack(void)}}+staticvoidunpack_sideband(void)+{+structpacket_readerreader;+packet_reader_init(&reader,0,NULL,0,+PACKET_READ_GENTLE_ON_EOF|+PACKET_READ_CHOMP_NEWLINE);++while(packet_reader_read(&reader)!=PACKET_READ_EOF){+intband;+intfd;++switch(reader.status){+casePACKET_READ_EOF:+break;+casePACKET_READ_NORMAL:+band=reader.line[0]&0xff;+if(band<1||band>2)+die("unexpected side band %d",band);+fd=band;++write_or_die(fd,reader.line+1,reader.pktlen-1);+break;+casePACKET_READ_FLUSH:+return;+casePACKET_READ_DELIM:+break;+}+}+}+intcmd_main(intargc,constchar**argv){if(argc<2)
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:47
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 7 ++
Documentation/technical/protocol-v2.txt | 28 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 66 ++++++++++
4 files changed, 253 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,21 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -379,6 +392,19 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.++ * Always begins with the section header "wanted-refs"++ * The server will send a ref listing ("<oid> <refname>") for+ each reference requested using 'want-ref' lines.++ * The server SHOULD NOT send any refs which were not requested+ using 'want-ref' lines.+ packfile section * This section is only included if the client has sent 'want' lines in its request and either requested that no more
@@ -1210,6 +1245,8 @@ static void process_args(struct packet_reader *request,/* process want */if(parse_want(arg))continue;+if(allow_ref_in_want&&parse_want_ref(arg,&data->wanted_refs))+continue;/* process have line */if(parse_have(arg,&data->haves))continue;
@@ -1352,6 +1389,24 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)returnret;}+staticvoidsend_wanted_ref_info(structupload_pack_data*data)+{+conststructstring_list_item*item;++if(!data->wanted_refs.nr)+return;++packet_write_fmt(1,"wanted-refs\n");++for_each_string_list_item(item,&data->wanted_refs){+packet_write_fmt(1,"%s %s\n",+oid_to_hex(item->util),+item->string);+}++packet_delim(1);+}+staticvoidsend_shallow_info(structupload_pack_data*data){/* No shallow info needs to be sent */
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:51
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,if(!update_head_ok)check_not_current_branch(ref_map);-for(rm=ref_map;rm;rm=rm->next){-if(rm->peer_ref){-structstring_list_item*peer_item=-string_list_lookup(&existing_refs,-rm->peer_ref->name);-if(peer_item){-structobject_id*old_oid=peer_item->util;-oidcpy(&rm->peer_ref->old_oid,old_oid);-}-}-}-if(tags==TAGS_DEFAULT&&autotags)transport_set_option(transport,TRANS_OPT_FOLLOWTAGS,"1");if(prune){
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,}cleanup:-string_list_clear(&existing_refs,1);returnretcode;}
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:52
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)if(ret)ret=transport_fetch_refs(transport,ref_map);if(!ret)-ret|=store_updated_refs(transport->url,-transport->remote->name,-ref_map);+/*+*Keepthenewpack's".keep"filearoundtoallowthecaller+*timetoupdaterefstoreferencethenewobjects.+*/+return0;+transport_unlock_pack(transport);+returnret;+}++/* Update local refs based on the ref values fetched from a remote */+staticintconsume_refs(structtransport*transport,structref*ref_map)+{+intret=store_updated_refs(transport->url,+transport->remote->name,+ref_map);transport_unlock_pack(transport);returnret;}
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)){+if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:56
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 52 ++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 26 deletions(-)
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,if(refmap.nr)fetch_refspec=&refmap;else-fetch_refspec=&transport->remote->fetch;+fetch_refspec=&remote->fetch;for(i=0;i<fetch_refspec->nr;i++)get_fetch_map(ref_map,&fetch_refspec->items[i],&oref_tail,1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,die("--refmap option is only meaningful with command-line refspec(s).");}else{/* Use the defaults */-structremote*remote=transport->remote;structbranch*branch=branch_get(NULL);inthas_merge=branch_has_merge_config(branch);if(remote&&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,/* also fetch all tags */get_fetch_map(remote_refs,tag_refspec,&tail,0);elseif(tags==TAGS_DEFAULT&&*autotags)-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);/* Now append any refs to be updated opportunistically: */*tail=orefs;
@@ -1143,6 +1127,8 @@ static int do_fetch(struct transport *transport,structref*ref_map;intautotags=(transport->remote->fetch_tags==1);intretcode=0;+conststructref*remote_refs;+structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){if(transport->remote->fetch_tags==2)
@@ -1158,7 +1144,21 @@ static int do_fetch(struct transport *transport,gotocleanup;}-ref_map=get_ref_map(transport,rs,tags,&autotags);+if(rs->nr)+refspec_ref_prefixes(rs,&ref_prefixes);+elseif(transport->remote&&transport->remote->fetch.nr)+refspec_ref_prefixes(&transport->remote->fetch,&ref_prefixes);++if(ref_prefixes.argc&&+(tags==TAGS_SET||(tags==TAGS_DEFAULT&&!rs->nr))){+argv_array_push(&ref_prefixes,"refs/tags/");+}++remote_refs=transport_get_remote_refs(transport,&ref_prefixes);+argv_array_clear(&ref_prefixes);++ref_map=get_ref_map(transport->remote,remote_refs,rs,+tags,&autotags);if(!update_head_ok)check_not_current_branch(ref_map);
@@ -1190,7 +1190,7 @@ static int do_fetch(struct transport *transport,if(tags==TAGS_DEFAULT&&autotags){structref**tail=&ref_map;ref_map=NULL;-find_non_local_tags(transport,&ref_map,&tail);+find_non_local_tags(remote_refs,&ref_map,&tail);if(ref_map)backfill_tags(transport,ref_map);free_refs(ref_map);
From: Brandon Williams <hidden> Date: 2018-06-25 18:53:59
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 <redacted>
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28 ++++++++++++++++++++++++----
fetch-object.c | 2 +-
fetch-pack.c | 15 ++++++++-------
transport-helper.c | 6 ++++--
transport-internal.h | 9 ++++++++-
transport.c | 34 ++++++++++++++++++++++++++++------
transport.h | 3 ++-
8 files changed, 77 insertions(+), 24 deletions(-)
@@ -1128,6 +1130,7 @@ static int do_fetch(struct transport *transport,intautotags=(transport->remote->fetch_tags==1);intretcode=0;conststructref*remote_refs;+structref*updated_remote_refs=NULL;structargv_arrayref_prefixes=ARGV_ARRAY_INIT;if(tags==TAGS_DEFAULT){
@@ -1178,7 +1181,24 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){++if(fetch_refs(transport,ref_map,&updated_remote_refs)){+free_refs(ref_map);+retcode=1;+gotocleanup;+}+if(updated_remote_refs){+/*+*Regenerateref_mapusingtheupdatedremoterefs.Thisis+*toaccountforadditionalinformationwhichmaybeprovided+*bythetransport(e.g.shallowinfo).+*/+free_refs(ref_map);+ref_map=get_ref_map(transport->remote,updated_remote_refs,rs,+tags,&autotags);+free_refs(updated_remote_refs);+}+if(consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;
@@ -151,7 +151,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,}staticintfetch_refs_from_bundle(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){structbundle_transport_data*data=transport->data;returnunbundle(&data->header,data->fd,
@@ -287,7 +288,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus}staticintfetch_refs_via_pack(structtransport*transport,-intnr_heads,structref**to_fetch)+intnr_heads,structref**to_fetch,+structref**fetched_refs){intret=0;structgit_transport_data*data=transport->data;
@@ -354,8 +356,12 @@ static int fetch_refs_via_pack(struct transport *transport,if(report_unmatched_refs(to_fetch,nr_heads))ret=-1;+if(fetched_refs)+*fetched_refs=refs;+else+free_refs(refs);+free_refs(refs_tmp);-free_refs(refs);free(dest);returnret;}
@@ -1215,19 +1221,31 @@ const struct ref *transport_get_remote_refs(struct transport *transport,returntransport->remote_refs;}-inttransport_fetch_refs(structtransport*transport,structref*refs)+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs){intrc;intnr_heads=0,nr_alloc=0,nr_refs=0;structref**heads=NULL;+structref*nop_head=NULL,**nop_tail=&nop_head;structref*rm;for(rm=refs;rm;rm=rm->next){nr_refs++;if(rm->peer_ref&&!is_null_oid(&rm->old_oid)&&-!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid))+!oidcmp(&rm->peer_ref->old_oid,&rm->old_oid)){+/*+*Theseneedtobereportedasfetched,butwedon't+*actuallyneedtofetchthem.+*/+if(fetched_refs){+structref*nop_ref=copy_ref(rm);+*nop_tail=nop_ref;+nop_tail=&nop_ref->next;+}continue;+}ALLOC_GROW(heads,nr_heads+1,nr_alloc);heads[nr_heads++]=rm;}
@@ -1245,7 +1263,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)heads[nr_heads++]=rm;}-rc=transport->vtable->fetch(transport,nr_heads,heads);+rc=transport->vtable->fetch(transport,nr_heads,heads,fetched_refs);+if(fetched_refs&&nop_head){+*nop_tail=*fetched_refs;+*fetched_refs=nop_head;+}free(heads);returnrc;
@@ -218,7 +218,8 @@ int transport_push(struct transport *connection,conststructref*transport_get_remote_refs(structtransport*transport,conststructargv_array*ref_prefixes);-inttransport_fetch_refs(structtransport*transport,structref*refs);+inttransport_fetch_refs(structtransport*transport,structref*refs,+structref**fetched_refs);voidtransport_unlock_pack(structtransport*transport);inttransport_disconnect(structtransport*transport);char*transport_anonymize_url(constchar*url);
From: Brandon Williams <hidden> Date: 2018-06-25 18:54:04
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 <redacted>
---
fetch-pack.c | 35 +++++++++++++++++++++++++++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in want' 'grep"ERR upload-pack: not our ref"err'-test_expect_failure'server is initially ahead - ref in want''+test_expect_success'server is initially ahead - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in want' 'test_cmpexpectedactual'-test_expect_failure'server is initially behind - ref in want''+test_expect_success'server is initially behind - ref in want''git-C"$REPO"configuploadpack.allowRefInWanttrue&&rm-rflocal&&cp-r"$LOCAL_PRISTINE"local&&
From: Brandon Williams <hidden> Date: 2018-06-25 18:54:05
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams <redacted>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 8 +++
t/lib-httpd/one-time-sed.sh | 16 ++++++
t/t5703-upload-pack-ref-in-want.sh | 92 ++++++++++++++++++++++++++++++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have commit for' 'check_output'+."$TEST_DIRECTORY"/lib-httpd.sh+start_httpd++REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"+LOCAL_PRISTINE="$(pwd)/local_pristine"++test_expect_success'setup repos for change-while-negotiating test''+(+gitinit"$REPO"&&+cd"$REPO"&&+>.git/git-daemon-export-ok&&+test_commitm1&&+gittag-dm1&&++# Local repo with many commits (so that negotiation will take+# more than 1 request/response pair)+gitclone"http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo""$LOCAL_PRISTINE"&&+cd"$LOCAL_PRISTINE"&&+gitcheckout-bside&&+foriin$(seq133);dotest_commits$i;done&&++# Add novel commits to upstream+gitcheckoutmaster&&+cd"$REPO"&&+test_commitm2&&+test_commitm3&&+gittag-dm2m3+)&&+git-C"$LOCAL_PRISTINE"remoteset-urlorigin"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo"&&+git-C"$LOCAL_PRISTINE"configprotocol.version2+'++inconsistency(){+# Simulate that the server initially reports $2 as the ref+# corresponding to $1, and after that, $1 as the ref corresponding to+# $1. This corresponds to the real-life situation where the server's+# repository appears to change during negotiation, for example, when+# different servers in a load-balancing arrangement serve (stateless)+# RPCs during a single negotiation.+printf"s/%s/%s/"\+$(git-C"$REPO"rev-parse$1|tr-d"\n")\+$(git-C"$REPO"rev-parse$2|tr-d"\n")\+>"$HTTPD_ROOT_PATH/one-time-sed"+}++test_expect_success'server is initially ahead - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+test_must_failgit-Clocalfetch2>err&&+grep"ERR upload-pack: not our ref"err+'++test_expect_failure'server is initially ahead - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster1234567890123456789012345678901234567890&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verifymaster>expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_success'server is initially behind - no ref in want''+git-C"$REPO"configuploadpack.allowRefInWantfalse&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master^">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++test_expect_failure'server is initially behind - ref in want''+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+rm-rflocal&&+cp-r"$LOCAL_PRISTINE"local&&+inconsistencymaster"master^"&&+git-Clocalfetch&&++git-C"$REPO"rev-parse--verify"master">expected&&+git-Clocalrev-parse--verifyrefs/remotes/origin/master>actual&&+test_cmpexpectedactual+'++stop_httpd+ test_done
Add an explanatory comment somewhere (maybe, at the beginning),
something like:
If "one-time-sed" exists in $HTTPD_ROOT_PATH, run sed on the HTTP
response, using the contents of "one-time-sed" as the sed command to
be run. If the response was modified as a result, delete
"one-time-sed" so that subsequent HTTP responses are no longer
modified.
This can be used to simulate the effects of the repository changing in
between HTTP request-response pairs.
+test_expect_failure 'server is initially ahead - ref in want' '
[snip]
+test_expect_failure 'server is initially behind - ref in want' '
[snip]
These are test_expect_failure, I assume because the fetch part has not
been implemented yet. Can this be moved to the end of the patch set,
once the fetch part has been implemented?
There's also the case of when the server initially has a ref but later
does not - can this be tested here too? The server part is already
covered by the upload-pack test in which we craft a request with a
non-existent ref, but it would be good to test the client part too.
From: Jonathan Tan <hidden> Date: 2018-06-25 22:36:45
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
I see that due to the narrowing of get_ref_map() to take the refs (and
the remote) instead of the whole transport, the computation of the refs
(including computation of the ref prefixes) is also moved from
get_ref_map() to its caller, do_fetch(). As in a previous patch,
get_ref_map() is only used once, so this movement is safe.
Reviewed-by: Jonathan Tan <redacted>
From: Jonathan Tan <hidden> Date: 2018-06-25 23:03:18
Changes in v4 are fairly minor. There are a few documentation changes,
commit message updates, as well as a few small style tweaks based on
reviewer comments.
Patches 4 and 7, which I have commented on previously, look good.
As for patch 2, it still has a missing period in the documentation that
I remarked upon in [1], but I'm not too worried about that. Having said
that, Jonathan Nieder suggested [2]:
Stefan mentioned that the spec says
* The server MUST NOT send any refs which were not requested
using 'want-ref' lines.
Can client enforce that? If not, can the spec say SHOULD NOT for the
server and add a MUST describing appropriate client behavior?
I noticed that you did use "SHOULD NOT" instead of "MUST NOT" - in this
case, you should probably also follow the second part about appropriate
client behavior - it's probably best to document and implement that we
ignore all unwanted refs. But considering this situation, though, I
think it's better to just put "MUST NOT" and have the client enforce
this.
One more thing - I think that the fetch part needs to be tested more. In
particular, test cases similar to that of the upload-pack tests
(multiple ref names, ref name + exact SHA-1), and in addition, handling
of wildcards (for example, a wildcard that expands to nothing and a
wildcard that expands to 2 refs).
[1] https://public-inbox.org/git/20180625174056.53053-1-jonathantanmy@google.com/
[2] https://public-inbox.org/git/20180622230119.GL12013@aiede.svl.corp.google.com/
From: Brandon Williams <hidden> Date: 2018-06-26 20:54:49
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams <redacted>
---
t/helper/test-pkt-line.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
@@ -48,6 +49,36 @@ static void unpack(void)}}+staticvoidunpack_sideband(void)+{+structpacket_readerreader;+packet_reader_init(&reader,0,NULL,0,+PACKET_READ_GENTLE_ON_EOF|+PACKET_READ_CHOMP_NEWLINE);++while(packet_reader_read(&reader)!=PACKET_READ_EOF){+intband;+intfd;++switch(reader.status){+casePACKET_READ_EOF:+break;+casePACKET_READ_NORMAL:+band=reader.line[0]&0xff;+if(band<1||band>2)+die("unexpected side band %d",band);+fd=band;++write_or_die(fd,reader.line+1,reader.pktlen-1);+break;+casePACKET_READ_FLUSH:+return;+casePACKET_READ_DELIM:+break;+}+}+}+intcmd_main(intargc,constchar**argv){if(argc<2)
From: Brandon Williams <hidden> Date: 2018-06-26 20:54:55
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref <ref>" parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams <redacted>
---
Documentation/config.txt | 7 ++
Documentation/technical/protocol-v2.txt | 29 ++++-
t/t5703-upload-pack-ref-in-want.sh | 153 ++++++++++++++++++++++++
upload-pack.c | 66 ++++++++++
4 files changed, 254 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from untrusted repositories).+uploadpack.allowRefInWant::+ If this option is set, `upload-pack` will support the `ref-in-want`+ feature of the protocol version 2 `fetch` command. This feature+ is intended for the benefit of load-balanced servers which may+ not have the same view of what OIDs their refs point to due to+ replication delay.+ url.<base>.insteadOf:: Any URL that starts with this value will be rewritten to start, instead, with <base>. In cases where some site serves a
@@ -299,12 +299,21 @@ included in the client's request: for use with partial clone and partial fetch operations. See `rev-list` for possible "filter-spec" values.+If the 'ref-in-want' feature is advertised, the following argument can+be included in the client's request as well as the potential addition of+the 'wanted-refs' section in the server's response as explained below.++ want-ref <ref>+ Indicates to the server that the client wants to retrieve a+ particular ref, where <ref> is the full name of a ref on the+ server.+ The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. output = *section- section = (acknowledgments | shallow-info | packfile)+ section = (acknowledgments | shallow-info | wanted-refs | packfile) (flush-pkt | delim-pkt) acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -379,6 +392,20 @@ header. * This section is only included if a packfile section is also included in the response.+ wanted-refs section+ * This section is only included if the client has requested a+ ref using a 'want-ref' line and if a packfile section is also+ included in the response.++ * Always begins with the section header "wanted-refs".++ * The server will send a ref listing ("<oid> <refname>") for+ each reference requested using 'want-ref' lines.++ * The server SHOULD NOT send any refs which were not requested+ using 'want-ref' lines and a client MUST ignore refs which+ weren't requested.+ packfile section * This section is only included if the client has sent 'want' lines in its request and either requested that no more
@@ -1210,6 +1245,8 @@ static void process_args(struct packet_reader *request,/* process want */if(parse_want(arg))continue;+if(allow_ref_in_want&&parse_want_ref(arg,&data->wanted_refs))+continue;/* process have line */if(parse_have(arg,&data->haves))continue;
@@ -1352,6 +1389,24 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)returnret;}+staticvoidsend_wanted_ref_info(structupload_pack_data*data)+{+conststructstring_list_item*item;++if(!data->wanted_refs.nr)+return;++packet_write_fmt(1,"wanted-refs\n");++for_each_string_list_item(item,&data->wanted_refs){+packet_write_fmt(1,"%s %s\n",+oid_to_hex(item->util),+item->string);+}++packet_delim(1);+}+staticvoidsend_shallow_info(structupload_pack_data*data){/* No shallow info needs to be sent */
From: Brandon Williams <hidden> Date: 2018-06-26 20:54:59
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This is in preparation
for allowing additional processing of the fetched refs before updating
the local ref store.
Signed-off-by: Brandon Williams <redacted>
---
builtin/fetch.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -968,9 +968,21 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)if(ret)ret=transport_fetch_refs(transport,ref_map);if(!ret)-ret|=store_updated_refs(transport->url,-transport->remote->name,-ref_map);+/*+*Keepthenewpack's".keep"filearoundtoallowthecaller+*timetoupdaterefstoreferencethenewobjects.+*/+return0;+transport_unlock_pack(transport);+returnret;+}++/* Update local refs based on the ref values fetched from a remote */+staticintconsume_refs(structtransport*transport,structref*ref_map)+{+intret=store_updated_refs(transport->url,+transport->remote->name,+ref_map);transport_unlock_pack(transport);returnret;}
@@ -1165,7 +1178,7 @@ static int do_fetch(struct transport *transport,transport->url);}}-if(fetch_refs(transport,ref_map)){+if(fetch_refs(transport,ref_map)||consume_refs(transport,ref_map)){free_refs(ref_map);retcode=1;gotocleanup;