[PATCH 1/3] git-instaweb: Fix custom apache log placement

[PATCH 1/3] git-instaweb: Fix custom apache log placement

[Dan McGee]

'CustomLog' is provided by mod_log_config so we need to include the module
in our generated config. This was added in d94775e1f9a.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 git-instaweb.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 6635fbe..a01476a 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -314,7 +314,7 @@ PidFile "$fqgitdir/pid"
 Listen $bind$port
 EOF
 
-	for mod in mime dir; do
+	for mod in mime dir log_config; do
 		if test -e $module_path/mod_${mod}.so; then
 			echo "LoadModule ${mod}_module " \
 			     "$module_path/mod_${mod}.so" >> "$conf"
-- 
1.7.1

[PATCH 2/3] git-instaweb: Fix Apache environment variable passing

[Dan McGee]

We were passing the non-existent GIT_EXEC_DIR through instead of the real
GIT_EXEC_PATH. In addition, these weren't being passed at all for CGI (non
mod_perl) execution so get them included there as well.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 git-instaweb.sh |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index a01476a..1d349b8 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -314,7 +314,7 @@ PidFile "$fqgitdir/pid"
 Listen $bind$port
 EOF
 
-	for mod in mime dir log_config; do
+	for mod in mime dir env log_config; do
 		if test -e $module_path/mod_${mod}.so; then
 			echo "LoadModule ${mod}_module " \
 			     "$module_path/mod_${mod}.so" >> "$conf"
@@ -334,7 +334,7 @@ EOF
 		cat >> "$conf" <<EOF
 LoadModule perl_module $module_path/mod_perl.so
 PerlPassEnv GIT_DIR
-PerlPassEnv GIT_EXEC_DIR
+PerlPassEnv GIT_EXEC_PATH
 PerlPassEnv GITWEB_CONFIG
 <Location /gitweb.cgi>
 	SetHandler perl-script
@@ -364,6 +364,9 @@ EOF
 			echo "ScriptSock logs/gitweb.sock" >> "$conf"
 		fi
 		cat >> "$conf" <<EOF
+PassEnv GIT_DIR
+PassEnv GIT_EXEC_PATH
+PassEnv GITWEB_CONFIG
 AddHandler cgi-script .cgi
 <Location /gitweb.cgi>
 	Options +ExecCGI
-- 
1.7.1

[PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Dan McGee]

On Arch Linux, we keep the original 'httpd' name for the exectuable and
don't rename it or the path to the modules. Remove some of these assumptions
and add the httpd name in where it may be required.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 git-instaweb.sh |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 1d349b8..d0091f2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -43,7 +43,8 @@ test -z "$port" && port=1234
 
 resolve_full_httpd () {
 	case "$httpd" in
-	*apache2*|*lighttpd*)
+	*apache2*|*lighttpd*|*httpd*)
+		# yes, *httpd* covers *lighttpd* above, but it is there for clarity
 		# ensure that the apache2/lighttpd command ends with "-f"
 		if ! echo "$httpd" | sane_grep -- '-f *$' >/dev/null 2>&1
 		then
@@ -300,7 +301,10 @@ EOF
 }
 
 apache2_conf () {
-	test -z "$module_path" && module_path=/usr/lib/apache2/modules
+	if test -z "$module_path"; then
+		test -d "/usr/lib/httpd/modules" && module_path="/usr/lib/httpd/modules"
+		test -d "/usr/lib/apache2/modules" && module_path="/usr/lib/apache2/modules"
+	fi
 	bind=
 	test x"$local" = xtrue && bind='127.0.0.1:'
 	echo 'text/css css' > "$fqgitdir/mime.types"
@@ -563,7 +567,7 @@ case "$httpd" in
 *lighttpd*)
 	lighttpd_conf
 	;;
-*apache2*)
+*apache2*|*httpd*)
 	apache2_conf
 	;;
 webrick)
-- 
1.7.1

Re: [PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Eric Wong]

Dan McGee <dpmcgee@gmail.com> wrote:
> On Arch Linux, we keep the original 'httpd' name for the exectuable and
> don't rename it or the path to the modules. Remove some of these assumptions
> and add the httpd name in where it may be required.
> 
> Signed-off-by: Dan McGee <dpmcgee@gmail.com>

Thanks Dan,

