string is\n\tcreated and drop the leading SP from the value of $diff_class.\n\t\nExplained this way, it does not even have to mention that the return value\nwill be changed in a different patch.\n\nWe all know that the hidden motivation of this change is that the caller\nof this function, after it starts using the returned value of $diff_class,\ndoes not want to worry about the ugly SP prefix in that variable. Saying\nonly \"This function will return this variable in the future\" still does\nnot fully explain that hidden motivation unless you say \"and the caller\nshouldn't have to worry about the leading SP\", so let's not even mention\nit in the log message of this change.\n\n> ---\n\nSign-off?\n\n> gitweb/gitweb.perl | 24 +++++++++++++-----------\n> 1 files changed, 13 insertions(+), 11 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 85d64b2..095adda 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n> ...\n> +\n> +\tmy $div_open = '
';\n\nI think using a separate helper variable like this is a good change. You\ndo not have to worry about the issue in three different places.\n\nBut doesn't join(\" \", (\"frotz\", \"\")) still give you \"frotz \"? It is OK to\npunt and say\n\n\tmy $div_open = '
';\n\nwhich would be far easier to read. It may sacrifice a bit of tidiness in\nthe resulting HTML but the tidiness of the source outweighs it.\n\nOf course, if you have tons of classes, it may be worth doing something\nlike\n\n\tjoin(\" \", grep { defined $_ && $_ ne \"\"} @diff_classes);\n\nbut I do not think it is worth it in this particular case.\n\nThanks.\n"},{"id":"177888","messageId":"m38voj72xy.fsf@localhost.localdomain","threadId":"28701","inReplyTo":"CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com","subject":"Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-17T20:56:30Z","receivedAt":"2011-10-17T20:56:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"By the way, it is common to either have following patches to be chain\nreply to first patch, or better provide cover letter for patch series\nand have all patches be reply to cover letter.\n\n\nKato Kazuyoshi
writes:\n\nThis commit needs some more explanation in the commit message, like\nJunio said.\n\n> The format_diff_line() will return $diff_class and HTML in upcoming changes.\n\nThis is not necessary, I think; this change stands alone as a style\n(semantic) cleanup.\n\nSignoff? See Documentation/SubmittingPatches\n\n> ---\n> gitweb/gitweb.perl | 24 +++++++++++++-----------\n> 1 files changed, 13 insertions(+), 11 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 85d64b2..095adda 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n> \t\t# combined diff\n> \t\tmy $prefix = substr($line, 0, scalar @{$from->{'href'}});\n> \t\tif ($line =~ m/^\\@{3}/) {\n> -\t\t\t$diff_class = \" chunk_header\";\n> +\t\t\t$diff_class = \"chunk_header\";\n> \t\t} elsif ($line =~ m/^\\\\/) {\n> -\t\t\t$diff_class = \" incomplete\";\n> +\t\t\t$diff_class = \"incomplete\";\n> \t\t} elsif ($prefix =~ tr/+/+/) {\n> -\t\t\t$diff_class = \" add\";\n> +\t\t\t$diff_class = \"add\";\n> \t\t} elsif ($prefix =~ tr/-/-/) {\n> -\t\t\t$diff_class = \" rem\";\n> +\t\t\t$diff_class = \"rem\";\n> \t\t}\n\nHmmm... that reminds me: this if-elsif chain is a bit ugly, but would\nbe hard to replace without given ... when, I think.\n\n> \t} else {\n> \t\t# assume ordinary diff\n> \t\tmy $char = substr($line, 0, 1);\n> \t\tif ($char eq '+') {\n> -\t\t\t$diff_class = \" add\";\n> +\t\t\t$diff_class = \"add\";\n> \t\t} elsif ($char eq '-') {\n> -\t\t\t$diff_class = \" rem\";\n> +\t\t\t$diff_class = \"rem\";\n> \t\t} elsif ($char eq '@') {\n> -\t\t\t$diff_class = \" chunk_header\";\n> +\t\t\t$diff_class = \"chunk_header\";\n> \t\t} elsif ($char eq \"\\\\\") {\n> -\t\t\t$diff_class = \" incomplete\";\n> +\t\t\t$diff_class = \"incomplete\";\n> \t\t}\n\nThis is also ugly, but this one could in theory be replaced by hash\nlookup. But this would remove symmetry...\n\n> \t}\n> \t$line = untabify($line);\n> +\n> +\tmy $div_open = '';\n\nAnother solution would be to use\n\n +\n +\tmy $diff_classes = \"diff\";\n +\t$diff_classes .= \" $diff_class\" if ($diff_class);\n +\n\n> \tif ($from && $to && $line =~ m/^\\@{2} /) {\n> \t\tmy ($from_text, $from_start, $from_lines, $to_text, $to_start,\n> $to_lines, $section) =\n> \t\t\t$line =~ m/^\\@{2} (-(\\d+)(?:,(\\d+))?) (\\+(\\d+)(?:,(\\d+))?) \\@{2}(.*)$/;\n> @@ -2274,7 +2276,7 @@ sub format_diff_line {\n> \t\t}\n> \t\t$line = \"
@@ $from_text $to_text @@\" .\n> \t\t \"
\" . esc_html($section, -nbsp=>1) .\n> \"\";\n\nThe above is word-wrapped. Please turn off word wrapping in the\nfuture to avoid such damage to the patch.\n\n> -\t\treturn \"
$line
\\n\";\n> +\t\treturn \"$div_open$line
$line
\\n\";\n\nwhich is a bit more symmetric.\n\nBut that is just a matter of taste.\n\n> \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n> \t\tmy ($prefix, $ranges, $section) = $line =~ m/^(\\@+) (.*?) \\@+(.*)$/;\n> \t\tmy (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);\n> @@ -2307,9 +2309,9 @@ sub format_diff_line {\n> \t\t}\n> \t\t$line .= \" $prefix\" .\n> \t\t \"\" . esc_html($section, -nbsp=>1)\n> . \"\";\n> -\t\treturn \"$line
\\n\";\n> +\t\treturn \"$div_open$line\\n\";\n> \t}\n> -\treturn \"
\" . esc_html($line, -nbsp=>1)\n> . \"
\\n\";\n> +\treturn $div_open . esc_html($line, -nbsp=>1) . \"
\\n\";\n> }\n> \n> # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n> -- \n> 1.7.7.213.g8b0e1.dirty\n\n-- \nJakub Narębski\n"},{"id":"177891","messageId":"7v62jnl3ef.fsf@alter.siamese.dyndns.org","threadId":"28701","inReplyTo":"m38voj72xy.fsf@localhost.localdomain","subject":"Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T21:22:16Z","receivedAt":"2011-10-17T21:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski
writes:\n\n> By the way, it is common to either have following patches to be chain\n> reply to first patch, or better provide cover letter for patch series\n> and have all patches be reply to cover letter.\n\nIt may be a good discipline for 14 patch series, but it doesn't matter for\nsomething this small.\n\n>> gitweb/gitweb.perl | 24 +++++++++++++-----------\n>> 1 files changed, 13 insertions(+), 11 deletions(-)\n>> \n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 85d64b2..095adda 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n>> \t\t# combined diff\n>> \t\tmy $prefix = substr($line, 0, scalar @{$from->{'href'}});\n>> \t\tif ($line =~ m/^\\@{3}/) {\n>> -\t\t\t$diff_class = \" chunk_header\";\n>> +\t\t\t$diff_class = \"chunk_header\";\n>> \t\t} elsif ($line =~ m/^\\\\/) {\n>> -\t\t\t$diff_class = \" incomplete\";\n>> +\t\t\t$diff_class = \"incomplete\";\n>> \t\t} elsif ($prefix =~ tr/+/+/) {\n>> -\t\t\t$diff_class = \" add\";\n>> +\t\t\t$diff_class = \"add\";\n>> \t\t} elsif ($prefix =~ tr/-/-/) {\n>> -\t\t\t$diff_class = \" rem\";\n>> +\t\t\t$diff_class = \"rem\";\n>> \t\t}\n>\n> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would\n> be hard to replace without given ... when, I think.\n\nI wasn't reading the existing context line, but now that you mention it,\nthey are not just ugly but are borderline of being incorrect (e.g. it does\nnot catch a broken hunk-header that begins with \"@@@@\" for a two-way\nmerge).\n\nAssuming that $from->{'href'} has all the parents (i.e. 2 for two-way\nmerge), shouldn't the code be more like this?\n\n\t# combined diff\n my $num_sign = @{$from->{'href'}} + 1;\n\tmy @cc_classifier = (\n\t\t[\"\\@{$num_sign} \", \"chunk_header\"],\n ['\\\\', \"incomplete\"],\n [\" {$num_sign}\", \"\"],\n [\"[+ ]{$num_sign}\", \"add\"],\n [\"[- ]{$num_sign}\", \"rem\"],\n );\n for my $cls (@cc_classifier) {\n if ($line =~ /^$cls->[0]$/) {\n \t$diff_class = $cls->[1];\n last;\n\t\t}\n\t}\n\nAlso don't we want to use \"context\" or something for the css class for the\ncontext lines, instead of assuming that we won't want to paint it in any\nspecial color?\n"},{"id":"177896","messageId":"201110180007.31008.jnareb@gmail.com","threadId":"28701","inReplyTo":"7v62jnl3ef.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-17T22:07:30Z","receivedAt":"2011-10-17T22:07:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 17 Oct 2011, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>> By the way, it is common to either have following patches to be chain\n>> reply to first patch, or better provide cover letter for patch series\n>> and have all patches be reply to cover letter.\n> \n> It may be a good discipline for 14 patch series, but it doesn't matter for\n> something this small.\n\nWell, cover letter for 2-patch series might be overkill, but having\npatches in series connected e.g. chain-replied-to, or all replies to\nfirst patch in series, is IMHO a good idea.\n\n>>> gitweb/gitweb.perl | 24 +++++++++++++-----------\n>>> 1 files changed, 13 insertions(+), 11 deletions(-)\n>>> \n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index 85d64b2..095adda 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n>>> \t\t# combined diff\n>>> \t\tmy $prefix = substr($line, 0, scalar @{$from->{'href'}});\n>>> \t\tif ($line =~ m/^\\@{3}/) {\n>>> -\t\t\t$diff_class = \" chunk_header\";\n>>> +\t\t\t$diff_class = \"chunk_header\";\n>>> \t\t} elsif ($line =~ m/^\\\\/) {\n>>> -\t\t\t$diff_class = \" incomplete\";\n>>> +\t\t\t$diff_class = \"incomplete\";\n>>> \t\t} elsif ($prefix =~ tr/+/+/) {\n>>> -\t\t\t$diff_class = \" add\";\n>>> +\t\t\t$diff_class = \"add\";\n>>> \t\t} elsif ($prefix =~ tr/-/-/) {\n>>> -\t\t\t$diff_class = \" rem\";\n>>> +\t\t\t$diff_class = \"rem\";\n>>> \t\t}\n>>\n>> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would\n>> be hard to replace without given ... when, I think.\n> \n> I wasn't reading the existing context line, but now that you mention it,\n> they are not just ugly but are borderline of being incorrect (e.g. it does\n> not catch a broken hunk-header that begins with \"@@@@\" for a two-way\n> merge).\n> \n> Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way\n> merge), shouldn't the code be more like this?\n> \n> \t# combined diff\n\nWouldn't that cover not only combined diff, but an \"ordinary\" diff as\nwell then (i.e. comment should change)? I think that was the intent,\nisn't it?\n\n> my $num_sign = @{$from->{'href'}} + 1;\n\n$from->{'href'} is array reference only for combined diff (>= 2 parents).\nFor ordinary diff it is a scalar.\n\n> \tmy @cc_classifier = (\n> \t\t [\"\\@{$num_sign} \", \"chunk_header\"],\n\nO.K.\n\nNb., it is \"chunk_header\", or \"hunk_header\"?\n\n> ['\\\\', \"incomplete\"],\n\nO.K.\n\n> [\" {$num_sign}\", \"\"],\n\nO.K.\n\n> [\"[+ ]{$num_sign}\", \"add\"],\n> [\"[- ]{$num_sign}\", \"rem\"],\n\nIt would be slightly different to what current code does.\n\nCurrent code for combined diff uses \"add\" if there is at least one '+',\n\"rem\" if there are no '+' and at least one '-', and context otherwise.\n\n\nI wonder if with sufficiently evil merge we can have a line that is\nadded (changed) in some children, and removed in other, i.e. pluses\nand minuses combined.\n\nNb. we can put regexp here, not only stringification of regexp.\ni.e.\n\n [qr/[+ ]{$num_sign}/, \"add\"],\n [qr/[- ]{$num_sign}/, \"rem\"],\n\n\n> );\n> for my $cls (@cc_classifier) {\n> if ($line =~ /^$cls->[0]$/) {\n> \t$diff_class = $cls->[1];\n> last;\n> \t\t}\n> \t}\n\nNice trick.\n \n> Also don't we want to use \"context\" or something for the css class for the\n> context lines, instead of assuming that we won't want to paint it in any\n> special color?\n\nRight. We use \"diff\" class without anything else for context, but probably\nit would be better to state this explicitly.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"177909","messageId":"7vehybjjdd.fsf@alter.siamese.dyndns.org","threadId":"28701","inReplyTo":"201110180007.31008.jnareb@gmail.com","subject":"Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T23:20:14Z","receivedAt":"2011-10-17T23:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> O.K.\n>\n>> [\" {$num_sign}\", \"\"],\n>\n> O.K.\n>\n>> [\"[+ ]{$num_sign}\", \"add\"],\n>> [\"[- ]{$num_sign}\", \"rem\"],\n>\n> It would be slightly different to what current code does.\n\n> Current code for combined diff uses \"add\" if there is at least one '+',\n> \"rem\" if there are no '+' and at least one '-', and context otherwise.\n\nThe only possible difference would be for a line with all blank, but\nbecause there is an additional explicit rule for context, the behaviour is\nthe same. In a combined diff, you will never see + and - together on the\nsame line.\n\n> I wonder if with sufficiently evil merge we can have a line that is\n> added (changed) in some children, and removed in other, i.e. pluses\n> and minuses combined.\n\nThe logic in combine-diff.c::dump_sline() was written in such a way to\navoid such a confusing output.\n\n> Nb. we can put regexp here, not only stringification of regexp.\n> i.e.\n>\n> [qr/[+ ]{$num_sign}/, \"add\"],\n> [qr/[- ]{$num_sign}/, \"rem\"],\n\nThat would be a good change.\n\n>> Also don't we want to use \"context\" or something for the css class for the\n>> context lines, instead of assuming that we won't want to paint it in any\n>> special color?\n>\n> Right. We use \"diff\" class without anything else for context, but probably\n> it would be better to state this explicitly.\n"},{"id":"177912","messageId":"CAFo4x0KayxUfnVfO1-Qhu1LiMvwMs6g=bgPXZjqqZUxpUJsDWQ@mail.gmail.com","threadId":"28701","inReplyTo":"7v62jnmog3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class","fromName":"Kato Kazuyoshi","fromEmail":"kato.kazuyoshi@gmail.com","sentAt":"2011-10-17T23:58:02Z","receivedAt":"2011-10-17T23:58:02Z","isPatch":true,"sender":{"key":"kato.kazuyoshi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19111?v=4"},"body":"On Tue, Oct 18, 2011 at 4:02 AM, Junio C Hamano wrote:\n> Kato Kazuyoshi writes:\n>\n>> The format_diff_line() will return $diff_class and HTML in upcoming changes.\n>\n> An auxiliary piece of information like this is fine at the end of the\n> commit log message, but the patch itself wants to be justified\n> standalone. Perhaps this should be sufficient:\n>\n> The $diff_class variable to classify the kind of line in the diff\n> output was prefixed with a SP, only so that the code to synthesize\n> value for \"class\" attribute can blindly concatenate it with\n> another value \"diff\". This made the code unnecessarily ugly.\n>\n> Instead, add SP that separates the value of $diff_class from\n> another class value \"diff\" where string is\n> created and drop the leading SP from the value of $diff_class.\n>\n> Explained this way, it does not even have to mention that the return value\n> will be changed in a different patch.\n>\n\nThanks. I couldn't write a good summary for my patch because it was just\nan \"adjust\" for me. However your summary is really clear!\n\n>> gitweb/gitweb.perl | 24 +++++++++++++-----------\n>> 1 files changed, 13 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 85d64b2..095adda 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n>> ...\n>> +\n>> + my $div_open = '
';\n>\n> I think using a separate helper variable like this is a good change. You\n> do not have to worry about the issue in three different places.\n>\n> But doesn't join(\" \", (\"frotz\", \"\")) still give you \"frotz \"? It is OK to\n> punt and say\n>\n> my $div_open = '
';\n>\n> which would be far easier to read. It may sacrifice a bit of tidiness in\n> the resulting HTML but the tidiness of the source outweighs it.\n>\n> Of course, if you have tons of classes, it may be worth doing something\n> like\n>\n> join(\" \", grep { defined $_ && $_ ne \"\"} @diff_classes);\n>\n> but I do not think it is worth it in this particular case.\n>\n\nYeah, I want to remove unnecessary SP that you mentioned before.\nBut well, join(\" \", (\"frotz\", \"\")) give me \"frotz \".\nI will add some per-function test cases to gitweb before this patch series.\n\nThanks,\n\n-- \nKato Kazuyoshi\n"},{"id":"177960","messageId":"201110182026.22610.jnareb@gmail.com","threadId":"28701","inReplyTo":"7v62jnl3ef.fsf@alter.siamese.dyndns.org","subject":"[PATCH] gitweb: Refactor diff body line classification","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-18T18:26:21Z","receivedAt":"2011-10-18T18:26:21Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Simplify classification of diff line body in format_diff_line(),\nreplacing two if-elsif chains (one for ordinary diff and one for\ncombined diff of a merge commit) with regexp match. Refactor this\ncode into diff_line_class() function.\n\nWhile at it:\n\n* Fix an artifact in that $diff_class included leading space to be\n able to compose classes like this \"class=\\\"diff$diff_class\\\"', even\n when $diff_class was an empty string. This made code unnecessary\n ugly: $diff_class is now just class name or an empty string.\n\n* Introduce \"ctx\" class for context lines ($diff_class was set to \"\"\n in this case before this commit).\n\nIdea and initial code by Junio C Hamano, polish and testing by Jakub\nNarebski. Inspired by patch adding side-by-side diff by Kato Kazuyoshi,\nwhich required $diff_class to be name of class without extra space.\n\nSigned-off-by: Junio C Hamano
\nSigned-off-by: Jakub Narebski \n---\nJunio C Hamano wrote:\n> Jakub Narebski writes:\n>> Kato Kazuyoshi wrote:\n\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl \n>>> index 85d64b2..095adda 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -2235,28 +2235,30 @@ sub format_diff_line {\n>>> # combined diff\n>>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});\n>>> if ($line =~ m/^\\@{3}/) {\n>>> - $diff_class = \" chunk_header\";\n>>> + $diff_class = \"chunk_header\";\n>>> } elsif ($line =~ m/^\\\\/) {\n>>> - $diff_class = \" incomplete\";\n>>> + $diff_class = \"incomplete\";\n>>> } elsif ($prefix =~ tr/+/+/) {\n>>> - $diff_class = \" add\";\n>>> + $diff_class = \"add\";\n>>> } elsif ($prefix =~ tr/-/-/) {\n>>> - $diff_class = \" rem\";\n>>> + $diff_class = \"rem\";\n>>> }\n>>\n>> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would\n>> be hard to replace without given ... when, I think.\n> \n> I wasn't reading the existing context line, but now that you mention it,\n> they are not just ugly but are borderline of being incorrect (e.g. it does\n> not catch a broken hunk-header that begins with \"@@@@\" for a two-way\n> merge).\n> \n> Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way\n> merge), shouldn't the code be more like this?\n> \n> # combined diff\n> my $num_sign = @{$from->{'href'}} + 1;\n> my @cc_classifier = (\n> [\"\\@{$num_sign} \", \"chunk_header\"],\n> ['\\\\', \"incomplete\"],\n> [\" {$num_sign}\", \"\"],\n> [\"[+ ]{$num_sign}\", \"add\"],\n> [\"[- ]{$num_sign}\", \"rem\"],\n> );\n> for my $cls (@cc_classifier) {\n> if ($line =~ /^$cls->[0]$/) {\n> $diff_class = $cls->[1];\n> last;\n> }\n> }\n> \n> Also don't we want to use \"context\" or something for the css class for the\n> context lines, instead of assuming that we won't want to paint it in any\n> special color?\n\nHow about this?\n\nKato Kazuyoshi, this should replace your\n\n [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class\n\n gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++-----------------------\n 1 files changed, 37 insertions(+), 30 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex b0eaad7..b3284d4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2247,40 +2247,47 @@ sub format_diff_cc_simplified {\n \treturn $result;\n }\n \n+sub diff_line_class {\n+\tmy ($line, $from, $to) = @_;\n+\n+\t# ordinary diff\n+\tmy $num_sign = 1;\n+\t# combined diff\n+\tif ($from && $to && ref($from->{'href'}) eq \"ARRAY\") {\n+\t\t$num_sign = scalar @{$from->{'href'}};\n+\t}\n+\n+\tmy @diff_line_classifier = (\n+\t\t{ regexp => qr/^\\@\\@{$num_sign} /, class => \"chunk_header\"},\n+\t\t{ regexp => qr/^\\\\/, class => \"incomplete\" },\n+\t\t{ regexp => qr/^ {$num_sign}/, class => \"ctx\" },\n+\t\t# classifier for context must come before classifier add/rem,\n+\t\t# or we would have to use more complicated regexp, for example\n+\t\t# qr/(?= {0,$m}\\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;\n+\t\t{ regexp => qr/^[+ ]{$num_sign}/, class => \"add\" },\n+\t\t{ regexp => qr/^[- ]{$num_sign}/, class => \"rem\" },\n+\t);\n+\tfor my $clsfy (@diff_line_classifier) {\n+\t\treturn $clsfy->{'class'}\n+\t\t\tif ($line =~ $clsfy->{'regexp'});\n+\t}\n+\n+\t# fallback\n+\treturn \"\";\n+}\n+\n # format patch (diff) line (not to be used for diff headers)\n sub format_diff_line {\n \tmy $line = shift;\n \tmy ($from, $to) = @_;\n-\tmy $diff_class = \"\";\n \n-\tchomp $line;\n+\tmy $diff_class = diff_line_class($line, $from, $to);\n+\tmy $diff_classes = \"diff\";\n+\t$diff_classes .= \" $diff_class\" if ($diff_class);\n \n-\tif ($from && $to && ref($from->{'href'}) eq \"ARRAY\") {\n-\t\t# combined diff\n-\t\tmy $prefix = substr($line, 0, scalar @{$from->{'href'}});\n-\t\tif ($line =~ m/^\\@{3}/) {\n-\t\t\t$diff_class = \" chunk_header\";\n-\t\t} elsif ($line =~ m/^\\\\/) {\n-\t\t\t$diff_class = \" incomplete\";\n-\t\t} elsif ($prefix =~ tr/+/+/) {\n-\t\t\t$diff_class = \" add\";\n-\t\t} elsif ($prefix =~ tr/-/-/) {\n-\t\t\t$diff_class = \" rem\";\n-\t\t}\n-\t} else {\n-\t\t# assume ordinary diff\n-\t\tmy $char = substr($line, 0, 1);\n-\t\tif ($char eq '+') {\n-\t\t\t$diff_class = \" add\";\n-\t\t} elsif ($char eq '-') {\n-\t\t\t$diff_class = \" rem\";\n-\t\t} elsif ($char eq '@') {\n-\t\t\t$diff_class = \" chunk_header\";\n-\t\t} elsif ($char eq \"\\\\\") {\n-\t\t\t$diff_class = \" incomplete\";\n-\t\t}\n-\t}\n+\tchomp $line;\n \t$line = untabify($line);\n+\n \tif ($from && $to && $line =~ m/^\\@{2} /) {\n \t\tmy ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =\n \t\t\t$line =~ m/^\\@{2} (-(\\d+)(?:,(\\d+))?) (\\+(\\d+)(?:,(\\d+))?) \\@{2}(.*)$/;\n@@ -2298,7 +2305,7 @@ sub format_diff_line {\n \t\t}\n \t\t$line = \"@@ $from_text $to_text @@\" .\n \t\t \"\" . esc_html($section, -nbsp=>1) . \"\";\n-\t\treturn \"$line
\\n\";\n+\t\treturn \"$line
\\n\";\n \t} elsif ($from && $to && $line =~ m/^\\@{3}/) {\n \t\tmy ($prefix, $ranges, $section) = $line =~ m/^(\\@+) (.*?) \\@+(.*)$/;\n \t\tmy (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);\n@@ -2331,9 +2338,9 @@ sub format_diff_line {\n \t\t}\n \t\t$line .= \" $prefix\" .\n \t\t \"\" . esc_html($section, -nbsp=>1) . \"\";\n-\t\treturn \"$line
\\n\";\n+\t\treturn \"$line
\\n\";\n \t}\n-\treturn \"\" . esc_html($line, -nbsp=>1) . \"
\\n\";\n+\treturn \"\" . esc_html($line, -nbsp=>1) . \"
\\n\";\n }\n \n # Generates undef or something like \"_snapshot_\" or \"snapshot (_tbz2_ _zip_)\",\n-- \n1.7.6\n"}]}