Re: [PATCHv2 GSoC] git-instaweb: Configure it to work with new gitweb structure

From: Jakub Narebski <jnareb@gmail.com>
Date: May 22, 2010, 15:57 UTC
Message-ID: <201005221757.38376.jnareb@gmail.com>
In-Reply-To: <1274523105-3327-1-git-send-email-pavan.sss1991@gmail.com>

On Sat, 22 May 2010, Pavan Kumar Sunkara wrote:

Show 9 quoted lines

> git-instaweb in its current form (re)creates gitweb.cgi and
> (some of) required static files in $GIT_DIR/gitweb/ directory.
> Splitting gitweb would make it difficult for git-instaweb to
> continue with this method.
> 
> Use the instaweb.gitwebdir config variable to point git-instaweb script
> to a global directory which contains gitweb files as server root
> and the httpd.conf along with server logs and pid go into
> '$(GIT_DIR)/gitweb' directory.

That's not all this patch changes, isn't it?

  While at it, change apache2 configuration to use the same access log
  and error log files as the rest of web servers supported by
  git-instaweb.

While it would be better to have it as a separate commit, I think it doesn't matter much, and having it in this patch is acceptable as "while at it" change.

> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

I like this change, because it simplifies greatly git-instaweb generation; besides the fact that it is necessary for future splitting gitweb for better maintability, and for write functionality for gitweb.

Acked-by: Jakub Narebski <jnareb@gmail.com>

_If_ there is no problem with $(gitwebdir) and not $(gitwebdir_SQ), see below for details.

> ---
> 
> This patch is based on commit 'jn/gitweb-install' in the next branch.

I think it is based on your earlier patches:

* gitweb: Move static files into seperate subdirectory
  http://article.gmane.org/gmane.comp.version-control.git/147321 
* gitweb: Set default destination directory for installing gitweb in Makefile
  http://article.gmane.org/gmane.comp.version-control.git/147160

Those are necessary for this patch to be applied. Well, the second is necessary for it to make sense...

BTW. which web servers supported by git-instaweb: lighttpd, apache2, webrick, mongoose you have tested your change with?

Show 10 quoted lines

>  Makefile        |   10 +------
>  git-instaweb.sh |   71 ++++++++++++++++++++----------------------------------
>  2 files changed, 28 insertions(+), 53 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index caf2f64..91cd726 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1592,15 +1592,8 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/static/gitweb.css gitweb/
>  	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \

[...]

Show 13 quoted lines

> +	    -e 's|@@GITWEBDIR@@|$(gitwebdir)|g' \
>  	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
> -            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \
> -            -e 's|@@GITWEB_JS_NAME@@|$(GITWEB_JS)|' \
>  	    $@.sh > $@+ && \
>  	chmod +x $@+ && \
>  	mv $@+ $@
> @@ -1972,6 +1965,7 @@ install: all
>  	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
>  ifndef NO_PERL
>  	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
> +	$(MAKE) -C gitweb gitwebdir=$(gitwebdir) install
>  endif

Have you checked that you can use $(gitwebdir) and don't need $(gitwebdir_SQ) here? Does git-instaweb installs and works correctly if 'gitwebdir' contains spaces and single or double quote characters?

But perhaps that doesn't matter in practice, and this is good enough.

Show 19 quoted lines

> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index f608014..b3e9192 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -24,6 +24,7 @@ restart        restart the web server
>  fqgitdir="$GIT_DIR"
>  local="$(git config --bool --get instaweb.local)"
>  httpd="$(git config --get instaweb.httpd)"
> +root="$(git config --get instaweb.gitwebdir)"
>  port=$(git config --get instaweb.port)
>  module_path="$(git config --get instaweb.modulepath)"
>  
> @@ -34,6 +35,9 @@ conf="$GIT_DIR/gitweb/httpd.conf"
>  # if installed, it doesn't need further configuration (module_path)
>  test -z "$httpd" && httpd='lighttpd -f'
>  
> +# Default is @@GITWEBDIR@@
> +test -z "$root" && root='@@GITWEBDIR@@'
> +

Nice.

Show 9 quoted lines

