From: Jakub Narebski <jnareb@gmail.com>
Date: Fri, 10 Dec 2010 01:01:05 GMT
Subject: Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
Message-ID: <m3zksezbkm.fsf@localhost.localdomain>
In-Reply-To: <1291931844-28454-16-git-send-email-warthog9@eaglescrag.net>

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> die_error() is an immediate and abrupt action.  show_warning() more or less
> functions identically, except that the page generated doesn't use the
> gitweb header or footer (in case they are broken) and has an auto-refresh
> (10 seconds) built into it.

Why not use gitweb header/footer?  If they are broken, it should be
caught in git development.  If we don't se them, the show_warning()
output would look out of place.

> 
> This makes use of print_transient_header() which is also used in the
> 'Generating...' page.  Currently the only warning it throws is about
> the cache needing to be created.  If that fails it's a fatal error
> and we call die_error()

Why do you feel the need to single out this case giving it warning,
and single out this warning by showing warning page?

Nevertheless show_warning() _might_ be a good idea.

> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
>  gitweb/lib/cache.pl |   36 +++++++++++++++++++++++++++++++++---
>  1 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 723ae9b..28e4240 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -25,9 +25,13 @@ sub cache_fetch {
>  	my $cacheTime = 0;
>  
>  	if(! -d $cachedir){
> -		print "*** Warning ***: Caching enabled but cache directory does not exsist.  ($cachedir)\n";
> -		mkdir ("cache", 0755) || die "Cannot create cache dir - you will need to manually create";
> -		print "Cache directory created successfully\n";
> +		mkdir ("cache", 0755) || die_error(500, "Internal Server Error", "Cannot create cache dir () - you will need to manually create");
> +		show_warning(
> +				"<p>".
> +				"<strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist.  ($cachedir)<br/>/\n".

Minor nit: s/exsist/exist/

Don't you need to use esc_path() on $cachedir, 
using either

  ...did not exist.  (".esc_path($cachedir).")<br/>\n";

or using this trick

  ...did not exist.  (@{[esc_path($cachedir)]})<br/>\n";

> +				"Cache directory created successfully\n".
> +				"<p>"
> +				);
>  	}
>  
>  	$full_url = "$my_url?". $ENV{'QUERY_STRING'};
> @@ -119,6 +123,32 @@ sub print_transient_header {
>  	return;
>  }
>  
> +sub show_warning {
> +	$| = 1;

  +	local $| = 1;

$| is global variable, and otherwise you would turn autoflush for all
code, which would matter e.g. for FastCGI.

> +
> +	my $warning = esc_html(shift) || "Unknown Warning";
> +
> +	print_transient_header();
> +
> +	print <<EOF;
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
> +<!-- git web w/caching interface version $version, (C) 2006-2010, John 'Warthog9' Hawley <warthog9\@kernel.org> -->
> +<!-- git core binaries version $git_version -->
> +<head>
> +<meta http-equiv="content-type" content="$content_type; charset=utf-8"/>

$content_type is not defined here.

> +<meta name="generator" content="gitweb/$version git/$git_version"/>
> +<meta name="robots" content="index, nofollow"/>

It is "noindex, nofollow", isn't it?

> +<meta http-equiv="refresh" content="10"/>

Why 10 seconds?

> +<title>$title</title>

$title is not defined here.

> +</head>
> +<body>
> +$warning
> +</body>
> +</html>
> +EOF
> +	exit(0);

"exit(0)" and not "goto DONE_GITWEB", or "goto DONE_REQUEST"?

> +}
> +
>  sub isBinaryAction {
>  	my ($action) = @_;

Didn't you ran gitweb tests?

-- 
Jakub Narebski
Poland
ShadeHawk on #git