[PATCHv2 GSoC 1/3] gitweb: Set default destination directory for installing gitweb in Makefile

[PATCHv2 GSoC 1/3] gitweb: Set default destination directory for installing gitweb in Makefile

[Pavan Kumar Sunkara]

Currently installing gitweb requires to give a target directory
(via 'gitwebdir' build variable).  Giving it a default value
protects against user errors.

Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index de7f680..caf2f64 100644
--- a/Makefile
+++ b/Makefile
@@ -269,6 +269,7 @@ mandir = share/man
 infodir = share/info
 gitexecdir = libexec/git-core
 sharedir = $(prefix)/share
+gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
-- 
1.7.1.16.g5d405c.dirty

[PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Pavan Kumar Sunkara]

git-instaweb in its current form (re)creates gitweb.cgi and
(some of) required static files in $GIT_DIR/gitweb/ directory for
each repository it is ran. Splitting gitweb would make it difficult
for git-instaweb to continue with this method.

Use the instaweb.root 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
'$(HOME)/.gitweb' directory.

As there is no need to call git-instaweb in every git repository,
configure gitweb to get $projects_list from file '$(HOME)/.gitweb/list'
and $projectroot is ''

Example of ~/.gitweb/list:
home%2Fpavan%2Fgit%2F.git Linus+Torvalds
home%2Fpavan%2Fgsoc%2F.git Pavan+Kumar+Sunkara

Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---
 Makefile        |    9 +----
 git-instaweb.sh |  100 ++++++++++++++++++-------------------------------------
 2 files changed, 34 insertions(+), 75 deletions(-)

diff --git a/Makefile b/Makefile
index caf2f64..1e9fb77 100644
--- a/Makefile
+++ b/Makefile
@@ -1592,15 +1592,7 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/static/gitweb.css gitweb/
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    -e '/@@GITWEB_CGI@@/r gitweb/gitweb.cgi' \
-	    -e '/@@GITWEB_CGI@@/d' \
-	    -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \
-	    -e '/@@GITWEB_CSS@@/d' \
-	    -e '/@@GITWEB_JS@@/r $(GITWEB_JS)' \
-	    -e '/@@GITWEB_JS@@/d' \
 	    -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 +1964,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) GITWEB_LIST=$(HOME)/.gitweb/list GITWEB_PROJECTROOT='' install
 endif
 ifndef NO_PYTHON
 	$(MAKE) -C git_remote_helpers prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
diff --git a/git-instaweb.sh b/git-instaweb.sh
index f608014..4aaacbb 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -19,21 +19,30 @@ start          start the web server
 restart        restart the web server
 "
 
+# This must be capable of running outside of git directory, so
+# the vanilla git-sh-setup should not be used.
+NONGIT_OK=Yes
 . git-sh-setup
 
-fqgitdir="$GIT_DIR"
+fqgitdir="$HOME/.gitweb"
 local="$(git config --bool --get instaweb.local)"
 httpd="$(git config --get instaweb.httpd)"
+root="$(git config --get instaweb.root)" 
 port=$(git config --get instaweb.port)
 module_path="$(git config --get instaweb.modulepath)"
 
-conf="$GIT_DIR/gitweb/httpd.conf"
+mkdir -p "$fqgitdir/tmp"
+test ! -w "$fqgitdir/list" && touch "$fqgitdir/list"
+conf="$fqgitdir/httpd.conf"
 
 # Defaults:
 
 # if installed, it doesn't need further configuration (module_path)
 test -z "$httpd" && httpd='lighttpd -f'
 
+# Default is /usr/share/gitweb
+test -z "$root" && root='/usr/share/gitweb'
+
 # any untaken local port will do...
 test -z "$port" && port=1234
 
@@ -56,8 +65,8 @@ resolve_full_httpd () {
 		# many httpds are installed in /usr/sbin or /usr/local/sbin
 		# 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"
+		# in $fqgitdir.
+		for i in /usr/local/sbin /usr/sbin "$fqgitdir"
 		do
 			if test -x "$i/$httpd_only"
 			then
@@ -85,7 +94,7 @@ start_httpd () {
 	case "$httpd" in
 	*mongoose*)
 		#The mongoose server doesn't have a daemon mode so we'll have to fork it
-		$full_httpd "$fqgitdir/gitweb/httpd.conf" &
+		$full_httpd "$conf" &
 		#Save the pid before doing anything else (we'll print it later)
 		pid=$!
 
@@ -99,7 +108,7 @@ $pid
 EOF
 		;;
 	*)
-		$full_httpd "$fqgitdir/gitweb/httpd.conf"
+		$full_httpd "$conf"
 		if test $? != 0; then
 			echo "Could not execute http daemon $httpd."
 			exit 1
@@ -156,15 +165,9 @@ do
 	shift
 done
 
