Subject: Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh

From: "J.H." <[email-masked]>
Date: Thu, 09 Dec 2010 23:38:42 -0800

On 12/09/2010 05:01 PM, Jakub Narebski wrote:
> "John 'Warthog9' Hawley" <[email-masked]> 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.

The only other 'transient' style page, the 'Generating...' page doesn't
use it, and I felt that since this was also transient, and only (likely)
to be seen once it wasn't worth the header & footer.

That said I've added it back in, in v9.

>> +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.

Since the execution exits immediately after, wouldn't FastCGI reset at
that point, since execution of that thread has stopped?  Or does FastCGI
retain everything as is across subsequent executions of a process?

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

Long enough to see the error, but not too long to be a nuisance.  Mainly
just there to warn the admin that it did something automatic they may
not have been expecting.

>> +</head>
>> +<body>
>> +$warning
>> +</body>
>> +</html>
>> +EOF
>> +	exit(0);
> 
> "exit(0)" and not "goto DONE_GITWEB", or "goto DONE_REQUEST"?

DONE_REQUEST doesn't actually exist as a label, the exit was used
partially for my lack of love for goto's, but mostly out of not
realizing what that was calling back to (mainly for the excitement of
things like PSGI and their ilk)

I will change that that, but considering there are other locations where
I do explicit exit's and those are actually inherent to the way the
caching engine currently works, I might need to go take a look at what's
going on with respect to multi-threaded items inside of PSGI and their
like.  It's possible the caching engine doesn't actually work on those...

>> +}
>> +
>>  sub isBinaryAction {
>>  	my ($action) = @_;
> 
> Didn't you ran gitweb tests?

I did, they passed for me - for whatever reason my cache dir wasn't
cleaned up, and stayed resident once it was created.

- John 'Warthog9' Hawley