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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2012, 20:15 UTC
Message-ID: <7vy5n0txdi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1341295377-22083-4-git-send-email-namhyung@kernel.org>

Namhyung Kim <namhyung@kernel.org> writes:

Show 27 quoted lines

> 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.

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 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]: ".

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;
	}