[PATCH 0/2] Move push logic to transport.c

[PATCH 0/2] Move push logic to transport.c

[Daniel Barkalow]

Currently, send_pack() does the work of figuring out what the refspecs 
mean before pushing (with the help of functions from remote.c), updating 
local state afterwards, and reporting what it did.

Transports other than the native protocol use the same logic for the first 
of these, and don't do the other two at all. In order to be able to share 
more code and make it easier to be consist, move the logic to 
transport_push. This is also more similar to how fetch is organized, 
although fetch puts most of the logic in builtin-fetch, which isn't really 
feasible until all of the transports are converted to the new functions, 
and may be less convenient anyway.

This series only goes as far as adding a new "push_refs" method to struct 
transport and using it for the git native transport. It doesn't convert 
http-push or the rsync transports, largely because I don't have test 
setups for rsync or webdav to make sure that they're still working.

It also leaves copies of matching, updating, and reporting code in 
builtin-send-pack, but these are only used for "git send-pack", and are 
not used in the code paths for "git push". Hopefully, we can deprecate the 
protocol-specific command at some point in favor of just using "git push".

This is on top of next for Jay's patch to make get_local_heads() common.

Daniel Barkalow (2):
  Use a common function to get the pretty name of refs
  Move push matching and reporting logic into transport.c

 builtin-fetch.c     |    6 +-
 builtin-send-pack.c |  153 +++++++++++++--------------
 refs.c              |   10 ++
 refs.h              |    2 +
 send-pack.h         |    6 +-
 transport.c         |  283 +++++++++++++++++++++++++++++++++++++++++++++++----
 transport.h         |    3 +-
 7 files changed, 356 insertions(+), 107 deletions(-)

Re: [PATCH 0/2] Move push logic to transport.c

[Johannes Schindelin]

Hi,

On Sun, 8 Mar 2009, Daniel Barkalow wrote:

> It doesn't convert http-push or the rsync transports, largely because I 
> don't have test setups for rsync or webdav to make sure that they're 
> still working.

$ ls t/*http-push*
t/t5540-http-push.sh

$ git grep -n test.*rsync t/
t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '

It should be just a matter of installing an apache and rsync.

BTW thanks for the patch, I guess it will help Peff to complete "push 
--track" properly ;-)

Ciao,
Dscho

Re: [PATCH 0/2] Move push logic to transport.c

[Jeff King]

On Mon, Mar 09, 2009 at 10:35:45AM +0100, Johannes Schindelin wrote:

> BTW thanks for the patch, I guess it will help Peff to complete "push 
> --track" properly ;-)

If I wait long enough, maybe my original patch will Just Work. ;)

-Peff

Re: [PATCH 0/2] Move push logic to transport.c

[Daniel Barkalow]

On Mon, 9 Mar 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> 
> > It doesn't convert http-push or the rsync transports, largely because I 
> > don't have test setups for rsync or webdav to make sure that they're 
> > still working.
> 
> $ ls t/*http-push*
> t/t5540-http-push.sh
> 
> $ git grep -n test.*rsync t/
> t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> 
> It should be just a matter of installing an apache and rsync.

And configuring them suitably, yes. That's the part I haven't previously 
done.

> BTW thanks for the patch, I guess it will help Peff to complete "push 
> --track" properly ;-)

Yes, I expect so.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/2] Move push logic to transport.c

[Johannes Schindelin]

Hi,

On Mon, 9 Mar 2009, Daniel Barkalow wrote:

> On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> 
> > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > 
> > > It doesn't convert http-push or the rsync transports, largely 
> > > because I don't have test setups for rsync or webdav to make sure 
> > > that they're still working.
> > 
> > $ ls t/*http-push*
> > t/t5540-http-push.sh
> > 
> > $ git grep -n test.*rsync t/
> > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > 
> > It should be just a matter of installing an apache and rsync.
> 
> And configuring them suitably, yes. That's the part I haven't previously 
> done.

If you have to configure apache (or rsync) for the test to run properly, 
we have a serious bug in our test suite.  Please share the output in that 
case.

Ciao,
Dscho

Re: [PATCH 0/2] Move push logic to transport.c

[Daniel Barkalow]

On Mon, 9 Mar 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> 
> > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > 
> > > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > > 
> > > > It doesn't convert http-push or the rsync transports, largely 
> > > > because I don't have test setups for rsync or webdav to make sure 
> > > > that they're still working.
> > > 
> > > $ ls t/*http-push*
> > > t/t5540-http-push.sh
> > > 
> > > $ git grep -n test.*rsync t/
> > > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > > 
> > > It should be just a matter of installing an apache and rsync.
> > 
> > And configuring them suitably, yes. That's the part I haven't previously 
> > done.
> 
> If you have to configure apache (or rsync) for the test to run properly, 
> we have a serious bug in our test suite.  Please share the output in that 
> case.

rsync: failed to connect to 127.0.0.1: Connection refused (111)
rsync error: error in socket IO (code 10) at clientserver.c(104) [receiver=2.6.9]
fatal: Could not run rsync to get refs

I mean, I won't need to do anything really special in the configuration, 
but I do need to have rsyncd running giving access to that directory 
without any funny mapping on localhost.

The apache test does, in fact, run its own server, so that should be easy 
enough (although I do need to install an apache with mod_dav.so, which I 
don't have handy).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/2] Move push logic to transport.c

[Johannes Schindelin]

Hi,

On Mon, 9 Mar 2009, Daniel Barkalow wrote:

> On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> 
> > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > 
> > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > 
> > > > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > > > 
> > > > > It doesn't convert http-push or the rsync transports, largely 
> > > > > because I don't have test setups for rsync or webdav to make sure 
> > > > > that they're still working.
> > > > 
> > > > $ ls t/*http-push*
> > > > t/t5540-http-push.sh
> > > > 
> > > > $ git grep -n test.*rsync t/
> > > > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > > > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > > > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > > > 
> > > > It should be just a matter of installing an apache and rsync.
> > > 
> > > And configuring them suitably, yes. That's the part I haven't previously 
> > > done.
> > 
> > If you have to configure apache (or rsync) for the test to run properly, 
> > we have a serious bug in our test suite.  Please share the output in that 
> > case.
> 
> rsync: failed to connect to 127.0.0.1: Connection refused (111)
> rsync error: error in socket IO (code 10) at clientserver.c(104) [receiver=2.6.9]
> fatal: Could not run rsync to get refs
> 
> I mean, I won't need to do anything really special in the configuration, 
> but I do need to have rsyncd running giving access to that directory 
> without any funny mapping on localhost.

That is my mistake, I guess.  I tried to be lazy and not change 
transport.c so that it groks "rsync:$(pwd)" style paths.

Sorry,
Dscho

Re: [PATCH 0/2] Move push logic to transport.c

[Daniel Barkalow]

On Mon, 9 Mar 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> 
> > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > 
> > > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > > 
> > > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > > 
> > > > > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > > > > 
> > > > > > It doesn't convert http-push or the rsync transports, largely 
> > > > > > because I don't have test setups for rsync or webdav to make sure 
> > > > > > that they're still working.
> > > > > 
> > > > > $ ls t/*http-push*
> > > > > t/t5540-http-push.sh
> > > > > 
> > > > > $ git grep -n test.*rsync t/
> > > > > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > > > > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > > > > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > > > > 
> > > > > It should be just a matter of installing an apache and rsync.
> > > > 
> > > > And configuring them suitably, yes. That's the part I haven't previously 
> > > > done.
> > > 
> > > If you have to configure apache (or rsync) for the test to run properly, 
> > > we have a serious bug in our test suite.  Please share the output in that 
> > > case.
> > 
> > rsync: failed to connect to 127.0.0.1: Connection refused (111)
> > rsync error: error in socket IO (code 10) at clientserver.c(104) [receiver=2.6.9]
> > fatal: Could not run rsync to get refs
> > 
> > I mean, I won't need to do anything really special in the configuration, 
> > but I do need to have rsyncd running giving access to that directory 
> > without any funny mapping on localhost.
> 
> That is my mistake, I guess.  I tried to be lazy and not change 
> transport.c so that it groks "rsync:$(pwd)" style paths.

Oh, I think it's probably not worth allowing rsync:$(pwd) (particularly 
since rsync itself seems not to let you say "rsync:" for local paths). 
I'll set something up to run rsyncd with an appropriate config, like the 
lib-httpd stuff. Oh, and it looks like nobody actually runs the rsync 
tests; they're broken by $(pwd) including whitespace, and haven't been 
fixed yet.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/2] Move push logic to transport.c

[Johannes Schindelin]

Hi,

On Mon, 9 Mar 2009, Daniel Barkalow wrote:

> On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> 
> > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > 
> > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > 
> > > > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > > > 
> > > > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > > > 
> > > > > > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > > > > > 
> > > > > > > It doesn't convert http-push or the rsync transports, largely 
> > > > > > > because I don't have test setups for rsync or webdav to make sure 
> > > > > > > that they're still working.
> > > > > > 
> > > > > > $ ls t/*http-push*
> > > > > > t/t5540-http-push.sh
> > > > > > 
> > > > > > $ git grep -n test.*rsync t/
> > > > > > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > > > > > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > > > > > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > > > > > 
> > > > > > It should be just a matter of installing an apache and rsync.
> > > > > 
> > > > > And configuring them suitably, yes. That's the part I haven't previously 
> > > > > done.
> > > > 
> > > > If you have to configure apache (or rsync) for the test to run properly, 
> > > > we have a serious bug in our test suite.  Please share the output in that 
> > > > case.
> > > 
> > > rsync: failed to connect to 127.0.0.1: Connection refused (111)
> > > rsync error: error in socket IO (code 10) at clientserver.c(104) [receiver=2.6.9]
> > > fatal: Could not run rsync to get refs
> > > 
> > > I mean, I won't need to do anything really special in the configuration, 
> > > but I do need to have rsyncd running giving access to that directory 
> > > without any funny mapping on localhost.
> > 
> > That is my mistake, I guess.  I tried to be lazy and not change 
> > transport.c so that it groks "rsync:$(pwd)" style paths.
> 
> Oh, I think it's probably not worth allowing rsync:$(pwd) (particularly 
> since rsync itself seems not to let you say "rsync:" for local paths). 

This contradicts that:

> I'll set something up to run rsyncd with an appropriate config, like the 
> lib-httpd stuff. Oh, and it looks like nobody actually runs the rsync 
> tests; they're broken by $(pwd) including whitespace, and haven't been 
> fixed yet.

If we would support local rsync paths, testing it would be easy.

Ciao,
Dscho

Re: [PATCH 0/2] Move push logic to transport.c

[Daniel Barkalow]

On Mon, 9 Mar 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> 
> > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > 
> > > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > > 
> > > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > > 
> > > > > On Mon, 9 Mar 2009, Daniel Barkalow wrote:
> > > > > 
> > > > > > On Mon, 9 Mar 2009, Johannes Schindelin wrote:
> > > > > > 
> > > > > > > On Sun, 8 Mar 2009, Daniel Barkalow wrote:
> > > > > > > 
> > > > > > > > It doesn't convert http-push or the rsync transports, largely 
> > > > > > > > because I don't have test setups for rsync or webdav to make sure 
> > > > > > > > that they're still working.
> > > > > > > 
> > > > > > > $ ls t/*http-push*
> > > > > > > t/t5540-http-push.sh
> > > > > > > 
> > > > > > > $ git grep -n test.*rsync t/
> > > > > > > t/t5510-fetch.sh:195:test_expect_success 'fetch via rsync' '
> > > > > > > t/t5510-fetch.sh:206:test_expect_success 'push via rsync' '
> > > > > > > t/t5510-fetch.sh:217:test_expect_success 'push via rsync' '
> > > > > > > 
> > > > > > > It should be just a matter of installing an apache and rsync.
> > > > > > 
> > > > > > And configuring them suitably, yes. That's the part I haven't previously 
> > > > > > done.
> > > > > 
> > > > > If you have to configure apache (or rsync) for the test to run properly, 
> > > > > we have a serious bug in our test suite.  Please share the output in that 
> > > > > case.
> > > > 
> > > > rsync: failed to connect to 127.0.0.1: Connection refused (111)
> > > > rsync error: error in socket IO (code 10) at clientserver.c(104) [receiver=2.6.9]
> > > > fatal: Could not run rsync to get refs
> > > > 
> > > > I mean, I won't need to do anything really special in the configuration, 
> > > > but I do need to have rsyncd running giving access to that directory 
> > > > without any funny mapping on localhost.
> > > 
> > > That is my mistake, I guess.  I tried to be lazy and not change 
> > > transport.c so that it groks "rsync:$(pwd)" style paths.
> > 
> > Oh, I think it's probably not worth allowing rsync:$(pwd) (particularly 
> > since rsync itself seems not to let you say "rsync:" for local paths). 
> 
> This contradicts that:
> 
> > I'll set something up to run rsyncd with an appropriate config, like the 
> > lib-httpd stuff. Oh, and it looks like nobody actually runs the rsync 
> > tests; they're broken by $(pwd) including whitespace, and haven't been 
> > fixed yet.
> 
> If we would support local rsync paths, testing it would be easy.

Ok, I think having transport_get strip off the "rsync:" would make it work 
with the rest of the functions unchanged, which would in turn mean that 
the tests would be a pretty reasonable approximation of the real-world 
code.

	-Daniel
*This .sig left intentionally blank*

[PATCH] rsync transport: allow local paths, and fix tests

[Johannes Schindelin]

Earlier, the rsync tests were disabled by default, as they needed a
running rsyncd daemon.  This was only due to the limitation that our
rsync transport only allowed full URLs of the form

	rsync://<host>/<path>

Relaxing the URLs to allow

	rsync:<path>

permitted the change in the tests to run whenever rsync is available,
without requiring a fully configured and running rsyncd.

While at it, the tests were fixed so that they run in directories with a 
space in their name.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5510-fetch.sh |   39 ++++++++++++++++++++-------------------
 transport.c      |   23 +++++++++++++++--------
 2 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 9e679b4..bee3424 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -191,38 +191,39 @@ test_expect_success 'bundle should be able to create a full history' '
 
 '
 
-test "$TEST_RSYNC" && {
+! rsync --help > /dev/null 2> /dev/null &&
+say 'Skipping rsync tests because rsync was not found' || {
 test_expect_success 'fetch via rsync' '
 	git pack-refs &&
 	mkdir rsynced &&
-	cd rsynced &&
-	git init &&
-	git fetch rsync://127.0.0.1$(pwd)/../.git master:refs/heads/master &&
-	git gc --prune &&
-	test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
-	git fsck --full
+	(cd rsynced &&
+	 git init --bare &&
+	 git fetch "rsync:$(pwd)/../.git" master:refs/heads/master &&
+	 git gc --prune &&
+	 test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+	 git fsck --full)
 '
 
 test_expect_success 'push via rsync' '
-	mkdir ../rsynced2 &&
-	(cd ../rsynced2 &&
+	mkdir rsynced2 &&
+	(cd rsynced2 &&
 	 git init) &&
-	git push rsync://127.0.0.1$(pwd)/../rsynced2/.git master &&
-	cd ../rsynced2 &&
-	git gc --prune &&
-	test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
-	git fsck --full
+	(cd rsynced &&
+	 git push "rsync:$(pwd)/../rsynced2/.git" master) &&
+	(cd rsynced2 &&
+	 git gc --prune &&
+	 test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+	 git fsck --full)
 '
 
 test_expect_success 'push via rsync' '
-	cd .. &&
 	mkdir rsynced3 &&
 	(cd rsynced3 &&
 	 git init) &&
-	git push --all rsync://127.0.0.1$(pwd)/rsynced3/.git &&
-	cd rsynced3 &&
-	test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
-	git fsck --full
+	git push --all "rsync:$(pwd)/rsynced3/.git" &&
+	(cd rsynced3 &&
+	 test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+	 git fsck --full)
 '
 }
 
diff --git a/transport.c b/transport.c
index 7bc5a55..3015ed1 100644
--- a/transport.c
+++ b/transport.c
@@ -138,6 +138,11 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 	}
 }
 
+static const char *rsync_url(const char *url)
+{
+	return prefixcmp(url, "rsync://") ? skip_prefix(url, "rsync:") : url;
+}
+
 static struct ref *get_refs_via_rsync(struct transport *transport)
 {
 	struct strbuf buf = STRBUF_INIT, temp_dir = STRBUF_INIT;
@@ -153,7 +158,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport)
 		die ("Could not make temporary directory");
 	temp_dir_len = temp_dir.len;
 
-	strbuf_addstr(&buf, transport->url);
+	strbuf_addstr(&buf, rsync_url(transport->url));
 	strbuf_addstr(&buf, "/refs");
 
 	memset(&rsync, 0, sizeof(rsync));
@@ -169,7 +174,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport)
 		die ("Could not run rsync to get refs");
 
 	strbuf_reset(&buf);
-	strbuf_addstr(&buf, transport->url);
+	strbuf_addstr(&buf, rsync_url(transport->url));
 	strbuf_addstr(&buf, "/packed-refs");
 
 	args[2] = buf.buf;
@@ -206,7 +211,7 @@ static int fetch_objs_via_rsync(struct transport *transport,
 	const char *args[8];
 	int result;
 
-	strbuf_addstr(&buf, transport->url);
+	strbuf_addstr(&buf, rsync_url(transport->url));
 	strbuf_addstr(&buf, "/objects/");
 
 	memset(&rsync, 0, sizeof(rsync));
@@ -285,7 +290,7 @@ static int rsync_transport_push(struct transport *transport,
 
 	/* first push the objects */
 
