"John 'Warthog9' Hawley" <warthog9@xxxxxxxxxxxxxx> writes:
> To quote myself from an e-mail of mine:
>
> I've got a hammer, it clearly solves all problems!
>
> This is the prepatory work to set up a mechanism inside the
> caching engine to cache the error pages instead of throwing
> them straight out to the client.
There is no problem with capturing output of die_error, nor there is a
problem with caching error pages (perhaps transiently in memory).
The problem is that subroutines calling die_error assum that it would
exit ending subroutine that is responsible for generating current
action; see "goto DONE_GITWEB" which should be "goto DONE_REQUEST",
and which was "exit 0" some time ago at the end of die_error().
With caching error pages you want die_error to exit $actions{$action}->(),
but not exit cache_fetch(). How do you intend to do it?
>
> This adds two functions:
>
> die_error_cache() - this gets back called from die_error() so
> that the error message generated can be cached.
*How* die_error_cache() gets called back from die_error()? I don't
see any changes to die_error(), or actually any calling sites for
die_error_cache() in the patch below.
> cacheDisplayErr() - this is a simplified version of cacheDisplay()
> that does an initial check, if the error page exists - display it
> and exit. If not, return.
Errr... isn't it removed in _preceding_ patch? WTF???
>
> Signed-off-by: John 'Warthog9' Hawley <warthog9@xxxxxxxxxxxxxx>
> ---
> gitweb/lib/cache.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index a8c902d..6cb82c8 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -302,6 +302,36 @@ sub cacheUpdate {
> }
> }
>
> +sub die_error_cache {
> + my ($output) = @_;
> +
> + open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
> + my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);
Why do you need to lock here? A comment would be nice.
> +
> + if (! $lockStatus ){
> + if ( $areForked ){
Grrrr...
But if it is here to stay, a comment if you please.
> + exit(0);
> + }else{
> + return;
> + }
> + }
> +
> + # Actually dump the output to the proper file handler
> + local $/ = undef;
> + $|++;
Why not
+ local $| = 1;
> + print $cacheFileErr "$output";
> + $|--;
> +
> + flock($cacheFileErr,LOCK_UN);
> + close($cacheFileErr);
Closing file will unlock it.
> +
> + if ( $areForked ){
> + exit(0);
> + }else{
> + return;
So die_error_cache would not actually work like "die" here and like
die_error(), isn't it?
> + }
> +}
> +
>
> sub cacheWaitForUpdate {
> my ($action) = @_;
> @@ -380,6 +410,28 @@ EOF
> return;
> }
>
> +sub cacheDisplayErr {
> +
> + return if ( ! -e "$fullhashpath.err" );
> +
> + open($cacheFileErr, '<:utf8', "$fullhashpath.err");
> + $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
> +
> + if (! $lockStatus ){
> + show_warning(
> + "<p>".
> + "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
esc_path
> + "This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
> + "<p>"
> + );
> + }
> +
> + while( <$cacheFileErr> ){
> + print $_;
> + }
Why not 'print <$cacheFileErr>' (list context), like in insert_file()
subroutine?
> + exit(0);
> +}
Callsites?
Note: I have't read next commit yet.
--
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