threads / patch / 43063
Subject: Re: [PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Jakub Narebski[PATCH 0/n] gitweb: Better quoting and New improved patchset view
This series of patches is meant to introduce "New improved patchset view" in part-by-part case.
Would change only gitweb/gitweb.perl and gitweb/gitweb/css
-- Jakub Narebski
Jakub Narebski[PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
Extend unquote subroutine, which unquotes quoted and escaped filenames which git may return, to deal not only with octal char sequence quoting, but also quoting ordinary characters including '\"' and '\\' which are respectively quoted '"' and '\', and to deal also with C escape sequences including '\t' for TAB and '\n' for LF.
Add esc_path subroutine for gitweb quoting and HTML escaping filenames (currently it does equivalent of ls' --hide-control-chars, which means showing undisplayable characters (including '\n' and '\t') as '?' (question mark) character. Convert gitweb to use esc_path instead of simply esc_html to print pathnames.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- By the way, testing this patch uncovered some errors in gitweb, some related to files with strange name, some unrelated. I'll address them in further patches. They are:
1. Using m/..\t(.+)$/; to catch filename instead of m/..\t(.+)$/s;
2. Lack of '--' after $hash/$hash_base parameter which gives error
if there exist branch (ref) and file (or directory) with the same
nameThe current implementation of esc_path is meant as preliminary: if you have better idea for quoting names in gitweb, please tell us, or better send code/patches.
gitweb/gitweb.perl | 68 +++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index ec46b80..a15e916 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -563,12 +563,42 @@ sub esc_html { return $str; } +# quote unsafe characters and escape filename to HTML +sub esc_path { + my $str = shift; + $str = esc_html($str); + $str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/?/g; # like --hide-control-chars in ls + return $str; +} + # git may return quoted and escaped filenames sub unquote { my $str = shift; + + sub unq { + my $seq = shift; + my %es = ( + 't' => "\t", # tab (HT, TAB) + 'n' => "\n", # newline (NL) + 'r' => "\r", # return (CR) + 'f' => "\f", # form feed (FF) + 'b' => "\b", # backspace (BS) + 'a' => "\a", # alarm (bell) (BEL) + #'e' => "\e", # escape (ESC) + 'v' => "\011", # vertical tab (VT) + ); + + # octal char sequence + return chr(oct($seq)) if ($seq =~ m/^[0-7]{1,3}$/); + # C escape sequence (this includes '\n' (LF) and '\t' (TAB)) + return $es{$seq} if ($seq =~ m/^[abefnrtv]$/); + # quted ordinary character (this includes '\\' and '\"') + return $seq; + } + if ($str =~ m/^"(.*)"$/) { $str = $1; - $str =~ s/\\([0-7]{1,3})/chr(oct($1))/eg; + $str =~ s/\\([^0-7]|[0-7]{1,3})/unq($1)/eg; } return $str; } @@ -1435,7 +1465,7 @@ sub git_header_html { if (defined $action) { $title .= "/$action"; if (defined $file_name) { - $title .= " - " . esc_html($file_name); + $title .= " - " . esc_path($file_name); if ($action eq "tree" && $file_name !~ m|/$|) { $title .= "/"; } @@ -1706,20 +1736,20 @@ sub git_print_page_path { $fullname .= ($fullname ? '/' : '') . $dir; print $cgi->a({-href => href(action=>"tree", file_name=>$fullname, hash_base=>$hb), - -title => $fullname}, esc_html($dir)); + -title => $fullname}, esc_path($dir)); print " / "; } if (defined $type && $type eq 'blob') { print $cgi->a({-href => href(action=>"blob_plain", file_name=>$file_name, hash_base=>$hb), - -title => $name}, esc_html($basename)); + -title => $name}, esc_path($basename)); } elsif (defined $type && $type eq 'tree') { print $cgi->a({-href => href(action=>"tree", file_name=>$file_name, hash_base=>$hb), - -title => $name}, esc_html($basename)); + -title => $name}, esc_path($basename)); print " / "; } else { - print esc_html($basename); + print esc_path($basename); } } print "<br/></div>\n"; @@ -1791,7 +1821,7 @@ sub git_print_tree_entry { print "<td class=\"list\">" . $cgi->a({-href => href(action=>"blob", hash=>$t->{'hash'}, file_name=>"$basedir$t->{'name'}", %base_key), - -class => "list"}, esc_html($t->{'name'})) . "</td>\n"; + -class => "list"}, esc_path($t->{'name'})) . "</td>\n"; print "<td class=\"link\">"; print $cgi->a({-href => href(action=>"blob", hash=>$t->{'hash'}, file_name=>"$basedir$t->{'name'}", %base_key)}, @@ -1818,7 +1848,7 @@ sub git_print_tree_entry { print "<td class=\"list\">"; print $cgi->a({-href => href(action=>"tree", hash=>$t->{'hash'}, file_name=>"$basedir$t->{'name'}", %base_key)}, - esc_html($t->{'name'})); + esc_path($t->{'name'})); print "</td>\n"; print "<td class=\"link\">"; print $cgi->a({-href => href(action=>"tree", hash=>$t->{'hash'}, @@ -1883,7 +1913,7 @@ sub git_difftree_body { print "<td>"; print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'}, hash_base=>$hash, file_name=>$diff{'file'}), - -class => "list"}, esc_html($diff{'file'})); + -class => "list"}, esc_path($diff{'file'})); print "</td>\n"; print "<td>$mode_chng</td>\n"; print "<td class=\"link\">"; @@ -1899,7 +1929,7 @@ sub git_difftree_body { print "<td>"; print $cgi->a({-href => href(action=>"blob", hash=>$diff{'from_id'}, hash_base=>$parent, file_name=>$diff{'file'}), - -class => "list"}, esc_html($diff{'file'})); + -class => "list"}, esc_path($diff{'file'})); print "</td>\n"; print "<td>$mode_chng</td>\n"; print "<td class=\"link\">"; @@ -1939,7 +1969,7 @@ sub git_difftree_body { print "<td>"; print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'}, hash_base=>$hash, file_name=>$diff{'file'}), - -class => "list"}, esc_html($diff{'file'})); + -class => "list"}, esc_path($diff{'file'})); print "</td>\n"; print "<td>$mode_chnge</td>\n"; print "<td class=\"link\">"; @@ -1979,11 +2009,11 @@ sub git_difftree_body { print "<td>" . $cgi->a({-href => href(action=>"blob", hash_base=>$hash, hash=>$diff{'to_id'}, file_name=>$diff{'to_file'}), - -class => "list"}, esc_html($diff{'to_file'})) . "</td>\n" . + -class => "list"}, esc_path($diff{'to_file'})) . "</td>\n" . "<td><span class=\"file_status $nstatus\">[$nstatus from " . $cgi->a({-href => href(action=>"blob", hash_base=>$parent, hash=>$diff{'from_id'}, file_name=>$diff{'from_file'}), - -class => "list"}, esc_html($diff{'from_file'})) . + -class => "list"}, esc_path($diff{'from_file'})) . " with " . (int $diff{'similarity'}) . "% similarity$mode_chng]</span></td>\n" . "<td class=\"link\">"; if ($diff{'to_id'} ne $diff{'from_id'}) { @@ -2113,7 +2143,7 @@ sub git_patchset_body { $file ||= $diffinfo->{'file'}; $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, hash=>$diffinfo->{'from_id'}, file_name=>$file), - -class => "list"}, esc_html($file)); + -class => "list"}, esc_path($file)); $patch_line =~ s|a/.*$|a/$file|g; print "<div class=\"diff from_file\">$patch_line</div>\n"; @@ -2125,7 +2155,7 @@ sub git_patchset_body { $file ||= $diffinfo->{'file'}; $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash, hash=>$diffinfo->{'to_id'}, file_name=>$file), - -class => "list"}, esc_html($file)); + -class => "list"}, esc_path($file)); $patch_line =~ s|b/.*|b/$file|g; print "<div class=\"diff to_file\">$patch_line</div>\n"; @@ -3373,8 +3403,8 @@ sub git_blobdiff { } else { while (my $line = <$fd>) { - $line =~ s!a/($hash|$hash_parent)!'a/'.esc_html($diffinfo{'from_file'})!eg; - $line =~ s!b/($hash|$hash_parent)!'b/'.esc_html($diffinfo{'to_file'})!eg; + $line =~ s!a/($hash|$hash_parent)!'a/'.esc_path($diffinfo{'from_file'})!eg; + $line =~ s!b/($hash|$hash_parent)!'b/'.esc_path($diffinfo{'to_file'})!eg; print $line; @@ -3729,7 +3759,7 @@ sub git_search { print $cgi->a({-href => href(action=>"blob", hash_base=>$co{'id'}, hash=>$set{'id'}, file_name=>$set{'file'}), -class => "list"}, - "<span class=\"match\">" . esc_html($set{'file'}) . "</span>") . + "<span class=\"match\">" . esc_path($set{'file'}) . "</span>") . "<br/>\n"; } print "</td>\n" . @@ -3863,7 +3893,7 @@ XML if (!($line =~ m/^:([0-7]{6}) ([0-7]{6}) ([0-9a-fA-F]{40}) ([0-9a-fA-F]{40}) (.)([0-9]{0,3})\t(.*)$/)) { next; } - my $file = esc_html(unquote($7)); + my $file = esc_path(unquote($7)); $file = to_utf8($file); print "$file<br/>\n"; }
-- 1.4.3.3
Junio C HamanoRe: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
Jakub Narebski <jnareb@gmail.com> writes:
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ec46b80..a15e916 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -563,12 +563,42 @@ sub esc_html {
> return $str;
> }
>
> +# quote unsafe characters and escape filename to HTML
> +sub esc_path {
> + my $str = shift;
> + $str = esc_html($str);
> + $str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/?/g; # like --hide-control-chars in ls
> + return $str;
> +}
> +When you say "[:cntrl:]" do you need to say anything more?
I was initially puzzled by "\t\011" bit, but I realize that is a bug that is consistent with the next part I'll comment on.
> # git may return quoted and escaped filenames
> sub unquote {
> my $str = shift;
> +
> + sub unq {
> + my $seq = shift;
> + my %es = (
> + 't' => "\t", # tab (HT, TAB)
> + 'n' => "\n", # newline (NL)
> + 'r' => "\r", # return (CR)
> + 'f' => "\f", # form feed (FF)
> + 'b' => "\b", # backspace (BS)
> + 'a' => "\a", # alarm (bell) (BEL)
> + #'e' => "\e", # escape (ESC)
> + 'v' => "\011", # vertical tab (VT)
> + );
> +
> + # octal char sequence
> + return chr(oct($seq)) if ($seq =~ m/^[0-7]{1,3}$/);
> + # C escape sequence (this includes '\n' (LF) and '\t' (TAB))
> + return $es{$seq} if ($seq =~ m/^[abefnrtv]$/);Problems in this part of the code X-<.
* Was there a reason not to unwrap '\e' to "\e"?
* The vertical tab is \013 (decimal 11), not \011 (which is TAB).
* The name and the abbreviated name of the character "\n" are "line feed" and "LF"; I personally do not think these character name comments are needed in this part of the code, but I do not object if you want to have them there, as long as you spell them correctly. cf. ISO/IEC 6429:1992 or http://www.unicode.org/charts/PDF/U0000.pdf for example.
* The hash %es and the pattern /[abef...]/ must be kept in sync; it is a maintenance nightmare,
* Worse yet, they do not agree even in this initial version, which proves the previous point.
Perhaps this is better written as:
if (exists $es{$seq}) {
return $es{$seq};
}The rest must have been a lot of work to sift through esc_html and identify which is path and which is not. Much appreciated and expect a cleaned up patch to be applied.
Jakub NarebskiRe: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index ec46b80..a15e916 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -563,12 +563,42 @@ sub esc_html {
> > return $str;
> > }
> >
> > +# quote unsafe characters and escape filename to HTML
> > +sub esc_path {
> > + my $str = shift;
> > + $str = esc_html($str);
> > + $str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/?/g; # like --hide-control-chars in ls
> > + return $str;
> > +}
> > +
>
> When you say "[:cntrl:]" do you need to say anything more?Ooops. Yes, the \a\b\e\f\n\r\t\011 part is redundant.
> > # git may return quoted and escaped filenames
> > sub unquote {
> > my $str = shift;
> > +
> > + sub unq {
> > + my $seq = shift;
> > + my %es = (
> > + 't' => "\t", # tab (HT, TAB)
> > + 'n' => "\n", # newline (NL)
> > + 'r' => "\r", # return (CR)
> > + 'f' => "\f", # form feed (FF)
> > + 'b' => "\b", # backspace (BS)
> > + 'a' => "\a", # alarm (bell) (BEL)
> > + #'e' => "\e", # escape (ESC)
> > + 'v' => "\011", # vertical tab (VT)
> > + );
> > +
> > + # octal char sequence
> > + return chr(oct($seq)) if ($seq =~ m/^[0-7]{1,3}$/);
> > + # C escape sequence (this includes '\n' (LF) and '\t' (TAB))
> > + return $es{$seq} if ($seq =~ m/^[abefnrtv]$/);
>
> Problems in this part of the code X-<.
>
> * Was there a reason not to unwrap '\e' to "\e"?It was not mentioned in description of git pathname quoting in the message http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2;
> * The vertical tab is \013 (decimal 11), not \011 (which is TAB).
Oops. ASCII 11 is decimal 11.
> * The name and the abbreviated name of the character "\n" are > "line feed" and "LF"; I personally do not think these > character name comments are needed in this part of the code, > but I do not object if you want to have them there, as long > as you spell them correctly. cf. ISO/IEC 6429:1992 or > http://www.unicode.org/charts/PDF/U0000.pdf for example.
Perhaps it should be "LF ('\n') and TAB ('\t')".> * The hash %es and the pattern /[abef...]/ must be kept in
> sync; it is a maintenance nightmare,
>
> * Worse yet, they do not agree even in this initial version,
> which proves the previous point.
>
> Perhaps this is better written as:
>
> if (exists $es{$seq}) {
> return $es{$seq};
> }Fact.
Or
return $es{$seq} if exists $es{$seq};-- Jakub Narebski
Junio C HamanoRe: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
Jakub Narebski <jnareb@gmail.com> writes:
> Perhaps it should be "LF ('\n') and TAB ('\t')".Official terminology seems to call \t "HT", but my feeling is that we would not need that comment there.
> Or
> return $es{$seq} if exists $es{$seq};Although gitweb is full of that syntax, I personally do not like it very much. It is really hard to read when you are trying to skim through the code quickly. You would have to say "why return it? ah -- only when it exists, then it makes sense", which is a hiccup that disrupts the thought process.
Jakub NarebskiRe: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
Dnia piątek 3. listopada 2006 12:58, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Perhaps it should be "LF ('\n') and TAB ('\t')".
>
> Official terminology seems to call \t "HT", but my feeling is
> that we would not need that comment there.O.K. I'll remove that comment.
>> Or
>> return $es{$seq} if exists $es{$seq};
>
> Although gitweb is full of that syntax, I personally do not like
> it very much. It is really hard to read when you are trying to
> skim through the code quickly. You would have to say "why
> return it? ah -- only when it exists, then it makes sense",
> which is a hiccup that disrupts the thought process.O.K., I'll change style from
return chr(oct($seq)) if ($seq =~ m/^[0-7]{1,3}$/);
return $es{$seq} if exists $es{$seq};
return $seq;to the more readable form
if ($seq =~ m/^[0-7]{1,3}$/) {
return chr(oct($seq));
} elsif (exists $es{$seq}) {
return $es{$seq};
}
return $seq;I think it is better to leave final "return $seq;" outside else block.
-- Jakub Narebski
Jakub Narebski[PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Use "&iquot;" Latin 1 entity ("¿" -- inverted question mark = turned question mark, U+00BF ISOnum) instead '?' as replacements for control characters and other undisplayable characters.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- gitweb/gitweb.perl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a15e916..edca27d 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -567,7 +567,7 @@ sub esc_html { sub esc_path { my $str = shift; $str = esc_html($str); - $str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/?/g; # like --hide-control-chars in ls + $str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/¿/g; # like --hide-control-chars in ls return $str; }
-- 1.4.3.3
Junio C HamanoJakub Narebski <jnareb@gmail.com> writes:
> Use "&iquot;" Latin 1 entity ("¿" -- inverted question mark =
> turned question mark, U+00BF ISOnum) instead '?' as replacements for
> control characters and other undisplayable characters.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>Do you have something against our Spanish and Latin American friends? ;-)
I wonder if there is a more suitable replacement character that is accepted across scripts?
Japanese printing industry has a long tradition of using U+3013 ("geta") as a filler character. Originally they placed a type of otherwise unused character upside down while packing types into a row, and the reverse side of a type, when inked and printed, left imprint that looked like footprint somebody who wore a "geta" (a traditional footware) would leave.
http://ja.wikipedia.org/wiki/%E4%B8%8B%E9%A7%84
shows how a "geta" looks like, and
http://unicode.org/charts/PDF/U3000.pdf
shows how the filler character looks like.
Note that I am not suggesting to use ௅ as a replacement at all. I however think inverted question is inappropriate, and we should pick something else if we are fixing the question mark which is obviously inappropriate.
Junio C HamanoJunio C Hamano <junkio@cox.net> writes:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Use "&iquot;" Latin 1 entity ("¿" -- inverted question mark =
>> turned question mark, U+00BF ISOnum) instead '?' as replacements for
>> control characters and other undisplayable characters.
>>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>
> Do you have something against our Spanish and Latin American
> friends? ;-)
>
> I wonder if there is a more suitable replacement character that
> is accepted across scripts?I have a suspicion that instead of finding an exotic character, just showing the byte value in \octal, perhaps in different color, might be more portable and easier. For one thing, it helps to show the exact byte value than just one substitution character if you are troubleshooting gitweb.
Jakub NarebskiJunio C Hamano wrote:
>
> Junio C Hamano <junkio@cox.net> writes:
>
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> Use "&iquot;" Latin 1 entity ("¿" -- inverted question mark =
>>> turned question mark, U+00BF ISOnum) instead '?' as replacements for
>>> control characters and other undisplayable characters.
>>>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>
>> Do you have something against our Spanish and Latin American
>> friends? ;-)
>>
>> I wonder if there is a more suitable replacement character that
>> is accepted across scripts?
>
> I have a suspicion that instead of finding an exotic character,
> just showing the byte value in \octal, perhaps in different
> color, might be more portable and easier. For one thing, it
> helps to show the exact byte value than just one substitution
> character if you are troubleshooting gitweb.Will do. Well, with the exception of control characters which have literal escape characters, for example showing LF as \n instead of \octal. By the way, wouldn't \xhex be better?
Different color/style is good. Otherwise we would have escape escape character, i.e. write '\\' and perhaps als mark filename as quoted. git surrounds filename with doublequotes, "<quoted filename>", adding '"' to the list of quoted characters: for gitweb this kind of marking filename as quoted is PITA, as we first, don't want I think to include '"' in link ("_filename_" vs _"filename"_), and second and more important if we hyperlink parts of filename we would want for quote to be placed outside whole filename ("a/_filename_" vs a/_"filename"_ and also "_path_/_to_/_file_" vs _path_/_to_/_"file"_).
Should we mark only replaced character, or the whole filename?
On the technical side, should I send the patch as replacement for this patch, or on top of this patch?
-- Jakub Narebski
Jakub NarebskiJunio C Hamano wrote:
>
> Junio C Hamano <junkio@cox.net> writes:
>
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> Use "&iquot;" Latin 1 entity ("¿" -- inverted question mark =
>>> turned question mark, U+00BF ISOnum) instead '?' as replacements for
>>> control characters and other undisplayable characters.
>>>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>
>> Do you have something against our Spanish and Latin American
>> friends? ;-)
>>
>> I wonder if there is a more suitable replacement character that
>> is accepted across scripts?
>
> I have a suspicion that instead of finding an exotic character,
> just showing the byte value in \octal, perhaps in different
> color, might be more portable and easier. For one thing, it
> helps to show the exact byte value than just one substitution
> character if you are troubleshooting gitweb.
Or perhaps we can use Unicode Control Pictures U+2400 – U+243F
(9216–9279) ("Unicode quoting"):http://www.alanwood.net/unicode/control_pictures.html http://www.unicode.org/charts/PDF/U2400.pdf
# quote unsafe characters and escape filename to HTML
sub esc_path {
my $str = shift;
$str = esc_html($str);
$str =~ s!([[:cntrl:]])!sprintf('<span class="cntrl">&#%04d;</span>', 9216+ord($1))!eg;
return $str;
}with perhaps the following CSS
span.cntrl {
border: dashed #aaaaaa;
border-width: 1px;
padding: 0px 2px 0px 2px;
margin: 0px 2px 0px 2px;
}What do you think of it?
-- Jakub Narebski
Junio C HamanoJakub Narebski <jnareb@gmail.com> writes:
> # quote unsafe characters and escape filename to HTML
> sub esc_path {
> my $str = shift;
> $str = esc_html($str);
> $str =~ s!([[:cntrl:]])!sprintf('<span class="cntrl">&#%04d;</span>', 9216+ord($1))!eg;
> return $str;
> }
>
> with perhaps the following CSS
>
> span.cntrl {
> border: dashed #aaaaaa;
> border-width: 1px;
> padding: 0px 2px 0px 2px;
> margin: 0px 2px 0px 2px;
> }
>
> What do you think of it?Probably "# quote unsafe characters" is not what it does yet (it just quotes controls currently and nothing else), but we have to start somewhere and I think this is a good start.
Jakub NarebskiJunio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> # quote unsafe characters and escape filename to HTML
>> sub esc_path {
>> my $str = shift;
>> $str = esc_html($str);
>> $str =~ s!([[:cntrl:]])!sprintf('<span class="cntrl">&#%04d;</span>', 9216+ord($1))!eg;
>> return $str;
>> }
>>
>> with perhaps the following CSS
>>
>> span.cntrl {
>> border: dashed #aaaaaa;
>> border-width: 1px;
>> padding: 0px 2px 0px 2px;
>> margin: 0px 2px 0px 2px;
>> }
>>
>> What do you think of it?
>
> Probably "# quote unsafe characters" is not what it does yet (it
> just quotes controls currently and nothing else), but we have to
> start somewhere and I think this is a good start.Well, control characters (at least some of them) are not correct characters in UTF-8 HTML output; Mozilla in strict XHTML mode complains. Currently for example esc_html escapes FORM FEED (FF) and ESCAPE (ESC) characters, because they happened to be present in git.git repository (in COPYING file and in commit v1.4.2.1-g20a3847 respectively).
As I see it, we can either replace non-safe characters (control characters) by single characters a la --hide-control-chars: that is minimal solution, or we can quote unseafe characters somewhat, but if we do that we have to indicate that we quote. Git core and ls encloses material which needs escaping with quotes; in gitweb it is somewhat impractical; besides we have more possibilities to mark fragment of text (span element encompassing representation of escaped characters for example).
I have thought of the following escaping:
1. Hide control characters using '?' or other similar character like ċ for example 2. Use "Unicode" quoting, i.e. replace control characters by their Unicode Printable Representation (PR), as shown above. Has the advantage that it is simple and does not need theoretically marking that it is quoted; has the disadvantage that browser must support this part of Unicode, and that those characters are less than readable with default font size gitweb uses. 3. Use Character Escape Codes (CEC), using alphabetic and octal backslash sequences like those used in C. Probably need to escape backslash (quoting character) too. Has the advantage of being widely understood in POSIX world. Has the disadvantage of need for escape sequence table/hash. Has the advantage that it works for all characters - simple octal backslash sequence if they have no special escape sequence. 4. Control key Sequence (CS), like the one used in esc_html currently, replacing control characters by key sequence that produces them, for example replacing LF with ^J, CR with ^M, FF with ^L, ESC with ^[, TAB with ^I. Has the advantage of being undestodd I think in MS-DOS/MS WIndows world. Has the advantage of being used in esc_html. Has the advantage that some text editors use this representation. Has the disadvantage of need for large key sequence table/hash. Has the disadvantage that less common control characters have cryptic control key sequences. 5. Percent encoding, also know as URL encoding. Use %<hex> encoding used in URL, taken for example from core of esc_url/esc_param subroutine. Simple, but does need marking that is escaped. Disadvantage of hardly readable.
Which solution do you think it's best? Or perhaps other solution, like using Unicode Printable Representation, or Character Escape Codes with the exception of LF which would be replaced by ¶ (paragraph sign), RET by ↵ and TAB by either þ, ↦ or →.
-- Jakub Narebski
Junio C HamanoJakub Narebski <jnareb@gmail.com> writes:
> Well, control characters (at least some of them) are not correct > characters in UTF-8 HTML output; Mozilla in strict XHTML mode complains. > Currently...
I said you quote controls (and only controls) as unsafe, so you are not disagreeing with me here. However "controls are unsafe" does not necessarily mean "unsafe characters are all controls", does it? That was what my comments abuot the comment before the function was.
> Which solution do you think it's best?
Sorry, if it was not clear in my message, I wanted to say that I kinda liked those "control pictures" in U+2400 range.
Petr BaudisDear diary, on Fri, Nov 03, 2006 at 11:44:48PM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...
> Jakub Narebski <jnareb@gmail.com> writes: > > Which solution do you think it's best? > > Sorry, if it was not clear in my message, I wanted to say that I > kinda liked those "control pictures" in U+2400 range.
In principle, right now it should be pretty easy for a project that for some reason does not use UTF-8 in commits etc. to adjust gitweb to work properly, right? Just change the encoding in HTTP headers and you're done, I think.
Is it worth trying to preserve that flexibility?
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1Jakub NarebskiPetr Baudis wrote:
> Dear diary, on Fri, Nov 03, 2006 at 11:44:48PM CET, I got a letter > where Junio C Hamano <junkio@cox.net> said that... >> Jakub Narebski <jnareb@gmail.com> writes: >> > Which solution do you think it's best? >> >> Sorry, if it was not clear in my message, I wanted to say that I >> kinda liked those "control pictures" in U+2400 range. > > In principle, right now it should be pretty easy for a project that for > some reason does not use UTF-8 in commits etc. to adjust gitweb to work > properly, right? Just change the encoding in HTTP headers and you're > done, I think. > > Is it worth trying to preserve that flexibility?
It should be also quite easy to change to_uft8 subroutine to actually convert to utf8. But even if we don't use UTF-8 encoding for HTML in gitweb, numerical character entities (and that's how those "control pictures" are entered) should work equally well regardless of encoding.
-- Jakub Narebski Warsaw, Poland ShadeHawk on #git
Junio C HamanoPetr Baudis <pasky@suse.cz> writes:
> Dear diary, on Fri, Nov 03, 2006 at 11:44:48PM CET, I got a letter > where Junio C Hamano <junkio@cox.net> said that... >> Jakub Narebski <jnareb@gmail.com> writes: >> > Which solution do you think it's best? >> >> Sorry, if it was not clear in my message, I wanted to say that I >> kinda liked those "control pictures" in U+2400 range. > > In principle, right now it should be pretty easy for a project that for > some reason does not use UTF-8 in commits etc. to adjust gitweb to work > properly, right? Just change the encoding in HTTP headers and you're > done, I think. > > Is it worth trying to preserve that flexibility?
Absolutely, and I got your point. Maybe <blink>\011</blink> would be more portable and appropriate.
Also munging [:cntrl:] would break iso-2022 encoding if it munges ESC, but the function under discussion was only for paths and I think no sane platform would use iso-2022 for filenames.
Each repository has commit charset/encoding configuration item if I am not mistaken, so it is probably a sane thing to do to convert commit messages from that uniformly to utf-8, I think.
Petr BaudisDear diary, on Sat, Nov 04, 2006 at 01:02:43AM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...
> Petr Baudis <pasky@suse.cz> writes: > > > Dear diary, on Fri, Nov 03, 2006 at 11:44:48PM CET, I got a letter > > where Junio C Hamano <junkio@cox.net> said that... > >> Jakub Narebski <jnareb@gmail.com> writes: > >> > Which solution do you think it's best? > >> > >> Sorry, if it was not clear in my message, I wanted to say that I > >> kinda liked those "control pictures" in U+2400 range. > > > > In principle, right now it should be pretty easy for a project that for > > some reason does not use UTF-8 in commits etc. to adjust gitweb to work > > properly, right? Just change the encoding in HTTP headers and you're > > done, I think. > > > > Is it worth trying to preserve that flexibility? > > Absolutely, and I got your point. Maybe <blink>\011</blink> > would be more portable and appropriate.
Actually Jakub is right, &#number; is always a Unicode codepoint, regardless of document encoding.
Anything but <blink>! ;-)
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1Jakub NarebskiJakub Narebski wrote:
> 3. Use Character Escape Codes (CEC), using alphabetic and octal > backslash sequences like those used in C. Probably need to escape > backslash (quoting character) too. Has the advantage of being widely > understood in POSIX world. Has the disadvantage of need for escape > sequence table/hash. Has the advantage that it works for all > characters - simple octal backslash sequence if they have no special > escape sequence.
Here is example code for this:
sub esc_path {
my $str = shift; sub quot {
my $seq = shift;
my %es = (
"\t" => '\t', # tab (HT, TAB)
"\n" => '\n', # newline (NL)
"\r" => '\r', # return (CR)
"\f" => '\f', # form feed (FF)
"\b" => '\b', # backspace (BS)
"\a" => '\a', # alarm (bell) (BEL)
"\e" => '\e', # escape (ESC)
"\013" => '\v', # vertical tab (VT)
); if (exists $es{seq}) {
return $es{$seq};
}
return sprintf("\\%03o", ord($seq));
}
$str = esc_html($str);
$str =~ s/([[:cntrl:]])/'<span class="cntrl">' . quot($1) . '</span>'/eg;
return $str;
}-- Jakub Narebski
Junio C HamanoJakub Narebski <jnareb@gmail.com> writes:
> Here is example code for this:
Ok. The issues I raised in the previous round seem to have been addressed. Maybe you would want not to use nested 'sub' and it is good to go, I think.
Jakub NarebskiJunio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes: > >> Here is example code for this: > > Ok. The issues I raised in the previous round seem to have been > addressed. Maybe you would want not to use nested 'sub' and it > is good to go, I think.
Nested sub makes it easy to change gitweb quoting from Character Escape Codes (CEC) to e.g. Unicode Printable Representation (PR).
-- Jakub Narebski Warsaw, Poland ShadeHawk on #git
Jakub NarebskiJunio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes: > >> Here is example code for this: > > Ok. The issues I raised in the previous round seem to have been > addressed. Maybe you would want not to use nested 'sub' and it > is good to go, I think.
Should I understand this as a statement that you prefer backslash sequences aka. Character Escape Codes (CEC) than "Unicode" escaping aka. Unicode Printable Representation (PR)?
Should I send better quoting/unquoting work as two patches: unquote correction plus '?' using esc_path + esc_path which uses backslash sequences and span.cntrl element, or should it be send as one, admittedly quite large patch.
I don't think it would be good idea to separate unquote correction with esc_path work, because havin unquote which unquotes fully means that we can have filenames which have for exampl newline characters in them, hence the need of separate quoting subroutine, esc_path, and using it for filename escaping.
-- Jakub Narebski
Junio C HamanoJakub Narebski <jnareb@gmail.com> writes:
> I don't think it would be good idea to separate unquote correction with > esc_path work, because havin unquote which unquotes fully means that we > can have filenames which have for exampl newline characters in them, > hence the need of separate quoting subroutine, esc_path, and using it > for filename escaping.
Yeah, I think that makes sense.
No matter what escaping we would end up picking in the second part, we should do the unquote plus esc_path anyway, so I think a two patch series to replace to '?' first and then pretty-print that '?' with whatever encoding and span.cntrl is a very sensible way to go.
As to backslash, cute control pictures and just plain '?', I do not have a strong preference among them. Especially if you have the span.cntrl around them, we can tell even a plain '?' from the substitution of funny byte values, so it probably is just a matter of taste and I'd leave that up to you and gitweb cabal on the list ;-).
Jakub Narebski[PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filenames with LF
Use 's' (treat string as single line) regexp modifier in git_get_hash_by_path (against future changes, probably unnecessary) and in parse_ls_tree_line (when called with '-z'=>1 option) to secure against filenames containing newline.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- Without this patch filename with LF broke "tree" view.
gitweb/gitweb.perl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index edca27d..0fd1360 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -890,7 +890,7 @@ sub git_get_hash_by_path { close $fd or return undef; #'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c' - $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/; + $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/s; if (defined $type && $type ne $2) { # type doesn't match return undef; @@ -1305,7 +1305,7 @@ sub parse_ls_tree_line ($;%) { my %res; #'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c' - $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/; + $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/s; $res{'mode'} = $1; $res{'type'} = $2;
-- 1.4.3.3
Jakub Narebski[PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Add "--" after <commit-ish> or <tree-ish> argument to clearly mark it as <commit-ish> or <tree-ish> and not pathspec, securing against refs with the same names as files or directories in [live] repository.
Some wrapping to reduce line length as well.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- I uses branch named 'gitweb/test' to test gitweb against files with funny characters (like '"', '\', TAB, LF) in filenames. I run gitweb on "live" (not bare) repository, and there is directory 'gitweb/test' in it. So I had some parts of gitweb not functioning, and errors in the web server logs. This patch fixes that issue.
gitweb/gitweb.perl | 38 +++++++++++++++++++++++--------------- 1 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 0fd1360..4554067 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1141,7 +1141,9 @@ sub parse_commit { @commit_lines = @$commit_text; } else { local $/ = "\0"; - open my $fd, "-|", git_cmd(), "rev-list", "--header", "--parents", "--max-count=1", $commit_id + open my $fd, "-|", git_cmd(), "rev-list", + "--header", "--parents", "--max-count=1", + $commit_id, "--" or return; @commit_lines = split '\n', <$fd>; close $fd or return; @@ -2559,7 +2561,7 @@ sub git_summary { } open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17", - git_get_head_hash($project) + git_get_head_hash($project), "--" or die_error(undef, "Open git-rev-list failed"); my @revlist = map { chomp; $_ } <$fd>; close $fd; @@ -2970,7 +2972,7 @@ sub git_tree { } } $/ = "\0"; - open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash + open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash, "--" or die_error(undef, "Open git-ls-tree failed"); my @entries = map { chomp; $_ } <$fd>; close $fd or die_error(undef, "Reading tree failed"); @@ -3102,7 +3104,7 @@ sub git_log { my $refs = git_get_references(); my $limit = sprintf("--max-count=%i", (100 * ($page+1))); - open my $fd, "-|", git_cmd(), "rev-list", $limit, $hash + open my $fd, "-|", git_cmd(), "rev-list", $limit, $hash, "--" or die_error(undef, "Open git-rev-list failed"); my @revlist = map { chomp; $_ } <$fd>; close $fd; @@ -3160,7 +3162,7 @@ sub git_commit { $parent = "--root"; } open my $fd, "-|", git_cmd(), "diff-tree", '-r', "--no-commit-id", - @diff_opts, $parent, $hash + @diff_opts, $parent, $hash, "--" or die_error(undef, "Open git-diff-tree failed"); my @difftree = map { chomp; $_ } <$fd>; close $fd or die_error(undef, "Reading git-diff-tree failed"); @@ -3265,7 +3267,8 @@ sub git_blobdiff { if (defined $hash_base && defined $hash_parent_base) { if (defined $file_name) { # read raw output - open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, $hash_parent_base, $hash_base, + open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, + $hash_parent_base, $hash_base, "--", $file_name or die_error(undef, "Open git-diff-tree failed"); @difftree = map { chomp; $_ } <$fd>; @@ -3279,7 +3282,8 @@ sub git_blobdiff { # try to find filename from $hash # read filtered raw output - open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, $hash_parent_base, $hash_base + open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, + $hash_parent_base, $hash_base, "--" or die_error(undef, "Open git-diff-tree failed"); @difftree = # ':100644 100644 03b21826... 3b93d5e7... M ls-files.c' @@ -3349,7 +3353,8 @@ sub git_blobdiff { } # open patch output - open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts, $hash_parent, $hash + open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts, + $hash_parent, $hash, "--" or die_error(undef, "Open git-diff failed"); } else { die_error('404 Not Found', "Missing one of the blob diff parameters") @@ -3480,8 +3485,8 @@ sub git_commitdiff { my @difftree; if ($format eq 'html') { open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, - "--no-commit-id", - "--patch-with-raw", "--full-index", $hash_parent, $hash + "--no-commit-id", "--patch-with-raw", "--full-index", + $hash_parent, $hash, "--" or die_error(undef, "Open git-diff-tree failed"); while (chomp(my $line = <$fd>)) { @@ -3492,7 +3497,7 @@ sub git_commitdiff { } elsif ($format eq 'plain') { open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, - '-p', $hash_parent, $hash + '-p', $hash_parent, $hash, "--" or die_error(undef, "Open git-diff-tree failed"); } else { @@ -3669,7 +3674,9 @@ sub git_search { my $alternate = 1; if ($searchtype eq 'commit' or $searchtype eq 'author' or $searchtype eq 'committer') { $/ = "\0"; - open my $fd, "-|", git_cmd(), "rev-list", "--header", "--parents", $hash or next; + open my $fd, "-|", git_cmd(), "rev-list", + "--header", "--parents", $hash, "--" + or next; while (my $commit_text = <$fd>) { if (!grep m/$searchtext/i, $commit_text) { next; @@ -3815,7 +3822,7 @@ sub git_shortlog { my $refs = git_get_references(); my $limit = sprintf("--max-count=%i", (100 * ($page+1))); - open my $fd, "-|", git_cmd(), "rev-list", $limit, $hash + open my $fd, "-|", git_cmd(), "rev-list", $limit, $hash, "--" or die_error(undef, "Open git-rev-list failed"); my @revlist = map { chomp; $_ } <$fd>; close $fd; @@ -3843,7 +3850,8 @@ sub git_shortlog { sub git_rss { # http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ - open my $fd, "-|", git_cmd(), "rev-list", "--max-count=150", git_get_head_hash($project) + open my $fd, "-|", git_cmd(), "rev-list", "--max-count=150", + git_get_head_hash($project), "--" or die_error(undef, "Open git-rev-list failed"); my @revlist = map { chomp; $_ } <$fd>; close $fd or die_error(undef, "Reading git-rev-list failed"); @@ -3867,7 +3875,7 @@ XML } my %cd = parse_date($co{'committer_epoch'}); open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, - $co{'parent'}, $co{'id'} + $co{'parent'}, $co{'id'}, "--" or next; my @difftree = map { chomp; $_ } <$fd>; close $fd
-- 1.4.3.3
Jakub NarebskiRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Jakub Narebski wrote:
> @@ -2970,7 +2972,7 @@ sub git_tree {
> }
> }
> $/ = "\0";
> - open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
> + open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash, "--"
> or die_error(undef, "Open git-ls-tree failed");
> my @entries = map { chomp; $_ } <$fd>;
> close $fd or die_error(undef, "Reading tree failed");Please remove this chunk from patch!. It makes gitweb "tree" view empty. I have forgot that git-ls-tree _requires_ <tree-ish> so there is no way to mistake pathspec with <tree-ish>.
Bit overeager adding of "--"...
-- Jakub Narebski
Junio C HamanoRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Jakub Narebski <jnareb@gmail.com> writes:
> Please remove this chunk from patch!. It makes gitweb "tree" view > empty. I have forgot that git-ls-tree _requires_ <tree-ish> so there > is no way to mistake pathspec with <tree-ish>.
To be honest, I dislike these */n series where the the end is unknown. It just confuses me what's still surviving, what's already shot down, and what's being rerolled.
Let's step back a bit and see if we share the same view as to the status of each one:
[PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p...
Marked preliminary, perhaps need some discussion and rerolling but I haven't looked at it.
[PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Discussed; we agreed that showing byte values in different colors is preferable. Waiting for re-roll.
[PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena...
I looked at it although haven't said anything yet. Probably a safe and good change but I wonder how LF at the end of the line matches /...(.+)$/s pattern; iow, if we do not use -z does it still do the right thing? Otherwise I suspect you would perhaps need to chomp?
[PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same...
Good fix and even improves readability; will apply after dropping -- from ls-tree args.
[PATCH 5/n] [take 3] gitweb: New improved patchset view [PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_... [PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view [PATCH 8/n] gitweb: Fix two issues with quoted filenames in git_patc...
Haven't looked at them and I do not think people have had enough time to comment on them yet.
Jakub NarebskiRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Junio C Hamano wrote:
> > To be honest, I dislike these */n series where the the end is > unknown. It just confuses me what's still surviving, what's > already shot down, and what's being rerolled.
Well, it looks like this patch series is closing to final patch. The "New improved patchset view" is done.
> Let's step back a bit and see if we share the same view as to > the status of each one: > > [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p... > > Marked preliminary, perhaps need some discussion and rerolling > but I haven't looked at it.
I'm not sure if without this patch (well, the unquote part) gitweb can work with filenames which git quotes using escape sequences, like ", \, LF, TAB. Former version didn't unquote fully, and it passed partially unquoted filename to git.
> [PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path > > Discussed; we agreed that showing byte values in different > colors is preferable. Waiting for re-roll.
The problem with using text color or background color is that the filenames tends to be shown with different color and background color: "tree" view, parts of difftree, parts of diff header, etc. Perhaps text-decoration: overline;? Just kidding...
> [PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena... > > I looked at it although haven't said anything yet. Probably a > safe and good change but I wonder how LF at the end of the line > matches /...(.+)$/s pattern; iow, if we do not use -z does it > still do the right thing? Otherwise I suspect you would perhaps > need to chomp?
We always pass chomped lines. First chunk is unnecessary (we care only for type), without second "tree" view look strange for files with embedded newline in filename.
> [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same... > > Good fix and even improves readability; will apply after > dropping -- from ls-tree args.
As I said, noticed while testing gitweb with strange filenames in 'gitweb/test' branch.
> [PATCH 5/n] [take 3] gitweb: New improved patchset view > [PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_... > [PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view > [PATCH 8/n] gitweb: Fix two issues with quoted filenames in git_patc... > > Haven't looked at them and I do not think people have had enough > time to comment on them yet.
Well, patch 5 and 8 could be collapsed.
-- Jakub Narebski
Junio C HamanoRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Jakub Narebski <jnareb@gmail.com> writes:
>> [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p... >> >> Marked preliminary, perhaps need some discussion and rerolling >> but I haven't looked at it. > > I'm not sure if without this patch (well, the unquote part) gitweb > can work with filenames which git quotes using escape sequences,
I am reasonably sure it wouldn't, and it sounded like you wanted to fix it better than the preliminary one, so I think we are in agreement.
>> [PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path >> >> Discussed; we agreed that showing byte values in different >> colors is preferable. Waiting for re-roll. > > The problem with using text color or background color is that > the filenames tends to be shown with different color and background > color: "tree" view, parts of difftree, parts of diff header, etc. > Perhaps text-decoration: overline;? Just kidding...
Use of overstrike may actually not be a bad thing. It _is_ unusual situation after all.
>> [PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena... >> >> I looked at it although haven't said anything yet. Probably a >> safe and good change but I wonder how LF at the end of the line >> matches /...(.+)$/s pattern; iow, if we do not use -z does it >> still do the right thing? Otherwise I suspect you would perhaps >> need to chomp? > > We always pass chomped lines. First chunk is unnecessary (we care only > for type), without second "tree" view look strange for files with > embedded newline in filename.
The codepath affected by the first chunk does not chomp, which was what I was referring to. So in the meantime will apply only the second hunk.
>> [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same... >> >> Good fix and even improves readability; will apply after >> dropping -- from ls-tree args.
I just applied this. I'll be pushing out a "master" update sometime today, and do not expect to be able to get to your "n turned out to be ten" series, so it might be worthwhile to reroll the remaining bits that you still care about on top of what I push out tonight to make sure we are on the same page.
Preferably:
- you should avoid making a series out of more-or-less unrelated things;
- if you are doing related things in one series, do not send half-baked early parts out until you are finished and are confident with it. If you do not know how many patches you need to complete that logically single topic yet, that is a sure sign that you are not done. Instead, finish writing and testing it, and if your test finds an earlier mistake, especially a trivial one, go back and fix it in the earlier patch in the series. Everybody makes mistakes so fixing up before submission is a norm, and other people do not have to be forced to see your "oops" in the development history.
Thanks.
Jakub NarebskiRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Junio C Hamano wrote:
> I'll be pushing out a "master" update > sometime today, and do not expect to be able to get to your "n > turned out to be ten" series, so it might be worthwhile to > reroll the remaining bits that you still care about on top of > what I push out tonight to make sure we are on the same page.
I'll wait a while if there are any comments (for example on formatting used), and resend cleaned-up series.
> Preferably: > > - you should avoid making a series out of more-or-less > unrelated things;
Well, truly unrelated were adding "--" to secure against ref with the same name as path in the repository (but it was discovered during testing the series) and replacing ' ' with ' ' in blob and diff body for non-CSS aware browsers.
Better quoting and unquoting was needed for better commitdiff view. Ah, well, perhaps it is unrelated.
Securing against filenames with LF for example has sense only if there can be filenames with LF, and earlier gitweb unquoted it halfway leaving '\n' instead of LF.
> - if you are doing related things in one series, do not send > half-baked early parts out until you are finished and are > confident with it.
I've send series early to get some comments, but I see while I got some comments on "take 1" and "take 2" on _single_ "new commitdiff" RFC patch, I got comments only about half-baked '&iquot;' idea.
-- Jakub Narebski
Junio C HamanoRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Jakub Narebski <jnareb@gmail.com> writes:
> Junio C Hamano wrote: > >> I'll be pushing out a "master" update >> sometime today, and do not expect to be able to get to your "n >> turned out to be ten" series, so it might be worthwhile to >> reroll the remaining bits that you still care about on top of >> what I push out tonight to make sure we are on the same page. > > I'll wait a while if there are any comments (for example on formatting > used), and resend cleaned-up series. > ... > I've send series early to get some comments, but I see while I got > some comments on "take 1" and "take 2" on _single_ "new commitdiff" > RFC patch, I got comments only about half-baked '&iquot;' idea.
Well, I think some of the major reasons you did not get any response were:
(1) it was unclear where it started, where it was heading to,
and where it ended until you sent out "by the way n=10"
message at the end; (2) the first major oand interesting one in the series (5/n)
were linewrapped and could not be applied;and it is rather hard to comment on gitweb changes unless you view two instances of gitweb output side-by-side for before and after each patch.
I'd see if I can add some constructive comments on patches 5-10 tonight, but I'm in the middle of other things so don't hold your breath ;-).
Junio C HamanoRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Junio C Hamano <junkio@cox.net> writes:
> I'd see if I can add some constructive comments on patches 5-10 > tonight, but I'm in the middle of other things so don't hold > your breath ;-).
7 and 9 look obviously good, so I've applied them without others.
gitweb: Output also empty patches in "commitdiff" view
gitweb: Better support for non-CSS aware web browsers5 is terminally linewrapped and rather big to comment on without comparing pre- and post- patch outputs, so I'll refrain from commenting on it. 8 is "oops, I made a mistake when I did 5", which discourages me even more from looking at 5 X-<.
Jakub NarebskiRe: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes: > >> I'd see if I can add some constructive comments on patches 5-10 >> tonight, but I'm in the middle of other things so don't hold >> your breath ;-). > > 7 and 9 look obviously good, so I've applied them without > others. > > gitweb: Output also empty patches in "commitdiff" view
This patch in my opinion has no sense without having extended diff header in commitdiff view, i.e. without "New improved patchset view" (I have send non-line wrapped version).
> gitweb: Better support for non-CSS aware web browsers
Thats independent from other changes, true.
> 5 is terminally linewrapped and rather big to comment on without > comparing pre- and post- patch outputs, so I'll refrain from > commenting on it.
You can check out new gitweb at work at my site (when it is up) http://roke . dyndns . info/cgi-bin/gitweb/gitweb.cgi
> 8 is "oops, I made a mistake when I did 5", > which discourages me even more from looking at 5 X-<.
Well, you wouldn't notice error corrected by 8 unless you have files with funny filenames.
-- Jakub Narebski
Jakub Narebski[PATCH 5/n] [take 3] gitweb: New improved patchset view
Replace "gitweb diff header" with its full sha1 of blobs and replace it by "git diff" header and extended diff header. Change also somewhat highlighting of diffs.
Added `file_type_long' subroutine to convert file mode in octal to file type description (only for file modes which used by git).
Changes:
* "gitweb diff header" which looked for example like below:
file:_<sha1 before>_ -> file:_<sha1 after>_
where 'file' is file type and '<sha1>' is full sha1 of blob is
changed to
diff --git _a/<file before>_ _b/<file after>_
In both cases links are visible and use default link style. If file
is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
hyperlinked (and <hash> shortened to 7 characters), and <mode>
explained: '<mode>' is extended to '<mode> (<file type description>)',
where added text is slightly lighter to easy distinguish that it
was added (and it is difference from git-diff output).
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 2px, both barely visible.Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- It still does not output empty patches. This is to be adressed in later patch.
As usual, you can see it at work at my web page http://roke (.) dyndns (.) info/cgi-bin/gitweb/gitweb.cgi
Comments?
gitweb/gitweb.css | 66 +++++++++++++++---- gitweb/gitweb.perl | 187 ++++++++++++++++++++++++++++++++++------------------ 2 files changed, 176 insertions(+), 77 deletions(-)
gitweb/gitweb.css, gitweb/gitweb.perl
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css index 0eda982..87dbc52 100644 --- a/gitweb/gitweb.css +++ b/gitweb/gitweb.css @@ -291,40 +291,82 @@ td.mode { font-family: monospace; } -div.diff a.list { +/* styling of diffs (patchsets): commitdiff and blobdiff views */ +div.diff.header, +div.diff.extended_header { + white-space: normal; +} + +div.diff.header { + font-weight: bold; + + background-color: #edece6; + + margin-top: 4px; + padding: 4px 0px 2px 0px; + border: solid #d9d8d1; + border-width: 1px 0px 1px 0px; +} + +div.diff.header a.path { + text-decoration: underline; +} + +div.diff.extended_header, +div.diff.extended_header a.path, +div.diff.extended_header a.hash { + color: #777777; +} + +div.diff.extended_header .info { + color: #b0b0b0; +} + +div.diff.extended_header { + background-color: #f6f5ee; + padding: 2px 0px 2px 0px; +} + +div.diff a.path, +div.diff a.hash { text-decoration: none; } -div.diff a.list:hover { +div.diff a.path:hover, +div.diff a.hash:hover { text-decoration: underline; } -div.diff.to_file a.list, -div.diff.to_file, +div.diff.to_file a.path, +div.diff.to_file { + color: #007000; +} + div.diff.add { color: #008800; } -div.diff.from_file a.list, -div.diff.from_file, +div.diff.from_file a.path, +div.diff.from_file { + color: #aa0000; +} + div.diff.rem { color: #cc0000; } div.diff.chunk_header { color: #990099; + + border: dotted #ffe0ff; + border-width: 1px 0px 0px 0px; + margin-top: 2px; } div.diff.incomplete { color: #cccccc; } -div.diff_info { - font-family: monospace; - color: #000099; - background-color: #edece6; - font-style: italic; -} div.index_include { border: solid #d9d8d1; diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4554067..c22fcc5 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -732,6 +732,32 @@ sub file_type { } } +# convert file mode in octal to file type description string +sub file_type_long { + my $mode = shift; + + if ($mode !~ m/^[0-7]+$/) { + return $mode; + } else { + $mode = oct $mode; + } + + if (S_ISDIR($mode & S_IFMT)) { + return "directory"; + } elsif (S_ISLNK($mode)) { + return "symlink"; + } elsif (S_ISREG($mode)) { + if ($mode & S_IXUSR) { + return "executable"; + } else { + return "file"; + }; + } else { + return "unknown"; + } +} + + ## ---------------------------------------------------------------------- ## functions returning short HTML fragments, or transforming HTML fragments ## which don't beling to other sections @@ -2055,7 +2081,9 @@ sub git_patchset_body { my $patch_idx = 0; my $in_header = 0; my $patch_found = 0; + my $skip_patch = 0; my $diffinfo; + my (%from, %to); print "<div class=\"patchset\">\n"; @@ -2065,6 +2093,8 @@ sub git_patchset_body { if ($patch_line =~ m/^diff /) { # "git diff" header # beginning of patch (in patchset) + $skip_patch = 0; + if ($patch_found) { # close previous patch print "</div>\n"; # class="patch" @@ -2074,96 +2104,123 @@ sub git_patchset_body { } print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n"; + # read and prepare patch information if (ref($difftree->[$patch_idx]) eq "HASH") { + # pre-parsed (or generated by hand) $diffinfo = $difftree->[$patch_idx]; } else { $diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]); } + $from{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'}; + $to{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'}; + if ($diffinfo->{'status'} ne "A") { # not new (added) file + $from{'href'} = href(action=>"blob", hash_base=>$hash_parent, + hash=>$diffinfo->{'from_id'}, + file_name=>$from{'file'}); + } + if ($diffinfo->{'status'} ne "D") { # not deleted file + $to{'href'} = href(action=>"blob", hash_base=>$hash, + hash=>$diffinfo->{'to_id'}, + file_name=>$to{'file'}); + } $patch_idx++; - # for now, no extended header, hence we skip empty patches - # companion to next LINE if $in_header; + # for now we skip empty patches if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) { # no change $in_header = 1; + $skip_patch = 0; next LINE; } - if ($diffinfo->{'status'} eq "A") { # added - print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'to_id'}) . " (new)" . - "</div>\n"; # class="diff_info" - - } elsif ($diffinfo->{'status'} eq "D") { # deleted - print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'from_id'}) . " (deleted)" . - "</div>\n"; # class="diff_info" - - } elsif ($diffinfo->{'status'} eq "R" || # renamed - $diffinfo->{'status'} eq "C" || # copied - $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff) - print "<div class=\"diff_info\">" . - file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})}, - $diffinfo->{'from_id'}) . - " -> " . - file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})}, - $diffinfo->{'to_id'}); - print "</div>\n"; # class="diff_info" - - } else { # modified, mode changed, ... - print "<div class=\"diff_info\">" . - file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'from_id'}) . - " -> " . - file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'to_id'}); - print "</div>\n"; # class="diff_info" + # print "git diff" header + $patch_line =~ s!^(diff (.*?) )a/.*$!$1!; + if ($from{'href'}) { + $patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"}, + 'a/' . esc_path($from{'file'})); + } else { # file was added + $patch_line .= 'a/' . esc_path($from{'file'}); + } + $patch_line .= ' '; + if ($to{'href'}) { + $patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"}, + 'b/' . esc_path($to{'file'})); + } else { # file was deleted + $patch_line .= 'b/' . esc_path($to{'file'}); } - #print "<div class=\"diff extended_header\">\n"; + print "<div class=\"diff header\">$patch_line</div>\n"; + print "<div class=\"diff extended_header\">\n"; $in_header = 1; next LINE; + } else { + # actual skipping of empty patches + next LINE if $skip_patch; } # start of patch in patchset + if ($in_header) { + if ($patch_line !~ m/^---/) { + # match <path> + if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) { + $patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"}, + esc_path($from{'file'})); + } + if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) { + $patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"}, + esc_path($to{'file'})); + } + # match <mode> + if ($patch_line =~ m/\s(\d{6})$/) { + $patch_line .= '<span class="info"> (' . + file_type_long($1) . + ')</span>'; + } + # match <hash> + if ($patch_line =~ m/^index/) { + my ($from_link, $to_link); + if ($from{'href'}) { + $from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"}, + substr($diffinfo->{'from_id'},0,7)); + } else { + $from_link = '0' x 7; + } + if ($to{'href'}) { + $to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"}, + substr($diffinfo->{'to_id'},0,7)); + } else { + $to_link = '0' x 7; + } + my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'}); + $patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!; + } + print $patch_line . "<br/>\n"; - if ($in_header && $patch_line =~ m/^---/) { - #print "</div>\n"; # class="diff extended_header" - $in_header = 0; + } else { + #$in_header && $patch_line =~ m/^---/; + print "</div>\n"; # class="diff extended_header" + $in_header = 0; + + if ($from{'href'}) { + $patch_line = '--- a/' . + $cgi->a({-href=>$from{'href'}, -class=>"path"}, + esc_path($from{'file'})); + } + print "<div class=\"diff from_file\">$patch_line</div>\n"; - my $file = $diffinfo->{'from_file'}; - $file ||= $diffinfo->{'file'}; - $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$file), - -class => "list"}, esc_path($file)); - $patch_line =~ s|a/.*$|a/$file|g; - print "<div class=\"diff from_file\">$patch_line</div>\n"; + $patch_line = <$fd>; + chomp $patch_line; - $patch_line = <$fd>; - chomp $patch_line; + #$patch_line =~ m/^+++/; + if ($to{'href'}) { + $patch_line = '+++ b/' . + $cgi->a({-href=>$to{'href'}, -class=>"path"}, + esc_html($to{'file'})); + } + print "<div class=\"diff to_file\">$patch_line</div>\n"; - #$patch_line =~ m/^+++/; - $file = $diffinfo->{'to_file'}; - $file ||= $diffinfo->{'file'}; - $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$file), - -class => "list"}, esc_path($file)); - $patch_line =~ s|b/.*|b/$file|g; - print "<div class=\"diff to_file\">$patch_line</div>\n"; + } next LINE; } - next LINE if $in_header; print format_diff_line($patch_line); }
-- 1.4.3.3
Jakub Narebski[PATCH 5/10] gitweb: New improved patchset view
Replace "gitweb diff header" with its full sha1 of blobs and replace it by "git diff" header and extended diff header. Change also somewhat highlighting of diffs.
Added `file_type_long' subroutine to convert file mode in octal to file type description (only for file modes which used by git).
Changes:
* "gitweb diff header" which looked for example like below:
file:_<sha1 before>_ -> file:_<sha1 after>_
where 'file' is file type and '<sha1>' is full sha1 of blob is
changed to
diff --git _a/<file before>_ _b/<file after>_
In both cases links are visible and use default link style. If file
is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
hyperlinked (and <hash> shortened to 7 characters), and <mode>
explained: '<mode>' is extended to '<mode> (<file type description>)',
where added text is slightly lighter to easy distinguish that it
was added (and it is difference from git-diff output).
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 2px, both barely visible.Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- I'm very sorry, previous version was linewrapped.
As usual, you can see new gitweb in action (after all the series, including this patch) at my site http://roke . dyndns . info/cgi-bin/gitweb/gitweb.cgi (anti-anti-spam protected against "DOT info" being vger banned word).
You can doenload and view snapshots at: http://repo.or.cz/w/git/jnareb-git.git/gitweb/test:gitweb/snapshots/ (before means current 'master' gitweb, middle means after this patch, after means after all patches in the series).
gitweb/gitweb.css | 66 +++++++++++++++---- gitweb/gitweb.perl | 187 ++++++++++++++++++++++++++++++++++------------------ 2 files changed, 176 insertions(+), 77 deletions(-)
gitweb/gitweb.css, gitweb/gitweb.perl
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css index 0eda982..87dbc52 100644 --- a/gitweb/gitweb.css +++ b/gitweb/gitweb.css @@ -291,40 +291,82 @@ td.mode { font-family: monospace; } -div.diff a.list { +/* styling of diffs (patchsets): commitdiff and blobdiff views */ +div.diff.header, +div.diff.extended_header { + white-space: normal; +} + +div.diff.header { + font-weight: bold; + + background-color: #edece6; + + margin-top: 4px; + padding: 4px 0px 2px 0px; + border: solid #d9d8d1; + border-width: 1px 0px 1px 0px; +} + +div.diff.header a.path { + text-decoration: underline; +} + +div.diff.extended_header, +div.diff.extended_header a.path, +div.diff.extended_header a.hash { + color: #777777; +} + +div.diff.extended_header .info { + color: #b0b0b0; +} + +div.diff.extended_header { + background-color: #f6f5ee; + padding: 2px 0px 2px 0px; +} + +div.diff a.path, +div.diff a.hash { text-decoration: none; } -div.diff a.list:hover { +div.diff a.path:hover, +div.diff a.hash:hover { text-decoration: underline; } -div.diff.to_file a.list, -div.diff.to_file, +div.diff.to_file a.path, +div.diff.to_file { + color: #007000; +} + div.diff.add { color: #008800; } -div.diff.from_file a.list, -div.diff.from_file, +div.diff.from_file a.path, +div.diff.from_file { + color: #aa0000; +} + div.diff.rem { color: #cc0000; } div.diff.chunk_header { color: #990099; + + border: dotted #ffe0ff; + border-width: 1px 0px 0px 0px; + margin-top: 2px; } div.diff.incomplete { color: #cccccc; } -div.diff_info { - font-family: monospace; - color: #000099; - background-color: #edece6; - font-style: italic; -} div.index_include { border: solid #d9d8d1; diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 4554067..c22fcc5 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -732,6 +732,32 @@ sub file_type { } } +# convert file mode in octal to file type description string +sub file_type_long { + my $mode = shift; + + if ($mode !~ m/^[0-7]+$/) { + return $mode; + } else { + $mode = oct $mode; + } + + if (S_ISDIR($mode & S_IFMT)) { + return "directory"; + } elsif (S_ISLNK($mode)) { + return "symlink"; + } elsif (S_ISREG($mode)) { + if ($mode & S_IXUSR) { + return "executable"; + } else { + return "file"; + }; + } else { + return "unknown"; + } +} + + ## ---------------------------------------------------------------------- ## functions returning short HTML fragments, or transforming HTML fragments ## which don't beling to other sections @@ -2055,7 +2081,9 @@ sub git_patchset_body { my $patch_idx = 0; my $in_header = 0; my $patch_found = 0; + my $skip_patch = 0; my $diffinfo; + my (%from, %to); print "<div class=\"patchset\">\n"; @@ -2065,6 +2093,8 @@ sub git_patchset_body { if ($patch_line =~ m/^diff /) { # "git diff" header # beginning of patch (in patchset) + $skip_patch = 0; + if ($patch_found) { # close previous patch print "</div>\n"; # class="patch" @@ -2074,96 +2104,123 @@ sub git_patchset_body { } print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n"; + # read and prepare patch information if (ref($difftree->[$patch_idx]) eq "HASH") { + # pre-parsed (or generated by hand) $diffinfo = $difftree->[$patch_idx]; } else { $diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]); } + $from{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'}; + $to{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'}; + if ($diffinfo->{'status'} ne "A") { # not new (added) file + $from{'href'} = href(action=>"blob", hash_base=>$hash_parent, + hash=>$diffinfo->{'from_id'}, + file_name=>$from{'file'}); + } + if ($diffinfo->{'status'} ne "D") { # not deleted file + $to{'href'} = href(action=>"blob", hash_base=>$hash, + hash=>$diffinfo->{'to_id'}, + file_name=>$to{'file'}); + } $patch_idx++; - # for now, no extended header, hence we skip empty patches - # companion to next LINE if $in_header; + # for now we skip empty patches if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) { # no change $in_header = 1; + $skip_patch = 0; next LINE; } - if ($diffinfo->{'status'} eq "A") { # added - print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'to_id'}) . " (new)" . - "</div>\n"; # class="diff_info" - - } elsif ($diffinfo->{'status'} eq "D") { # deleted - print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'from_id'}) . " (deleted)" . - "</div>\n"; # class="diff_info" - - } elsif ($diffinfo->{'status'} eq "R" || # renamed - $diffinfo->{'status'} eq "C" || # copied - $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff) - print "<div class=\"diff_info\">" . - file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})}, - $diffinfo->{'from_id'}) . - " -> " . - file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})}, - $diffinfo->{'to_id'}); - print "</div>\n"; # class="diff_info" - - } else { # modified, mode changed, ... - print "<div class=\"diff_info\">" . - file_type($diffinfo->{'from_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'from_id'}) . - " -> " . - file_type($diffinfo->{'to_mode'}) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})}, - $diffinfo->{'to_id'}); - print "</div>\n"; # class="diff_info" + # print "git diff" header + $patch_line =~ s!^(diff (.*?) )a/.*$!$1!; + if ($from{'href'}) { + $patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"}, + 'a/' . esc_path($from{'file'})); + } else { # file was added + $patch_line .= 'a/' . esc_path($from{'file'}); + } + $patch_line .= ' '; + if ($to{'href'}) { + $patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"}, + 'b/' . esc_path($to{'file'})); + } else { # file was deleted + $patch_line .= 'b/' . esc_path($to{'file'}); } - #print "<div class=\"diff extended_header\">\n"; + print "<div class=\"diff header\">$patch_line</div>\n"; + print "<div class=\"diff extended_header\">\n"; $in_header = 1; next LINE; + } else { + # actual skipping of empty patches + next LINE if $skip_patch; } # start of patch in patchset + if ($in_header) { + if ($patch_line !~ m/^---/) { + # match <path> + if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) { + $patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"}, + esc_path($from{'file'})); + } + if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) { + $patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"}, + esc_path($to{'file'})); + } + # match <mode> + if ($patch_line =~ m/\s(\d{6})$/) { + $patch_line .= '<span class="info"> (' . + file_type_long($1) . + ')</span>'; + } + # match <hash> + if ($patch_line =~ m/^index/) { + my ($from_link, $to_link); + if ($from{'href'}) { + $from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"}, + substr($diffinfo->{'from_id'},0,7)); + } else { + $from_link = '0' x 7; + } + if ($to{'href'}) { + $to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"}, + substr($diffinfo->{'to_id'},0,7)); + } else { + $to_link = '0' x 7; + } + my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'}); + $patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!; + } + print $patch_line . "<br/>\n"; - if ($in_header && $patch_line =~ m/^---/) { - #print "</div>\n"; # class="diff extended_header" - $in_header = 0; + } else { + #$in_header && $patch_line =~ m/^---/; + print "</div>\n"; # class="diff extended_header" + $in_header = 0; + + if ($from{'href'}) { + $patch_line = '--- a/' . + $cgi->a({-href=>$from{'href'}, -class=>"path"}, + esc_path($from{'file'})); + } + print "<div class=\"diff from_file\">$patch_line</div>\n"; - my $file = $diffinfo->{'from_file'}; - $file ||= $diffinfo->{'file'}; - $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$diffinfo->{'from_id'}, file_name=>$file), - -class => "list"}, esc_path($file)); - $patch_line =~ s|a/.*$|a/$file|g; - print "<div class=\"diff from_file\">$patch_line</div>\n"; + $patch_line = <$fd>; + chomp $patch_line; - $patch_line = <$fd>; - chomp $patch_line; + #$patch_line =~ m/^+++/; + if ($to{'href'}) { + $patch_line = '+++ b/' . + $cgi->a({-href=>$to{'href'}, -class=>"path"}, + esc_html($to{'file'})); + } + print "<div class=\"diff to_file\">$patch_line</div>\n"; - #$patch_line =~ m/^+++/; - $file = $diffinfo->{'to_file'}; - $file ||= $diffinfo->{'file'}; - $file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$diffinfo->{'to_id'}, file_name=>$file), - -class => "list"}, esc_path($file)); - $patch_line =~ s|b/.*|b/$file|g; - print "<div class=\"diff to_file\">$patch_line</div>\n"; + } next LINE; } - next LINE if $in_header; print format_diff_line($patch_line); }
-- 1.4.3.3
Jakub Narebski[PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_body
This revert parts of commit 4777b0141a4812177390da4b6ebc9d40ac3da4b5 "gitweb: Restore object-named links in item lists" by Petr Baudis, which bring back "blob" links in difftree-raw like part of "commit" and "commitdiff" views, namely in git_difftree_body subroutine. First, it did that incompletely: it did not add "blob" link for added files, and added block used mixture of tabs and spaces for align. Second, in "difftree" view the "blob" link is not the most interesting, *contrary* to "blob"/"tree" link in "tree" view, so it should be enough to have hidden link in the form of file name entry.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- I'm still for having those object-named links in "tree" view.
But perhaps it would be better to add "blob" links for all possible statuses, including added files... Pasky, Luben, Junio?
gitweb/gitweb.perl | 9 --------- 1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index c22fcc5..542dbca 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1967,9 +1967,6 @@ sub git_difftree_body { print $cgi->a({-href => "#patch$patchno"}, "patch"); print " | "; } - print $cgi->a({-href => href(action=>"blob", hash=>$diff{'from_id'}, - hash_base=>$parent, file_name=>$diff{'file'})}, - "blob") . " | "; print $cgi->a({-href => href(action=>"blame", hash_base=>$parent, file_name=>$diff{'file'})}, "blame") . " | "; @@ -2015,9 +2012,6 @@ sub git_difftree_body { } print " | "; } - print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'}, - hash_base=>$hash, file_name=>$diff{'file'})}, - "blob") . " | "; print $cgi->a({-href => href(action=>"blame", hash_base=>$hash, file_name=>$diff{'file'})}, "blame") . " | "; @@ -2058,9 +2052,6 @@ sub git_difftree_body { } print " | "; } - print $cgi->a({-href => href(action=>"blob", hash=>$diff{'from_id'}, - hash_base=>$parent, file_name=>$diff{'from_file'})}, - "blob") . " | "; print $cgi->a({-href => href(action=>"blame", hash_base=>$parent, file_name=>$diff{'from_file'})}, "blame") . " | ";
-- 1.4.3.3
Junio C HamanoRe: [PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_body
Jakub Narebski <jnareb@gmail.com> writes:
> ... First, it did that incompletely: it did not add "blob" > link for added files, and added block used mixture of tabs and spaces > for align. Second, in "difftree" view the "blob" link is not the most > interesting, *contrary* to "blob"/"tree" link in "tree" view, so it > should be enough to have hidden link in the form of file name entry.
I think these "blob" links are good thing to have, and if you think the earlier work was incomplete and know some cases are not covered I think it would be better to help completing it rather than reverting.
I do not understand why you feel "blob" is not the most interesting. Often, when it is not obvious if a patch is correct only with the context, it is useful to view the whole postimage after applying the patch, and the "blob" link helps that.
Jakub NarebskiRe: [PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_body
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes: > >> ... First, it did that incompletely: it did not add "blob" >> link for added files, and added block used mixture of tabs and spaces >> for align. Second, in "difftree" view the "blob" link is not the most >> interesting, *contrary* to "blob"/"tree" link in "tree" view, so it >> should be enough to have hidden link in the form of file name entry. > > I think these "blob" links are good thing to have, and if you > think the earlier work was incomplete and know some cases are > not covered I think it would be better to help completing it > rather than reverting. > > I do not understand why you feel "blob" is not the most > interesting. Often, when it is not obvious if a patch is > correct only with the context, it is useful to view the whole > postimage after applying the patch, and the "blob" link helps > that.
O.K. I'll choose the "add blob links where there are none in difftree view" approach in cleaned up and resend series.
I'll wait a while (a day or two) for further comments before redoing the series.
P.S. Remove empty patches might produce incorrect HTML (one of <div> is not closed). I'll correct it on resend.
-- Jakub Narebski
Jakub Narebski[PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view
Remove skipping over empty patches (i.e. patches which consist solely of extended headers) in git_patchset_body, and add links to those header-only patches in git_difftree_body (but not generate blobdiff links when there were no change in file contents).
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- As promised...
gitweb/gitweb.perl | 67 +++++++++++++++++++++------------------------------- 1 files changed, 27 insertions(+), 40 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 542dbca..e0f7a3f 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -1998,19 +1998,19 @@ sub git_difftree_body { print "</td>\n"; print "<td>$mode_chnge</td>\n"; print "<td class=\"link\">"; - if ($diff{'to_id'} ne $diff{'from_id'}) { # modified - if ($action eq 'commitdiff') { - # link to patch - $patchno++; - print $cgi->a({-href => "#patch$patchno"}, "patch"); - } else { - print $cgi->a({-href => href(action=>"blobdiff", - hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, hash_parent_base=>$parent, - file_name=>$diff{'file'})}, - "diff"); - } - print " | "; + if ($action eq 'commitdiff') { + # link to patch + $patchno++; + print $cgi->a({-href => "#patch$patchno"}, "patch") . + " | "; + } elsif ($diff{'to_id'} ne $diff{'from_id'}) { + # "commit" view and modified file (not onlu mode changed) + print $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'file'})}, + "diff") . + " | "; } print $cgi->a({-href => href(action=>"blame", hash_base=>$hash, file_name=>$diff{'file'})}, @@ -2038,19 +2038,19 @@ sub git_difftree_body { -class => "list"}, esc_path($diff{'from_file'})) . " with " . (int $diff{'similarity'}) . "% similarity$mode_chng]</span></td>\n" . "<td class=\"link\">"; - if ($diff{'to_id'} ne $diff{'from_id'}) { - if ($action eq 'commitdiff') { - # link to patch - $patchno++; - print $cgi->a({-href => "#patch$patchno"}, "patch"); - } else { - print $cgi->a({-href => href(action=>"blobdiff", - hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, hash_parent_base=>$parent, - file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})}, - "diff"); - } - print " | "; + if ($action eq 'commitdiff') { + # link to patch + $patchno++; + print $cgi->a({-href => "#patch$patchno"}, "patch") . + " | "; + } elsif ($diff{'to_id'} ne $diff{'from_id'}) { + # "commit" view and modified file (not only pure rename or copy) + print $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})}, + "diff") . + " | "; } print $cgi->a({-href => href(action=>"blame", hash_base=>$parent, file_name=>$diff{'from_file'})}, @@ -2072,7 +2072,6 @@ sub git_patchset_body { my $patch_idx = 0; my $in_header = 0; my $patch_found = 0; - my $skip_patch = 0; my $diffinfo; my (%from, %to); @@ -2084,8 +2083,6 @@ sub git_patchset_body { if ($patch_line =~ m/^diff /) { # "git diff" header # beginning of patch (in patchset) - $skip_patch = 0; - if ($patch_found) { # close previous patch print "</div>\n"; # class="patch" @@ -2116,13 +2113,6 @@ sub git_patchset_body { } $patch_idx++; - # for now we skip empty patches - if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) { # no change - $in_header = 1; - $skip_patch = 0; - next LINE; - } - # print "git diff" header $patch_line =~ s!^(diff (.*?) )a/.*$!$1!; if ($from{'href'}) { @@ -2143,10 +2133,7 @@ sub git_patchset_body { print "<div class=\"diff extended_header\">\n"; $in_header = 1; next LINE; - } else { - # actual skipping of empty patches - next LINE if $skip_patch; - } # start of patch in patchset + } if ($in_header) { if ($patch_line !~ m/^---/) {
-- 1.4.3.3
Jakub NarebskiRe: [PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view
Jakub Narebski wrote:
> Remove skipping over empty patches (i.e. patches which consist solely > of extended headers) in git_patchset_body, and add links to those > header-only patches in git_difftree_body (but not generate blobdiff > links when there were no change in file contents).
Attention: I think this patch causes that in some cases gitweb generates incorrect HTML (one of <div> elements is not closed).
-- Jakub Narebski
Jakub Narebski[PATCH 8/n] gitweb: Fix two issues with quoted filenames in git_patchset_body
This fixes the following (minor) bugs: * gitweb didn't remove quoted filenames from "git diff" header * the filename in to-file +++ header used esc_html not esc_path
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- gitweb/gitweb.perl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index e0f7a3f..837b88e 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -2114,7 +2114,7 @@ sub git_patchset_body { $patch_idx++; # print "git diff" header - $patch_line =~ s!^(diff (.*?) )a/.*$!$1!; + $patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!; if ($from{'href'}) { $patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"}, 'a/' . esc_path($from{'file'})); @@ -2191,7 +2191,7 @@ sub git_patchset_body { if ($to{'href'}) { $patch_line = '+++ b/' . $cgi->a({-href=>$to{'href'}, -class=>"path"}, - esc_html($to{'file'})); + esc_path($to{'file'})); } print "<div class=\"diff to_file\">$patch_line</div>\n";
-- 1.4.3.3
Jakub Narebski[PATCH 9/n] gitweb: Better support for non-CSS aware web browsers
Add option to replace SPC (' ') with hard (non-breakable) space HTML entity ' ' in esc_html subroutine.
Replace ' ' with ' ' for the code/diff display part in git_blob and git_patchset_body; this is to be able to view code and diffs in web browsers which doesn't understand "white-space: pre;" CSS declaration.
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- For people who want to use non-CSS aware browsers, like w3m, elinks, links text browsers, for looking at "blob" and "commitdiff"/"blobdiff" views of gitweb.
HTH.
gitweb/gitweb.perl | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 5c82093..b8fcafc 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -554,12 +554,17 @@ sub esc_url { } # replace invalid utf8 character with SUBSTITUTION sequence -sub esc_html { +sub esc_html ($;%) { my $str = shift; + my %opts = @_; + $str = to_utf8($str); $str = escapeHTML($str); $str =~ s/\014/^L/g; # escape FORM FEED (FF) character (e.g. in COPYING file) $str =~ s/\033/^[/g; # "escape" ESCAPE (\e) character (e.g. commit 20a3847d8a5032ce41f90dcc68abfb36e6fee9b1) + if ($opts{'-nbsp'}) { + $str =~ s/ / /g; + } return $str; } @@ -840,7 +845,7 @@ sub format_diff_line { $diff_class = " incomplete"; } $line = untabify($line); - return "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n"; + return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n"; } ## ---------------------------------------------------------------------- @@ -2985,7 +2990,7 @@ sub git_blob { $nr++; $line = untabify($line); printf "<div class=\"pre\"><a id=\"l%i\" href=\"#l%i\" class=\"linenr\">%4i</a> %s</div>\n", - $nr, $nr, $nr, esc_html($line); + $nr, $nr, $nr, esc_html($line, -nbsp=>1); } close $fd or print "Reading blob failed.\n";
-- 1.4.3.3
Petr BaudisRe: [PATCH 9/n] gitweb: Better support for non-CSS aware web browsers
Dear diary, on Wed, Nov 01, 2006 at 02:33:21PM CET, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...
> For people who want to use non-CSS aware browsers, like w3m, elinks, links > text browsers, for looking at "blob" and "commitdiff"/"blobdiff" views > of gitweb.
JFYI, ELinks supports CSS. (Well, not completely, but it does handle white-space: pre.)
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1Jakub Narebski[PATCH 10/n] gitweb: New improved formatting of chunk header in diff
If we have provided enough info, and diff is not combined diff, and if provided diff line is chunk header, then: * split chunk header into .chunk_info and .section span elements, first containing proper chunk header, second section heading (aka. which function), for separate styling: the proper chunk header is on non-white background, section heading part uses slightly lighter color. * hyperlink from-file-range to starting line of from-file, if file was not created. * hyperlink to-file-range to starting line of to-file, if file was not deleted. Links are of invisible variety (and "list" class).
Signed-off-by: Jakub Narebski <jnareb@gmail.com> --- As usual, you can check out the new version of gitweb at work at my web page: http://roke_._dyndns_._info/cgi-bin/gitweb/gitweb.cgi
The actual formatting is open for discussion.
gitweb/gitweb.css | 13 +++++++++++++ gitweb/gitweb.perl | 18 +++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletions(-)
gitweb/gitweb.css, gitweb/gitweb.perl
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css index 87dbc52..41c3084 100644 --- a/gitweb/gitweb.css +++ b/gitweb/gitweb.css @@ -327,11 +327,13 @@ div.diff.extended_header { padding: 2px 0px 2px 0px; } +div.diff a.list, div.diff a.path, div.diff a.hash { text-decoration: none; } +div.diff a.list:hover, div.diff a.path:hover, div.diff a.hash:hover { text-decoration: underline; @@ -355,14 +357,25 @@ div.diff.rem { color: #cc0000; } +div.diff.chunk_header a, div.diff.chunk_header { color: #990099; +} +div.diff.chunk_header { border: dotted #ffe0ff; border-width: 1px 0px 0px 0px; margin-top: 2px; } +div.diff.chunk_header span.chunk_info { + background-color: #ffeeff; +} + +div.diff.chunk_header span.section { + color: #aa22aa; +} + div.diff.incomplete { color: #cccccc; } diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index b8fcafc..a7739af 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -830,6 +830,7 @@ sub format_subject_html { sub format_diff_line { my $line = shift; + my ($from, $to) = @_; my $char = substr($line, 0, 1); my $diff_class = ""; @@ -845,6 +846,21 @@ sub format_diff_line { $diff_class = " incomplete"; } $line = untabify($line); + if ($from && $to && $line =~ m/^\@{2} /) { + my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) = + $line =~ m/^\@{2} (-(\d+),(\d+)) (\+(\d+),(\d+)) \@{2}(.*)$/; + if ($from->{'href'}) { + $from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start", + -class=>"list"}, $from_text); + } + if ($to->{'href'}) { + $to_text = $cgi->a({-href=>"$to->{'href'}#l$to_start", + -class=>"list"}, $to_text); + } + $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" . + "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>"; + return "<div class=\"diff$diff_class\">$line</div>\n"; + } return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n"; } @@ -2205,7 +2221,7 @@ sub git_patchset_body { next LINE; } - print format_diff_line($patch_line); + print format_diff_line($patch_line, \%from, \%to); } print "</div>\n" if $patch_found; # class="patch"
-- 1.4.3.3
Jakub NarebskiRe: [PATCH 00/10] gitweb: Better quoting and New improved patchset view
Jakub Narebski wrote:
> This series of patches is meant to introduce > "New improved patchset view" in part-by-part > case.
And thus endeth "New inproved patchset view" saga.
Patches in series:
[PATCH 01/10] gitweb: Better git-unquoting and gitweb-quoting of
pathnames
[PATCH 02/10] gitweb: Use '&iquot;' instead of '?' in esc_path
[PATCH 03/10] gitweb: Use 's' regexp modifier to secure against
filenames with LF
[PATCH 04/10] gitweb: Secure against commit-ish/tree-ish with
the same name as path
[PATCH 05/10] gitweb: New improved patchset view
[PATCH 06/10] gitweb: Remove redundant "blob" links from
git_difftree_body
[PATCH 07/10] gitweb: Output also empty patches in "commitdiff" view
[PATCH 08/10] gitweb: Fix two issues with quoted filenames
in git_patchset_body
[PATCH 09/10] gitweb: Better support for non-CSS aware web browsers
[PATCH 10/10] gitweb: New improved formatting of chunk header in diffDiffstat: gitweb/gitweb.css | 79 +++++++++-- gitweb/gitweb.perl | 378 ++++++++++++++++++++++++++++++++-------------------- 2 files changed, 303 insertions(+), 154 deletions(-)
Shortlog:
Jakub Narebski (11):
gitweb: Better git-unquoting and gitweb-quoting of pathnames
gitweb: Use '&iquot;' instead of '?' in esc_path
gitweb: Use 's' regexp modifier to secure against filenames with LF
gitweb: Secure against commit-ish/tree-ish with the same name as path
gitweb: New improved patchset view
gitweb: Remove redundant "blob" links from git_difftree_body
gitweb: Output also empty patches in "commitdiff" view
gitweb: Fix two issues with quoted filenames in git_patchset_body
[*1*] gitweb: Remove "--" from "git ls-tree -z <hash> --", added overeagerily
gitweb: Better support for non-CSS aware web browsers
gitweb: New improved formatting of chunk header in diff[*1*] Not send as patch itself, but as comment about patch
-- Jakub Narebski
Jakub NarebskiJunio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes: > >> Junio C Hamano wrote: >> >>> Jakub Narebski <jnareb@gmail.com> writes: >>> >>>> Here is example code for this: >>> >>> Ok. The issues I raised in the previous round seem to have been >>> addressed. Maybe you would want not to use nested 'sub' and it >>> is good to go, I think. >> >> Nested sub makes it easy to change gitweb quoting from Character Escape >> Codes (CEC) to e.g. Unicode Printable Representation (PR). > > Yes, sub makes it easy, it is not needed to be nested.
Well, this sub is of no use (I think) ourside esc_path subroutine. Similarly for unq sub nested in unescape in PATCH 1/10.
-- Jakub Narebski