Allow fetch-pack to send ref names (globs allowed)
cover Allow fetch-pack to send ref names (globs allowed)
Hello everyone - this is a proposal for a protocol change to allow the fetch-pack/upload-pack to converse in terms of ref names (globs allowed), and also an implementation of the server (upload-pack) and fetch-from-HTTP client (fetch-pack invoked through fetch).
Negotiation currently happens by upload-pack initially sending a list of refs with names and SHA-1 hashes, and then several request/response pairs in which the request from fetch-pack consists of SHA-1 hashes (selected from the initial list). Allowing the request to consist of names instead of SHA-1 hashes increases tolerance to refs changing (due to time, and due to having load-balanced servers without strong consistency), and is a step towards eliminating the need for the server to send the list of refs first (possibly improving performance).
The protocol is extended by allowing fetch-pack to send "want-ref <name>", where <name> is a full name (refs/...) [1], possibly including glob characters. Upload-pack will inform the client of the refs actually matched by sending "wanted-ref <SHA-1> <name>" before sending the last ACK or NAK.
This patch set is laid out in this way:
1-2:
Upload-pack, protocol documentation, tests that test upload-pack independently. A configuration option is added to control if the "ref-in-want" capability is advertised. (It is always supported even if not advertised - this is so that this feature in multiple load-balanced servers can be switched on or off without needing any atomic switching.)
3:
Mechanism to test a repo that changes over the negotiation (currently, only with the existing mechanism).
4-9:
The current transport mechanism takes in an array of refs which is used as both input and output. Since we are planning to extend the transport mechanism to also allow the specification of ref names (which may include globs, and thus do not have a 1-1 correspondence to refs), refactor to make this parameter to be solely an input parameter.
10-11:
Changes to fetch-pack (which is used by remote-curl) to support "want-ref".
12-13:
Changes to the rest (fetch -> transport -> transport-helper -> remote-curl) to support "want-ref" when fetching from HTTP. The transport fetch function signature has been widened to allow passing in ref names - transports may use those ref names instead of or in addition to refs if they support it. (I chose to preserve refs in the function signature because many parts of Git, including remote helpers, only understand the old functionality anyway, and because precalculating the refs allows some optimizations.)
14:
This is not meant for submission - this is just to show that the tests pass if ref-in-want was advertised by default (except for some newly added tests that explicitly check for the old behavior).
[1] There has been some discussion about whether the server should accept partial ref names, e.g. [2]. In this patch set, I have made the server only accept full names, and it is the responsibility of the client to send the multiple patterns which it wants to match. Quoting from the commit message of the second patch:
For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
[2] <[email protected]>
Jonathan Tan (14):
upload-pack: move parsing of "want" line
upload-pack: allow ref name and glob requests
upload-pack: test negotiation with changing repo
fetch: refactor the population of hashes
fetch: refactor fetch_refs into two functions
fetch: refactor to make function args narrower
fetch-pack: put shallow info in out param
fetch-pack: check returned refs for matches
transport: put ref oid in out param
fetch-pack: support partial names and globs
fetch-pack: support want-ref
fetch-pack: do not printf after closing stdout
fetch: send want-ref and receive fetched refs DONT USE advertise_ref_in_want=1
Documentation/technical/http-protocol.txt | 20 +- Documentation/technical/pack-protocol.txt | 24 +- Documentation/technical/protocol-capabilities.txt | 6 + builtin/clone.c | 16 +- builtin/fetch-pack.c | 64 ++-- builtin/fetch.c | 178 +++++++--- fetch-pack.c | 226 +++++++++---- fetch-pack.h | 6 +- remote-curl.c | 91 +++-- remote.c | 67 +++- remote.h | 20 +- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 8 + t/t5500-fetch-pack.sh | 82 +++++ t/t5536-fetch-conflicts.sh | 2 + t/t5552-upload-pack-ref-in-want.sh | 386 ++++++++++++++++++++++ transport-helper.c | 105 ++++-- transport.c | 40 ++- transport.h | 23 +- upload-pack.c | 117 +++++-- 21 files changed, 1232 insertions(+), 258 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5552-upload-pack-ref-in-want.sh
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 1/14 upload-pack: move parsing of "want" line
Refactor to parse "want" lines when the prefix is found. This makes it easier to add support for another prefix, which will be done in a subsequent commit.
--- upload-pack.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
upload-pack.c +14 -14
@@ -793,8 +793,20 @@ static void receive_needs(void) deepen_rev_list =1 ;continue ; } -if (!skip_prefix (line,"want " , &arg) || -get_sha1_hex (arg, sha1_buf)) +if (skip_prefix (line,"want " , &arg) && + !get_sha1_hex (arg, sha1_buf)) { + o =parse_object (sha1_buf); +if (!o) +die ("git upload-pack: not our ref %s" , +sha1_to_hex (sha1_buf)); +if (!(o->flags &WANTED )) { + o->flags |=WANTED ; +if (!((allow_unadvertised_object_request &ALLOW_ANY_SHA1 ) ==ALLOW_ANY_SHA1 + ||is_our_ref (o))) + has_non_tip =1 ; +add_object_array (o,NULL , &want_obj); + } + }else die ("git upload-pack: protocol error, " "expected to get sha, not '%s'" , line);
@@ -820,18 +832,6 @@ static void receive_needs(void) no_progress =1 ;if (parse_feature_request (features,"include-tag" )) use_include_tag =1 ; - - o =parse_object (sha1_buf); -if (!o) -die ("git upload-pack: not our ref %s" , -sha1_to_hex (sha1_buf)); -if (!(o->flags &WANTED )) { - o->flags |=WANTED ; -if (!((allow_unadvertised_object_request &ALLOW_ANY_SHA1 ) ==ALLOW_ANY_SHA1 - ||is_our_ref (o))) - has_non_tip =1 ; -add_object_array (o,NULL , &want_obj); - } }/*
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 2/14 upload-pack: allow ref name and glob requests
In reply to Jonathan TanCurrently, while performing packfile negotiation [1], upload-pack allows clients to specify their desired objects only as SHA-1s. This causes:
(a) vulnerability to failure when an object turns non-existent during negotiation, which may happen if, for example, upload-pack is provided by multiple Git servers in a load-balancing arrangement, and
(b) dependence on the server first publishing a list of refs with associated objects.
To eliminate (a) and take a step towards eliminating (b), teach upload-pack to support requests in the form of ref names and globs (in addition to the existing support for SHA-1s) through a new line of the form "want-ref <ref>" where ref is the full name of a ref, a glob pattern, or a SHA-1. At the conclusion of negotiation, the server will write "wanted-ref <SHA-1> <name>" for all requests that have been specified this way.
The server indicates that it supports this feature by advertising the capability "ref-in-want". Advertisement of this capability is by default disabled, but can be enabled through a configuration option.
To be flexible with respect to client needs, the server does not indicate an error if a "want-ref" line corresponds to no refs, but instead relies on the client to ensure that what the user needs has been fetched. For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
[1] pack-protocol.txt
--- Documentation/technical/http-protocol.txt | 20 +- Documentation/technical/pack-protocol.txt | 24 +- Documentation/technical/protocol-capabilities.txt | 6 + t/t5552-upload-pack-ref-in-want.sh | 295 ++++++++++++++++++++++ upload-pack.c | 89 ++++++- 5 files changed, 411 insertions(+), 23 deletions(-) create mode 100755 t/t5552-upload-pack-ref-in-want.sh
Documentation/technical/http-protocol.txt +14 -6
@@ -316,7 +316,8 @@ to prevent caching of the response. Servers SHOULD support all capabilities defined here. -Clients MUST send at least one "want" command in the request body. +Clients MUST send at least one "want" or "want-ref" command in the +request body. Clients MUST NOT reference an id in a "want" command which did not appear in the response obtained through ref discovery unless the server advertises capability `allow-tip-sha1-in-want` or
@@ -330,7 +331,7 @@ server advertises capability `allow-tip-sha1-in-want` or want_list = PKT-LINE(want NUL cap_list LF) *(want_pkt) want_pkt = PKT-LINE(want LF) - want = "want" SP id + want = "want" SP id / "want-ref" SP name cap_list = *(SP capability) SP have_list = *PKT-LINE("have" SP id LF)
@@ -352,7 +353,8 @@ C: Build an empty set, `common`, to hold the objects that are later determined to be on both ends. C: Build a set, `want`, of the objects from `advertised` the client - wants to fetch, based on what it saw during ref discovery. + wants to fetch, based on what it saw during ref discovery. This is to + be used if the server does not support the ref-in-want capability. C: Start a queue, `c_pending`, ordered by commit time (popping newest first). Add all client refs. When a commit is popped from
@@ -363,8 +365,8 @@ C: Start a queue, `c_pending`, ordered by commit time (popping newest C: Send one `$GIT_URL/git-upload-pack` request: - C: 0032want <want #1>............................... - C: 0032want <want #2>............................... + C: <size>want-ref <want #1>............................... + C: <size>want-ref <want #2>............................... .... C: 0032have <common #1>............................. C: 0032have <common #2>.............................
@@ -384,7 +386,7 @@ the pkt-line value. Commands MUST appear in the following order, if they appear at all in the request stream: -* "want" +* "want" or "want-ref" * "have" The stream is terminated by a pkt-line flush (`0000`).
@@ -393,6 +395,9 @@ A single "want" or "have" command MUST have one hex formatted SHA-1 as its value. Multiple SHA-1s MUST be sent by sending multiple commands. +A "want-ref" command MUST be followed by a ref name (which may include +shell glob characters) or a hex formatted SHA-1. + The `have` list is created by popping the first 32 commits from `c_pending`. Less can be supplied if `c_pending` empties.
@@ -410,6 +415,9 @@ Verify all objects in `want` are directly reachable from refs. The server MAY walk backwards through history or through the reflog to permit slightly stale requests. +Treat "want-ref" requests as if the equivalent 0 or more "want" commands +(according to the server's refs) were given instead. + If no "want" objects are received, send an error: TODO: Define error if no "want" lines are requested.
Documentation/technical/pack-protocol.txt +16 -8
@@ -223,15 +223,20 @@ out of what the server said it could do with the first 'want' line. PKT-LINE("deepen-since" SP timestamp) / PKT-LINE("deepen-not" SP ref) - first-want = PKT-LINE("want" SP obj-id SP capability-list) - additional-want = PKT-LINE("want" SP obj-id) + first-want = PKT-LINE(want SP capability-list) + additional-want = PKT-LINE(want) + + want = "want" SP obj-id / "want-ref" SP name depth = 1*DIGIT ---- -Clients MUST send all the obj-ids it wants from the reference -discovery phase as 'want' lines. Clients MUST send at least one -'want' command in the request body. Clients MUST NOT mention an +Clients may specify the objects it wants by selecting obj-ids from the +refs that have been advertised (using "want") or by specifying ref names +and ref patterns directly (using "want-ref"). If the latter, the server +will return the list of effective refs used using "wanted-ref" lines. +Either way, clients MUST send at least one +'want' or 'want-ref' command in the request body. Clients MUST NOT mention an obj-id in a 'want' command which did not appear in the response obtained through ref discovery.
@@ -249,7 +254,7 @@ complete those commits. Commits whose parents are not received as a result are defined as shallow and marked as such in the server. This information is sent back to the client in the next step. -Once all the 'want's and 'shallow's (and optional 'deepen') are +Once all the wants and 'shallow's (and optional 'deepen') are transferred, clients MUST send a flush-pkt, to tell the server side that it is done sending the list.
@@ -344,7 +349,9 @@ implementation if we have received at least one "ACK %s continue" during a prior round. This helps to ensure that at least one common ancestor is found before we give up entirely. -Once the 'done' line is read from the client, the server will either +Once the 'done' line is read from the client, the server will send all the +refs corresponding to "want-ref" lines from the client (prefixed by +"wanted-ref"), and then either send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the object name of the last commit determined to be common. The server only sends ACK after 'done' if there is at least one common base and multi_ack or
@@ -354,9 +361,10 @@ if there is no common base found. Then the server will start sending its packfile data. ---- - server-response = *ack_multi ack / nak + server-response = *ack_multi *wanted_ref ack / nak ack_multi = PKT-LINE("ACK" SP obj-id ack_status) ack_status = "continue" / "common" / "ready" + wanted_ref = PKT-LINE("wanted-ref" SP obj-id SP name) ack = PKT-LINE("ACK" SP obj-id) nak = PKT-LINE("NAK") ----
Documentation/technical/protocol-capabilities.txt +6 -0
@@ -309,3 +309,9 @@ to accept a signed push certificate, and asks the <nonce> to be included in the push certificate. A send-pack client MUST NOT send a push-cert packet unless the receive-pack server advertises this capability. + +ref-in-want +----------- + +If the upload-pack server advertises this capability, fetch-pack may +send "want-ref" lines.
t/t5552-upload-pack-ref-in-want.sh new file +295 -0
295 lines not on this page · raw message
upload-pack.c +80 -9
162 lines not on this page · raw message
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [RFC 02/14] upload-pack: allow ref name and glob requests
Quoted 9 lines · Jonathan Tan
Currently, while performing packfile negotiation [1], upload-pack allows clients to specify their desired objects only as SHA-1s. This causes:
(a) vulnerability to failure when an object turns non-existent during negotiation, which may happen if, for example, upload-pack is provided by multiple Git servers in a load-balancing arrangement, and
(b) dependence on the server first publishing a list of refs with associated objects.
Quoted 7 lines
To eliminate (a) and take a step towards eliminating (b), teach upload-pack to support requests in the form of ref names and globs (in addition to the existing support for SHA-1s) through a new line of the form "want-ref <ref>" where ref is the full name of a ref, a glob pattern, or a SHA-1. At the conclusion of negotiation, the server will write "wanted-ref <SHA-1> <name>" for all requests that have been specified this way.
I am not sure if this "at the conclusion of" is sensible. It is OK to assume that what the client side has is fixed, and it is probably OK to desire that what the server side has can change, but at the same time, it feels quite fragile to move the goalpost in between.
Stepping back a bit, in an environment that involves multiple server instances that have inconsistent set of refs, can the negotiation even be sensibly and safely implemented? The first server the client contacts may, in response to a "have", say "I do have that commit so you do not have to send its ancestors to me. We found one cut-off point. Please do explore other lines of histories." The next server that concludes the negotiation exchange may not have that commit and will be unable to produce a pack that excludes the objects reachable from that commit---wouldn't that become a problem?
One way to prevent such a problem from hurting clients may be for these multiple server instances to coordinate and make sure they have a shared perception of the common history among them. Some pushes may have come to one instance but may not have propagated to other instances, and such a commit cannot be accepted as usable "have" if the servers anticipate that the final client request would go to any of the servers. Otherwise the multiple server arrangement would not work safely, methinks.
And if the servers are ensuring the safety using such a mechanism, they can use the same mechanism to restrain "faster" instances from sending too fresh state of refs that other instances haven't caught up to, which would mean they can present a consistent set of refs to the client in the first place, no?
So I am not sure if the mechanism to request history by refname instead of the tip commit would help the multi-server environment as advertised. It may help solving other problems, though (e.g. like "somebody pushed to update after the initial advertisement was sent out" which can happen even in a single server environment).
Quoted 3 lines
The server indicates that it supports this feature by advertising the capability "ref-in-want". Advertisement of this capability is by default disabled, but can be enabled through a configuration option.
OK.
Quoted 7 lines
To be flexible with respect to client needs, the server does not indicate an error if a "want-ref" line corresponds to no refs, but instead relies on the client to ensure that what the user needs has been fetched. For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
Cute. This may be one way to implement the DWIM thing within the constraint of eventually wanting to go to "client speaks first, the server does not advertise things the client is not interested in" world.
But at the same time it may end up bloating the set of refs the client asks instead. Instead of receiving the advertisement and then sending one request after picking the matching one from it, the client needs to send "refs/{heads,tags,whatever}/foo".
Re: [RFC 02/14] upload-pack: allow ref name and glob requests
Quoted 19 lines · Junio C Hamano
Quoted 9 lines · Jonathan Tan
Currently, while performing packfile negotiation [1], upload-pack allows clients to specify their desired objects only as SHA-1s. This causes:
(a) vulnerability to failure when an object turns non-existent during negotiation, which may happen if, for example, upload-pack is provided by multiple Git servers in a load-balancing arrangement, and
(b) dependence on the server first publishing a list of refs with associated objects.Quoted 7 lines
To eliminate (a) and take a step towards eliminating (b), teach upload-pack to support requests in the form of ref names and globs (in addition to the existing support for SHA-1s) through a new line of the form "want-ref <ref>" where ref is the full name of a ref, a glob pattern, or a SHA-1. At the conclusion of negotiation, the server will write "wanted-ref <SHA-1> <name>" for all requests that have been specified this way.
Quoted 4 lines
I am not sure if this "at the conclusion of" is sensible. It is OK to assume that what the client side has is fixed, and it is probably OK to desire that what the server side has can change, but at the same time, it feels quite fragile to move the goalpost in between.
Do you have any specific concerns as to this fragility? Peff mentioned some concerns with the client making some decisions based on the initial SHA-1 vs the SHA-1 reported by "wanted-ref", to which I replied [1].
Quoted 9 lines
Stepping back a bit, in an environment that involves multiple server instances that have inconsistent set of refs, can the negotiation even be sensibly and safely implemented? The first server the client contacts may, in response to a "have", say "I do have that commit so you do not have to send its ancestors to me. We found one cut-off point. Please do explore other lines of histories." The next server that concludes the negotiation exchange may not have that commit and will be unable to produce a pack that excludes the objects reachable from that commit---wouldn't that become a problem?
It's true that this patch set wouldn't solve this problem. This problem only occurs when there is a commit that the client knows but only a few of the servers know (maybe because the client just pushed it to one of them). If, for example, the client does not know a commit and only a few of the servers know it (for example, because another user just pushed it), this patch set does help. The latter scenario seems like it would occur relatively commonly.
Quoted 15 lines
One way to prevent such a problem from hurting clients may be for these multiple server instances to coordinate and make sure they have a shared perception of the common history among them. Some pushes may have come to one instance but may not have propagated to other instances, and such a commit cannot be accepted as usable "have" if the servers anticipate that the final client request would go to any of the servers. Otherwise the multiple server arrangement would not work safely, methinks.
And if the servers are ensuring the safety using such a mechanism, they can use the same mechanism to restrain "faster" instances from sending too fresh state of refs that other instances haven't caught up to, which would mean they can present a consistent set of refs to the client in the first place, no?
Quoted 5 lines
So I am not sure if the mechanism to request history by refname instead of the tip commit would help the multi-server environment as advertised. It may help solving other problems, though (e.g. like "somebody pushed to update after the initial advertisement was sent out" which can happen even in a single server environment).
This patch set would solve the problem you describe (whether in a single server environment or the coordination between multiple servers that provides "strong consistency"). (Although it may not be an important problem to solve, since it is probably OK if the client got a "slow" version of the state of the refs.)
Quoted 13 lines
Quoted 7 lines
To be flexible with respect to client needs, the server does not indicate an error if a "want-ref" line corresponds to no refs, but instead relies on the client to ensure that what the user needs has been fetched. For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
Cute. This may be one way to implement the DWIM thing within the constraint of eventually wanting to go to "client speaks first, the server does not advertise things the client is not interested in" world.
Quoted 4 lines
But at the same time it may end up bloating the set of refs the client asks instead. Instead of receiving the advertisement and then sending one request after picking the matching one from it, the client needs to send "refs/{heads,tags,whatever}/foo".
That is true, although I think that the client will typically send only a few ref names (with or without globs), so the request packet is still not that large.
[1] <[email protected]>
Re: [RFC 02/14] upload-pack: allow ref name and glob requests
Quoted 9 lines · Jonathan Tan
Quoted 4 lines
I am not sure if this "at the conclusion of" is sensible. It is OK to assume that what the client side has is fixed, and it is probably OK to desire that what the server side has can change, but at the same time, it feels quite fragile to move the goalpost in between.
Do you have any specific concerns as to this fragility? Peff mentioned some concerns with the client making some decisions based on the initial SHA-1 vs the SHA-1 reported by "wanted-ref", to which I replied [1].
There were two but I think you are aware of both. One is what Peff already mentioned, the client may want to make the decision before going through the negotiation. The other is "moving the goalpost", the history the last server has may violate the view of the history common between the server and the client that is established during the negotiation with previous servers.
RFC 3/14 upload-pack: test negotiation with changing repo
In reply to Jonathan TanMake upload-pack report "not our ref" errors to the client. (If not, the client would be left waiting for a response when the server is already dead.)
Add tests to check the behavior of upload-pack and fetch-pack when upload-pack is served from a changing repository (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 in subsequent commits), and ensures that subsequent commits do not change existing behavior.
As part of this effort, a mechanism to substitute strings in an HTTP response only on the first invocation is added.
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 ++++ t/lib-httpd/one-time-sed.sh | 8 ++++ t/t5552-upload-pack-ref-in-want.sh | 91 ++++++++++++++++++++++++++++++++++++++ upload-pack.c | 6 ++- 5 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 t/lib-httpd/one-time-sed.sh
t/lib-httpd.sh +1 -0
@@ -132,6 +132,7 @@ prepare_httpd() { cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH" install_script broken-smart-http.sh install_script error.sh + install_script one-time-sed.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +8 -0
@@ -106,9 +106,14 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL </LocationMatch> +<LocationMatch /one_time_sed/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL +</LocationMatch> ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error/ error.sh/ +ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
@@ -118,6 +123,9 @@ ScriptAlias /error/ error.sh/ <Files error.sh> Options ExecCGI </Files> +<Files one-time-sed.sh> + Options ExecCGI +</Files> <Files ${GIT_EXEC_PATH}/git-http-backend> Options ExecCGI </Files>
t/lib-httpd/one-time-sed.sh new file +8 -0
@@ -0,0 +1,8 @@ +#!/bin/sh + +if [ -e one-time-sed ]; then + "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)" + rm one-time-sed +else + "$GIT_EXEC_PATH/git-http-backend" +fi
t/t5552-upload-pack-ref-in-want.sh +91 -0
95 lines not on this page · raw message
upload-pack.c +5 -1
@@ -862,9 +862,13 @@ static void receive_needs(struct string_list *wanted_ns_refs) }else if (skip_prefix (line,"want " , &arg) && !get_sha1_hex (arg, sha1_buf)) { o =parse_object (sha1_buf); -if (!o) +if (!o) { +packet_write_fmt (1 , +"ERR upload-pack: not our ref %s" , +sha1_to_hex (sha1_buf));die ("git upload-pack: not our ref %s" ,sha1_to_hex (sha1_buf)); + }if (!(o->flags &WANTED )) { o->flags |=WANTED ; if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [RFC 03/14] upload-pack: test negotiation with changing repo
Quoted 6 lines · Jonathan Tan
t/lib-httpd/one-time-sed.sh new file +0
-0
t/lib-httpd/one-time-sed.sh line 8
Quoted 8 lines
t/lib-httpd/one-time-sed.sh new file +8
-0@@ -0,0 +1,8 @@ +#!/bin/sh + +if [ -e one-time-sed ]; then + "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)" + rm one-time-sed +else + "$GIT_EXEC_PATH/git-http-backend" +fi
CodingGuidelines?
t/lib-httpd/one-time-sed.sh
Quoted 11 lines
t/lib-httpd/one-time-sed.sh new file +11
-0+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"
I'd prefer for the printf'd result to have a final LF (i.e. not leaving the resulting one-time-sed with a final incomplete line). Also, do you really need the pipe to tr-d? Doesn't the result of $(command substitution) omit the final LF anyway?
$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OKQuoted 6 lines
upload-pack.c +0
-0@@ -862,9 +862,13 @@ static void receive_needs(struct string_list *wanted_ns_refs) } else if (skip_prefix(line, "want ", &arg) &&
upload-pack.c static void receive_needs(struct string_list *wanted_ns_refs) · line 871
Quoted 10 lines
upload-pack.c +5
-1!get_sha1_hex(arg, sha1_buf)) { o =parse_object (sha1_buf); -if (!o) +if (!o) { +packet_write_fmt (1 , +"ERR upload-pack: not our ref %s" , +sha1_to_hex (sha1_buf));die ("git upload-pack: not our ref %s" ,sha1_to_hex (sha1_buf)); + }
This somehow looks like a good thing to do even in production. Am I mistaken?
Re: [RFC 03/14] upload-pack: test negotiation with changing repo
Quoted 18 lines · Junio C Hamano
Quoted 6 lines · Jonathan Tan
t/lib-httpd/one-time-sed.sh new file +0
-0t/lib-httpd/one-time-sed.sh line 8
Quoted 8 lines
t/lib-httpd/one-time-sed.sh new file +8
-0@@ -0,0 +1,8 @@ +#!/bin/sh + +if [ -e one-time-sed ]; then + "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)" + rm one-time-sed +else + "$GIT_EXEC_PATH/git-http-backend" +fiCodingGuidelines?
Thanks - done locally and will send out in the next reroll.
Quoted 17 lines
t/lib-httpd/one-time-sed.sh
Quoted 11 lines
t/lib-httpd/one-time-sed.sh new file +11
-0+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"I'd prefer for the printf'd result to have a final LF (i.e. not leaving the resulting one-time-sed with a final incomplete line). Also, do you really need the pipe to tr-d? Doesn't the result of $(command substitution) omit the final LF anyway?
Quoted 3 lines
$ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
1 foo 2 bar 3
OKDone.
Quoted 11 lines
Quoted 11 lines
upload-pack.c +2
-1@@ -862,9 +862,13 @@ static void receive_needs(struct string_list *wanted_ns_refs) }else if (skip_prefix (line,"want " , &arg) && !get_sha1_hex (arg, sha1_buf)) { o =parse_object (sha1_buf); -if (!o) +if (!o) { + packet_write_fmt(1,
Quoted 8 lines
upload-pack.c static void receive_needs(struct string_list *wanted_ns_refs) · line 871
Quoted 5 lines
upload-pack.c +3
-0+"ERR upload-pack: not our ref %s" , + sha1_to_hex(sha1_buf));die ("git upload-pack: not our ref %s" ,sha1_to_hex (sha1_buf)); + }This somehow looks like a good thing to do even in production. Am I mistaken?
Yes, that's true. If this patch set stalls (for whatever reason), I'll spin this off into an independent patch.
Re: [RFC 03/14] upload-pack: test negotiation with changing repo
Quoted 5 lines · Jonathan Tan
Quoted 2 lines
This somehow looks like a good thing to do even in production. Am I mistaken?
Yes, that's true. If this patch set stalls (for whatever reason), I'll spin this off into an independent patch.
... which may be needed.
As to the main goal of this topic, I think the "ask by refname (with glob)" is very good thing to start the "client speaks first" v2 protocol, as it would allow us to reduce the size of the initial advertisement for common cases (i.e. remote.<name>.fetch is likely to list only refs/heads/* on the left hand side of a refspec). And adding this to v1 is probably a good first step to make sure the code that is currently used by v1 protocol exchange that will be shared with the v2 protocols are prepared to be driven by refname without knowing the exact object name until the final round.
PATCH 1/1 upload-pack: report "not our ref" to client
Make upload-pack report "not our ref" errors to the client as an "ERR" line. (If not, the client would be left waiting for a response when the server is already dead.)
---
Thanks, here is the independent patch.
upload-pack.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
upload-pack.c +5 -1
@@ -822,9 +822,13 @@ static void receive_needs(void) use_include_tag =1 ; o =parse_object (sha1_buf); -if (!o) +if (!o) { +packet_write_fmt (1 , +"ERR upload-pack: not our ref %s" , +sha1_to_hex (sha1_buf));die ("git upload-pack: not our ref %s" ,sha1_to_hex (sha1_buf)); + }if (!(o->flags &WANTED )) { o->flags |=WANTED ; if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH] upload-pack: report "not our ref" to client
Thanks.
RFC 4/14 fetch: refactor the population of hashes
In reply to Jonathan TanPopulate SHA-1 ref hashes in get_ref_map instead of do_fetch. Besides tightening scopes of variables in the code, this also prepares for a future patch where get_ref_map is called multiple times within do_fetch.
--- builtin/fetch.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)
builtin/fetch.c +19 -18
@@ -316,6 +316,8 @@ static struct ref *get_ref_map(struct transport *transport,const struct ref *remote_refs =transport_get_remote_refs (transport); +struct string_list existing_refs =STRING_LIST_INIT_DUP ; +if (refspec_count) {struct refspec *fetch_refspec;int fetch_refspec_nr;
@@ -411,7 +413,23 @@ static struct ref *get_ref_map(struct transport *transport, tail = &rm->next; } -return ref_remove_duplicates (ref_map); + ref_map =ref_remove_duplicates (ref_map); + +for_each_ref (add_existing, &existing_refs); +for (rm = ref_map; rm; rm = rm->next) { +if (rm->peer_ref) { +struct string_list_item *peer_item = +string_list_lookup (&existing_refs, + rm->peer_ref->name); +if (peer_item) { +struct object_id *old_oid = peer_item->util; +oidcpy (&rm->peer_ref->old_oid, old_oid); + } + } + } +string_list_clear (&existing_refs,1 ); + +return ref_map; }#define STORE_REF_ERROR_OTHER 1
@@ -1055,14 +1073,10 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)static int do_fetch (struct transport *transport,struct refspec *refs,int ref_count) { -struct string_list existing_refs =STRING_LIST_INIT_DUP ;struct ref *ref_map; -struct ref *rm;int autotags = (transport->remote->fetch_tags ==1 );int retcode =0 ; -for_each_ref (add_existing, &existing_refs); -if (tags ==TAGS_DEFAULT ) {if (transport->remote->fetch_tags ==2 ) tags =TAGS_SET ;
@@ -1084,18 +1098,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) { -struct string_list_item *peer_item = -string_list_lookup (&existing_refs, - rm->peer_ref->name); -if (peer_item) { -struct object_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) {
@@ -1132,7 +1134,6 @@ static int do_fetch(struct transport *transport, } cleanup: -string_list_clear (&existing_refs,1 );return retcode; }
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 5/14 fetch: refactor fetch_refs into two functions
In reply to Jonathan TanRefactor the fetch_refs function into a function that does the fetching of refs and another function that stores them. This prepares for a future patch where some processing may be done between those tasks.
--- builtin/fetch.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
builtin/fetch.c +13 -6
@@ -918,10 +918,16 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)int ret =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); +return ret; +} + +static int consume_refs (struct transport *transport,struct ref *ref_map) +{ +int ret =store_updated_refs (transport->url, + transport->remote->name, + ref_map);transport_unlock_pack (transport);return ret; }
@@ -1062,7 +1068,8 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)transport_set_option (transport,TRANS_OPT_FOLLOWTAGS ,NULL );transport_set_option (transport,TRANS_OPT_DEPTH ,"0" );transport_set_option (transport,TRANS_OPT_DEEPEN_RELATIVE ,NULL ); -fetch_refs (transport, ref_map); +if (!fetch_refs (transport, ref_map)) +consume_refs (transport, ref_map);if (gsecondary) {transport_disconnect (gsecondary);
@@ -1115,7 +1122,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 ; goto cleanup;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 6/14 fetch: refactor to make function args narrower
In reply to Jonathan TanRefactor 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, which will be needed in a future patch.
--- builtin/fetch.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
builtin/fetch.c +12 -11
@@ -220,7 +220,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)return 0 ; } -static void find_non_local_tags (struct transport *transport, +static void find_non_local_tags (const struct ref *refs,struct ref **head,struct ref ***tail) {
@@ -230,7 +230,7 @@ static void find_non_local_tags(struct transport *transport,struct string_list_item *item =NULL ;for_each_ref (add_existing, &existing_refs); -for (ref =transport_get_remote_refs (transport); ref; ref = ref->next) { +for (ref = refs; ref; ref = ref->next) {if (!starts_with (ref->name,"refs/tags/" ))continue ;
@@ -302,7 +302,8 @@ static void find_non_local_tags(struct transport *transport,string_list_clear (&remote_refs,0 ); } -static struct ref *get_ref_map (struct transport *transport, +static struct ref *get_ref_map (const struct remote *remote, +const struct ref *remote_refs,struct refspec *refspecs,int refspec_count,int tags,int *autotags) {
@@ -314,8 +315,6 @@ static struct ref *get_ref_map(struct transport *transport,/* opportunistically-updated references: */ struct ref *orefs =NULL , **oref_tail = &orefs; -const struct ref *remote_refs =transport_get_remote_refs (transport); -struct string_list existing_refs =STRING_LIST_INIT_DUP ;if (refspec_count) {
@@ -355,8 +354,8 @@ static struct ref *get_ref_map(struct transport *transport, fetch_refspec =parse_fetch_refspec (refmap_nr, refmap_array); fetch_refspec_nr = refmap_nr; }else { - fetch_refspec = transport->remote->fetch; - fetch_refspec_nr = transport->remote->fetch_refspec_nr; + fetch_refspec = remote->fetch; + fetch_refspec_nr = remote->fetch_refspec_nr; }for (i =0 ; i < fetch_refspec_nr; i++)
@@ -365,7 +364,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 */ -struct remote *remote = transport->remote;struct branch *branch =branch_get (NULL );int has_merge =branch_has_merge_config (branch); if (remote &&
@@ -404,7 +402,7 @@ static struct ref *get_ref_map(struct transport *transport,/* also fetch all tags */ get_fetch_map (remote_refs, tag_refspec, &tail,0 ); else if (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;
@@ -1083,6 +1081,7 @@ static int do_fetch(struct transport *transport,struct ref *ref_map;int autotags = (transport->remote->fetch_tags ==1 );int retcode =0 ; +const struct ref *remote_refs;if (tags ==TAGS_DEFAULT ) {if (transport->remote->fetch_tags ==2 )
@@ -1101,7 +1100,9 @@ static int do_fetch(struct transport *transport, goto cleanup; } - ref_map =get_ref_map (transport, refs, ref_count, tags, &autotags); + remote_refs =transport_get_remote_refs (transport); + ref_map =get_ref_map (transport->remote, remote_refs, refs, ref_count, + tags, &autotags);if (!update_head_ok)check_not_current_branch (ref_map);
@@ -1134,7 +1135,7 @@ static int do_fetch(struct transport *transport,if (tags ==TAGS_DEFAULT && autotags) {struct ref **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);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 7/14 fetch-pack: put shallow info in out param
In reply to Jonathan TanExpand the transport fetch method signature, by adding an out 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: one generation from the list of refs provided by the remote (as is currently done) and potentially one regeneration from the new list of refs that the fetch mechanism provides (added in this patch). The double generation may result in duplicate error messages when a remote-tracking branch seems to track more than one remote branch.
This is the 1st of 3 patches to eliminate using input refs to communicate information obtained by the fetch mechanism.
--- builtin/clone.c | 4 ++-- builtin/fetch.c | 23 +++++++++++++++++++---- fetch-pack.c | 18 +++++++++++------- remote.c | 12 +++++++++++- remote.h | 1 + t/t5536-fetch-conflicts.sh | 2 ++ transport-helper.c | 5 +++-- transport.c | 32 ++++++++++++++++++++++++++------ transport.h | 12 ++++++++++-- 9 files changed, 85 insertions(+), 24 deletions(-)
builtin/clone.c +2 -2
@@ -1076,7 +1076,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) }if (!is_local && !complete_refs_before_fetch) -transport_fetch_refs (transport, mapped_refs); +transport_fetch_refs (transport, mapped_refs,NULL ); remote_head =find_ref_by_name (refs,"HEAD" ); remote_head_points_at =
@@ -1115,7 +1115,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)if (is_local)clone_local (path, git_dir);else if (refs && complete_refs_before_fetch) -transport_fetch_refs (transport, mapped_refs); +transport_fetch_refs (transport, mapped_refs,NULL );update_remote_refs (refs, mapped_refs, remote_head_points_at, branch_top.buf, reflog_msg.buf, transport, !is_local);
builtin/fetch.c +19 -4
@@ -911,11 +911,13 @@ static int quickfetch(struct ref *ref_map)return check_connected (iterate_ref_map, &rm, &opt); } -static int fetch_refs (struct transport *transport,struct ref *ref_map) +static int fetch_refs (struct transport *transport,struct ref *ref_map, +struct ref **updated_remote_refs) {int ret =quickfetch (ref_map);if (ret) - ret =transport_fetch_refs (transport, ref_map); + ret =transport_fetch_refs (transport, ref_map, + updated_remote_refs);if (ret)transport_unlock_pack (transport);return ret;
@@ -1066,7 +1068,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)transport_set_option (transport,TRANS_OPT_FOLLOWTAGS ,NULL );transport_set_option (transport,TRANS_OPT_DEPTH ,"0" );transport_set_option (transport,TRANS_OPT_DEEPEN_RELATIVE ,NULL ); -if (!fetch_refs (transport, ref_map)) +if (!fetch_refs (transport, ref_map,NULL ))consume_refs (transport, ref_map);if (gsecondary) {
@@ -1082,6 +1084,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 ;if (tags ==TAGS_DEFAULT ) {if (transport->remote->fetch_tags ==2 )
@@ -1123,7 +1126,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 ; + goto cleanup; + } +if (new_remote_refs) { +free_refs (ref_map); + ref_map =get_ref_map (transport->remote, new_remote_refs, + refs, ref_count, tags, &autotags); +free_refs (new_remote_refs); + } + +if (consume_refs (transport, ref_map)) {free_refs (ref_map); retcode =1 ; goto cleanup;
fetch-pack.c +11 -7
@@ -974,12 +974,13 @@ static int remove_duplicates_in_refs(struct ref **ref, int nr) }static void update_shallow (struct fetch_pack_args *args, -struct ref **sought,int nr_sought, +struct ref *refs,struct shallow_info *si) {struct sha1_array ref =SHA1_ARRAY_INIT ;int *status;int i; +struct ref *r;if (args->deepen && alternate_shallow_file) {if (*alternate_shallow_file =='\0' ) {/* --unshallow */
@@ -1021,8 +1022,8 @@ static void update_shallow(struct fetch_pack_args *args,remove_nonexistent_theirs_shallow (si);if (!si->nr_ours && !si->nr_theirs)return ; -for (i =0 ; i < nr_sought; i++) -sha1_array_append (&ref, sought[i]->old_oid.hash); +for (r = refs; r; r = r->next) +sha1_array_append (&ref, r->old_oid.hash); si->ref = &ref;if (args->update_shallow) {
@@ -1056,12 +1057,15 @@ static void update_shallow(struct fetch_pack_args *args,* remote is also shallow, check what ref is safe to update * without updating .git/shallow */ - status =xcalloc (nr_sought,sizeof (*status)); + status =xcalloc (ref.nr,sizeof (*status));assign_shallow_commits_to_refs (si,NULL , status);if (si->nr_ours || si->nr_theirs) { -for (i =0 ; i < nr_sought; i++) + i =0 ; +for (r = refs; r; r = r->next) {if (status[i]) - sought[i]->status =REF_STATUS_REJECT_SHALLOW ; + r->status =REF_STATUS_REJECT_SHALLOW ; + i++; + } }free (status);sha1_array_clear (&ref);
@@ -1090,7 +1094,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, ref_cpy =do_fetch_pack (args, fd, ref, sought, nr_sought, &si, pack_lockfile);reprepare_packed_git (); -update_shallow (args, sought, nr_sought, &si); +update_shallow (args, ref_cpy, &si);clear_shallow_info (&si);return ref_cpy; }
remote.c +11 -1
@@ -929,7 +929,7 @@ struct ref *alloc_ref(const char *name)return alloc_ref_with_prefix ("" ,0 , name); } -struct ref *copy_ref (const struct ref *ref) +struct ref *copy_ref_peerless (const struct ref *ref) {struct ref *cpy;size_t len;
@@ -941,6 +941,16 @@ struct ref *copy_ref(const struct ref *ref) cpy->next =NULL ; cpy->symref =xstrdup_or_null (ref->symref); cpy->remote_status =xstrdup_or_null (ref->remote_status); + cpy->peer_ref =NULL ; +return cpy; +} + +struct ref *copy_ref (const struct ref *ref) +{ +struct ref *cpy; +if (!ref) +return NULL ; + cpy =copy_ref_peerless (ref); cpy->peer_ref =copy_ref (ref->peer_ref);return cpy; }
remote.h +1 -0
@@ -131,6 +131,7 @@ struct ref {extern struct ref *find_ref_by_name (const struct ref *list,const char *name);struct ref *alloc_ref (const char *name); +struct ref *copy_ref_peerless (const struct ref *ref);struct ref *copy_ref (const struct ref *ref);struct ref *copy_ref_list (const struct ref *ref);void sort_ref_list (struct ref **,int (*cmp)(const void *,const void *));
t/t5536-fetch-conflicts.sh +2 -0
@@ -93,6 +93,8 @@ test_expect_success 'fetch conflict: criss-cross args' ' verify_stderr <<-\EOF warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2 warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1 + warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2 + warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1 EOF ) '
transport-helper.c +3 -2
@@ -646,14 +646,15 @@ static int connect_helper(struct transport *transport, const char *name, }static int fetch (struct transport *transport, -int nr_heads,struct ref **to_fetch) +int nr_heads,struct ref **to_fetch,struct ref **fetched_refs) {struct helper_data *data = transport->data;int i, count;if (process_connect (transport,0 )) {do_take_over (transport); -return transport->fetch (transport, nr_heads, to_fetch); +return transport->fetch (transport, nr_heads, to_fetch, + fetched_refs); } count =0 ;
transport.c +26 -6
@@ -95,7 +95,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus }static int fetch_refs_from_bundle (struct transport *transport, -int nr_heads,struct ref **to_fetch) +int nr_heads,struct ref **to_fetch, +struct ref **fetched_refs) {struct bundle_transport_data *data = transport->data; return unbundle(&data->header, data->fd,
@@ -202,7 +203,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus }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) {struct git_transport_data *data = transport->data;struct ref *refs;
@@ -250,8 +252,12 @@ static int fetch_refs_via_pack(struct transport *transport, data->options.self_contained_and_connected = args.self_contained_and_connected; +if (fetched_refs) + *fetched_refs = refs; +else +free_refs (refs); +free_refs (refs_tmp); -free_refs (refs);free (dest);return (refs ?0 :-1 ); }
@@ -1090,19 +1096,29 @@ const struct ref *transport_get_remote_refs(struct transport *transport)return transport->remote_refs; } -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 do not +* actually need to fetch them. */ +if (fetched_refs) { +struct ref *nop_ref =copy_ref_peerless (rm); + *nop_tail = nop_ref; + nop_tail = &nop_ref->next; + }continue ; + }ALLOC_GROW (heads, nr_heads +1 , nr_alloc); heads[nr_heads++] = rm; }
@@ -1120,7 +1136,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) heads[nr_heads++] = rm; } - rc = transport->fetch (transport, nr_heads, heads); + rc = transport->fetch (transport, nr_heads, heads, fetched_refs); +if (nop_head) { + *nop_tail = *fetched_refs; + *fetched_refs = nop_head; + }free (heads);return rc;
transport.h +10 -2
@@ -82,11 +82,18 @@ struct transport {* Fetch the objects for the given refs. Note that this gets * an array, and should ignore the list structure. * +* The transport *may* provide, in fetched_refs, the list of refs that +* it fetched. If the transport knows anything about the fetched refs +* that the caller does not know (for example, shallow status), it +* should provide that list of refs and include that information in the +* list. +* * If the transport did not get hashes for refs in * get_refs_list(), it should set the old_sha1 fields in the * provided refs now. **/ -int (*fetch)(struct transport *transport,int refs_nr,struct ref **refs); +int (*fetch)(struct transport *transport,int refs_nr,struct ref **refs, +struct ref **fetched_refs);/** * Push the objects and refs. Send the necessary objects, and
@@ -230,7 +237,8 @@ int transport_push(struct transport *connection,const struct ref *transport_get_remote_refs (struct transport *transport); -int transport_fetch_refs (struct transport *transport,struct ref *refs); +int transport_fetch_refs (struct transport *transport,struct ref *refs, +struct ref **fetched_refs);void transport_unlock_pack (struct transport *transport);int transport_disconnect (struct transport *transport);char *transport_anonymize_url (const char *url);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 8/14 fetch-pack: check returned refs for matches
In reply to Jonathan TanInstead of setting "matched" on matched refs, detect matches by checking the sought refs against the returned refs. Also, since the "matched" field in struct ref is now no longer used, remove it.
This is the 2nd of 3 patches to eliminate using input refs to communicate information obtained by the fetch mechanism.
(There are possibly ways more efficient than a nested for loop to accomplish this. However, since a subsequent patch will compare the user-provided refspecs against the fetched refs directly, and thus necessitating the nested for loop anyway, I decided to use the nested for loop in this patch.)
--- builtin/fetch-pack.c | 7 ++++++- fetch-pack.c | 9 ++++++--- fetch-pack.h | 2 -- remote.h | 3 +-- 4 files changed, 13 insertions(+), 8 deletions(-)
builtin/fetch-pack.c +6 -1
@@ -4,6 +4,7 @@#include "remote.h" #include "connect.h" #include "sha1-array.h" +#include "refs.h" static const char fetch_pack_usage[] ="git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -220,7 +221,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)* an error. */ for (i =0 ; i < nr_sought; i++) { -if (!sought[i] || sought[i]->matched) +struct ref *r; +for (r = ref; r; r = r->next) +if (!sought[i] ||refname_match (sought[i]->name, r->name)) +break ; +if (r)continue ;error ("no such remote ref %s" , sought[i]->name); ret =1 ;
fetch-pack.c +6 -3
@@ -562,6 +562,7 @@ static void filter_refs(struct fetch_pack_args *args,struct ref **newtail = &newlist;struct ref *ref, *next;int i; +int *matched =xcalloc (nr_sought,sizeof (*matched)); i =0 ;for (ref = *refs; ref; ref = next) {
@@ -578,7 +579,7 @@ static void filter_refs(struct fetch_pack_args *args,break ;/* definitely do not have it */ else if (cmp == 0) { keep =1 ;/* definitely have it */ - sought[i]->matched =1 ; + matched[i] =1 ; } i++; }
@@ -604,19 +605,21 @@ static void filter_refs(struct fetch_pack_args *args,unsigned char sha1[20 ]; ref = sought[i]; -if (ref->matched) +if (matched[i])continue ;if (get_sha1_hex (ref->name, sha1) || ref->name[40 ] !='\0' ||hashcmp (sha1, ref->old_oid.hash))continue ; - ref->matched =1 ; + matched[i] =1 ; *newtail =copy_ref (ref); newtail = &(*newtail)->next; } } *refs = newlist; + +free (matched); }static void mark_alternate_complete (const struct ref *ref,void *unused)
fetch-pack.h +0 -2
@@ -33,8 +33,6 @@ struct fetch_pack_args {/* * sought represents remote references that should be updated from. -* On return, the names that were found on the remote will have been -* marked as such. */ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn,
remote.h +1 -2
@@ -89,8 +89,7 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, - deletion:1, - matched:1; + deletion:1;/* * Order is important here, as we write to FETCH_HEAD
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 9/14 transport: put ref oid in out param
In reply to Jonathan TanReturn new OID information obtained through fetching in new structs instead of reusing the existing ones. With this change, the input structs are no longer used for output, and are thus marked const.
This is the 3rd of 3 patches to eliminate using input refs to communicate information obtained by the fetch mechanism.
--- builtin/clone.c | 14 ++++++++++++-- builtin/fetch-pack.c | 4 ++-- fetch-pack.c | 26 +++++++++++++++----------- fetch-pack.h | 2 +- transport-helper.c | 34 +++++++++++++++++++++++----------- transport.c | 6 +++--- transport.h | 13 +++++-------- 7 files changed, 61 insertions(+), 38 deletions(-)
builtin/clone.c +12 -2
@@ -858,6 +858,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)struct refspec *refspec;const char *fetch_pattern; +struct ref *new_remote_refs =NULL ; +packet_trace_identity ("clone" ); argc =parse_options (argc, argv, prefix, builtin_clone_options, builtin_clone_usage,0 );
@@ -1075,8 +1077,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)break ; } -if (!is_local && !complete_refs_before_fetch) -transport_fetch_refs (transport, mapped_refs,NULL ); +if (!is_local && !complete_refs_before_fetch) { +transport_fetch_refs (transport, mapped_refs, + &new_remote_refs); +if (new_remote_refs) { + refs = new_remote_refs; +free_refs (mapped_refs); + mapped_refs =wanted_peer_refs (refs, refspec); + } + } remote_head =find_ref_by_name (refs,"HEAD" ); remote_head_points_at =
@@ -1148,5 +1157,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) junk_mode =JUNK_LEAVE_ALL ;free (refspec); +free_refs (new_remote_refs);return err; }
builtin/fetch-pack.c +2 -2
@@ -11,7 +11,7 @@ static const char fetch_pack_usage[] ="[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] " "[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]" ; -static void add_sought_entry (struct ref ***sought,int *nr,int *alloc, +static void add_sought_entry (const struct ref ***sought,int *nr,int *alloc,const char *name) {struct ref *ref;
@@ -44,7 +44,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)int i, ret;struct ref *ref =NULL ;const char *dest =NULL ; -struct ref **sought =NULL ; +const struct ref **sought =NULL ;int nr_sought =0 , alloc_sought =0 ;int fd[2 ];char *pack_lockfile =NULL ;
fetch-pack.c +15 -11
@@ -52,6 +52,10 @@ static int non_common_revs, multi_ack, use_sideband;#define ALLOW_REACHABLE_SHA1 02static unsigned int allow_unadvertised_object_request; +/* An arbitrary non-NULL non-const pointer to assign to the util field of +* string list items when we need one. */ +#define ARBITRARY (&transfer_unpack_limit) + __attribute__((format (printf,2 ,3 )))static inline void print_verbose (const struct fetch_pack_args *args,const char *fmt, ...)
@@ -556,7 +560,7 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,static void filter_refs (struct fetch_pack_args *args,struct ref **refs, -struct ref **sought,int nr_sought) +const struct ref **sought,int nr_sought) {struct ref *newlist =NULL ;struct ref **newtail = &newlist;
@@ -604,16 +608,16 @@ static void filter_refs(struct fetch_pack_args *args,for (i =0 ; i < nr_sought; i++) {unsigned char sha1[20 ]; - ref = sought[i]; +const struct ref *sref = sought[i];if (matched[i])continue ; -if (get_sha1_hex (ref->name, sha1) || - ref->name[40 ] !='\0' || -hashcmp (sha1, ref->old_oid.hash)) +if (get_sha1_hex (sref->name, sha1) || + sref->name[40 ] !='\0' || +hashcmp (sha1, sref->old_oid.hash))continue ; matched[i] =1 ; - *newtail =copy_ref (ref); + *newtail =copy_ref (sref); newtail = &(*newtail)->next; } }
@@ -629,7 +633,7 @@ static void mark_alternate_complete(const struct ref *ref, void *unused)static int everything_local (struct fetch_pack_args *args,struct ref **refs, -struct ref **sought,int nr_sought) +const struct ref **sought,int nr_sought) {struct ref *ref;int retval;
@@ -831,7 +835,7 @@ static int cmp_ref_by_name(const void *a_, const void *b_)static struct ref *do_fetch_pack (struct fetch_pack_args *args,int fd[2 ],const struct ref *orig_ref, -struct ref **sought,int nr_sought, +const struct ref **sought,int nr_sought,struct shallow_info *si,char **pack_lockfile) {
@@ -955,7 +959,7 @@ static void fetch_pack_setup(void) did_setup =1 ; } -static int remove_duplicates_in_refs (struct ref **ref,int nr) +static int remove_duplicates_in_refs (const struct ref **ref,int nr) {struct string_list names =STRING_LIST_INIT_NODUP ;int src, dst;
@@ -965,7 +969,7 @@ static int remove_duplicates_in_refs(struct ref **ref, int nr) item =string_list_insert (&names, ref[src]->name);if (item->util)continue ;/* already have it */ - item->util = ref[src]; + item->util =ARBITRARY ;if (src != dst) ref[dst] = ref[src]; dst++;
@@ -1078,7 +1082,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, const char *dest, - struct ref **sought, int nr_sought, + const struct ref **sought, int nr_sought, struct sha1_array *shallow, char **pack_lockfile) {
fetch-pack.h +1 -1
@@ -38,7 +38,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, const char *dest, - struct ref **sought, + const struct ref **sought, int nr_sought, struct sha1_array *shallow, char **pack_lockfile);
transport-helper.c +23 -11
@@ -384,7 +384,7 @@ static int release_helper(struct transport *transport) }static int fetch_with_fetch (struct transport *transport, -int nr_heads,struct ref **to_fetch) +int nr_heads,const struct ref **to_fetch) {struct helper_data *data = transport->data;int i;
@@ -477,13 +477,14 @@ static int get_exporter(struct transport *transport, }static int fetch_with_import (struct transport *transport, -int nr_heads,struct ref **to_fetch) +int nr_heads,const struct ref **to_fetch,struct ref **fetched_refs) {struct child_process fastimport;struct helper_data *data = transport->data;int i; -struct ref *posn; +const struct ref *posn;struct strbuf buf =STRBUF_INIT ; +struct ref *fr_head =NULL , **fr_tail = &fr_head;get_helper (transport);
@@ -522,28 +523,38 @@ static int fetch_with_import(struct transport *transport,* (If no "refspec" capability was specified, for historical * reasons we default to the equivalent of *:*.) * -* Store the result in to_fetch[i].old_sha1. Callers such +* Store the result in old_oid in fetched_refs. Callers such * as "git fetch" can use the value to write feedback to the * terminal, populate FETCH_HEAD, and determine what new value * should be written to peer_ref if the update is a * fast-forward or this is a forced update. */ +if (!fetched_refs) + goto cleanup;for (i =0 ; i < nr_heads; i++) { -char *private, *name; - posn = to_fetch[i]; -if (posn->status &REF_STATUS_UPTODATE ) +struct ref *ref; +char *private; +const char *name; + + ref =copy_ref_peerless (to_fetch[i]); + *fr_tail = ref; + fr_tail = &ref->next; +if (ref->status &REF_STATUS_UPTODATE )continue ; - name = posn->symref ? posn->symref : posn->name; + name = ref->symref ? ref->symref : ref->name;if (data->refspecs) private =apply_refspecs (data->refspecs, data->refspec_nr, name);else private =xstrdup (name);if (private) { -if (read_ref (private, posn->old_oid.hash) <0 ) +if (read_ref (private, ref->old_oid.hash) <0 )die ("Could not read ref %s" , private);free (private); } } + *fetched_refs = fr_head; + +cleanup:strbuf_release (&buf);return 0 ; }
@@ -646,7 +657,8 @@ static int connect_helper(struct transport *transport, const char *name, }static int fetch (struct transport *transport, -int nr_heads,struct ref **to_fetch,struct ref **fetched_refs) +int nr_heads,const struct ref **to_fetch, +struct ref **fetched_refs) {struct helper_data *data = transport->data;int i, count;
@@ -679,7 +691,7 @@ static int fetch(struct transport *transport,return fetch_with_fetch (transport, nr_heads, to_fetch);if (data->import) -return fetch_with_import (transport, nr_heads, to_fetch); +return fetch_with_import (transport, nr_heads, to_fetch, fetched_refs);return -1 ; }
transport.c +3 -3
@@ -95,7 +95,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus }static int fetch_refs_from_bundle (struct transport *transport, -int nr_heads,struct ref **to_fetch, +int nr_heads,const struct ref **to_fetch,struct ref **fetched_refs) {struct bundle_transport_data *data = transport->data;
@@ -203,7 +203,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus }static int fetch_refs_via_pack (struct transport *transport, -int nr_heads,struct ref **to_fetch, +int nr_heads,const struct ref **to_fetch,struct ref **fetched_refs) {struct git_transport_data *data = transport->data;
@@ -1101,7 +1101,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs, {int rc;int nr_heads =0 , nr_alloc =0 , nr_refs =0 ; -struct ref **heads =NULL ; +const struct ref **heads =NULL ;struct ref *nop_head =NULL , **nop_tail = &nop_head;struct ref *rm;
transport.h +5 -8
@@ -84,15 +84,12 @@ struct transport {* * The transport *may* provide, in fetched_refs, the list of refs that * it fetched. If the transport knows anything about the fetched refs -* that the caller does not know (for example, shallow status), it -* should provide that list of refs and include that information in the -* list. -* -* If the transport did not get hashes for refs in -* get_refs_list(), it should set the old_sha1 fields in the -* provided refs now. +* that the caller does not know (for example, shallow status or ref +* hashes), it should provide that list of refs and include that +* information in the list. **/ -int (*fetch)(struct transport *transport,int refs_nr,struct ref **refs, +int (*fetch)(struct transport *transport, +int refs_nr,const struct ref **refs,struct ref **fetched_refs);/**
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 10/14 fetch-pack: support partial names and globs
In reply to Jonathan TanTeach fetch-pack to support partial ref names and ref patterns as input.
This does not use "want-ref" yet - support for that will be added in a future patch.
--- builtin/fetch-pack.c | 40 ++++++++++++------------------------- remote.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++ remote.h | 16 +++++++++++++++ t/t5500-fetch-pack.sh | 38 +++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 27 deletions(-)
builtin/fetch-pack.c +13 -27
@@ -11,32 +11,12 @@ static const char fetch_pack_usage[] ="[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] " "[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]" ; -static void add_sought_entry (const struct ref ***sought,int *nr,int *alloc, +static void add_sought_entry (struct refspec **sought,int *nr,int *alloc,const char *name) { -struct ref *ref; -struct object_id oid; - -if (!get_oid_hex (name, &oid)) { -if (name[GIT_SHA1_HEXSZ ] ==' ' ) { -/* <sha1> <ref>, find refname */ - name +=GIT_SHA1_HEXSZ +1 ; - }else if (name[GIT_SHA1_HEXSZ ] =='\0' ) { - ;/* <sha1>, leave sha1 as name */ - }else { -/* <ref>, clear cruft from oid */ -oidclr (&oid); - } - }else { -/* <ref>, clear cruft from get_oid_hex */ -oidclr (&oid); - } - - ref =alloc_ref (name); -oidcpy (&ref->old_oid, &oid); (*nr)++;ALLOC_GROW (*sought, *nr, *alloc); - (*sought)[*nr -1 ] = ref; +parse_ref_or_pattern (&(*sought)[*nr -1 ], name); }int cmd_fetch_pack (int argc,const char **argv,const char *prefix)
@@ -44,8 +24,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)int i, ret;struct ref *ref =NULL ;const char *dest =NULL ; -const struct ref **sought =NULL ; +struct refspec *sought =NULL ;int nr_sought =0 , alloc_sought =0 ; +const struct ref **sought_refs; +int nr_sought_refs;int fd[2 ];char *pack_lockfile =NULL ;char **pack_lockfile_ptr =NULL ;
@@ -195,8 +177,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)return args.diag_url ?0 :1 ; }get_remote_heads (fd[0 ],NULL ,0 , &ref,0 ,NULL , &shallow); +get_ref_array (&sought_refs, &nr_sought_refs, ref, sought, nr_sought); - ref =fetch_pack (&args, fd, conn, ref, dest, sought, nr_sought, + ref =fetch_pack (&args, fd, conn, ref, dest, sought_refs, nr_sought_refs, &shallow, pack_lockfile_ptr);if (pack_lockfile) {printf ("lock %s\n" , pack_lockfile);
@@ -222,12 +205,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)*/ for (i =0 ; i < nr_sought; i++) {struct ref *r; -for (r = ref; r; r = r->next) -if (!sought[i] ||refname_match (sought[i]->name, r->name)) +if (sought[i].pattern) +continue ;/* patterns do not need to match anything */ +for (r = ref; r; r = r->next) { +if (refname_match (sought[i].src, r->name))break ; + }if (r)continue ; -error ("no such remote ref %s" , sought[i]->name); +error ("no such remote ref %s" , sought[i].src); ret =1 ; }
remote.c +55 -0
67 lines not on this page · raw message
remote.h +16 -0
@@ -162,6 +162,13 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);*/ struct ref *ref_remove_duplicates (struct ref *ref_map); +/* +* Parse the given ref or ref pattern. If a ref, write a refspec with that ref +* as src, and with an empty dst. If a ref pattern, write a glob refspec with +* that pattern as src and dst. +*/ +void parse_ref_or_pattern (struct refspec *refspec,const char *str); +int valid_fetch_refspec (const char *refspec);struct refspec *parse_fetch_refspec (int nr_refspec,const char **refspec);
@@ -192,6 +199,15 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,int get_fetch_map (const struct ref *remote_refs,const struct refspec *refspec,struct ref ***tail,int missing_ok); +/* +* Convenience function to generate an array of refs corresponding to the given +* refspecs. This is equivalent to repeatedly calling get_fetch_map and +* rearranging the returned refs as an array. +*/ +void get_ref_array (const struct ref ***refs,int *nr_ref, +const struct ref *remote_refs, +const struct refspec *refspecs,int nr_refspecs); +struct ref *get_remote_ref (const struct ref *remote_refs,const char *name);/*
t/t5500-fetch-pack.sh +38 -0
@@ -547,6 +547,44 @@ test_expect_success 'fetch-pack can fetch a raw sha1' ' git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one) ' +test_expect_success 'fetch-pack can fetch refs using a partial name' ' + git init server && + ( + cd server && + test_commit 1 && + test_commit 2 && + git checkout -b one + ) && + rm -f trace && + GIT_TRACE_PACKET="$(pwd)/trace" git fetch-pack server one >actual && + grep " want " trace && + ! grep " want-ref " trace && + + grep "$(printf "%s refs/heads/one" $(git -C server rev-parse --verify one))" actual +' + +test_expect_success 'fetch-pack can fetch refs using a glob' ' + rm -rf server && + git init server && + ( + cd server && + test_commit 1 && + test_commit 2 && + git checkout -b ona && + git checkout -b onb && + test_commit 3 && + git checkout -b onc + ) && + rm -f trace && + GIT_TRACE_PACKET="$(pwd)/trace" git fetch-pack server "refs/heads/on*" >actual && + grep " want " trace && + ! grep " want-ref " trace && + + grep "$(printf "%s refs/heads/ona" $(git -C server rev-parse --verify ona))" actual && + grep "$(printf "%s refs/heads/onb" $(git -C server rev-parse --verify onb))" actual && + grep "$(printf "%s refs/heads/onc" $(git -C server rev-parse --verify onc))" actual +' + check_prot_path () { cat >expected <<-EOF && Diag: url=$1
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 11/14 fetch-pack: support want-ref
In reply to Jonathan TanTeach fetch-pack to use the want-ref mechanism whenever the server advertises it.
--- builtin/fetch-pack.c | 5 +- fetch-pack.c | 173 ++++++++++++++++++++++++++++++++++++-------------- fetch-pack.h | 2 + t/t5500-fetch-pack.sh | 42 ++++++++++++ transport.c | 2 +- 5 files changed, 175 insertions(+), 49 deletions(-)
builtin/fetch-pack.c +3 -2
@@ -179,8 +179,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)get_remote_heads (fd[0 ],NULL ,0 , &ref,0 ,NULL , &shallow);get_ref_array (&sought_refs, &nr_sought_refs, ref, sought, nr_sought); - ref =fetch_pack (&args, fd, conn, ref, dest, sought_refs, nr_sought_refs, - &shallow, pack_lockfile_ptr); + ref =fetch_pack (&args, fd, conn, ref, dest, sought, nr_sought, + sought_refs, nr_sought_refs, &shallow, + pack_lockfile_ptr);if (pack_lockfile) {printf ("lock %s\n" , pack_lockfile);fflush (stdout);
fetch-pack.c +127 -46
267 lines not on this page · raw message
fetch-pack.h +2 -0
@@ -5,6 +5,7 @@#include "run-command.h" struct sha1_array ; +struct refspec ;struct fetch_pack_args {const char *uploadpack;
@@ -38,6 +39,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, const char *dest, + const struct refspec *refspecs, int nr_refspec, const struct ref **sought, int nr_sought, struct sha1_array *shallow,
t/t5500-fetch-pack.sh +42 -0
54 lines not on this page · raw message
transport.c +1 -1
@@ -239,7 +239,7 @@ static int fetch_refs_via_pack(struct transport *transport, refs =fetch_pack (&args, data->fd, data->conn, refs_tmp ? refs_tmp : transport->remote_refs, - dest, to_fetch, nr_heads, &data->shallow, + dest,NULL ,0 , to_fetch, nr_heads, &data->shallow, &transport->pack_lockfile);close (data->fd[0 ]);close (data->fd[1 ]);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 12/14 fetch-pack: do not printf after closing stdout
In reply to Jonathan TanIn fetch-pack, during a stateless RPC, printf is invoked after stdout is closed. Update the code to not do this, preserving the existing behavior.
--- builtin/fetch-pack.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
builtin/fetch-pack.c +10 -4
@@ -191,10 +191,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)printf ("connectivity-ok\n" );fflush (stdout); } -close (fd[0 ]); -close (fd[1 ]); -if (finish_connect (conn)) -return 1 ; +if (finish_connect (conn)) { + ret =1 ; + goto cleanup; + } ret = !ref;
@@ -218,11 +218,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ret =1 ; } +if (args.stateless_rpc) + goto cleanup; +while (ref) {printf ("%s %s\n" ,oid_to_hex (&ref->old_oid), ref->name); ref = ref->next; } +cleanup: +close (fd[0 ]); +close (fd[1 ]);return ret; }
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [RFC 12/14] fetch-pack: do not printf after closing stdout
Quoted 3 lines · Jonathan Tan
In fetch-pack, during a stateless RPC, printf is invoked after stdout is closed. Update the code to not do this, preserving the existing behavior.
This seems to me as if it could go as an independent bugfix(?) or refactoring as this seems to be unclear from the code?
Quoted 46 lines
Signed-off-by Jonathan Tan <[email protected]>--- builtin/fetch-pack.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)builtin/fetch-pack.c +10
-5@@ -191,10 +191,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)printf ("connectivity-ok\n" );fflush (stdout); } -close (fd[0 ]); -close (fd[1 ]); -if (finish_connect (conn)) -return 1 ; +if (finish_connect (conn)) { + ret =1 ; + goto cleanup; + } ret = !ref;@@ -218,11 +218,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ret =1 ; } +if (args.stateless_rpc) + goto cleanup; +while (ref) {printf ("%s %s\n" ,oid_to_hex (&ref->old_oid), ref->name); ref = ref->next; } +cleanup: +close (fd[0 ]); +close (fd[1 ]);return ret; } --2.11.0.483.g087da7b7c-goog
Re: [RFC 12/14] fetch-pack: do not printf after closing stdout
Quoted 7 lines · Stefan Beller
Quoted 3 lines · Jonathan Tan
In fetch-pack, during a stateless RPC, printf is invoked after stdout is closed. Update the code to not do this, preserving the existing behavior.
This seems to me as if it could go as an independent bugfix(?) or refactoring as this seems to be unclear from the code?
The subsequent patches in this patch set are dependent on this patch, but it's true that this could be sent out on its own first.
I'm not sure if bugfix is the right word, since the existing behavior is correct (except perhaps that we rely on the fact that printf after closing stdout does effectively nothing).
RFC 13/14 fetch: send want-ref and receive fetched refs
In reply to Jonathan TanTeach fetch to send refspecs to the underlying transport, and teach all components used by the HTTP transport (remote-curl, transport-helper, fetch-pack) to understand and propagate the names and SHA-1s of the refs fetched.
--- builtin/clone.c | 4 +- builtin/fetch-pack.c | 8 ++- builtin/fetch.c | 100 ++++++++++++++++++++++++++++++------- remote-curl.c | 91 ++++++++++++++++++++------------- t/t5552-upload-pack-ref-in-want.sh | 4 +- transport-helper.c | 74 +++++++++++++++++++++------ transport.c | 10 ++-- transport.h | 20 +++++--- 8 files changed, 229 insertions(+), 82 deletions(-)
builtin/clone.c +2 -2
@@ -1078,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) }if (!is_local && !complete_refs_before_fetch) { -transport_fetch_refs (transport, mapped_refs, +transport_fetch_refs (transport,NULL ,0 , mapped_refs, &new_remote_refs);if (new_remote_refs) { refs = new_remote_refs;
@@ -1124,7 +1124,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)if (is_local)clone_local (path, git_dir);else if (refs && complete_refs_before_fetch) -transport_fetch_refs (transport, mapped_refs,NULL ); +transport_fetch_refs (transport,NULL ,0 , mapped_refs,NULL );update_remote_refs (refs, mapped_refs, remote_head_points_at, branch_top.buf, reflog_msg.buf, transport, !is_local);
builtin/fetch-pack.c +7 -1
@@ -35,6 +35,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)struct fetch_pack_args args;struct sha1_array shallow =SHA1_ARRAY_INIT ;struct string_list deepen_not =STRING_LIST_INIT_DUP ; +int always_print_refs =0 ;packet_trace_identity ("fetch-pack" );
@@ -126,6 +127,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.update_shallow =1 ;continue ; } +if (!strcmp ("--always-print-refs" , arg)) { + always_print_refs =1 ; +continue ; + }usage (fetch_pack_usage); }if (deepen_not.nr)
@@ -218,7 +223,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ret =1 ; } -if (args.stateless_rpc) +if (args.stateless_rpc && !always_print_refs) goto cleanup;while (ref) {
@@ -226,6 +231,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) oid_to_hex(&ref->old_oid), ref->name); ref = ref->next; } +fflush (stdout); cleanup:close (fd[0 ]);
builtin/fetch.c +83 -17
169 lines not on this page · raw message
remote-curl.c +55 -36
164 lines not on this page · raw message
t/t5552-upload-pack-ref-in-want.sh +2 -2
@@ -345,7 +345,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" config uploadpack.advertiseRefInWant true && rm -rf local && cp -r "$LOCAL_PRISTINE" local &&
@@ -369,7 +369,7 @@ test_expect_success 'server is initially behind - no ref in want' ' test_cmp expected actual ' -test_expect_failure 'server is initially behind - ref in want' ' +test_expect_success 'server is initially behind - ref in want' ' git -C "$REPO" config uploadpack.advertiseRefInWant true && rm -rf local && cp -r "$LOCAL_PRISTINE" local &&
transport-helper.c +59 -15
132 lines not on this page · raw message
transport.c +7 -3
@@ -95,6 +95,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus }static int fetch_refs_from_bundle (struct transport *transport, +int nr_refspec,const struct refspec *refspecs,int nr_heads,const struct ref **to_fetch,struct ref **fetched_refs) {
@@ -203,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus }static int fetch_refs_via_pack (struct transport *transport, +int nr_refspec,const struct refspec *refspecs,int nr_heads,const struct ref **to_fetch,struct ref **fetched_refs) {
@@ -1096,8 +1098,9 @@ const struct ref *transport_get_remote_refs(struct transport *transport)return transport->remote_refs; } -int transport_fetch_refs (struct transport *transport,struct ref *refs, -struct ref **fetched_refs) +int transport_fetch_refs (struct transport *transport, +const struct refspec *refspecs,int refspec_nr, +struct ref *refs,struct ref **fetched_refs) {int rc;int nr_heads =0 , nr_alloc =0 , nr_refs =0 ;
@@ -1136,7 +1139,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs, heads[nr_heads++] = rm; } - rc = transport->fetch (transport, nr_heads, heads, fetched_refs); + rc = transport->fetch (transport, refspec_nr, refspecs, + nr_heads, heads, fetched_refs);if (nop_head) { *nop_tail = *fetched_refs; *fetched_refs = nop_head;
transport.h +14 -6
@@ -82,13 +82,20 @@ struct transport {* Fetch the objects for the given refs. Note that this gets * an array, and should ignore the list structure. * +* The user may provide the array of refspecs used to generate the +* given refs. If provided, the transport should prefer the refspecs if +* possible (but may still use the refs for pre-fetch optimizations, +* for example). +* * The transport *may* provide, in fetched_refs, the list of refs that -* it fetched. If the transport knows anything about the fetched refs -* that the caller does not know (for example, shallow status or ref -* hashes), it should provide that list of refs and include that -* information in the list. +* it fetched, and must do so if it is different from the given refs. +* If the transport knows anything about the fetched refs that the +* caller does not know (for example, shallow status or ref hashes), it +* should provide that list of refs and include that information in the +* list. **/ int (*fetch)(struct transport *transport, +int refspec_nr,const struct refspec *refspecs,int refs_nr,const struct ref **refs,struct ref **fetched_refs);
@@ -234,8 +241,9 @@ int transport_push(struct transport *connection,const struct ref *transport_get_remote_refs (struct transport *transport); -int transport_fetch_refs (struct transport *transport,struct ref *refs, -struct ref **fetched_refs); +int transport_fetch_refs (struct transport *transport, +const struct refspec *refspecs,int refspec_nr, +struct ref *refs,struct ref **fetched_refs);void transport_unlock_pack (struct transport *transport);int transport_disconnect (struct transport *transport);char *transport_anonymize_url (const char *url);
Signature · 1 line
2.11.0.483.g087da7b7c-goog
RFC 14/14 DONT USE advertise_ref_in_want=1
In reply to Jonathan Tan--- t/t5500-fetch-pack.sh | 2 ++ upload-pack.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
t/t5500-fetch-pack.sh +2 -0
@@ -551,6 +551,7 @@ test_expect_success 'fetch-pack can fetch refs using a partial name' ' git init server && ( cd server && + git config uploadpack.advertiseRefInWant false test_commit 1 && test_commit 2 && git checkout -b one
@@ -587,6 +588,7 @@ test_expect_success 'fetch-pack can fetch refs using a glob' ' git init server && ( cd server && + git config uploadpack.advertiseRefInWant false test_commit 1 && test_commit 2 && git checkout -b ona &&
upload-pack.c +1 -1
@@ -62,7 +62,7 @@ static int use_sideband;static int advertise_refs;static int stateless_rpc;static const char *pack_objects_hook; -static int advertise_ref_in_want; +static int advertise_ref_in_want =1 ;static void reset_timeout (void ) {
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Quoted 14 lines · Jonathan Tan
Hello everyone - this is a proposal for a protocol change to allow the fetch-pack/upload-pack to converse in terms of ref names (globs allowed), and also an implementation of the server (upload-pack) and fetch-from-HTTP client (fetch-pack invoked through fetch).
Negotiation currently happens by upload-pack initially sending a list of refs with names and SHA-1 hashes, and then several request/response pairs in which the request from fetch-pack consists of SHA-1 hashes (selected from the initial list). Allowing the request to consist of names instead of SHA-1 hashes increases tolerance to refs changing (due to time, and due to having load-balanced servers without strong consistency), and is a step towards eliminating the need for the server to send the list of refs first (possibly improving performance).
Quoted 5 lines
The protocol is extended by allowing fetch-pack to send "want-ref <name>", where <name> is a full name (refs/...) [1], possibly including glob characters. Upload-pack will inform the client of the refs actually matched by sending "wanted-ref <SHA-1> <name>" before sending the last ACK or NAK.
I have reviewed the patches and think they are a good idea,
cc'ing Jeff who you linked to and who had some ideas about the protocol as well.
Stefan
Quoted 7 lines · Jonathan Tan
Negotiation currently happens by upload-pack initially sending a list of refs with names and SHA-1 hashes, and then several request/response pairs in which the request from fetch-pack consists of SHA-1 hashes (selected from the initial list). Allowing the request to consist of names instead of SHA-1 hashes increases tolerance to refs changing (due to time, and due to having load-balanced servers without strong consistency),
Interesting. My big question is: what happens when a ref _does_ change? How does the client handle this?
The existing uploadpack.allowReachableSHA1InWant is there to work around the problem that an http client may get a ref advertisement in one step, and then come back later to do the want/have negotiation, at which point the server has moved on (or maybe it's even a different server). There the client says "I want sha1 X", and the server needs to say "well, X isn't my tip now, but it's still acceptable for you to fetch".
But this seems to go in the opposite direction. After the advertisement, the client decides "OK, I want to fetch refs/heads/master which is at SHA1 X". It connects to the server and says "I want refs/heads/master". Let's say the server has moved its version of the ref to SHA1 Y.
What happens? I think the server will say "wanted-ref master Y". Does the client just decide to use "Y" then? How does that interact with any decisions the client might have made about X? I guess things like fast-forwards have to be decided after we fetch the objects anyway (since we cannot compute them until we get the pack), so maybe there aren't any such decisions. I haven't checked.
Quoted 2 lines
and is a step towards eliminating the need for the server to send the list of refs first (possibly improving performance).
I'm not sure it is all that useful towards that end. You still have to break compatibility so that the client tells the server to suppress the ref advertisement. After that, it is just a question of asking for the refs. And you have two options:
1. Ask the server to tell you the values of some subset of the refs, pick what you want, and then do the want/have as normal.
2. Go straight to the want/have, but tell the server the refs you want instead of their sha1s.
I think your approach here would lead to (2).
But (1), besides being closer to how the protocol works now, seems like it's more flexible. I can ask about the ref state without necessarily having to retrieve the objects. How would you write git-ls-remote with such a system?
Quoted 10 lines
[1] There has been some discussion about whether the server should accept partial ref names, e.g. [2]. In this patch set, I have made the server only accept full names, and it is the responsibility of the client to send the multiple patterns which it wants to match. Quoting from the commit message of the second patch:
For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
That has a cost that scales linearly with the number of refs, because you have to ask for each name 6 times. After the discussion you linked, I think my preference is more like:
1. Teach servers to accept a list of patterns from the client which will be resolved in order. Unlike your system, the client only needs to specify the list once per session, rather than once per ref.
2. (Optional) Give a shorthand for the stock patterns that git has had in place for years. That saves some bytes over specifying the patterns completely (though it's really not _that_ many bytes, so perhaps the complication isn't a big deal).
-Peff
Thanks for your comments.
Quoted 25 lines · Jeff King
Quoted 7 lines · Jonathan Tan
Negotiation currently happens by upload-pack initially sending a list of refs with names and SHA-1 hashes, and then several request/response pairs in which the request from fetch-pack consists of SHA-1 hashes (selected from the initial list). Allowing the request to consist of names instead of SHA-1 hashes increases tolerance to refs changing (due to time, and due to having load-balanced servers without strong consistency),
Interesting. My big question is: what happens when a ref _does_ change? How does the client handle this?
The existing uploadpack.allowReachableSHA1InWant is there to work around the problem that an http client may get a ref advertisement in one step, and then come back later to do the want/have negotiation, at which point the server has moved on (or maybe it's even a different server). There the client says "I want sha1 X", and the server needs to say "well, X isn't my tip now, but it's still acceptable for you to fetch".
But this seems to go in the opposite direction. After the advertisement, the client decides "OK, I want to fetch refs/heads/master which is at SHA1 X". It connects to the server and says "I want refs/heads/master". Let's say the server has moved its version of the ref to SHA1 Y.
Quoted 6 lines
What happens? I think the server will say "wanted-ref master Y". Does the client just decide to use "Y" then? How does that interact with any decisions the client might have made about X? I guess things like fast-forwards have to be decided after we fetch the objects anyway (since we cannot compute them until we get the pack), so maybe there aren't any such decisions. I haven't checked.
Yes, the server will say "wanted-ref master Y". The relevant code regarding the decisions the client makes regarding X or Y is in do_fetch in builtin/fetch.c.
There, I can see these decisions done using X:
- check_not_current_branch (forbidding fetching that modifies the current branch) (I just noticed that this has to be done for Y too, and will do so)
- prune refs [*]
- automatic tag following [*]
[*] X and Y may differ in that one relevant ref appears in one set but not in the other (because a ref was added or removed in the meantime), causing a different result if these decisions were to be done using Y, but I think that it is OK either way.
Fetch optimizations (for example, everything_local in fetch-pack.c) that check if the client really needs to fetch are also done using X, of course (and if the optimization succeeds, there is no Y).
Fast-forwards (and everything else in store_updated_refs) are decided using Y.
Quoted 14 lines
Quoted 2 lines
and is a step towards eliminating the need for the server to send the list of refs first (possibly improving performance).
I'm not sure it is all that useful towards that end. You still have to break compatibility so that the client tells the server to suppress the ref advertisement. After that, it is just a question of asking for the refs. And you have two options:
1. Ask the server to tell you the values of some subset of the refs, pick what you want, and then do the want/have as normal.
2. Go straight to the want/have, but tell the server the refs you want instead of their sha1s.
Quoted 6 lines
I think your approach here would lead to (2).
But (1), besides being closer to how the protocol works now, seems like it's more flexible. I can ask about the ref state without necessarily having to retrieve the objects. How would you write git-ls-remote with such a system?
Assuming a new protocol with the appropriate backwards compatibility (which would have to be done for both options), (2) does save a request/response pair (because we can send the ref names directly as "want-ref" in the initial request instead of sending ref names in the initial request and then confirming them using "want <SHA-1>" in the subsequent request). Also, (2) is more tolerant towards refs changing over time. (1) might be closer to the current protocol, but I think that the difference is not so significant (only in "want-ref" vs "want" request and the "wanted-ref" response).
As for git-ls-remote, I currently think that it would have to use the existing protocol.
Quoted 20 lines
Quoted 10 lines
[1] There has been some discussion about whether the server should accept partial ref names, e.g. [2]. In this patch set, I have made the server only accept full names, and it is the responsibility of the client to send the multiple patterns which it wants to match. Quoting from the commit message of the second patch:
For example, a client could reasonably expand an abbreviated name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref refs/tags/foo", among others, and ensure that at least one such ref has been fetched.
That has a cost that scales linearly with the number of refs, because you have to ask for each name 6 times. After the discussion you linked, I think my preference is more like:
1. Teach servers to accept a list of patterns from the client which will be resolved in order. Unlike your system, the client only needs to specify the list once per session, rather than once per ref.
Quoted 4 lines
2. (Optional) Give a shorthand for the stock patterns that git has had in place for years. That saves some bytes over specifying the patterns completely (though it's really not _that_ many bytes, so perhaps the complication isn't a big deal).
I envision that most fetches would be done on a single name (with or without globs), that expands to 5 (so it is not so crucial to factor out the list of patterns). Having said that, I am open to changing this.
Looking back at the comments I have received in reply, I think that there were two major concerns: (i) the case where a server ACKs a client "have" line and the client forever thinks that the server has it, but it may not be the case for future servers (or future invocations of the same server), and (ii) what the client does with 2 "versions" of remote refs.
For (i), the issue already exists and as far as I can tell, this patch set does not directly impact it, positively or negatively. The "have"/"ACK" part of negotiation is kept the same - the only difference in this patch set is that wants can be specified by name instead of by SHA-1 hash. This patch set does not help the "have"/"ACK" part of negotiation, but it helps the "want" part.
For (ii), I have prepared a patch to be squashed, and extended the commit message with an explanation of what is happening. (The commit message and the patch are appended to this e-mail).
(There was also some discussion of the client being required to send exact matches in its "want-ref" lines.)
Please let me know if you have any other opinions or thoughts. It does seem to me like such a protocol update (or something similar) would help for large repositories with many ever-changing refs (like refs/changes in Gerrit or refs/pull in GitHub) - and the creation of a ref would look like a deletion depending on the order in which we access the servers in a load-balancing arrangement and the order in which those servers synchronize themselves. And deletion of refs does not work with the current protocol, but would work with a protocol that supports wildcards (like this one).
-- 8< --
fetch: send want-ref and receive fetched refs
Teach fetch to send refspecs to the underlying transport, and teach all components used by the HTTP transport (remote-curl, transport-helper, fetch-pack) to understand and propagate the names and SHA-1s of the refs fetched.
The do_fetch method in builtin/fetch.c originally had only one remote-local ref map, generated from the already-fetched list of remote refs. This patch introduces a new ref map generated from the list of fetched refs. Usages of the list of remote refs or the remote-local ref map are updated as follows:
- check_not_current_branch (which checks that the current branch is not affected by the fetch) is performed both on the original ref map (to die before the fetch if we can, as an optimization) and on the new ref map (since the new ref map is the one actually applied).
- Pruning is done based on the new ref map.
- backfill_tags (for tag following) is performed using the original list of remote refs because the new list of fetched refs is not guaranteed to contain tag information. (Since backfill_tags performs another fetch, it does not need to be fully consistent with the just-returned packfile.)
The list of remote refs and the remote-local ref map are not otherwise used by do_fetch or any function in its invocation chain (fetch_one and cmd_fetch).
--- builtin/fetch.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
builtin/fetch.c +14 -12
@@ -1177,6 +1177,20 @@ static int do_fetch(struct transport *transport,if (tags ==TAGS_DEFAULT && autotags)transport_set_option (transport,TRANS_OPT_FOLLOWTAGS ,"1" ); +if (fetch_refs (transport, e_rs, e_rs_nr, ref_map, &new_remote_refs)) { +free_refs (ref_map); + retcode =1 ; + goto cleanup; + } +if (new_remote_refs) { +free_refs (ref_map); + ref_map =get_ref_map (transport->remote, new_remote_refs, + refs, ref_count, tags, autotags); +if (!update_head_ok) +check_not_current_branch (ref_map); +free_refs (new_remote_refs); + } +if (prune) {/* * We only prune based on refspecs specified
@@ -1192,18 +1206,6 @@ static int do_fetch(struct transport *transport, transport->url); } } -if (fetch_refs (transport, e_rs, e_rs_nr, ref_map, &new_remote_refs)) { -free_refs (ref_map); - retcode =1 ; - goto cleanup; - } -if (new_remote_refs) { -free_refs (ref_map); - ref_map =get_ref_map (transport->remote, new_remote_refs, - refs, ref_count, tags, autotags); -free_refs (new_remote_refs); - } -if (consume_refs (transport, ref_map)) {free_refs (ref_map); retcode =1 ;
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Quoted 6 lines · Jonathan Tan
Usages of the list of remote refs or the remote-local ref map are updated as follows:
-check_not_current_branch(which checks that the current branch is not affected by the fetch) is performed both on the original ref map (to die before the fetch if we can, as an optimization) and on the new ref map (since the new ref map is the one actually applied).
OK.
Quoted 1 line
- Pruning is done based on the new ref map.
OK. As that is what eventually gets "installed" on the local side, it makes sense to become consistent with that set, not the set the original server gave you.
Quoted 5 lines
-
backfill_tags(for tag following) is performed using the original list of remote refs because the new list of fetched refs is not guaranteed to contain tag information. (Sincebackfill_tagsperforms another fetch, it does not need to be fully consistent with the just-returned packfile.)
This smells correct but I need to think about this one a bit more.
Overall I think the strategy is agreeable.