-mkdir -p "$GIT_DIR/gitweb/tmp"
-GIT_EXEC_PATH="$(git --exec-path)"
-GIT_DIR="$fqgitdir"
-export GIT_EXEC_PATH GIT_DIR
-
-
 webrick_conf () {
 	# generate a standalone server script in $fqgitdir/gitweb.
-	cat >"$fqgitdir/gitweb/$httpd.rb" <<EOF
+	cat >"$fqgitdir/$httpd.rb" <<EOF
 require 'webrick'
 require 'yaml'
 options = YAML::load_file(ARGV[0])
@@ -184,15 +187,15 @@ EOF
 	# which assumes _ruby_ is in the user's $PATH. that's _one_
 	# portable way to run ruby, which could be installed anywhere,
 	# really.
-	cat >"$fqgitdir/gitweb/$httpd" <<EOF
+	cat >"$fqgitdir/$httpd" <<EOF
 #!/bin/sh
-exec ruby "$fqgitdir/gitweb/$httpd.rb" \$*
+exec ruby "$fqgitdir/$httpd.rb" \$*
 EOF
-	chmod +x "$fqgitdir/gitweb/$httpd"
+	chmod +x "$fqgitdir/$httpd"
 
 	cat >"$conf" <<EOF
 :Port: $port
-:DocumentRoot: "$fqgitdir/gitweb"
+:DocumentRoot: "$root"
 :DirectoryIndex: ["gitweb.cgi"]
 :PidFile: "$fqgitdir/pid"
 EOF
@@ -201,16 +204,16 @@ EOF
 
 lighttpd_conf () {
 	cat > "$conf" <<EOF
-server.document-root = "$fqgitdir/gitweb"
+server.document-root = "$root"
 server.port = $port
 server.modules = ( "mod_setenv", "mod_cgi" )
 server.indexfiles = ( "gitweb.cgi" )
 server.pid-file = "$fqgitdir/pid"
-server.errorlog = "$fqgitdir/gitweb/error.log"
+server.errorlog = "$fqgitdir/error.log"
 
 # to enable, add "mod_access", "mod_accesslog" to server.modules
 # variable above and uncomment this
-#accesslog.filename = "$fqgitdir/gitweb/access.log"
+#accesslog.filename = "$fqgitdir/access.log"
 
 setenv.add-environment = ( "PATH" => env.PATH )
 
@@ -277,14 +280,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/error.log"
+CustomLog "$fqgitdir/access.log" combined
 PidFile "$fqgitdir/pid"
 Listen $bind$port
 EOF
@@ -303,13 +307,11 @@ EOF
 	# check to see if Dennis Stosberg's mod_perl compatibility patch
 	# (<20060621130708.Gcbc6e5c@leonov.stosberg.net>) has been applied
 	if test -f "$module_path/mod_perl.so" &&
-	   sane_grep 'MOD_PERL' "$GIT_DIR/gitweb/gitweb.cgi" >/dev/null
+	   sane_grep 'MOD_PERL' "$root/gitweb.cgi" >/dev/null
 	then
 		# favor mod_perl if available
 		cat >> "$conf" <<EOF
 LoadModule perl_module $module_path/mod_perl.so
-PerlPassEnv GIT_DIR
-PerlPassEnv GIT_EXEC_DIR
 <Location /gitweb.cgi>
 	SetHandler perl-script
 	PerlResponseHandler ModPerl::Registry
@@ -353,15 +355,15 @@ mongoose_conf() {
 # For detailed description of every option, visit
 # http://code.google.com/p/mongoose/wiki/MongooseManual
 
-root		$fqgitdir/gitweb
+root		$root
 ports		$port
 index_files	gitweb.cgi
-#ssl_cert	$fqgitdir/gitweb/ssl_cert.pem
-error_log	$fqgitdir/gitweb/error.log
-access_log	$fqgitdir/gitweb/access.log
+#ssl_cert	$fqgitdir/ssl_cert.pem
+error_log	$fqgitdir/error.log
+access_log	$fqgitdir/access.log
 
 #cgi setup
-cgi_env		PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
+cgi_env		PATH=$PATH
 cgi_interp	$PERL
 cgi_ext		cgi,pl
 
@@ -370,42 +372,6 @@ 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 () {
-	cat > "$1.tmp" <<\EOFGITWEB
-@@GITWEB_CGI@@
-EOFGITWEB
-	# Use the configured full path to perl to match the generated
-	# scripts' 'hashpling' line
-	"$PERL" -p -e "$script" "$1.tmp"  > "$1"
-	chmod +x "$1"
-	rm -f "$1.tmp"
-}
-
-gitweb_css () {
-	cat > "$1" <<\EOFGITWEB
-@@GITWEB_CSS@@
-
-EOFGITWEB
-}
-
-gitweb_js () {
-	cat > "$1" <<\EOFGITWEB
-@@GITWEB_JS@@
-
-EOFGITWEB
-}
-
-gitweb_cgi "$GIT_DIR/gitweb/gitweb.cgi"
-gitweb_css "$GIT_DIR/@@GITWEB_CSS_NAME@@"
-gitweb_js  "$GIT_DIR/@@GITWEB_JS_NAME@@"
-
 case "$httpd" in
 *lighttpd*)
 	lighttpd_conf
-- 
1.7.1.16.g5d405c.dirty

[PATCH 3/3] git-web--browse: Add support for google chrome

[Pavan Kumar Sunkara]

Add support for another web browser called chrome. To
select it, the value of the browser should be 'chromium'.

Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---
 git-web--browse.sh |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/git-web--browse.sh b/git-web--browse.sh
index a578c3a..72dde3b 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -31,7 +31,7 @@ valid_custom_tool()
 
 valid_tool() {
 	case "$1" in
-		firefox | iceweasel | konqueror | w3m | links | lynx | dillo | open | start)
+		firefox | iceweasel | chromium | konqueror | w3m | links | lynx | dillo | open | start)
 			;; # happy
 		*)
 			valid_custom_tool "$1" || return 1
