On 2020-10-21 at 21:29:17, Jeff King wrote:
> 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.

> 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