From: Luben Tuikov <ltuikov@yahoo•com>
To: git@vger•kernel.org, Jakub Narebski <jnareb@gmail•com>
Subject: Re: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()
Date: Tue, 9 Dec 2008 22:24:49 -0800 (PST) [thread overview]
Message-ID: <910359.52983.qm@web31807.mail.mud.yahoo.com> (raw)
In-Reply-To: <20081209224814.28106.83387.stgit@localhost.localdomain>
--- On Tue, 12/9/08, Jakub Narebski <jnareb@gmail•com> wrote:
> From: Jakub Narebski <jnareb@gmail•com>
> Subject: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()
> To: git@vger•kernel.org
> Cc: "Luben Tuikov" <ltuikov@yahoo•com>, "Jakub Narebski" <jnareb@gmail•com>
> Date: Tuesday, December 9, 2008, 2:48 PM
> Among others:
> * move variable declaration closer to the place it is set
> and used,
> if possible,
> * uniquify and simplify coding style a bit, which includes
> removing
> unnecessary '()'.
> * check type only if $hash was defined, as otherwise from
> the way
> git_get_hash_by_path() is called (and works), we know
> that it is
> a blob,
> * use modern calling convention for git-blame,
> * remove unused variable,
> * don't use implicit variables ($_),
> * add some comments
>
> Signed-off-by: Jakub Narebski <jnareb@gmail•com>
Acked-by: Luben Tuikov <ltuikov@yahoo•com>
Looks good.
> ---
> Not stricly necessary... but the code looked not very nice
>
> gitweb/gitweb.perl | 65
> ++++++++++++++++++++++++++++++----------------------
> 1 files changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 916396a..68aa3f8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4575,28 +4575,32 @@ sub git_tag {
> }
>
> sub git_blame {
> - my $fd;
> - my $ftype;
> -
> + # permissions
> gitweb_check_feature('blame')
> - or die_error(403, "Blame view not
> allowed");
> + or die_error(403, "Blame view not allowed");
>
> + # error checking
> die_error(400, "No file name given") unless
> $file_name;
> $hash_base ||= git_get_head_hash($project);
> - die_error(404, "Couldn't find base commit")
> unless ($hash_base);
> + die_error(404, "Couldn't find base commit")
> unless $hash_base;
> my %co = parse_commit($hash_base)
> or die_error(404, "Commit not found");
> if (!defined $hash) {
> $hash = git_get_hash_by_path($hash_base, $file_name,
> "blob")
> or die_error(404, "Error looking up file");
> + } else {
> + my $ftype = git_get_type($hash);
> + if ($ftype !~ "blob") {
> + die_error(400, "Object is not a blob");
> + }
> }
> - $ftype = git_get_type($hash);
> - if ($ftype !~ "blob") {
> - die_error(400, "Object is not a blob");
> - }
> - open ($fd, "-|", git_cmd(), "blame",
> '-p', '--',
> - $file_name, $hash_base)
> +
> + # run git-blame --porcelain
> + open my $fd, "-|", git_cmd(),
> "blame", '-p',
> + $hash_base, '--', $file_name
> or die_error(500, "Open git-blame failed");
> +
> + # page header
> git_header_html();
> my $formats_nav =
> $cgi->a({-href =>
> href(action=>"blob", -replay=>1)},
> @@ -4610,40 +4614,43 @@ sub git_blame {
> git_print_page_nav('','',
> $hash_base,$co{'tree'},$hash_base, $formats_nav);
> git_print_header_div('commit',
> esc_html($co{'title'}), $hash_base);
> git_print_page_path($file_name, $ftype, $hash_base);
> - my @rev_color = (qw(light2 dark2));
> +
> + # page body
> + my @rev_color = qw(light2 dark2);
> my $num_colors = scalar(@rev_color);
> my $current_color = 0;
> - my $last_rev;
> + my %metainfo = ();
> +
> print <<HTML;
> <div class="page_body">
> <table class="blame">
>
> <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
> HTML
> - my %metainfo = ();
> - while (1) {
> - $_ = <$fd>;
> - last unless defined $_;
> + LINE:
> + while (my $line = <$fd>) {
> + chomp $line;
> + # the header: <SHA-1> <src lineno> <dst
> lineno> [<lines in group>]
> + # no <lines in group> for subsequent lines in
> group of lines
> my ($full_rev, $orig_lineno, $lineno, $group_size) =
> - /^([0-9a-f]{40}) (\d+) (\d+)(?:
> (\d+))?$/;
> + ($line =~ /^([0-9a-f]{40}) (\d+) (\d+)(?:
> (\d+))?$/);
> if (!exists $metainfo{$full_rev}) {
> $metainfo{$full_rev} = {};
> }
> my $meta = $metainfo{$full_rev};
> - while (<$fd>) {
> - last if (s/^\t//);
> - if (/^(\S+) (.*)$/) {
> + while (my $data = <$fd>) {
> + chomp $data;
> + last if ($data =~ s/^\t//); # contents of line
> + if ($data =~ /^(\S+) (.*)$/) {
> $meta->{$1} = $2;
> }
> }
> - my $data = $_;
> - chomp $data;
> - my $rev = substr($full_rev, 0, 8);
> + my $short_rev = substr($full_rev, 0, 8);
> my $author = $meta->{'author'};
> - my %date = parse_date($meta->{'author-time'},
> - $meta->{'author-tz'});
> + my %date =
> + parse_date($meta->{'author-time'},
> $meta->{'author-tz'});
> my $date = $date{'iso-tz'};
> if ($group_size) {
> - $current_color = ++$current_color % $num_colors;
> + $current_color = ($current_color + 1) % $num_colors;
> }
> print "<tr id=\"l$lineno\"
> class=\"$rev_color[$current_color]\">\n";
> if ($group_size) {
> @@ -4654,7 +4661,7 @@ HTML
> print $cgi->a({-href =>
> href(action=>"commit",
> hash=>$full_rev,
>
> file_name=>$file_name)},
> - esc_html($rev));
> + esc_html($short_rev));
> print "</td>\n";
> }
> my $parent_commit;
> @@ -4683,6 +4690,8 @@ HTML
> print "</div>";
> close $fd
> or print "Reading blob failed\n";
> +
> + # page footer
> git_footer_html();
> }
next prev parent reply other threads:[~2008-12-10 6:26 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-12-09 22:43 [PATCH 0/3] gitweb: Improve git_blame in preparation for incremental blame Jakub Narebski
2008-12-09 22:46 ` [PATCH 1/3] gitweb: Move 'lineno' id from link to row element in git_blame Jakub Narebski
2008-12-10 5:55 ` Luben Tuikov
2008-12-17 8:13 ` Petr Baudis
2008-12-09 22:48 ` [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame() Jakub Narebski
2008-12-10 3:49 ` Nanako Shiraishi
2008-12-10 13:39 ` Jakub Narebski
2008-12-10 20:27 ` Junio C Hamano
2008-12-11 0:33 ` [PATCH 2/3 (edit v2)] " Jakub Narebski
2008-12-11 4:08 ` Luben Tuikov
2008-12-11 4:18 ` Junio C Hamano
2008-12-12 3:05 ` Junio C Hamano
2008-12-12 17:20 ` Jakub Narebski
2008-12-17 8:19 ` Petr Baudis
2008-12-17 8:34 ` Junio C Hamano
2008-12-10 6:20 ` [PATCH 2/3] " Luben Tuikov
2008-12-10 15:15 ` Jakub Narebski
2008-12-10 20:05 ` Luben Tuikov
2008-12-10 21:03 ` Jakub Narebski
2008-12-10 21:15 ` Luben Tuikov
2008-12-09 22:48 ` [PATCH 3/3] gitweb: A bit of code cleanup " Jakub Narebski
2008-12-10 2:13 ` Jakub Narebski
2008-12-10 8:35 ` Junio C Hamano
2008-12-10 6:24 ` Luben Tuikov [this message]
2008-12-10 20:11 ` [RFC/PATCH 4/3] gitweb: Incremental blame (proof of concept) Jakub Narebski
2008-12-11 0:47 ` Junio C Hamano
2008-12-11 1:22 ` Jakub Narebski
2008-12-11 17:28 ` Jakub Narebski
2008-12-11 22:34 ` Jakub Narebski
2008-12-14 0:17 ` [RFC/PATCH v2] " Jakub Narebski
2008-12-14 16:11 ` [RFC] gitweb: Incremental blame - suggestions for improvements Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=910359.52983.qm@web31807.mail.mud.yahoo.com \
--to=ltuikov@yahoo$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox