[PATCH 0/4] git-instaweb: Three fixes and one improvement

[PATCH 0/4] git-instaweb: Three fixes and one improvement

[Jakub Narebski]

Actually only the first patch is a proper bugfix for issue noticed by
Tadeusz Sośnierz ('tadzik' on github, https://github.com/tadzik) in
GitHub private message; second and third are hardening git-instaweb
against differences in configuration and environment.

Junio, could you at least apply the first one before 1.7.4?  This is a
fix for a real bug, if for not often used "git instaweb 
--httpd=plackup".

tadzik, does the first patch fixes the issue you noticed?

Jakub Narebski (4):
  git-instaweb: Fix issue with static files for 'plackup' server
  git-instaweb: Static files are under "static/" in gitweb_conf
  git-instaweb: Add checks that web server can be started and is
    started
  git-instaweb: Use "git-instaweb" as sitename (for page titles)

 git-instaweb.sh |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

-- 
Jakub Narębski
Poland

[BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server

[Jakub Narebski]

The default (in gitweb/Makefile) is to use relative paths for gitweb
static files, e.g. "static/gitweb.css" for GITWEB_CSS.  But the
configuration for Plack::Middleware::Static in plackup_conf assumed
that static files must be absolute paths starting with "/gitweb/"
prefix which had to be stripped, e.g. "/gitweb/static/gitweb.css".
This in turn caused web server run by "git instaweb --httpd=plackup"
to not access static files (e.g. CSS) correctly.

This is a minimal fixup, making 'plackup' web server in git-instaweb
work with default gitweb build configuration.

Reported-by: Tadeusz Sośnierz <tadzikes@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The regexp is probably too strict: qr{^/static/} should be enough,
but I didn't want to change too much at once.

This bug was not noticed because we don't have any test for
git-instaweb, not mentioning tests for all web servers supported.  And
the fact that I was checking "git instaweb -httpd=plackup" against
gitweb.cgi built with custom configuration (including the fact that
GITWEB_CSS="/gitweb/static/gitweb.css").

tadzik, does that fix the issue you noticed?

Junio, could you apply this before 1.7.4?  Thanks in advance.

 git-instaweb.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 10fcebb..bb57d81 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -549,7 +549,7 @@ my \$app = builder {
 	};
 	# serve static files, i.e. stylesheet, images, script
 	enable 'Static',
-		path => sub { m!\.(js|css|png)\$! && s!^/gitweb/!! },
+		path => qr{^/static/.*(?:js|css|png)\$},
 		root => "$root/",
 		encoding => 'utf-8'; # encoding for 'text/plain' files
 	# convert CGI application to PSGI app
-- 
1.7.3

[PATCH 2/4] git-instaweb: Static files are under "static/" in gitweb_conf

[Jakub Narebski]

The variables used to compose links for gitweb static files,
i.e. @stylesheets / $stylesheet, $logo, etc. are configurable via
GITWEB_CSS, GITWEB_LOGO, etc. gitweb build configuration variables.
The installation directory of those static files is currently always
set to $(gitwebdir)/static.  Thus git-instaweb could rely on the place
where those files are installed, but not on $logo, $favicon
etc. variables.

Set @stylesheets, $logo, $favicon and $javascript to their default
values in gitweb config file for git-instaweb (in gitweb_conf
function), so that web servers invoked by git-instaweb (for example
Plack::Middleware::Static component for 'plackup' web server) can
correctly serve links to those static files.

We assume here that git-instaweb uses modern gitweb with names of
gitweb config variables as mentioned above.

Note that we had to take care that possible optional minimizations of
stylesheet and of javascript is correctly taken into account.


This is continuation of fixup for issue noticed by Tadeusz Sośnierz
with --httpd=plackup and gitweb stylesheet (CSS).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The subject (commit description) is not the best, but that is what I
was able to come up with.

This doesn't matter for default configuration and setup, but it allows
me to use my custom gitweb build configuration without any changes in
result.

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

diff --git a/git-instaweb.sh b/git-instaweb.sh
index bb57d81..da569f2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -581,6 +581,11 @@ our \$projectroot = "$(dirname "$fqgitdir")";
 our \$git_temp = "$fqgitdir/gitweb/tmp";
 our \$projects_list = \$projectroot;
 
+our @stylesheets = ("static/".basename(\$stylesheets[0]));
+our \$logo = "static/git-logo.png";
+our \$favicon = "static/git-favicon.png";
+our \$javascript = "static/".basename(\$javascript);
+
 \$feature{'remote_heads'}{'default'} = [1];
 EOF
 }
-- 
1.7.3

[PATCH 3/4] git-instaweb: Add checks that web server can be started and is started

[Jakub Narebski]

Better to check that "$root/gitweb.cgi" exists, and that web server
started, rather than have (for example) the following error message

  $ git instaweb --httpd=plackup --browser=mozilla
  Waiting for 'plackup' to start .../usr/share/gitweb/gitweb.cgi:
  No such file or directory at <somewhere>/perl5/CGI/Compile.pm line 97.
  ........

and keep waiting in httpd_is_ready for gitweb.cgi which would not start.

Note that the first check is not strictly necessary, but checking
separately that gitweb.cgi script can be found at expected place leads
to better error message.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
It can be considered bugfix, or it can be considered hardening of
git-instaweb (making it more resilent).

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

diff --git a/git-instaweb.sh b/git-instaweb.sh
index da569f2..94f8b07 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -41,6 +41,9 @@ test -z "$root" && root='@@GITWEBDIR@@'
 # any untaken local port will do...
 test -z "$port" && port=1234
 
+# Sanity check
+test -e "$root/gitweb.cgi" || die "gitweb.cgi not found at '$root'"
+
 resolve_full_httpd () {
 	case "$httpd" in
 	*apache2*|*lighttpd*|*httpd*)
@@ -617,7 +620,7 @@ webrick)
 	;;
 esac
 