Everything in this series looks reasonable to me (haven't tested):

Acked-by: Eric Wong <normalperson@yhbt.net>

> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
>  apache2_conf () {
> -	test -z "$module_path" && module_path=/usr/lib/apache2/modules
> +	if test -z "$module_path"; then
> +		test -d "/usr/lib/httpd/modules" && module_path="/usr/lib/httpd/modules"
> +		test -d "/usr/lib/apache2/modules" && module_path="/usr/lib/apache2/modules"
> +	fi

Minor style nitpicks (feel free to ignore unless somebody else feels
more strongly):

The preferred style in git is to have the "then" statement on a separate
line, omitting the semi-colon, but there remain plenty of examples
contrary in our code:

Likewise with "for i in x y z; do"

We also prefer to keep lines shorter than 80 characters (hard tabs being
8 chars), but we also have plenty of examples contrary to that in
instaweb, too :x

-- 
Eric Wong

[PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Dan McGee]

On Arch Linux, we keep the original 'httpd' name for the exectuable and
don't rename it or the path to the modules. Remove some of these assumptions
and add the httpd name in where it may be required. Finally, make some
slight style adjustments to the code we are touching to make it fit the
style of the rest of the script.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---

Style cleanups as suggested by Eric, at least to the parts of the code I
touched.

-Dan

 git-instaweb.sh |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 1d349b8..b7342e2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -43,7 +43,8 @@ test -z "$port" && port=1234
 
 resolve_full_httpd () {
 	case "$httpd" in
-	*apache2*|*lighttpd*)
+	*apache2*|*lighttpd*|*httpd*)
+		# yes, *httpd* covers *lighttpd* above, but it is there for clarity
 		# ensure that the apache2/lighttpd command ends with "-f"
 		if ! echo "$httpd" | sane_grep -- '-f *$' >/dev/null 2>&1
 		then
@@ -300,7 +301,13 @@ EOF
 }
 
 apache2_conf () {
-	test -z "$module_path" && module_path=/usr/lib/apache2/modules
+	if test -z "$module_path"
+	then
+		test -d "/usr/lib/httpd/modules" &&
+			module_path="/usr/lib/httpd/modules"
+		test -d "/usr/lib/apache2/modules" &&
+			module_path="/usr/lib/apache2/modules"
+	fi
 	bind=
 	test x"$local" = xtrue && bind='127.0.0.1:'
 	echo 'text/css css' > "$fqgitdir/mime.types"
@@ -314,8 +321,10 @@ PidFile "$fqgitdir/pid"
 Listen $bind$port
 EOF
 
-	for mod in mime dir env log_config; do
-		if test -e $module_path/mod_${mod}.so; then
+	for mod in mime dir env log_config
+	do
+		if test -e $module_path/mod_${mod}.so
+		then
 			echo "LoadModule ${mod}_module " \
 			     "$module_path/mod_${mod}.so" >> "$conf"
 		fi
@@ -563,7 +572,7 @@ case "$httpd" in
 *lighttpd*)
 	lighttpd_conf
 	;;
-*apache2*)
+*apache2*|*httpd*)
 	apache2_conf
 	;;
 webrick)
-- 
1.7.1

Re: [PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Junio C Hamano]

Dan McGee <dpmcgee@gmail.com> writes:

> On Arch Linux, we keep the original 'httpd' name for the exectuable and
> don't rename it or the path to the modules.

Sorry, but I cannot parse the last 6 words here.

> Remove some of these assumptions
> and add the httpd name in where it may be required.
> ...
>  resolve_full_httpd () {
>  	case "$httpd" in
> -	*apache2*|*lighttpd*)
> +	*apache2*|*lighttpd*|*httpd*)
> +		# yes, *httpd* covers *lighttpd* above, but it is there for clarity

That may make things work better on Arch but I wonder if there are
platforms that allow users to point at their favorite, non-Apache, http
server with a canonical name "httpd" that gets broken by this change.  Am
I being too paranoid?

Other two patches looked very sane.  Thanks.

Re: [PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Dan McGee]

On Wed, Jun 30, 2010 at 11:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> On Arch Linux, we keep the original 'httpd' name for the exectuable and
>> don't rename it or the path to the modules.
>
> Sorry, but I cannot parse the last 6 words here.

On Arch Linux, the executable for the Apache HTTP server keeps the
'httpd' name and is not named 'apache2'. The path to the server
modules also contains 'httpd' rather than 'apache2'.

>> Remove some of these assumptions
>> and add the httpd name in where it may be required.
>> ...
>>  resolve_full_httpd () {
>>       case "$httpd" in
>> -     *apache2*|*lighttpd*)
>> +     *apache2*|*lighttpd*|*httpd*)
>> +             # yes, *httpd* covers *lighttpd* above, but it is there for clarity
>
> That may make things work better on Arch but I wonder if there are
> platforms that allow users to point at their favorite, non-Apache, http
> server with a canonical name "httpd" that gets broken by this change.  Am
> I being too paranoid?
I think you are being overcautious. Even in this case, you can specify
the real name of the web server through git-config or command line
parameters to make sure it picks up the right settings. In addition,
any web server named 'httpd' wouldn't have worked before anyway so we
aren't regressing here. Finally, I've not encountered any platform
that does this kind of canonical naming as web servers have very
different command line flags and interfaces anyway.

