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

Hi,

On Tue, 28 Jul 2009, Daniel Barkalow wrote:

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?

And I thought we had strbufs...

+
+	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.

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.

 #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