Re: [PATCH 0/7] gitweb: Cleanups, fixes and small improvements

From: Jakub Narebski <jnareb@gmail.com>
Date: Aug 26, 2006, 21:18 UTC
Message-ID: <ecqdqt$rcl$1@sea.gmane.org>
In-Reply-To: <7vd5an1afz.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

Show 13 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> * gitweb: Restore old git_blame using git-annotate under "annotate"
> 
>   I actually was hoping to see a patch to remove the git_blame
>   which is not used as far as I can see.
> 
>   Although we carried annotate and blame in git.git tree for
>   quite a while, the intention was always to deprecate one over
>   the other once pros-and-cons of each implementation become
>   clear (and I think people would not miss annotate if we remove 
>   annotate and make it an alias for blame -c anymore).  What's
>   the reason we would want to have both?

Well, removing it completely might be not a best idea for now, as git_annotate has for example two more columns ("Age" and "Author") which might be usefull.

I guess the intention is to provide patch for those who want to improve git_blame, to have comparison with git_annotate, i.e. patch not applied but available.

Show 27 quoted lines

> * gitweb: Use 'local $/ = undef;' before 'print <$fd>;'
> 
>   You changed:
> 
>         $/ = undef;
>         print <$fd>;
>         ... hope that nobody depends on standard value of $/
>         ... around here, which may still break if you did sub
>         ... calls, the sub did not localize $/ (who would?),
>         ... and depended to have a sane $/.
>         $/ = "\n";
> 
>   to
> 
>         local $/ = undef;
>         print <$fd>;
>         ... hope that nobody depends on standard value of $/
>         ... until the end of scope, and whoever changes this
>         ... sub is careful enough in the future
> 
>   which I think is worse.  Introducing an extra scope explicitly
>   delimit the part you want to use localized $/ like this
> 
>         { local $/; print <$fd>; } 
> 
>   might have been more palatable.  Am I guessing the reason of
>   your change wrong?

Actually only one change was from '$/ = undef; print <$fd>; $/ = "\n"', namely at the end of git_blob_plain. I think nobody will print anything to the end of the sub.

Two other chunks actually _introduced_ 'local $/ = undef', both in the fairly short 'if' body. First to read $home_text (there change is not that important, as $home_text should be fairly short), next more important in git_snapshot.

IMVHO 'local $/ = undef' in all commands that get the rest of the output from filehandle (and outputting nothing later), like all 'plain' outputs should be the idiom to use.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git