Re: [PATCH] gitweb: reset input record separator in parse_commit even on error

From: Jeff King <peff@peff.net>
Date: 2006-09-10 06:51:59
On Sat, Sep 09, 2006 at 05:12:36PM +0200, Sven Verdoolaege wrote:

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7afdf33..60dd598 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -897,8 +897,8 @@ sub parse_commit {
>  		my $fd = git_pipe("rev-list", "--header", "--parents", "--max-count=1", $commit_id)
>  			or return;
>  		@commit_lines = split '\n', <$fd>;
> -		close $fd or return;
>  		$/ = "\n";
> +		close $fd or return;
>  		pop @commit_lines;
>  	}
>  	my $header = shift @commit_lines;

You missed the other early return from git_pipe. However, I think the
approach is wrong; this is a great opportunity to use the dynamic
scoping offered by 'local':

  else {
    local $/ = "\0";
    # do stuff with $/ as "\0"
  }
  # $/ has automatically been reset at the end of the block

Looking further at this bit of code, it seems even more confusing,
though. We split the output of rev-list on NUL, grab presumably the
entire thing (since there shouldn't be any NULs in the output, right?)
and then split it on newline into a list. Why aren't we doing this:
  else {
    open my $fd, ...;
    @commit_lines = map { chomp; $_ } <$fd>;
    ...

-Peff
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Received on Sun Sep 10 06:52:14 2006

This archive was generated by hypermail 2.1.8 : 2006-09-10 06:53:03 EST