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

From: Jakub Narebski <[email-masked]>
Date: Fri, 10 Dec 2010 15:10:15 +0100

On Fri, 10 Dec 2010, J.H. wrote:
> 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.

Well, the contents and feel of show_warning() is more like die_error()
rather than "Generating..." page, so I feel that if die_error() conforms
to style of rest of gitweb pages, then show_warning() should too.
 
>>> +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?

Well, with exit(0) it is a moot point... but it is good habit to localize
punctation variables ($|, $/,)
 
>>> +<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.

A comment if you please, then?
 
Hmmm... I guess there is no ned to make it configurable.

>>> +</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,

Errr... DONE_REQUEST was introduced in

  [PATCH/RFC] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error
  Message-ID: <[email-masked]>
  http://permalink.gmane.org/gmane.comp.version-control.git/162156

> 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)

You would have to do more than that.  ModPerl::Registry that is used
for mod_perl support (which as deployment is I guess more widespread
than PSGI via wrapper using Plack::App::WrapCGI, or FastCGI deployment)
redefines 'exit' so that CGI scripts that use 'exit' to end request
keep working without need to restart worker at each request; for real
exit, for example from background process, you need to use CORE::exit.
See e.g. http://repo.or.cz/w/git/jnareb-git.git/commitdiff/8bd99a6d37cc
the ->_set_maybe_background() method.

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

That would be a pity.  In my rewrite I tried to take into acount both
non-persistent (plain CGI, running as script) and persistent (mod_perl,
FastCGI, PSGI) web environments.

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

Hmmm... I wonder why new tests in t9502 and t9503 didn't pass for me...

P.S. I'll write separate email about problems with die_error, die-ing
and output caching.

-- 
Jakub Narebski
Poland