threads / patch / 9860

patch, 5 partsRemove unnecessary 'fetch' argument from transport_get API

Subject: [PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API

## tl;dr

2 messages between Sep 15, 2007 and Sep 16, 2007. Diffs are folded; open one to read it.

replies: 1people: 1as markdown or json

Shawn O. Pearce· Sep 15, 2007, 07:23 UTC · lore

We don't actually need to know at the time of transport_get if the caller wants to fetch, push, or do both on the returned object. It is easier to just delay the initialization of the HTTP walker until we know we will need it by providing a CURL specific fetch function in the curl_transport that makes sure the walker instance is initialized before use.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch.c |    2 +-
 builtin-push.c  |    2 +-
 transport.c     |   23 +++++++++++++++++------
 transport.h     |    4 +---
 4 files changed, 20 insertions(+), 11 deletions(-)
Show changes to 4 files +20 −11

builtin-fetch.c, builtin-push.c, transport.c, transport.h

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8b0fdbe..300d563 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -496,7 +496,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	else
 		remote = remote_get(argv[i++]);
 
-	transport = transport_get(remote, remote->uri[0], 1);
+	transport = transport_get(remote, remote->uri[0]);
 	if (verbose >= 2)
 		transport->verbose = 1;
 	if (quiet)
diff --git a/builtin-push.c b/builtin-push.c
index f496b46..7d7e826 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -59,7 +59,7 @@ static int do_push(const char *repo, int flags)
 	errs = 0;
 	for (i = 0; i < remote->uri_nr; i++) {
 		struct transport *transport =
-			transport_get(remote, remote->uri[i], 0);
+			transport_get(remote, remote->uri[i]);
 		int err;
 		if (receivepack)
 			transport_set_option(transport,
diff --git a/transport.c b/transport.c
index 5eabe8d..7f94d30 100644
--- a/transport.c
+++ b/transport.c
@@ -174,6 +174,14 @@ static struct ref *get_refs_via_curl(const struct transport *transport)
 	return refs;
 }
 
+static int fetch_objs_via_curl(struct transport *transport,
+				 int nr_objs, struct ref **to_fetch)
+{
+	if (!transport->data)
+		transport->data = get_http_walker(transport->url);
+	return fetch_objs_via_walker(transport, nr_objs, to_fetch);
+}
+
 #else
 
 static struct ref *get_refs_via_curl(const struct transport *transport)
@@ -182,12 +190,19 @@ static struct ref *get_refs_via_curl(const struct transport *transport)
 	return NULL;
 }
 
+static int fetch_objs_via_curl(struct transport *transport,
+				 int nr_objs, struct ref **to_fetch)
+{
+	die("Cannot fetch from '%s' without curl ...", transport->url);
+	return -1;
+}
+
 #endif
 
 static const struct transport_ops curl_transport = {
 	/* set_option */	NULL,
 	/* get_refs_list */	get_refs_via_curl,
-	/* fetch */		fetch_objs_via_walker,
+	/* fetch */		fetch_objs_via_curl,
 	/* push */		curl_transport_push,
 	/* disconnect */	disconnect_walker
 };
@@ -408,14 +423,12 @@ static int is_file(const char *url)
 	return S_ISREG(buf.st_mode);
 }
 
-struct transport *transport_get(struct remote *remote, const char *url,
-				int fetch)
+struct transport *transport_get(struct remote *remote, const char *url)
 {
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
 	ret->remote = remote;
 	ret->url = url;
-	ret->fetch = !!fetch;
 
 	if (!prefixcmp(url, "rsync://")) {
 		ret->ops = &rsync_transport;
@@ -423,8 +436,6 @@ struct transport *transport_get(struct remote *remote, const char *url,
 	        || !prefixcmp(url, "https://")
 	        || !prefixcmp(url, "ftp://")) {
 		ret->ops = &curl_transport;
-		if (fetch)
-			ret->data = get_http_walker(url);
 	} else if (is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
diff --git a/transport.h b/transport.h
index f2bbdf7..6a95d66 100644
--- a/transport.h
+++ b/transport.h
@@ -6,7 +6,6 @@
 
 struct transport {
 	unsigned verbose : 1;
-	unsigned fetch : 1;
 	struct remote *remote;
 	const char *url;
 
@@ -38,8 +37,7 @@ struct transport_ops {
 };
 
 /* Returns a transport suitable for the url */
-struct transport *transport_get(struct remote *remote, const char *url,
-				int fetch);
+struct transport *transport_get(struct remote *, const char *);
 
 /* Transport options which apply to git:// and scp-style URLs */
 

-- 
1.5.3.1.84.gaf82-dirty
Shawn O. Pearce· Sep 16, 2007, 08:11 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API

"Shawn O. Pearce" <spearce@spearce.org> wrote:
Show 6 quoted lines
> We don't actually need to know at the time of transport_get if the
> caller wants to fetch, push, or do both on the returned object.
> It is easier to just delay the initialization of the HTTP walker
> until we know we will need it by providing a CURL specific fetch
> function in the curl_transport that makes sure the walker instance
> is initialized before use.

Daniel privately emailed me his rationale for why this fetch argument was here in the first place and I mostly agree with him, but will be working up a more clear API replacement in the near future. The "1/0","fetch/push" thing is not the clearest way that we could define the API.

Right now this patch is required to go along with my 1/5 bug fix, as without this patch we get the sequence:

	http_init()
	http_init()
	... use http ...
	http_cleanup()
	... try to use http again and barf ...

This 5/5 makes the sequence be in proper order by delaying creation of the HTTP walker object (and thus one of the http_init() calls) to after the first http_cleanup(), so we get the nice neat order of:

	http_init()
	... use http ...
	http_cleanup()
	http_init()
	... use http just fine ...
	http_cleanup()

I'll be honest here; I did not test 1/5 or 5/5 on their own. I only tested the combined result of them, and that creates a working HTTP fetch. But I'm pretty sure that one of these patches alone will still cause SIGSEGV/SIGBUS errors during HTTP fetch due to either the bad cleanup (1/5 fixes) or the bad init ordering I'm talking about above (5/5 fixes).

Reinstating a replacement for the fetch parameter that I removed
in this patch isn't critical for functionality, but will be
necessary to do performance optimization in the form of reusing
the connection between ref discovery and pack transfer in the
native protocol.  Right now I'm focusing on making builtin-fetch
stable and implementing prior behavior.  Once its solid enough to
graduate to `next` we can start doing some of the optimization work.
 
-- 
Shawn.

← back to recent threads