threads / patch / 20257

v2, 3 partsUse an external program to implement fetching with curl

Subject: [PATCH 2/3 v2] Use an external program to implement fetching with curl

## tl;dr

6 messages between Jul 28, 2009 and Jul 29, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Daniel Barkalow· Jul 28, 2009, 06:08 UTC · lore
Use the transport shim mechanism to fetch by http (and ftp, etc).
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Makefile    |    5 ++
 shim-curl.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 transport.c |  136 +----------------------------------------------------------
 3 files changed, 138 insertions(+), 135 deletions(-)
 create mode 100644 shim-curl.c
Show changes to 3 files +138 −135

Makefile, shim-curl.c, transport.c

diff --git a/Makefile b/Makefile
index 01efc73..d3dd2ed 100644
--- a/Makefile
+++ b/Makefile
@@ -980,6 +980,7 @@ else
 		CURL_LIBCURL = -lcurl
 	endif
 	BUILTIN_OBJS += builtin-http-fetch.o
+	PROGRAMS += git-shim-curl$X
 	EXTLIBS += $(CURL_LIBCURL)
 	LIB_OBJS += http.o http-walker.o
 	curl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)
@@ -1490,6 +1491,10 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
+git-shim-curl$X: shim-curl.o http.o http-walker.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H) $(wildcard */*.h)
 builtin-revert.o wt-status.o: wt-status.h
diff --git a/shim-curl.c b/shim-curl.c
new file mode 100644
index 0000000..bde041b
--- /dev/null
+++ b/shim-curl.c
@@ -0,0 +1,132 @@
+#include "cache.h"
+#include "remote.h"
+#include "strbuf.h"
+#include "walker.h"
+#include "http.h"
+
+static struct ref *get_refs(struct walker *walker, const char *url)
+{
+	int http_ret;
+	char *refs_url;
+	struct strbuf buffer = STRBUF_INIT;
+	char *data, *start, *mid;
+	int i = 0;
+	char *ref_name;
+	struct ref *refs = NULL;
+	struct ref *ref = NULL;
+	struct ref *last_ref = NULL;
+
+	refs_url = xmalloc(strlen(url) + 11);
+	sprintf(refs_url, "%s/info/refs", url);
+
+	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
+	switch (http_ret) {
+	case HTTP_OK:
+		break;
+	case HTTP_MISSING_TARGET:
+		die("%s not found: did you run git update-server-info on the"
+		    " server?", refs_url);
+	default:
+		http_error(refs_url, http_ret);
+		die("HTTP request failed");
+	}
+	free(refs_url);
+
+	data = buffer.buf;
+	start = NULL;
+	mid = data;
+	while (i < buffer.len) {
+		if (!start) {
+			start = &data[i];
+		}
+		if (data[i] == '\t')
+			mid = &data[i];
+		if (data[i] == '\n') {
+			data[i] = 0;
+			ref_name = mid + 1;
+			ref = xmalloc(sizeof(struct ref) +
+				      strlen(ref_name) + 1);
+			memset(ref, 0, sizeof(struct ref));
+			strcpy(ref->name, ref_name);
+			get_sha1_hex(start, ref->old_sha1);
+			if (!refs)
+				refs = ref;
+			if (last_ref)
+				last_ref->next = ref;
+			last_ref = ref;
+			start = NULL;
+		}
+		i++;
+	}
+	strbuf_release(&buffer);
+
+	ref = alloc_ref("HEAD");
+	if (!walker->fetch_ref(walker, ref) &&
+	    !resolve_remote_symref(ref, refs)) {
+		ref->next = refs;
+		refs = ref;
+	} else {
+		free(ref);
+	}
+
+	return refs;
+}
+
+int main(int argc, const char **argv)
+{
+	struct remote *remote;
+	struct strbuf buf = STRBUF_INIT;
+	const char *url;
+	struct walker *walker = NULL;
+
+	setup_git_directory();
+	if (argc < 2) {
+		fprintf(stderr, "Remote needed\n");
+		return 1;
+	}
+
+	remote = remote_get(argv[1]);
+
+	if (argc > 2) {
+		url = argv[2];
+	} else {
+		url = remote->url[0];
+	}
+
+	do {
+		if (strbuf_getline(&buf, stdin, '\n') == EOF)
+			break;
+		if (!prefixcmp(buf.buf, "fetch ")) {
+			char *obj = buf.buf + strlen("fetch ");
+			if (!walker)
+				walker = get_http_walker(url, remote);
+			walker->get_all = 1;
+			walker->get_tree = 1;
+			walker->get_history = 1;
+			walker->get_verbosely = 0;
+			walker->get_recover = 0;
+			if (walker_fetch(walker, 1, &obj, NULL, NULL))
+				die("Fetch failed.");
+			printf("\n");
+			fflush(stdout);
+		} else if (!strcmp(buf.buf, "list")) {
+			struct ref *refs;
+			struct ref *posn;
+			if (!walker)
+				walker = get_http_walker(url, remote);
+			refs = get_refs(walker, url);
+			for (posn = refs; posn; posn = posn->next) {
+				if (posn->symref)
+					printf("@%s %s\n", posn->symref, posn->name);
+				else
+					printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
+			}
+			printf("\n");
+			fflush(stdout);
+		} else {
+			return 1;
+		}
+		strbuf_reset(&buf);
+	} while (1);
+	return 0;
+}
diff --git a/transport.c b/transport.c
index de0d587..6d13b0e 100644
--- a/transport.c
+++ b/transport.c
@@ -1,9 +1,6 @@
 #include "cache.h"
 #include "transport.h"
 #include "run-command.h"
-#ifndef NO_CURL
-#include "http.h"
-#endif
 #include "pkt-line.h"
 #include "fetch-pack.h"
 #include "send-pack.h"
@@ -352,45 +349,6 @@ static int rsync_transport_push(struct transport *transport,
 	return result;
 }
 
-/* Generic functions for using commit walkers */
-
-#ifndef NO_CURL /* http fetch is the only user */
-static int fetch_objs_via_walker(struct transport *transport,
-				 int nr_objs, const struct ref **to_fetch)
-{
-	char *dest = xstrdup(transport->url);
-	struct walker *walker = transport->data;
-	char **objs = xmalloc(nr_objs * sizeof(*objs));
-	int i;
-
-	walker->get_all = 1;
-	walker->get_tree = 1;
-	walker->get_history = 1;
-	walker->get_verbosely = transport->verbose >= 0;
-	walker->get_recover = 0;
-
-	for (i = 0; i < nr_objs; i++)
-		objs[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
-
-	if (walker_fetch(walker, nr_objs, objs, NULL, NULL))
-		die("Fetch failed.");
-
-	for (i = 0; i < nr_objs; i++)
-		free(objs[i]);
-	free(objs);
-	free(dest);
-	return 0;
-}
-#endif /* NO_CURL */
-
-static int disconnect_walker(struct transport *transport)
-{
-	struct walker *walker = transport->data;
-	if (walker)
-		walker_free(walker);
-	return 0;
-}
-
 #ifndef NO_CURL
 static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)
 {
@@ -432,96 +390,6 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 	return !!err;
 }
 
-static struct ref *get_refs_via_curl(struct transport *transport, int for_push)
-{
-	struct strbuf buffer = STRBUF_INIT;
-	char *data, *start, *mid;
-	char *ref_name;
-	char *refs_url;
-	int i = 0;
-	int http_ret;
-
-	struct ref *refs = NULL;
-	struct ref *ref = NULL;
-	struct ref *last_ref = NULL;
-
-	struct walker *walker;
-
-	if (for_push)
-		return NULL;
-
-	if (!transport->data)
-		transport->data = get_http_walker(transport->url,
-						transport->remote);
-
-	walker = transport->data;
-
-	refs_url = xmalloc(strlen(transport->url) + 11);
-	sprintf(refs_url, "%s/info/refs", transport->url);
-
-	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
-	switch (http_ret) {
-	case HTTP_OK:
-		break;
-	case HTTP_MISSING_TARGET:
-		die("%s not found: did you run git update-server-info on the"
-		    " server?", refs_url);
-	default:
-		http_error(refs_url, http_ret);
-		die("HTTP request failed");
-	}
-
-	data = buffer.buf;
-	start = NULL;
-	mid = data;
-	while (i < buffer.len) {
-		if (!start)
-			start = &data[i];
-		if (data[i] == '\t')
-			mid = &data[i];
-		if (data[i] == '\n') {
-			data[i] = 0;
-			ref_name = mid + 1;
-			ref = xmalloc(sizeof(struct ref) +
-				      strlen(ref_name) + 1);
-			memset(ref, 0, sizeof(struct ref));
-			strcpy(ref->name, ref_name);
-			get_sha1_hex(start, ref->old_sha1);
-			if (!refs)
-				refs = ref;
-			if (last_ref)
-				last_ref->next = ref;
-			last_ref = ref;
-			start = NULL;
-		}
-		i++;
-	}
-
-	strbuf_release(&buffer);
-
-	ref = alloc_ref("HEAD");
-	if (!walker->fetch_ref(walker, ref) &&
-	    !resolve_remote_symref(ref, refs)) {
-		ref->next = refs;
-		refs = ref;
-	} else {
-		free(ref);
-	}
-
-	strbuf_release(&buffer);
-	free(refs_url);
-	return refs;
-}
-
-static int fetch_objs_via_curl(struct transport *transport,
-				 int nr_objs, const struct ref **to_fetch)
-{
-	if (!transport->data)
-		transport->data = get_http_walker(transport->url,
-						transport->remote);
-	return fetch_objs_via_walker(transport, nr_objs, to_fetch);
-}
-
 #endif
 
 struct bundle_transport_data {
@@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	} else if (!prefixcmp(url, "http://")
 	        || !prefixcmp(url, "https://")
 	        || !prefixcmp(url, "ftp://")) {
+		transport_shim_init(ret, "curl");
 #ifdef NO_CURL
 		error("git was compiled without libcurl support.");
 #else
-		ret->get_refs_list = get_refs_via_curl;
-		ret->fetch = fetch_objs_via_curl;
 		ret->push = curl_transport_push;
 #endif
-		ret->disconnect = disconnect_walker;
 
 	} else if (is_local(url) && is_file(url)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
-- 
1.6.4.rc2.13.ga9762.dirty
Johannes Schindelin· Jul 28, 2009, 13:19 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl

Hi,
On Tue, 28 Jul 2009, Daniel Barkalow wrote:
Show 9 quoted lines
> diff --git a/Makefile b/Makefile
> index 01efc73..d3dd2ed 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -980,6 +980,7 @@ else
>  		CURL_LIBCURL = -lcurl
>  	endif
>  	BUILTIN_OBJS += builtin-http-fetch.o
> +	PROGRAMS += git-shim-curl$X

So now you not only use that disgustingly meaningless name "shim", but also abolish the notion that the helper is called by its protocol name?

Show 26 quoted lines
> diff --git a/shim-curl.c b/shim-curl.c
> new file mode 100644
> index 0000000..bde041b
> --- /dev/null
> +++ b/shim-curl.c
> @@ -0,0 +1,132 @@
> +#include "cache.h"
> +#include "remote.h"
> +#include "strbuf.h"
> +#include "walker.h"
> +#include "http.h"
> +
> +static struct ref *get_refs(struct walker *walker, const char *url)
> +{
> +	int http_ret;
> +	char *refs_url;
> +	struct strbuf buffer = STRBUF_INIT;
> +	char *data, *start, *mid;
> +	int i = 0;
> +	char *ref_name;
> +	struct ref *refs = NULL;
> +	struct ref *ref = NULL;
> +	struct ref *last_ref = NULL;
> +
> +	refs_url = xmalloc(strlen(url) + 11);
> +	sprintf(refs_url, "%s/info/refs", url);
And I thought we had strbufs...
Show 17 quoted lines
> +
> +	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
> +	switch (http_ret) {
> +	case HTTP_OK:
> +		break;
> +	case HTTP_MISSING_TARGET:
> +		die("%s not found: did you run git update-server-info on the"
> +		    " server?", refs_url);
> +	default:
> +		http_error(refs_url, http_ret);
> +		die("HTTP request failed");
> +	}
> +	free(refs_url);
> +
> +	data = buffer.buf;
> +	start = NULL;
> +	mid = data;
What does "mid" stand for?
> +	while (i < buffer.len) {
> +		if (!start) {
> +			start = &data[i];
> +		}
Why curly brackets around this one, but not this:
> +		if (data[i] == '\t')
> +			mid = &data[i];
Show 5 quoted lines
> +		if (data[i] == '\n') {
> +			data[i] = 0;
> +			ref_name = mid + 1;
> +			ref = xmalloc(sizeof(struct ref) +
> +				      strlen(ref_name) + 1);
What is alloc_ref() for?
Show 8 quoted lines
> +			memset(ref, 0, sizeof(struct ref));
> +			strcpy(ref->name, ref_name);
> +			get_sha1_hex(start, ref->old_sha1);
> +			if (!refs)
> +				refs = ref;
> +			if (last_ref)
> +				last_ref->next = ref;
> +			last_ref = ref;
Is this another "tail"?
> +			start = NULL;
> +		}
> +		i++;
Oh, so it _is_ a for() loop.
Show 73 quoted lines
> +	}
> +	strbuf_release(&buffer);
> +
> +	ref = alloc_ref("HEAD");
> +	if (!walker->fetch_ref(walker, ref) &&
> +	    !resolve_remote_symref(ref, refs)) {
> +		ref->next = refs;
> +		refs = ref;
> +	} else {
> +		free(ref);
> +	}
> +
> +	return refs;
> +}
> +
> +int main(int argc, const char **argv)
> +{
> +	struct remote *remote;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *url;
> +	struct walker *walker = NULL;
> +
> +	setup_git_directory();
> +	if (argc < 2) {
> +		fprintf(stderr, "Remote needed\n");
> +		return 1;
> +	}
> +
> +	remote = remote_get(argv[1]);
> +
> +	if (argc > 2) {
> +		url = argv[2];
> +	} else {
> +		url = remote->url[0];
> +	}
> +
> +	do {
> +		if (strbuf_getline(&buf, stdin, '\n') == EOF)
> +			break;
> +		if (!prefixcmp(buf.buf, "fetch ")) {
> +			char *obj = buf.buf + strlen("fetch ");
> +			if (!walker)
> +				walker = get_http_walker(url, remote);
> +			walker->get_all = 1;
> +			walker->get_tree = 1;
> +			walker->get_history = 1;
> +			walker->get_verbosely = 0;
> +			walker->get_recover = 0;
> +			if (walker_fetch(walker, 1, &obj, NULL, NULL))
> +				die("Fetch failed.");
> +			printf("\n");
> +			fflush(stdout);
> +		} else if (!strcmp(buf.buf, "list")) {
> +			struct ref *refs;
> +			struct ref *posn;
> +			if (!walker)
> +				walker = get_http_walker(url, remote);
> +			refs = get_refs(walker, url);
> +			for (posn = refs; posn; posn = posn->next) {
> +				if (posn->symref)
> +					printf("@%s %s\n", posn->symref, posn->name);
> +				else
> +					printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
> +			}
> +			printf("\n");
> +			fflush(stdout);
> +		} else {
> +			return 1;
> +		}
> +		strbuf_reset(&buf);
> +	} while (1);
> +	return 0;
> +}

If you already start some infrastructure like this, you should go the whole nine yards and make helper functions in remote.c or transport.c that help implementing "git-remote-<protocol>" helpers.

Show 9 quoted lines
> diff --git a/transport.c b/transport.c
> index de0d587..6d13b0e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  	} else if (!prefixcmp(url, "http://")
>  	        || !prefixcmp(url, "https://")
>  	        || !prefixcmp(url, "ftp://")) {
> +		transport_shim_init(ret, "curl");

Like I said, identifying the remote helper by anything else than the protocol is a design catastrophe.

>  #ifdef NO_CURL
>  		error("git was compiled without libcurl support.");

Why the heck does this have to be guarded by an #ifdef NO_CURL? It is the helper's task to say, not transport.c's.

Show 9 quoted lines
>  #else
> -		ret->get_refs_list = get_refs_via_curl;
> -		ret->fetch = fetch_objs_via_curl;
>  		ret->push = curl_transport_push;
>  #endif
> -		ret->disconnect = disconnect_walker;
>  
>  	} else if (is_local(url) && is_file(url)) {
>  		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));

