* Re: [PATCH] git-gui: fix browser with initial path
From: Bert Wesarg @ 2010-12-10 8:48 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Bert Wesarg, Shawn O. Pearce, git
In-Reply-To: <1290497870-28673-1-git-send-email-bert.wesarg@googlemail.com>
Ping.
On Tue, Nov 23, 2010 at 08:37, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> The path given to the browser does not end in a slash, which results in bad
> path given to blame and broke [Up To Parent]. Also the path was not
> escaped before displaying.
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> git-gui/lib/browser.tcl | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/git-gui/lib/browser.tcl b/git-gui/lib/browser.tcl
> index c241572..a88a68b 100644
> --- a/git-gui/lib/browser.tcl
> +++ b/git-gui/lib/browser.tcl
> @@ -26,8 +26,14 @@ constructor new {commit {path {}}} {
> wm withdraw $top
> wm title $top [append "[appname] ([reponame]): " [mc "File Browser"]]
>
> + if {$path ne {}} {
> + if {[string index $path end] ne {/}} {
> + append path /
> + }
> + }
> +
> set browser_commit $commit
> - set browser_path $browser_commit:$path
> + set browser_path "$browser_commit:[escape_path $path]"
>
> ${NS}::label $w.path \
> -textvariable @browser_path \
> --
> tg: (6f10c41..) bw/git-gui/fix-browser-up (depends on: master)
>
^ permalink raw reply
* Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
From: J.H. @ 2010-12-10 8:33 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3r5dqz9c5.fsf@localhost.localdomain>
> 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?
Well there's one bug in how that function ends in looking at it again,
basically the return case shouldn't happen, and that function should
end, like your suggesting in the first part of your question (with
respect to DONE_GITWEB)
In the second part, your not thinking with the fork() going (though in
thinking sans the fork this might not work right).
It's the background process that will call die_error in such a way that
die_error_cache will get invoked. die_error_cache will write the .err
file out, and the whole thing should just exit.
Though now that I say that there's an obvious bug in the case where
forking didn't work at all, in that case you would get a blank page as
the connection would just be closed. If you refreshed (say hitting F5)
you'd get the error at that point.
Need to fix that non-forked problem though.
>> 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???
in breaking up the series it got included in the wrong spot, and
apparently removed and re-added correctly, should be fixed in v9
>> +sub die_error_cache {
>> + my ($output) = @_;
>> +
>> + open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
>> + my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);
>
> Why do you need to lock here? A comment would be nice.
At any point when a write happens there's the potential for multiple
simultaneous writes. Locking becomes obvious, when your trying to
prevent multiple processes from writing to the same thing at the same
time...
>> +
>> + if (! $lockStatus ){
>> + if ( $areForked ){
>
> Grrrr...
>
> But if it is here to stay, a comment if you please.
>
>> + exit(0);
>> + }else{
>> + return;
>> + }
>> + }
The exit(0) or return have been removed in favor of DONE_GITWEB, as
we've already errored if we are broken here we should just die.
>> +
>> + # Actually dump the output to the proper file handler
>> + local $/ = undef;
>> + $|++;
>
> Why not
>
> + local $| = 1;
>
Done.
>
>> + print $cacheFileErr "$output";
>> + $|--;
>> +
>> + flock($cacheFileErr,LOCK_UN);
>> + close($cacheFileErr);
>
> Closing file will unlock it.
Doesn't really hurt to be explicit though.
>> +
>> + if ( $areForked ){
>> + exit(0);
>> + }else{
>> + return;
>
> So die_error_cache would not actually work like "die" here and like
> die_error(), isn't it?
that was ejected, it was a bug. DONE_GITWEB is more correct, though I
might need to add a hook to display the error message in the case that
the process didn't fork.
>> + }
>> +}
>> +
>>
>> sub cacheWaitForUpdate {
>> my ($action) = @_;
>> @@ -380,6 +410,28 @@ EOF
>> return;
>> }
>>
>> +sub cacheDisplayErr {
>> +
>> + return if ( ! -e "$fullhashpath.err" );
>> +
>> + open($cacheFileErr, '<:utf8', "$fullhashpath.err");
>> + $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
>> +
>> + if (! $lockStatus ){
>> + show_warning(
>> + "<p>".
>> + "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
>
> 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?
I've had buffer problems with 'print <$cacheFileErr>' in some cases.
This is small enough it shouldn't happen, but I've gotten into the habit
of doing it this way. I can change it if you like.
>
>> + exit(0);
>> +}
>
> Callsites?
>
> Note: I have't read next commit yet.
Next patch.
If you'd rather I can squash 17 & 18 into a single commit.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
From: J.H. @ 2010-12-10 7:38 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3zksezbkm.fsf@localhost.localdomain>
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.
>> +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
^ permalink raw reply
* Re: [PATCH 14/18] gitweb: add print_transient_header() function for central header printing
From: J.H. @ 2010-12-10 6:18 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m34oam1n3t.fsf@localhost.localdomain>
>> +sub print_transient_header {
>> + print $::cgi->header(
>
> Why you use $::cgi->header() instead of equivalent $cgi->header()?
> Note that $::cgi->header() is $main::cgi->header(), and is not
> CGI::header().
Because $main::cgi already was setup. Since I'm not redefining $cgi
anywhere they evaluate to the same thing since cgi is already a global
variable coming from gitweb itself.
The way I have it now is it's more explicit to being the parent (main).
It doesn't really matter either way, but I can change it if you like.
>> + -type=>'text/html',
>> + -charset => 'utf-8',
>> + -status=> 200,
>> + -expires => 'now',
>> + # HTTP/1.0
>> + -Pragma => 'no-cache',
>> + # HTTP/1.1
>> + -Cache_Control => join(
>> + ', ',
>> + qw(
>> + private
>> + no-cache
>> + no-store
>> + must-revalidate
>> + max-age=0
>> + pre-check=0
>> + post-check=0
>> + )
>> + )
>> + );
>> + return;
>> +}
>
> Why not use
>
> our %no_cache = (
> # HTTP/1.0
> -Pragma => 'no-cache',
> # HTTP/1.1
> -Cache_Control => join(', ', qw(private no-cache no-store must-revalidate
> max-age=0 pre-check=0 post-check=0)),
> );
>
> (or something like that). This way you can reuse it even if content
> type is different (e.g. 'text/plain').
>
> But that is just a proposal.
Finer grained control, though they have the same basic setup. Probably
will add that, though it's not that big of a deal.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 13/18] gitweb: Add commented url & url hash to page footer
From: J.H. @ 2010-12-10 6:10 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m38vzy1nkl.fsf@localhost.localdomain>
>> <!--
>> Full URL: |http://localhost/gitweb-caching/gitweb.cgi?p=/project.git;a=summary|
>> URL Hash: |7a31cfb8a43f5643679eec88aa9d7981|
>> -->
>
> Nice idea. It helps with debugging and doesn't introduce information
> leakage.
That was the plan, and I've already got a scenario where it would be useful.
>> The first bit tells you what the url that generated the page actually was, the second is
>> the hash used to store the file with the first two characters being used as the directory:
>>
>> <cachedir>/7a/31cfb8a43f5643679eec88aa9d7981
>
> Isn't it
>
> <cachedir>/7a/7a31cfb8a43f5643679eec88aa9d7981
>
> in your series?
Nope
our $fullhashdir = "$cachedir/". substr( $urlhash, 0, 2) ."/";
and then a couple of lines later:
$fullhashpath = "$fullhashdir/". substr( $urlhash, 2 );
right at the top of cache_fetch()
>> Also useful for greping through the existing cache and finding files with unique paths that
>> you may want to explicitly flush.
>
> Though probably better 'cache_admin' page would be ultimately best
> solution, see proof of concept in
The biggest problem with the cache admin page you've got there, is that
gitweb itself doesn't have a framework for user administration,
privileges, etc. Limiting it to the local machine is also useless,
there are very few people who are going to have access, from 127.0.0.1
to their web server, and this also breaks anything even remotely
resembling virtual hosts.
The fact that it's unusable from virtual hosts makes this pretty much DOA.
Like I've said in the past, we need to at least look at web frameworks
for gitweb, and if we want to provide things like the admin page than we
need to consider that we are going to need user management. That, in
particular, starts drifting towards needing a database to store things
in and I for one am *NOT* in favor of that.
I like the idea of a framework helping deal with things like page
layout, separating data access from content, etc. I do not like the
idea of gitweb having a full blown setup with a database and all behind it.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs
From: J.H. @ 2010-12-10 5:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, git
In-Reply-To: <7vwrnieac8.fsf@alter.siamese.dyndns.org>
On 12/09/2010 04:32 PM, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> +# Global declarations
>>> +our $cacheFile;
>>> +our $cacheFileBG;
>>> +our $cacheFileBinWT;
>>> +our $cacheFileBin;
>>
>> You are trading globs for global (well, package) variables. They are
>> not lexical filehandles... though I'm not sure if it would be possible
>> without restructuring code; note that if variable holding filehandle
>> falls out of scope, then file would be automatically closed.
>
> Hmm. why is it a bad idea, when you need to access these from practically
> everywhere, to use global variables to begin with? To a certain degree,
> it sounds like an unnecessary burden without much gain to me.
This was why I used globs in the first place.
To answer Jakub's question first: yes, some of those are holding locks
open while things are happening. Since there's locks open, through a
lot of functions, it's
Generally speaking I'm pretty good about opening and closing the files
when it's needed. I *THINK* they can be made local variables, as I
don't think anything is kept open (within the caching engine) across
functions.
I've made a couple of changes, I'm going to have to test them, this
might be fixed in v9.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [RFC] Implementing gitweb output caching - issues to solve
From: J.H. @ 2010-12-10 4:46 UTC (permalink / raw)
To: Olaf Alders
Cc: Jonathan Nieder, Jakub Narebski, git,
John 'Warthog9' Hawley, Junio C Hamano, Petr Baudis,
admin
In-Reply-To: <88CF82F1-0363-47B4-8C6F-AE4A2DA1714B@wundersolutions.com>
>> Interesting. http://www.user-agents.org/ seems to suggest that many
>> robots do use Mozilla (though I don't think it's worth bending over
>> backwards to help them see the page correctly).
If a robot reports itself and we don't know about it, I'm fine with
giving it the 'Generating...' page as opposed to what it's expecting.
The number of robots and things of that nature that won't handle the
meta refresh are fewer than the number of people who will be clicking
with eyeballs on a screen.
>> HTTP::BrowserDetect uses a blacklist as far as I can tell. Maybe in
>> the long term it would be nice to add a whitelist ->human() method.
>>
>> Cc-ing Olaf Alders for ideas.
>
> Thanks for including me in this. :) I'm certainly open to patching the module, but I'm not 100% clear on how you would want to implement this. How is ->is_human different from !->is_robot? To clarify, I should say that from the snippet above, I'm not 100% clear on what the problem is which needs to be solved.
At this point I don't really see an issue with HTTP::BrowserDetect's
robot() function, and I agree with human = !->is_robot.
One thing I would like to see is the ability to do some sort of an add
to the list of things to check for. As you are probably aware there are
more agents that exist than what you have setup, I'm moving forward and
handling it with the following:
sub is_dumb_client {
my($user_agent) = lc $ENV{'HTTP_USER_AGENT'};
my $browser_detect = HTTP::BrowserDetect->new($user_agent);
return 1 if ( $browser_detect->robot() );
foreach my $adc ( @additional_dumb_clients ) {
return 1 if ( index( $user_agent, lc $adc ) != -1 );
}
return 0;
}
which could be simplified if there was just some way to do
my($user_agent) = lc $ENV{'HTTP_USER_AGENT'};
my $browser_detect = HTTP::BrowserDetect->new($user_agent);
$browser_detect->add_robots( @array );
return 1 if ( $browser_detect->robot() );
Not sure that particularly generalizes, and honestly it's only 4 lines
of code to do add additional checks.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [RFC] Implementing gitweb output caching - issues to solve
From: Jonathan Nieder @ 2010-12-10 4:11 UTC (permalink / raw)
To: Olaf Alders
Cc: Jakub Narebski, J.H., git, John 'Warthog9' Hawley,
Junio C Hamano, Petr Baudis, admin
In-Reply-To: <88CF82F1-0363-47B4-8C6F-AE4A2DA1714B@wundersolutions.com>
Olaf Alders wrote:
> On 2010-12-09, at 5:52 PM, Jonathan Nieder wrote:
>> HTTP::BrowserDetect uses a blacklist as far as I can tell. Maybe in
>> the long term it would be nice to add a whitelist ->human() method.
>>
>> Cc-ing Olaf Alders for ideas.
>
> Thanks for including me in this. :) I'm certainly open to patching
> the module, but I'm not 100% clear on how you would want to
> implement this. How is ->is_human different from !->is_robot? To
> clarify, I should say that from the snippet above, I'm not 100%
> clear on what the problem is which needs to be solved.
Context (sorry I did not include this in the first place):
The caching code (in development) for git's web interface uses a page
that says "Generating..." for cache misses, with an http refresh
redirecting to the generated content. The big downside is that if
done naively this breaks wget, curl, and similar user agents that are
not patient enough to grab the actual content instead of the redirect
page.
The first solution tried was to explicitly special case wget and curl.
But in this case it is better to be more inclusive[2]; when in doubt,
leave out the nice "Generating..." page and just serve the actual
content slowly just in case.
In other words, the idea was that user agents fall into three
categories:
A. definitely will not replace content with target of HTTP refresh
B. definitely will replace content with target of HTTP refresh
C. unknown
and maybe ->is_robot could return true for A and ->is_human return
true for B (leaving C as !->is_human && !->is_robot). In this case,
we should show the "Generating..." page only in the ->is_human (B)
case.
That said, I know almost nothing on this subject, so it is likely
this analysis misses something. J.H. or Jakub can likely say more.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH 11/18] gitweb: add isDumbClient() check
From: J.H. @ 2010-12-10 4:00 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3hbem1o7a.fsf@localhost.localdomain>
>> + my($user_agent) = $ENV{'HTTP_USER_AGENT'};
>
> What if $ENV{'HTTP_USER_AGENT'} is unset / undef, e.g. because we are
> runing gitweb as a script... which includes running gitweb tests?
It can be disabled for the running of tests, but the default is to show
'Generating...' vs. not. I'd rather assume there's an intelligent
client on the other end and give users a reason why they aren't staring
at their initial content immediately (and thus thinking something is
broken).
>> +
>> + if(
>> + # wget case
>> + $user_agent =~ /^Wget/i
>> + ||
>> + # curl should be excluded I think, probably better safe than sorry
>> + $user_agent =~ /^curl/i
>> + ){
>> + return 1; # True
>> + }
>> +
>> + return 0;
>> +}
>
> Compare (note: handcrafted solution is to whitelist, not blacklist):
>
> +sub browser_is_robot {
> + return 1 if !exists $ENV{'HTTP_USER_AGENT'}; # gitweb run as script
> + if (eval { require HTTP::BrowserDetect; }) {
> + my $browser = HTTP::BrowserDetect->new();
> + return $browser->robot();
> + }
> + # fallback on detecting known web browsers
> + return 0 if ($ENV{'HTTP_USER_AGENT'} =~ /\b(?:Mozilla|Opera|Safari|IE)\b/);
> + # be conservative; if not sure, assume non-interactive
> + return 1;
> +}
My initial look indicated that perl-http-browserdetect wasn't available
for RHEL / CentOS 5 - it is however available in EPEL.
However there are a couple of things to note about User Agents at all:
- They lie... a lot
- Robots lie even more
Blacklisting is still the better option, by a lot. I'll re-work this
some in v9, as I'm fine with the added dependency.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type
From: J.H. @ 2010-12-10 3:39 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3lj3y1ogb.fsf@localhost.localdomain>
>> This is fairly self explanitory, these are here just to centralize the checking
>> for these types of actions, as special things need to be done with regards to
>> them inside the caching engine.
>>
>> isBinaryAction() returns true if the action deals with creating binary files
>> (this needing :raw output)
>
> Why do you need special case binary / :raw output? It is not really
> necessary if it is done in right way, as shown in my rewrite.
Because that's not how my caching engine does it, and the reason for
that is I am mimicking how the rest of gitweb does it.
I attempted at one point to do as you were suggesting, and it became too
cumbersome. I eventually broke out the 'binary' packages into a special
case (thus mimicking how gitweb is already doing things), which also
gives me the advantage of being able to checksum the resulting binary
out of band, as well as being able to more trivially calculate the file
size being sent.
>> isFeedAction() returns true if the action deals with a news feed of some sort,
>> basically used to bypass the 'Generating...' message should it be a news reader
>> as those will explode badly on that page.
>
> Why blacklisting 'feed', instead of whitelisting HTML-output?
There are a limited number of feed types and their ilk (standard xml
formatted feed and atom), there are lots of html-output like things.
Easier to default and have things work, generally, than to have things
not work the way you would expect.
> BTW., please don't use mixedCase names, but underline_separated.
fixed in v9
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [RFC] Implementing gitweb output caching - issues to solve
From: Olaf Alders @ 2010-12-10 3:17 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jakub Narebski, J.H., git, John 'Warthog9' Hawley,
Junio C Hamano, Petr Baudis, admin
In-Reply-To: <20101209225211.GA20426@burratino>
On 2010-12-09, at 5:52 PM, Jonathan Nieder wrote:
> Jakub Narebski wrote:
>
>> In my rewrite
>>
>> [PATCHv6 17/24] gitweb: Show appropriate "Generating..." page when regenerating cache
>> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163040
>> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/48679f7985ccda16dc54fda97790841bab4a0ba2#patch1
>>
>> (see the browser_is_robot() subroutine:
>>
>> http://repo.or.cz/w/git/jnareb-git.git/blob/48679f7985ccda16dc54fda97790841bab4a0ba2:/gitweb/gitweb.perl#l870
>>
>> I use HTTP::BrowserDetect package if available and it's ->robot() method.
>>
>> The fallback is to use *whitelist*, assuming that it would be better to
>> not show "Generating..." page rather than download the wrong thing.
>> I also guess that most (all?) web browsers use "Mozilla compatibile"
>> somewhere in their User-Agent string, thus matching 'Mozilla'.
>
> Interesting. http://www.user-agents.org/ seems to suggest that many
> robots do use Mozilla (though I don't think it's worth bending over
> backwards to help them see the page correctly).
>
> HTTP::BrowserDetect uses a blacklist as far as I can tell. Maybe in
> the long term it would be nice to add a whitelist ->human() method.
>
> Cc-ing Olaf Alders for ideas.
Thanks for including me in this. :) I'm certainly open to patching the module, but I'm not 100% clear on how you would want to implement this. How is ->is_human different from !->is_robot? To clarify, I should say that from the snippet above, I'm not 100% clear on what the problem is which needs to be solved.
Olaf
--
Olaf Alders
olaf@wundersolutions.com
http://www.wundersolutions.com
http://twitter.com/wundercounter
866 503 2204 (Toll free - North America)
416 944 8306 (direct)
^ permalink raw reply
* Re: [PATCH 09/18] gitweb: Revert reset_output() back to original code
From: J.H. @ 2010-12-10 2:43 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3pqta1ou3.fsf@localhost.localdomain>
>> Reverted change to reset_output as
>>
>> open STDOUT, ">&", \*STDOUT_REAL;
>
> For somebody not following our discussion the above would be very,
> very cryptic... though I suppose this would be squashed in final
> (ready to be merged in) version of the code.
>
>> causes assertion failures:
>>
>> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file "scalar.xs", line 49 at gitweb.cgi line 1221.
>
> It looks like bug in Perl, because it should give some kind of Perl
> error, not failed assertion from within guts of Perl C code.
>
> Which Perl version are you using?
This is perl, v5.10.0 built for x86_64-linux-thread-multi
>> if we encounter an error *BEFORE* we've ever changed the output.
>
> And how to reproduce this error (i.e. how did you found it)?
Cause an error to occur before the caching engine switches output, for
instance fail on creating the cache dir, or disable caching all together
and generate an error. I think the former is where I noticed it, it was
consistent though.
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled
From: J.H. @ 2010-12-10 2:38 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3y67y1psd.fsf@localhost.localdomain>
> Formally, there is no backward compatibility with any released code.
> Using out-of-tree patches is on one's own risk.
I will have to beg to differ with you on this, the entirety of the
existing caching engine has been released code for a number of years,
there are rpm packages available for it, at the very least, in Fedora
and in EPEL.
The caching engine *IS* released code, and this patchset is as much a
new feature as an attempt to merge a fork. Kernel.org isn't the only
one running this code, and that has been the case for several years now
already.
Claiming that this isn't released code is doing me a disservice to me,
and those who have submitted patches to it independent of git and the
mainline gitweb.
Thinking about the patch series outside of that context will lead to me
putting my foot down and arguing on those other users behalf. I'm not
keen on breaking them for no good reason, and I'm not seeing your change
here as one that's particularly worthwhile, while causing external
breakage for no reason.
> But even discarding that, I'd rather use the same solution as in
>
> [PATCHv6/RFC 22/24] gitweb: Support legacy options used by kernel.org caching engine
> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163058
> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/27ec67ad90ecd56ac3d05f6a9ea49b6faabf7d0a
>
> i.e.
>
> our $cache_enable;
>
> [...]
>
> # somewhere just before call to cache_fetch()
> $caching_enabled = !!$cache_enable if defined $cache_enable;
>
>>
>> This reverts back to the previous variable to enable / disable caching
Is there really any point in changing the name at all? The intention of
cache_enable, at one point, was to allow for other caching engines and
while there aren't any other caching engines that use it, it's already
treated identically to cache_enable.
If it really adds enough to the readability to the code, then I'm fine
with adding:
$caching_enabled = $cache_enable if defined $cache_enable;
But now you are setting up two variables that control the same thing,
adding the possibility for conflicts and confusion to end users.
I just want that stated.
Also, why the double negative in your original snippet - that doesn't
entirely make sense....
| cache_enable | caching_enabled
----------+-------------------+---------------------
enabled: | 1 | 1
disabled: | 0 | 0
doing a double negative like that doesn't really buy you much except
turning 0 into NULL or '' which is equivalent to 0...
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 18/18] gitweb: Add better error handling for gitweb caching
From: Jakub Narebski @ 2010-12-10 1:56 UTC (permalink / raw)
To: John 'Warthog9' Hawley; +Cc: git
In-Reply-To: <1291931844-28454-19-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
> This basically finishes the plumbing for caching the error pages
> as the are generated.
>
> If an error is hit, create a <hash>.err file with the error. This
> will interrupt all currently waiting processes and they will display
> the error, without any additional refreshing.
>
> On a new request a generation will be attempted, should it succed the
> <hash.err> file is removed (if it exists).
Could you split 17 and 18 patches slightly differently, at least not
using variables which were not declared first?
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
Hmmm... I certainly hope that this complication is not really needed.
I have trouble following code flow (no comments), so I'd try to do
fresh review again tomorrow.
> ---
> gitweb/gitweb.perl | 8 ++++++++
> gitweb/lib/cache.pl | 14 ++++++++++++++
> 2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d39982a..5a9660a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -41,6 +41,7 @@ our $version = "++GIT_VERSION++";
>
> # Output buffer variable
> our $output = "";
> +our $output_err = "";
It is used by earlier patches, and declared only there.
>
> our ($my_url, $my_uri, $base_url, $path_info, $home_link);
> sub evaluate_uri {
> @@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;
> our $fullhashbinpath = *STDOUT;
> our $fullhashbinpathfinal = *STDOUT;
>
> +our $cacheErrorCache = 0; # false
$capture_error_output, isn't it?
> +our $cacheErrorCount = 0;
$cached_error_count, or something like that, isn't it?
> +
> our $full_url;
> our $urlhash;
>
> @@ -3786,6 +3790,7 @@ sub die_error {
> # Reset the output so that we are actually going to STDOUT as opposed
> # to buffering the output.
> reset_output() if ($cache_enable && ! $cacheErrorCache);
> + $cacheErrorCount++ if( $cacheErrorCache );
Where it is decremented? A comment, if you please.
>
> git_header_html($http_responses{$status}, undef, %opts);
> print <<EOF;
> @@ -3801,6 +3806,9 @@ EOF
> print "</div>\n";
>
> git_footer_html();
> +
> + die_error_cache($output) if ( $cacheErrorCache );
> +
That's cache_die_error_output, or something like that, isn't it?
It's hard to review this patch when die_error_cache is defined in
separate (previous) patch.
> goto DONE_GITWEB
> unless ($opts{'-error_handler'});
> }
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 6cb82c8..2e7ca69 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -240,8 +240,14 @@ sub cacheUpdate {
> # Trap all output from the action
> change_output();
>
> + # Set the error handler so we cache
> + $cacheErrorCache = 1; # true
> +
> $actions{$action}->();
>
> + # Reset Error Handler to not cache
> + $cacheErrorCache = 0; # false
> +
> # Reset the outputs as we should be fine now
> reset_output();
>
> @@ -295,6 +301,8 @@ sub cacheUpdate {
> close($cacheFileBG);
> }
>
> + unlink("$fullhashpath.err") if (-e "$fullhashpath.err");
> +
> if ( $areForked ){
> exit(0);
> } else {
> @@ -339,6 +347,9 @@ sub cacheWaitForUpdate {
> my $max = 10;
> my $lockStat = 0;
>
> + # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
> + cacheDisplayErr($action);
> +
> if( $backgroundCache ){
> if( -e "$fullhashpath" ){
> open($cacheFile, '<:utf8', "$fullhashpath");
> @@ -402,6 +413,9 @@ EOF
> close($cacheFile);
> $x++;
> $combinedLockStat = $lockStat;
> +
> + # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
> + cacheDisplayErr($action);
> } while ((! $combinedLockStat) && ($x < $max));
> print <<EOF;
> </body>
> --
> 1.7.2.3
>
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 3/3] config: add core.sharedconfig
From: Nguyen Thai Ngoc Duy @ 2010-12-10 1:53 UTC (permalink / raw)
To: Thiago Farina; +Cc: git
In-Reply-To: <AANLkTinVw+H1OsdJVofiGMkpNAdOjS5n0-HZCGCg3f5g@mail.gmail.com>
2010/12/9 Thiago Farina <tfransosi@gmail.com>:
>> diff --git a/cache.h b/cache.h
>> index e83bc2d..e91ce35 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -559,6 +559,7 @@ extern int read_replace_refs;
>> extern int fsync_object_files;
>> extern int core_preload_index;
>> extern int core_apply_sparse_checkout;
>> +extern const char *core_shared_config;
>>
>
> Why you need to export this string? Isn't it used only in config.c?
>
git-prune should know about this but I haven't got that far.
--
Duy
^ permalink raw reply
* Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
From: Jakub Narebski @ 2010-12-10 1:49 UTC (permalink / raw)
To: John 'Warthog9' Hawley; +Cc: git, Jakub Narebski
In-Reply-To: <1291931844-28454-18-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> 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???
>
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
> gitweb/lib/cache.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index a8c902d..6cb82c8 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -302,6 +302,36 @@ sub cacheUpdate {
> }
> }
>
> +sub die_error_cache {
> + my ($output) = @_;
> +
> + open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
> + my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);
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?
> + }
> +}
> +
>
> sub cacheWaitForUpdate {
> my ($action) = @_;
> @@ -380,6 +410,28 @@ EOF
> return;
> }
>
> +sub cacheDisplayErr {
> +
> + return if ( ! -e "$fullhashpath.err" );
> +
> + open($cacheFileErr, '<:utf8', "$fullhashpath.err");
> + $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
> +
> + if (! $lockStatus ){
> + show_warning(
> + "<p>".
> + "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
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
^ permalink raw reply
* RE: Tonight's pushout
From: Neal Kreitzinger @ 2010-12-10 1:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git@vger.kernel.org
In-Reply-To: <20101209200143.GC6884@burratino>
Jonathan,
Thanks for pointing that out. I tried to preserve the cclist per your observation. My newsreader is outlook express and my mail client is outlook. As a result, I can only preserve the entire cc list on actual emails in my inbox. So in this case Junio was preserved in the cclist because I was able to reply-to-all in outlook (not outlook express). (But I took him out manually because I don't want to annoy him.) If I reply to a message on the newsgroup that was not explicitly emailed to me, then only the newsgroup is cc'd and any extra-newsgroup cc list is lost. I can see the sender's original cc list in outlook express in a header box at the top of the message preview pane, but I can't copy it (not even with hilite-copy). So in those cases I can only reply to the sender and the newsg
roup, not to the extra-newsgroup cc list. Outlook only allows:
"reply to newsgroup and not anyone else, not even the sender" (aka Reply-Group),
"reply only to the sender" (aka Reply),
"reply to the sender and cc the newsgroup only" (aka Reply-All)
There is no option to "reply to sender and preserve the sender's entire original cc list".
Maybe I need a better newsreader. I like how outlook express keeps the threads grouped together by the parent thread so I can expand the thread or collapse it. I don't like that can't preserve the extra-newgroup cclist. I appreciate your help as this is my first newsgroup I've ever posted to, and I apologize for any bad-newsgroup-etiquette due to my ignorance.
v/r,
Neal
-----Original Message-----
From: Jonathan Nieder [mailto:jrnieder@gmail.com]
Sent: Thursday, December 09, 2010 2:02 PM
To: Neal Kreitzinger
Cc: git@vger.kernel.org; Junio C Hamano
Subject: Re: Tonight's pushout
Neal Kreitzinger wrote:
> "prerelease freeze" is not in the git-workflows manpage. I'm interested in
> how you-all do this because I use the git-workflows mangpage to help me
> figure out my workflows. Can someone explain?
See http://en.wikipedia.org/wiki/Freeze_(software_engineering)
In git's incarnation of it, presumably the idea is that new features
do not get merged to 'master' (while they still would be merged to
'pu' and perhaps 'next').
See also <http://sites.google.com/site/maintnotes/> for some direct
discussion of the branches used by git.git.
Hope that helps,
Jonathan [1]
[1] Unrelated note: please try to preserve the cc list in replies
(i.e., "reply-to-all-by-mail" rather than "followup").
"This message may contain confidential and/or privileged
information. If you are not the addressee or authorized to
receive this for the addressee, you must not use, copy,
disclose, or take any action based on this message or any
information herein. If you have received this message in error,
please advise the sender immediately by reply e-mail and delete
this message. Thank you for your cooperation."
^ permalink raw reply
* Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well
From: Jakub Narebski @ 2010-12-10 1:36 UTC (permalink / raw)
To: John 'Warthog9' Hawley; +Cc: git, Jakub Narebski
In-Reply-To: <1291931844-28454-17-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
> This sets up a trap for STDERR as well as STDOUT. This should
> prevent any transient error messages from git itself percolating
> up to gitweb and outputting errant information before the HTTP
> header has been sent.
Hmm... anuthing that happens after 'use CGI::Carp;' is parsed should
have STDERR redirected to web server logs, see CGI::Carp manpage
[...]
use CGI::Carp
And the standard warn(), die (), croak(), confess() and carp() calls will
automagically be replaced with functions that write out nicely time-stamped
messages to the HTTP server error log.
[...]
REDIRECTING ERROR MESSAGES
By default, error messages are sent to STDERR. Most HTTPD servers direct
STDERR to the server's error log.
[...]
Especially the second part.
Could you give us example which causes described misbehaviour?
I have nothing against this patch: if you have to have it, then you
have to have it. I oly try to understand what might be core cause
behind the issue that this patch is to solve...
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
> gitweb/gitweb.perl | 22 +++++++++++++++++++++-
> gitweb/lib/cache.pl | 22 ----------------------
> 2 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7f8292e..d39982a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1214,6 +1214,10 @@ sub evaluate_argv {
> sub change_output {
> our $output;
>
> + #
> + # STDOUT
> + #
> +
> # Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
> open(STDOUT_REAL,">&STDOUT") or die "Unable to capture STDOUT $!\n";
> print STDOUT_REAL "";
> @@ -1223,12 +1227,28 @@ sub change_output {
>
> # Trap STDOUT to the $output variable, which is what I was using in the original
> # patch anyway.
> - open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $var
> + open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $output
> +
> + #
> + # STDERR
> + #
> +
> + # Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
> + open(STDERR_REAL,">&STDERR") or die "Unable to capture STDERR $!\n";
> + print STDERR_REAL "";
'print STDERR_REAL "";' nicely solves the spurious warning problem.
Nice.
> +
> + # Close STDOUT, so that it isn't being used anymore.
> + close STDERR;
> +
> + # Trap STDOUT to the $output variable, which is what I was using in the original
> + # patch anyway.
> + open(STDERR,">", \$output_err) || die "Unable to open STDERR: $!"; #open STDERR handle to use $output_err
Err... where $output_err is defined?
> }
>
> sub reset_output {
> # This basically takes STDOUT_REAL and puts it back as STDOUT
> open(STDOUT,">&STDOUT_REAL");
> + open(STDERR,">&STDERR_REAL");
> }
>
> sub run {
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 28e4240..a8c902d 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -380,28 +380,6 @@ EOF
> return;
> }
>
> -sub cacheDisplayErr {
> -
> - return if ( ! -e "$fullhashpath.err" );
> -
> - open($cacheFileErr, '<:utf8', "$fullhashpath.err");
> - $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
> -
> - if (! $lockStatus ){
> - show_warning(
> - "<p>".
> - "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
> - "This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
> - "<p>"
> - );
> - }
> -
> - while( <$cacheFileErr> ){
> - print $_;
> - }
> - exit(0);
> -}
Errr... in which patch it was added?
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] get_sha1: support relative path "<obj>:<sth>" syntax
From: Nguyen Thai Ngoc Duy @ 2010-12-10 1:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jnareb, dirson, kevin, peff
In-Reply-To: <7v7hfifx00.fsf@alter.siamese.dyndns.org>
2010/12/10 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I think it is just the matter of moving "if (startup-info)..." logic
>> outside the "is the :$path lacking an explicit stage number" block and
>> having it after that if/else, no?
>
> Like this, perhaps?
Yeah, looks good. Sorry I missed that else block.
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] config: add core.sharedconfig
From: Nguyen Thai Ngoc Duy @ 2010-12-10 1:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwrnig6gr.fsf@alter.siamese.dyndns.org>
2010/12/10 Junio C Hamano <gitster@pobox.com>:
>> @@ -910,6 +918,12 @@ int git_config(config_fn_t fn, void *data)
>>
>> repo_config = git_pathdup("config");
>> if (!access(repo_config, R_OK)) {
>> + git_config_from_file(get_shared_config, repo_config, NULL);
>> + if (core_shared_config) {
>> + ret += git_config_from_sha1(fn, core_shared_config, data);
>> + found += 1;
>> + }
>> +
>
> What is the point of this "found++" when you will increment it for finding
> the repository-local configuration anyway?
It seems that every time a config source is used, found++. So I
increase it because there's is another source used, a blob.
> I originally liked what the first two tried to do, but think about the use
> case. How would this whole thing work?
>
> - The user clones from the project to get a repository with a working
> tree;
>
> - The user somehow learns that s/he can run one command to get
> project-wide preference of the project:
>
> $ git config core.sharedconfig refs/remotes/origin/config:git.config
>
> - Everything hopefully should work the way project wishes in that blob,
> unless the end user later overrides them by adding different settings
> to .git/config.
>
> How is that different from:
>
> - The user clones from the project to get a repository with a working
> tree;
>
> - The user somehow learns that s/he can run one command to get
> project-wide preference of the project:
>
> $ ./setup-project-preference.sh
>
> Typically, such a ./setup-project-preference.sh script would only
> consist of a series of "git config $foo $bar", so any user who can say
> "git config core.sharedconfig $foo" should be able to use it as well.
>
> - Everything should work the way project wishes with the settings made to
> .git/config by the script, unless the end user later overrides them by
> modifying settings in .git/config.
The shared config can be updated automatically (after fetching, of
course). Remote setup for a small team, for example, can benefit from
this. Every time a new member joins, somebody adds new remote to the
shared config and pushes out. Everybody else will have it.
> One minor difference is that some configuration variables are additive,
> and you cannot subtract from them with your approach.
You can't subtract some keys from $HOME/.gitconfig by modifying
$GIT_DIR/config either. I mean, that could be addressed in a general
way, not specific to shared config.
--
Duy
^ permalink raw reply
* Re: [PATCH 00/18] Gitweb caching v8
From: Jakub Narebski @ 2010-12-10 1:27 UTC (permalink / raw)
To: J.H.; +Cc: git
In-Reply-To: <4D017796.4030506@eaglescrag.net>
On Fri, 10 Dec 2010, J.H. wrote:
> On 12/09/2010 03:26 PM, Jakub Narebski wrote:
>> John, could you please in the future Cc me? I am interested in gitweb
>> output caching development. Thanks in advance.
>
> Apologies, apparently screwed up on my git send-email line. I'll get
> that right one of these eons.
Ah, I can understand this.
>> I guess that in the final submission (i.e. the one that is to be
>> merged in into git.git repository) those changes would be squashed in,
>> isn't it?
>
> I have no objections to squashing the reversions into a single patch,
> just figured it was easier to break them out for the time being.
I guess that interdiff in comments would work as well, or almost as well...
>>> There is one additional script I've written that the Fedora folks are using,
>>> and that might be useful to include, which is an 'offline' cache file generator.
>>> It basically wraps gitweb.cgi and at the end moves the cache file into the right
>>> place. The Fedora folks were finding it took hours to generate their front
>>> page, and that doing a background generation almost never completed (due to
>>> process death). This was a simple way to handle that. If people would like
>>> I can add it in as an additional patch.
>>
>> Are you detaching the background process?
Errr... what I meant here is that perhaps detaching background process
would make it not die, but I am guessing here.
> No, in fact I completely turn off forking (using the $cacheDoFork variable.)
BTW. what I don't like is your code forking indiscriminately even if it
is not needed (e.g. background cache generation is turned off).
>
>> It would be nice to have it as separate patch.
>
> I can add it easily enough.
It is only about caching most IO intensive page, i.e. projects_list page,
isn't it? Why doesn't _it_ die, like background process?
>
>>> v8:
>>> - Reverting several changes from Jakub's change set that make no sense
>>> - is_cacheable changed to always return true - nothing special about
>>> blame or blame_incremental as far as the caching engine is concerned
>>
>> 'blame_incremental' is just another version of 'blame' view. I have
>> disabled it when caching is enabled in my rewrite (you instead disabled
>> caching for 'blame_incremental' in your v7 and mine v7.x) because I
>> couldn't get it to work together with caching. Did you check that it
>> works?
>
> blame works fine, blame_incremental generates but doesn't..... ohhhh
> someone added ajaxy kinda stuff and doesn't mention it anywhere.
Errr... I thought that the 'incremental' part is self-explaining that
it is Ajax-y stuff. Well, while commit is 4af819d (gitweb: Incremental
blame (using JavaScript), 2009-09-01), perhaps I should have added some
comment in the code.
>
> Exciting.
>
> blame_data needs to not get a 'generating...' page in all likelihood,
> generating a blame_incremental page, letting it load and then refreshing
> the whole thing gets me what I'm expecting.
Hmmm... I wonder why it didn't work for me at that time...
>
> Is enough to mask.
>
> Guess I'm looking at a v9 now.
>
>> Besides, withou "tee"-ing, i.e. printing output as it is captured,
>> cached 'blame_data' means that 'blame_incremental' is not incremental,
>> and therefore it vanishes its advantage over 'blame'.
I mean here that with current state of caching 'blame_incremental' stops
to be incremental...
> There are only 2 ways to get to a blame_incremental page
>
> 1) By going to a blame page and clicking on the incremental link in the nav
>
> 2) By enabling it by default so when you click 'blame' it goes to
> incremental first.
3) By having JavaScript add ';js=1' to all links, so clicking on
'blame' link (with action set to 'blame') would result in
'blame_incremental' view.
>
>> In the case data is in cache, then 'blame_inremental' doesn't have
>> advantage over 'blame' either.
>
> Agreed, though it's easy enough to support in the caching engine,
> basically don't return 'Generating...' and wait for that data to cache.
> Not really an advantage except that your not waiting for the whole
> generation to get a page back at all.
>
>>> - Reverted change to reset_output as
>>> open STDOUT, ">&", \*STDOUT_REAL;
>>> causes assertion failures:
>>> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file "scalar.xs", line 49 at gitweb.cgi line 1221.
>>> if we encounter an error *BEFORE* we've ever changed the output.
>>
>> Which Perl version are you using? Because I think you found error in Perl.
>> Well, at least I have not happen on this bug.
>
> This is perl, v5.10.0 built for x86_64-linux-thread-multi
Could you check with newer perl? I don't get this error.
>> I have nothing againts using
>>
>> open STDOUT, ">&STDOUT_REAL";
>>
>> though I really prefer that you used lexical filehandles, instead of
>> "globs" which are global variables.
And using 'print STDOUT_REAL "";' protects against spurious warning
(the warning is really wrong in this case).
>> The following works:
>>
>> open STDOUT, '>&', fileno($fh);
>>
>> Note that fileno(Symbol::qualify_to_ref($fh)) might be needed...
>
> I see 0 advantage to shifting around STDOUT and STDERR to a lexical
> filehandle vs. a glob in this case. STDOUT_REAL retains all the
> properties of STDOUT should it be needed elsewhere, including what it
> was going and what it was doing.
>
> I have no objection to shifting the file handles I'm using to lexical
> variables, if nothing else the argument about them closing when falling
> out of scope is worth it, but for STDOUT, STDERR, etc I don't think
> switching to lexicals makes a lot of sense
Well... I'd have to agree that in current case (capturing engine embedded
in gitweb, and gitweb-specific; no need for recursive capture) it would
be enough to use such globs.
>
>>> - Cleanups there were indirectly mentioned by Jakub
>>> - Elimination of anything even remotely looking like duplicate code
>>> - Creation of isBinaryAction() and isFeedAction()
>>
>> Could you please do not use mixedCase names?
>
> I'm fine with renaming those if you wish.
>
>> First, that is what %actions_info from
>>
>> [PATCH 16/24] gitweb: Introduce %actions_info, gathering information about actions
>> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163038
>> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/305a10339b33d56b4a50708d71e8f42453c8cb1f
>>
>> I have invented for.
>
> I have not based any of my caching engine, right now, on anything you've
> done for your rewrite.
What I meant here that if you will be doing yet another version, you
can take a look at it as a way to avoiding not very clear and nice
long alternatives in condition, or in regexp matched.
>
>> Second, why 'isBinaryAction()'? there isn't something inherently
>> different between binary (':raw') and text (':utf8') output, as I have
>> repeatedly said before.
>
> It's a binary action in that you are shoving something down the pipe
> with the intention of sending the bits completely raw. You read the
> data raw, and write the data raw. There is no interpretation of the
> data as being anything but straight raw.
>
> Right now, in gitweb already, there are two places that treat output
> completely differently:
>
> - snapshot
> - blob_plain
>
> The only reason isBinaryAction() (or any other function name or process
> you want to grant it) exists is so that I can figure out if it's one of
> those actions so I can deal with the cache and output handling
> differently for each.
>
> Yes, I could flip the entire caching engine over to following the same
> mantra for everything and thus there is no need to care, but gitweb
> itself isn't really setup to handle that separation cleanly right now,
> and I'm trying to make as few bigger changes right now as is.
Always reading from cache in ':raw' mode and always printing from cache
in ':raw' mode (i.e. setting STDOUT to ':raw' before printing / copying
cache entry) would be in gitweb case enough to not special-case binary
files.
In gitweb you always do "binmode STDOUT, ':raw';" _after_ starting capture,
which means that it gets applied to cache file; and gitweb always do
"binmode STDOUT, ':utf8';" before stopping capture.
If you print text data to file using ':utf8' layer (applied at beginning
to cache file) it is in this file as correct sequence of bytes. Therefore
you can dump said cache file to STDOUT in ':raw' mode (or in ':utf8' mode)
- both STDOUT and read cache file has to have the same mode.
>>> - Added better error handling
>>> - Creation of .err file in the cache directory
>>> - Trap STDERR output into $output_err as this was spewing data prior
>>> to any header information being sent
>>
>> Why it is needed? We capture output of "die" via CGI::Util::set_message,
>> and "warn" output is captured to web server logs... unless you explicitely
>> use "print STDERR <sth>" -- don't do that instead.
>
> I have seen, in several instances, a case where git itself will generate
> an error, it shoves it to STDERR which makes it to the client before
> anything else, thus causing 500 level errors.
>
> Added this so that STDERR got trapped and those messages didn't make it out.
Could you give examples when it happens? Anything that happens after
"use CGI::Carp" is parsed should have STDERR redirected to web server
errors log.
I'll read the actual patch and comment on it.
>
>>> - Added hidden field in footer for url & hash of url, which is extremely useful
>>> for debugging
>>
>> Nice idea, I'll see it. Can it be disabled (information leakage)?
>
> There's not really any information leakage per-se, unless you call
> md5suming the url information leakage.
Ah, sorry, I send this comment before actually reading patch in question.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
From: Jakub Narebski @ 2010-12-10 1:01 UTC (permalink / raw)
To: John 'Warthog9' Hawley; +Cc: git, Jakub Narebski
In-Reply-To: <1291931844-28454-16-git-send-email-warthog9@eaglescrag.net>
"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.
>
> This makes use of print_transient_header() which is also used in the
> 'Generating...' page. Currently the only warning it throws is about
> the cache needing to be created. If that fails it's a fatal error
> and we call die_error()
Why do you feel the need to single out this case giving it warning,
and single out this warning by showing warning page?
Nevertheless show_warning() _might_ be a good idea.
>
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
> gitweb/lib/cache.pl | 36 +++++++++++++++++++++++++++++++++---
> 1 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 723ae9b..28e4240 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -25,9 +25,13 @@ sub cache_fetch {
> my $cacheTime = 0;
>
> if(! -d $cachedir){
> - print "*** Warning ***: Caching enabled but cache directory does not exsist. ($cachedir)\n";
> - mkdir ("cache", 0755) || die "Cannot create cache dir - you will need to manually create";
> - print "Cache directory created successfully\n";
> + mkdir ("cache", 0755) || die_error(500, "Internal Server Error", "Cannot create cache dir () - you will need to manually create");
> + show_warning(
> + "<p>".
> + "<strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist. ($cachedir)<br/>/\n".
Minor nit: s/exsist/exist/
Don't you need to use esc_path() on $cachedir,
using either
...did not exist. (".esc_path($cachedir).")<br/>\n";
or using this trick
...did not exist. (@{[esc_path($cachedir)]})<br/>\n";
> + "Cache directory created successfully\n".
> + "<p>"
> + );
> }
>
> $full_url = "$my_url?". $ENV{'QUERY_STRING'};
> @@ -119,6 +123,32 @@ sub print_transient_header {
> return;
> }
>
> +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.
> +
> + my $warning = esc_html(shift) || "Unknown Warning";
> +
> + print_transient_header();
> +
> + print <<EOF;
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
> +<!-- git web w/caching interface version $version, (C) 2006-2010, John 'Warthog9' Hawley <warthog9\@kernel.org> -->
> +<!-- git core binaries version $git_version -->
> +<head>
> +<meta http-equiv="content-type" content="$content_type; charset=utf-8"/>
$content_type is not defined here.
> +<meta name="generator" content="gitweb/$version git/$git_version"/>
> +<meta name="robots" content="index, nofollow"/>
It is "noindex, nofollow", isn't it?
> +<meta http-equiv="refresh" content="10"/>
Why 10 seconds?
> +<title>$title</title>
$title is not defined here.
> +</head>
> +<body>
> +$warning
> +</body>
> +</html>
> +EOF
> + exit(0);
"exit(0)" and not "goto DONE_GITWEB", or "goto DONE_REQUEST"?
> +}
> +
> sub isBinaryAction {
> my ($action) = @_;
Didn't you ran gitweb tests?
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs
From: Jakub Narebski @ 2010-12-10 0:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John 'Warthog9' Hawley, git
In-Reply-To: <7vwrnieac8.fsf@alter.siamese.dyndns.org>
On Fri, 10 Dec 2010, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> "John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
>>>
>>> +# Global declarations
>>> +our $cacheFile;
>>> +our $cacheFileBG;
>>> +our $cacheFileBinWT;
>>> +our $cacheFileBin;
>>
>> You are trading globs for global (well, package) variables. They are
>> not lexical filehandles... though I'm not sure if it would be possible
>> without restructuring code; note that if variable holding filehandle
>> falls out of scope, then file would be automatically closed.
>
> Hmm. why is it a bad idea, when you need to access these from practically
> everywhere, to use global variables to begin with? To a certain degree,
> it sounds like an unnecessary burden without much gain to me.
If you check my rewrite of gitweb output caching:
"[PATCHv6/RFC 00/24] gitweb: Simple file based output caching"
http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/origin..refs/heads/gitweb/cache-kernel-v6
https://github.com/jnareb/git/compare/origin...gitweb/cache-kernel-v6
you would see that I always use lexical filehandles, and I never need
to use global variables / glob filehandles.
http://en.wikipedia.org/wiki/Global_variables
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 00/18] Gitweb caching v8
From: J.H. @ 2010-12-10 0:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsjy6ea0k.fsf@alter.siamese.dyndns.org>
On 12/09/2010 04:39 PM, Junio C Hamano wrote:
> It seems that t950X tests do not like me. I am getting these in gitweb.body:
>
> <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
> <!-- git web w/caching interface version current, (C) 2006-2010, John 'Warthog9' Hawley <warthog9@kernel.org> -->
> <!-- git core binaries version 1.7.3.3.494.gcf92e -->
> <head>
> <meta http-equiv="content-type" content="; charset=utf-8"/>
> <meta name="generator" content="gitweb/current git/1.7.3.3.494.gcf92e"/>
> <meta name="robots" content="index, nofollow"/>
> <meta http-equiv="refresh" content="10"/>
> <title></title>
> </head>
> <body>
> <p><strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist. (cache)<br/>/<span class="cntrl">\n</span>Cache directory created successfully<span class="cntrl">\n</span><p>
> </body>
> </html>
caching directory didn't exist prior to running the test, and so it
throws the warning that it needed to create it. The cache directory
should likely be created before caching tests.
(Sorry, didn't catch that one as I've already got my caching directory
created, and it doesn't really get deleted)
- John 'Warthog9' Hawley
^ permalink raw reply
* Re: [PATCH 00/18] Gitweb caching v8
From: J.H. @ 2010-12-10 0:43 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3bp4u34vj.fsf@localhost.localdomain>
On 12/09/2010 03:26 PM, Jakub Narebski wrote:
> John, could you please in the future Cc me? I am interested in gitweb
> output caching development. Thanks in advance.
Apologies, apparently screwed up on my git send-email line. I'll get
that right one of these eons.
> Could you please rebase it on top of v7.2 version? The v7.2 patch
> series contained a few bugs that needs to be corrected.
I assume you mean 7.4, as opposed to 7.2... otherwise already done!
> I guess that in the final submission (i.e. the one that is to be
> merged in into git.git repository) those changes would be squashed in,
> isn't it?
I have no objections to squashing the reversions into a single patch,
just figured it was easier to break them out for the time being.
>> This still differs, by two patches, from whats in production on kernel.org.
>> It's missing the index page git:// link, and kernel.org and kernel.org also
>
>>
>> has the forced version matching. As a note I'll probably let this stew
>> another day or so on kernel.org and then I'll push it into the Fedora update
>> stream, as there's a couple of things in this patch series that would be
>> good for them to have.
>
> There was some discussion about git:// link in the past; nevertheless
> this issue is independent on gitweb caching and can (and should) be
> sent as a aeparate patch.
>
> IIRC we agreed that because of backward compatibility forced versions
> match is quite useless (in general)...
The former wasn't submitted as that is a separate issue, the later was
not agreed on really but mostly me retracting the patches as they
weren't making any headway.
I mention the patches at all as clarification of what's actually running
on kernel.org, and eventually what will be in the gitweb-caching
packages that are part of Fedora and EPEL.
>> There is one additional script I've written that the Fedora folks are using,
>> and that might be useful to include, which is an 'offline' cache file generator.
>> It basically wraps gitweb.cgi and at the end moves the cache file into the right
>> place. The Fedora folks were finding it took hours to generate their front
>> page, and that doing a background generation almost never completed (due to
>> process death). This was a simple way to handle that. If people would like
>> I can add it in as an additional patch.
>
> Are you detaching the background process?
No, in fact I completely turn off forking (using the $cacheDoFork variable.)
> It would be nice to have it as separate patch.
I can add it easily enough.
>> v8:
>> - Reverting several changes from Jakub's change set that make no sense
>> - is_cacheable changed to always return true - nothing special about
>> blame or blame_incremental as far as the caching engine is concerned
>
> 'blame_incremental' is just another version of 'blame' view. I have
> disabled it when caching is enabled in my rewrite (you instead disabled
> caching for 'blame_incremental' in your v7 and mine v7.x) because I
> couldn't get it to work together with caching. Did you check that it
> works?
blame works fine, blame_incremental generates but doesn't..... ohhhh
someone added ajaxy kinda stuff and doesn't mention it anywhere.
Exciting.
blame_data needs to not get a 'generating...' page in all likelihood,
generating a blame_incremental page, letting it load and then refreshing
the whole thing gets me what I'm expecting.
Is enough to mask.
Guess I'm looking at a v9 now.
> Besides, withou "tee"-ing, i.e. printing output as it is captured,
> cached 'blame_data' means that 'blame_incremental' is not incremental,
> and therefore it vanishes its advantage over 'blame'.
There are only 2 ways to get to a blame_incremental page
1) By going to a blame page and clicking on the incremental link in the nav
2) By enabling it by default so when you click 'blame' it goes to
incremental first.
> In the case data is in cache, then 'blame_inremental' doesn't have
> advantage over 'blame' either.
Agreed, though it's easy enough to support in the caching engine,
basically don't return 'Generating...' and wait for that data to cache.
Not really an advantage except that your not waiting for the whole
generation to get a page back at all.
>> - Reverted change to reset_output as
>> open STDOUT, ">&", \*STDOUT_REAL;
>> causes assertion failures:
>> Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file "scalar.xs", line 49 at gitweb.cgi line 1221.
>> if we encounter an error *BEFORE* we've ever changed the output.
>
> Which Perl version are you using? Because I think you found error in Perl.
> Well, at least I have not happen on this bug.
This is perl, v5.10.0 built for x86_64-linux-thread-multi
> I have nothing againts using
>
> open STDOUT, ">&STDOUT_REAL";
>
> though I really prefer that you used lexical filehandles, instead of
> "globs" which are global variables.
>
> The following works:
>
> open STDOUT, '>&', fileno($fh);
>
> Note that fileno(Symbol::qualify_to_ref($fh)) might be needed...
I see 0 advantage to shifting around STDOUT and STDERR to a lexical
filehandle vs. a glob in this case. STDOUT_REAL retains all the
properties of STDOUT should it be needed elsewhere, including what it
was going and what it was doing.
I have no objection to shifting the file handles I'm using to lexical
variables, if nothing else the argument about them closing when falling
out of scope is worth it, but for STDOUT, STDERR, etc I don't think
switching to lexicals makes a lot of sense
>> - Cleanups there were indirectly mentioned by Jakub
>> - Elimination of anything even remotely looking like duplicate code
>> - Creation of isBinaryAction() and isFeedAction()
>
> Could you please do not use mixedCase names?
I'm fine with renaming those if you wish.
> First, that is what %actions_info from
>
> [PATCH 16/24] gitweb: Introduce %actions_info, gathering information about actions
> http://thread.gmane.org/gmane.comp.version-control.git/163052/focus=163038
> http://repo.or.cz/w/git/jnareb-git.git/commitdiff/305a10339b33d56b4a50708d71e8f42453c8cb1f
>
> I have invented for.
I have not based any of my caching engine, right now, on anything you've
done for your rewrite.
> Second, why 'isBinaryAction()'? there isn't something inherently
> different between binary (':raw') and text (':utf8') output, as I have
> repeatedly said before.
It's a binary action in that you are shoving something down the pipe
with the intention of sending the bits completely raw. You read the
data raw, and write the data raw. There is no interpretation of the
data as being anything but straight raw.
Right now, in gitweb already, there are two places that treat output
completely differently:
- snapshot
- blob_plain
The only reason isBinaryAction() (or any other function name or process
you want to grant it) exists is so that I can figure out if it's one of
those actions so I can deal with the cache and output handling
differently for each.
Yes, I could flip the entire caching engine over to following the same
mantra for everything and thus there is no need to care, but gitweb
itself isn't really setup to handle that separation cleanly right now,
and I'm trying to make as few bigger changes right now as is.
>> - Added better error handling
>> - Creation of .err file in the cache directory
>> - Trap STDERR output into $output_err as this was spewing data prior
>> to any header information being sent
>
> Why it is needed? We capture output of "die" via CGI::Util::set_message,
> and "warn" output is captured to web server logs... unless you explicitely
> use "print STDERR <sth>" -- don't do that instead.
I have seen, in several instances, a case where git itself will generate
an error, it shoves it to STDERR which makes it to the client before
anything else, thus causing 500 level errors.
Added this so that STDERR got trapped and those messages didn't make it out.
>> - Added hidden field in footer for url & hash of url, which is extremely useful
>> for debugging
>
> Nice idea, I'll see it. Can it be disabled (information leakage)?
There's not really any information leakage per-se, unless you call
md5suming the url information leakage.
- John 'Warthog9' Hawley
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox