Re: git svn log: Use of uninitialized value $sha1_short

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 21, 2020, 22:29 UTC
Message-ID: <20201021222901.GK490427@camp.crustytoothpaste.net>
In-Reply-To: <20201021212917.GA62005@coredump.intra.peff.net>

On 2020-10-21 at 21:29:17, Jeff King wrote:

Show 31 quoted lines

> Yeah, I'm almost certain this is the solution, but it was a little
> disturbing that no tests catch it. Besides the warning, it probably is a
> functional problem (I guess that regex is now overly broad since its
> last half is blank). But maybe it doesn't matter much. It looks like
> we're parsing raw diff output from git-log. Short of a really bizarre
> --format parameter, those are the only lines that would match /^:/
> anyway.
> 
> The tests do catch it if we do:
> 
> diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
> index 3858fcf27d..92e223caed 100644
> --- a/perl/Git/SVN/Log.pm
> +++ b/perl/Git/SVN/Log.pm
> @@ -1,6 +1,6 @@
>  package Git::SVN::Log;
>  use strict;
> -use warnings;
> +use warnings FATAL => qw(all);
>  use Git::SVN::Utils qw(fatal);
>  use Git qw(command
>             command_oneline
> 
> but:
> 
>   - we'd need to do that in each .pm file, as well as git-svn.perl
> 
>   - I wonder if it's suitable for production use (i.e., would it become
>     annoying when a newer version of perl issues a harmless warning;
>     right now that's a minor inconvenience, but aborting the whole
>     program might be a show-stopper).

No, that's not suitable for production use. Perl does add new warnings from time to time and breaking things when Perl gets upgraded will definitely not make us the friends of Linux distros. Doing this is like using -Werror: fine for your personal development needs, but not suitable for shipping to others.

We could run "perl -w" on each file and look for a single-line output with "OK"; that's what we did at a previous job. However, any change we make here needs to be conditional on DEVELOPER, because otherwise anyone who needs to build an Git with a new version of Perl will potentially have a broken testsuite.

Show 9 quoted lines

> It would be nice if we could crank up the severity just while running
> the tests, but I don't think there's an easy built-in way to do that.
> This seems to work:
> 
>   use warnings ($ENV{GIT_PERL_STRICT} ? qw(FATAL all) : ());
> 
> though I'm honestly surprised it does (because "use" is generally
> resolved at read/compile time. I guess perl is smart enough to run
> that code snippet at that point.

Yup, that would run at BEGIN time.

-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US