{"thread":{"id":"26155","subject":"[PATCH 0/4] git-instaweb: Three fixes and one improvement","startedAt":"2010-12-29T16:43:40Z","lastAt":"2010-12-30T06:23:29Z","messageCount":9,"participants":["Jakub Narebski","Junio C Hamano","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"158713","messageId":"201012291743.41213.jnareb@gmail.com","threadId":"26155","inReplyTo":null,"subject":"[PATCH 0/4] git-instaweb: Three fixes and one improvement","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-29T16:43:40Z","receivedAt":"2010-12-29T16:43:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Actually only the first patch is a proper bugfix for issue noticed by\nTadeusz Sośnierz ('tadzik' on github, https://github.com/tadzik) in\nGitHub private message; second and third are hardening git-instaweb\nagainst differences in configuration and environment.\n\nJunio, could you at least apply the first one before 1.7.4? This is a\nfix for a real bug, if for not often used \"git instaweb \n--httpd=plackup\".\n\ntadzik, does the first patch fixes the issue you noticed?\n\nJakub Narebski (4):\n git-instaweb: Fix issue with static files for 'plackup' server\n git-instaweb: Static files are under \"static/\" in gitweb_conf\n git-instaweb: Add checks that web server can be started and is\n started\n git-instaweb: Use \"git-instaweb\" as sitename (for page titles)\n\n git-instaweb.sh | 13 +++++++++++--\n 1 files changed, 11 insertions(+), 2 deletions(-)\n\n-- \nJakub Narębski\nPoland\n"},{"id":"158714","messageId":"201012291747.01288.jnareb@gmail.com","threadId":"26155","inReplyTo":"201012291743.41213.jnareb@gmail.com","subject":"[BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-29T16:47:00Z","receivedAt":"2010-12-29T16:47:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The default (in gitweb/Makefile) is to use relative paths for gitweb\nstatic files, e.g. \"static/gitweb.css\" for GITWEB_CSS. But the\nconfiguration for Plack::Middleware::Static in plackup_conf assumed\nthat static files must be absolute paths starting with \"/gitweb/\"\nprefix which had to be stripped, e.g. \"/gitweb/static/gitweb.css\".\nThis in turn caused web server run by \"git instaweb --httpd=plackup\"\nto not access static files (e.g. CSS) correctly.\n\nThis is a minimal fixup, making 'plackup' web server in git-instaweb\nwork with default gitweb build configuration.\n\nReported-by: Tadeusz Sośnierz \nSigned-off-by: Jakub Narebski \n---\nThe regexp is probably too strict: qr{^/static/} should be enough,\nbut I didn't want to change too much at once.\n\nThis bug was not noticed because we don't have any test for\ngit-instaweb, not mentioning tests for all web servers supported. And\nthe fact that I was checking \"git instaweb -httpd=plackup\" against\ngitweb.cgi built with custom configuration (including the fact that\nGITWEB_CSS=\"/gitweb/static/gitweb.css\").\n\ntadzik, does that fix the issue you noticed?\n\nJunio, could you apply this before 1.7.4? Thanks in advance.\n\n git-instaweb.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex 10fcebb..bb57d81 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -549,7 +549,7 @@ my \\$app = builder {\n \t};\n \t# serve static files, i.e. stylesheet, images, script\n \tenable 'Static',\n-\t\tpath => sub { m!\\.(js|css|png)\\$! && s!^/gitweb/!! },\n+\t\tpath => qr{^/static/.*(?:js|css|png)\\$},\n \t\troot => \"$root/\",\n \t\tencoding => 'utf-8'; # encoding for 'text/plain' files\n \t# convert CGI application to PSGI app\n-- \n1.7.3\n"},{"id":"158715","messageId":"201012291748.14484.jnareb@gmail.com","threadId":"26155","inReplyTo":"201012291743.41213.jnareb@gmail.com","subject":"[PATCH 2/4] git-instaweb: Static files are under \"static/\" in gitweb_conf","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-29T16:48:14Z","receivedAt":"2010-12-29T16:48:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The variables used to compose links for gitweb static files,\ni.e. @stylesheets / $stylesheet, $logo, etc. are configurable via\nGITWEB_CSS, GITWEB_LOGO, etc. gitweb build configuration variables.\nThe installation directory of those static files is currently always\nset to $(gitwebdir)/static. Thus git-instaweb could rely on the place\nwhere those files are installed, but not on $logo, $favicon\netc. variables.\n\nSet @stylesheets, $logo, $favicon and $javascript to their default\nvalues in gitweb config file for git-instaweb (in gitweb_conf\nfunction), so that web servers invoked by git-instaweb (for example\nPlack::Middleware::Static component for 'plackup' web server) can\ncorrectly serve links to those static files.\n\nWe assume here that git-instaweb uses modern gitweb with names of\ngitweb config variables as mentioned above.\n\nNote that we had to take care that possible optional minimizations of\nstylesheet and of javascript is correctly taken into account.\n\n\nThis is continuation of fixup for issue noticed by Tadeusz Sośnierz\nwith --httpd=plackup and gitweb stylesheet (CSS).\n\nSigned-off-by: Jakub Narebski \n---\nThe subject (commit description) is not the best, but that is what I\nwas able to come up with.\n\nThis doesn't matter for default configuration and setup, but it allows\nme to use my custom gitweb build configuration without any changes in\nresult.\n\n git-instaweb.sh | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex bb57d81..da569f2 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -581,6 +581,11 @@ our \\$projectroot = \"$(dirname \"$fqgitdir\")\";\n our \\$git_temp = \"$fqgitdir/gitweb/tmp\";\n our \\$projects_list = \\$projectroot;\n \n+our @stylesheets = (\"static/\".basename(\\$stylesheets[0]));\n+our \\$logo = \"static/git-logo.png\";\n+our \\$favicon = \"static/git-favicon.png\";\n+our \\$javascript = \"static/\".basename(\\$javascript);\n+\n \\$feature{'remote_heads'}{'default'} = [1];\n EOF\n }\n-- \n1.7.3\n"},{"id":"158716","messageId":"201012291749.14772.jnareb@gmail.com","threadId":"26155","inReplyTo":"201012291743.41213.jnareb@gmail.com","subject":"[PATCH 3/4] git-instaweb: Add checks that web server can be started and is started","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-29T16:49:14Z","receivedAt":"2010-12-29T16:49:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Better to check that \"$root/gitweb.cgi\" exists, and that web server\nstarted, rather than have (for example) the following error message\n\n $ git instaweb --httpd=plackup --browser=mozilla\n Waiting for 'plackup' to start .../usr/share/gitweb/gitweb.cgi:\n No such file or directory at /perl5/CGI/Compile.pm line 97.\n ........\n\nand keep waiting in httpd_is_ready for gitweb.cgi which would not start.\n\nNote that the first check is not strictly necessary, but checking\nseparately that gitweb.cgi script can be found at expected place leads\nto better error message.\n\nSigned-off-by: Jakub Narebski \n---\nIt can be considered bugfix, or it can be considered hardening of\ngit-instaweb (making it more resilent).\n\n git-instaweb.sh | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex da569f2..94f8b07 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -41,6 +41,9 @@ test -z \"$root\" && root='@@GITWEBDIR@@'\n # any untaken local port will do...\n test -z \"$port\" && port=1234\n \n+# Sanity check\n+test -e \"$root/gitweb.cgi\" || die \"gitweb.cgi not found at '$root'\"\n+\n resolve_full_httpd () {\n \tcase \"$httpd\" in\n \t*apache2*|*lighttpd*|*httpd*)\n@@ -617,7 +620,7 @@ webrick)\n \t;;\n esac\n \n-start_httpd\n+start_httpd || die \"Could not start '$httpd'\"\n url=http://127.0.0.1:$port\n \n if test -n \"$browser\"; then\n-- \n1.7.3\n"},{"id":"158717","messageId":"201012291750.09610.jnareb@gmail.com","threadId":"26155","inReplyTo":"201012291743.41213.jnareb@gmail.com","subject":"[PATCH 4/4] git-instaweb: Use \"git-instaweb\" as sitename (for page titles)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-29T16:50:09Z","receivedAt":"2010-12-29T16:50:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The default is to use \"$ENV{'SERVER_NAME'} Git\" or \"Untitled Git\";\nuse \"git-instaweb\" instead.\n\nSigned-off-by: Jakub Narebski \n---\nThis is new feature, and doesn't need to be in 1.7.4\n\nIt would be an RFC, if not for the fact that it is such trivial change.\n\n git-instaweb.sh | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex 94f8b07..935515d 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -583,6 +583,7 @@ gitweb_conf() {\n our \\$projectroot = \"$(dirname \"$fqgitdir\")\";\n our \\$git_temp = \"$fqgitdir/gitweb/tmp\";\n our \\$projects_list = \\$projectroot;\n+our \\$site_name = \"git-instaweb\";\n \n our @stylesheets = (\"static/\".basename(\\$stylesheets[0]));\n our \\$logo = \"static/git-logo.png\";\n-- \n1.7.3\n"},{"id":"158737","messageId":"7v62ucrwm4.fsf@alter.siamese.dyndns.org","threadId":"26155","inReplyTo":"201012291747.01288.jnareb@gmail.com","subject":"Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-29T23:24:19Z","receivedAt":"2010-12-29T23:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> The default (in gitweb/Makefile) is to use relative paths for gitweb\n> static files, e.g. \"static/gitweb.css\" for GITWEB_CSS. But the\n> configuration for Plack::Middleware::Static in plackup_conf assumed\n> that static files must be absolute paths starting with \"/gitweb/\"\n> prefix which had to be stripped, e.g. \"/gitweb/static/gitweb.css\".\n> This in turn caused web server run by \"git instaweb --httpd=plackup\"\n> to not access static files (e.g. CSS) correctly.\n>\n> This is a minimal fixup, making 'plackup' web server in git-instaweb\n> work with default gitweb build configuration.\n>\n> Reported-by: Tadeusz Sośnierz \n> Signed-off-by: Jakub Narebski \n> ---\n> The regexp is probably too strict: qr{^/static/} should be enough,\n> but I didn't want to change too much at once.\n>\n> This bug was not noticed because we don't have any test for\n> git-instaweb, not mentioning tests for all web servers supported. And\n> the fact that I was checking \"git instaweb -httpd=plackup\" against\n> gitweb.cgi built with custom configuration (including the fact that\n> GITWEB_CSS=\"/gitweb/static/gitweb.css\").\n>\n> tadzik, does that fix the issue you noticed?\n>\n> Junio, could you apply this before 1.7.4?\n\nHmph, I was kind of hoping that I can issue 1.7.3.5 tonight...\n\n>\n> git-instaweb.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-instaweb.sh b/git-instaweb.sh\n> index 10fcebb..bb57d81 100755\n> --- a/git-instaweb.sh\n> +++ b/git-instaweb.sh\n> @@ -549,7 +549,7 @@ my \\$app = builder {\n> \t};\n> \t# serve static files, i.e. stylesheet, images, script\n> \tenable 'Static',\n> -\t\tpath => sub { m!\\.(js|css|png)\\$! && s!^/gitweb/!! },\n> +\t\tpath => qr{^/static/.*(?:js|css|png)\\$},\n> \t\troot => \"$root/\",\n> \t\tencoding => 'utf-8'; # encoding for 'text/plain' files\n> \t# convert CGI application to PSGI app\n> -- \n> 1.7.3\n"},{"id":"158738","messageId":"7v1v50rvat.fsf@alter.siamese.dyndns.org","threadId":"26155","inReplyTo":"201012291747.01288.jnareb@gmail.com","subject":"Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-29T23:52:42Z","receivedAt":"2010-12-29T23:52:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> The default (in gitweb/Makefile) is to use relative paths for gitweb\n> static files, e.g. \"static/gitweb.css\" for GITWEB_CSS. But the\n> configuration for Plack::Middleware::Static in plackup_conf assumed\n> that static files must be absolute paths starting with \"/gitweb/\"\n> prefix which had to be stripped, e.g. \"/gitweb/static/gitweb.css\".\n> This in turn caused web server run by \"git instaweb --httpd=plackup\"\n> to not access static files (e.g. CSS) correctly.\n>\n> This is a minimal fixup, making 'plackup' web server in git-instaweb\n> work with default gitweb build configuration.\n>\n> Reported-by: Tadeusz Sośnierz \n> Signed-off-by: Jakub Narebski \n> ---\n> The regexp is probably too strict: qr{^/static/} should be enough,\n> but I didn't want to change too much at once.\n>\n> This bug was not noticed because we don't have any test for\n> git-instaweb, not mentioning tests for all web servers supported. And\n> the fact that I was checking \"git instaweb -httpd=plackup\" against\n> gitweb.cgi built with custom configuration (including the fact that\n> GITWEB_CSS=\"/gitweb/static/gitweb.css\").\n>\n> tadzik, does that fix the issue you noticed?\n>\n> Junio, could you apply this before 1.7.4? Thanks in advance.\n>\n> git-instaweb.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-instaweb.sh b/git-instaweb.sh\n> index 10fcebb..bb57d81 100755\n> --- a/git-instaweb.sh\n> +++ b/git-instaweb.sh\n> @@ -549,7 +549,7 @@ my \\$app = builder {\n> \t};\n> \t# serve static files, i.e. stylesheet, images, script\n> \tenable 'Static',\n> -\t\tpath => sub { m!\\.(js|css|png)\\$! && s!^/gitweb/!! },\n> +\t\tpath => qr{^/static/.*(?:js|css|png)\\$},\n\nI wonder if you meant qr{^/static/.*\\\\.(?:js|css|png)\\$} here, to make\nsure that these three tokens are file suffixes, not just random\nsubstring.\n\n> \t\troot => \"$root/\",\n> \t\tencoding => 'utf-8'; # encoding for 'text/plain' files\n> \t# convert CGI application to PSGI app\n> -- \n> 1.7.3\n"},{"id":"158740","messageId":"201012300140.07843.jnareb@gmail.com","threadId":"26155","inReplyTo":"7v1v50rvat.fsf@alter.siamese.dyndns.org","subject":"Re: [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-30T00:40:06Z","receivedAt":"2010-12-30T00:40:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 30 Dec 2010, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > The default (in gitweb/Makefile) is to use relative paths for gitweb\n> > static files, e.g. \"static/gitweb.css\" for GITWEB_CSS. But the\n> > configuration for Plack::Middleware::Static in plackup_conf assumed\n> > that static files must be absolute paths starting with \"/gitweb/\"\n> > prefix which had to be stripped, e.g. \"/gitweb/static/gitweb.css\".\n> > This in turn caused web server run by \"git instaweb --httpd=plackup\"\n> > to not access static files (e.g. CSS) correctly.\n> >\n> > This is a minimal fixup, making 'plackup' web server in git-instaweb\n> > work with default gitweb build configuration.\n> >\n> > Reported-by: Tadeusz Sośnierz \n> > Signed-off-by: Jakub Narebski \n> > ---\n> > The regexp is probably too strict: qr{^/static/} should be enough,\n> > but I didn't want to change too much at once.\n[...]\n> > diff --git a/git-instaweb.sh b/git-instaweb.sh\n> > index 10fcebb..bb57d81 100755\n> > --- a/git-instaweb.sh\n> > +++ b/git-instaweb.sh\n> > @@ -549,7 +549,7 @@ my \\$app = builder {\n> > \t};\n> > \t# serve static files, i.e. stylesheet, images, script\n> > \tenable 'Static',\n> > -\t\tpath => sub { m!\\.(js|css|png)\\$! && s!^/gitweb/!! },\n> > +\t\tpath => qr{^/static/.*(?:js|css|png)\\$},\n> \n> I wonder if you meant qr{^/static/.*\\\\.(?:js|css|png)\\$} here, to make\n> sure that these three tokens are file suffixes, not just random\n> substring.\n\nYes, it should be qr{^/static/.*\\\\.(?:js|css|png)\\$} though as I said:\n\"The regexp is probably too strict: qr{^/static/} should be enough,\"\n\n-- \nJakub Narebski\nPoland\n"},{"id":"158749","messageId":"20101230062329.GD16090@dcvr.yhbt.net","threadId":"26155","inReplyTo":"201012291750.09610.jnareb@gmail.com","subject":"Re: [PATCH 4/4] git-instaweb: Use \"git-instaweb\" as sitename (for page titles)","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2010-12-30T06:23:29Z","receivedAt":"2010-12-30T06:23:29Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jakub Narebski wrote:\n> This is new feature, and doesn't need to be in 1.7.4\n> \n> It would be an RFC, if not for the fact that it is such trivial change.\n\nThanks Jakub. This series all looks reasonable to me (untested).\n"}]}