{"thread":{"id":"29427","subject":"Test t9500 fails if Time::HiRes is missing","startedAt":"2012-01-23T04:50:23Z","lastAt":"2012-01-29T02:29:09Z","messageCount":13,"participants":["Hallvard Breien Furuseth","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jakub Narębski","Hallvard B Furuseth","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182951","messageId":"hbf.20120123rqzg@bombur.uio.no","threadId":"29427","inReplyTo":null,"subject":"Test t9500 fails if Time::HiRes is missing","fromName":"Hallvard Breien Furuseth","fromEmail":"h.b.furuseth@usit.uio.no","sentAt":"2012-01-23T04:50:23Z","receivedAt":"2012-01-23T04:50:23Z","isPatch":false,"sender":{"key":"h.b.furuseth@usit.uio.no","avatar":null},"body":"t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL\n6.2, Perl 5.10.1. Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1\nfixes it. The commit expects Time::HiRes to be present, but RedHat\nhas split it out to a separate RPM perl-Time-HiRes. Better add a\ncomment about that, so it doesn't get re-reverted.\n\nOr pacify the test and expect gitweb@RHEL-users to install the RPM:\n\n--- git-1.7.9.rc2/t/gitweb-lib.sh~\n+++ git-1.7.9.rc2/t/gitweb-lib.sh\n@@ -113,4 +113,9 @@\n \ttest_done\n }\n \n+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n+\tskip_all='skipping gitweb tests, Time::HiRes module not available'\n+\ttest_done\n+}\n+\n gitweb_init\n\n-- \nHallvard\n"},{"id":"182952","messageId":"hbf.20120123smq1@bombur.uio.no","threadId":"29427","inReplyTo":"hbf.20120123rqzg@bombur.uio.no","subject":"Test t9500 fails if Time::HiRes is missing","fromName":"Hallvard Breien Furuseth","fromEmail":"h.b.furuseth@usit.uio.no","sentAt":"2012-01-23T05:39:21Z","receivedAt":"2012-01-23T05:39:21Z","isPatch":false,"sender":{"key":"h.b.furuseth@usit.uio.no","avatar":null},"body":"I wrote:\n> Better add a comment about that, so it doesn't get re-reverted.\n\nPerhaps I should follow my own advise...\n\n> Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n\n--- git-1.7.9.rc2/t/gitweb-lib.sh~\n+++ git-1.7.9.rc2/t/gitweb-lib.sh\n@@ -113,4 +113,10 @@\n \ttest_done\n }\n \n+# RedHat has moved Time::HiRes out from core Perl to a separate package.\n+perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n+\tskip_all='skipping gitweb tests, Time::HiRes module not available'\n+\ttest_done\n+}\n+\n gitweb_init\n\n-- \nHallvard\n"},{"id":"182956","messageId":"CACBZZX4cjcY5d3mPJAV+rbSTqCEUOrF=_dd3ny_jSM++G-Bg1Q@mail.gmail.com","threadId":"29427","inReplyTo":"hbf.20120123rqzg@bombur.uio.no","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-01-23T09:42:02Z","receivedAt":"2012-01-23T09:42:02Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth\n wrote:\n> t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL\n> 6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1\n> fixes it.  The commit expects Time::HiRes to be present, but RedHat\n> has split it out to a separate RPM perl-Time-HiRes.  Better add a\n> comment about that, so it doesn't get re-reverted.\n>\n> Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n>\n> --- git-1.7.9.rc2/t/gitweb-lib.sh~\n> +++ git-1.7.9.rc2/t/gitweb-lib.sh\n> @@ -113,4 +113,9 @@\n>        test_done\n>  }\n>\n> +perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n> +       skip_all='skipping gitweb tests, Time::HiRes module not available'\n> +       test_done\n> +}\n> +\n>  gitweb_init\n\n[Adding Jakub to CC]\n\nThis doesn't actually fix the issue, it only sweeps it under the rug\nby making the tests pass, gitweb will still fail to compile on Red\nHat once installed.\n\nI think the right solution is to partially revert\n3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the\ncode indicating that it's to deal with RedHat's broken fork of Perl.\n\nHowever even if it's required in an eval it might still fail at\nruntime in the reset_timer() function, which'll need to deal with it\ntoo.\n"},{"id":"183163","messageId":"7v8vkt1yry.fsf@alter.siamese.dyndns.org","threadId":"29427","inReplyTo":"CACBZZX4cjcY5d3mPJAV+rbSTqCEUOrF=_dd3ny_jSM++G-Bg1Q@mail.gmail.com","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-27T05:48:33Z","receivedAt":"2012-01-27T05:48:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This doesn't actually fix the issue, it only sweeps it under the rug\n> by making the tests pass, gitweb will still fail to compile on Red\n> Hat once installed.\n\nIn the short term for 1.7.9, let's at least warn users about this issue.\n\n-- >8 --\nSubject: INSTALL: warn about recent Fedora breakage\n\nRecent releases of Redhat/Fedora are reported to ship Perl binary package\nwith some core modules stripped away (see http://lwn.net/Articles/477234/)\nagainst the upstream Perl5 people's wishes. The Time::HiRes module used by\ngitweb one of them.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Hopefully, this may resolve itself over time.\n\n INSTALL | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/INSTALL b/INSTALL\nindex 8120641..6fa83fe 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -83,7 +83,11 @@ Issues of note:\n \t- \"Perl\" version 5.8 or later is needed to use some of the\n \t features (e.g. preparing a partial commit using \"git add -i/-p\",\n \t interacting with svn repositories with \"git svn\"). If you can\n-\t live without these, use NO_PERL.\n+\t live without these, use NO_PERL. Note that recent releases of\n+\t Redhat/Fedora are reported to ship Perl binary package with some\n+\t core modules stripped away (see http://lwn.net/Articles/477234/),\n+\t so you might need to install additional packages other than Perl\n+\t itself, e.g. Time::HiRes.\n \n \t- \"openssl\" library is used by git-imap-send to use IMAP over SSL.\n \t If you don't need it, use NO_OPENSSL.\n"},{"id":"183176","messageId":"CANQwDwcZNy8DDwqj+C4sjdvObKiNhqc7R3DVL6BpwweQdBBkCw@mail.gmail.com","threadId":"29427","inReplyTo":"CACBZZX4cjcY5d3mPJAV+rbSTqCEUOrF=_dd3ny_jSM++G-Bg1Q@mail.gmail.com","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-27T09:18:24Z","receivedAt":"2012-01-27T09:18:24Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, Jan 23, 2012 at 10:42 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth wrote:\n>>\n>> t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL\n>> 6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1\n>> fixes it.  The commit expects Time::HiRes to be present, but RedHat\n>> has split it out to a separate RPM perl-Time-HiRes.  Better add a\n>> comment about that, so it doesn't get re-reverted.\n>>\n>> Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n>>\n>> --- git-1.7.9.rc2/t/gitweb-lib.sh~\n>> +++ git-1.7.9.rc2/t/gitweb-lib.sh\n>> @@ -113,4 +113,9 @@\n>>        test_done\n>>  }\n>>\n>> +perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n>> +       skip_all='skipping gitweb tests, Time::HiRes module not available'\n>> +       test_done\n>> +}\n>> +\n>>  gitweb_init\n>\n> [Adding Jakub to CC]\n>\n> This doesn't actually fix the issue, it only sweeps it under the rug\n> by making the tests pass, gitweb will still fail to compile on Red\n> Hat once installed.\n>\n> I think the right solution is to partially revert\n> 3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the\n> code indicating that it's to deal with RedHat's broken fork of Perl.\n>\n> However even if it's required in an eval it might still fail at\n> runtime in the reset_timer() function, which'll need to deal with it\n> too.\n\nI'll try to send a fix today. Time::HiRes is needed only for optional timing\ninfo.\n\n-- \nJakub Narebski\n"},{"id":"183177","messageId":"CACBZZX5Y5u=8U4s2aohr6wERuybCMRTamQK7v=JRUOr+ZpJjxQ@mail.gmail.com","threadId":"29427","inReplyTo":"7v8vkt1yry.fsf@alter.siamese.dyndns.org","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-01-27T09:32:29Z","receivedAt":"2012-01-27T09:32:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jan 27, 2012 at 06:48, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> This doesn't actually fix the issue, it only sweeps it under the rug\n>> by making the tests pass, gitweb will still fail to compile on Red\n>> Hat once installed.\n>\n> In the short term for 1.7.9, let's at least warn users about this issue.\n>\n> -- >8 --\n> Subject: INSTALL: warn about recent Fedora breakage\n>\n> Recent releases of Redhat/Fedora are reported to ship Perl binary package\n> with some core modules stripped away (see http://lwn.net/Articles/477234/)\n> against the upstream Perl5 people's wishes. The Time::HiRes module used by\n> gitweb one of them.\n\nSince I wrote that E-Mail I learned what RedHat was doing, I think\nthat's a far better option. They're splitting up the perl core into\nmultiple packages, and anyone who has issues with this on RedHat can\ntrivially just install those packages. So we should just note it in\nthe INSTALL file as a platform-specific issue and leave it at that.\n\nWe *could* deal with this in our code, but I don't think dealing with\nevery vendor's slightly different perl version is a viable strategy in\nthe long run.\n"},{"id":"183182","messageId":"69c90e626682e60d33bebcc6d3ff3fdb@ulrik.uio.no","threadId":"29427","inReplyTo":"CACBZZX4cjcY5d3mPJAV+rbSTqCEUOrF=_dd3ny_jSM++G-Bg1Q@mail.gmail.com","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Hallvard B Furuseth","fromEmail":"h.b.furuseth@usit.uio.no","sentAt":"2012-01-27T10:15:10Z","receivedAt":"2012-01-27T10:15:10Z","isPatch":false,"sender":{"key":"h.b.furuseth@usit.uio.no","avatar":null},"body":" On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason \n wrote:\n> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth\n> wrote:\n>> Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n>>\n>> --- git-1.7.9.rc2/t/gitweb-lib.sh~\n>> +++ git-1.7.9.rc2/t/gitweb-lib.sh\n>> @@ -113,4 +113,9 @@\n>>        test_done\n>>  }\n>>\n>> +perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n>> +       skip_all='skipping gitweb tests, Time::HiRes module not \n>> available'\n>> +       test_done\n>> +}\n>> +\n>>  gitweb_init\n>\n> [Adding Jakub to CC]\n>\n> This doesn't actually fix the issue, it only sweeps it under the rug\n> by making the tests pass, gitweb will still fail to compile on Red\n> Hat once installed.\n\n Is that relevant? gitweb-lib.sh already has code to pass the tests if\n Encode, CGI, CGI::Util or CGI::Carp are missing. I just added another.\n\n-- \n Hallvard\n"},{"id":"183183","messageId":"CANQwDwfsdCGhNLQrJ5Ajz+BNdZmWEu=2b1UHmP=x0RsaZQOPrQ@mail.gmail.com","threadId":"29427","inReplyTo":"69c90e626682e60d33bebcc6d3ff3fdb@ulrik.uio.no","subject":"Re: Test t9500 fails if Time::HiRes is missing","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-27T10:59:36Z","receivedAt":"2012-01-27T10:59:36Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, Jan 27, 2012 at 11:15 AM, Hallvard B Furuseth\n wrote:\n> On Mon, 23 Jan 2012 10:42:02 +0100, Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth\n>> wrote:\n>>>\n>>> Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n>>>\n>>> --- git-1.7.9.rc2/t/gitweb-lib.sh~\n>>> +++ git-1.7.9.rc2/t/gitweb-lib.sh\n>>> @@ -113,4 +113,9 @@\n>>>        test_done\n>>>  }\n>>>\n>>> +perl -MTime::HiRes -e 0 >/dev/null 2>&1 || {\n>>> +       skip_all='skipping gitweb tests, Time::HiRes module not available'\n>>> +       test_done\n>>> +}\n>>> +\n>>>  gitweb_init\n>>\n>>\n>> [Adding Jakub to CC]\n>>\n>> This doesn't actually fix the issue, it only sweeps it under the rug\n>> by making the tests pass, gitweb will still fail to compile on Red\n>> Hat once installed.\n>\n>\n> Is that relevant?  gitweb-lib.sh already has code to pass the tests if\n> Encode, CGI, CGI::Util or CGI::Carp are missing.  I just added another.\n\nThe difference is that:\n1.) Time::HiRes is a core Perl module, so theoretically it should be always\n installed.\n2.) Time::HiRes is not really required for gitweb to work, only for optional\n feature (timing information).\n\n-- \nJakub Narebski\n"},{"id":"183194","messageId":"201201271845.39576.jnareb@gmail.com","threadId":"29427","inReplyTo":"CACBZZX4cjcY5d3mPJAV+rbSTqCEUOrF=_dd3ny_jSM++G-Bg1Q@mail.gmail.com","subject":"[PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-27T17:45:38Z","receivedAt":"2012-01-27T17:45:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth wrote:\n> >\n> > t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL\n> > 6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1\n> > fixes it.  The commit expects Time::HiRes to be present, but RedHat\n> > has split it out to a separate RPM perl-Time-HiRes.  Better add a\n> > comment about that, so it doesn't get re-reverted.\n> >\n> > Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n[...]\n \n> This doesn't actually fix the issue, it only sweeps it under the rug\n> by making the tests pass, gitweb will still fail to compile on Red\n> Hat once installed.\n\nI think you meant \"fail to run\" here.\n\n> I think the right solution is to partially revert\n> 3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the\n> code indicating that it's to deal with RedHat's broken fork of Perl.\n\nI have added comment in commit message, but not in code. I wonder if\nit would be enough.\n\n> However even if it's required in an eval it might still fail at\n> runtime in the reset_timer() function, which'll need to deal with it\n> too.\n\nIt shouldn't; everything else related to timer is protected with\n'if defined $t0', which is false if Time::HiRes module is not available.\n\nHere is the patch\n-- >8 --\nFrom: Jakub Narebski \nSubject: [PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"\n\nThis reverts commit 3962f1d756ab41c1d180e35483d1c8dffe51e0d1.\n\nThough Time::HiRes is a core Perl module, it doesn't necessarily mean\nthat it is included in 'perl' package, and that it is installed if\nPerl is installed.\n\nFor example RedHat has split it out to a separate RPM perl-Time-HiRes.\n\nNoticed-by: Hallvard Breien Furuseth \nSuggested-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Jakub Narębski \n---\n gitweb/gitweb.perl | 12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex abb5a79..c86224a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -17,10 +17,12 @@ use Encode;\n use Fcntl ':mode';\n use File::Find qw();\n use File::Basename qw(basename);\n-use Time::HiRes qw(gettimeofday tv_interval);\n binmode STDOUT, ':utf8';\n \n-our $t0 = [ gettimeofday() ];\n+our $t0;\n+if (eval { require Time::HiRes; 1; }) {\n+\t$t0 = [Time::HiRes::gettimeofday()];\n+}\n our $number_of_git_cmds = 0;\n \n BEGIN {\n@@ -1142,7 +1144,7 @@ sub dispatch {\n }\n \n sub reset_timer {\n-\tour $t0 = [ gettimeofday() ]\n+\tour $t0 = [Time::HiRes::gettimeofday()]\n \t\tif defined $t0;\n \tour $number_of_git_cmds = 0;\n }\n@@ -3974,7 +3976,7 @@ sub git_footer_html {\n \t\tprint \"
\\n\";\n \t\tprint 'This page took '.\n \t\t ''.\n-\t\t tv_interval($t0, [ gettimeofday() ]).\n+\t\t Time::HiRes::tv_interval($t0, [Time::HiRes::gettimeofday()]).\n \t\t ' seconds '.\n \t\t ' and '.\n \t\t ''.\n@@ -6253,7 +6255,7 @@ sub git_blame_common {\n \t\tprint 'END';\n \t\tif (defined $t0 && gitweb_check_feature('timed')) {\n \t\t\tprint ' '.\n-\t\t\t tv_interval($t0, [ gettimeofday() ]).\n+\t\t\t Time::HiRes::tv_interval($t0, [Time::HiRes::gettimeofday()]).\n \t\t\t ' '.$number_of_git_cmds;\n \t\t}\n \t\tprint \"\\n\";\n-- \n1.7.6\n"},{"id":"183198","messageId":"7vty3gzxhs.fsf@alter.siamese.dyndns.org","threadId":"29427","inReplyTo":"201201271845.39576.jnareb@gmail.com","subject":"Re: [PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-27T20:44:31Z","receivedAt":"2012-01-27T20:44:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Though Time::HiRes is a core Perl module, it doesn't necessarily mean\n> that it is included in 'perl' package, and that it is installed if\n> Perl is installed.\n\nI do not think we have seen the end of Redhat/Fedora Perl saga. I am\nhoping that either one of the two things to happen:\n\n (1) Redhat/Fedora distrubution reconsiders the situation and fix their\n packages so that by default when its users ask for \"Perl\" they get\n what the upstream distributes as \"Perl\" in full, while still allowing\n people who know what they are doing to install a minimum subset\n \"perl-base\"; or\n\n (2) Many applications that use and rely on Perl like we do are hit by\n this issue, and Redhat/Fedora users are trained to install the\n perl-full (or whatever it is called) package when applications want\n \"Perl\".\n\nIn other words, I am hoping that \"it doesn't necessarily mean\" will not\nstay true for a long time. So please hold onto this patch until the dust\nsettles, and resend it if (1) does not look to be happening in say 3\nmonths.\n\n\n> For example RedHat has split it out to a separate RPM perl-Time-HiRes.\n>\n> Noticed-by: Hallvard Breien Furuseth \n> Suggested-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Jakub Narębski \n> ---\n> gitweb/gitweb.perl | 12 +++++++-----\n> 1 files changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index abb5a79..c86224a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -17,10 +17,12 @@ use Encode;\n> use Fcntl ':mode';\n> use File::Find qw();\n> use File::Basename qw(basename);\n> -use Time::HiRes qw(gettimeofday tv_interval);\n> binmode STDOUT, ':utf8';\n> \n> -our $t0 = [ gettimeofday() ];\n> +our $t0;\n> +if (eval { require Time::HiRes; 1; }) {\n> +\t$t0 = [Time::HiRes::gettimeofday()];\n> +}\n> our $number_of_git_cmds = 0;\n\nWhy should these even be initialized here? Doesn't reset_timer gets\ncalled at the beginning of run_request()?\n> \n> BEGIN {\n> @@ -1142,7 +1144,7 @@ sub dispatch {\n> }\n> \n> sub reset_timer {\n> -\tour $t0 = [ gettimeofday() ]\n> +\tour $t0 = [Time::HiRes::gettimeofday()]\n> \t\tif defined $t0;\n> \tour $number_of_git_cmds = 0;\n\nThe statement modifier look ugly.\n\nMore importantly, if you are not profiling, i.e. if we didn't initialize\n$t0 at the beginning, do you need to reset $number_of_git_cmds at all?\n\nI also think this should take gitweb_check_feature('timed') into\naccount, perhaps like this:\n\n\tsub reset_timer {\n \treturn unless gitweb_check_feature('timed');\n our $t0 = ...\n our $number_of_git_cmds = 0;\n\t}\n\nThen all the other\n\n\tif (defined $t0 && gitweb_check_feature('timed'))\n\ncan become\n\n\tif (defined $t0)\n\nIf you go this route, even though tee-zero, the beginning of the time, is\na good name for the variable, you may want to rename it to avoid confusing\nreaders who might take it as a temporary variable #0.\n"},{"id":"183228","messageId":"201201281848.49483.jnareb@gmail.com","threadId":"29427","inReplyTo":"7vty3gzxhs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-28T17:48:48Z","receivedAt":"2012-01-28T17:48:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 27 Jan 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > Though Time::HiRes is a core Perl module, it doesn't necessarily mean\n> > that it is included in 'perl' package, and that it is installed if\n> > Perl is installed.\n> \n> I do not think we have seen the end of Redhat/Fedora Perl saga. I am\n> hoping that either one of the two things to happen:\n> \n> (1) Redhat/Fedora distrubution reconsiders the situation and fix their\n> packages so that by default when its users ask for \"Perl\" they get\n> what the upstream distributes as \"Perl\" in full, while still allowing\n> people who know what they are doing to install a minimum subset\n> \"perl-base\"; or\n> \n> (2) Many applications that use and rely on Perl like we do are hit by\n> this issue, and Redhat/Fedora users are trained to install the\n> perl-full (or whatever it is called) package when applications want\n> \"Perl\".\n> \n> In other words, I am hoping that \"it doesn't necessarily mean\" will not\n> stay true for a long time. So please hold onto this patch until the dust\n> settles, and resend it if (1) does not look to be happening in say 3\n> months.\n \nSo for the time being (those \"3 months\") you would apply instead your\nchange to INSTALL (or equivalent to gitweb/INSTALL) mentioning Time::HiRes\nissue, and perhaps also original patch by Hallvard skipping gitweb tests\nif Time::HiRes is not available, isn't it?\n \n> > For example RedHat has split it out to a separate RPM perl-Time-HiRes.\n\n[...]\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index abb5a79..c86224a 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -17,10 +17,12 @@ use Encode;\n> > use Fcntl ':mode';\n> > use File::Find qw();\n> > use File::Basename qw(basename);\n> > -use Time::HiRes qw(gettimeofday tv_interval);\n> > binmode STDOUT, ':utf8';\n> > \n> > -our $t0 = [ gettimeofday() ];\n> > +our $t0;\n> > +if (eval { require Time::HiRes; 1; }) {\n> > +\t$t0 = [Time::HiRes::gettimeofday()];\n> > +}\n> > our $number_of_git_cmds = 0;\n> \n> Why should these even be initialized here? Doesn't reset_timer gets\n> called at the beginning of run_request()?\n\nI think it predates adding reset_timer() to gitweb. Anyway $t0 has\nto be set to something defined anyway to denote that Time::HiRes is\navailable... though if Time::HiRes is required unconditionally it would\nnot be really needed, and we can always check $INC{'Time/HiRes.pm'}\nif it was loaded or not.\n\n> > BEGIN {\n> > @@ -1142,7 +1144,7 @@ sub dispatch {\n> > }\n> > \n> > sub reset_timer {\n> > -\tour $t0 = [ gettimeofday() ]\n> > +\tour $t0 = [Time::HiRes::gettimeofday()]\n> > \t\tif defined $t0;\n> > \tour $number_of_git_cmds = 0;\n> \n> The statement modifier look ugly.\n> \n> More importantly, if you are not profiling, i.e. if we didn't initialize\n> $t0 at the beginning, do you need to reset $number_of_git_cmds at all?\n> \n> I also think this should take gitweb_check_feature('timed') into\n> account, perhaps like this:\n> \n> \tsub reset_timer {\n> \treturn unless gitweb_check_feature('timed');\n> our $t0 = ...\n> our $number_of_git_cmds = 0;\n> \t}\n> \n> Then all the other\n> \n> \tif (defined $t0 && gitweb_check_feature('timed'))\n> \n> can become\n> \n> \tif (defined $t0)\n\nI think this is a good idea... though it would complicate applying revert\na bit ;-(\n\n> If you go this route, even though tee-zero, the beginning of the time, is\n> a good name for the variable, you may want to rename it to avoid confusing\n> readers who might take it as a temporary variable #0.\n\nGood idea. $request_start_time perhaps? Or $time_start?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"183237","messageId":"CACBZZX7M83PAdLPXLYoBtcihf=5AHruk9=JZo7mh+uNyLtaOhg@mail.gmail.com","threadId":"29427","inReplyTo":"201201271845.39576.jnareb@gmail.com","subject":"Re: [PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-01-29T02:21:17Z","receivedAt":"2012-01-29T02:21:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jan 27, 2012 at 18:45, Jakub Narebski wrote:\n> On Mon, 23 Jan 2012, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Jan 23, 2012 at 05:50, Hallvard Breien Furuseth wrote:\n>> >\n>> > t9500-gitweb-standalone-no-errors fails: Git 1.7.9.rc2/1.7.8.4, RHEL\n>> > 6.2, Perl 5.10.1.  Reverting 3962f1d756ab41c1d180e35483d1c8dffe51e0d1\n>> > fixes it.  The commit expects Time::HiRes to be present, but RedHat\n>> > has split it out to a separate RPM perl-Time-HiRes.  Better add a\n>> > comment about that, so it doesn't get re-reverted.\n>> >\n>> > Or pacify the test and expect gitweb@RHEL-users to install the RPM:\n> [...]\n>\n>> This doesn't actually fix the issue, it only sweeps it under the rug\n>> by making the tests pass, gitweb will still fail to compile on Red\n>> Hat once installed.\n>\n> I think you meant \"fail to run\" here.\n\nI mean fail to compile, \"use\" statements are executed at compile time,\nif it was a \"require\" outside of BEGIN-time it would fail at runtime.\n\nI realize though that you probably thought I meant fail in Git's\nMakefile-driven compilation phase, but no, it'll install just fine,\nhowever the perl interpreter won't compile it.\n\n>> I think the right solution is to partially revert\n>> 3962f1d756ab41c1d180e35483d1c8dffe51e0d1, but add a comment in the\n>> code indicating that it's to deal with RedHat's broken fork of Perl.\n>\n> I have added comment in commit message, but not in code.  I wonder if\n> it would be enough.\n>\n>> However even if it's required in an eval it might still fail at\n>> runtime in the reset_timer() function, which'll need to deal with it\n>> too.\n>\n> It shouldn't; everything else related to timer is protected with\n> 'if defined $t0', which is false if Time::HiRes module is not available.\n\nCorrect, I didn't look carefully enough.\n"},{"id":"183238","messageId":"CACBZZX4KOc6Roz7U5rLrNnzJ_JY9WsSyV6zU_KOsHC+A8y7w4w@mail.gmail.com","threadId":"29427","inReplyTo":"7vty3gzxhs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"gitweb: Time::HiRes is in core for Perl 5.8\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2012-01-29T02:29:09Z","receivedAt":"2012-01-29T02:29:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jan 27, 2012 at 21:44, Junio C Hamano wrote:\n>\n>        if (defined $t0)\n>\n> If you go this route, even though tee-zero, the beginning of the\n> time, is a good name for the variable, you may want to rename it to\n> avoid confusing readers who might take it as a temporary variable\n> #0.\n\n\n\nPersonally I'd have written it as $START_TIME, but as a bit of Perl\ntrivia you might not realize $t0 is a commonly used and undestood\nvariable for dealing with a start time in Perl in the same way that\n`i` is common for dealing with array indexes in C.\n\nI.e. someone used to Perl will immediately think \"oh that's the start\ntime\" having seen it hundreds of times before, but someone not used to\nPerl will go \"what's this t-zero thing?\".\n\nMeanwhile some Lisp programmer is wondering what the hell \"i\" means in\nyour C for-loops, iterator? :)\n"}]}