threads / discuss / 26841

git svn perl issues

Subject: git svn perl issues

## tl;dr

4 messages between Mar 23, 2011 and Mar 23, 2011.

replies: 3people: 3as markdown or json

Stephen Hemminger· Mar 23, 2011, 15:52 UTC · lore

1. The following needs to be fixed:

$ git svn clone Use of uninitialized value $_[0] in substitution (s///) at /usr/share/perl/5.10.1/File/Basename.pm line 341. fileparse(): need a valid pathname at /usr/lib/git-core/git-svn line 403

2. The git-svn perl script does not follow Perl Best Practices.
If you run the perlcritic script on it, all the following warnings/errors
are generated:

$ perlcritic /usr/lib/git-core/git-svn Code before strictures are enabled at line 2, column 10. See page 429 of PBP. (Severity: 5) Variable declared in conditional statement at line 18, column 1. Declare variables outside of the condition. (Severity: 5) Subroutine prototypes used at line 39, column 1. See page 194 of PBP. (Severity: 5) Stricture disabled at line 65, column 2. See page 429 of PBP. (Severity: 5) Variable declared in conditional statement at line 287, column 1. Declare variables outside of the condition. (Severity: 5) Variable declared in conditional statement at line 533, column 2. Declare variables outside of the condition. (Severity: 5) Bareword file handle opened at line 851, column 3. See pages 202,204 of PBP. (Severity: 5) Bareword file handle opened at line 1169, column 4. See pages 202,204 of PBP. (Severity: 5) Variable declared in conditional statement at line 1571, column 3. Declare variables outside of the condition. (Severity: 5) Stricture disabled at line 1682, column 2. See page 429 of PBP. (Severity: 5) Don't modify $_ in list functions at line 1815, column 11. See page 114 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 1891, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 1969, column 2. See page 199 of PBP. (Severity: 5) Variable declared in conditional statement at line 1990, column 3. Declare variables outside of the condition. (Severity: 5) Nested named subroutine at line 2135, column 2. Declaring a named sub inside another named sub does not prevent the inner sub from being global. (Severity: 5) "return" statement with explicit "undef" at line 2140, column 3. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 2663, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 2671, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 2682, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 2753, column 2. See page 199 of PBP. (Severity: 5) Nested named subroutine at line 2789, column 2. Declaring a named sub inside another named sub does not prevent the inner sub from being global. (Severity: 5) "return" statement with explicit "undef" at line 3373, column 3. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 3707, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 3723, column 3. See page 199 of PBP. (Severity: 5) Bareword file handle opened at line 3977, column 3. See pages 202,204 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4116, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4119, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4204, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4212, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4220, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4228, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4244, column 2. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 4291, column 2. See page 199 of PBP. (Severity: 5) Nested named subroutine at line 4463, column 2. Declaring a named sub inside another named sub does not prevent the inner sub from being global. (Severity: 5) Subroutine prototypes used at line 4707, column 1. See page 194 of PBP. (Severity: 5) Stricture disabled at line 4817, column 2. See page 429 of PBP. (Severity: 5) Subroutine prototypes used at line 4831, column 1. See page 194 of PBP. (Severity: 5) Nested named subroutine at line 5160, column 3. Declaring a named sub inside another named sub does not prevent the inner sub from being global. (Severity: 5) Nested named subroutine at line 5271, column 2. Declaring a named sub inside another named sub does not prevent the inner sub from being global. (Severity: 5) Use IO::Interactive::is_interactive() instead of -t at line 5477, column 8. See page 218 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 5823, column 2. See page 199 of PBP. (Severity: 5) Glob written as <...> at line 5904, column 11. See page 167 of PBP. (Severity: 5)

Lasse Makholm· Mar 23, 2011, 21:38 UTC · re: Stephen Hemminger · lore

Re: git svn perl issues

On 23 March 2011 16:52, Stephen Hemminger <shemminger@vyatta.com> wrote:

Show 5 quoted lines

> 1. The following needs to be fixed:
>
> $ git svn clone
> Use of uninitialized value $_[0] in substitution (s///) at /usr/share/perl/5.10.1/File/Basename.pm line 341.
> fileparse(): need a valid pathname at /usr/lib/git-core/git-svn line 403

While noisy and ugly, uninitialized warnings are usually pretty harmless...

> 2. The git-svn perl script does not follow Perl Best Practices.
> If you run the perlcritic script on it, all the following warnings/errors
> are generated:

Some of these are undoubtedly valid complaints, but the so called best practices that the perl critic policies implement are, in my opinion, not widely accepted as such by the perl community. At least not all of them. I wouldn't go following them blindly - especially in working production code...

/Lasse

Stephen Hemminger· Mar 23, 2011, 21:45 UTC · re: Lasse Makholm · lore

Re: git svn perl issues

Show 12 quoted lines

> On 23 March 2011 16:52, Stephen Hemminger <shemminger@vyatta.com>
> wrote:
> > 1. The following needs to be fixed:
> >
> > $ git svn clone
> > Use of uninitialized value $_[0] in substitution (s///) at
> > /usr/share/perl/5.10.1/File/Basename.pm line 341.
> > fileparse(): need a valid pathname at /usr/lib/git-core/git-svn line
> > 403
> 
> While noisy and ugly, uninitialized warnings are usually pretty
> harmless...

User should never see perl splat, it is sloppy.

Show 10 quoted lines

> > 2. The git-svn perl script does not follow Perl Best Practices.
> > If you run the perlcritic script on it, all the following
> > warnings/errors
> > are generated:
> 
> Some of these are undoubtedly valid complaints, but the so called best
> practices that the perl critic policies implement are, in my opinion,
> not widely accepted as such by the perl community. At least not all of
> them. I wouldn't go following them blindly - especially in working
> production code...

Some of them are crap, but like sparse warnings it is trivial to fix them and make it clean so why not.

If you don't maintain code it just rots.

Lasse Makholm· Mar 23, 2011, 22:19 UTC · re: Stephen Hemminger · lore

Re: git svn perl issues

On 23 March 2011 22:45, Stephen Hemminger <stephen.hemminger@vyatta.com> wrote:

Show 15 quoted lines

>
>> On 23 March 2011 16:52, Stephen Hemminger <shemminger@vyatta.com>
>> wrote:
>> > 1. The following needs to be fixed:
>> >
>> > $ git svn clone
>> > Use of uninitialized value $_[0] in substitution (s///) at
>> > /usr/share/perl/5.10.1/File/Basename.pm line 341.
>> > fileparse(): need a valid pathname at /usr/lib/git-core/git-svn line
>> > 403
>>
>> While noisy and ugly, uninitialized warnings are usually pretty
>> harmless...
>
> User should never see perl splat, it is sloppy.

Agreed.

Show 13 quoted lines

>> > 2. The git-svn perl script does not follow Perl Best Practices.
>> > If you run the perlcritic script on it, all the following
>> > warnings/errors
>> > are generated:
>>
>> Some of these are undoubtedly valid complaints, but the so called best
>> practices that the perl critic policies implement are, in my opinion,
>> not widely accepted as such by the perl community. At least not all of
>> them. I wouldn't go following them blindly - especially in working
>> production code...
>
> Some of them are crap, but like sparse warnings it is trivial to
> fix them and make it clean so why not.

Well, personally I don't think they all add any value but that's a bit beside the point here...

> If you don't maintain code it just rots.

True enough. That said, I haven't actually looked into the git-svn code oh my god why do people write 6K line scripts... *sigh*

My first suggestion would be to split it... :-) It's already 75% classes anyway...

[forgot to reply all, sorry for the spam Stephen...]

-- 
/Lasse