Skip to contentSkip to conversation

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

PATCH In review Clemens Buchacher 4 participants 27 messages Jun 15, 2016

lore
Share & tools
lore

Clemens Buchacher

author
#0 lore raw

PATCH 1/1 http-push: Fix error message for push <remote> :<branch>

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 <[email protected]>

---

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(-)
http-push.c +9 -0
index f9b77d6..8de091f 100644
@@ -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);
Signature · 1 line
1.5.4.2.183.g1ae81
#1 lore raw

Hi,

Quoted 11 lines · 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 <[email protected]>

---

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.

Quoted 7 lines
http-push.c +0 -0
index f9b77d6..8de091f 100644
@@ -2311,6 +2311,15 @@ int main(int argc, char **argv)
 
 		if (!ref->peer_ref)

http-push.c int main(int argc, char **argv) · line 2322

Quoted 10 lines
http-push.c +9 -0
 			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

author
#2 lore raw

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

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

Quoted 1 line · Johannes Schindelin

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.

Quoted 12 lines

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(-)
http-push.c +13 -5
index f9b77d6..e98c52f 100644
@@ -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);
Signature · 1 line
1.5.4.2.183.g69d3
author
#3 lore raw

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

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(-)
http-push.c +12 -5
index f9b77d6..2c928fc 100644
@@ -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);
Signature · 1 line
1.5.4.2.183.g69d3
#4 lore raw

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

Hi,

Quoted 4 lines · Clemens Buchacher

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

#5 lore raw

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

In reply to Clemens Buchacher

Hi,

http-push.c static int delete_remote_branch(char *pattern, int force) · line 2142

Quoted 10 lines · Clemens Buchacher
http-push.c +2 -0
index f9b77d6..e98c52f 100644
@@ -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

author
#6 lore raw

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

Hi,

Quoted 2 lines · Johannes Schindelin

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

#7 lore raw

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

Hi,

Quoted 8 lines · Clemens Buchacher
Quoted 2 lines · Johannes Schindelin

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

Quoted 1 line

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

Thanks for the clarification.

Ciao,
Dscho

#8 lore raw

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

Quoted 13 lines · Johannes Schindelin

Hi,

Quoted 8 lines · Clemens Buchacher
Quoted 2 lines · Johannes Schindelin

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?

Quoted 8 lines

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

#9 lore raw

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

Hi,

Quoted 18 lines · Mike Hommey
Quoted 16 lines · Johannes Schindelin
Quoted 8 lines · Clemens Buchacher
Quoted 2 lines · Johannes Schindelin

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.

Quoted 8 lines
Quoted 3 lines

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

#10 lore raw

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

Quoted 3 lines · Johannes Schindelin

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.

#11 lore raw

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

Hi,

Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Johannes Schindelin

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:

Quoted 7 lines

- 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

author
#12 lore raw

PATCH 1/1 http-push: add regression tests

In reply to Johannes Schindelin

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 <[email protected]>
---
 t/t5540-http-push.sh |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 t/t5540-http-push.sh
t/t5540-http-push.sh new file +97 -0
new file mode 100644
index 0000000..60210ff
@@ -0,0 +1,97 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <[email protected]>
+#
+
+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
Signature · 1 line
1.5.4.2.156.ge3c5
#13 lore raw

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

Quoted 5 lines · 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),

(...)

t/t5540-http-push.sh line 32

Quoted 2 lines
t/t5540-http-push.sh new file +2 -0
+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

author
#14 lore raw

PATCH 1/1 http-push: add regression tests

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 <[email protected]>

---

Quoted 4 lines · Mike Hommey

Review

Quoted 2 lines
(quoted diff) +2 -0
+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.

Quoted 2 lines

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
t/lib-httpd.sh new file +181 -0
new file mode 100644
index 0000000..7fc090e
@@ -0,0 +1,181 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <[email protected]>
+#
+
+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
+}
t/t5540-http-push.sh new file +83 -0

83 lines not on this page · raw message

Signature · 1 line
1.5.4.2.156.ge3c5
#15 lore raw

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

Quoted 10 lines · 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

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

author
#16 lore raw

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

Hi,

Quoted 3 lines · Mike Hommey

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.

Quoted 4 lines

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.

Quoted 2 lines

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

author
#17 lore raw

PATCH 1/1 http-push: add regression tests

In reply to Mike Hommey

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 <[email protected]>

---

Quoted 3 lines · Mike Hommey

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
t/lib-httpd.sh new file +88 -0
new file mode 100644
index 0000000..2c621c4
@@ -0,0 +1,88 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <[email protected]>
+#
+
+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
+}
t/lib-httpd/apache.conf new file +34 -0
new file mode 100644
index 0000000..a447346
@@ -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>
t/lib-httpd/ssl.cnf new file +8 -0
new file mode 100644
index 0000000..8d89a05
@@ -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
t/t5540-http-push.sh new file +73 -0

73 lines not on this page · raw message

