From: Jakub Narebski <jnareb@gmail.com>
To: "J.H." <warthog9@eaglescrag.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
Date: Fri, 10 Dec 2010 15:10:15 +0100 [thread overview]
Message-ID: <201012101510.16504.jnareb@gmail.com> (raw)
In-Reply-To: <4D01D902.1030102@eaglescrag.net>
On Fri, 10 Dec 2010, J.H. wrote:
> On 12/09/2010 05:01 PM, Jakub Narebski wrote:
>> "John 'Warthog9' Hawley" <warthog9@eaglescrag.net> 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: <1290723308-21685-1-git-send-email-jnareb@gmail.com>
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
next prev parent reply other threads:[~2010-12-10 14:10 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-09 21:57 [PATCH 00/18] Gitweb caching v8 John 'Warthog9' Hawley
2010-12-09 21:57 ` [PATCH 01/18] gitweb: Prepare for splitting gitweb John 'Warthog9' Hawley
2010-12-09 23:30 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 02/18] gitweb: add output buffering and associated functions John 'Warthog9' Hawley
2010-12-09 21:57 ` [PATCH 03/18] gitweb: File based caching layer (from git.kernel.org) John 'Warthog9' Hawley
2010-12-09 21:57 ` [PATCH 04/18] gitweb: Minimal testing of gitweb caching John 'Warthog9' Hawley
2010-12-09 21:57 ` [PATCH 05/18] gitweb: Regression fix concerning binary output of files John 'Warthog9' Hawley
2010-12-09 23:33 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 06/18] gitweb: Add more explicit means of disabling 'Generating...' page John 'Warthog9' Hawley
2010-12-09 21:57 ` [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled John 'Warthog9' Hawley
2010-12-09 23:38 ` Jakub Narebski
2010-12-10 2:38 ` J.H.
2010-12-10 13:48 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 08/18] gitweb: Change is_cacheable() to return true always John 'Warthog9' Hawley
2010-12-09 23:46 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 09/18] gitweb: Revert reset_output() back to original code John 'Warthog9' Hawley
2010-12-09 23:58 ` Jakub Narebski
2010-12-10 2:43 ` J.H.
2010-12-09 21:57 ` [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type John 'Warthog9' Hawley
2010-12-10 0:06 ` Jakub Narebski
2010-12-10 3:39 ` J.H.
2010-12-10 12:10 ` Jakub Narebski
2010-12-10 12:25 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 11/18] gitweb: add isDumbClient() check John 'Warthog9' Hawley
2010-12-10 0:12 ` Jakub Narebski
2010-12-10 4:00 ` J.H.
2010-12-11 0:07 ` Junio C Hamano
2010-12-11 0:15 ` Jakub Narebski
2010-12-11 1:15 ` J.H.
2010-12-11 1:40 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs John 'Warthog9' Hawley
2010-12-10 0:16 ` Jakub Narebski
2010-12-10 0:32 ` Junio C Hamano
2010-12-10 0:47 ` Jakub Narebski
2010-12-10 5:56 ` J.H.
2010-12-09 21:57 ` [PATCH 13/18] gitweb: Add commented url & url hash to page footer John 'Warthog9' Hawley
2010-12-10 0:26 ` Jakub Narebski
2010-12-10 6:10 ` J.H.
2010-12-09 21:57 ` [PATCH 14/18] gitweb: add print_transient_header() function for central header printing John 'Warthog9' Hawley
2010-12-10 0:36 ` Jakub Narebski
2010-12-10 6:18 ` J.H.
2010-12-09 21:57 ` [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh John 'Warthog9' Hawley
2010-12-10 1:01 ` Jakub Narebski
2010-12-10 7:38 ` J.H.
2010-12-10 14:10 ` Jakub Narebski [this message]
2010-12-09 21:57 ` [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well John 'Warthog9' Hawley
2010-12-10 1:36 ` Jakub Narebski
2010-12-12 5:25 ` J.H.
2010-12-12 15:17 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling John 'Warthog9' Hawley
2010-12-10 1:49 ` Jakub Narebski
2010-12-10 8:33 ` J.H.
2010-12-10 20:33 ` Jakub Narebski
2010-12-09 21:57 ` [PATCH 18/18] gitweb: Add better error handling for gitweb caching John 'Warthog9' Hawley
2010-12-10 1:56 ` Jakub Narebski
2010-12-09 23:26 ` [PATCH 00/18] Gitweb caching v8 Jakub Narebski
2010-12-10 0:43 ` J.H.
2010-12-10 1:27 ` Jakub Narebski
2010-12-10 0:39 ` Junio C Hamano
2010-12-10 0:45 ` J.H.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201012101510.16504.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
--cc=warthog9@eaglescrag.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).