On Sat, 22 May 2010, Pavan Kumar Sunkara wrote: > 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@xxxxxxxxx> 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@xxxxxxxxx> _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? > 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)|' \ [...] > + -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. > 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. > @@ -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 > @@ -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. > @@ -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? > @@ -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 () { [...] > +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. > -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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html