[PATCH] http-push: Fix error message for push <remote> :<branch>

[PATCH] http-push: Fix error message for push <remote> :<branch>

[Clemens Buchacher]

git http-push <remote> :<branch> currently fails with the error message
"cannot happen anymore". Print a more useful error message instead.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

We should actually support this feature, of course. Unfortunately, a lot has
changed since the version of send-pack which http-push is based on
(v1.2.4-398-gaa1dbc9). I think http-push should be updated to reflect these
changes at a larger scope. Is there any work going on in this direction?

 http-push.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index f9b77d6..8de091f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2311,6 +2311,15 @@ int main(int argc, char **argv)
 
 		if (!ref->peer_ref)
 			continue;
+
+		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
+			error("Deleting remote branches via :<branch> is not "
+				"currently supported by http-push. Use git "
+				"http-push -d <remote-url> <branch> instead.");
+			rc = -2;
+			continue;
+		}
+
 		if (!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (push_verbosely || 1)
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
-- 
1.5.4.2.183.g1ae81

Re: [PATCH] http-push: Fix error message for push <remote> :<branch>

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Clemens Buchacher wrote:

> git http-push <remote> :<branch> currently fails with the error message 
> "cannot happen anymore". Print a more useful error message instead.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> 
> We should actually support this feature, of course. Unfortunately, a lot 
> has changed since the version of send-pack which http-push is based on 
> (v1.2.4-398-gaa1dbc9). I think http-push should be updated to reflect 
> these changes at a larger scope. Is there any work going on in this 
> direction?

Nope.  You are the ideal candidate to start, though.

> diff --git a/http-push.c b/http-push.c
> index f9b77d6..8de091f 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -2311,6 +2311,15 @@ int main(int argc, char **argv)
>  
>  		if (!ref->peer_ref)
>  			continue;
> +
> +		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
> +			error("Deleting remote branches via :<branch> is not "
> +				"currently supported by http-push. Use git "
> +				"http-push -d <remote-url> <branch> instead.");
> +			rc = -2;
> +			continue;
> +		}
> +

You forgot to remove the now-obsolete warning...

Besides, why don't you just try to imitate the code for "-d"?  It says

        if (delete_branch) {
                if (delete_remote_branch(refspec[0], force_delete) == -1)
                        fprintf(stderr, "Unable to delete remote branch %s\n",
                                refspec[0]);
                goto cleanup;
        }

So I think something like this should work, at the same place you added 
your code:

		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
			if (delete_remote_branch(ref->peer_ref->name,
					force_delete || ref->force) == -1) {
				error("Could not remove %s",
					ref->peer_ref->name);
				rc = -4;
			}
			continue;
		}

Care to try that?

Ciao,
Dscho

[PATCH] http-push: push <remote> :<branch> deletes remote branch

[Clemens Buchacher]

This mirrors current ssh/git push syntax.
---
On Mon, Feb 18, 2008 at 01:45:08PM +0000, Johannes Schindelin wrote:
> Besides, why don't you just try to imitate the code for "-d"?

I was hesitant to implement something that will only partially mirror the
behavior of git/ssh push. For example, git/ssh push will also update the
corresponding remote-tracking branches and give a status report
(new/deleted/updated/rejected etc.) at the end.

After playing with the code a little I feel more confident that the other
issues with http-push are unrelated to this patch and the differences between
git/ssh push and http-push are acceptable for now.

> So I think something like this should work, at the same place you added 
> your code:
> 
> 		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
> 			if (delete_remote_branch(ref->peer_ref->name,
> 					force_delete || ref->force) == -1) {
> 				error("Could not remove %s",
> 					ref->peer_ref->name);
> 				rc = -4;
> 			}
> 			continue;
> 		}

Indeed. According to the rules listed in builtin-send-pack.c:442, I changed
(force_delete || ref->force) to 1.

Clemens

 http-push.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index f9b77d6..e98c52f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2138,6 +2138,8 @@ static int delete_remote_branch(char *pattern, int force)
 
 	/* Send delete request */
 	fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name);
+	if (dry_run)
+		return 0;
 	url = xmalloc(strlen(remote->url) + strlen(remote_ref->name) + 1);
 	sprintf(url, "%s%s", remote->url, remote_ref->name);
 	slot = get_active_slot();
@@ -2311,6 +2313,17 @@ int main(int argc, char **argv)
 
 		if (!ref->peer_ref)
 			continue;
+
+		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
+			if (delete_remote_branch(ref->name, 1) == -1) {
+				error("Could not remove %s",
+					ref->peer_ref->name);
+				rc = -4;
+			}
+			new_refs++;
+			continue;
+		}
+
 		if (!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (push_verbosely || 1)
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
@@ -2342,11 +2355,6 @@ int main(int argc, char **argv)
 			}
 		}
 		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		if (is_zero_sha1(ref->new_sha1)) {
-			error("cannot happen anymore");
-			rc = -3;
-			continue;
-		}
 		new_refs++;
 		strcpy(old_hex, sha1_to_hex(ref->old_sha1));
 		new_hex = sha1_to_hex(ref->new_sha1);
-- 
1.5.4.2.183.g69d3

[PATCH] http-push: push <remote> :<branch> deletes remote branch

[Clemens Buchacher]

This mirrors current ssh/git push syntax.
---

The previous patch gives the wrong branch name in the failure message.

Clemens

 http-push.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index f9b77d6..2c928fc 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2138,6 +2138,8 @@ static int delete_remote_branch(char *pattern, int force)
 
 	/* Send delete request */
 	fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name);
+	if (dry_run)
+		return 0;
 	url = xmalloc(strlen(remote->url) + strlen(remote_ref->name) + 1);
 	sprintf(url, "%s%s", remote->url, remote_ref->name);
 	slot = get_active_slot();
@@ -2311,6 +2313,16 @@ int main(int argc, char **argv)
 
 		if (!ref->peer_ref)
 			continue;
+
+		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
+			if (delete_remote_branch(ref->name, 1) == -1) {
+				error("Could not remove %s", ref->name);
+				rc = -4;
+			}
+			new_refs++;
+			continue;
+		}
+
 		if (!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (push_verbosely || 1)
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
@@ -2342,11 +2354,6 @@ int main(int argc, char **argv)
 			}
 		}
 		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		if (is_zero_sha1(ref->new_sha1)) {
-			error("cannot happen anymore");
-			rc = -3;
-			continue;
-		}
 		new_refs++;
 		strcpy(old_hex, sha1_to_hex(ref->old_sha1));
 		new_hex = sha1_to_hex(ref->new_sha1);
-- 
1.5.4.2.183.g69d3

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Clemens Buchacher wrote:

> This mirrors current ssh/git push syntax.
> ---
> 
> The previous patch gives the wrong branch name in the failure message.

Oh?  ref->peer_ref is not the remote side?

Ciao,
Dscho

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Clemens Buchacher wrote:

> diff --git a/http-push.c b/http-push.c
> index f9b77d6..e98c52f 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -2138,6 +2138,8 @@ static int delete_remote_branch(char *pattern, int force)
>  
>  	/* Send delete request */
>  	fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name);
> +	if (dry_run)
> +		return 0;

Good catch.

Thanks for the patch, I think it good.  Now, if we only had automated 
tests... ;-)

Ciao,
Dscho

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Clemens Buchacher]

Hi,

On Mon, Feb 18, 2008 at 05:34:45PM +0000, Johannes Schindelin wrote:
> Thanks for the patch, I think it good.  Now, if we only had automated 
> tests... ;-)

Thank you. Implementing a test should be fairly difficult, since we need at
least a WebDAV capable webserver, right? Do you having something in mind?

And yes, 'peer' apparently refers to the local end in this case.

Clemens

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Clemens Buchacher wrote:

> On Mon, Feb 18, 2008 at 05:34:45PM +0000, Johannes Schindelin wrote:
>
> > Thanks for the patch, I think it good.  Now, if we only had automated 
> > tests... ;-)
> 
> Thank you. Implementing a test should be fairly difficult, since we need 
> at least a WebDAV capable webserver, right? Do you having something in 
> mind?

Actually, thinking about it again, it should not be _that_ difficult.  We 
would need to ship a httpd.conf.in, pick a port (maybe 8111, because that 
is already used in t5703-daemon.sh), generate httpd.conf and then start 
Apache with that.

If Apache does not want to start, skip this test.  (This might happen due 
to Apache not being installed, or the DAV modules not being installed, or 
Apache not finding them, or...)

And of course, we would want to have something like NO_SVN_TESTS (maybe 
NO_APACHE_TESTS) to be able to skip the tests, since they are pretty 
expensive (starting Apache and all).

> And yes, 'peer' apparently refers to the local end in this case.

Thanks for the clarification.

Ciao,
Dscho

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Mike Hommey]

On Tue, Feb 19, 2008 at 01:17:24PM +0000, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Tue, 19 Feb 2008, Clemens Buchacher wrote:
> 
> > On Mon, Feb 18, 2008 at 05:34:45PM +0000, Johannes Schindelin wrote:
> >
> > > Thanks for the patch, I think it good.  Now, if we only had automated 
> > > tests... ;-)
> > 
> > Thank you. Implementing a test should be fairly difficult, since we need 
> > at least a WebDAV capable webserver, right? Do you having something in 
> > mind?
> 
> Actually, thinking about it again, it should not be _that_ difficult.  We 
> would need to ship a httpd.conf.in, pick a port (maybe 8111, because that 
> is already used in t5703-daemon.sh), generate httpd.conf and then start 
> Apache with that.
> 
> If Apache does not want to start, skip this test.  (This might happen due 
> to Apache not being installed, or the DAV modules not being installed, or 
> Apache not finding them, or...)

Except that means coping with the fact that modules are probably not
installed in the same place on all distros, or can have been built in
apache instead of being DSOs, etc.
The server itself doesn't even have the same name across distros (httpd vs.
apache vs. apache2)

Mike

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Mike Hommey wrote:

> On Tue, Feb 19, 2008 at 01:17:24PM +0000, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > On Tue, 19 Feb 2008, Clemens Buchacher wrote:
> > 
> > > On Mon, Feb 18, 2008 at 05:34:45PM +0000, Johannes Schindelin wrote:
> > >
> > > > Thanks for the patch, I think it good.  Now, if we only had 
> > > > automated tests... ;-)
> > > 
> > > Thank you. Implementing a test should be fairly difficult, since we 
> > > need at least a WebDAV capable webserver, right? Do you having 
> > > something in mind?
> > 
> > Actually, thinking about it again, it should not be _that_ difficult.  
> > We would need to ship a httpd.conf.in, pick a port (maybe 8111, 
> > because that is already used in t5703-daemon.sh), generate httpd.conf 
> > and then start Apache with that.
> > 
> > If Apache does not want to start, skip this test.  (This might happen 
> > due to Apache not being installed, or the DAV modules not being 
> > installed, or Apache not finding them, or...)
> 
> Except that means coping with the fact that modules are probably not 
> installed in the same place on all distros, or can have been built in 
> apache instead of being DSOs, etc. The server itself doesn't even have 
> the same name across distros (httpd vs. apache vs. apache2)

I think this would not be too much of a hassle.  These days, an Apache2 is 
installed on many machines, and it is better to have tests, than to have 
no tests, even if they are not exercised everywhere, right?

Besides, for those machines that do not have Apache installed (such as 
MinGW based setups), we can always add yet another environment variable, 
say "HTTP_PUSH_TEST_URL" to tell the test that there is already a 
web server set up at that URL, ready to be used.

Ciao,
Dscho

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I think this would not be too much of a hassle.  These days, an Apache2 is 
> installed on many machines, and it is better to have tests, than to have 
> no tests, even if they are not exercised everywhere, right?

I think a sensible guideline would be:

 - the core tools are tested by default everywhere;

 - i18n bits and foreign SCM interface are tested by default,
   but should allow opting-out;

 - networking test that needs to open listening ports should be
   off by default but should allow opting-in.

Re: [PATCH] http-push: push <remote> :<branch> deletes remote branch

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I think this would not be too much of a hassle.  These days, an 
> > Apache2 is installed on many machines, and it is better to have tests, 
> > than to have no tests, even if they are not exercised everywhere, 
> > right?
> 
> I think a sensible guideline would be:
> 
>  - the core tools are tested by default everywhere;
> 
>  - i18n bits and foreign SCM interface are tested by default,
>    but should allow opting-out;
> 
>  - networking test that needs to open listening ports should be
>    off by default but should allow opting-in.

Fair enough.  Although there is nothing there to debate about, yet.

Ciao,
Dscho

[PATCH] http-push: add regression tests

[Clemens Buchacher]

These tests require a webserver. The executable path and listening port
are configured using the environment variables

HTTPD_PATH (default: /usr/sbin/apache2) and
HTTPD_PORT (default: 8111),

respectively.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t5540-http-push.sh |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 t/t5540-http-push.sh

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
new file mode 100644
index 0000000..60210ff
--- /dev/null
+++ b/t/t5540-http-push.sh
@@ -0,0 +1,97 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+test_description='test http-push
+
+This test runs various sanity checks on http-push.'
+
+. ./test-lib.sh
+
+HTTPD_PATH=${HTTPD_PATH-'/usr/sbin/apache2'}
+HTTPD_PORT=${HTTPD_PORT-'8111'}
+
+if ! test -x "$HTTPD_PATH"
+then
+        test_expect_success 'skipping http-push tests, no suitable webserver' :
+        test_done
+        exit
+fi
+
+TRASH_PATH=$PWD
+
+start_httpd() {
+	cat > "$TRASH_PATH/httpd.conf" <<EOF
+ServerRoot "$TRASH_PATH"
+PidFile "$TRASH_PATH/httpd.pid"
+DocumentRoot "$TRASH_PATH"
+ErrorLog "$TRASH_PATH/error.log"
+Listen 127.0.0.1:$HTTPD_PORT
+LoadModule dav_module /usr/lib/apache2/modules/mod_dav.so
+LoadModule dav_fs_module /usr/lib/apache2/modules/mod_dav_fs.so
+DAVLockDB DAVLock
+<Location />
+	Dav on
+</Location>
+EOF
+
+	"$HTTPD_PATH" -f "$TRASH_PATH"/httpd.conf -k start
+}
+
+stop_httpd() {
+	"$HTTPD_PATH" -f "$TRASH_PATH"/httpd.conf -k stop
+}
+
+test_expect_success 'setup webserver' '
+	start_httpd
+'
+
+test_expect_success 'setup remote repository' '
+	cd "$TRASH_PATH" &&
+	mkdir test_repo &&
+	cd test_repo &&
+	git init &&
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -m initial &&
+	cd - &&
+	git clone --bare test_repo test_repo.git &&
+	cd test_repo.git &&
+	git --bare update-server-info &&
+	chmod +x hooks/post-update
+'
+	
+test_expect_success 'clone remote repository' '
+	cd "$TRASH_PATH" &&
+	git clone http://127.0.0.1:$HTTPD_PORT/test_repo.git test_repo_clone
+'
+
+test_expect_success 'push to remote repository' '
+	cd "$TRASH_PATH"/test_repo_clone &&
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -m path2 &&
+	git push
+'
+
+test_expect_success 'create and delete remote branch' '
+	cd "$TRASH_PATH"/test_repo_clone &&
+	git checkout -b dev &&
+	: >path3 &&
+	git add path3 &&
+	test_tick &&
+	git commit -m dev &&
+	git push origin dev &&
+	git fetch &&
+	git push origin :dev &&
+	git branch -d -r origin/dev &&
+	git fetch &&
+	! git show-ref --verify refs/remotes/origin/dev
+'
+
+stop_httpd
+
+test_done
-- 
1.5.4.2.156.ge3c5

Re: [PATCH] http-push: add regression tests

[Mike Hommey]

On Sat, Feb 23, 2008 at 10:28:44PM +0100, Clemens Buchacher wrote:
> These tests require a webserver. The executable path and listening port
> are configured using the environment variables
> 
> HTTPD_PATH (default: /usr/sbin/apache2) and
> HTTPD_PORT (default: 8111),
(...)
> +LoadModule dav_module /usr/lib/apache2/modules/mod_dav.so
> +LoadModule dav_fs_module /usr/lib/apache2/modules/mod_dav_fs.so

You'd need something for these paths too.

It would be nice if the httpd setup could be done in another file so
that other tests could use it. Also, an https setup would be nice ;)

Mike

[PATCH] http-push: add regression tests

[Clemens Buchacher]

http-push tests require a web server with WebDAV support.

This commit introduces a HTTPD test library, which can be configured using
the following environment variables.

LIB_HTTPD_PATH	web server path
LIB_HTTPD_PORT	listening port
LIB_HTTPD_DAV	enable DAV
LIB_HTTPD_SVN	enable SVN
LIB_HTTPD_SSL	enable SSL

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sun, Feb 24, 2008 at 09:58:30AM +0100, Mike Hommey wrote:
> > +LoadModule dav_module /usr/lib/apache2/modules/mod_dav.so
> > +LoadModule dav_fs_module /usr/lib/apache2/modules/mod_dav_fs.so
> 
> You'd need something for these paths too.

Indeed. If Apache or the DAV modules are not found, the test is now skipped. If
Apache is older than version 2, the test is skipped as well. I had some
problems with WebDAV "lock tokens" and couldn't get http-push to work. In
principle, however, lib-http.sh also works with Apache version 1.3.

If LIB_HTTPD_PATH points to an executable, but not to an Apache web server, the
test fails with an error.

> It would be nice if the httpd setup could be done in another file so
> that other tests could use it. Also, an https setup would be nice ;)

Good idea.

I also added SVN support. I simply tried to mimic the code from lib-git-svn.sh.
It should be straightforward to integrate lib-httpd.sh into lib-git-svn.sh now.

Regards,
Clemens

 t/lib-httpd.sh       |  181 ++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5540-http-push.sh |   83 +++++++++++++++++++++++
 2 files changed, 264 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-httpd.sh
 create mode 100644 t/t5540-http-push.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