> Other two patches looked very sane.  Thanks.
No problem.

-Dan

Re: [PATCH 3/3] git-instaweb: Don't assume Apache executable is named apache2

[Eric Wong]

Dan McGee <dpmcgee@gmail.com> wrote:
> On Wed, Jun 30, 2010 at 11:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Dan McGee <dpmcgee@gmail.com> writes:
> >
> >> On Arch Linux, we keep the original 'httpd' name for the exectuable and
> >> don't rename it or the path to the modules.
> >
> > Sorry, but I cannot parse the last 6 words here.
> 
> On Arch Linux, the executable for the Apache HTTP server keeps the
> 'httpd' name and is not named 'apache2'. The path to the server
> modules also contains 'httpd' rather than 'apache2'.

(I missed this message the first time around since I wasn't Cc-ed).

Thanks Dan,

I've pushed your series of patches up to my "instaweb" branch on
git://git.bogomips.org/git-svn and reworked the commit message of 3/3
below:

>From 4bdf85995b5d21fdb085a480d529c8da0314189e Mon Sep 17 00:00:00 2001
From: Dan McGee <dpmcgee@gmail.com>
Date: Wed, 30 Jun 2010 07:29:08 -0500
Subject: [PATCH] git-instaweb: Don't assume Apache executable is named apache2

On Arch Linux, the executable for the Apache HTTP server keeps
the 'httpd' name and is not named 'apache2'. The path to the
server modules also contains 'httpd' rather than 'apache2'.
Remove some of these assumptions and add the httpd name in where
it may be required. Finally, make some slight style adjustments
to the code we are touching to make it fit the style of the rest
of the script.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
Acked-by: Eric Wong <normalperson@yhbt.net>
---
 git-instaweb.sh |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 1d349b8..b7342e2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -43,7 +43,8 @@ test -z "$port" && port=1234
 
 resolve_full_httpd () {
 	case "$httpd" in
-	*apache2*|*lighttpd*)
+	*apache2*|*lighttpd*|*httpd*)
+		# yes, *httpd* covers *lighttpd* above, but it is there for clarity
 		# ensure that the apache2/lighttpd command ends with "-f"
 		if ! echo "$httpd" | sane_grep -- '-f *$' >/dev/null 2>&1
 		then
@@ -300,7 +301,13 @@ EOF
 }
 
 apache2_conf () {
-	test -z "$module_path" && module_path=/usr/lib/apache2/modules
+	if test -z "$module_path"
+	then
+		test -d "/usr/lib/httpd/modules" &&
+			module_path="/usr/lib/httpd/modules"
+		test -d "/usr/lib/apache2/modules" &&
+			module_path="/usr/lib/apache2/modules"
+	fi
 	bind=
 	test x"$local" = xtrue && bind='127.0.0.1:'
 	echo 'text/css css' > "$fqgitdir/mime.types"
@@ -314,8 +321,10 @@ PidFile "$fqgitdir/pid"
 Listen $bind$port
 EOF
 
-	for mod in mime dir env log_config; do
-		if test -e $module_path/mod_${mod}.so; then
+	for mod in mime dir env log_config
+	do
+		if test -e $module_path/mod_${mod}.so
+		then
 			echo "LoadModule ${mod}_module " \
 			     "$module_path/mod_${mod}.so" >> "$conf"
 		fi
@@ -563,7 +572,7 @@ case "$httpd" in
 *lighttpd*)
 	lighttpd_conf
 	;;
-*apache2*)
+*apache2*|*httpd*)
 	apache2_conf
 	;;
 webrick)
-- 
Eric Wong

Re: [PATCH 1/3] git-instaweb: Fix custom apache log placement

[Dan McGee]

On Tue, Jun 29, 2010 at 9:44 PM, Dan McGee <dpmcgee@gmail.com> wrote:
> 'CustomLog' is provided by mod_log_config so we need to include the module
> in our generated config. This was added in d94775e1f9a.
>
> Signed-off-by: Dan McGee <dpmcgee@gmail.com>

I noticed this set of patches didn't go anywhere...at least the first
two were more bug fixes than improvements so I would have expected
them to get into 1.7.2. Did they get lost in the shuffle?

