Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling

"John 'Warthog9' Hawley" [off-list ref] 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???

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?

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