new file mode 100644
index 0000000..7fc090e
--- /dev/null
+++ b/t/lib-httpd.sh
@@ -0,0 +1,181 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+LIB_HTTPD_ROOT_PATH=${LIB_HTTPD_ROOT_PATH-"$PWD"}
+LIB_HTTPD_PATH=${LIB_HTTPD_PATH-'/usr/sbin/apache2'}
+
+if test -x "$LIB_HTTPD_PATH"; then
+
+HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
+	sed -n 's/^Server version: Apache\/\([0-9\.]*\).*$/\1/p'`
+HTTPD_VERSION_MAJOR=$((`echo $HTTPD_VERSION | sed 's/\..*//'`))
+HTTPD_VERSION_MINOR=\
+$((`echo $HTTPD_VERSION | sed 's/^[0-9]*\.//' | sed 's/\..*//'`))
+
+if test -n "$HTTPD_VERSION"
+then
+	if test -z "$LIB_HTTPD_MODULE_PATH"
+	then
+		if test $HTTPD_VERSION_MAJOR -ge 2
+		then
+			LIB_HTTPD_MODULE_PATH='/usr/lib/apache2/modules'
+		else
+			HTTPD_VERSION_SHORT=$HTTPD_VERSION_MAJOR.$HTTPD_VERSION_MINOR
+			LIB_HTTPD_MODULE_PATH="/usr/lib/apache/$HTTPD_VERSION_SHORT"
+		fi
+	fi
+else
+	error "Could not identify web server '$LIB_HTTPD_PATH'"
+fi
+
+fi # test -x "$LIB_HTTPD_PATH"
+
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
+
+if test -z "$LIB_HTTPD_SSL"
+then
+	HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
+else
+	HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT
+fi
+
+conf_init() {
+	test $# -eq 0 ||
+	error "error: not 0 parameters to conf_init"
+
+	: >"$LIB_HTTPD_ROOT_PATH/httpd.conf"
+}
+
+conf_write() {
+	test $# -eq 0 ||
+	error "error: not 0 parameters to conf_write"
+
+	cat >> "$LIB_HTTPD_ROOT_PATH/httpd.conf"
+}
+
+conf_load_module() {
+	test $# -eq 2 ||
+	error "error: not 2 parameters to conf_load_module"
+
+	if ! test -f $LIB_HTTPD_MODULE_PATH/$2
+	then
+		test_expect_success "Could find $1 at $LIB_HTTPD_MODULE_PATH/$2" :
+		test_done
+		exit
+	fi
+
+	conf_write <<EOF
+LoadModule $1 $LIB_HTTPD_MODULE_PATH/$2
+EOF
+}
+
+prepare_httpd() {
+	test $# -eq 0 ||
+	error "error: not 0 parameters to prepare_httpd"
+
+	conf_write <<EOF
+ServerRoot $LIB_HTTPD_ROOT_PATH
+PidFile $LIB_HTTPD_ROOT_PATH/httpd.pid
+DocumentRoot $LIB_HTTPD_ROOT_PATH
+ErrorLog $LIB_HTTPD_ROOT_PATH/error.log
+Listen 127.0.0.1:$LIB_HTTPD_PORT
+EOF
+
+	if test "$LIB_HTTPD_SSL" != ""
+	then
+		cat > $LIB_HTTPD_ROOT_PATH/ssl.cnf <<EOF
+RANDFILE                = $ENV::HOME/.rnd
+
+[ req ]
+default_bits            = 1024
+distinguished_name      = req_distinguished_name
+prompt                  = no
+[ req_distinguished_name ]
+commonName                      = 127.0.0.1
+EOF
+		openssl req \
+			-config $LIB_HTTPD_ROOT_PATH/ssl.cnf \
+			-new -x509 -nodes \
+			-out $LIB_HTTPD_ROOT_PATH/httpd.pem \
+			-keyout $LIB_HTTPD_ROOT_PATH/httpd.pem
+		export GIT_SSL_NO_VERIFY=t
+		conf_load_module ssl_module mod_ssl.so
+		conf_write <<EOF
+SSLCertificateFile $LIB_HTTPD_ROOT_PATH/httpd.pem
+SSLCertificateKeyFile $LIB_HTTPD_ROOT_PATH/httpd.pem
+SSLRandomSeed startup file:/dev/urandom 512
+SSLRandomSeed connect file:/dev/urandom 512
+SSLSessionCache none
+SSLMutex file:$LIB_HTTPD_ROOT_PATH/ssl_mutex
+SSLEngine On
+EOF
+	fi
+		
+	if test "$LIB_HTTPD_DAV" != "" -o "$LIB_HTTPD_SVN" != ""
+	then
+		if test $HTTPD_VERSION_MAJOR -ge 2
+		then
+			conf_load_module dav_module mod_dav.so
+		else
+			conf_load_module dav_module libdav.so
+		fi
+
+		if test $HTTPD_VERSION_MAJOR -ge 2
+		then
+			conf_load_module dav_fs_module mod_dav_fs.so
+		fi
+
+		conf_write <<EOF
+DAVLockDB DAVLock
+<Location />
+	Dav on
+</Location>
+EOF
+
+		if test "$LIB_HTTPD_SVN" != ""
+		then
+			rawsvnrepo="$LIB_HTTPD_ROOT_PATH/svnrepo"
+			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"
+
+			conf_load_module dav_svn_module mod_dav_svn.so
+			conf_write <<EOF
+<Location /svn>
+	DAV svn
+	SVNPath $rawsvnrepo
+</Location>
+EOF
+		fi
+	fi
+}
+
+start_httpd() {
+	test $# -eq 0 ||
+	error "error: not 0 parameters to start_httpd"
+
+	prepare_httpd
+
+	if test $HTTPD_VERSION_MAJOR -ge 2
+	then
+		"$LIB_HTTPD_PATH" -f "$LIB_HTTPD_ROOT_PATH"/httpd.conf -k start
+	else
+		start-stop-daemon --start \
+			--pidfile "$LIB_HTTPD_ROOT_PATH/httpd.pid" \
+			--exec "$LIB_HTTPD_PATH" \
+			-- -f "$LIB_HTTPD_ROOT_PATH"/httpd.conf
+	fi
+}
+
+stop_httpd() {
+	test $# -eq 0 ||
+	error "error: not 0 parameters to stop_httpd"
+
+	if test $HTTPD_VERSION_MAJOR -ge 2
+	then
+		"$LIB_HTTPD_PATH" -f "$LIB_HTTPD_ROOT_PATH"/httpd.conf -k stop
+	else
+		start-stop-daemon --stop \
+			--pidfile "$LIB_HTTPD_ROOT_PATH/httpd.pid"
+	fi
+}
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
new file mode 100644
index 0000000..9706f30
--- /dev/null
+++ b/t/t5540-http-push.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+test_description='test http-push
+
+This test runs various sanity checks on http-push.'
+
+. ./test-lib.sh
+
+ROOT_PATH=$PWD
+LIB_HTTPD_ROOT_PATH=$ROOT_PATH
+LIB_HTTPD_DAV=t
+
+. ../lib-httpd.sh
+
+if ! test -x "$LIB_HTTPD_PATH"
+then
+        test_expect_success "skipping http-push tests, no web server found at $LIB_HTTPD_PATH" :
+        test_done
+        exit
+fi
+
+test_expect_success 'setup web server' '
+	start_httpd
+'
+
+test_expect_success 'setup remote repository' '
+	cd "$ROOT_PATH" &&
+	mkdir test_repo &&
+	cd test_repo &&
+	git init &&
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -m initial &&
+	cd - &&
+	git clone --bare test_repo test_repo.git &&
+	cd test_repo.git &&
+	git --bare update-server-info &&
+	chmod +x hooks/post-update
+'
+	
+test_expect_success 'clone remote repository' '
+	cd "$ROOT_PATH" &&
+	git clone $HTTPD_URL/test_repo.git test_repo_clone
+'
+
+if ! test -n "$HTTPD_VERSION" -a $HTTPD_VERSION_MAJOR -ge 2
+then
+	test_expect_success "skipping remaining http-push tests, at least Apache version 2 is required" :
+        test_done
+        exit
+fi
+
+test_expect_success 'push to remote repository' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -m path2 &&
+	git push
+'
+
+test_expect_success 'create and delete remote branch' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout -b dev &&
+	: >path3 &&
+	git add path3 &&
+	test_tick &&
+	git commit -m dev &&
+	git push origin dev &&
+	git fetch &&
+	git push origin :dev &&
+	git branch -d -r origin/dev &&
+	git fetch &&
+	! git show-ref --verify refs/remotes/origin/dev
+'
+
+stop_httpd
+
+test_done
-- 
1.5.4.2.156.ge3c5

Re: [PATCH] http-push: add regression tests

[Mike Hommey]

On Sun, Feb 24, 2008 at 07:03:40PM +0100, Clemens Buchacher wrote:
> http-push tests require a web server with WebDAV support.
> 
> This commit introduces a HTTPD test library, which can be configured using
> the following environment variables.
> 
> LIB_HTTPD_PATH	web server path
> LIB_HTTPD_PORT	listening port
> LIB_HTTPD_DAV	enable DAV
> LIB_HTTPD_SVN	enable SVN
> LIB_HTTPD_SSL	enable SSL

I'd add LIB_HTTPD_MODULE_PATH in here ;)

I took a quick but deeper look to your script, and I think it would be
better to have a httpd.conf with proper <IfDefine> directives, and
toggle the proper defines on the httpd command line.

Note the ServerRoot is used when paths are relative, so only the
ServerRoot need to be set, and it could be set with the -C argument to
httpd, avoiding the need of a .in file. If necessary, modules path could
be made relative to the ServerRoot by means of a symlink.

Avoiding to have logs, dav lock, ssl mutex, etc. in the document
root would be better, too, but that is nitpicking.

Mike

Re: [PATCH] http-push: add regression tests

[Clemens Buchacher]

Hi,

On Sun, Feb 24, 2008 at 07:48:32PM +0100, Mike Hommey wrote:
> I took a quick but deeper look to your script, and I think it would be
> better to have a httpd.conf with proper <IfDefine> directives, and
> toggle the proper defines on the httpd command line.

I wanted to avoid that because I thought it would make it harder to adapt the
script to different web servers. However, the script turned out to be very
apache specific anyways. I'll have a look at those IfDefine's.

> Note the ServerRoot is used when paths are relative, so only the
> ServerRoot need to be set, and it could be set with the -C argument to
> httpd, avoiding the need of a .in file. If necessary, modules path could
> be made relative to the ServerRoot by means of a symlink.

I see.

> Avoiding to have logs, dav lock, ssl mutex, etc. in the document
> root would be better, too, but that is nitpicking.

True. Maybe we should also make a separate working directory for the clones.

Clemens

[PATCH] http-push: add regression tests

[Clemens Buchacher]

http-push tests require a web server with WebDAV support.

This commit introduces a HTTPD test library, which can be configured using
the following environment variables.

LIB_HTTPD_PATH		web server path
LIB_HTTPD_MODULE_PATH	web server modules path
LIB_HTTPD_PORT		listening port
LIB_HTTPD_DAV		enable DAV
LIB_HTTPD_SVN		enable SVN
LIB_HTTPD_SSL		enable SSL

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sun, Feb 24, 2008 at 07:48:32PM +0100, Mike Hommey wrote:
> I took a quick but deeper look to your script, and I think it would be
> better to have a httpd.conf with proper <IfDefine> directives, and
> toggle the proper defines on the httpd command line.

Alright, here we go.

 t/lib-httpd.sh          |   88 +++++++++++++++++++++++++++++++++++++++++++++++
 t/lib-httpd/apache.conf |   34 ++++++++++++++++++
 t/lib-httpd/ssl.cnf     |    8 ++++
 t/t5540-http-push.sh    |   73 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 203 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-httpd.sh
 create mode 100644 t/lib-httpd/apache.conf
 create mode 100644 t/lib-httpd/ssl.cnf
 create mode 100644 t/t5540-http-push.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
new file mode 100644
index 0000000..2c621c4
--- /dev/null
+++ b/t/lib-httpd.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+LIB_HTTPD_PATH=${LIB_HTTPD_PATH-'/usr/sbin/apache2'}
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
+
+TEST_PATH="$PWD"/../lib-httpd
+HTTPD_ROOT_PATH="$PWD"/httpd
+HTTPD_DOCUMENT_ROOT_PATH="$PWD"/httpd/www
+
+if ! test -x "$LIB_HTTPD_PATH"
+then
+        say_color "" "skipping test, no web server found at $LIB_HTTPD_PATH"
+        test_done
+        exit
+fi
+
+HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
+	sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q'`
+
+if test -n "$HTTPD_VERSION"
+then
+	if test -z "$LIB_HTTPD_MODULE_PATH"
+	then
+		if ! test $HTTPD_VERSION -ge 2
+		then
+			say_color "" "skipping test, at least Apache version 2 is required"
+			test_done
+			exit
+		fi
+
+		LIB_HTTPD_MODULE_PATH='/usr/lib/apache2/modules'
+	fi
+else
+	error "Could not identify web server '$LIB_HTTPD_PATH'"
+fi
+
+HTTPD_PARA="-d $HTTPD_ROOT_PATH -f $TEST_PATH/apache.conf"
+
+prepare_httpd() {
+	mkdir $HTTPD_ROOT_PATH
+	mkdir $HTTPD_DOCUMENT_ROOT_PATH
+
+	ln -s $LIB_HTTPD_MODULE_PATH $HTTPD_ROOT_PATH/modules
+
+	if test -n "$LIB_HTTPD_SSL"
+	then
+		HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT
+	else
+		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
+	fi
+
+	if test "$LIB_HTTPD_SSL" != ""
+	then
+		openssl req \
+			-config $TEST_PATH/ssl.cnf \
+			-new -x509 -nodes \
+			-out $HTTPD_ROOT_PATH/httpd.pem \
+			-keyout $HTTPD_ROOT_PATH/httpd.pem
+		export GIT_SSL_NO_VERIFY=t
+		HTTPD_PARA="$HTTPD_PARA -DSSL"
+	fi
+			
+	if test "$LIB_HTTPD_DAV" != "" -o "$LIB_HTTPD_SVN" != ""
+	then
+		HTTPD_PARA="$HTTPD_PARA -DDAV"
+
+		if test "$LIB_HTTPD_SVN" != ""
+		then
+			HTTPD_PARA="$HTTPD_PARA -DSVN"
+			rawsvnrepo="$HTTPD_ROOT_PATH/svnrepo"
+			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"
+		fi
+	fi
+}
+
+start_httpd() {
+	prepare_httpd
+
+	"$LIB_HTTPD_PATH" $HTTPD_PARA \
+		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start
+}
+
+stop_httpd() {
+	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
+}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
new file mode 100644
index 0000000..a447346
--- /dev/null
+++ b/t/lib-httpd/apache.conf
@@ -0,0 +1,34 @@
+PidFile httpd.pid
+DocumentRoot www
+ErrorLog error.log
+
+<IfDefine SSL>
+LoadModule ssl_module modules/mod_ssl.so
+
+SSLCertificateFile httpd.pem
+SSLCertificateKeyFile httpd.pem
+SSLRandomSeed startup file:/dev/urandom 512
+SSLRandomSeed connect file:/dev/urandom 512
+SSLSessionCache none
+SSLMutex file:ssl_mutex
+SSLEngine On
+</IfDefine>
+
+<IfDefine DAV>
+	LoadModule dav_module modules/mod_dav.so
+	LoadModule dav_fs_module modules/mod_dav_fs.so
+
+	DAVLockDB DAVLock
+	<Location />
+		Dav on
+	</Location>
+</IfDefine>
+
+<IfDefine SVN>
+	LoadModule dav_svn_module modules/mod_dav_svn.so
+
+	<Location /svn>
+		DAV svn
+		SVNPath svnrepo
+	</Location>
+</IfDefine>
diff --git a/t/lib-httpd/ssl.cnf b/t/lib-httpd/ssl.cnf
new file mode 100644
index 0000000..8d89a05
--- /dev/null
+++ b/t/lib-httpd/ssl.cnf
@@ -0,0 +1,8 @@
+RANDFILE                = $ENV::HOME/.rnd
+
+[ req ]
+default_bits            = 1024
+distinguished_name      = req_distinguished_name
+prompt                  = no
+[ req_distinguished_name ]
+commonName              = 127.0.0.1
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
new file mode 100644
index 0000000..c12e2ad
--- /dev/null
+++ b/t/t5540-http-push.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+test_description='test http-push
+
+This test runs various sanity checks on http-push.'
+
+. ./test-lib.sh
+
+ROOT_PATH="$PWD"
+LIB_HTTPD_DAV=t
+
+. ../lib-httpd.sh
+
+if ! start_httpd >&3 2>&4
+then
+	say_color "" "skipping test, web server setup failed"
+	test_done
+	exit
+fi
+
+test_expect_success 'setup remote repository' '
+	cd "$ROOT_PATH" &&
+	mkdir test_repo &&
+	cd test_repo &&
+	git init &&
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -m initial &&
+	cd - &&
+	git clone --bare test_repo test_repo.git &&
+	cd test_repo.git &&
+	git --bare update-server-info &&
+	chmod +x hooks/post-update &&
+	cd - &&
+	mv test_repo.git $HTTPD_DOCUMENT_ROOT_PATH
+'
+	
+test_expect_success 'clone remote repository' '
+	cd "$ROOT_PATH" &&
+	git clone $HTTPD_URL/test_repo.git test_repo_clone
+'
+
+test_expect_success 'push to remote repository' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -m path2 &&
+	git push
+'
+
+test_expect_success 'create and delete remote branch' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout -b dev &&
+	: >path3 &&
+	git add path3 &&
+	test_tick &&
+	git commit -m dev &&
+	git push origin dev &&
+	git fetch &&
+	git push origin :dev &&
+	git branch -d -r origin/dev &&
+	git fetch &&
+	! git show-ref --verify refs/remotes/origin/dev
+'
+
+stop_httpd
+
+test_done
-- 
1.5.4.2.156.ge3c5

Re: [PATCH] http-push: add regression tests

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> http-push tests require a web server with WebDAV support.

I'd rather see these tests that are heavyweight and (more
importantly) open a listened network ports kept strictly
optional.  IOW, please enable them only when the user asks for
them, like some of the heavier GIT_SVN tests already do with
"make full-svn-test" target.

>  create mode 100644 t/lib-httpd.sh
>  create mode 100644 t/lib-httpd/apache.conf
>  create mode 100644 t/lib-httpd/ssl.cnf
>  create mode 100644 t/t5540-http-push.sh

Others are Ok but the last one should be 100755 per convention.

> +TEST_PATH="$PWD"/../lib-httpd
> +HTTPD_ROOT_PATH="$PWD"/httpd
> +HTTPD_DOCUMENT_ROOT_PATH="$PWD"/httpd/www
> +
> +if ! test -x "$LIB_HTTPD_PATH"
> +then
> +        say_color "" "skipping test, no web server found at $LIB_HTTPD_PATH"

Just use "say" to let "say" function pick the color.

> +	if test "$LIB_HTTPD_DAV" != "" -o "$LIB_HTTPD_SVN" != ""
> +	then
> +		HTTPD_PARA="$HTTPD_PARA -DDAV"
> +
> +		if test "$LIB_HTTPD_SVN" != ""
> +		then
> +			HTTPD_PARA="$HTTPD_PARA -DSVN"
> +			rawsvnrepo="$HTTPD_ROOT_PATH/svnrepo"
> +			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"

Subversion???  Ah, you are preparing for enhancement of t91XX series?

> +		fi
> +	fi
> +}
> +
> +start_httpd() {
> +	prepare_httpd
> +
> +	"$LIB_HTTPD_PATH" $HTTPD_PARA \
> +		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start
> +}
> +
> +stop_httpd() {
> +	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
> +}

I have to wonder where it is arranged so that a test failure
(including ^C out of it, or losing SSH connection to the
terminal) would cause the server to be killed off.

[PATCH] http-push: add regression tests

[Clemens Buchacher]

http-push tests require a web server with WebDAV support.

This commit introduces a HTTPD test library, which can be configured using
the following environment variables.

LIB_HTTPD_PATH		web server path
LIB_HTTPD_MODULE_PATH	web server modules path
LIB_HTTPD_PORT		listening port
LIB_HTTPD_DAV		enable DAV
LIB_HTTPD_SVN		enable SVN
LIB_HTTPD_SSL		enable SSL

Networking tests are disabled by default. Use GIT_TEST_OPTS=--network to
enable.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Mon, Feb 25, 2008 at 04:24:30PM -0800, Junio C Hamano wrote:
> I'd rather see these tests that are heavyweight and (more
> importantly) open a listened network ports kept strictly
> optional.

Ok. I added the test option --network, which enables networking tests.

> Subversion???  Ah, you are preparing for enhancement of t91XX series?

Yes.

> I have to wonder where it is arranged so that a test failure
> (including ^C out of it, or losing SSH connection to the
> terminal) would cause the server to be killed off.

I simply didn't know how to do that. I now copied the trap mechanism from
test-lib.sh, which I hope is the right thing to do. I didn't want to override
the test-lib.sh trap in lib-httpd.sh, so I wrapped the trap code in the
function die().

On Tue, Feb 26, 2008 at 08:32:06PM +0100, Mike Hommey wrote:
> > +HTTPD_ROOT_PATH="$PWD"/httpd
> > +HTTPD_DOCUMENT_ROOT_PATH="$PWD"/httpd/www
> 
> Why not just use $HTTPD_ROOT_PATH/www and don't use the
> $HTTPD_DOCUMENT_ROOT_PATH variable ?

I wanted to discourage the test case writer to use HTTPD_ROOT_PATH directly,
because it should not be any of his concern.

> > +	mkdir $HTTPD_ROOT_PATH
> > +	mkdir $HTTPD_DOCUMENT_ROOT_PATH
> 
> This could become mkdir -p $HTTPD_ROOT_PATH/www

I wanted to state explicitly that HTTPD_ROOT_PATH and HTTPD_DOCUMENT_ROOT_PATH
do not have to be related. I added the -p flag to both commands, however.

> > +	if test -n "$LIB_HTTPD_SSL"
> > +	if test "$LIB_HTTPD_SSL" != ""
> 
> Why use different tests?

I picked up the latter one while reading code from other tests, so my coding
style changed while implementing this patch. I'll now stick to test -n and test
-z, if that's ok.

> > +		openssl req \
> > +			-config $TEST_PATH/ssl.cnf \
> > +			-new -x509 -nodes \
> > +			-out $HTTPD_ROOT_PATH/httpd.pem \
> > +			-keyout $HTTPD_ROOT_PATH/httpd.pem
> > +		export GIT_SSL_NO_VERIFY=t
> > +		HTTPD_PARA="$HTTPD_PARA -DSSL"
> 
> For future improvement (that could be another patch), it would probably
> be nice to generate a CAcert and a certificate signed by it, so we can
> also test without GIT_SSL_NO_VERIFY.
> 
> By the way, why not just include the .pem ?

