Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:50:13
On Fri, 10 Dec 2010, J.H. wrote:
On 12/09/2010 05:01 PM, Jakub Narebski wrote:quoted
"John 'Warthog9' Hawley" [off-list ref] writes:quoted
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.
quoted
quoted
+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 ($|, $/,)
quoted
quoted
+<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.
quoted
quoted
+</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: [ref] 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.
quoted
quoted
+} + 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