writes:\n\n> I like v2 version, and had only small corrections. For you to not have\n> to resend this patch once again, I have added those corrections and sent\n> them as a patch myself.\n>\n> Changes from original v2 version from Kevin Cernekee:\n>\n> * Fix (re-add) accidentally lost in v2 '\"\"' in first\n> chunk using href(-anchor => ...)\n> * Reword commit message (hopefully make it better, and not only longer)\n\nThanks.\n\n> * Move code that sets implicit -replay when -anchor is the only parameter\n> lower, and change order of subexpression, for better possible future\n> extendability, if there would be other cases when we would want to \n> automatically turn on -replay.\n\nI think you meant this part:\n\n\t# implicit -replay\n\t$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});\n\nBut I don't think it is that much of an improvement as long as it uses the\nstatement modifier syntax (\"A if B\") for a thing like this.\n\nI will not bother rewriting the commit I made out of this patch myself,\nbut the next person who wants to add the auto-replay for his option will\nfirst have to rewrite the above into:\n\n\tif (key %params == 1 && $params{-anchor}) {\n\t\t$params{-replay} = 1;\n\t}\n\nbefore turning it into:\n\n\tif ((key %params == 1 && $params{-anchor}) ||\n\t (\"here comes my new condition\")) {\n\t\t$params{-replay} = 1;\n\t}\n\nanyway. If your rewrite were to a plain-vanilla \"if () { ... }\", it would\nhave been a real improvement.\n\nBesides, I find it a bad taste to use statement modifier syntax unless the\nmodifying condition (\"if B\" part) is much more likely to hold true than\nfalse.\n\nIf you limit your use of statement modifiers for conditions that almost\nalways hold true, it will become much easier to scan the code for the\nfirst time, because you can skim through and almost ignore the condition\npart to follow the logic for the normal case. But turning 'replay' on in a\nspecific narrow case (and later in a specific set of narrow cases) is a\ncomplete opposite of normal codeflow.\n\nAnyway, thanks for the clean-up. Applied.\n"},{"id":"163655","messageId":"201103181818.59901.jnareb@gmail.com","threadId":"26764","inReplyTo":"7v7hbwxt7e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-18T17:18:53Z","receivedAt":"2011-03-18T17:18:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano napisał:\n\n> Anyway, thanks for the clean-up. Applied.\n\nI hope you applied amended version - this one has stupid bug in it.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"163657","messageId":"201103181846.04979.jnareb@gmail.com","threadId":"26764","inReplyTo":"64c70e95e767572e5be732dc7e17815b@localhost","subject":"[PATCH 3/3 (alternate)] gitweb: Mark \"atnight\" author/committer times also for 'localtime'","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-18T17:46:02Z","receivedAt":"2011-03-18T17:46:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"From: Kevin Cernekee \n\nBy default, with 'localtime' feature disabled, the dates in 'commit',\n'commitdiff' and 'tag' views show both GMT time, and localtime in\nrecorded author/committer/tagger timezone, marking localtime with\n\"atnight\" class to notify times between 0 and 6 AM local time.\n\nAn example output can look like this:\n\n author A U Thor \n Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)\n ^^^^^\n\nwhere underlined part is marked with \"atnight\" class (in red with\ndefault stylesheet).\n\nIf $feature{'localtime'} is enabled, we display the RFC 2822 date/time\nin the author's/committer's/tagger's local timezone; previous commit\nremoved marking \"atnight\" times, because there wasn't separate local\ntime to mark up after GMT time.\n\nThis commit makes gitweb mark _whole_ RFC 2822 date/time with\n\"atnight\" class for times between 0 and 6 AM.\n\nAn example output can look like this:\n\n author A U Thor \n Wed, 16 Mar 2011 02:02:42 -0500\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nwhere again underlined part is marked with \"atnight\".\n\nWe probably should mark only time part of RFC 2822 date/time with\n\"atnight\" class, but such solution would be more involved.\n\nWhile at it fix whitespace, using spaces for align, tabs for indent.\n\n\nNOTE that git_print_authorship subroutine is for now left as is; there\nis no caller in gitweb that uses it with -localtime=>1.\n\nSigned-off-by: Kevin Cernekee \nSigned-off-by: Jakub Narebski \n---\nKevin, how about something like this instead? This preserves _intent_\nfor why there is local time beside GMT time when 'localtime' is disabled\nbetter, I think.\n\nJunio and Kevin, I am not sure if authorship should remain with Kevin,\nor should it revert to me; the solution is quite different.\n\nAbout no-change to git_print_authorship: alternate solution would be\nto remove support for -localtime option, like in original patches.\n\n gitweb/gitweb.perl | 16 ++++++++++++----\n 1 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cdc2a96..5bda0a8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {\n \t\tmy %wd = parse_date($co->{\"${who}_epoch\"}, $co->{\"${who}_tz\"});\n \t\tprint \"| $who | \" .\n \t\t format_search_author($co->{\"${who}_name\"}, $who,\n-\t\t\t esc_html($co->{\"${who}_name\"})) . \" \" .\n+\t\t esc_html($co->{\"${who}_name\"})) . \" \" .\n \t\t format_search_author($co->{\"${who}_email\"}, $who,\n-\t\t\t esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n+\t\t esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n \t\t \" | \" .\n \t\t git_get_avatar($co->{\"${who}_email\"}, -size => 'double') .\n \t\t \" | \\n\" .\n \t\t \"\" .\n-\t\t \" | $wd{'rfc2822'}\";\n-\t\tprint_local_time(%wd) if !gitweb_check_feature('localtime');\n+\t\t \" | | \";\n+\t\tif (gitweb_check_feature('localtime')) {\n+\t\t\tif ($wd{'hour_local'} < 6) {\n+\t\t\t\tprint \"$wd{'rfc2822'}\";\n+\t\t\t} else {\n+\t\t\t\tprint $wd{'rfc2822'};\n+\t\t\t}\n+\t\t} else {\n+\t\t\tprint $wd{'rfc2822'} . format_local_time(%wd);\n+\t\t}\n \t\tprint \" | \" .\n \t\t \" \\n\";\n \t}\n-- \n1.7.3\n"},{"id":"163660","messageId":"7vy64cwal7.fsf@alter.siamese.dyndns.org","threadId":"26764","inReplyTo":"201103181540.03431.jnareb@gmail.com","subject":"Re: [PATCH v3 2/3] gitweb: introduce localtime feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T18:24:36Z","receivedAt":"2011-03-18T18:24:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> From: Kevin Cernekee \n>\n> With this feature enabled, all timestamps are shown in the local\n> timezone instead of GMT. The timezone is taken from the appropriate\n> timezone string stored in the commit object.\n>\n> This is useful if most of contributors (to a project) are based in a\n> single office, all within the same timezone. In such case local time\n> is more useful than GMT / UTC time that gitweb uses by default, and\n> which is better choice for geographically scattered contributors.\n>\n> This change does not affect relative timestamps (e.g. \"5 hours ago\"),\n> and neither does it affect 'patch' and 'patches' views which already\n> use localtime because they are generated by \"git format-patch\".\n>\n> Affected views include:\n> * 'summary' view, \"last change\" field (commit time from latest change)\n> * 'log' view, author time\n> * 'commit' and 'commitdiff' views, author/committer time\n> * 'tag' view, tagger time\n>\n> In the case of 'commit', 'commitdiff' and 'tag' views gitweb used to\n> print both GMT time and time in timezone of author/tagger/comitter,\n> marking localtime with \"atnight\" as appropriate; after this commit\n> gitweb shows only local time. Marking localtime with \"atnight\" when\n> needed is left for subsequent commit.\n>\n> Signed-off-by: Kevin Cernekee \n> Signed-off-by: Jakub Narebski \n\nThanks for moving the explanation up into the log message. Much easier to\nunderstand the motivation.\n\n> @@ -2930,6 +2943,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 $days[$wday], $mday, $months[$mon],\n> +\t\t 1900+$year, $hour ,$min, $sec;\n> +\t}\n> \treturn %date;\n> }\n\nTwo comments (hint: when reviewing, look a bit wider outside the context\nprovided by the patch):\n\n - This gets seconds-since-epoch and returns a bag of pieces of formatted\n timestamp (some are mere elements like \"hour\", some are full timestamp\n like \"iso-8601\"). Doesn't sound like \"parse\"-date, does it?\n\n - It looks somewhat ugly to unconditionally assign to 'rfc2822' first\n (before the context of the hunk) and then overwrite it. Wouldn't it be\n more useful later to have a separate 'rfc2822_local' field, just like\n existing 'hour_local' and 'minute_local' are counterparts for 'hour'\n and 'minute'?\n\n> @@ -3992,7 +4011,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> \t\tprint \" | \" .\n> \t\t \"
\\n\";\n> \t}\n\nVery confusing. \"Ok, we print local time. --ah, wait, only when localtime\nfeature is not used???\"\n\nIt turns out that the hijacking of $wd{'rfc2822'} made above already gives\nus the local time so this patch turns the meaning of print-local-time used\nhere into additionally-print-local-time.\n\nBoth call sites to print_local_time() follow this pattern:\n\n\tprint \"... some string ...\" .\n \t\"... that is sometimes long ...\" .\n \"... and more but ends with $bag{'rfc2822'}\";\n\tprint_local_time(%bag); # perhaps if \"some condition\";\n\tprint \"... more string ...\";\n\nI am referring to \"if (${opts-localtime})\" in the existing code and\n\"if !gitweb_c_f('localtime')\" in this patch as \"some condition\".\n\nIt appears to me that it may be a better idea to hide the \"rfc2822\" part\nas an implementation detail behind a helper function, to make the above\npattern to look perhaps like this:\n\n\tprint \"... some string ...\" .\n \t\"... that is sometimes long ...\" .\n \"... and more but ends with \" .\n\t\ttimestamp_string(%bag, \"some condition\") .\n\t \"... more string ...\";\n\nHmm?\n"},{"id":"163662","messageId":"AANLkTi=4wyph4fp7sbtw01+eb7FV=WVN4+dGcYiov35v@mail.gmail.com","threadId":"26764","inReplyTo":"201103181846.04979.jnareb@gmail.com","subject":"Re: [PATCH 3/3 (alternate)] gitweb: Mark \"atnight\" author/committer times also for 'localtime'","fromName":"Kevin Cernekee","fromEmail":"cernekee@gmail.com","sentAt":"2011-03-18T19:07:43Z","receivedAt":"2011-03-18T19:07:43Z","isPatch":true,"sender":{"key":"cernekee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1631864?v=4"},"body":"On Fri, Mar 18, 2011 at 10:46 AM, Jakub Narebski wrote:\n> Kevin, how about something like this instead? This preserves _intent_\n> for why there is local time beside GMT time when 'localtime' is disabled\n> better, I think.\n\nFine with me. I had to dig around for a while before I could find an\n\"atnight\" commit, so I don't think this is something that is likely to\noccur often in my environment. But I can see how it would be useful\nto preserve the existing functionality.\n\nI applied your patch and verified it in both localtime=0 and\nlocaltime=1 cases. So:\n\nTested-by: Kevin Cernekee \n\n> Junio and Kevin, I am not sure if authorship should remain with Kevin,\n> or should it revert to me; the solution is quite different.\n\nI would suggest reverting it to you.\n\n> @@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {\n> my %wd = parse_date($co->{\"${who}_epoch\"}, $co->{\"${who}_tz\"});\n> print \"| $who | \" .\n> format_search_author($co->{\"${who}_name\"}, $who,\n> - esc_html($co->{\"${who}_name\"})) . \" \" .\n> + esc_html($co->{\"${who}_name\"})) . \" \" .\n> format_search_author($co->{\"${who}_email\"}, $who,\n> - esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n> + esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n\nFWIW, this does create a few >80 character lines. But\nCodingGuidelines doesn't say whether that limit applies to Perl\nscripts or just C.\n"},{"id":"163671","messageId":"7vmxksw3x8.fsf@alter.siamese.dyndns.org","threadId":"26764","inReplyTo":"201103181846.04979.jnareb@gmail.com","subject":"Re: [PATCH 3/3 (alternate)] gitweb: Mark \"atnight\" author/committer times also for 'localtime'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T20:48:35Z","receivedAt":"2011-03-18T20:48:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Kevin, how about something like this instead? This preserves _intent_\n> for why there is local time beside GMT time when 'localtime' is disabled\n> better, I think.\n>\n> Junio and Kevin, I am not sure if authorship should remain with Kevin,\n> or should it revert to me; the solution is quite different.\n> About no-change to git_print_authorship: alternate solution would be\n> to remove support for -localtime option, like in original patches.\n\nI don't think it is worth anything to keep dead code that anybody\nexercises to support -localtime option that nobody asks.\n\nI thought we were getting closer (especially if you consider my suggestion\nto the earlier round, but obviously I am biased), but this looks far worse\nthan your previous clean-up of Kevin's patch. What is the point of\nduplicating the atnight logic her? Why not kill the useless helper\nfunction \"print-local-time\", and instead enhance \"format-local-time\" so\nthat whatever this added code does is performed there when the caller asks?\n\nThen the caller here would look more or less like:\n\n\tprint \"| $who | \" .\n \"... author name, email, avatar ...\" .\n\t \" | \" .\n format_timestamp(\\%wd, gitweb_check_feature('localtime')) .\n\t \" | \\n\";\n\nand format_timestamp would be like\n\n\tsub format_timestamp {\n\t\tmy %date = %$_[0];\n my $use_localtime = $_[1];\n\t\tmy $localtime, $ret, $nite;\n\n\t\t$nite = ($date{'hour_local'} < 6);\n\n\t\tif ($use_localtime) {\n\t\t\t$ret = $date{'rfc2822_local'};\n if ($nite) {\n \t$ret = sprintf(\"%s\", $ret);\n\t\t\t}\n\t\t} else {\n\t\t\t... what the current format_local_time does to set\n\t ... including the spanning part\n $ret = \"$date{'rfc2822'} ($localtime)\";\n\t\t}\n\t\treturn $ret;\n\t}\n\nWouldn't it be much cleaner? You can then clean up the other call site of\nprint_local_time in git_print_authorship using the same helper function\n(presumably you would always pass 0 to $use_localtime there), no?\n\n> gitweb/gitweb.perl | 16 ++++++++++++----\n> 1 files changed, 12 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index cdc2a96..5bda0a8 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {\n> \t\tmy %wd = parse_date($co->{\"${who}_epoch\"}, $co->{\"${who}_tz\"});\n> \t\tprint \"| $who | \" .\n> \t\t format_search_author($co->{\"${who}_name\"}, $who,\n> -\t\t\t esc_html($co->{\"${who}_name\"})) . \" \" .\n> +\t\t esc_html($co->{\"${who}_name\"})) . \" \" .\n> \t\t format_search_author($co->{\"${who}_email\"}, $who,\n> -\t\t\t esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n> +\t\t esc_html(\"<\" . $co->{\"${who}_email\"} . \">\")) .\n> \t\t \" | \" .\n> \t\t git_get_avatar($co->{\"${who}_email\"}, -size => 'double') .\n> \t\t \" | \\n\" .\n> \t\t \"\" .\n> -\t\t \" | $wd{'rfc2822'}\";\n> -\t\tprint_local_time(%wd) if !gitweb_check_feature('localtime');\n> +\t\t \" | | \";\n> +\t\tif (gitweb_check_feature('localtime')) {\n> +\t\t\tif ($wd{'hour_local'} < 6) {\n> +\t\t\t\tprint \"$wd{'rfc2822'}\";\n> +\t\t\t} else {\n> +\t\t\t\tprint $wd{'rfc2822'};\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tprint $wd{'rfc2822'} . format_local_time(%wd);\n> +\t\t}\n> \t\tprint \" | \" .\n> \t\t \" \\n\";\n> \t}\n"},{"id":"163683","messageId":"201103182258.43368.jnareb@gmail.com","threadId":"26764","inReplyTo":"7vy64cwal7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] gitweb: introduce localtime feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-18T21:58:39Z","receivedAt":"2011-03-18T21:58:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 18 March 2011, Junio C Hamano wrote:\n> Jakub Narebski writes:\n\n> > With this feature enabled, all timestamps are shown in the local\n> > timezone instead of GMT. The timezone is taken from the appropriate\n> > timezone string stored in the commit object.\n> >\n> > This is useful if most of contributors (to a project) are based in a\n> > single office, all within the same timezone. In such case local time\n> > is more useful than GMT / UTC time that gitweb uses by default, and\n> > which is better choice for geographically scattered contributors.\n[...]\n\n> Thanks for moving the explanation up into the log message. Much easier to\n> understand the motivation.\n \n> > @@ -2930,6 +2943,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 $days[$wday], $mday, $months[$mon],\n> > +\t\t 1900+$year, $hour ,$min, $sec;\n> > +\t}\n> > \treturn %date;\n> > }\n\n> (hint: when reviewing, look a bit wider outside the context\n> provided by the patch):\n\nI was concentrating mainly on providing good commit message, but I should\nprobably review also code/change of 2/3 and 3/3 patches as a whole.\n\n> Two comments: \n> \n> - This gets seconds-since-epoch and returns a bag of pieces of formatted\n> timestamp (some are mere elements like \"hour\", some are full timestamp\n> like \"iso-8601\"). Doesn't sound like \"parse\"-date, does it?\n\nThat's right. This 2/3 commit, and even more the next 3/3 one shows that\nhandling dates in gitweb really needs refactoring. date_parse does both\nparsing date and formatting dates, something that made difficult to do\n3/3 easily and obviously.\n\nThe correct solution would be to replace current 2/3 and 3/3 by two commits:\nfirst refactoring that separates parsing date from formatting date, and\nsecond that introduces 'localtime' feature and brings changes to only \nsingle place: the new date formatting subroutine.\n\n> - It looks somewhat ugly to unconditionally assign to 'rfc2822' first\n> (before the context of the hunk) and then overwrite it. Wouldn't it be\n> more useful later to have a separate 'rfc2822_local' field, just like\n> existing 'hour_local' and 'minute_local' are counterparts for 'hour'\n> and 'minute'?\n\nAnd similar to 'iso-8601' vs 'iso-tz'.\n\nThis would not be needed with proposed by you refactoring that makes\nparse_date do only parsing of timestamp + timezone.\n \n> > @@ -3992,7 +4011,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> > \t\tprint \" | \" .\n> > \t\t \"
\\n\";\n> > \t}\n> \n> Very confusing. \"Ok, we print local time. --ah, wait, only when localtime\n> feature is not used???\"\n> \n> It turns out that the hijacking of $wd{'rfc2822'} made above already gives\n> us the local time so this patch turns the meaning of print-local-time used\n> here into additionally-print-local-time.\n\nYou are right that is very confusing.\n\n> \n> Both call sites to print_local_time() follow this pattern:\n> \n> \tprint \"... some string ...\" .\n> \t\"... that is sometimes long ...\" .\n> \"... and more but ends with $bag{'rfc2822'}\";\n> \tprint_local_time(%bag); # perhaps if \"some condition\";\n> \tprint \"... more string ...\";\n> \n> I am referring to \"if (${opts-localtime})\" in the existing code and\n> \"if !gitweb_c_f('localtime')\" in this patch as \"some condition\".\n> \n> It appears to me that it may be a better idea to hide the \"rfc2822\" part\n> as an implementation detail behind a helper function, to make the above\n> pattern to look perhaps like this:\n> \n> \tprint \"... some string ...\" .\n> \t\"... that is sometimes long ...\" .\n> \"... and more but ends with \" .\n> \t\ttimestamp_string(%bag, \"some condition\") .\n> \t \"... more string ...\";\n\nYes, it is better idea.\n\n\nNote however that gitweb uses a few different date formats, and in some\nplaces it really has to be rfc2822 and nothing else.\n\nThose formats are:\n\n * 'RFC 2822' format used by 'log' view for author date, \n no place for \"atnight\" without 'localtime' now;\n displayed with avatar (gravatar or picon) if enabled\n\n * 'RFC 2822' + local time, with optional \"atnight\" marker,\n the local time part is here to be able to put \"atnight\" warning;\n used by 'commit', 'commitdiff', 'tag' views;\n displayed with avatar (gravatar or picon) if enabled\n\nThose two can use \"atnight\", second should use \"atnight\" both for\ndefault case and when 'localtime' feature is enabled.\n\n * 'RFC 2822' format used by \"last change\" field in summary part\n of 'summary' view for a project; no avatar, but perhaps we would\n want to add relative change\n\n * Strange 'RFC 2822' + timezone in parentheses used by 'commitdiff_plain'\n view; we should probably use the same as git-format-patch, i.e. always\n localtime RFC2822 format.\n\n * 'iso-tz' format (ISO 8601, but with ' ' instead of 'Z' as timezone\n separator) is used in mouseover info in 'blame' view.\n\n * 'Short ISO format' (YYYY-MM-DD) is used together with relative date\n in many places; it is set during parse_commit. Timezone not shown,\n but GMT is used. In this place width is quite precious (longest is\n \"NN months ago\", I think).\n\n\n * RFC 2822 as value of 'Last-Modified:' HTTP header in 'feed' actions;\n I'd have to check if it should be in GMT always, and if it is RFC 2822\n or some variation. \n\n * RFC 2822 is format used for dates ('pubDate', 'lastBuildDate') for RSS\n feed format. I'm not sure if it can be localtime, or must be GMT\n\n * ISO 8601 in UTC / GMT version (with 'Z' as timezone) is used for\n 'updated' and 'published' dates in Atom feeds. I'd have to check\n if dates here must be in GMT.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"163688","messageId":"201103182328.19141.jnareb@gmail.com","threadId":"26764","inReplyTo":"7vmxksw3x8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3 (alternate)] gitweb: Mark \"atnight\" author/committer times also for 'localtime'","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-03-18T22:28:13Z","receivedAt":"2011-03-18T22:28:13Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > Kevin, how about something like this instead? This preserves _intent_\n> > for why there is local time beside GMT time when 'localtime' is disabled\n> > better, I think.\n> >\n> > Junio and Kevin, I am not sure if authorship should remain with Kevin,\n> > or should it revert to me; the solution is quite different.\n> > About no-change to git_print_authorship: alternate solution would be\n> > to remove support for -localtime option, like in original patches.\n> \n> I don't think it is worth anything to keep dead code that anybody\n> exercises to support -localtime option that nobody asks.\n\nRight. Especially that refactoring would significantly change this\ncode.\n\n> I thought we were getting closer (especially if you consider my suggestion\n> to the earlier round, but obviously I am biased), but this looks far worse\n> than your previous clean-up of Kevin's patch. What is the point of\n> duplicating the atnight logic her? Why not kill the useless helper\n> function \"print-local-time\", and instead enhance \"format-local-time\" so\n> that whatever this added code does is performed there when the caller asks?\n\nYou are right. I concentrated on the fact that IMVHO Kevin's version of\nthis 3/3 solved wrong issue (the problem is not GMT + local, but atnight\nwhich needs localtime), but forgot to stop to think how ugly and\nduplicated the code is... or at least mark this patch as RFC, request\nfor comments on resulting look.\n \n> Then the caller here would look more or less like:\n> \n> \tprint \"| $who | \" .\n> \"... author name, email, avatar ...\" .\n> \t \" | \" .\n> format_timestamp(\\%wd, gitweb_check_feature('localtime')) .\n> \t \" |
\\n\";\n\nRight.\n\n> \n> and format_timestamp would be like\n> \n> \tsub format_timestamp {\n> \t\tmy %date = %$_[0];\n> \tmy $use_localtime = $_[1];\n> \t\tmy $localtime, $ret, $nite;\n> \n> \t\t$nite = ($date{'hour_local'} < 6);\n> \n> \t\tif ($use_localtime) {\n> \t\t\t$ret = $date{'rfc2822_local'};\n> \t\tif ($nite) {\n> \t$ret = sprintf(\"%s\", $ret);\n> \t\t\t}\n> \t\t} else {\n> \t\t\t... what the current format_local_time does to set\n> \t \t... including the spanning part\n> \t$ret = \"$date{'rfc2822'} ($localtime)\";\n> \t\t}\n> \t\treturn $ret;\n> \t}\n\nWell, if we go this route, and assuming that parse_date does only parsing\nand we use separate subroutine for generating date in an rfc2822 format,\nthen we could mark only time with \"atnight\" also when 'localtime' feature\nis enabled.\n \n> Wouldn't it be much cleaner? You can then clean up the other call site of\n> print_local_time in git_print_authorship using the same helper function\n> (presumably you would always pass 0 to $use_localtime there), no?\n\nRight. Well, I'd have to think a bit about API for format_timestamp,\nbut it looks like good direction.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"163690","messageId":"7voc58rqyr.fsf@alter.siamese.dyndns.org","threadId":"26764","inReplyTo":"201103182258.43368.jnareb@gmail.com","subject":"Re: [PATCH v3 2/3] gitweb: introduce localtime feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T22:42:04Z","receivedAt":"2011-03-18T22:42:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> This would not be needed with proposed by you refactoring that makes\n> parse_date do only parsing of timestamp + timezone.\n\nI didn't mean to propose any such thing. I just pointed out that the\nfunction whose name says \"parse\" does not just parse but does more useful\nthings; I meant to hint that it might want to get renamed, not butchered\ninto smaller pieces\n"},{"id":"163698","messageId":"7vaagrsxyk.fsf@alter.siamese.dyndns.org","threadId":"26764","inReplyTo":"201103182328.19141.jnareb@gmail.com","subject":"Re: [PATCH 3/3 (alternate)] gitweb: Mark \"atnight\" author/committer times also for 'localtime'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-19T01:25:39Z","receivedAt":"2011-03-19T01:25:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Junio C Hamano wrote:\n> ...\n>> and format_timestamp would be like\n>> \n>> \tsub format_timestamp {\n>> \t\tmy %date = %$_[0];\n>> \tmy $use_localtime = $_[1];\n>> \t\tmy $localtime, $ret, $nite;\n>> \n>> \t\t$nite = ($date{'hour_local'} < 6);\n>> \n>> \t\tif ($use_localtime) {\n>> \t\t\t$ret = $date{'rfc2822_local'};\n>> \t\tif ($nite) {\n>> \t$ret = sprintf(\"%s\", $ret);\n>> \t\t\t}\n>> \t\t} else {\n>> \t\t\t... what the current format_local_time does to set\n>> \t \t... including the spanning part\n>> \t$ret = \"$date{'rfc2822'} ($localtime)\";\n>> \t\t}\n>> \t\treturn $ret;\n>> \t}\n>\n> Well, if we go this route, and assuming that parse_date does only parsing\n> and we use separate subroutine for generating date in an rfc2822 format,\n> then we could mark only time with \"atnight\" also when 'localtime' feature\n> is enabled.\n> \n>> Wouldn't it be much cleaner? You can then clean up the other call site of\n>> print_local_time in git_print_authorship using the same helper function\n>> (presumably you would always pass 0 to $use_localtime there), no?\n>\n> Right. Well, I'd have to think a bit about API for format_timestamp,\n> but it looks like good direction.\n\nI don't think there is much to think about for format_timestamp, as I was\nsuggesting to keep what comes in %date more or less the same as what the\ncurrent parse_date() generates. I was only hinting that parse_date() is\nmisnamed.\n"}]}