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

3 messages from 2006-09-09 to 2006-09-09. Participants: Sven Verdoolaege, Jeff King, Jakub Narebski.
Thread: https://gitlist.dev/t/5529

## Sven Verdoolaege, 2006-09-09 15:12

Subject: [PATCH] gitweb: reset input record separator in parse_commit even on error
Message-ID: <20060909151236.GA25518@liacs.nl>
URL: https://gitlist.dev/e/20060909151236.GA25518%40liacs.nl

```
---
This probably never happens, but just in case...

 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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;
-- 
0.99.8c.g64e8-dirty

```

## Jeff King, 2006-09-09 20:51

Subject: Re: [PATCH] gitweb: reset input record separator in parse_commit even on error
Message-ID: <20060909205159.GC16906@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20060909205159.GC16906%40coredump.intra.peff.net
In-Reply-To: <20060909151236.GA25518@liacs.nl>

```
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

```

## Jakub Narebski, 2006-09-09 22:00

Subject: Re: [PATCH] gitweb: reset input record separator in parse_commit even on error
Message-ID: <edvdgb$mtt$1@sea.gmane.org>
URL: https://gitlist.dev/e/edvdgb%24mtt%241%40sea.gmane.org
In-Reply-To: <20060909205159.GC16906@coredump.intra.peff.net>

```
Jeff King wrote:

> 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

And this should be done consistently, for all plain formats 
(blob_plain, blobdiff_plain, commitdiff_plain), and some other
 places.

Sometimes it is
  local $/ = "\0";
more often
  local $/ = undef;
(or equivalently but slightly less human friendly, just 'local $/');

> 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>;
>     ...

That is because by default git-rev-list separates output for different
commits (when --header option is used) bu NULL... only because of 
"--max-count=1" there is only _one_ record... nevertheless it ends with 
"\0" (NULL) character.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

```