I included the generation of the .pem file mainly for documentation purposes
and in order to allow improvements like the one you are suggesting. The
downside is that it requires openssl, but I think whoever wants to test SSL is
likely to have it installed. Right?

> > +stop_httpd() {
> > +	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
> > +}
> 
> As Junio said, it might be good to add a trap.

Indeed.

> > +RANDFILE                = $ENV::HOME/.rnd
> 
> Wouldn't it be better elsewhere ?

True.

I tried to incorporate all your suggestions. I hope I didn't miss anything.

Regards,
Clemens

 t/lib-httpd.sh          |   97 +++++++++++++++++++++++++++++++++++++++++++++++
 t/lib-httpd/apache.conf |   34 ++++++++++++++++
 t/lib-httpd/ssl.cnf     |    8 ++++
 t/t5540-http-push.sh    |   73 +++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |   16 +++++++-
 5 files changed, 226 insertions(+), 2 deletions(-)
 create mode 100644 t/lib-httpd.sh
 create mode 100644 t/lib-httpd/apache.conf
 create mode 100644 t/lib-httpd/ssl.cnf
 create mode 100755 t/t5540-http-push.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
new file mode 100644
index 0000000..2d01fc9
--- /dev/null
+++ b/t/lib-httpd.sh
@@ -0,0 +1,97 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+if test -z "$GIT_TEST_NET"
+then
+	say "skipping test, network testing disabled by default"
+	say "(define GIT_TEST_OPTS=--net to enable)"
+	test_done
+	exit
+fi
+
+LIB_HTTPD_PATH=${LIB_HTTPD_PATH-'/usr/sbin/apache2'}
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
+
+TEST_PATH="$PWD"/../lib-httpd
+HTTPD_ROOT_PATH="$PWD"/httpd
+HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
+
+if ! test -x "$LIB_HTTPD_PATH"
+then
+        say "skipping test, no web server found at '$LIB_HTTPD_PATH'"
+        test_done
+        exit
+fi
+
+HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
+	sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q'`
+
+if test -n "$HTTPD_VERSION"
+then
+	if test -z "$LIB_HTTPD_MODULE_PATH"
+	then
+		if ! test $HTTPD_VERSION -ge 2
+		then
+			say "skipping test, at least Apache version 2 is required"
+			test_done
+			exit
+		fi
+
+		LIB_HTTPD_MODULE_PATH='/usr/lib/apache2/modules'
+	fi
+else
+	error "Could not identify web server at '$LIB_HTTPD_PATH'"
+fi
+
+HTTPD_PARA="-d $HTTPD_ROOT_PATH -f $TEST_PATH/apache.conf"
+
+prepare_httpd() {
+	mkdir -p $HTTPD_ROOT_PATH
+	mkdir -p $HTTPD_DOCUMENT_ROOT_PATH
+
+	ln -s $LIB_HTTPD_MODULE_PATH $HTTPD_ROOT_PATH/modules
+
+	if test -n "$LIB_HTTPD_SSL"
+	then
+		HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT
+
+		RANDFILE_PATH="$HTTPD_ROOT_PATH"/.rnd openssl req \
+			-config $TEST_PATH/ssl.cnf \
+			-new -x509 -nodes \
+			-out $HTTPD_ROOT_PATH/httpd.pem \
+			-keyout $HTTPD_ROOT_PATH/httpd.pem
+		export GIT_SSL_NO_VERIFY=t
+		HTTPD_PARA="$HTTPD_PARA -DSSL"
+	else
+		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
+	fi
+
+	if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN"
+	then
+		HTTPD_PARA="$HTTPD_PARA -DDAV"
+
+		if test -n "$LIB_HTTPD_SVN"
+		then
+			HTTPD_PARA="$HTTPD_PARA -DSVN"
+			rawsvnrepo="$HTTPD_ROOT_PATH/svnrepo"
+			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"
+		fi
+	fi
+}
+
+start_httpd() {
+	prepare_httpd
+
+	trap 'stop_httpd; die' exit
+
+	"$LIB_HTTPD_PATH" $HTTPD_PARA \
+		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start
+}
+
+stop_httpd() {
+	trap 'die' exit
+
+	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
+}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
new file mode 100644
index 0000000..a447346
--- /dev/null
+++ b/t/lib-httpd/apache.conf
@@ -0,0 +1,34 @@
+PidFile httpd.pid
+DocumentRoot www
+ErrorLog error.log
+
+<IfDefine SSL>
+LoadModule ssl_module modules/mod_ssl.so
+
+SSLCertificateFile httpd.pem
+SSLCertificateKeyFile httpd.pem
+SSLRandomSeed startup file:/dev/urandom 512
+SSLRandomSeed connect file:/dev/urandom 512
+SSLSessionCache none
+SSLMutex file:ssl_mutex
+SSLEngine On
+</IfDefine>
+
+<IfDefine DAV>
+	LoadModule dav_module modules/mod_dav.so
+	LoadModule dav_fs_module modules/mod_dav_fs.so
+
+	DAVLockDB DAVLock
+	<Location />
+		Dav on
+	</Location>
+</IfDefine>
+
+<IfDefine SVN>
+	LoadModule dav_svn_module modules/mod_dav_svn.so
+
+	<Location /svn>
+		DAV svn
+		SVNPath svnrepo
+	</Location>
+</IfDefine>
diff --git a/t/lib-httpd/ssl.cnf b/t/lib-httpd/ssl.cnf
new file mode 100644
index 0000000..6dab257
--- /dev/null
+++ b/t/lib-httpd/ssl.cnf
@@ -0,0 +1,8 @@
+RANDFILE                = $ENV::RANDFILE_PATH
+
+[ req ]
+default_bits            = 1024
+distinguished_name      = req_distinguished_name
+prompt                  = no
+[ req_distinguished_name ]
+commonName              = 127.0.0.1
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
new file mode 100755
index 0000000..7372439
--- /dev/null
+++ b/t/t5540-http-push.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+test_description='test http-push
+
+This test runs various sanity checks on http-push.'
+
+. ./test-lib.sh
+
+ROOT_PATH="$PWD"
+LIB_HTTPD_DAV=t
+
+. ../lib-httpd.sh
+
+if ! start_httpd >&3 2>&4
+then
+	say "skipping test, web server setup failed"
+	test_done
+	exit
+fi
+
+test_expect_success 'setup remote repository' '
+	cd "$ROOT_PATH" &&
+	mkdir test_repo &&
+	cd test_repo &&
+	git init &&
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -m initial &&
+	cd - &&
+	git clone --bare test_repo test_repo.git &&
+	cd test_repo.git &&
+	git --bare update-server-info &&
+	chmod +x hooks/post-update &&
+	cd - &&
+	mv test_repo.git $HTTPD_DOCUMENT_ROOT_PATH
+'
+	
+test_expect_success 'clone remote repository' '
+	cd "$ROOT_PATH" &&
+	git clone $HTTPD_URL/test_repo.git test_repo_clone
+'
+
+test_expect_success 'push to remote repository' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -m path2 &&
+	git push
+'
+
+test_expect_success 'create and delete remote branch' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout -b dev &&
+	: >path3 &&
+	git add path3 &&
+	test_tick &&
+	git commit -m dev &&
+	git push origin dev &&
+	git fetch &&
+	git push origin :dev &&
+	git branch -d -r origin/dev &&
+	git fetch &&
+	! git show-ref --verify refs/remotes/origin/dev
+'
+
+stop_httpd
+
+test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..e33b13b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -79,8 +79,10 @@ do
 		verbose=t; shift ;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		quiet=t; shift ;;
+	-n|--n|--ne|--net|--netw|--netwo|--networ|--network)
+		network=t; shift ;;
 	--no-color)
-	    color=; shift ;;
+		color=; shift ;;
 	--no-python)
 		# noop now...
 		shift ;;
@@ -89,6 +91,11 @@ do
 	esac
 done
 
