[PATCH] Fix "git clone" for git:// protocol

[PATCH] Fix "git clone" for git:// protocol

[Johannes Schindelin]

In the commit "Reduce the number of connects when fetching", we checked 
the return value of git_connect() to see if the connection was successful.

However, for the git:// protocol, there is no need to have another 
process, so the return value is NULL.

The thing is: git_connect() does not return at all if it fails, so we need 
not check the return value of git_connect().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-fetch-pack.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index f401352..184782f 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -707,17 +707,13 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	conn = git_connect(fd, (char *)dest, args.uploadpack,
 			   args.verbose ? CONNECT_VERBOSE : 0);
-	if (conn) {
-		get_remote_heads(fd[0], &ref, 0, NULL, 0);
-
-		ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
-		close(fd[0]);
-		close(fd[1]);
-		if (finish_connect(conn))
-			ref = NULL;
-	} else {
+	get_remote_heads(fd[0], &ref, 0, NULL, 0);
+
+	ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
+	close(fd[0]);
+	close(fd[1]);
+	if (finish_connect(conn))
 		ref = NULL;
-	}
 	ret = !ref;
 
 	if (!ret && nr_heads) {
-- 
1.5.4.1264.g42770c

Re: [PATCH] Fix "git clone" for git:// protocol

[Daniel Barkalow]

On Sat, 9 Feb 2008, Johannes Schindelin wrote:

> In the commit "Reduce the number of connects when fetching", we checked 
> the return value of git_connect() to see if the connection was successful.
> 
> However, for the git:// protocol, there is no need to have another 
> process, so the return value is NULL.
> 
> The thing is: git_connect() does not return at all if it fails, so we need 
> not check the return value of git_connect().

Huh. Sure enough. Actually, there's a similar problem in transport.c, 
where it assumes that the return value of git_connect is non-zero, which 
makes it not reuse the connection (not that you can really tell). It might 
be good to roll in a fix for that. Or maybe git_connect should return a 
pointer to a static struct child_process if it doesn't need a subprocess, 
just to distinguish "we're doing it ourselves" from "it's not being done"? 
If not, maybe the variables that store the return from git_connect should 
be renamed to "subproc" or something that doesn't suggest they can't be 
NULL if you're actually connected.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix "git clone" for git:// protocol

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Daniel Barkalow wrote:

> On Sat, 9 Feb 2008, Johannes Schindelin wrote:
> 
> > In the commit "Reduce the number of connects when fetching", we 
> > checked the return value of git_connect() to see if the connection was 
> > successful.
> > 
> > However, for the git:// protocol, there is no need to have another 
> > process, so the return value is NULL.
> > 
> > The thing is: git_connect() does not return at all if it fails, so we 
> > need not check the return value of git_connect().
> 
> Huh. Sure enough. Actually, there's a similar problem in transport.c, 
> where it assumes that the return value of git_connect is non-zero, which 
> makes it not reuse the connection (not that you can really tell). It might 
> be good to roll in a fix for that. Or maybe git_connect should return a 
> pointer to a static struct child_process if it doesn't need a subprocess, 
> just to distinguish "we're doing it ourselves" from "it's not being done"? 

Yeah, I like that.  Will do that tomorrow.

Ciao,
Dscho

[PATCH] Fix "git clone" for git:// protocol

[Johannes Schindelin]

In ba227857(Reduce the number of connects when fetching), we checked
the return value of git_connect() to see if the connection was
successful.

However, for the git:// protocol, there is no need to have another
process, so the return value was NULL.

Now, it makes sense to assume the rule that git_connect() will return
NULL if it fails (at the moment, it die()s if it fails), so return
a dummy child process.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	
	On Sat, 9 Feb 2008, Daniel Barkalow wrote:

	> On Sat, 9 Feb 2008, Johannes Schindelin wrote:
	> 
	> > In the commit "Reduce the number of connects when fetching", 
	> > we checked the return value of git_connect() to see if the 
	> > connection was successful.
	> > 
	> > However, for the git:// protocol, there is no need to have 
	> > another process, so the return value is NULL.
	> > 
	> > The thing is: git_connect() does not return at all if it 
	> > fails, so we need not check the return value of git_connect().
	> 
	> Huh. Sure enough. Actually, there's a similar problem in 
	> transport.c, where it assumes that the return value of 
	> git_connect is non-zero, which makes it not reuse the connection 
	> (not that you can really tell). It might be good to roll in a 
	> fix for that. Or maybe git_connect should return a pointer to a 
	> static struct child_process if it doesn't need a subprocess, 
	> just to distinguish "we're doing it ourselves" from "it's not 
	> being done"? If not, maybe the variables that store the return 
	> from git_connect should be renamed to "subproc" or something 
	> that doesn't suggest they can't be NULL if you're actually 
	> connected.

	How about this?

 connect.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index 3aefd4a..700ceba 100644
--- a/connect.c
+++ b/connect.c
@@ -472,14 +472,18 @@ char *get_port(char *host)
 	return NULL;
 }
 
