Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

[Junio C Hamano <gitster@pobox.com>]

Jakub Narebski <jnareb@gmail.com> 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.


> For example RedHat has split it out to a separate RPM perl-Time-HiRes.
>
> Noticed-by: Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no>
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Jakub Narębski <jnareb@gmail.com>
> ---
>  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()?
>  
>  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.