@@ -103,7 +103,7 @@ fi
 
 if test -z "$browser" ; then
     if test -n "$DISPLAY"; then
-	browser_candidates="firefox iceweasel konqueror w3m links lynx dillo"
+	browser_candidates="firefox iceweasel chromium konqueror w3m links lynx dillo"
 	if test "$KDE_FULL_SESSION" = "true"; then
 	    browser_candidates="konqueror $browser_candidates"
 	fi
@@ -146,6 +146,11 @@ case "$browser" in
 	test "$vers" -lt 2 && NEWTAB=''
 	"$browser_path" $NEWTAB "$@" &
 	;;
+    chromium)
+	# Actual command for chromium is chromium-browser.
+	# No need to specify newTab. It's default in chromium
+	eval "$browser_path-browser" "$@" &
+	;;
     konqueror)
 	case "$(basename "$browser_path")" in
 	    konqueror)
-- 
1.7.1.16.g5d405c.dirty

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Dévai Tamás]

> +# Default is /usr/share/gitweb
> +test -z "$root" && root='/usr/share/gitweb'

Just one question: what happens, when one installs to /usr/local (or any
other directory) instead of /usr? I'd use the $(gitwebdir) variable from
the Makefile as default, such as

  # if installed, it doesn't need further configuration (module_path)
  test -z "$httpd" && httpd='lighttpd -f'
  
-+# Default is /usr/share/gitweb
-+test -z "$root" && root='/usr/share/gitweb'
++# Default is @@gitwebdir@@
++test -z "$root" && root='@@gitwebdir@@'
 +
  # any untaken local port will do...
  test -z "$port" && port=1234

and in the Makefile

         sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
             -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
             -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
++           -e 's|@@gitwebdir@@|$(gitwebdir)|g' \
 -           -e '/@@GITWEB_CGI@@/r gitweb/gitweb.cgi' \
 -           -e '/@@GITWEB_CGI@@/d' \
 -           -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \

Hope that you find it reasonable.
--
Dévai Tamás

Re: [PATCH 3/3] git-web--browse: Add support for google chrome

[Erik Faye-Lund]

On Sat, May 15, 2010 at 9:58 PM, Pavan Kumar Sunkara
<pavan.sss1991@gmail.com> wrote:
> Add support for another web browser called chrome. To
> select it, the value of the browser should be 'chromium'.

Since we already have firefox AND iceweasel, wouldn't it make sense to
add chrome AND chromium?

-- 
Erik "kusma" Faye-Lund

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Pavan Kumar Sunkara]

Thanks Devai. It's great.

- pavan

On Sun, May 16, 2010 at 5:46 AM, Dévai Tamás <devait@vnet.hu> wrote:
>> +# Default is /usr/share/gitweb
>> +test -z "$root" && root='/usr/share/gitweb'
>
> Just one question: what happens, when one installs to /usr/local (or any
> other directory) instead of /usr? I'd use the $(gitwebdir) variable from
> the Makefile as default, such as
>
>  # if installed, it doesn't need further configuration (module_path)
>  test -z "$httpd" && httpd='lighttpd -f'
>
> -+# Default is /usr/share/gitweb
> -+test -z "$root" && root='/usr/share/gitweb'
> ++# Default is @@gitwebdir@@
> ++test -z "$root" && root='@@gitwebdir@@'
>  +
>  # any untaken local port will do...
>  test -z "$port" && port=1234
>
> and in the Makefile
>
>         sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>             -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>             -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> ++           -e 's|@@gitwebdir@@|$(gitwebdir)|g' \
>  -           -e '/@@GITWEB_CGI@@/r gitweb/gitweb.cgi' \
>  -           -e '/@@GITWEB_CGI@@/d' \
>  -           -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \
>
> Hope that you find it reasonable.
> --
> Dévai Tamás
>
>

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Jakub Narebski]

Cc-ed to Eric Wond, main author and maintainer of git-instaweb.

Pavan Kumar Sunkara <pavan.sss1991@gmail.com> writes:

> git-instaweb in its current form (re)creates gitweb.cgi and
> (some of) required static files in $GIT_DIR/gitweb/ directory for
> each repository it is ran. Splitting gitweb would make it difficult
> for git-instaweb to continue with this method.

I agree with that.

(By the way, I really like this paragraph of commit mesage, which is
introduction to a commit, describing current situation and problems
with it.)

> 
> Use the instaweb.root config variable to point git-instaweb script
> to a global directory which contains gitweb files as server root

A nitpick about style of grammar of this commit message: there should
be fullstop here, ending sentence.

While I can agree with `instaweb.root' config variable to *override*
the default, it should have sane default, and setting it should be not
required to be able to run git-instaweb.  Therefore the 'install'
target of main Makefile should either:
a.) install gitweb into gitdir=$(sharedir)/gitweb, and make 
    instaweb.root be $(sharedir)/gitweb by default
b.) install gitweb into $(gitwebdir), which only have $(sharedir)/gitweb
    as default, and embed $(gitwebdir) in git-instaweb script when 
    building, so that it would be default value of instaweb.root

This would probably mean replacing either @@sharedir@@ or @@gitwebdir@@
placeholders in git-instaweb.sh when building git-instaweb.

> and the httpd.conf along with server logs and pid go into
> '$(HOME)/.gitweb' directory.
> 
> As there is no need to call git-instaweb in every git repository,
> configure gitweb to get $projects_list from file '$(HOME)/.gitweb/list'
> and $projectroot is ''
> 
> Example of ~/.gitweb/list:
> home%2Fpavan%2Fgit%2F.git Linus+Torvalds
> home%2Fpavan%2Fgsoc%2F.git Pavan+Kumar+Sunkara

This is quite a large change on how git-instaweb works.

First, I think such change should be better left for a separate
commit, splitting this one in two: one making git-instaweb use
installed gitweb files, and installing gitweb files somewhere when
installing git, and second changing how git-instaweb behave.
"Do one thing, and do it well."  It would make easier to check
if there are errors in the commit.

Second, in my opinion it is not a good change at all.  Currently you
can run "git instaweb" when inside git repository, and get a web
browser (or a new tab in existing session of a running web browser)
with current repository in it, to browse its history.  It is similar
to running gitk (or other graphical history browser, like qgit, tig,
etc.), or running "git log", but with web interface.

Now, current git-instaweb behavior has its quirks, but having
git-instaweb show _current_ repository is a very important feature,
and I'd rather we didn't lose it in transition.

So in my opinion it would be better to just update git-instaweb and
generating git-instaweb to make use of installed gitweb and installed
gitweb files, but do not change organization of generated files; just
instead of gitweb.cgi there should be gitweb_config.perl with
appropriate configuration to show current repository.  And of course
there would be no gitweb files in $GIT_DIR/gitweb (in .git/gitweb)
directory.

> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
> ---
>  Makefile        |    9 +----
>  git-instaweb.sh |  100 ++++++++++++++++++-------------------------------------
>  2 files changed, 34 insertions(+), 75 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index caf2f64..1e9fb77 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1592,15 +1592,7 @@ git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/static/gitweb.css gitweb/
>  	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>  	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>  	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> -	    -e '/@@GITWEB_CGI@@/r gitweb/gitweb.cgi' \
> -	    -e '/@@GITWEB_CGI@@/d' \
> -	    -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \
> -	    -e '/@@GITWEB_CSS@@/d' \
> -	    -e '/@@GITWEB_JS@@/r $(GITWEB_JS)' \
> -	    -e '/@@GITWEB_JS@@/d' \
>  	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \

Good to leave this change.

> -            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \
> -            -e 's|@@GITWEB_JS_NAME@@|$(GITWEB_JS)|' \

Hmmm... I winder why we had there indenting using spaces only, instead
of initial tab here...  Doesn't matter for this commit, though.

>  	    $@.sh > $@+ && \
>  	chmod +x $@+ && \
>  	mv $@+ $@
> @@ -1972,6 +1964,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) GITWEB_LIST=$(HOME)/.gitweb/list GITWEB_PROJECTROOT='' install

There is no need to pass "gitwebdir=$(gitwebdir)" to submakefile, I
think, but you should check that.  Anyway, it should be

  +	$(MAKE) -C gitweb gitwebdir=$(gitwebdir_SQ) ...


See also my comments about why change in git-instaweb behavior is not
good idea, especially not in this commit.  _Perhaps_ it should be
instead somthing like this:

  +	$(MAKE) -C gitweb gitwebdir=$(gitwebdir_SQ) \
  +		GITWEB_CONFIG='$ENV{GIT_DIR}/gitweb/gitweb_config.perl' install

Or something like that (not tested!)

> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index f608014..4aaacbb 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -19,21 +19,30 @@ start          start the web server
>  restart        restart the web server
>  "
>  
> +# This must be capable of running outside of git directory, so
> +# the vanilla git-sh-setup should not be used.
> +NONGIT_OK=Yes

This is related to the change in git-instaweb behavior.  IMVHO 
"git instaweb" should work just like "git log" or gitk, so requiring
to be run from git repository is not a bad requirement.

>  . git-sh-setup
>  
> -fqgitdir="$GIT_DIR"
> +fqgitdir="$HOME/.gitweb"

This is related to the change in git-instaweb behavior.  Anyway, the
'fqgitdir' name for this variable doesn't make much sense after this
change, isn't it?

>  local="$(git config --bool --get instaweb.local)"
>  httpd="$(git config --get instaweb.httpd)"
> +root="$(git config --get instaweb.root)" 

Trailing space.

I'm not entirely happy with the name of this config variable.  Perhaps
instaweb.gitwebdir would be better?  Also, we have to make sure that
git-instaweb would work even if this config variable is unset; perhaps
you do this later.

>  port=$(git config --get instaweb.port)
>  module_path="$(git config --get instaweb.modulepath)"
>  
> -conf="$GIT_DIR/gitweb/httpd.conf"
> +mkdir -p "$fqgitdir/tmp"
> +test ! -w "$fqgitdir/list" && touch "$fqgitdir/list"
> +conf="$fqgitdir/httpd.conf"

First, a functional change.

Second, "mkdir -p" is not portable, although I am not sure if it is a
problem in practice (i.e. if it is a problem on any platform that
gi-instaweb works now).

But I see that git-instaweb used "mkdir -p" before...

>  # Defaults:
>  
>  # if installed, it doesn't need further configuration (module_path)
>  test -z "$httpd" && httpd='lighttpd -f'
>  
> +# Default is /usr/share/gitweb
> +test -z "$root" && root='/usr/share/gitweb'
> +

It should be either

  +test -z "$root" && root='@@gitwebdir@@'

or

  +test -z "$root" && root='@@sharedir@@/gitweb'

(with placeholders replaced by "make git-instaweb").

> @@ -56,8 +65,8 @@ resolve_full_httpd () {
>  		# many httpds are installed in /usr/sbin or /usr/local/sbin
>  		# 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"
> +		# in $fqgitdir.
> +		for i in /usr/local/sbin /usr/sbin "$fqgitdir"

I think we should *add* "$root" here, but not remove the feature that
server script might be generated in "$GIT_DIR/gitweb", i.e. in
"$fqgitdir".  So the last line would be:

  +		for i in /usr/local/sbin /usr/sbin "$root" "$fqgitdir/gitweb"

>  		do
>  			if test -x "$i/$httpd_only"
>  			then
> @@ -85,7 +94,7 @@ start_httpd () {
>  	case "$httpd" in
>  	*mongoose*)
>  		#The mongoose server doesn't have a daemon mode so we'll have to fork it
> -		$full_httpd "$fqgitdir/gitweb/httpd.conf" &
> +		$full_httpd "$conf" &
>  		#Save the pid before doing anything else (we'll print it later)
>  		pid=$!
>  

This is change in how git-instaweb works.  Mind you, perhaps *this*
part of change is good... but not in this commit.

We might want to introduce $fqconf variable in preparatory commit...

> @@ -99,7 +108,7 @@ $pid
>  EOF
>  		;;
>  	*)
> -		$full_httpd "$fqgitdir/gitweb/httpd.conf"
> +		$full_httpd "$conf"
>  		if test $? != 0; then
>  			echo "Could not execute http daemon $httpd."
>  			exit 1

Same as above.

> @@ -156,15 +165,9 @@ do
>  	shift
>  done
>  
> -mkdir -p "$GIT_DIR/gitweb/tmp"

Ah, I see that git-instaweb used "mkdir -p" before...

>  	cat >"$conf" <<EOF
>  :Port: $port
> -:DocumentRoot: "$fqgitdir/gitweb"
> +:DocumentRoot: "$root"

> -server.document-root = "$fqgitdir/gitweb"
> +server.document-root = "$root"

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

> -root		$fqgitdir/gitweb
> +root		$root

Good.

Ah, I see, that is why instaweb.root name for config variable, and
$root name for variable in git-instaweb script... but 'root' meaning
'DocumentRoot' makes sense *only* in context.  That is why
instaweb.root is not IMHO a good name.

I am not against $root as name of variable, because it is hidden, and
is invariably in the context ;-)

> -server.errorlog = "$fqgitdir/gitweb/error.log"
> +server.errorlog = "$fqgitdir/error.log"
>  
>  # to enable, add "mod_access", "mod_accesslog" to server.modules
>  # variable above and uncomment this
> -#accesslog.filename = "$fqgitdir/gitweb/access.log"
> +#accesslog.filename = "$fqgitdir/access.log"

Without change in how git-instaweb work, i.e. with separate server
invoked for each repository (it might be a thing that we want to
change, but again: not in this commit), it makes sense to also have
error log and access log separate for each repository.

We could have used $fqgitwebdir variable here, or something like that.

>  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"
> +ErrorLog "$fqgitdir/error.log"
> +CustomLog "$fqgitdir/access.log" combined
>  PidFile "$fqgitdir/pid"
>  Listen $bind$port
>  EOF

Is this something new?  Did apache2 produced error log and access log
before this?

If it is something new, it should be put in separate (probably
preparatory) commit.

> @@ -303,13 +307,11 @@ EOF
>  	# check to see if Dennis Stosberg's mod_perl compatibility patch
>  	# (<20060621130708.Gcbc6e5c@leonov.stosberg.net>) has been applied
>  	if test -f "$module_path/mod_perl.so" &&
> -	   sane_grep 'MOD_PERL' "$GIT_DIR/gitweb/gitweb.cgi" >/dev/null
> +	   sane_grep 'MOD_PERL' "$root/gitweb.cgi" >/dev/null

Sidenote: I gues that this check could be removed now, but this is an
independent change.

>  	then
>  		# favor mod_perl if available
>  		cat >> "$conf" <<EOF
>  LoadModule perl_module $module_path/mod_perl.so
> -PerlPassEnv GIT_DIR
> -PerlPassEnv GIT_EXEC_DIR

In a minimal patch, the one that doesn't change git-instaweb
behaviour, and simply creates gitweb_config.perl in $GIT_DIR/gitweb in
place of gitweb.cgi and gitweb files, we would not want to remove
those two lines, but add instead

  +PerlPassEnv GITWEB_CONFIG

>  #cgi setup
> -cgi_env		PATH=$PATH,GIT_DIR=$GIT_DIR,GIT_EXEC_PATH=$GIT_EXEC_PATH
> +cgi_env		PATH=$PATH
>  cgi_interp	$PERL
>  cgi_ext		cgi,pl

Similarly, append ',GITWEB_CONFIG=$GITWEB_CONFIG' here.

> -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_cgi "$GIT_DIR/gitweb/gitweb.cgi"
> -gitweb_css "$GIT_DIR/@@GITWEB_CSS_NAME@@"
> -gitweb_js  "$GIT_DIR/@@GITWEB_JS_NAME@@"

This should be in my opinion replaced by generating proper
gitweb_config.perl file in $GIT_DIR/gitweb, and setting GITWEB_CONFIG
variable before running web server.


P.S. As the main goal of your GSoC project is create web interface
equivalent of git-gui (like gitweb is web interface equivalent of
gitk), with a secondary goal of splitting gitweb to make it easy to
add such new functionality without losing maintability, I think you
should not concentrate on this part.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Pavan Kumar Sunkara]

> While I can agree with `instaweb.root' config variable to *override*
> the default, it should have sane default, and setting it should be not
> required to be able to run git-instaweb.  Therefore the 'install'
> target of main Makefile should either:
> a.) install gitweb into gitdir=$(sharedir)/gitweb, and make
>    instaweb.root be $(sharedir)/gitweb by default
> b.) install gitweb into $(gitwebdir), which only have $(sharedir)/gitweb
>    as default, and embed $(gitwebdir) in git-instaweb script when
>    building, so that it would be default value of instaweb.root
>
> This would probably mean replacing either @@sharedir@@ or @@gitwebdir@@
> placeholders in git-instaweb.sh when building git-instaweb.

