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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html