+if test -n "$network"
+then
+	GIT_TEST_NET=t
+fi
+
 if test -n "$color"; then
 	say_color () {
 		case "$1" in
@@ -142,7 +149,12 @@ test_count=0
 test_fixed=0
 test_broken=0
 
-trap 'echo >&5 "FATAL: Unexpected exit with code $?"; exit 1' exit
+die () {
+	echo >&5 "FATAL: Unexpected exit with code $?"
+	exit 1
+}
+
+trap 'die' exit
 
 test_tick () {
 	if test -z "${test_tick+set}"
-- 
1.5.4.2.156.ge3c5

Re: [PATCH] http-push: add regression tests

[Mike Hommey]

On Wed, Feb 27, 2008 at 09:54:42AM +0100, Clemens Buchacher <drizzd@aon.at> wrote:
> http-push tests require a web server with WebDAV support.
> 
> This commit introduces a HTTPD test library, which can be configured using
> the following environment variables.
> 
> LIB_HTTPD_PATH		web server path
> LIB_HTTPD_MODULE_PATH	web server modules path
> LIB_HTTPD_PORT		listening port
> LIB_HTTPD_DAV		enable DAV
> LIB_HTTPD_SVN		enable SVN
> LIB_HTTPD_SSL		enable SSL
> 
> Networking tests are disabled by default. Use GIT_TEST_OPTS=--network to
> enable.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Acked-by: Mike Hommey <mh@glandium.org>


However, last remark:
> I wanted to state explicitly that HTTPD_ROOT_PATH and HTTPD_DOCUMENT_ROOT_PATH
> do not have to be related. I added the -p flag to both commands, however.

They *are* related, because HTTPD_DOCUMENT_ROOT_PATH is what is in httpd.conf,
set with the DocumentRoot directive, and it's a path relative to
HTTPD_ROOT_PATH. And it is set statically to "www".

Mike

Re: [PATCH] http-push: add regression tests

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Clemens Buchacher wrote:

> Networking tests are disabled by default. Use GIT_TEST_OPTS=--network to
> enable.

One request in addition to Mike's: please make this an environment 
variable such as "GIT_TEST_HTTPD".  IOW, instead of this:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 83889c4..e33b13b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -79,8 +79,10 @@ do
>  		verbose=t; shift ;;
>  	-q|--q|--qu|--qui|--quie|--quiet)
>  		quiet=t; shift ;;
> +	-n|--n|--ne|--net|--netw|--netwo|--networ|--network)
> +		network=t; shift ;;
>  	--no-color)
> -	    color=; shift ;;
> +		color=; shift ;;
>  	--no-python)
>  		# noop now...
>  		shift ;;
> @@ -89,6 +91,11 @@ do
>  	esac
>  done
>  
> +if test -n "$network"
> +then
> +	GIT_TEST_NET=t
> +fi
> +
>  if test -n "$color"; then
>  	say_color () {
>  		case "$1" in

just rename the variable "GIT_TEST_NET" in lib-httpd.sh to 
"GIT_TEST_HTTPD".

Thanks,
Dscho

Re: [PATCH] http-push: add regression tests

[Clemens Buchacher]

Hi,

On Wed, Feb 27, 2008 at 09:51:35AM +0000, Johannes Schindelin wrote:
> just rename the variable "GIT_TEST_NET" in lib-httpd.sh to 
> "GIT_TEST_HTTPD".

My intention was to use this variable as a global "enable network testing"
flag, as implicated by Junio. I don't see a real usecase for enabling only
HTTPD tests, as opposed to other networking tests for git-daemon or git-shell,
for example. But here you go.

On Wed, Feb 27, 2008 at 10:16:42AM +0100, Mike Hommey wrote:
> > I wanted to state explicitly that HTTPD_ROOT_PATH and
> > HTTPD_DOCUMENT_ROOT_PATH do not have to be related. I added the -p flag to
> > both commands, however.
> 
> They *are* related, because HTTPD_DOCUMENT_ROOT_PATH is what is in
> httpd.conf, set with the DocumentRoot directive, and it's a path relative to
> HTTPD_ROOT_PATH. And it is set statically to "www".

This dependency can easily be removed by using a symlink. But I don't feel
strongly enough about this point to resist any longer. :-)

Thank you again for your numerous suggestions. I will resubmit both patches in
reply to this message.

Regards,
Clemens

[PATCH 1/2] http-push: push <remote> :<branch> deletes remote branch

[Clemens Buchacher]

This mirrors current ssh/git push syntax.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 http-push.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index 0beb740..4b31070 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2138,6 +2138,8 @@ static int delete_remote_branch(char *pattern, int force)
 
 	/* Send delete request */
 	fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name);
+	if (dry_run)
+		return 0;
 	url = xmalloc(strlen(remote->url) + strlen(remote_ref->name) + 1);
 	sprintf(url, "%s%s", remote->url, remote_ref->name);
 	slot = get_active_slot();
@@ -2311,6 +2313,16 @@ int main(int argc, char **argv)
 
 		if (!ref->peer_ref)
 			continue;
+
+		if (is_zero_sha1(ref->peer_ref->new_sha1)) {
+			if (delete_remote_branch(ref->name, 1) == -1) {
+				error("Could not remove %s", ref->name);
+				rc = -4;
+			}
+			new_refs++;
+			continue;
+		}
+
 		if (!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (push_verbosely || 1)
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
@@ -2342,11 +2354,6 @@ int main(int argc, char **argv)
 			}
 		}
 		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		if (is_zero_sha1(ref->new_sha1)) {
-			error("cannot happen anymore");
-			rc = -3;
-			continue;
-		}
 		new_refs++;
 		strcpy(old_hex, sha1_to_hex(ref->old_sha1));
 		new_hex = sha1_to_hex(ref->new_sha1);
-- 
1.5.4.2.156.ge3c5

[PATCH 2/2] http-push: add regression tests

[Clemens Buchacher]

http-push tests require a web server with WebDAV support.

This commit introduces a HTTPD test library, which can be configured using
the following environment variables.

GIT_TEST_HTTPD		enable HTTPD tests
LIB_HTTPD_PATH		web server path
LIB_HTTPD_MODULE_PATH	web server modules path
LIB_HTTPD_PORT		listening port
LIB_HTTPD_DAV		enable DAV
LIB_HTTPD_SVN		enable SVN
LIB_HTTPD_SSL		enable SSL

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/lib-httpd.sh          |   96 +++++++++++++++++++++++++++++++++++++++++++++++
 t/lib-httpd/apache.conf |   34 ++++++++++++++++
 t/lib-httpd/ssl.cnf     |    8 ++++
 t/t5540-http-push.sh    |   73 +++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |    9 +++-
 5 files changed, 218 insertions(+), 2 deletions(-)
 create mode 100644 t/lib-httpd.sh
 create mode 100644 t/lib-httpd/apache.conf
 create mode 100644 t/lib-httpd/ssl.cnf
 create mode 100755 t/t5540-http-push.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