-	strbuf_addstr(&buf, transport->url);
+	strbuf_addstr(&buf, rsync_url(transport->url));
 	strbuf_addch(&buf, '/');
 
 	memset(&rsync, 0, sizeof(rsync));
@@ -306,7 +311,8 @@ static int rsync_transport_push(struct transport *transport,
 	args[i++] = NULL;
 
 	if (run_command(&rsync))
-		return error("Could not push objects to %s", transport->url);
+		return error("Could not push objects to %s",
+				rsync_url(transport->url));
 
 	/* copy the refs to the temporary directory; they could be packed. */
 
@@ -327,10 +333,11 @@ static int rsync_transport_push(struct transport *transport,
 	if (!(flags & TRANSPORT_PUSH_FORCE))
 		args[i++] = "--ignore-existing";
 	args[i++] = temp_dir.buf;
-	args[i++] = transport->url;
+	args[i++] = rsync_url(transport->url);
 	args[i++] = NULL;
 	if (run_command(&rsync))
-		result = error("Could not push to %s", transport->url);
+		result = error("Could not push to %s",
+				rsync_url(transport->url));
 
 	if (remove_dir_recursively(&temp_dir, 0))
 		warning ("Could not remove temporary directory %s.",
@@ -705,7 +712,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	ret->remote = remote;
 	ret->url = url;
 
-	if (!prefixcmp(url, "rsync://")) {
+	if (!prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
-- 
1.6.2.363.g9793e