\n> ---\n> gitweb/gitweb.perl | 31 +++++++++++++++++++++++++------\n> 1 files changed, 25 insertions(+), 6 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1b9369d..3b6a90d 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1199,6 +1199,7 @@ if (defined caller) {\n> # -full => 0|1 - use absolute/full URL ($my_uri/$my_url as base)\n> # -replay => 1 - start from a current view (replay with modifications)\n> # -path_info => 0|1 - don't use/use path_info URL (if possible)\n> +# -anchor - add #ANCHOR to end of URL\n\nShouldn't it be:\n\n +# -anchor => ANCHOR - add #ANCHOR to end of URL\n\n> sub href {\n> \tmy %params = @_;\n> \t# default is to use -absolute url() i.e. $my_uri\n> @@ -1314,6 +1315,10 @@ sub href {\n> \t# final transformation: trailing spaces must be escaped (URI-encoded)\n> \t$href =~ s/(\\s+)$/CGI::escape($1)/e;\n> \n> +\tif (defined($params{'anchor'})) {\n> +\t\t$href .= \"#\".esc_param($params{'anchor'});\n> +\t}\n> +\n> \treturn $href;\n> }\n\nHere you have slight mismatch between description, which uses\n'-anchor', and code, which uses 'anchor'.\n\n> \n> @@ -4334,8 +4339,10 @@ sub git_difftree_body {\n> \t\t\tif ($action eq 'commitdiff') {\n> \t\t\t\t# link to patch\n> \t\t\t\t$patchno++;\n> -\t\t\t\tprint \"| \" .\n> -\t\t\t\t $cgi->a({-href => \"#patch$patchno\"}, \"patch\") .\n> +\t\t\t\tprint $cgi->a({-href =>\n> +\t\t\t\t href(action=>\"commitdiff\",\n> +\t\t\t\t hash=>$hash, anchor=>\"patch$patchno\")},\n> +\t\t\t\t \"patch\") .\n\nIt would be better (less error prone) and easier to use '-replay'\noption to href(), i.e. write\n\n> +\t\t\t\tprint $cgi->a({-href => href(-replay=>1, -anchor=>\"patch$patchno\")},\n> +\t\t\t\t \"patch\") .\n\nor even make it so 'href(-anchor=>\"ANCHOR\")' implies '-replay => 1'.\nThe href() part of patch would then look something like this:\n\n @@ -1199,6 +1199,7 @@ if (defined caller) {\n # -full => 0|1 - use absolute/full URL ($my_uri/$my_url as base)\n # -replay => 1 - start from a current view (replay with modifications)\n # -path_info => 0|1 - don't use/use path_info URL (if possible)\n +# -anchor => ANCHOR - add #ANCHOR to end of URL, implies -replay if used alone\n sub href {\n \tmy %params = @_;\n \t# default is to use -absolute url() i.e. $my_uri\n @@ -1310,6 +1310,7 @@ sub href {\n \n \t$params{'project'} = $project unless exists $params{'project'};\n \n -\tif ($params{-replay}) {\n +\tif ($params{-replay} ||\n +\t ($params{-anchor} && keys %params == 1)) {\n \t\twhile (my ($name, $symbol) = each %cgi_param_mapping) {\n \t\t\tif (!exists $params{$name}) {\n \t\t\t\t$params{$name} = $input_params{$name};\n @@ -1314,6 +1315,10 @@ sub href {\n \t# final transformation: trailing spaces must be escaped (URI-encoded)\n \t$href =~ s/(\\s+)$/CGI::escape($1)/e;\n \n +\tif (defined($params{'anchor'})) {\n +\t\t$href .= \"#\".esc_param($params{'anchor'});\n +\t}\n +\n \treturn $href;\n }\n\nDo you want to resend patch with those corrections yourself, or should\nI do this?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"163551","messageId":"m3d3lq57vw.fsf@localhost.localdomain","threadId":"26742","inReplyTo":"e272fa98ecab9d30edb4457e2e215688@localhost","subject":"Re: [PATCH 2/2] gitweb: introduce localtime feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-17T11:01:18Z","receivedAt":"2011-03-17T11:01:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Kevin Cernekee writes:\n\n> With this feature enabled, all timestamps are shown in the machine's\n> local timezone instead of GMT.\n\nThis does not describe why would one want such way of displaying\ntimestamps, and which views would be affected.\n\nBTW. should it be timezone of web server (machine where gitweb is\nrun), or local time of author / committer / tagger as described in the\ntimezone part of git timestamp?\n \n> Signed-off-by: Kevin Cernekee \n> ---\n> gitweb/gitweb.perl | 14 +++++++++++++-\n> 1 files changed, 13 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 3b6a90d..d171ad5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -504,6 +504,12 @@ our %feature = (\n> \t\t'sub' => sub { feature_bool('remote_heads', @_) },\n> \t\t'override' => 0,\n> \t\t'default' => [0]},\n> +\n> +\t# Use localtime rather than GMT for all timestamps. Disabled\n> +\t# by default. Project specific override is not supported.\n> +\t'localtime' => {\n> +\t\t'override' => 0,\n> +\t\t'default' => [0]},\n\nWhy project specific override is not supported? I think it might make\nsense to enable this feature on project-by-project basis; some\nprojects might be dispersed geographically, some might not.\n\nIt is not as if this feature affect only non-project views, or doesn't\nmake sense on less that site-wide basis, like other nonoverridable\nfeatures.\n\n> );\n> \n> sub gitweb_get_feature {\n> @@ -2927,6 +2933,12 @@ sub parse_date {\n> \t$date{'iso-tz'} = sprintf(\"%04d-%02d-%02d %02d:%02d:%02d %s\",\n> \t 1900+$year, $mon+1, $mday,\n> \t $hour, $min, $sec, $tz);\n> +\n> +\tif (gitweb_check_feature('localtime')) {\n> +\t\t$date{'rfc2822'} = sprintf \"%s, %d %s %4d %02d:%02d:%02d $tz\",\n> +\t\t\t\t $days[$wday], $mday, $months[$mon],\n> +\t\t\t\t 1900+$year, $hour ,$min, $sec;\n> +\t}\n\nIs it still an RFC 2822 conformant date? If it is not, then above\nchange is invalid, and we have to implement this feature in different\nway.\n\n> \treturn %date;\n> }\n> \n> @@ -3989,7 +4001,7 @@ sub git_print_authorship_rows {\n> \t\t \" | \\n\" .\n> \t\t \"\" .\n> \t\t \" | $wd{'rfc2822'}\";\n> -\t\tprint_local_time(%wd);\n> +\t\tprint_local_time(%wd) if !gitweb_check_feature('localtime');\n\nHmmm... I wonder if it wouldn't be better to print both times (perhaps\nreversed) in this case...\n\n> \t\tprint \" | \" .\n> \t\t \"
\\n\";\n> \t}\n> -- \n> 1.7.4.1\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"163570","messageId":"7vaagt4n9f.fsf@alter.siamese.dyndns.org","threadId":"26742","inReplyTo":"m3d3lq57vw.fsf@localhost.localdomain","subject":"Re: [PATCH 2/2] gitweb: introduce localtime feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-17T18:26:04Z","receivedAt":"2011-03-17T18:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Kevin Cernekee writes:\n>\n>> With this feature enabled, all timestamps are shown in the machine's\n>> local timezone instead of GMT.\n>\n> This does not describe why would one want such way of displaying\n> timestamps, and which views would be affected.\n>\n> BTW. should it be timezone of web server (machine where gitweb is\n> run), or local time of author / committer / tagger as described in the\n> timezone part of git timestamp?\n\nBoth are sensible choices; another plausible choice that normal people\nwould want to see is to use the zone of whoever is requesting the page\n(note that this would affect the caching layer).\n\n>> +\t# Use localtime rather than GMT for all timestamps. Disabled\n>> +\t# by default. Project specific override is not supported.\n>> +\t'localtime' => {\n>> +\t\t'override' => 0,\n>> +\t\t'default' => [0]},\n>\n> Why project specific override is not supported?\n\nGood question.\n"},{"id":"163572","messageId":"7v62rh4ml1.fsf@alter.siamese.dyndns.org","threadId":"26742","inReplyTo":"m3hbb258pw.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-17T18:40:42Z","receivedAt":"2011-03-17T18:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> It would be better (less error prone) and easier to use '-replay'\n> option to href(), i.e. write\n>\n>> +\t\t\t\tprint $cgi->a({-href => href(-replay=>1, -anchor=>\"patch$patchno\")},\n>> +\t\t\t\t \"patch\") .\n>\n> or even make it so 'href(-anchor=>\"ANCHOR\")' implies '-replay => 1'.\n\nI don't see why \"or even\" is an improvement, given the following\nimplementation.\n\n> @@ -1310,6 +1310,7 @@ sub href {\n> \n> \t$params{'project'} = $project unless exists $params{'project'};\n> \n> -\tif ($params{-replay}) {\n> +\tif ($params{-replay} ||\n> +\t ($params{-anchor} && keys %params == 1)) {\n> \t\twhile (my ($name, $symbol) = each %cgi_param_mapping) {\n> \t\t\tif (!exists $params{$name}) {\n> \t\t\t\t$params{$name} = $input_params{$name};\n\nI don't share your intuition that \"anchor\" is so special that this part\nwill not grow into a long chain of \"(I want an implicit replay too) ||\".\n\nImplicitly enabling it in certain obvious cases is perfectly fine, but the\nlogic to do so should be in a separate place. Wouldn't it better to have\na separate code that sets 'replay' under this and that condition so that\nother people can later add to it at the very beginning of \"sub href\"?\n\nUnless we do so, if there are other places that need to change the\nbehaviour based on 'replay', they need to duplicate the \"implicit\" logic.\n"},{"id":"163576","messageId":"201103172020.05055.jnareb@gmail.com","threadId":"26742","inReplyTo":"7v62rh4ml1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-17T19:19:59Z","receivedAt":"2011-03-17T19:19:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 17 Mar 2011, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>> It would be better (less error prone) and easier to use '-replay'\n>> option to href(), i.e. write\n>>\n>>> +\t\t\t\tprint $cgi->a({-href => href(-replay=>1, -anchor=>\"patch$patchno\")},\n>>> +\t\t\t\t \"patch\") .\n>>\n>> or even make it so 'href(-anchor=>\"ANCHOR\")' implies '-replay => 1'.\n> \n> I don't see why \"or even\" is an improvement, given the following\n> implementation.\n\nWell, \n\n -href => href(-anchor=>\"patch$patchno\")\n\nis closer in spirit to\n\n -href => \"#patch$patchno\"\n\nthat is currently used, and does not work with path_info.\n \n> > @@ -1310,6 +1310,7 @@ sub href {\n> > \n> > \t$params{'project'} = $project unless exists $params{'project'};\n> > \n> > -\tif ($params{-replay}) {\n> > +\tif ($params{-replay} ||\n> > +\t ($params{-anchor} && keys %params == 1)) {\n> > \t\twhile (my ($name, $symbol) = each %cgi_param_mapping) {\n> > \t\t\tif (!exists $params{$name}) {\n> > \t\t\t\t$params{$name} = $input_params{$name};\n> \n> I don't share your intuition that \"anchor\" is so special that this part\n> will not grow into a long chain of \"(I want an implicit replay too) ||\".\n> \n> Implicitly enabling it in certain obvious cases is perfectly fine, but the\n> logic to do so should be in a separate place. Wouldn't it better to have\n> a separate code that sets 'replay' under this and that condition so that\n> other people can later add to it at the very beginning of \"sub href\"?\n> \n> Unless we do so, if there are other places that need to change the\n> behaviour based on 'replay', they need to duplicate the \"implicit\" logic.\n\nSo you would prefer to have something like this:\n\n \t# implicit -replay\n \tif (keys %params == 1 && $params{-anchor}) {\n \t\t# href(-anchor=>\"ANCHOR\") works like \"#ANCHOR\", \n \t\t# correctly if base href is set (for path_info URLs)\n \t\t$params{-replay} = 1;\n \t}\n\nset above 'if ($params{-replay}) {'?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"163590","messageId":"AANLkTi=+aAeV4mx4YTHw4rWZahLS0tnjXsDLEgoAj9-o@mail.gmail.com","threadId":"26742","inReplyTo":"m3d3lq57vw.fsf@localhost.localdomain","subject":"Re: [PATCH 2/2] gitweb: introduce localtime feature","fromName":"Kevin Cernekee","fromEmail":"cernekee@gmail.com","sentAt":"2011-03-17T20:12:39Z","receivedAt":"2011-03-17T20:12:39Z","isPatch":true,"sender":{"key":"cernekee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1631864?v=4"},"body":"On Thu, Mar 17, 2011 at 4:01 AM, Jakub Narebski wrote:\n\nJakub,\n\nThanks for all of your constructive feedback. I have taken your\nsuggestions into account and posted an updated series of patches.\n\n> This does not describe why would one want such way of displaying\n> timestamps, and which views would be affected.\n>\n> BTW. should it be timezone of web server (machine where gitweb is\n> run), or local time of author / committer / tagger as described in the\n> timezone part of git timestamp?\n\nThe case I am currently trying to improve is the one in which all\ndevelopers are at a single site.\n\nIn the open source world it is common to have developers scattered all\nover the globe, so some of them will inevitably have to perform\ntimezone conversions.\n\nBut Git is becoming a popular tool in the private sector and it is\ncommon to have most/all contributors based in a single office. In the\nlatter case, it is helpful to display the local timezone instead of\nGMT. This also helps make the data more readable by program managers\nand other non-developers who have an interest in tracking the project.\n\n> Why project specific override is not supported? I think it might make\n> sense to enable this feature on project-by-project basis; some\n> projects might be dispersed geographically, some might not.\n\nMostly ease of testing. I did not need it for any of my projects.\n\nIt turned out to be a simple change, and it is in v2. The cases I tested were:\n\ndefault 0\ndefault 1\noverride 1, project unset\noverride 1, project 0\noverride 1, project 1\n\n> Is it still an RFC 2822 conformant date? If it is not, then above\n> change is invalid, and we have to implement this feature in different\n> way.\n\nI believe it is still valid.\n\nOriginal date: Thu, 17 Mar 2011 02:11:05 +0000\nNew date: Wed, 16 Mar 2011 19:11:05 -0700\nSample date from RFC 2822 Appendix A: Fri, 21 Nov 1997 09:55:06 -0600\n\n> Hmmm... I wonder if it wouldn't be better to print both times (perhaps\n> reversed) in this case...\n\nI have submitted a third patch which does this.\n"},{"id":"163594","messageId":"7v8vwd31sa.fsf@alter.siamese.dyndns.org","threadId":"26742","inReplyTo":"201103172020.05055.jnareb@gmail.com","subject":"Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-17T20:55:17Z","receivedAt":"2011-03-17T20:55:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n>>> or even make it so 'href(-anchor=>\"ANCHOR\")' implies '-replay => 1'.\n>> \n>> I don't see why \"or even\" is an improvement, given the following\n>> implementation.\n>\n> Well, \n>\n> -href => href(-anchor=>\"patch$patchno\")\n>\n> is closer in spirit to\n>\n> -href => \"#patch$patchno\"\n>\n> that is currently used, and does not work with path_info.\n\nI questioned \"implies '-replay => 1'\" part, not the use of href(...) part.\n"},{"id":"163600","messageId":"201103172330.07332.jnareb@gmail.com","threadId":"26742","inReplyTo":"AANLkTi=+aAeV4mx4YTHw4rWZahLS0tnjXsDLEgoAj9-o@mail.gmail.com","subject":"Re: [PATCH 2/2] gitweb: introduce localtime feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-17T22:30:00Z","receivedAt":"2011-03-17T22:30:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, Mar 17, 2011 at 21:12, Kevin Cernekee wrote:\n> On Thu, Mar 17, 2011 at 4:01 AM, Jakub Narebski wrote:\n\n> > This does not describe why would one want such way of displaying\n> > timestamps, and which views would be affected.\n> >\n> > BTW. should it be timezone of web server (machine where gitweb is\n> > run), or local time of author / committer / tagger as described in the\n> > timezone part of git timestamp?\n> \n> The case I am currently trying to improve is the one in which all\n> developers are at a single site.\n> \n> In the open source world it is common to have developers scattered all\n> over the globe, so some of them will inevitably have to perform\n> timezone conversions.\n> \n> But Git is becoming a popular tool in the private sector and it is\n> common to have most/all contributors based in a single office. In the\n> latter case, it is helpful to display the local timezone instead of\n> GMT. This also helps make the data more readable by program managers\n> and other non-developers who have an interest in tracking the project.\n\nSuch explanation should in my opinion be present in the proposed commit\nmessage. Good commit message should explain not only what the change is\nintent to do (to catch when implementation and intent differs), but also\nwhys behind the change (to find whether commit is worth having).\n\nThe above nicely explains why and when such feature would be useful,\n \n> > Why project specific override is not supported? I think it might make\n> > sense to enable this feature on project-by-project basis; some\n> > projects might be dispersed geographically, some might not.\n> \n> Mostly ease of testing. I did not need it for any of my projects.\n\nYou mean that for your instance of gitweb all projects were single\noffice (not dispersed geographically), so for you enabling it site-wide\nwas enough, isn't it?\n \n> It turned out to be a simple change, and it is in v2.\n\nThanks.\n\n> The cases I tested were: \n> \n> default 0\n> default 1\n> override 1, project unset\n> override 1, project 0\n> override 1, project 1\n\nWell, I think the non-overridden / overridden feature we have tested quite\nwell. BTW did you add test for 'localtime' feature to t9500 test? Perhaps\nit is not strictly necessary, though...\n\n> > Is it still an RFC 2822 conformant date? If it is not, then above\n> > change is invalid, and we have to implement this feature in different\n> > way.\n> \n> I believe it is still valid.\n> \n> Original date: Thu, 17 Mar 2011 02:11:05 +0000\n> New date: Wed, 16 Mar 2011 19:11:05 -0700\n> Sample date from RFC 2822 Appendix A: Fri, 21 Nov 1997 09:55:06 -0600\n\nThanks.\n\n> > Hmmm... I wonder if it wouldn't be better to print both times (perhaps\n> > reversed) in this case...\n> \n> I have submitted a third patch which does this.\n\nNote that we print localtime (time in author / committer / tagger timezone)\nto be able to mark given time as \"atnight\", so one can easily see commits\nand tags which needs more careful review because they were made 4 AM or\nsomething.\n\nIf you reverse the direction you still should make sure that \"atnight\"\nstyling applies to localtime.\n\n-- \nJakub Narebski\nPoland\n"}]}