-start_httpd
+start_httpd || die "Could not start '$httpd'"
 url=http://127.0.0.1:$port
 
 if test -n "$browser"; then
-- 
1.7.3

[PATCH 4/4] git-instaweb: Use "git-instaweb" as sitename (for page titles)

[Jakub Narebski]

The default is to use "$ENV{'SERVER_NAME'} Git" or "Untitled Git";
use "git-instaweb" instead.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is new feature, and doesn't need to be in 1.7.4

It would be an RFC, if not for the fact that it is such trivial change.

 git-instaweb.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 94f8b07..935515d 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -583,6 +583,7 @@ gitweb_conf() {
 our \$projectroot = "$(dirname "$fqgitdir")";
 our \$git_temp = "$fqgitdir/gitweb/tmp";
 our \$projects_list = \$projectroot;
+our \$site_name = "git-instaweb";
 
 our @stylesheets = ("static/".basename(\$stylesheets[0]));
 our \$logo = "static/git-logo.png";
-- 
1.7.3

Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The default (in gitweb/Makefile) is to use relative paths for gitweb
> static files, e.g. "static/gitweb.css" for GITWEB_CSS.  But the
> configuration for Plack::Middleware::Static in plackup_conf assumed
> that static files must be absolute paths starting with "/gitweb/"
> prefix which had to be stripped, e.g. "/gitweb/static/gitweb.css".
> This in turn caused web server run by "git instaweb --httpd=plackup"
> to not access static files (e.g. CSS) correctly.
>
> This is a minimal fixup, making 'plackup' web server in git-instaweb
> work with default gitweb build configuration.
>
> Reported-by: Tadeusz Sośnierz <tadzikes@gmail.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> The regexp is probably too strict: qr{^/static/} should be enough,
> but I didn't want to change too much at once.
>
> This bug was not noticed because we don't have any test for
> git-instaweb, not mentioning tests for all web servers supported.  And
> the fact that I was checking "git instaweb -httpd=plackup" against
> gitweb.cgi built with custom configuration (including the fact that
> GITWEB_CSS="/gitweb/static/gitweb.css").
>
> tadzik, does that fix the issue you noticed?
>
> Junio, could you apply this before 1.7.4?

Hmph, I was kind of hoping that I can issue 1.7.3.5 tonight...

>
>  git-instaweb.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 10fcebb..bb57d81 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -549,7 +549,7 @@ my \$app = builder {
>  	};
>  	# serve static files, i.e. stylesheet, images, script
>  	enable 'Static',
> -		path => sub { m!\.(js|css|png)\$! && s!^/gitweb/!! },
> +		path => qr{^/static/.*(?:js|css|png)\$},
>  		root => "$root/",
>  		encoding => 'utf-8'; # encoding for 'text/plain' files
>  	# convert CGI application to PSGI app
> -- 
> 1.7.3

Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The default (in gitweb/Makefile) is to use relative paths for gitweb
> static files, e.g. "static/gitweb.css" for GITWEB_CSS.  But the
> configuration for Plack::Middleware::Static in plackup_conf assumed
> that static files must be absolute paths starting with "/gitweb/"
> prefix which had to be stripped, e.g. "/gitweb/static/gitweb.css".
> This in turn caused web server run by "git instaweb --httpd=plackup"
> to not access static files (e.g. CSS) correctly.
>
> This is a minimal fixup, making 'plackup' web server in git-instaweb
> work with default gitweb build configuration.
>
> Reported-by: Tadeusz Sośnierz <tadzikes@gmail.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> The regexp is probably too strict: qr{^/static/} should be enough,
> but I didn't want to change too much at once.
>
> This bug was not noticed because we don't have any test for
> git-instaweb, not mentioning tests for all web servers supported.  And
> the fact that I was checking "git instaweb -httpd=plackup" against
> gitweb.cgi built with custom configuration (including the fact that
> GITWEB_CSS="/gitweb/static/gitweb.css").
>
> tadzik, does that fix the issue you noticed?
>
> Junio, could you apply this before 1.7.4?  Thanks in advance.
>
>  git-instaweb.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 10fcebb..bb57d81 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -549,7 +549,7 @@ my \$app = builder {
>  	};
>  	# serve static files, i.e. stylesheet, images, script
>  	enable 'Static',
> -		path => sub { m!\.(js|css|png)\$! && s!^/gitweb/!! },
> +		path => qr{^/static/.*(?:js|css|png)\$},

I wonder if you meant qr{^/static/.*\\.(?:js|css|png)\$} here, to make
sure that these three tokens are file suffixes, not just random
substring.

>  		root => "$root/",
>  		encoding => 'utf-8'; # encoding for 'text/plain' files
>  	# convert CGI application to PSGI app
> -- 
> 1.7.3

Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server

[Jakub Narebski]

On Thu, 30 Dec 2010, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > The default (in gitweb/Makefile) is to use relative paths for gitweb
> > static files, e.g. "static/gitweb.css" for GITWEB_CSS.  But the
> > configuration for Plack::Middleware::Static in plackup_conf assumed
> > that static files must be absolute paths starting with "/gitweb/"
> > prefix which had to be stripped, e.g. "/gitweb/static/gitweb.css".
> > This in turn caused web server run by "git instaweb --httpd=plackup"
> > to not access static files (e.g. CSS) correctly.
> >
> > This is a minimal fixup, making 'plackup' web server in git-instaweb
> > work with default gitweb build configuration.
> >
> > Reported-by: Tadeusz Sośnierz <tadzikes@gmail.com>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > The regexp is probably too strict: qr{^/static/} should be enough,
> > but I didn't want to change too much at once.
[...]
> > diff --git a/git-instaweb.sh b/git-instaweb.sh
> > index 10fcebb..bb57d81 100755
> > --- a/git-instaweb.sh
> > +++ b/git-instaweb.sh
> > @@ -549,7 +549,7 @@ my \$app = builder {
> >  	};
> >  	# serve static files, i.e. stylesheet, images, script
> >  	enable 'Static',
> > -		path => sub { m!\.(js|css|png)\$! && s!^/gitweb/!! },
> > +		path => qr{^/static/.*(?:js|css|png)\$},
> 
> I wonder if you meant qr{^/static/.*\\.(?:js|css|png)\$} here, to make
> sure that these three tokens are file suffixes, not just random
> substring.

Yes, it should be qr{^/static/.*\\.(?:js|css|png)\$} though as I said:
"The regexp is probably too strict: qr{^/static/} should be enough,"

-- 
Jakub Narebski
Poland

Re: [PATCH 4/4] git-instaweb: Use "git-instaweb" as sitename (for page titles)

[Eric Wong]

Jakub Narebski <jnareb@gmail.com> wrote:
> This is new feature, and doesn't need to be in 1.7.4
> 
> It would be an RFC, if not for the fact that it is such trivial change.

Thanks Jakub.  This series all looks reasonable to me (untested).