1: http://marc.info/?l=git&m=127786592330110&w=2
2: http://marc.info/?l=git&m=127786592030105&w=2
3: http://marc.info/?l=git&m=127791591706076&w=2

-Dan

> ---
>  git-instaweb.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 6635fbe..a01476a 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -314,7 +314,7 @@ PidFile "$fqgitdir/pid"
>  Listen $bind$port
>  EOF
>
> -       for mod in mime dir; do
> +       for mod in mime dir log_config; do
>                if test -e $module_path/mod_${mod}.so; then
>                        echo "LoadModule ${mod}_module " \
>                             "$module_path/mod_${mod}.so" >> "$conf"
> --
> 1.7.1
>
>

Re: [PATCH 1/3] git-instaweb: Fix custom apache log placement

[Eric Wong]

Dan McGee <dpmcgee@gmail.com> wrote:
> On Tue, Jun 29, 2010 at 9:44 PM, Dan McGee <dpmcgee@gmail.com> wrote:
> > 'CustomLog' is provided by mod_log_config so we need to include the module
> > in our generated config. This was added in d94775e1f9a.
> >
> > Signed-off-by: Dan McGee <dpmcgee@gmail.com>
> 
> I noticed this set of patches didn't go anywhere...at least the first
> two were more bug fixes than improvements so I would have expected
> them to get into 1.7.2. Did they get lost in the shuffle?
> 
> 1: http://marc.info/?l=git&m=127786592330110&w=2
> 2: http://marc.info/?l=git&m=127786592030105&w=2
> 3: http://marc.info/?l=git&m=127791591706076&w=2

Hi Dan, can you address Junio's concerns here?

  http://marc.info/?l=git&m=127791382102449&w=2

Thanks.

-- 
Eric Wong

Re: [PATCH 1/3] git-instaweb: Fix custom apache log placement

[Dan McGee]

On Thu, Jul 22, 2010 at 6:42 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Dan McGee <dpmcgee@gmail.com> wrote:
>> On Tue, Jun 29, 2010 at 9:44 PM, Dan McGee <dpmcgee@gmail.com> wrote:
>> > 'CustomLog' is provided by mod_log_config so we need to include the module
>> > in our generated config. This was added in d94775e1f9a.
>> >
>> > Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>>
>> I noticed this set of patches didn't go anywhere...at least the first
>> two were more bug fixes than improvements so I would have expected
>> them to get into 1.7.2. Did they get lost in the shuffle?
>>
>> 1: http://marc.info/?l=git&m=127786592330110&w=2
>> 2: http://marc.info/?l=git&m=127786592030105&w=2
>> 3: http://marc.info/?l=git&m=127791591706076&w=2
>
> Hi Dan, can you address Junio's concerns here?
>
>  http://marc.info/?l=git&m=127791382102449&w=2

I did already, and got no feedback...
http://marc.info/?l=git&m=127791591706076&w=2

-Dan

Re: [PATCH 1/3] git-instaweb: Fix custom apache log placement

[Junio C Hamano]

Dan McGee <dpmcgee@gmail.com> writes:

> On Thu, Jul 22, 2010 at 6:42 PM, Eric Wong <normalperson@yhbt.net> wrote:
> ...
>> Hi Dan, can you address Junio's concerns here?
>>
>> http://marc.info/?l=git&m=127791382102449&w=2
>
> I did already, and got no feedback...
> http://marc.info/?l=git&m=127791591706076&w=2

Thanks for a (half-) pointer [*1*]; I didn't see that message til now.

>> I think you are being overcautious. Even in this case, you can specify
>> the real name of the web server through git-config or command line
>> parameters to make sure it picks up the right settings. 

If there is an HTTP server "httpd" that does not understand Apache style
configuration file given via "-f" (use of which is a misconfiguration this
patch will start allowing users to make), we would need to add a new
codepath to the script to generate a configuration file to support that
server.  I do not think we have a mechanism to disambiguate different HTTP
server implementations that claim to be "httpd", so at that point we would
need to add something other than the command name for disambiguation.  But
for now I think your patch suffices.

As you pointed out, the last "case" statement in instaweb explicitly have
rejected servers we did not know about, so my worry was unfounded; nobody
could have been using a daemon whose name is "httpd", be it ArchLinux's
Apache or something incompatible.

Thanks.


[Footnote]

*1* Links to marc do not have an easy way other than re-typing to get to a
usable message-id to find the message to reply to, so while I wanted to
make this as a reply to your message from June 30th, I couldn't, and
instead I am sending this as a reply to the new message, and I had to copy
and paste the parts of the message I responded to.