Yeah I will do that.

>> and the httpd.conf along with server logs and pid go into
>> '$(HOME)/.gitweb' directory.
>>
>> As there is no need to call git-instaweb in every git repository,
>> configure gitweb to get $projects_list from file '$(HOME)/.gitweb/list'
>> and $projectroot is ''
>>
>> Example of ~/.gitweb/list:
>> home%2Fpavan%2Fgit%2F.git Linus+Torvalds
>> home%2Fpavan%2Fgsoc%2F.git Pavan+Kumar+Sunkara
>
> This is quite a large change on how git-instaweb works.
>
> First, I think such change should be better left for a separate
> commit, splitting this one in two: one making git-instaweb use
> installed gitweb files, and installing gitweb files somewhere when
> installing git, and second changing how git-instaweb behave.
> "Do one thing, and do it well."  It would make easier to check
> if there are errors in the commit.

Ok.

> Second, in my opinion it is not a good change at all.  Currently you
> can run "git instaweb" when inside git repository, and get a web
> browser (or a new tab in existing session of a running web browser)
> with current repository in it, to browse its history.  It is similar
> to running gitk (or other graphical history browser, like qgit, tig,
> etc.), or running "git log", but with web interface.

Yes. But this change is vital for the success of my GSoC project. I
will explain why.
If you remember, my GSoC project contains some functionalities like
creating/cloning repositories which need a server which need to start
outside git directory.

