Petr "Pasky" Baudis wrote:
> Dear diary, on Thu, Sep 21, 2006 at 11:56:31PM CEST, I got a letter
> where Jakub Narebski <jnareb@xxxxxxxxx> said that...
> > + # The committag subroutine is called with match for pattern,
> > + # and options if they are defined. Match is replaced by return
> > + # value of committag-sub.
>
> You should note that the pattern matches in an already HTML-escaped
> text. Which is actually quite ugly especially wrt. the , perhaps
> it would be worth considering converting the spaces after this? (The
> Marc links would need fixing tho'.)
Perhaps we better use "white-space: pre;" for commit (and tag) messages
instead of 'manually' doing this via converting ' ' to ' '.
> ..snip..
> > + 'message_id' => {
> > + 'pattern' => qr/(message|msg)[- ]?id <([^&]*)&rt;/i,
> > + 'enabled' => 1,
> > + 'options' => [
> > + 'http://news.gmane.org/find-root.php?message_id=',
> > + \"e_msgid_gmane ],
> > + 'sub' => \&tag_msgid},
>
> Mention quote_msgid_marc() (and the URL) in a comment so that it's not
> completely unreferenced?
Well, that is RFC. I'm not sure if the final patch would have all those
committags, all those committags but disabled with exception of commitsha/sha,
or just commitsha tag in source and the rest in commit message as examples.
For MARC:
'options' => [
'http://marc.theaimsgroup.com/?i=',
\"e_msgid_marc ],
> > +);
> ..snip..
> > +sub quote_msgid_gmane {
> > + my $msgid = shift || return;
> > +
> > + return '<'.(quotemeta $msgid).'>';
> > +}
>
> <> should be HTML-escaped (unless CGI::a() does that).
CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
return '%3c' . (quotemeta $msgid) . '%3e';
(Quotemeta is because Message-Id contains '@').
> > @@ -577,18 +693,43 @@ ## which don't beling to other sections
> ..snip..
> > - if ($line =~ m/([0-9a-fA-F]{40})/) {
> > - my $hash_text = $1;
> > - if (git_get_type($hash_text) eq "commit") {
> > - my $link =
> > - $cgi->a({-href => href(action=>"commit", hash=>$hash_text),
> > - -class => "text"}, $hash_text);
> > - $line =~ s/$hash_text/$link/;
> > +
> > + foreach my $ct (@tags) {
> > + next unless exists $committags{$ct};
>
> At this point, I'd complain (even die) rather than silently pass,
> that's definitely a bug.
I've programmed defensively. Perhaps too defensively.
> > + my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
>
> $a_attr can't be but a hashref, that test is redundant.
$a_attr can be undefined or be a hashref. It could theoretically
be empty hashref, but that is a mistake (<a>...</a>).
> > @@ -626,12 +767,13 @@ sub format_subject_html {
> > $extra = '' unless defined($extra);
> >
> > if (length($short) < length($long)) {
> > - return $cgi->a({-href => $href, -class => "list subject",
> > - -title => $long},
> > - esc_html($short) . $extra);
> > + my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> > + return $cgi->a($a_attr,
> > + format_log_line_html($short, $a_attr, @subjecttags) . $extra);
> > } else {
> > - return $cgi->a({-href => $href, -class => "list subject"},
> > - esc_html($long) . $extra);
> > + my $a_attr = {-href => $href, -class => "list subject"};
> > + return $cgi->a($a_attr,
> > + format_log_line_html($long, $a_attr, @subjecttags) . $extra);
> > }
> > }
> >
>
> Subjects are often clickable and we don't want links in those.
The extra code with $a_attr is to have links within links. It works
quite well, I'd say. The subject link is broken, and the committag
link is inserted in the break (gitweb-xmms2 committag code did the same,
but did not preserved all the subject link attributes, like title or class,
only the target of the link).
The result is somethink like:
<a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a>
> > @@ -693,9 +835,9 @@ sub git_get_type {
> >
> > open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
> > my $type = <$fd>;
> > - close $fd or return;
> > + close $fd or return "unknown";
> > chomp $type;
> > - return $type;
> > + return ($type || "unknown");
> > }
> >
> > sub git_get_project_config {
>
> > P.S. I've corrected git_get_type for comparison
> > git_get_type($hash) eq "commit"
> > to work without Perl syntax errors.
>
> Hey it's just a warning. ;-) And if you are calling git_get_type() on a
> non-existing object, don't you have a problem you should better know
> about? Couldn't this break git_get_refs_list()?
There are many cherry-picked comments with commitsha of object which
existed on some topic branch, was pruned, and does not exist anymore.
Perhaps we should add "2> /dev/null", too...
I've not tested how this change works with git_get_refs_list(), and other
subroutines that use git_get_type().
> > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
> > $empty = 0;
> > }
> >
> > - print format_log_line_html($line) . "<br/>\n";
> > + print format_log_line_html($line, @committags) . "<br/>\n";
> > }
> >
> > if ($opts{'-final_empty_line'}) {
>
> What about the tags? Or perhaps even blobs, for that matter?
What about? In commit messages you usually reference other commits
(as: this correct some commit, this finishes what was started in commit,
this reverts commit (!), cherry-picked commit).
Thanks for all the comments.
--
Jakub Narebski
Poland
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html