I think the basic idea of yours is sound, but all I see is some incomplete separation between builtin git-fetch/-push and the helper. As it stands, git-fetch/-push have to be compiled with the exact knowledge what helper is there, git-fetch/-push are _still_ aware if Git was compiled with http support (so that it is not possible to just install the helper afterwards), and the infrastructure forbids any extension of capabilities in the future.

I think these issues are serious enough to merit a rework.

Thanks, Dscho

Erik Faye-Lund· Jul 28, 2009, 13:30 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl

On Tue, Jul 28, 2009 at 3:19 PM, Johannes Schindelin<Johannes.Schindelin@gmx.de> wrote:

> If you already start some infrastructure like this, you should go the
> whole nine yards and make helper functions in remote.c or transport.c that
> help implementing "git-remote-<protocol>" helpers.
You mean helper-helpers? :)
-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656
Johannes Schindelin· Jul 28, 2009, 15:03 UTC · re: Erik Faye-Lund · lore

Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl

Hi,
On Tue, 28 Jul 2009, Erik Faye-Lund wrote:
Show 7 quoted lines
> On Tue, Jul 28, 2009 at 3:19 PM, Johannes
> Schindelin<Johannes.Schindelin@gmx.de> wrote:
> > If you already start some infrastructure like this, you should go the 
> > whole nine yards and make helper functions in remote.c or transport.c 
> > that help implementing "git-remote-<protocol>" helpers.
> 
> You mean helper-helpers? :)

