Re: [PATCH 3/3] gitweb: Add support to Link: tag

[Namhyung Kim <namhyung@kernel.org>]

Hi,

On Wed, Jul 4, 2012 at 5:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Namhyung Kim <namhyung@kernel.org> writes:
>
>> The tip tree is the one of major subsystem tree in the
>> Linux kernel project. On the tip tree, the Link: tag is
>> used for tracking the original discussion or context.
>> Since it's ususally in the s-o-b area, it'd be better
>> using same style with others.
>>
>> Also as it tends to contain a message-id sent from git
>> send-email, a part of the line which has more than 8
>> (hex-)digit characters would set a wrong hyperlink
>> like [1]. Fix it by not using format_log_line_html().
>>
>> [1] git.kernel.org/?p=linux/kernel/git/tip/tip.git;a=commit;h=08942f6d5d992e9486b07653fd87ea8182a22fa0
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  gitweb/gitweb.perl |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index e0701af..d07bcb7 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -4493,6 +4493,13 @@ sub git_print_log {
>>                               print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
>>                       }
>>                       next;
>> +             } elsif ($line =~ m,^ *link[ :](http://[\w/~.@%&=?+-]*),i) {
>
> Hrm, I am somewhat confused.  This catches "link:http://..." and
> "link http://...", but not "link: http://...", which looks a lot
> more natural looking at least to me.
>

Oops, right. Actually I found it after some local testing,
but forgot to update the patch. Sorry :)


> Looking at a random sample:
>
> http://git.kernel.org/?p=linux/kernel/git/tip/tip.git;a=commit;h=fe85227347738eb9b871bc163e7fb0db8b6cd2a0
>
> I see a "Buglink: " which I think deserves to be handled by this patch
> but would not.  Probably the pattern needs to be loosened
> sufficiently, e.g.
>
>         m,^\s*[a-z]*link: (https?://\S+),i
>
> to catch it as well.  Note that I am rejecting space before ":" and
> requiring a space after ":" in the above.
>

I think 'l' in 'link' should be '[Ll]'. Otherwise looks good to me.


> I also notice that "Reported-bisected-and-tested-by: " in that
> example, which is the topic of your [PATCH 2/3].  Perhaps the logic
> should catch everythinng that match "^[A-Z][-a-z]*[a-z]: ".
>

Isn't "^[A-Z][-A-Za-z]*-[Bb]y: " enough?
Just FYI, please see this too:

http://lwn.net/Articles/503829/


> As to coding style, if you end the body of if () clause with 'next',
> I tend to think it is easier to read if you structure it like this:
>
>         if (condition 1) {
>                 ... action 1 ...
>                 next;
>         }
>
>         if (condition 2) {
>                 ... action 2 ...
>                 next;
>         }
>
> instead of like this:
>
>         if (condition 1) {
>                 ... action 1 ...
>                 next;
>         } elsif (condition 2) {
>                 ... action 2 ...
>                 next;
>         }
>

Ok, I'll send v2 soon after addressing all of your comments
(including previous mails).

Thanks,
Namhyung