Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 10, 2010, 01:01 UTC
- 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.
Show 5 quoted lines
> > 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.
Show 21 quoted lines
>
> 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";Show 12 quoted lines
> + "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.
Show 11 quoted lines
> + > + 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.
Show 7 quoted lines
> +</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