Not really... I meant functions that do all the stdin parsing for you, together with error handling, and only return proper lists of verified arguments in the appropriate data structure. This functionality is likely to be needed by all remote helpers, that's why I would prefer to have it exposed as functions in libgit.a, to be reused by all the helpers.

Of course, this would not help shell scripting such helpers, but you have to start somewhere. And that somewhere happens to be the http transport.

Ciao, Dscho

Daniel Barkalow· Jul 28, 2009, 17:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl

On Tue, 28 Jul 2009, Johannes Schindelin wrote:
Show 45 quoted lines
> Hi,
> 
> On Tue, 28 Jul 2009, Daniel Barkalow wrote:
> 
> > diff --git a/Makefile b/Makefile
> > index 01efc73..d3dd2ed 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -980,6 +980,7 @@ else
> >  		CURL_LIBCURL = -lcurl
> >  	endif
> >  	BUILTIN_OBJS += builtin-http-fetch.o
> > +	PROGRAMS += git-shim-curl$X
> 
> So now you not only use that disgustingly meaningless name "shim", but 
> also abolish the notion that the helper is called by its protocol name?
> 
> > diff --git a/shim-curl.c b/shim-curl.c
> > new file mode 100644
> > index 0000000..bde041b
> > --- /dev/null
> > +++ b/shim-curl.c
> > @@ -0,0 +1,132 @@
> > +#include "cache.h"
> > +#include "remote.h"
> > +#include "strbuf.h"
> > +#include "walker.h"
> > +#include "http.h"
> > +
> > +static struct ref *get_refs(struct walker *walker, const char *url)
> > +{
> > +	int http_ret;
> > +	char *refs_url;
> > +	struct strbuf buffer = STRBUF_INIT;
> > +	char *data, *start, *mid;
> > +	int i = 0;
> > +	char *ref_name;
> > +	struct ref *refs = NULL;
> > +	struct ref *ref = NULL;
> > +	struct ref *last_ref = NULL;
> > +
> > +	refs_url = xmalloc(strlen(url) + 11);
> > +	sprintf(refs_url, "%s/info/refs", url);
> 
> And I thought we had strbufs...

This whole chunk of code is a direct move from transport.c, which is unfortunately not obvious in the way we represent diffs. I'd prefer to keep the code the same across the move so that we'd be able to track it if "git blame" got especially smart.

Show 132 quoted lines
> > +
> > +	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
> > +	switch (http_ret) {
> > +	case HTTP_OK:
> > +		break;
> > +	case HTTP_MISSING_TARGET:
> > +		die("%s not found: did you run git update-server-info on the"
> > +		    " server?", refs_url);
> > +	default:
> > +		http_error(refs_url, http_ret);
> > +		die("HTTP request failed");
> > +	}
> > +	free(refs_url);
> > +
> > +	data = buffer.buf;
> > +	start = NULL;
> > +	mid = data;
> 
> What does "mid" stand for?
> 
> > +	while (i < buffer.len) {
> > +		if (!start) {
> > +			start = &data[i];
> > +		}
> 
> Why curly brackets around this one, but not this:
> 
> > +		if (data[i] == '\t')
> > +			mid = &data[i];
> 
> > +		if (data[i] == '\n') {
> > +			data[i] = 0;
> > +			ref_name = mid + 1;
> > +			ref = xmalloc(sizeof(struct ref) +
> > +				      strlen(ref_name) + 1);
> 
> What is alloc_ref() for?
> 
> > +			memset(ref, 0, sizeof(struct ref));
> > +			strcpy(ref->name, ref_name);
> > +			get_sha1_hex(start, ref->old_sha1);
> > +			if (!refs)
> > +				refs = ref;
> > +			if (last_ref)
> > +				last_ref->next = ref;
> > +			last_ref = ref;
> 
> Is this another "tail"?
> 
> > +			start = NULL;
> > +		}
> > +		i++;
> 
> Oh, so it _is_ a for() loop.
> 
> > +	}
> > +	strbuf_release(&buffer);
> > +
> > +	ref = alloc_ref("HEAD");
> > +	if (!walker->fetch_ref(walker, ref) &&
> > +	    !resolve_remote_symref(ref, refs)) {
> > +		ref->next = refs;
> > +		refs = ref;
> > +	} else {
> > +		free(ref);
> > +	}
> > +
> > +	return refs;
> > +}
> > +
> > +int main(int argc, const char **argv)
> > +{
> > +	struct remote *remote;
> > +	struct strbuf buf = STRBUF_INIT;
> > +	const char *url;
> > +	struct walker *walker = NULL;
> > +
> > +	setup_git_directory();
> > +	if (argc < 2) {
> > +		fprintf(stderr, "Remote needed\n");
> > +		return 1;
> > +	}
> > +
> > +	remote = remote_get(argv[1]);
> > +
> > +	if (argc > 2) {
> > +		url = argv[2];
> > +	} else {
> > +		url = remote->url[0];
> > +	}
> > +
> > +	do {
> > +		if (strbuf_getline(&buf, stdin, '\n') == EOF)
> > +			break;
> > +		if (!prefixcmp(buf.buf, "fetch ")) {
> > +			char *obj = buf.buf + strlen("fetch ");
> > +			if (!walker)
> > +				walker = get_http_walker(url, remote);
> > +			walker->get_all = 1;
> > +			walker->get_tree = 1;
> > +			walker->get_history = 1;
> > +			walker->get_verbosely = 0;
> > +			walker->get_recover = 0;
> > +			if (walker_fetch(walker, 1, &obj, NULL, NULL))
> > +				die("Fetch failed.");
> > +			printf("\n");
> > +			fflush(stdout);
> > +		} else if (!strcmp(buf.buf, "list")) {
> > +			struct ref *refs;
> > +			struct ref *posn;
> > +			if (!walker)
> > +				walker = get_http_walker(url, remote);
> > +			refs = get_refs(walker, url);
> > +			for (posn = refs; posn; posn = posn->next) {
> > +				if (posn->symref)
> > +					printf("@%s %s\n", posn->symref, posn->name);
> > +				else
> > +					printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
> > +			}
> > +			printf("\n");
> > +			fflush(stdout);
> > +		} else {
> > +			return 1;
> > +		}
> > +		strbuf_reset(&buf);
> > +	} while (1);
> > +	return 0;
> > +}
> 
> If you already start some infrastructure like this, you should go the 
> whole nine yards and make helper functions in remote.c or transport.c that 
> help implementing "git-remote-<protocol>" helpers.