> @@ -57,7 +61,7 @@ resolve_full_httpd () {
>  		# these days and those are not in most users $PATHs
>  		# in addition, we may have generated a server script
>  		# in $fqgitdir/gitweb.
> -		for i in /usr/local/sbin /usr/sbin "$fqgitdir/gitweb"
> +		for i in /usr/local/sbin /usr/sbin "$root" "$fqgitdir/gitweb"
>  		do
>  			if test -x "$i/$httpd_only"
>  			then

You probably want to update comment above this loop. But this is not something very important.

Alternatively, if e.g. webrick.rb and webrick (shell script) are installed in "$fqgitdir/gitweb" directory, there is no need to check "$root".

>  webrick_conf () {

[...]

> -:DocumentRoot: "$fqgitdir/gitweb"
> +:DocumentRoot: "$root"

>  lighttpd_conf () {
>  	cat > "$conf" <<EOF
> -server.document-root = "$fqgitdir/gitweb"
> +server.document-root = "$root"

[...]

> -setenv.add-environment = ( "PATH" => env.PATH )
> +setenv.add-environment = ( "PATH" => env.PATH, "GITWEB_CONFIG" => env.GITWEB_CONFIG )

>  apache2_conf () {

[...]

> -ServerRoot "$fqgitdir/gitweb"
> -DocumentRoot "$fqgitdir/gitweb"
> +ServerRoot "$root"
> +DocumentRoot "$root"

[...]

>  PerlPassEnv GIT_DIR
>  PerlPassEnv GIT_EXEC_DIR
> +PerlPassEnv GITWEB_CONFIG

Show 6 quoted lines

> @@ -353,7 +359,7 @@ mongoose_conf() {
>  # For detailed description of every option, visit
>  # http://code.google.com/p/mongoose/wiki/MongooseManual
>  
> -root		$fqgitdir/gitweb
> +root		$root

[...]

> -cgi_env		PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
> +cgi_env		PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH,GITWEB_CONFIG=$GITWEB_CONFIG

All right, those changes are pretty clear.

Show 19 quoted lines

> @@ -277,14 +281,15 @@ EOF
>  
>  apache2_conf () {
>  	test -z "$module_path" && module_path=/usr/lib/apache2/modules
> -	mkdir -p "$GIT_DIR/gitweb/logs"
>  	bind=
>  	test x"$local" = xtrue && bind='127.0.0.1:'
>  	echo 'text/css css' > "$fqgitdir/mime.types"
>  	cat > "$conf" <<EOF
>  ServerName "git-instaweb"
> -ServerRoot "$fqgitdir/gitweb"
> -DocumentRoot "$fqgitdir/gitweb"
> +ServerRoot "$root"
> +DocumentRoot "$root"
> +ErrorLog "$fqgitdir/gitweb/error.log"
> +CustomLog "$fqgitdir/gitweb/access.log" combined
>  PidFile "$fqgitdir/pid"
>  Listen $bind$port
>  EOF

This is independent change, modifying configuration of apache2 web server to use the same files for access log and for error log ("$fqgitdir/gitweb/access.log" and "$fqgitdir/gitweb/error.log", respectively) as the rest of web servers.

Isn't it?

Show 12 quoted lines

> @@ -370,41 +376,16 @@ mime_types	.gz=application/x-gzip,.tar.gz=application/x-tgz,.tgz=application/x-t
>  EOF
>  }
>  
> -
> -script='
> -s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
> -s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
> -s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
> -s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'
> -
> -gitweb_cgi () {

[...]

Show 8 quoted lines

> +gitweb_conf() {
> +	cat > "$fqgitdir/gitweb/gitweb_config.perl" <<EOF
> +#!/usr/bin/perl
> +our \$projectroot = "$(dirname "$fqgitdir")";
> +our \$git_temp = "$fqgitdir/gitweb/tmp";
> +our \$projects_list = \$projectroot;
> +EOF
>  }

Right, $GIT (formerly $gitbin) is set when generating gitweb.cgi from gitweb.perl.

Actually $git_temp is not needed by modern gitweb (from quite some time, since using external /usr/bin/diff was replaced by git-diff-tree), so setting it could be removed from gitweb_config.perl. Nevertheless it is not a problem having it.

Show 9 quoted lines

> -gitweb_cgi "$GIT_DIR/gitweb/gitweb.cgi"
> -gitweb_css "$GIT_DIR/@@GITWEB_CSS_NAME@@"
> -gitweb_js  "$GIT_DIR/@@GITWEB_JS_NAME@@"
> +gitweb_conf
>  
>  case "$httpd" in
>  *lighttpd*)
> -- 
> 1.7.1.18.g74211d.dirty

-- 
Jakub Narebski
Poland