Until now, I thought to use git-instaweb to do this. But I realised
now that it would be better if we have another script.
So we need to have a different "git-client" script which starts this client.

What do you say ?

> Now, current git-instaweb behavior has its quirks, but having
> git-instaweb show _current_ repository is a very important feature,
> and I'd rather we didn't lose it in transition.
>
> So in my opinion it would be better to just update git-instaweb and
> generating git-instaweb to make use of installed gitweb and installed
> gitweb files, but do not change organization of generated files; just
> instead of gitweb.cgi there should be gitweb_config.perl with
> appropriate configuration to show current repository.  And of course
> there would be no gitweb files in $GIT_DIR/gitweb (in .git/gitweb)
> directory.

Ok. Sure I will do it.

Thanks
- Pavan

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Eric Wong]

Jakub Narebski <jnareb@gmail.com> wrote:
> Second, in my opinion it is not a good change at all.  Currently you
> can run "git instaweb" when inside git repository, and get a web
> browser (or a new tab in existing session of a running web browser)
> with current repository in it, to browse its history.  It is similar
> to running gitk (or other graphical history browser, like qgit, tig,
> etc.), or running "git log", but with web interface.
> 
> Now, current git-instaweb behavior has its quirks, but having
> git-instaweb show _current_ repository is a very important feature,
> and I'd rather we didn't lose it in transition.