+static struct child_process no_fork;
+
 /*
- * This returns NULL if the transport protocol does not need fork(2), or a
- * struct child_process object if it does.  Once done, finish the connection
- * with finish_connect() with the value returned from this function
- * (it is safe to call finish_connect() with NULL to support the former
- * case).
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does.  Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
  *
- * If it returns, the connect is successful; it just dies on errors.
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
  */
 struct child_process *git_connect(int fd[2], const char *url_orig,
 				  const char *prog, int flags)
@@ -577,7 +581,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		free(url);
 		if (free_path)
 			free(path);
-		return NULL;
+		return &no_fork;
 	}
 
 	conn = xcalloc(1, sizeof(*conn));
@@ -635,7 +639,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 int finish_connect(struct child_process *conn)
 {
 	int code;
-	if (!conn)
+	if (!conn || conn == &no_fork)
 		return 0;
 
 	code = finish_command(conn);
-- 
1.5.4.1264.g42770c

Re: [PATCH] Fix "git clone" for git:// protocol

[Daniel Barkalow]

On Sun, 10 Feb 2008, Johannes Schindelin wrote:

> 	How about this?

Looks good to me.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix "git clone" for git:// protocol

[Junio C Hamano]

Thanks.  The patch makes sense.

I wonder this deserves a new test case to protect the fix from
future regressions.

Re: [PATCH] Fix "git clone" for git:// protocol

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Junio C Hamano wrote:

> Thanks.  The patch makes sense.
> 
> I wonder this deserves a new test case to protect the fix from future 
> regressions.

It would probably make sense; this would also exercise git-daemon for the 
first time in our test suite.

Will work on it right away.

Ciao,
Dscho

[PATCH] Add a test for git-daemon and clone via git:// protocol

[Johannes Schindelin]

The new test, t5703-daemon.sh, sets up a simple git-daemon at port 8111
(you can override it with the environment variable GIT_DAEMON_TEST_PORT),
and then tries to clone via git:// from it, first without the
git-daemon-export-ok file (which should fail), and then with it (which
should succeed).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---


	On Sun, 10 Feb 2008, Johannes Schindelin wrote:

	> On Sat, 9 Feb 2008, Junio C Hamano wrote:
	> 
	> > Thanks.  The patch makes sense.
	> > 
	> > I wonder this deserves a new test case to protect the fix from 
	> > future regressions.
	> 
	> It would probably make sense; this would also exercise 
	> git-daemon for the first time in our test suite.

	How's this?

 t/t5703-daemon.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100755 t/t5703-daemon.sh

diff --git a/t/t5703-daemon.sh b/t/t5703-daemon.sh
new file mode 100755
index 0000000..10696a1
--- /dev/null
+++ b/t/t5703-daemon.sh
@@ -0,0 +1,55 @@
+#!/bin/sh
+
+test_description='git daemon and cloning via git:// protocol'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	mkdir first &&
+	(cd first &&
+	 git init &&
+	 echo biomimicry > ted &&
+	 git add ted &&
+	 test_tick &&
+	 git commit -m initial &&
+	 git gc)
+
+'
+
+PORT=${GIT_DAEMON_TEST_PORT:-8111}
+
+start_daemon () {
+	trap 'kill "$DAEMON_PID"' 0
+	git daemon --base-path="$(pwd)" \
+		--port="$PORT" \
+		--detach \
+		--pid-file=pid-file \
+		-- "$(pwd)/first"
+	DAEMON_PID=$(cat pid-file)
+}
+
+test_expect_success 'daemon' '
+
+	start_daemon &&
+	test -f pid-file
+
+'
+
+test_expect_success 'clone fails without export-ok' '
+
+	! git clone git://127.0.0.1:"$PORT"/first second
+
+'
+
+test_expect_success 'clone succeeds with export-ok' '
+
+	: > first/.git/git-daemon-export-ok &&
+	git clone git://127.0.0.1:"$PORT"/first second &&
+	for f in refs/heads/master objects/pack/*.pack
+	do
+		cmp first/.git/$f second/.git/$f || break
+	done
+
+'
+
+test_done
-- 
1.5.4.1264.gb53928