I thought of that, but I couldn't think of any helper functions that wouldn't be harder to use than to not use. In any case, they should be in a separate file, since they'd only be used by a set of separate programs.

Show 12 quoted lines
> > diff --git a/transport.c b/transport.c
> > index de0d587..6d13b0e 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
> >  	} else if (!prefixcmp(url, "http://")
> >  	        || !prefixcmp(url, "https://")
> >  	        || !prefixcmp(url, "ftp://")) {
> > +		transport_shim_init(ret, "curl");
> 
> Like I said, identifying the remote helper by anything else than the 
> protocol is a design catastrophe.

But this helper supports all of the protocols supported by curl, and may eventually not be the only available helper that supports some of these protocols. I think it's better to represent this fact by having transport.c explicitly know when to use this helper, rather than linking the same executable to git-remote-https, git-remote-http, git-remote-ftp, etc.

Show 5 quoted lines
> >  #ifdef NO_CURL
> >  		error("git was compiled without libcurl support.");
> 
> Why the heck does this have to be guarded by an #ifdef NO_CURL?  It is the 
> helper's task to say, not transport.c's.
What helper? There is no helper at all if NO_CURL, and:
  git: 'remote-curl' is not a git-command. See 'git --help'.

is not a user-friendly message. For now, I'm keeping the existing behavior.

Show 17 quoted lines
> >  #else
> > -		ret->get_refs_list = get_refs_via_curl;
> > -		ret->fetch = fetch_objs_via_curl;
> >  		ret->push = curl_transport_push;
> >  #endif
> > -		ret->disconnect = disconnect_walker;
> >  
> >  	} else if (is_local(url) && is_file(url)) {
> >  		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
> 
> I think the basic idea of yours is sound, but all I see is some incomplete 
> separation between builtin git-fetch/-push and the helper.  As it stands, 
> git-fetch/-push have to be compiled with the exact knowledge what helper 
> is there, git-fetch/-push are _still_ aware if Git was compiled with http 
> support (so that it is not possible to just install the helper 
> afterwards), and the infrastructure forbids any extension of capabilities 
> in the future.

In the long run, I want to have two methods for running an external helper: one where the installation in some way knows when to run the helper (either by having code in place or by having helpers with particular names or something), and one where the user specifies something explicit. That is, git shouldn't know that a remote with no url but some "codeline" settings that start with "//" is p4, and the user shouldn't have to know that git repositories whose urls start with http:// are done with an external helper, so no one mechanism is sufficient.

It would be nice if git supported querying its environment to see whether a command is available and printing a message suitted to why it was looking for it, but that's a separate topic.

	-Daniel
*This .sig left intentionally blank*
Johannes Schindelin· Jul 29, 2009, 22:16 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl

Hi,
On Tue, 28 Jul 2009, Daniel Barkalow wrote:
Show 37 quoted lines
> On Tue, 28 Jul 2009, Johannes Schindelin wrote:
> 
> > On Tue, 28 Jul 2009, Daniel Barkalow wrote:
> > 
> > > diff --git a/shim-curl.c b/shim-curl.c
> > > new file mode 100644
> > > index 0000000..bde041b
> > > --- /dev/null
> > > +++ b/shim-curl.c
> > > @@ -0,0 +1,132 @@
> > > +#include "cache.h"
> > > +#include "remote.h"
> > > +#include "strbuf.h"
> > > +#include "walker.h"
> > > +#include "http.h"
> > > +
> > > +static struct ref *get_refs(struct walker *walker, const char *url)
> > > +{
> > > +	int http_ret;
> > > +	char *refs_url;
> > > +	struct strbuf buffer = STRBUF_INIT;
> > > +	char *data, *start, *mid;
> > > +	int i = 0;
> > > +	char *ref_name;
> > > +	struct ref *refs = NULL;
> > > +	struct ref *ref = NULL;
> > > +	struct ref *last_ref = NULL;
> > > +
> > > +	refs_url = xmalloc(strlen(url) + 11);
> > > +	sprintf(refs_url, "%s/info/refs", url);
> > 
> > And I thought we had strbufs...
> 
> This whole chunk of code is a direct move from transport.c, which is 
> unfortunately not obvious in the way we represent diffs. I'd prefer to 
> keep the code the same across the move so that we'd be able to track it 
> if "git blame" got especially smart.

... and make review of the current patch -- which is possible right now, without the need of a super clever git blame -- hard? Do you really think this is a wise decision?

Show 68 quoted lines
> > > [a couple of unaddressed comments]
> > >
> > > +int main(int argc, const char **argv)
> > > +{
> > > +	struct remote *remote;
> > > +	struct strbuf buf = STRBUF_INIT;
> > > +	const char *url;
> > > +	struct walker *walker = NULL;
> > > +
> > > +	setup_git_directory();
> > > +	if (argc < 2) {
> > > +		fprintf(stderr, "Remote needed\n");
> > > +		return 1;
> > > +	}
> > > +
> > > +	remote = remote_get(argv[1]);
> > > +
> > > +	if (argc > 2) {
> > > +		url = argv[2];
> > > +	} else {
> > > +		url = remote->url[0];
> > > +	}
> > > +
> > > +	do {
> > > +		if (strbuf_getline(&buf, stdin, '\n') == EOF)
> > > +			break;
> > > +		if (!prefixcmp(buf.buf, "fetch ")) {
> > > +			char *obj = buf.buf + strlen("fetch ");
> > > +			if (!walker)
> > > +				walker = get_http_walker(url, remote);
> > > +			walker->get_all = 1;
> > > +			walker->get_tree = 1;
> > > +			walker->get_history = 1;
> > > +			walker->get_verbosely = 0;
> > > +			walker->get_recover = 0;
> > > +			if (walker_fetch(walker, 1, &obj, NULL, NULL))
> > > +				die("Fetch failed.");
> > > +			printf("\n");
> > > +			fflush(stdout);
> > > +		} else if (!strcmp(buf.buf, "list")) {
> > > +			struct ref *refs;
> > > +			struct ref *posn;
> > > +			if (!walker)
> > > +				walker = get_http_walker(url, remote);
> > > +			refs = get_refs(walker, url);
> > > +			for (posn = refs; posn; posn = posn->next) {
> > > +				if (posn->symref)
> > > +					printf("@%s %s\n", posn->symref, posn->name);
> > > +				else
> > > +					printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
> > > +			}
> > > +			printf("\n");
> > > +			fflush(stdout);
> > > +		} else {
> > > +			return 1;
> > > +		}
> > > +		strbuf_reset(&buf);
> > > +	} while (1);
> > > +	return 0;
> > > +}
> > 
> > If you already start some infrastructure like this, you should go the 
> > whole nine yards and make helper functions in remote.c or transport.c that 
> > help implementing "git-remote-<protocol>" helpers.
> 
> I thought of that, but I couldn't think of any helper functions that 
> wouldn't be harder to use than to not use. In any case, they should be in 
> a separate file, since they'd only be used by a set of separate programs.
So every helper should implement the same parsing of the whole protocol?

I mean, the protocol _lends_ itself to an interface that implements handlers for the commands sent over the pipe, but with compile time errors when, say, a parameter type mistake happened.

Show 16 quoted lines
> > > diff --git a/transport.c b/transport.c
> > > index de0d587..6d13b0e 100644
> > > --- a/transport.c
> > > +++ b/transport.c
> > > @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)
> > >  	} else if (!prefixcmp(url, "http://")
> > >  	        || !prefixcmp(url, "https://")
> > >  	        || !prefixcmp(url, "ftp://")) {
> > > +		transport_shim_init(ret, "curl");
> > 
> > Like I said, identifying the remote helper by anything else than the 
> > protocol is a design catastrophe.
> 
> But this helper supports all of the protocols supported by curl, and may 
> eventually not be the only available helper that supports some of these 
> protocols.

Yes, and in the near future my computer may understand what I say, and guess my thoughts.

Let's stay with the presence, okay?
It is _all_ too easy to hardlink git-remote-http to git-remote-https.

And if you have two different handlers for the same protocol, how should _Git_ decide which one to take?

No, I firmly believe that this design decision is too short-sighted.
> I think it's better to represent this fact by having transport.c 
> explicitly know when to use this helper, rather than linking the same 
> executable to git-remote-https, git-remote-http, git-remote-ftp, etc.

You really want to recompile Git whenever somebody writes a new protocol helper? That's wrong.

Show 7 quoted lines
> > >  #ifdef NO_CURL
> > >  		error("git was compiled without libcurl support.");
> > 
> > Why the heck does this have to be guarded by an #ifdef NO_CURL?  It is the 
> > helper's task to say, not transport.c's.
> 
> What helper? There is no helper at all if NO_CURL,

Again, you want to _require_ that the helpers and Git are compiled together. I stand firmly against this idea.

Show 5 quoted lines
> and:
> 
>   git: 'remote-curl' is not a git-command. See 'git --help'.
> 
> is not a user-friendly message.

Oh? You would want to show the user that message? I wouldn't. I would want to show the user:

	No helper found to handle protocol 'https' (i.e. no 
	git-remote-http was found in the PATH)
Show 23 quoted lines
> > >  #else
> > > -		ret->get_refs_list = get_refs_via_curl;
> > > -		ret->fetch = fetch_objs_via_curl;
> > >  		ret->push = curl_transport_push;
> > >  #endif
> > > -		ret->disconnect = disconnect_walker;
> > >  
> > >  	} else if (is_local(url) && is_file(url)) {
> > >  		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
> > 
> > I think the basic idea of yours is sound, but all I see is some incomplete 
> > separation between builtin git-fetch/-push and the helper.  As it stands, 
> > git-fetch/-push have to be compiled with the exact knowledge what helper 
> > is there, git-fetch/-push are _still_ aware if Git was compiled with http 
> > support (so that it is not possible to just install the helper 
> > afterwards), and the infrastructure forbids any extension of capabilities 
> > in the future.
> 
> In the long run, I want to have two methods for running an external 
> helper: one where the installation in some way knows when to run the 
> helper (either by having code in place or by having helpers with 
> particular names or something), and one where the user specifies something 
> explicit.
I am not sure I like that.
> That is, git shouldn't know that a remote with no url but some 
> "codeline" settings that start with "//" is p4,

What speaks against a URL "p4://..."? That would be _totally_ intuitive and easy to handle.

> and the user shouldn't have to know that git repositories whose urls 
> start with http:// are done with an external helper, so no one mechanism 
> is sufficient.

Why on earth should the user need to know that http:// is handled by an external helper?

Of course, if the user wants to support, say, VSS, it is now easy (well, the Git part) to provide a simple git-remote-vss helper _and not having to recompile Git at all_.

> It would be nice if git supported querying its environment to see whether 
> a command is available and printing a message suitted to why it was 
> looking for it, but that's a separate topic.

We do have such a framework in place already. You even mentioned it. Our git-help can look in the PATH what is available.

But then, we do not have to do that at all.

We just try to handle the protocol internally. If that is not possible, we extract the protocol from the URL and try to call a program called git-remote-<protocol>. We inquire via the capabilities command if that helper can fetch/push and continue accordingly. If the program could not be launched, with exit status 127, we know there was no helper and can tell the user that.

We do not need to do any discovery of the environment at all. start_command() is all we ever need.

Simple is beautiful, Dscho

← back to recent threads