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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2012, 20:44 UTC
Message-ID: <7vty3gzxhs.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201201271845.39576.jnareb@gmail.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.

Show 26 quoted lines

> 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()?

Show 10 quoted lines

>  
>  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.