"John 'Warthog9' Hawley" <warthog9@xxxxxxxxxxxxxx> writes:
> This basically finishes the plumbing for caching the error pages
> as the are generated.
>
> If an error is hit, create a <hash>.err file with the error. This
> will interrupt all currently waiting processes and they will display
> the error, without any additional refreshing.
>
> On a new request a generation will be attempted, should it succed the
> <hash.err> file is removed (if it exists).
Could you split 17 and 18 patches slightly differently, at least not
using variables which were not declared first?
> Signed-off-by: John 'Warthog9' Hawley <warthog9@xxxxxxxxxxxxxx>
Hmmm... I certainly hope that this complication is not really needed.
I have trouble following code flow (no comments), so I'd try to do
fresh review again tomorrow.
> ---
> gitweb/gitweb.perl | 8 ++++++++
> gitweb/lib/cache.pl | 14 ++++++++++++++
> 2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d39982a..5a9660a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -41,6 +41,7 @@ our $version = "++GIT_VERSION++";
>
> # Output buffer variable
> our $output = "";
> +our $output_err = "";
It is used by earlier patches, and declared only there.
>
> our ($my_url, $my_uri, $base_url, $path_info, $home_link);
> sub evaluate_uri {
> @@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;
> our $fullhashbinpath = *STDOUT;
> our $fullhashbinpathfinal = *STDOUT;
>
> +our $cacheErrorCache = 0; # false
$capture_error_output, isn't it?
> +our $cacheErrorCount = 0;
$cached_error_count, or something like that, isn't it?
> +
> our $full_url;
> our $urlhash;
>
> @@ -3786,6 +3790,7 @@ sub die_error {
> # Reset the output so that we are actually going to STDOUT as opposed
> # to buffering the output.
> reset_output() if ($cache_enable && ! $cacheErrorCache);
> + $cacheErrorCount++ if( $cacheErrorCache );
Where it is decremented? A comment, if you please.
>
> git_header_html($http_responses{$status}, undef, %opts);
> print <<EOF;
> @@ -3801,6 +3806,9 @@ EOF
> print "</div>\n";
>
> git_footer_html();
> +
> + die_error_cache($output) if ( $cacheErrorCache );
> +
That's cache_die_error_output, or something like that, isn't it?
It's hard to review this patch when die_error_cache is defined in
separate (previous) patch.
> goto DONE_GITWEB
> unless ($opts{'-error_handler'});
> }
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 6cb82c8..2e7ca69 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -240,8 +240,14 @@ sub cacheUpdate {
> # Trap all output from the action
> change_output();
>
> + # Set the error handler so we cache
> + $cacheErrorCache = 1; # true
> +
> $actions{$action}->();
>
> + # Reset Error Handler to not cache
> + $cacheErrorCache = 0; # false
> +
> # Reset the outputs as we should be fine now
> reset_output();
>
> @@ -295,6 +301,8 @@ sub cacheUpdate {
> close($cacheFileBG);
> }
>
> + unlink("$fullhashpath.err") if (-e "$fullhashpath.err");
> +
> if ( $areForked ){
> exit(0);
> } else {
> @@ -339,6 +347,9 @@ sub cacheWaitForUpdate {
> my $max = 10;
> my $lockStat = 0;
>
> + # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
> + cacheDisplayErr($action);
> +
> if( $backgroundCache ){
> if( -e "$fullhashpath" ){
> open($cacheFile, '<:utf8', "$fullhashpath");
> @@ -402,6 +413,9 @@ EOF
> close($cacheFile);
> $x++;
> $combinedLockStat = $lockStat;
> +
> + # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
> + cacheDisplayErr($action);
> } while ((! $combinedLockStat) && ($x < $max));
> print <<EOF;
> </body>
> --
> 1.7.2.3
>
--
Jakub Narebski
Poland
ShadeHawk on #git
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html