new file mode 100644
index 0000000..7f206c5
--- /dev/null
+++ b/t/lib-httpd.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+if test -z "$GIT_TEST_HTTPD"
+then
+	say "skipping test, network testing disabled by default"
+	say "(define GIT_TEST_HTTPD to enable)"
+	test_done
+	exit
+fi
+
+LIB_HTTPD_PATH=${LIB_HTTPD_PATH-'/usr/sbin/apache2'}
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'8111'}
+
+TEST_PATH="$PWD"/../lib-httpd
+HTTPD_ROOT_PATH="$PWD"/httpd
+HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
+
+if ! test -x "$LIB_HTTPD_PATH"
+then
+        say "skipping test, no web server found at '$LIB_HTTPD_PATH'"
+        test_done
+        exit
+fi
+
+HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
+	sed -n 's/^Server version: Apache\/\([0-9]*\)\..*$/\1/p; q'`
+
+if test -n "$HTTPD_VERSION"
+then
+	if test -z "$LIB_HTTPD_MODULE_PATH"
+	then
+		if ! test $HTTPD_VERSION -ge 2
+		then
+			say "skipping test, at least Apache version 2 is required"
+			test_done
+			exit
+		fi
+
+		LIB_HTTPD_MODULE_PATH='/usr/lib/apache2/modules'
+	fi
+else
+	error "Could not identify web server at '$LIB_HTTPD_PATH'"
+fi
+
+HTTPD_PARA="-d $HTTPD_ROOT_PATH -f $TEST_PATH/apache.conf"
+
+prepare_httpd() {
+	mkdir -p $HTTPD_DOCUMENT_ROOT_PATH
+
+	ln -s $LIB_HTTPD_MODULE_PATH $HTTPD_ROOT_PATH/modules
+
+	if test -n "$LIB_HTTPD_SSL"
+	then
+		HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT
+
+		RANDFILE_PATH="$HTTPD_ROOT_PATH"/.rnd openssl req \
+			-config $TEST_PATH/ssl.cnf \
+			-new -x509 -nodes \
+			-out $HTTPD_ROOT_PATH/httpd.pem \
+			-keyout $HTTPD_ROOT_PATH/httpd.pem
+		export GIT_SSL_NO_VERIFY=t
+		HTTPD_PARA="$HTTPD_PARA -DSSL"
+	else
+		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
+	fi
+
+	if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN"
+	then
+		HTTPD_PARA="$HTTPD_PARA -DDAV"
+
+		if test -n "$LIB_HTTPD_SVN"
+		then
+			HTTPD_PARA="$HTTPD_PARA -DSVN"
+			rawsvnrepo="$HTTPD_ROOT_PATH/svnrepo"
+			svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"
+		fi
+	fi
+}
+
+start_httpd() {
+	prepare_httpd
+
+	trap 'stop_httpd; die' exit
+
+	"$LIB_HTTPD_PATH" $HTTPD_PARA \
+		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start
+}
+
+stop_httpd() {
+	trap 'die' exit
+
+	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
+}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
new file mode 100644
index 0000000..a447346
--- /dev/null
+++ b/t/lib-httpd/apache.conf
@@ -0,0 +1,34 @@
+PidFile httpd.pid
+DocumentRoot www
+ErrorLog error.log
+
+<IfDefine SSL>
+LoadModule ssl_module modules/mod_ssl.so
+
+SSLCertificateFile httpd.pem
+SSLCertificateKeyFile httpd.pem
+SSLRandomSeed startup file:/dev/urandom 512
+SSLRandomSeed connect file:/dev/urandom 512
+SSLSessionCache none
+SSLMutex file:ssl_mutex
+SSLEngine On
+</IfDefine>
+
+<IfDefine DAV>
+	LoadModule dav_module modules/mod_dav.so
+	LoadModule dav_fs_module modules/mod_dav_fs.so
+
+	DAVLockDB DAVLock
+	<Location />
+		Dav on
+	</Location>
+</IfDefine>
+
+<IfDefine SVN>
+	LoadModule dav_svn_module modules/mod_dav_svn.so
+
+	<Location /svn>
+		DAV svn
+		SVNPath svnrepo
+	</Location>
+</IfDefine>
diff --git a/t/lib-httpd/ssl.cnf b/t/lib-httpd/ssl.cnf
new file mode 100644
index 0000000..6dab257
--- /dev/null
+++ b/t/lib-httpd/ssl.cnf
@@ -0,0 +1,8 @@
+RANDFILE                = $ENV::RANDFILE_PATH
+
+[ req ]
+default_bits            = 1024
+distinguished_name      = req_distinguished_name
+prompt                  = no
+[ req_distinguished_name ]
+commonName              = 127.0.0.1
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
new file mode 100755
index 0000000..7372439
--- /dev/null
+++ b/t/t5540-http-push.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
+#
+
+test_description='test http-push
+
+This test runs various sanity checks on http-push.'
+
+. ./test-lib.sh
+
+ROOT_PATH="$PWD"
+LIB_HTTPD_DAV=t
+
+. ../lib-httpd.sh
+
+if ! start_httpd >&3 2>&4
+then
+	say "skipping test, web server setup failed"
+	test_done
+	exit
+fi
+
+test_expect_success 'setup remote repository' '
+	cd "$ROOT_PATH" &&
+	mkdir test_repo &&
+	cd test_repo &&
+	git init &&
+	: >path1 &&
+	git add path1 &&
+	test_tick &&
+	git commit -m initial &&
+	cd - &&
+	git clone --bare test_repo test_repo.git &&
+	cd test_repo.git &&
+	git --bare update-server-info &&
+	chmod +x hooks/post-update &&
+	cd - &&
+	mv test_repo.git $HTTPD_DOCUMENT_ROOT_PATH
+'
+	
+test_expect_success 'clone remote repository' '
+	cd "$ROOT_PATH" &&
+	git clone $HTTPD_URL/test_repo.git test_repo_clone
+'
+
+test_expect_success 'push to remote repository' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	: >path2 &&
+	git add path2 &&
+	test_tick &&
+	git commit -m path2 &&
+	git push
+'
+
+test_expect_success 'create and delete remote branch' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout -b dev &&
+	: >path3 &&
+	git add path3 &&
+	test_tick &&
+	git commit -m dev &&
+	git push origin dev &&
+	git fetch &&
+	git push origin :dev &&
+	git branch -d -r origin/dev &&
+	git fetch &&
+	! git show-ref --verify refs/remotes/origin/dev
+'
+
+stop_httpd
+
+test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..9d9cb8d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -80,7 +80,7 @@ do
 	-q|--q|--qu|--qui|--quie|--quiet)
 		quiet=t; shift ;;
 	--no-color)
-	    color=; shift ;;
+		color=; shift ;;
 	--no-python)
 		# noop now...
 		shift ;;
@@ -142,7 +142,12 @@ test_count=0
 test_fixed=0
 test_broken=0
 
-trap 'echo >&5 "FATAL: Unexpected exit with code $?"; exit 1' exit
+die () {
+	echo >&5 "FATAL: Unexpected exit with code $?"
+	exit 1
+}
+
+trap 'die' exit
 
 test_tick () {
 	if test -z "${test_tick+set}"
-- 
1.5.4.2.156.ge3c5

Re: [PATCH] http-push: add regression tests

[Johannes Schindelin]

Hi,

On Wed, 27 Feb 2008, Clemens Buchacher wrote:

> Thank you again for your numerous suggestions. I will resubmit both 
> patches in reply to this message.

It is not strictly my turf, so I cannot ACK it, but here goes my

Liked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Ciao,
Dscho

Re: [PATCH] http-push: add regression tests

[Mike Hommey]

On Tue, Feb 26, 2008 at 12:28:20AM +0100, Clemens Buchacher wrote:
> http-push tests require a web server with WebDAV support.
> 
> This commit introduces a HTTPD test library, which can be configured using
> the following environment variables.
> 
> LIB_HTTPD_PATH		web server path
> LIB_HTTPD_MODULE_PATH	web server modules path
> LIB_HTTPD_PORT		listening port
> LIB_HTTPD_DAV		enable DAV
> LIB_HTTPD_SVN		enable SVN
> LIB_HTTPD_SSL		enable SSL

Overall, this looks fine to me, though I haven't taken a deep look at
what the test itself does. I was more interested in the "library".

Just a few nitpicks.

> +HTTPD_ROOT_PATH="$PWD"/httpd
> +HTTPD_DOCUMENT_ROOT_PATH="$PWD"/httpd/www

Why not just use $HTTPD_ROOT_PATH/www and don't use the
$HTTPD_DOCUMENT_ROOT_PATH variable ?

> +	mkdir $HTTPD_ROOT_PATH
> +	mkdir $HTTPD_DOCUMENT_ROOT_PATH

This could become mkdir -p $HTTPD_ROOT_PATH/www

> +	if test -n "$LIB_HTTPD_SSL"
> +	if test "$LIB_HTTPD_SSL" != ""

Why use different tests?

> +		openssl req \
> +			-config $TEST_PATH/ssl.cnf \
> +			-new -x509 -nodes \
> +			-out $HTTPD_ROOT_PATH/httpd.pem \
> +			-keyout $HTTPD_ROOT_PATH/httpd.pem
> +		export GIT_SSL_NO_VERIFY=t
> +		HTTPD_PARA="$HTTPD_PARA -DSSL"

For future improvement (that could be another patch), it would probably
be nice to generate a CAcert and a certificate signed by it, so we can
also test without GIT_SSL_NO_VERIFY.

By the way, why not just include the .pem ?

> +stop_httpd() {
> +	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
> +}

As Junio said, it might be good to add a trap.

> +RANDFILE                = $ENV::HOME/.rnd

Wouldn't it be better elsewhere ?

Mike