Signature · 1 line
1.5.4.2.156.ge3c5
#18 lore raw

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

Quoted 1 line · Clemens Buchacher

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.

Quoted 4 lines
 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.

t/lib-httpd.sh line 15

Quoted 7 lines
t/lib-httpd.sh new file +7 -0
+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.

t/lib-httpd.sh line 74

Quoted 9 lines
t/lib-httpd.sh new file +9 -0
+	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?

Quoted 4 lines
t/lib-httpd.sh new file +4 -0
+		fi
+	fi
+}
+

t/lib-httpd.sh line 88

Quoted 10 lines
t/lib-httpd.sh new file +10 -0
+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.

author
#19 lore raw

PATCH 1/1 http-push: add regression tests

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 <[email protected]>

---

Quoted 3 lines · Junio C Hamano

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.

Quoted 1 line

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

Yes.

Quoted 3 lines

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().

Quoted 5 lines · Mike Hommey

Review

Quoted 2 lines
(quoted diff) +2 -0
+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.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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.

Quoted 12 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+		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.

Quoted 1 line

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?

Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+stop_httpd() {
+	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
+}

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

Indeed.

Quoted 3 lines
Quoted 1 line

+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
t/lib-httpd.sh new file +97 -0
new file mode 100644
index 0000000..2d01fc9
@@ -0,0 +1,97 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <[email protected]>
+#
+
+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
+}
t/lib-httpd/apache.conf new file +34 -0
new file mode 100644
index 0000000..a447346
@@ -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>
t/lib-httpd/ssl.cnf new file +8 -0
new file mode 100644
index 0000000..6dab257
@@ -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
t/t5540-http-push.sh new file +73 -0

73 lines not on this page · raw message

t/test-lib.sh +14 -2
index 83889c4..e33b13b 100644
@@ -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}"
Signature · 1 line
1.5.4.2.156.ge3c5
Acked-by on v1 1/1
#20 lore raw

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

Quoted 12 lines · 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

Quoted 4 lines

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

Signed-off-by Clemens Buchacher <[email protected]>
Acked-by Mike Hommey <[email protected]>

However, last remark:

Quoted 2 lines

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

#21 lore raw

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

In reply to Clemens Buchacher

Hi,

Quoted 2 lines · Clemens Buchacher

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:

Quoted 18 lines
t/test-lib.sh +3 -1
index 83889c4..e33b13b 100644
@@ -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

t/test-lib.sh do · line 101

Quoted 10 lines
t/test-lib.sh +5 -0
 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

author
#22 lore raw

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

Hi,

Quoted 2 lines · Johannes Schindelin

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.

Quoted 7 lines · Mike Hommey
Quoted 3 lines

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

author
#23 lore raw

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

This mirrors current ssh/git push syntax.

Signed-off-by Clemens Buchacher <[email protected]>
---
 http-push.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)
http-push.c +12 -5
index 0beb740..4b31070 100644
@@ -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);
Signature · 1 line
1.5.4.2.156.ge3c5
author
#24 lore raw

PATCH 2/2 http-push: add regression tests

In reply to 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 <[email protected]>
---
 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
t/lib-httpd.sh new file +96 -0
new file mode 100644
index 0000000..7f206c5
@@ -0,0 +1,96 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Clemens Buchacher <[email protected]>
+#
+
+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
+}
t/lib-httpd/apache.conf new file +34 -0
new file mode 100644
index 0000000..a447346
@@ -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>
t/lib-httpd/ssl.cnf new file +8 -0
new file mode 100644
index 0000000..6dab257
@@ -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
t/t5540-http-push.sh new file +73 -0

73 lines not on this page · raw message

t/test-lib.sh +7 -2
index 83889c4..9d9cb8d 100644
@@ -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}"
Signature · 1 line
1.5.4.2.156.ge3c5
#26 lore raw

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

In reply to Clemens Buchacher
Quoted 11 lines · 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

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.

t/lib-httpd.sh line 11

Quoted 2 lines
t/lib-httpd.sh new file +2 -0
+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 ?

t/lib-httpd.sh line 44

Quoted 2 lines
t/lib-httpd.sh new file +2 -0
+	mkdir $HTTPD_ROOT_PATH
+	mkdir $HTTPD_DOCUMENT_ROOT_PATH

This could become mkdir -p $HTTPD_ROOT_PATH/www

t/lib-httpd.sh line 55

Quoted 2 lines
t/lib-httpd.sh new file +2 -0
+	if test -n "$LIB_HTTPD_SSL"
+	if test "$LIB_HTTPD_SSL" != ""

Why use different tests?

t/lib-httpd.sh line 63

Quoted 7 lines
t/lib-httpd.sh new file +7 -0
+		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 ?

t/lib-httpd.sh line 88

Quoted 3 lines
t/lib-httpd.sh new file +3 -0
+stop_httpd() {
+	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
+}

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

Quoted 1 line

+RANDFILE = $ENV::HOME/.rnd

Wouldn't it be better elsewhere ?

Mike