gitweb: Further refactoring
cover gitweb: Further refactoring
This series of patches, on top of my merged series '[PATCH 7/5] Merge changes in "split patch 1" series' although probably would apply on top of 'next'.
* [PATCH 1/5] gitweb: Refactor untabifying - converting tabs to spaces
* [PATCH 2/6] gitweb: Simplify git_diff_print
* [PATCH 3/6] gitweb: Remove unused parse_date invocation from git_shortlog_body
* [PATCH 4/6] gitweb: Make blob diff -p1 like commit diff
* [PATCH 5/6] gitweb: Refactor printing shortened title in git_shortlog_body and git_tags_body
* [PATCH 6/6] gitweb: Refactor git_history_body
Signature · 2 lines
Jakub Narebski Poland
PATCH 1/6 gitweb: Refactor untabifying - converting tabs to spaces
Add untabify subroutine and use it.
--- gitweb/gitweb.perl | 38 +++++++++++++++++++------------------- 1 files changed, 19 insertions(+), 19 deletions(-)
gitweb/gitweb.perl +19 -19
@@ -222,6 +222,21 @@ sub unquote { return $str; } +# escape tabs (convert tabs to spaces) +sub untabify { + my $line = shift; + + while ((my $pos = index($line, "\t")) != -1) { + # one out of three implementations used ($pos - 1) instead of $pos + if (my $count = (8 - ($pos % 8))) { + my $spaces = ' ' x $count; + $line =~ s/\t/$spaces/; + } + } + + return $line; +} + ## ---------------------------------------------------------------------- ## HTML aware string manipulation
@@ -1255,12 +1270,7 @@ sub git_diff_print { # skip errors next; } - while ((my $pos = index($line, "\t")) != -1) { - if (my $count = (8 - (($pos-1) % 8))) { - my $spaces = ' ' x $count; - $line =~ s/\t/$spaces/; - } - } + $line = untabify($line); print "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n"; } }
@@ -1600,13 +1610,8 @@ HTML $age_class = age_class($age); $author = esc_html ($author); $author =~ s/ / /g; - # escape tabs - while ((my $pos = index($data, "\t")) != -1) { - if (my $count = (8 - ($pos % 8))) { - my $spaces = ' ' x $count; - $data =~ s/\t/$spaces/; - } - } + + $data = untabify($data); $data = esc_html ($data); print <<HTML;
@@ -1728,12 +1733,7 @@ sub git_blob { while (my $line = <$fd>) { chomp $line; $nr++; - while ((my $pos = index($line, "\t")) != -1) { - if (my $count = (8 - ($pos % 8))) { - my $spaces = ' ' x $count; - $line =~ s/\t/$spaces/; - } - } + $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); } close $fd or print "Reading blob failed.\n";
Signature · 1 line
1.4.1.1
Re: [PATCH 1/6] gitweb: Refactor untabifying - converting tabs to spaces
gitweb/gitweb.perl sub unquote { · line 230
Quoted 1 line · Jakub Narebski
gitweb/gitweb.perl +1
-0+ # one out of three implementations used ($pos - 1) instead of $pos
Perhaps this should be in the commit message instead of the source? I think no one will later understand this comment without the commit.
PATCH 2/6 gitweb: Simplify git_diff_print
In reply to Jakub NarebskiCopy to temporaty file more directly, not using temporary variable @file. Use list form of open for diff invocation (we cannot use git-diff because first it doesn't support -L/--label option, and we cannot generate diff between /dev/null and blob given by it's sha1 identifier).
Use "local $/ = undef;" for (temporary) slurp mode.
--- gitweb/gitweb.perl | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
gitweb/gitweb.perl +7 -7
@@ -1230,31 +1230,31 @@ sub git_diff_print { # create tmp from-file if (defined $from) { + local $/ = undef; $from_tmp = "$git_temp/gitweb_" . $$ . "_from"; open my $fd2, "> $from_tmp"; open my $fd, "-|", $GIT, "cat-file", "blob", $from; - my @file = <$fd>; - print $fd2 @file; + print { $fd2 } <$fd>; close $fd2; close $fd; } # create tmp to-file if (defined $to) { + local $/ = undef; $to_tmp = "$git_temp/gitweb_" . $$ . "_to"; open my $fd2, "> $to_tmp"; open my $fd, "-|", $GIT, "cat-file", "blob", $to; - my @file = <$fd>; - print $fd2 @file; + print { $fd2 } <$fd>; close $fd2; close $fd; } - open my $fd, "-|", "/usr/bin/diff -u -p -L \'$from_name\' -L \'$to_name\' $from_tmp $to_tmp"; + open my $fd, "-|", "/usr/bin/diff", + '-u', '-p', '-L', $from_name, '-L', $to_name, $from_tmp, $to_tmp; if ($format eq "plain") { - undef $/; + local $/ = undef; print <$fd>; - $/ = "\n"; } else { while (my $line = <$fd>) { chomp $line;
Signature · 1 line
1.4.1.1
Re: [PATCH 2/6] gitweb: Simplify git_diff_print
Quoted 8 lines · Jakub Narebski
Copy to temporaty file more directly, not using temporary variable @file. Use list form of open for diff invocation (we cannot use git-diff because first it doesn't support -L/--label option, and we cannot generate diff between
/dev/nulland blob given by it's sha1 identifier).Use "local $/ = undef;" for (temporary) slurp mode.
Signed-off-by Jakub Narebski <[email protected]>
I think this part of the gitweb code predates the git-diff support by the core level. If I recall correctly, back then we did not even have "git-diff-{tree,files,index}" when this part was written.
I think you could use "git-diff $from $to" instead of using temporary files and running /usr/bin/diff on it with today's git. Which is a big win from both security and ease of administration point of view. Not having to write anything into temporary file from gitweb.cgi means you do not have to worry about stray gitweb process leaving temporary files dangling behind, for example.
You obviously would need to emulate the -L$from_name -L$to_name part by hand in the loop that reads from the diff output.
One worrysome pathological case is what happens if files being tracked happen to have names that consist of 40-byte hexdigits. I think "git-diff $from $to --" should do the right thing for gitweb use ($from and $to should not mistaken taken as pathnames with the explicit -- disambiguator) and "git-diff -- $from $to" also should do the right thing to take $from and $to as pathnames even if there are revisions with the same object names, but somebody may want to verify that (and send patches to fix if it is not the case).
Re: [PATCH 2/6] gitweb: Simplify git_diff_print
Quoted 16 lines · Junio C Hamano
Quoted 8 lines · Jakub Narebski
Copy to temporaty file more directly, not using temporary variable @file. Use list form of open for diff invocation (we cannot use git-diff because first it doesn't support -L/--label option, and we cannot generate diff between
/dev/nulland blob given by it's sha1 identifier).Use "local $/ = undef;" for (temporary) slurp mode.
Signed-off-by Jakub Narebski <[email protected]>I think this part of the gitweb code predates the git-diff support by the core level. If I recall correctly, back then we did not even have "git-diff-{tree,files,index}" when this part was written.
Quoted 7 lines
I think you could use "git-diff $from $to" instead of using temporary files and running
/usr/bin/diffon it with today's git. Which is a big win from both security and ease of administration point of view. Not having to write anything into temporary file from gitweb.cgi means you do not have to worry about stray gitweb process leaving temporary files dangling behind, for example.
If I remember gitweb code correctly, diff to /dev/null is called _only_ in commitdiff, where we can use git-diff -p to generate patch directly, instead of generating difftree and calling diff on entries.
Still it would be nice if git-diff could diff between blob and arbitrary file on filesystem (including /dev/null), and acquired -L/--label option.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
PATCH 3/6 gitweb: Remove unused parse_date invocation from git_shortlog_body
In reply to Jakub Narebski---
Remainder from refactoring
gitweb/gitweb.perl | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
gitweb/gitweb.perl +0 -1
@@ -1077,7 +1077,6 @@ sub git_shortlog_body { #my $ref = defined $refs ? format_mark_referencing($refs, $commit) : ''; my $ref = format_mark_referencing($refs, $commit); my %co = parse_commit($commit); - my %ad = parse_date($co{'author_epoch'}); if ($alternate) { print "<tr class=\"dark\">\n"; } else {
Signature · 1 line
1.4.1.1
PATCH 4/6 gitweb: Make blob diff -p1 like commit diff
In reply to Jakub NarebskiAdd 'a/' and 'b/' as prefix for blobdiff, blobdiff_plain, like it is in commitdiff, commitdiff_plain. Ensure that label for /dev/null is actually /dev/null.
---
For consistency.
gitweb/gitweb.perl | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
gitweb/gitweb.perl +8 -2
@@ -1236,6 +1236,8 @@ sub git_diff_print { print { $fd2 } <$fd>; close $fd2; close $fd; + } else { + $from_name = "/dev/null"; } # create tmp to-file
@@ -1247,6 +1249,8 @@ sub git_diff_print { print { $fd2 } <$fd>; close $fd2; close $fd; + } else { + $to_name = "/dev/null"; } open my $fd, "-|", "/usr/bin/diff",
@@ -2105,7 +2109,8 @@ sub git_blobdiff { " -> blob:" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$hash;hb=$hash_base;f=$file_name")}, $hash) . "</div>\n"; - git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash); + git_diff_print($hash_parent, 'a/' . ($file_name || $hash_parent), + $hash, 'b/' . ($file_name || $hash)); print "</div>"; git_footer_html(); }
@@ -2113,7 +2118,8 @@ sub git_blobdiff { sub git_blobdiff_plain { mkdir($git_temp, 0700); print $cgi->header(-type => "text/plain", -charset => 'utf-8'); - git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash, "plain"); + git_diff_print($hash_parent, 'a/' . ($file_name || $hash_parent), + $hash, 'b/' . ($file_name || $hash), "plain"); } sub git_commitdiff {
Signature · 1 line
1.4.1.1
PATCH 5/6 gitweb: Refactor printing shortened title in git_shortlog_body and git_tags_body
In reply to Jakub NarebskiUses ugly workaround with $bold argument because style is not set via CSS, but via presentation element <b>...</b>.
---
Probably needs better name, too
gitweb/gitweb.perl | 39 +++++++++++++++++++++++---------------- 1 files changed, 23 insertions(+), 16 deletions(-)
gitweb/gitweb.perl +23 -16
@@ -1062,6 +1062,27 @@ sub git_print_page_path { } ## ...................................................................... +## functions printing or outputting HTML: inline elements + +# print, perhaps shortened and with markers, title line +sub print_title_html { + # $bold argument is workaround until style will be set via CSS + my ($long, $short, $query, $bold, $extra) = @_; + $extra = '' unless defined($extra); + + if (length($short) < length($long)) { + print $cgi->a({-href => "$my_uri?" . esc_param($query), + -class => "list", -title => $long}, + ($bold ? "<b>" : "") . esc_html($short) . $extra . ($bold ? "</b>" : "")); + } else { + print $cgi->a({-href => "$my_uri?" . esc_param($query), + -class => "list"}, + ($bold ? "<b>" : "") . esc_html($long) . $extra . ($bold ? "</b>" : "")); + } +} + + +## ...................................................................... ## functions printing large fragments of HTML sub git_shortlog_body {
@@ -1087,15 +1108,7 @@ sub git_shortlog_body { print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . "<td><i>" . esc_html(chop_str($co{'author_name'}, 10)) . "</i></td>\n" . "<td>"; - if (length($co{'title_short'}) < length($co{'title'})) { - print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"), - -class => "list", -title => "$co{'title'}"}, - "<b>" . esc_html($co{'title_short'}) . "$ref</b>"); - } else { - print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"), - -class => "list"}, - "<b>" . esc_html($co{'title'}) . "$ref</b>"); - } + print_title_html($co{'title'}, $co{'title_short'}, "p=$project;a=commit;h=$commit", 1, $ref); print "</td>\n" . "<td class=\"link\">" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") . " | " .
@@ -1141,13 +1154,7 @@ sub git_tags_body { "</td>\n" . "<td>"; if (defined $comment) { - if (length($comment_short) < length($comment)) { - print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tag;h=$tag{'id'}"), - -class => "list", -title => $comment}, $comment_short); - } else { - print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tag;h=$tag{'id'}"), - -class => "list"}, $comment); - } + print_title_html($comment, $comment_short, "p=$project;a=tag;h=$tag{'id'}"); } print "</td>\n" . "<td class=\"selflink\">";
Signature · 1 line
1.4.1.1
PATCH 6/6 gitweb: Refactor git_history_body
In reply to Jakub NarebskiSeparates main part of git_history into git_history_body subroutine, and make output more similar to git_shortlog.
---
We couldn't just modify git_shortlog, because git_shortlog reads full (--max-count limited) output of rev-list to array, which is needed for dividing into pages and pagination navigation bar, while git_history reads rev-list output line by line.
gitweb/gitweb.perl | 93 ++++++++++++++++++++++++++++++++-------------------- 1 files changed, 57 insertions(+), 36 deletions(-)
gitweb/gitweb.perl +57 -36
@@ -1124,6 +1124,62 @@ sub git_shortlog_body { print "</table>\n"; } +sub git_history_body { + # Warning: assumes constant type (blob or tree) during history + my ($fd, $refs, $hash_base, $ftype, $extra) = @_; + + print "<table class=\"history\" cellspacing=\"0\">\n"; + my $alternate = 0; + while (my $line = <$fd>) { + if ($line !~ m/^([0-9a-fA-F]{40})/) { + next; + } + + my $commit = $1; + my %co = parse_commit($commit); + if (!%co) { + next; + } + + my $ref = format_mark_referencing($refs, $commit); + + if ($alternate) { + print "<tr class=\"dark\">\n"; + } else { + print "<tr class=\"light\">\n"; + } + $alternate ^= 1; + print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . + # shortlog uses chop_str($co{'author_name'}, 10) + "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 3)) . "</i></td>\n" . + "<td>"; + # originally git_history used chop_str($co{'title'}, 50) + print_title_html($co{'title'}, $co{'title_short'}, "p=$project;a=commit;h=$commit", 1, $ref); + print "</td>\n" . + "<td class=\"link\">" . + $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") . " | " . + $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$commit")}, "commitdiff") . " | " . + $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=$ftype;hb=$commit;f=$file_name")}, $ftype); + + my $blob_current = git_get_hash_by_path($hash_base, $file_name); + my $blob_parent = git_get_hash_by_path($commit, $file_name); + if (defined $blob_current && defined $blob_parent && + $blob_current ne $blob_parent) { + print " | " . + $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blobdiff;h=$blob_current;hp=$blob_parent;hb=$commit;f=$file_name")}, + "diff to current"); + } + print "</td>\n" . + "</tr>\n"; + } + if (defined $extra) { + print "<tr>\n" . + "<td colspan=\"4\">$extra</td>\n" . + "</tr>\n"; + } + print "</table>\n"; +} + sub git_tags_body { # uses global variable $project my ($taglist, $from, $to, $extra) = @_;
@@ -2295,42 +2351,7 @@ sub git_history { open my $fd, "-|", $GIT, "rev-list", "--full-history", $hash_base, "--", $file_name; - print "<table cellspacing=\"0\">\n"; - my $alternate = 0; - while (my $line = <$fd>) { - if ($line =~ m/^([0-9a-fA-F]{40})/){ - my $commit = $1; - my %co = parse_commit($commit); - if (!%co) { - next; - } - my $ref = format_mark_referencing($refs, $commit); - if ($alternate) { - print "<tr class=\"dark\">\n"; - } else { - print "<tr class=\"light\">\n"; - } - $alternate ^= 1; - print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . - "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 3)) . "</i></td>\n" . - "<td>" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"), -class => "list"}, "<b>" . - esc_html(chop_str($co{'title'}, 50)) . "$ref</b>") . "</td>\n" . - "<td class=\"link\">" . - $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") . - " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$commit")}, "commitdiff") . - " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=$ftype;hb=$commit;f=$file_name")}, $ftype); - my $blob = git_get_hash_by_path($hash_base, $file_name); - my $blob_parent = git_get_hash_by_path($commit, $file_name); - if (defined $blob && defined $blob_parent && $blob ne $blob_parent) { - print " | " . - $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blobdiff;h=$blob;hp=$blob_parent;hb=$commit;f=$file_name")}, - "diff to current"); - } - print "</td>\n" . - "</tr>\n"; - } - } - print "</table>\n"; + git_history_body($fd, $refs, $hash_base, $ftype); close $fd; git_footer_html(); }
Signature · 1 line
1.4.1.1
The earlier 9-series looked fine, so I intend to apply them all to "next". However, they were all tab damaged and it was very painful to fix up the patch.
This series, PATCH 1/5 (6?) looks fine (it is a bugfix for git_diff_print), but among others, one of them had a seriously fishy preimage (e.g. as you can see in "next", I haven't accepted the function rename patch but 3/6 talks about format_mark_referencing), and at that point my patience ran out.
Please check what I pushed out on "next"/"pu" tonight, and consider patches missing from there dropped on the floor for now (which does not mean they are rejected, except for the one that touches diff_print and still uses /usr/bin/diff, which I commented on). It probably is a good idea to make sure further patches resent are based on (or at least apply cleanly to) the commit f16db173a4680aebc2f14c103a1e125c3f4d4531.
Please note that I am primarily concentrating on polishing what is in "master" to finish 1.4.2 for now, so I might have missed breakage in these gitweb patches that I queued for "next".
Comments, tests and fixups are all very welcomed.
Quoted 3 lines · Junio C Hamano
The earlier 9-series looked fine, so I intend to apply them all to "next". However, they were all tab damaged and it was very painful to fix up the patch.
I'm sorry for that. As far as I checked, KNode (news reader which I used for sending 9-(micro)series) preserves tabs when attaching file inline. I guess that GMane news-to-mail gateway might munge whitespace. I won't use KNode to send patches, then.
By the way, can anyone tell me how to setup git-send-email to send email via GMail (authenticated SMTP, via STARTTLS/SSL, on ports 25 or 465 or 587)?
Quoted 5 lines
This series, PATCH 1/5 (6?) looks fine (it is a bugfix for
git_diff_print), but among others, one of them had a seriously fishy preimage (e.g. as you can see in "next", I haven't accepted the function rename patch but 3/6 talks aboutformat_mark_referencing), and at that point my patience ran out.
I _have_ notified that this 6-series is on top of previous series, namely '[PATCH 7/5] Merge changes in "split patch 1" series' patch.
But I try to redo not accepted changes on top of next, and put "gitweb: Great subroutines renaming" on top of series. (BTW. this is simple search'n'replace on subroutine names... well, perhaps after 1.4.2...).
[...]
Quoted 3 lines
Please note that I am primarily concentrating on polishing what is in "master" to finish 1.4.2 for now, so I might have missed breakage in these gitweb patches that I queued for "next".
I'm trying to check my gitweb patches, by checking the output of all possible actions, althought I think it doesn't encompass all the cases...
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git