Jakub Narebski [off-list ref] writes:
Though Time::HiRes is a core Perl module, it doesn't necessarily mean
that it is included in 'perl' package, and that it is installed if
Perl is installed.
I do not think we have seen the end of Redhat/Fedora Perl saga. I am
hoping that either one of the two things to happen:
(1) Redhat/Fedora distrubution reconsiders the situation and fix their
packages so that by default when its users ask for "Perl" they get
what the upstream distributes as "Perl" in full, while still allowing
people who know what they are doing to install a minimum subset
"perl-base"; or
(2) Many applications that use and rely on Perl like we do are hit by
this issue, and Redhat/Fedora users are trained to install the
perl-full (or whatever it is called) package when applications want
"Perl".
In other words, I am hoping that "it doesn't necessarily mean" will not
stay true for a long time. So please hold onto this patch until the dust
settles, and resend it if (1) does not look to be happening in say 3
months.
quoted hunk
For example RedHat has split it out to a separate RPM perl-Time-HiRes.
Noticed-by: Hallvard Breien Furuseth [off-list ref]
Suggested-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Jakub Narębski <redacted>
---
gitweb/gitweb.perl | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..c86224a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -17,10 +17,12 @@ use Encode;
use Fcntl ':mode';
use File::Find qw();
use File::Basename qw(basename);
-use Time::HiRes qw(gettimeofday tv_interval);
binmode STDOUT, ':utf8';
-our $t0 = [ gettimeofday() ];
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+ $t0 = [Time::HiRes::gettimeofday()];
+}
our $number_of_git_cmds = 0;
Why should these even be initialized here? Doesn't reset_timer gets
called at the beginning of run_request()?
quoted hunk
BEGIN {@@ -1142,7 +1144,7 @@ sub dispatch {
}
sub reset_timer {
- our $t0 = [ gettimeofday() ]
+ our $t0 = [Time::HiRes::gettimeofday()]
if defined $t0;
our $number_of_git_cmds = 0;
The statement modifier look ugly.
More importantly, if you are not profiling, i.e. if we didn't initialize
$t0 at the beginning, do you need to reset $number_of_git_cmds at all?
I also think this should take gitweb_check_feature('timed') into
account, perhaps like this:
sub reset_timer {
return unless gitweb_check_feature('timed');
our $t0 = ...
our $number_of_git_cmds = 0;
}
Then all the other
if (defined $t0 && gitweb_check_feature('timed'))
can become
if (defined $t0)
If you go this route, even though tee-zero, the beginning of the time, is
a good name for the variable, you may want to rename it to avoid confusing
readers who might take it as a temporary variable #0.
On Fri, 27 Jan 2012, Junio C Hamano wrote:
Jakub Narebski [off-list ref] writes:
quoted
Though Time::HiRes is a core Perl module, it doesn't necessarily mean
that it is included in 'perl' package, and that it is installed if
Perl is installed.
I do not think we have seen the end of Redhat/Fedora Perl saga. I am
hoping that either one of the two things to happen:
(1) Redhat/Fedora distrubution reconsiders the situation and fix their
packages so that by default when its users ask for "Perl" they get
what the upstream distributes as "Perl" in full, while still allowing
people who know what they are doing to install a minimum subset
"perl-base"; or
(2) Many applications that use and rely on Perl like we do are hit by
this issue, and Redhat/Fedora users are trained to install the
perl-full (or whatever it is called) package when applications want
"Perl".
In other words, I am hoping that "it doesn't necessarily mean" will not
stay true for a long time. So please hold onto this patch until the dust
settles, and resend it if (1) does not look to be happening in say 3
months.
So for the time being (those "3 months") you would apply instead your
change to INSTALL (or equivalent to gitweb/INSTALL) mentioning Time::HiRes
issue, and perhaps also original patch by Hallvard skipping gitweb tests
if Time::HiRes is not available, isn't it?
quoted
For example RedHat has split it out to a separate RPM perl-Time-HiRes.
[...]
quoted
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..c86224a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -17,10 +17,12 @@ use Encode;
use Fcntl ':mode';
use File::Find qw();
use File::Basename qw(basename);
-use Time::HiRes qw(gettimeofday tv_interval);
binmode STDOUT, ':utf8';
-our $t0 = [ gettimeofday() ];
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+ $t0 = [Time::HiRes::gettimeofday()];
+}
our $number_of_git_cmds = 0;
Why should these even be initialized here? Doesn't reset_timer gets
called at the beginning of run_request()?
I think it predates adding reset_timer() to gitweb. Anyway $t0 has
to be set to something defined anyway to denote that Time::HiRes is
available... though if Time::HiRes is required unconditionally it would
not be really needed, and we can always check $INC{'Time/HiRes.pm'}
if it was loaded or not.
quoted
BEGIN {@@ -1142,7 +1144,7 @@ sub dispatch {
}
sub reset_timer {
- our $t0 = [ gettimeofday() ]
+ our $t0 = [Time::HiRes::gettimeofday()]
if defined $t0;
our $number_of_git_cmds = 0;
The statement modifier look ugly.
More importantly, if you are not profiling, i.e. if we didn't initialize
$t0 at the beginning, do you need to reset $number_of_git_cmds at all?
I also think this should take gitweb_check_feature('timed') into
account, perhaps like this:
sub reset_timer {
return unless gitweb_check_feature('timed');
our $t0 = ...
our $number_of_git_cmds = 0;
}
Then all the other
if (defined $t0 && gitweb_check_feature('timed'))
can become
if (defined $t0)
I think this is a good idea... though it would complicate applying revert
a bit ;-(
If you go this route, even though tee-zero, the beginning of the time, is
a good name for the variable, you may want to rename it to avoid confusing
readers who might take it as a temporary variable #0.
Good idea. $request_start_time perhaps? Or $time_start?
--
Jakub Narebski
Poland
On Fri, Jan 27, 2012 at 21:44, Junio C Hamano [off-list ref] wrote:
if (defined $t0)
If you go this route, even though tee-zero, the beginning of the
time, is a good name for the variable, you may want to rename it to
avoid confusing readers who might take it as a temporary variable
#0.
<trivia>
Personally I'd have written it as $START_TIME, but as a bit of Perl
trivia you might not realize $t0 is a commonly used and undestood
variable for dealing with a start time in Perl in the same way that
`i` is common for dealing with array indexes in C.
I.e. someone used to Perl will immediately think "oh that's the start
time" having seen it hundreds of times before, but someone not used to
Perl will go "what's this t-zero thing?".
Meanwhile some Lisp programmer is wondering what the hell "i" means in
your C for-loops, iterator? :)