git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>,
	gitster@pobox.com, peff@peff.net
Subject: Re: [RFC 00/14] Allow fetch-pack to send ref names (globs allowed)
Date: Tue,  7 Feb 2017 15:53:50 -0800
Message-ID: <20170207235350.10718-1-jonathantanmy@google.com> (raw)
In-Reply-To: <cover.1485381677.git.jonathantanmy@google.com>

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(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 87de00e49..b8432394c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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;
-- 
2.11.0.483.g087da7b7c-goog


  parent reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-25 22:02 Jonathan Tan
2017-01-25 22:02 ` [RFC 01/14] upload-pack: move parsing of "want" line Jonathan Tan
2017-01-25 22:02 ` [RFC 02/14] upload-pack: allow ref name and glob requests Jonathan Tan
2017-01-26 22:23   ` Junio C Hamano
2017-01-27  0:35     ` Jonathan Tan
2017-01-27  1:54       ` Junio C Hamano
2017-01-25 22:02 ` [RFC 03/14] upload-pack: test negotiation with changing repo Jonathan Tan
2017-01-26 22:33   ` Junio C Hamano
2017-01-27  0:44     ` Jonathan Tan
2017-02-22 23:36       ` Junio C Hamano
2017-02-23 18:43         ` [PATCH] upload-pack: report "not our ref" to client Jonathan Tan
2017-02-23 20:14           ` Junio C Hamano
2017-01-25 22:02 ` [RFC 04/14] fetch: refactor the population of hashes Jonathan Tan
2017-01-25 22:02 ` [RFC 05/14] fetch: refactor fetch_refs into two functions Jonathan Tan
2017-01-25 22:02 ` [RFC 06/14] fetch: refactor to make function args narrower Jonathan Tan
2017-01-25 22:03 ` [RFC 07/14] fetch-pack: put shallow info in out param Jonathan Tan
2017-01-25 22:03 ` [RFC 08/14] fetch-pack: check returned refs for matches Jonathan Tan
2017-01-25 22:03 ` [RFC 09/14] transport: put ref oid in out param Jonathan Tan
2017-01-25 22:03 ` [RFC 10/14] fetch-pack: support partial names and globs Jonathan Tan
2017-01-25 22:03 ` [RFC 11/14] fetch-pack: support want-ref Jonathan Tan
2017-01-25 22:03 ` [RFC 12/14] fetch-pack: do not printf after closing stdout Jonathan Tan
2017-01-26  0:50   ` Stefan Beller
2017-01-26 18:18     ` Jonathan Tan
2017-01-25 22:03 ` [RFC 13/14] fetch: send want-ref and receive fetched refs Jonathan Tan
2017-01-25 22:03 ` [RFC 14/14] DONT USE advertise_ref_in_want=1 Jonathan Tan
2017-01-26 22:15 ` [RFC 00/14] Allow fetch-pack to send ref names (globs allowed) Stefan Beller
2017-01-26 23:00 ` Jeff King
2017-01-27  0:26   ` Jonathan Tan
2017-02-07 23:53 ` Jonathan Tan [this message]
2017-02-09  0:26   ` Junio C Hamano
2018-05-20  8:24 Apinan Ponchan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170207235350.10718-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git