I agree completely with Jakub here.  My original inspiration for
instaweb was being able to make local commits, fire up instaweb in my
working directory, and then to get a non-git-using cow-orker on my LAN
to review some changes before I pushed them upstream.

I'm no longer in a situation with non-git-using cow-orkers anymore, but
I suspect many folks still are.

-- 
Eric Wong

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Jakub Narebski]

On Tue, 18 May 2010, Pavan Kumar Sunkara wrote:
> Jakub Narebski wrote:
>> Pavan Kumar Sunkara wrote:

>>> and the httpd.conf along with server logs and pid go into
>>> '$(HOME)/.gitweb' directory.
>>>
>>> As there is no need to call git-instaweb in every git repository,
>>> configure gitweb to get $projects_list from file '$(HOME)/.gitweb/list'
>>> and $projectroot is ''
>>>
>>> Example of ~/.gitweb/list:
>>> home%2Fpavan%2Fgit%2F.git Linus+Torvalds
>>> home%2Fpavan%2Fgsoc%2F.git Pavan+Kumar+Sunkara
>>
>> This is quite a large change on how git-instaweb works.
>>
>> First, I think such change should be better left for a separate
>> commit, splitting this one in two: one making git-instaweb use
>> installed gitweb files, and installing gitweb files somewhere when
>> installing git, and second changing how git-instaweb behave.
>> "Do one thing, and do it well."  It would make easier to check
>> if there are errors in the commit.
> 
> Ok.
> 
>> Second, in my opinion it is not a good change at all.  Currently you
>> can run "git instaweb" when inside git repository, and get a web
>> browser (or a new tab in existing session of a running web browser)
>> with current repository in it, to browse its history.  It is similar
>> to running gitk (or other graphical history browser, like qgit, tig,
>> etc.), or running "git log", but with web interface.
> 
> Yes. But this change is vital for the success of my GSoC project. I
> will explain why.
> If you remember, my GSoC project contains some functionalities like
> creating/cloning repositories which need a server which need to start
> outside git directory.
> 
> Until now, I thought to use git-instaweb to do this. But I realised
> now that it would be better if we have another script.
> So we need to have a different "git-client" script which starts this client.
> 
> What do you say ?

Well, I can understand that.

There are two options how to resolve this issue without adding yet
another script (although on the other hand git-web-gui / git-client
could share code with git-instaweb just like git-difftool and 
git-mergetool do).

First is to leave git-instaweb similar to how it is now, with pid file,
server config file, gitweb config file, etc. in $GIT_DIR/gitweb, but
if it is invoked outside any git repository, start it in "repository
administration" mode, i.e. on the page that allows one to create new
repository or clone repository.

