* [PATCH v2 2/3] gitweb: introduce localtime feature
2011-03-17 19:38 [PATCH v2 1/3] gitweb: fix #patchNN anchors when path_info is enabled Kevin Cernekee
@ 2011-03-17 19:38 ` Kevin Cernekee
2011-03-18 14:40 ` [PATCH v3 " Jakub Narebski
2011-03-17 19:38 ` [PATCH 3/3] gitweb: show alternate author/committer times Kevin Cernekee
2011-03-18 12:59 ` [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled Jakub Narebski
2 siblings, 1 reply; 17+ messages in thread
From: Kevin Cernekee @ 2011-03-17 19:38 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano; +Cc: git
With this feature enabled, all timestamps are shown in the local
timezone instead of GMT. The timezone is taken from the appropriate
timezone string stored in the commit object.
Affected views include:
summary page, last change field (commit time from latest change)
commit page, author/committer
commitdiff page, author/committer
log page, author time
No change to:
relative timestamps
patch page
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
gitweb/gitweb.perl | 21 ++++++++++++++++++++-
1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 57a3caf..bf341cb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -504,6 +504,19 @@ our %feature = (
'sub' => sub { feature_bool('remote_heads', @_) },
'override' => 0,
'default' => [0]},
+
+ # Use the author/commit localtime rather than GMT for all timestamps.
+ # Disabled by default.
+
+ # To enable system wide have in $GITWEB_CONFIG
+ # $feature{'localtime'}{'default'} = [1];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'localtime'}{'override'} = 1;
+ # and in project config gitweb.localtime = 0|1;
+ 'localtime' => {
+ 'sub' => sub { feature_bool('localtime', @_) },
+ 'override' => 0,
+ 'default' => [0]},
);
sub gitweb_get_feature {
@@ -2928,6 +2941,12 @@ sub parse_date {
$date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
1900+$year, $mon+1, $mday,
$hour, $min, $sec, $tz);
+
+ if (gitweb_check_feature('localtime')) {
+ $date{'rfc2822'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
+ $days[$wday], $mday, $months[$mon],
+ 1900+$year, $hour ,$min, $sec;
+ }
return %date;
}
@@ -3990,7 +4009,7 @@ sub git_print_authorship_rows {
"</td></tr>\n" .
"<tr>" .
"<td></td><td> $wd{'rfc2822'}";
- print_local_time(%wd);
+ print_local_time(%wd) if !gitweb_check_feature('localtime');
print "</td>" .
"</tr>\n";
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 2/3] gitweb: introduce localtime feature
2011-03-17 19:38 ` [PATCH v2 2/3] gitweb: introduce localtime feature Kevin Cernekee
@ 2011-03-18 14:40 ` Jakub Narebski
2011-03-18 18:24 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 14:40 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: Junio C Hamano, git
From: Kevin Cernekee <cernekee@gmail.com>
With this feature enabled, all timestamps are shown in the local
timezone instead of GMT. The timezone is taken from the appropriate
timezone string stored in the commit object.
This is useful if most of contributors (to a project) are based in a
single office, all within the same timezone. In such case local time
is more useful than GMT / UTC time that gitweb uses by default, and
which is better choice for geographically scattered contributors.
This change does not affect relative timestamps (e.g. "5 hours ago"),
and neither does it affect 'patch' and 'patches' views which already
use localtime because they are generated by "git format-patch".
Affected views include:
* 'summary' view, "last change" field (commit time from latest change)
* 'log' view, author time
* 'commit' and 'commitdiff' views, author/committer time
* 'tag' view, tagger time
In the case of 'commit', 'commitdiff' and 'tag' views gitweb used to
print both GMT time and time in timezone of author/tagger/comitter,
marking localtime with "atnight" as appropriate; after this commit
gitweb shows only local time. Marking localtime with "atnight" when
needed is left for subsequent commit.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from original v2 version by Kevin Cernekee:
* Expanded commit message, explaining "whys" behind introducing this new
feature (why and when can it be useful), as per
http://thread.gmane.org/gmane.comp.version-control.git/169096/focus=169284
* Minor whitespace changes
gitweb/gitweb.perl | 21 ++++++++++++++++++++-
1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3960d34..1df3652 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -504,6 +504,19 @@ our %feature = (
'sub' => sub { feature_bool('remote_heads', @_) },
'override' => 0,
'default' => [0]},
+
+ # Use the author/commit localtime rather than GMT for all timestamps.
+ # Disabled by default.
+
+ # To enable system wide have in $GITWEB_CONFIG
+ # $feature{'localtime'}{'default'} = [1];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'localtime'}{'override'} = 1;
+ # and in project config gitweb.localtime = 0|1;
+ 'localtime' => {
+ 'sub' => sub { feature_bool('localtime', @_) },
+ 'override' => 0,
+ 'default' => [0]},
);
sub gitweb_get_feature {
@@ -2930,6 +2943,12 @@ sub parse_date {
$date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
1900+$year, $mon+1, $mday,
$hour, $min, $sec, $tz);
+
+ if (gitweb_check_feature('localtime')) {
+ $date{'rfc2822'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
+ $days[$wday], $mday, $months[$mon],
+ 1900+$year, $hour ,$min, $sec;
+ }
return %date;
}
@@ -3992,7 +4011,7 @@ sub git_print_authorship_rows {
"</td></tr>\n" .
"<tr>" .
"<td></td><td> $wd{'rfc2822'}";
- print_local_time(%wd);
+ print_local_time(%wd) if !gitweb_check_feature('localtime');
print "</td>" .
"</tr>\n";
}
--
1.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/3] gitweb: introduce localtime feature
2011-03-18 14:40 ` [PATCH v3 " Jakub Narebski
@ 2011-03-18 18:24 ` Junio C Hamano
2011-03-18 21:58 ` Jakub Narebski
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2011-03-18 18:24 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kevin Cernekee, git
Jakub Narebski <jnareb@gmail.com> writes:
> From: Kevin Cernekee <cernekee@gmail.com>
>
> With this feature enabled, all timestamps are shown in the local
> timezone instead of GMT. The timezone is taken from the appropriate
> timezone string stored in the commit object.
>
> This is useful if most of contributors (to a project) are based in a
> single office, all within the same timezone. In such case local time
> is more useful than GMT / UTC time that gitweb uses by default, and
> which is better choice for geographically scattered contributors.
>
> This change does not affect relative timestamps (e.g. "5 hours ago"),
> and neither does it affect 'patch' and 'patches' views which already
> use localtime because they are generated by "git format-patch".
>
> Affected views include:
> * 'summary' view, "last change" field (commit time from latest change)
> * 'log' view, author time
> * 'commit' and 'commitdiff' views, author/committer time
> * 'tag' view, tagger time
>
> In the case of 'commit', 'commitdiff' and 'tag' views gitweb used to
> print both GMT time and time in timezone of author/tagger/comitter,
> marking localtime with "atnight" as appropriate; after this commit
> gitweb shows only local time. Marking localtime with "atnight" when
> needed is left for subsequent commit.
>
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Thanks for moving the explanation up into the log message. Much easier to
understand the motivation.
> @@ -2930,6 +2943,12 @@ sub parse_date {
> $date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
> 1900+$year, $mon+1, $mday,
> $hour, $min, $sec, $tz);
> +
> + if (gitweb_check_feature('localtime')) {
> + $date{'rfc2822'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
> + $days[$wday], $mday, $months[$mon],
> + 1900+$year, $hour ,$min, $sec;
> + }
> return %date;
> }
Two comments (hint: when reviewing, look a bit wider outside the context
provided by the patch):
- This gets seconds-since-epoch and returns a bag of pieces of formatted
timestamp (some are mere elements like "hour", some are full timestamp
like "iso-8601"). Doesn't sound like "parse"-date, does it?
- It looks somewhat ugly to unconditionally assign to 'rfc2822' first
(before the context of the hunk) and then overwrite it. Wouldn't it be
more useful later to have a separate 'rfc2822_local' field, just like
existing 'hour_local' and 'minute_local' are counterparts for 'hour'
and 'minute'?
> @@ -3992,7 +4011,7 @@ sub git_print_authorship_rows {
> "</td></tr>\n" .
> "<tr>" .
> "<td></td><td> $wd{'rfc2822'}";
> - print_local_time(%wd);
> + print_local_time(%wd) if !gitweb_check_feature('localtime');
> print "</td>" .
> "</tr>\n";
> }
Very confusing. "Ok, we print local time. --ah, wait, only when localtime
feature is not used???"
It turns out that the hijacking of $wd{'rfc2822'} made above already gives
us the local time so this patch turns the meaning of print-local-time used
here into additionally-print-local-time.
Both call sites to print_local_time() follow this pattern:
print "... some string ..." .
"... that is sometimes long ..." .
"... and more but ends with $bag{'rfc2822'}";
print_local_time(%bag); # perhaps if "some condition";
print "... more string ...";
I am referring to "if (${opts-localtime})" in the existing code and
"if !gitweb_c_f('localtime')" in this patch as "some condition".
It appears to me that it may be a better idea to hide the "rfc2822" part
as an implementation detail behind a helper function, to make the above
pattern to look perhaps like this:
print "... some string ..." .
"... that is sometimes long ..." .
"... and more but ends with " .
timestamp_string(%bag, "some condition") .
"... more string ...";
Hmm?
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/3] gitweb: introduce localtime feature
2011-03-18 18:24 ` Junio C Hamano
@ 2011-03-18 21:58 ` Jakub Narebski
2011-03-18 22:42 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 21:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Cernekee, git
On Fri, 18 March 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> > With this feature enabled, all timestamps are shown in the local
> > timezone instead of GMT. The timezone is taken from the appropriate
> > timezone string stored in the commit object.
> >
> > This is useful if most of contributors (to a project) are based in a
> > single office, all within the same timezone. In such case local time
> > is more useful than GMT / UTC time that gitweb uses by default, and
> > which is better choice for geographically scattered contributors.
[...]
> Thanks for moving the explanation up into the log message. Much easier to
> understand the motivation.
> > @@ -2930,6 +2943,12 @@ sub parse_date {
> > $date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
> > 1900+$year, $mon+1, $mday,
> > $hour, $min, $sec, $tz);
> > +
> > + if (gitweb_check_feature('localtime')) {
> > + $date{'rfc2822'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
> > + $days[$wday], $mday, $months[$mon],
> > + 1900+$year, $hour ,$min, $sec;
> > + }
> > return %date;
> > }
> (hint: when reviewing, look a bit wider outside the context
> provided by the patch):
I was concentrating mainly on providing good commit message, but I should
probably review also code/change of 2/3 and 3/3 patches as a whole.
> Two comments:
>
> - This gets seconds-since-epoch and returns a bag of pieces of formatted
> timestamp (some are mere elements like "hour", some are full timestamp
> like "iso-8601"). Doesn't sound like "parse"-date, does it?
That's right. This 2/3 commit, and even more the next 3/3 one shows that
handling dates in gitweb really needs refactoring. date_parse does both
parsing date and formatting dates, something that made difficult to do
3/3 easily and obviously.
The correct solution would be to replace current 2/3 and 3/3 by two commits:
first refactoring that separates parsing date from formatting date, and
second that introduces 'localtime' feature and brings changes to only
single place: the new date formatting subroutine.
> - It looks somewhat ugly to unconditionally assign to 'rfc2822' first
> (before the context of the hunk) and then overwrite it. Wouldn't it be
> more useful later to have a separate 'rfc2822_local' field, just like
> existing 'hour_local' and 'minute_local' are counterparts for 'hour'
> and 'minute'?
And similar to 'iso-8601' vs 'iso-tz'.
This would not be needed with proposed by you refactoring that makes
parse_date do only parsing of timestamp + timezone.
> > @@ -3992,7 +4011,7 @@ sub git_print_authorship_rows {
> > "</td></tr>\n" .
> > "<tr>" .
> > "<td></td><td> $wd{'rfc2822'}";
> > - print_local_time(%wd);
> > + print_local_time(%wd) if !gitweb_check_feature('localtime');
> > print "</td>" .
> > "</tr>\n";
> > }
>
> Very confusing. "Ok, we print local time. --ah, wait, only when localtime
> feature is not used???"
>
> It turns out that the hijacking of $wd{'rfc2822'} made above already gives
> us the local time so this patch turns the meaning of print-local-time used
> here into additionally-print-local-time.
You are right that is very confusing.
>
> Both call sites to print_local_time() follow this pattern:
>
> print "... some string ..." .
> "... that is sometimes long ..." .
> "... and more but ends with $bag{'rfc2822'}";
> print_local_time(%bag); # perhaps if "some condition";
> print "... more string ...";
>
> I am referring to "if (${opts-localtime})" in the existing code and
> "if !gitweb_c_f('localtime')" in this patch as "some condition".
>
> It appears to me that it may be a better idea to hide the "rfc2822" part
> as an implementation detail behind a helper function, to make the above
> pattern to look perhaps like this:
>
> print "... some string ..." .
> "... that is sometimes long ..." .
> "... and more but ends with " .
> timestamp_string(%bag, "some condition") .
> "... more string ...";
Yes, it is better idea.
Note however that gitweb uses a few different date formats, and in some
places it really has to be rfc2822 and nothing else.
Those formats are:
* 'RFC 2822' format used by 'log' view for author date,
no place for "atnight" without 'localtime' now;
displayed with avatar (gravatar or picon) if enabled
* 'RFC 2822' + local time, with optional "atnight" marker,
the local time part is here to be able to put "atnight" warning;
used by 'commit', 'commitdiff', 'tag' views;
displayed with avatar (gravatar or picon) if enabled
Those two can use "atnight", second should use "atnight" both for
default case and when 'localtime' feature is enabled.
* 'RFC 2822' format used by "last change" field in summary part
of 'summary' view for a project; no avatar, but perhaps we would
want to add relative change
* Strange 'RFC 2822' + timezone in parentheses used by 'commitdiff_plain'
view; we should probably use the same as git-format-patch, i.e. always
localtime RFC2822 format.
* 'iso-tz' format (ISO 8601, but with ' ' instead of 'Z' as timezone
separator) is used in mouseover info in 'blame' view.
* 'Short ISO format' (YYYY-MM-DD) is used together with relative date
in many places; it is set during parse_commit. Timezone not shown,
but GMT is used. In this place width is quite precious (longest is
"NN months ago", I think).
* RFC 2822 as value of 'Last-Modified:' HTTP header in 'feed' actions;
I'd have to check if it should be in GMT always, and if it is RFC 2822
or some variation.
* RFC 2822 is format used for dates ('pubDate', 'lastBuildDate') for RSS
feed format. I'm not sure if it can be localtime, or must be GMT
* ISO 8601 in UTC / GMT version (with 'Z' as timezone) is used for
'updated' and 'published' dates in Atom feeds. I'd have to check
if dates here must be in GMT.
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/3] gitweb: introduce localtime feature
2011-03-18 21:58 ` Jakub Narebski
@ 2011-03-18 22:42 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2011-03-18 22:42 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, Kevin Cernekee, git
Jakub Narebski <jnareb@gmail.com> writes:
> This would not be needed with proposed by you refactoring that makes
> parse_date do only parsing of timestamp + timezone.
I didn't mean to propose any such thing. I just pointed out that the
function whose name says "parse" does not just parse but does more useful
things; I meant to hint that it might want to get renamed, not butchered
into smaller pieces
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] gitweb: show alternate author/committer times
2011-03-17 19:38 [PATCH v2 1/3] gitweb: fix #patchNN anchors when path_info is enabled Kevin Cernekee
2011-03-17 19:38 ` [PATCH v2 2/3] gitweb: introduce localtime feature Kevin Cernekee
@ 2011-03-17 19:38 ` Kevin Cernekee
2011-03-18 17:46 ` [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime' Jakub Narebski
2011-03-18 12:59 ` [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled Jakub Narebski
2 siblings, 1 reply; 17+ messages in thread
From: Kevin Cernekee @ 2011-03-17 19:38 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano; +Cc: git
On the commit/commitdiff views, show the author/committer times as
follows:
If $feature{'localtime'} is disabled, display the RFC 2822 date/time in
GMT, then print (HH:MM -TZ) in the author's/committer's local timezone.
(i.e. no change to the current behavior)
If $feature{'localtime'} is enabled, display the RFC 2822 date/time in
the author's/committer's local timezone, then print (HH:MM +0000) in GMT.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
gitweb/gitweb.perl | 24 ++++++++++++++----------
1 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 578edc0..6b8f9a7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3953,22 +3953,27 @@ sub git_print_section {
print $cgi->end_div;
}
-sub print_local_time {
- print format_local_time(@_);
-}
-
-sub format_local_time {
+# If localtime is disabled, show the server's local time in parentheses.
+# If localtime is enabled, show GMT in parentheses.
+sub print_alt_time {
my $localtime = '';
my %date = @_;
+ my ($h, $m, $tz) = ($date{'hour_local'}, $date{'minute_local'},
+ $date{'tz_local'});
+
+ if (gitweb_check_feature('localtime')) {
+ ($h, $m, $tz) = ($date{'hour'}, $date{'minute'}, "+0000");
+ }
+
if ($date{'hour_local'} < 6) {
$localtime .= sprintf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
- $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+ $h, $m, $tz);
} else {
$localtime .= sprintf(" (%02d:%02d %s)",
- $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+ $h, $m, $tz);
}
- return $localtime;
+ print $localtime;
}
# Outputs the author name and date in long form
@@ -3982,7 +3987,6 @@ sub git_print_authorship {
print "<$tag class=\"author_date\">" .
format_search_author($author, "author", esc_html($author)) .
" [$ad{'rfc2822'}";
- print_local_time(%ad) if ($opts{-localtime});
print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
. "</$tag>\n";
}
@@ -4009,7 +4013,7 @@ sub git_print_authorship_rows {
"</td></tr>\n" .
"<tr>" .
"<td></td><td> $wd{'rfc2822'}";
- print_local_time(%wd) if !gitweb_check_feature('localtime');
+ print_alt_time(%wd);
print "</td>" .
"</tr>\n";
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'
2011-03-17 19:38 ` [PATCH 3/3] gitweb: show alternate author/committer times Kevin Cernekee
@ 2011-03-18 17:46 ` Jakub Narebski
2011-03-18 19:07 ` Kevin Cernekee
2011-03-18 20:48 ` Junio C Hamano
0 siblings, 2 replies; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 17:46 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: Junio C Hamano, git
From: Kevin Cernekee <cernekee@gmail.com>
By default, with 'localtime' feature disabled, the dates in 'commit',
'commitdiff' and 'tag' views show both GMT time, and localtime in
recorded author/committer/tagger timezone, marking localtime with
"atnight" class to notify times between 0 and 6 AM local time.
An example output can look like this:
author A U Thor <author@example.com>
Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
^^^^^
where underlined part is marked with "atnight" class (in red with
default stylesheet).
If $feature{'localtime'} is enabled, we display the RFC 2822 date/time
in the author's/committer's/tagger's local timezone; previous commit
removed marking "atnight" times, because there wasn't separate local
time to mark up after GMT time.
This commit makes gitweb mark _whole_ RFC 2822 date/time with
"atnight" class for times between 0 and 6 AM.
An example output can look like this:
author A U Thor <author@example.com>
Wed, 16 Mar 2011 02:02:42 -0500
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
where again underlined part is marked with "atnight".
We probably should mark only time part of RFC 2822 date/time with
"atnight" class, but such solution would be more involved.
While at it fix whitespace, using spaces for align, tabs for indent.
NOTE that git_print_authorship subroutine is for now left as is; there
is no caller in gitweb that uses it with -localtime=>1.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Kevin, how about something like this instead? This preserves _intent_
for why there is local time beside GMT time when 'localtime' is disabled
better, I think.
Junio and Kevin, I am not sure if authorship should remain with Kevin,
or should it revert to me; the solution is quite different.
About no-change to git_print_authorship: alternate solution would be
to remove support for -localtime option, like in original patches.
gitweb/gitweb.perl | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cdc2a96..5bda0a8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {
my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
print "<tr><td>$who</td><td>" .
format_search_author($co->{"${who}_name"}, $who,
- esc_html($co->{"${who}_name"})) . " " .
+ esc_html($co->{"${who}_name"})) . " " .
format_search_author($co->{"${who}_email"}, $who,
- esc_html("<" . $co->{"${who}_email"} . ">")) .
+ esc_html("<" . $co->{"${who}_email"} . ">")) .
"</td><td rowspan=\"2\">" .
git_get_avatar($co->{"${who}_email"}, -size => 'double') .
"</td></tr>\n" .
"<tr>" .
- "<td></td><td> $wd{'rfc2822'}";
- print_local_time(%wd) if !gitweb_check_feature('localtime');
+ "<td></td><td> ";
+ if (gitweb_check_feature('localtime')) {
+ if ($wd{'hour_local'} < 6) {
+ print "<span class=\"atnight\">$wd{'rfc2822'}</span>";
+ } else {
+ print $wd{'rfc2822'};
+ }
+ } else {
+ print $wd{'rfc2822'} . format_local_time(%wd);
+ }
print "</td>" .
"</tr>\n";
}
--
1.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'
2011-03-18 17:46 ` [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime' Jakub Narebski
@ 2011-03-18 19:07 ` Kevin Cernekee
2011-03-18 20:48 ` Junio C Hamano
1 sibling, 0 replies; 17+ messages in thread
From: Kevin Cernekee @ 2011-03-18 19:07 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
On Fri, Mar 18, 2011 at 10:46 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Kevin, how about something like this instead? This preserves _intent_
> for why there is local time beside GMT time when 'localtime' is disabled
> better, I think.
Fine with me. I had to dig around for a while before I could find an
"atnight" commit, so I don't think this is something that is likely to
occur often in my environment. But I can see how it would be useful
to preserve the existing functionality.
I applied your patch and verified it in both localtime=0 and
localtime=1 cases. So:
Tested-by: Kevin Cernekee <cernekee@gmail.com>
> Junio and Kevin, I am not sure if authorship should remain with Kevin,
> or should it revert to me; the solution is quite different.
I would suggest reverting it to you.
> @@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {
> my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
> print "<tr><td>$who</td><td>" .
> format_search_author($co->{"${who}_name"}, $who,
> - esc_html($co->{"${who}_name"})) . " " .
> + esc_html($co->{"${who}_name"})) . " " .
> format_search_author($co->{"${who}_email"}, $who,
> - esc_html("<" . $co->{"${who}_email"} . ">")) .
> + esc_html("<" . $co->{"${who}_email"} . ">")) .
FWIW, this does create a few >80 character lines. But
CodingGuidelines doesn't say whether that limit applies to Perl
scripts or just C.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'
2011-03-18 17:46 ` [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime' Jakub Narebski
2011-03-18 19:07 ` Kevin Cernekee
@ 2011-03-18 20:48 ` Junio C Hamano
2011-03-18 22:28 ` Jakub Narebski
1 sibling, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2011-03-18 20:48 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kevin Cernekee, git
Jakub Narebski <jnareb@gmail.com> writes:
> Kevin, how about something like this instead? This preserves _intent_
> for why there is local time beside GMT time when 'localtime' is disabled
> better, I think.
>
> Junio and Kevin, I am not sure if authorship should remain with Kevin,
> or should it revert to me; the solution is quite different.
> About no-change to git_print_authorship: alternate solution would be
> to remove support for -localtime option, like in original patches.
I don't think it is worth anything to keep dead code that anybody
exercises to support -localtime option that nobody asks.
I thought we were getting closer (especially if you consider my suggestion
to the earlier round, but obviously I am biased), but this looks far worse
than your previous clean-up of Kevin's patch. What is the point of
duplicating the atnight logic her? Why not kill the useless helper
function "print-local-time", and instead enhance "format-local-time" so
that whatever this added code does is performed there when the caller asks?
Then the caller here would look more or less like:
print "<tr><td>$who</td>" .
"... author name, email, avatar ..." .
"<td></td><td>" .
format_timestamp(\%wd, gitweb_check_feature('localtime')) .
"</td></tr>\n";
and format_timestamp would be like
sub format_timestamp {
my %date = %$_[0];
my $use_localtime = $_[1];
my $localtime, $ret, $nite;
$nite = ($date{'hour_local'} < 6);
if ($use_localtime) {
$ret = $date{'rfc2822_local'};
if ($nite) {
$ret = sprintf("<span class='atnight'>%s</span>", $ret);
}
} else {
... what the current format_local_time does to set
... including the spanning part
$ret = "$date{'rfc2822'} ($localtime)";
}
return $ret;
}
Wouldn't it be much cleaner? You can then clean up the other call site of
print_local_time in git_print_authorship using the same helper function
(presumably you would always pass 0 to $use_localtime there), no?
> gitweb/gitweb.perl | 16 ++++++++++++----
> 1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cdc2a96..5bda0a8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {
> my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
> print "<tr><td>$who</td><td>" .
> format_search_author($co->{"${who}_name"}, $who,
> - esc_html($co->{"${who}_name"})) . " " .
> + esc_html($co->{"${who}_name"})) . " " .
> format_search_author($co->{"${who}_email"}, $who,
> - esc_html("<" . $co->{"${who}_email"} . ">")) .
> + esc_html("<" . $co->{"${who}_email"} . ">")) .
> "</td><td rowspan=\"2\">" .
> git_get_avatar($co->{"${who}_email"}, -size => 'double') .
> "</td></tr>\n" .
> "<tr>" .
> - "<td></td><td> $wd{'rfc2822'}";
> - print_local_time(%wd) if !gitweb_check_feature('localtime');
> + "<td></td><td> ";
> + if (gitweb_check_feature('localtime')) {
> + if ($wd{'hour_local'} < 6) {
> + print "<span class=\"atnight\">$wd{'rfc2822'}</span>";
> + } else {
> + print $wd{'rfc2822'};
> + }
> + } else {
> + print $wd{'rfc2822'} . format_local_time(%wd);
> + }
> print "</td>" .
> "</tr>\n";
> }
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'
2011-03-18 20:48 ` Junio C Hamano
@ 2011-03-18 22:28 ` Jakub Narebski
2011-03-19 1:25 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Cernekee, git
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Kevin, how about something like this instead? This preserves _intent_
> > for why there is local time beside GMT time when 'localtime' is disabled
> > better, I think.
> >
> > Junio and Kevin, I am not sure if authorship should remain with Kevin,
> > or should it revert to me; the solution is quite different.
> > About no-change to git_print_authorship: alternate solution would be
> > to remove support for -localtime option, like in original patches.
>
> I don't think it is worth anything to keep dead code that anybody
> exercises to support -localtime option that nobody asks.
Right. Especially that refactoring would significantly change this
code.
> I thought we were getting closer (especially if you consider my suggestion
> to the earlier round, but obviously I am biased), but this looks far worse
> than your previous clean-up of Kevin's patch. What is the point of
> duplicating the atnight logic her? Why not kill the useless helper
> function "print-local-time", and instead enhance "format-local-time" so
> that whatever this added code does is performed there when the caller asks?
You are right. I concentrated on the fact that IMVHO Kevin's version of
this 3/3 solved wrong issue (the problem is not GMT + local, but atnight
which needs localtime), but forgot to stop to think how ugly and
duplicated the code is... or at least mark this patch as RFC, request
for comments on resulting look.
> Then the caller here would look more or less like:
>
> print "<tr><td>$who</td>" .
> "... author name, email, avatar ..." .
> "<td></td><td>" .
> format_timestamp(\%wd, gitweb_check_feature('localtime')) .
> "</td></tr>\n";
Right.
>
> and format_timestamp would be like
>
> sub format_timestamp {
> my %date = %$_[0];
> my $use_localtime = $_[1];
> my $localtime, $ret, $nite;
>
> $nite = ($date{'hour_local'} < 6);
>
> if ($use_localtime) {
> $ret = $date{'rfc2822_local'};
> if ($nite) {
> $ret = sprintf("<span class='atnight'>%s</span>", $ret);
> }
> } else {
> ... what the current format_local_time does to set
> ... including the spanning part
> $ret = "$date{'rfc2822'} ($localtime)";
> }
> return $ret;
> }
Well, if we go this route, and assuming that parse_date does only parsing
and we use separate subroutine for generating date in an rfc2822 format,
then we could mark only time with "atnight" also when 'localtime' feature
is enabled.
> Wouldn't it be much cleaner? You can then clean up the other call site of
> print_local_time in git_print_authorship using the same helper function
> (presumably you would always pass 0 to $use_localtime there), no?
Right. Well, I'd have to think a bit about API for format_timestamp,
but it looks like good direction.
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'
2011-03-18 22:28 ` Jakub Narebski
@ 2011-03-19 1:25 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2011-03-19 1:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kevin Cernekee, git
Jakub Narebski <jnareb@gmail.com> writes:
> Junio C Hamano wrote:
> ...
>> and format_timestamp would be like
>>
>> sub format_timestamp {
>> my %date = %$_[0];
>> my $use_localtime = $_[1];
>> my $localtime, $ret, $nite;
>>
>> $nite = ($date{'hour_local'} < 6);
>>
>> if ($use_localtime) {
>> $ret = $date{'rfc2822_local'};
>> if ($nite) {
>> $ret = sprintf("<span class='atnight'>%s</span>", $ret);
>> }
>> } else {
>> ... what the current format_local_time does to set
>> ... including the spanning part
>> $ret = "$date{'rfc2822'} ($localtime)";
>> }
>> return $ret;
>> }
>
> Well, if we go this route, and assuming that parse_date does only parsing
> and we use separate subroutine for generating date in an rfc2822 format,
> then we could mark only time with "atnight" also when 'localtime' feature
> is enabled.
>
>> Wouldn't it be much cleaner? You can then clean up the other call site of
>> print_local_time in git_print_authorship using the same helper function
>> (presumably you would always pass 0 to $use_localtime there), no?
>
> Right. Well, I'd have to think a bit about API for format_timestamp,
> but it looks like good direction.
I don't think there is much to think about for format_timestamp, as I was
suggesting to keep what comes in %date more or less the same as what the
current parse_date() generates. I was only hinting that parse_date() is
misnamed.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled
2011-03-17 19:38 [PATCH v2 1/3] gitweb: fix #patchNN anchors when path_info is enabled Kevin Cernekee
2011-03-17 19:38 ` [PATCH v2 2/3] gitweb: introduce localtime feature Kevin Cernekee
2011-03-17 19:38 ` [PATCH 3/3] gitweb: show alternate author/committer times Kevin Cernekee
@ 2011-03-18 12:59 ` Jakub Narebski
2011-03-18 15:25 ` Kevin Cernekee
2011-03-18 16:57 ` [PATCH v3 " Junio C Hamano
2 siblings, 2 replies; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 12:59 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: Junio C Hamano, git
From: Kevin Cernekee <cernekee@gmail.com>
When $feature{'pathinfo'} is used, gitweb script sets the base URL to
itself, so that relative links to static files work correctly. It
does it by adding something like below to HTML head:
<base href="http://HOST/gitweb.cgi">
This breaks the "patch" anchor links seen on the commitdiff pages,
because these links, being relative (<a href="#patch1">), are resolved
(computed) relative to the base URL and not relative to current URL,
i.e. as:
http://HOST/gitweb.cgi#patch1
Instead, they should look like this:
http://HOST/gitweb.cgi/myproject.git/commitdiff/35a9811ef9d68eae9afd76bede121da4f89b448c#patch1
Add an "-anchor" parameter to href(), and use href(-anchor=>"patch1")
to generate "patch" anchor links, so that the full path is included in
the patch link.
While at it, convert
print "foo";
print "bar";
to
print "foo" .
"bar";
in the neighborhood of changes.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I like v2 version, and had only small corrections. For you to not have
to resend this patch once again, I have added those corrections and sent
them as a patch myself.
Changes from original v2 version from Kevin Cernekee:
* Fix (re-add) accidentally lost in v2 '"<td class=\"link\">"' in first
chunk using href(-anchor => ...)
* Reword commit message (hopefully make it better, and not only longer)
* Move code that sets implicit -replay when -anchor is the only parameter
lower, and change order of subexpression, for better possible future
extendability, if there would be other cases when we would want to
automatically turn on -replay.
* Change indenting of code (whitespace-only change).
* Add 'while at it' fixup.
gitweb/gitweb.perl | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b04ab8c..3960d34 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1199,6 +1199,7 @@ if (defined caller) {
# -full => 0|1 - use absolute/full URL ($my_uri/$my_url as base)
# -replay => 1 - start from a current view (replay with modifications)
# -path_info => 0|1 - don't use/use path_info URL (if possible)
+# -anchor => ANCHOR - add #ANCHOR to end of URL, implies -replay if used alone
sub href {
my %params = @_;
# default is to use -absolute url() i.e. $my_uri
@@ -1206,6 +1207,9 @@ sub href {
$params{'project'} = $project unless exists $params{'project'};
+ # implicit -replay
+ $params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
+
if ($params{-replay}) {
while (my ($name, $symbol) = each %cgi_param_mapping) {
if (!exists $params{$name}) {
@@ -1314,6 +1318,10 @@ sub href {
# final transformation: trailing spaces must be escaped (URI-encoded)
$href =~ s/(\s+)$/CGI::escape($1)/e;
+ if ($params{-anchor}) {
+ $href .= "#".esc_param($params{-anchor});
+ }
+
return $href;
}
@@ -4335,7 +4343,8 @@ sub git_difftree_body {
# link to patch
$patchno++;
print "<td class=\"link\">" .
- $cgi->a({-href => "#patch$patchno"}, "patch") .
+ $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | " .
"</td>\n";
}
@@ -4432,8 +4441,9 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch");
- print " | ";
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
+ " | ";
}
print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
hash_base=>$hash, file_name=>$diff->{'file'})},
@@ -4452,8 +4462,9 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch");
- print " | ";
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
+ " | ";
}
print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'from_id'},
hash_base=>$parent, file_name=>$diff->{'file'})},
@@ -4494,7 +4505,8 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch") .
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | ";
} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
# "commit" view and modified file (not onlu mode changed)
@@ -4539,7 +4551,8 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch") .
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | ";
} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
# "commit" view and modified file (not only pure rename or copy)
--
1.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled
2011-03-18 12:59 ` [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled Jakub Narebski
@ 2011-03-18 15:25 ` Kevin Cernekee
2011-03-18 16:00 ` [PATCH v3 (amend) " Jakub Narebski
2011-03-18 16:57 ` [PATCH v3 " Junio C Hamano
1 sibling, 1 reply; 17+ messages in thread
From: Kevin Cernekee @ 2011-03-18 15:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
On Fri, Mar 18, 2011 at 5:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> $params{'project'} = $project unless exists $params{'project'};
>
> + # implicit -replay
> + $params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
If this test occurs after $params{'project'} is set, it needs to count
both 'project' and '-anchor':
> + $params{-replay} = 1 if (keys %params == 2 && $params{-anchor});
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3 (amend) 1/3] gitweb: fix #patchNN anchors when path_info is enabled
2011-03-18 15:25 ` Kevin Cernekee
@ 2011-03-18 16:00 ` Jakub Narebski
0 siblings, 0 replies; 17+ messages in thread
From: Jakub Narebski @ 2011-03-18 16:00 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: Junio C Hamano, git
Kevin Cernekee wrotr:
> On Fri, Mar 18, 2011 at 5:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> > $params{'project'} = $project unless exists $params{'project'};
> >
> > + # implicit -replay
> > + $params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
>
> If this test occurs after $params{'project'} is set, it needs to count
> both 'project' and '-anchor':
Right. I'm sorry about that.
> > + $params{-replay} = 1 if (keys %params == 2 && $params{-anchor});
The above is not a good solution, as it hides the fact that -anchor must
be only parameter for trigger implicit -replay.
-- >8 --
From: Kevin Cernekee <cernekee@gmail.com>
Subject: [PATCH] gitweb: fix #patchNN anchors when path_info is enabled
When $feature{'pathinfo'} is used, gitweb script sets the base URL to
itself, so that relative links to static files work correctly. It
does it by adding something like below to HTML head:
<base href="http://HOST/gitweb.cgi">
This breaks the "patch" anchor links seen on the commitdiff pages,
because these links, being relative (<a href="#patch1">), are resolved
(computed) relative to the base URL and not relative to current URL,
i.e. as:
http://HOST/gitweb.cgi#patch1
Instead, they should look like this:
http://HOST/gitweb.cgi/myproject.git/commitdiff/35a9811ef9d68eae9afd76bede121da4f89b448c#patch1
Add an "-anchor" parameter to href(), and use href(-anchor=>"patch1")
to generate "patch" anchor links, so that the full path is included in
the patch link.
While at it, convert
print "foo";
print "bar";
to
print "foo" .
"bar";
in the neighborhood of changes.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
gitweb/gitweb.perl | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b04ab8c..f275adb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1199,11 +1199,15 @@ if (defined caller) {
# -full => 0|1 - use absolute/full URL ($my_uri/$my_url as base)
# -replay => 1 - start from a current view (replay with modifications)
# -path_info => 0|1 - don't use/use path_info URL (if possible)
+# -anchor => ANCHOR - add #ANCHOR to end of URL, implies -replay if used alone
sub href {
my %params = @_;
# default is to use -absolute url() i.e. $my_uri
my $href = $params{-full} ? $my_url : $my_uri;
+ # implicit -replay, must be first of implicit params
+ $params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
+
$params{'project'} = $project unless exists $params{'project'};
if ($params{-replay}) {
@@ -1314,6 +1318,10 @@ sub href {
# final transformation: trailing spaces must be escaped (URI-encoded)
$href =~ s/(\s+)$/CGI::escape($1)/e;
+ if ($params{-anchor}) {
+ $href .= "#".esc_param($params{-anchor});
+ }
+
return $href;
}
@@ -4335,7 +4343,8 @@ sub git_difftree_body {
# link to patch
$patchno++;
print "<td class=\"link\">" .
- $cgi->a({-href => "#patch$patchno"}, "patch") .
+ $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | " .
"</td>\n";
}
@@ -4432,8 +4441,9 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch");
- print " | ";
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
+ " | ";
}
print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
hash_base=>$hash, file_name=>$diff->{'file'})},
@@ -4452,8 +4462,9 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch");
- print " | ";
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
+ " | ";
}
print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'from_id'},
hash_base=>$parent, file_name=>$diff->{'file'})},
@@ -4494,7 +4505,8 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch") .
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | ";
} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
# "commit" view and modified file (not onlu mode changed)
@@ -4539,7 +4551,8 @@ sub git_difftree_body {
if ($action eq 'commitdiff') {
# link to patch
$patchno++;
- print $cgi->a({-href => "#patch$patchno"}, "patch") .
+ print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+ "patch") .
" | ";
} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
# "commit" view and modified file (not only pure rename or copy)
--
1.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled
2011-03-18 12:59 ` [PATCH v3 1/3] gitweb: fix #patchNN anchors when path_info is enabled Jakub Narebski
2011-03-18 15:25 ` Kevin Cernekee
@ 2011-03-18 16:57 ` Junio C Hamano
2011-03-18 17:18 ` Jakub Narebski
1 sibling, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2011-03-18 16:57 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kevin Cernekee, git
Jakub Narebski <jnareb@gmail.com> writes:
> I like v2 version, and had only small corrections. For you to not have
> to resend this patch once again, I have added those corrections and sent
> them as a patch myself.
>
> Changes from original v2 version from Kevin Cernekee:
>
> * Fix (re-add) accidentally lost in v2 '"<td class=\"link\">"' in first
> chunk using href(-anchor => ...)
> * Reword commit message (hopefully make it better, and not only longer)
Thanks.
> * Move code that sets implicit -replay when -anchor is the only parameter
> lower, and change order of subexpression, for better possible future
> extendability, if there would be other cases when we would want to
> automatically turn on -replay.
I think you meant this part:
# implicit -replay
$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
But I don't think it is that much of an improvement as long as it uses the
statement modifier syntax ("A if B") for a thing like this.
I will not bother rewriting the commit I made out of this patch myself,
but the next person who wants to add the auto-replay for his option will
first have to rewrite the above into:
if (key %params == 1 && $params{-anchor}) {
$params{-replay} = 1;
}
before turning it into:
if ((key %params == 1 && $params{-anchor}) ||
("here comes my new condition")) {
$params{-replay} = 1;
}
anyway. If your rewrite were to a plain-vanilla "if () { ... }", it would
have been a real improvement.
Besides, I find it a bad taste to use statement modifier syntax unless the
modifying condition ("if B" part) is much more likely to hold true than
false.
If you limit your use of statement modifiers for conditions that almost
always hold true, it will become much easier to scan the code for the
first time, because you can skim through and almost ignore the condition
part to follow the logic for the normal case. But turning 'replay' on in a
specific narrow case (and later in a specific set of narrow cases) is a
complete opposite of normal codeflow.
Anyway, thanks for the clean-up. Applied.
^ permalink raw reply [flat|nested] 17+ messages in thread