[PATCH 0/3] gitweb changes

[PATCH 0/3] gitweb changes

[Namhyung Kim]

Hi,

This is a small gitweb patchset to address a few issues
when dealt with the tip tree in Linux kernel.

NOTE: I'm not familiar with the code base and even the
perl language itself. Please kindly let me know if I did
something (horribly) wrong. :)

Thanks,
Namhyung


Namhyung Kim (3):
  gitweb: Get rid of unnecessary check of $signoff
  gitweb: Handle a few other tags in git_print_log
  gitweb: Add support to Link: tag

 gitweb/gitweb.perl |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
1.7.10.2

[PATCH 1/3] gitweb: Get rid of unnecessary check of $signoff

[Namhyung Kim]

If $signoff set to 1, the $line would be handled in
the if statement for the both cases. So the outer of
the conditional always sees the $signoff always set
to 0 and no need to check it. Thus we can finally get
rid of it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 gitweb/gitweb.perl |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 55e0e9e..7585e08 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4484,27 +4484,20 @@ sub git_print_log {
 	}
 
 	# print log
-	my $signoff = 0;
 	my $empty = 0;
 	foreach my $line (@$log) {
 		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
-			$signoff = 1;
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
-				next;
-			} else {
-				# remove signoff lines
-				next;
 			}
-		} else {
-			$signoff = 0;
+			next;
 		}
 
 		# print only one empty line
 		# do not print empty line after signoff
 		if ($line eq "") {
-			next if ($empty || $signoff);
+			next if ($empty);
 			$empty = 1;
 		} else {
 			$empty = 0;
-- 
1.7.10.2

Re: [PATCH 1/3] gitweb: Get rid of unnecessary check of $signoff

[Junio C Hamano]

Namhyung Kim <namhyung@kernel.org> writes:

> If $signoff set to 1, the $line would be handled in
> the if statement for the both cases. So the outer of
> the conditional always sees the $signoff always set
> to 0 and no need to check it. Thus we can finally get
> rid of it.

While this may not change the behaviour of the code, I think that
only shows the original is broken.  You even have the hint left in
the context of your patch:

>  		# print only one empty line
>  		# do not print empty line after signoff
>  		if ($line eq "") {
> -			next if ($empty || $signoff);
> +			next if ($empty);

Given this input:

	If $signoff is set to 1, ...
        ...
        rid of it.

	Signed-off-by: N K

        Signed-off-by: J C H

isn't the code trying (and failing) to remove the empty line between
the two S-o-b lines?

So something like this on top?  I renamed "$empty" which is unclear
what it means ("We just saw an empty line? What does the variable
want us to do?") to "$skip_blank_line" which more clearly instructs
us what to do.  If we see a blank line when it is set, we skip it.

 gitweb/gitweb.perl | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7585e08..202286b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4484,12 +4484,12 @@ sub git_print_log {
 	}
 
 	# print log
-	my $empty = 0;
+	my $skip_blank_line = 0;
 	foreach my $line (@$log) {
 		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
-			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
+				$skip_blank_line = 1;
 			}
 			next;
 		}
@@ -4497,10 +4497,10 @@ sub git_print_log {
 		# print only one empty line
 		# do not print empty line after signoff
 		if ($line eq "") {
-			next if ($empty);
-			$empty = 1;
+			next if ($skip_blank_line);
+			$skip_blank_line = 1;
 		} else {
-			$empty = 0;
+			$skip_blank_line = 0;
 		}
 
 		print format_log_line_html($line) . "<br/>\n";

[PATCH 2/3] gitweb: Handle a few other tags in git_print_log

[Namhyung Kim]

There are many of tags used in s-o-b area. Add
support for a few of well-known ones.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 gitweb/gitweb.perl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7585e08..e0701af 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4485,8 +4485,9 @@ sub git_print_log {
 
 	# print log
 	my $empty = 0;
+	my $tags = "acked|reviewed|reported|tested|suggested"
 	foreach my $line (@$log) {
-		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
+		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|($tags)[ \-]by[ :]|cc[ :])/i) {
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
-- 
1.7.10.2

Re: [PATCH 2/3] gitweb: Handle a few other tags in git_print_log

[Junio C Hamano]

Namhyung Kim <namhyung@kernel.org> writes:

> There are many of tags used in s-o-b area. Add
> support for a few of well-known ones.
>
> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  gitweb/gitweb.perl |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7585e08..e0701af 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4485,8 +4485,9 @@ sub git_print_log {
>  
>  	# print log
>  	my $empty = 0;
> +	my $tags = "acked|reviewed|reported|tested|suggested"

Missing ';' at the end.

>  	foreach my $line (@$log) {
> -		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
> +		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|($tags)[ \-]by[ :]|cc[ :])/i) {

Is anybody actually helped by these spaces that make the regexp
unnecessarily cluttered?

I am very tempted to suggest doing something like this:

	my $tags = join('|', qw(signed-off acked reviewed reported tested suggested));
	for my $line (@$log) {
		if ($line =~ m/^\s*(?:(?:$tags)-by|cc):/i) {
			...

or even this:

	for my $line (@$log) {
		if ($line =~ m/^\s*(?:[a-z][-a-z]*[a-z]): /i) {
			...

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

[Namhyung Kim]

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) {
+			$empty = 0;
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"signoff\">Link: <a href=\"" . esc_html($1) . "\">" .
+					esc_html($1) . "</a></span><br/>\n";
+			}
+			next;
 		}
 
 		# print only one empty line
-- 
1.7.10.2

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

[Junio C Hamano]

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.

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

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

[Namhyung Kim]

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

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

[Namhyung Kim]

On Wed, Jul 4, 2012 at 10:24 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> On Wed, Jul 4, 2012 at 5:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Namhyung Kim <namhyung@kernel.org> writes:
>> 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?

Oh, I guess you considered "Cc: " also.
That could be handled specially, though.

Thanks,
Namhyung

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

[Namhyung Kim]

On Wed, Jul 4, 2012 at 10:24 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 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.
>

Oops (again), I missed 'i' at the end.

Thanks,
Namhyung