The alternate solution would be to follow the idea implemented in this
patch, namely per-user pid file, gitweb config file, server config file
etc. and the *list of projects* file in $HOME/.gitweb (or whenever
XDG / FHS / LSB says it should be named), _but_ add an easy way to add
a new project (a new repositoey) to list.  Perhaps even make 
'git instaweb', when run from inside git repository, add automatically
current repository to list (unless it is present there already), and
perhaps open 'summary' page for said repository.

But independently on which solution would be chosen, it should take place
in a separate commit,
> 
>> Now, current git-instaweb behavior has its quirks, but having
>> git-instaweb show _current_ repository is a very important feature,
>> and I'd rather we didn't lose it in transition.
>>
>> So in my opinion it would be better to just update git-instaweb and
>> generating git-instaweb to make use of installed gitweb and installed
>> gitweb files, but do not change organization of generated files; just
>> instead of gitweb.cgi there should be gitweb_config.perl with
>> appropriate configuration to show current repository.  And of course
>> there would be no gitweb files in $GIT_DIR/gitweb (in .git/gitweb)
>> directory.
> 
> Ok. Sure I will do it.

Thanks in advance.
-- 
Jakub Narebski
Poland

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Pavan Kumar Sunkara]

>
> Well, I can understand that.
>
> There are two options how to resolve this issue without adding yet
> another script (although on the other hand git-web-gui / git-client
> could share code with git-instaweb just like git-difftool and
> git-mergetool do).
>
> First is to leave git-instaweb similar to how it is now, with pid file,
> server config file, gitweb config file, etc. in $GIT_DIR/gitweb, but
> if it is invoked outside any git repository, start it in "repository
> administration" mode, i.e. on the page that allows one to create new
> repository or clone repository.

But this solution requires starting of many apache servers on many
ports which is quite complicated and even messier.

> The alternate solution would be to follow the idea implemented in this
> patch, namely per-user pid file, gitweb config file, server config file
> etc. and the *list of projects* file in $HOME/.gitweb (or whenever
> XDG / FHS / LSB says it should be named), _but_ add an easy way to add
> a new project (a new repositoey) to list.

The feature *Adding repository to client* in my project proposal will
take care of this.

>  Perhaps even make
> 'git instaweb', when run from inside git repository, add automatically
> current repository to list (unless it is present there already), and
> perhaps open 'summary' page for said repository.

Yeah, we can do that but I think I will do it in another commit.

> But independently on which solution would be chosen, it should take place
> in a separate commit,

So, I will implement the second solution but in seperate commit. First
I will take care of this patch :)

Thanks
- Pavan

Re: [PATCH GSoC 2/3] git-instaweb: Configure it to work with a global server root and projects list

[Jakub Narebski]

On Wed, 19 May 2010, Pavan Kumar Sunkara wrote:
> On Wed, 19 May 2010 01:05:57 +0200, Jakub Narebski wrote:

> > Well, I can understand that.
> >
> > There are two options how to resolve this issue without adding yet
> > another script (although on the other hand git-web-gui / git-client
> > could share code with git-instaweb just like git-difftool and
> > git-mergetool do).
> >
> > First is to leave git-instaweb similar to how it is now, with pid file,
> > server config file, gitweb config file, etc. in $GIT_DIR/gitweb, but
> > if it is invoked outside any git repository, start it in "repository
> > administration" mode, i.e. on the page that allows one to create new
> > repository or clone repository.
> 
> But this solution requires starting of many apache servers on many
> ports which is quite complicated and even messier.

I agree with that.  I guess that git-instaweb was created mainly for the
situation where you work in single repository, and does not support well
situation where you move from repository to repository, and run git-instaweb
in different repositories.


As I wrote earlier:

   Now, current git-instaweb behavior has its quirks, but having
   git-instaweb show _current_ repository is a very important feature,
   and I'd rather we didn't lose it in transition.

> > The alternate solution would be to follow the idea implemented in this
> > patch, namely per-user pid file, gitweb config file, server config file
> > etc. and the *list of projects* file in $HOME/.gitweb (or whenever
> > XDG / FHS / LSB says it should be named), _but_ add an easy way to add
> > a new project (a new repositoey) to list.
> 
> The feature *Adding repository to client* in my project proposal will
> take care of this.

That's good, but...

> >  Perhaps even make
> > 'git instaweb', when run from inside git repository, add automatically
> > current repository to list (unless it is present there already), and
> > perhaps open 'summary' page for said repository.
> 
> Yeah, we can do that but I think I will do it in another commit.

...this is really needed, in my opinion.

I agree that it should be in separate commit.

-- 
Jakub Narebski
Poland