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

From: Jeff King <peff@peff.net>
Date: Oct 21, 2020, 21:29 UTC
Message-ID: <20201021212917.GA62005@coredump.intra.peff.net>
In-Reply-To: <xmqqwnzj5mq5.fsf@gitster.c.googlers.com>

On Wed, Oct 21, 2020 at 01:48:50PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> > Looks like it got renamed, and this reference was somehow missed?
> >
> >   $ git log -1 -Ssha1_short perl
> >   commit 9ab33150a0d14089d0496dd8354d4a969e849571
> >   Author: brian m. carlson <sandals@crustytoothpaste.net>
> >   Date:   Mon Jun 22 18:04:12 2020 +0000
> >   
> >       perl: create and switch variables for hash constants
> >       
> >       git-svn has several variables for SHA-1 constants, including short hash
> >       values and full length hash values.  Since these are no longer SHA-1
> >       specific, let's start them with "oid" instead of "sha1".  Add a
> >       constant, oid_length, which is the length of the hash algorithm in use
> >       in hex.  We use the hex version because overwhelmingly that's what's
> >       used by git-svn.
> >   [...]
> 
> Looks that way.  '$::' as opposed to plain '$' threw the replacement
> off the track?

That was my guess, too, but that commit does convert some other references of that form. <shrug>

Show 16 quoted lines

>  perl/Git/SVN/Log.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git i/perl/Git/SVN/Log.pm w/perl/Git/SVN/Log.pm
> index 3858fcf27d..9c819188ea 100644
> --- i/perl/Git/SVN/Log.pm
> +++ w/perl/Git/SVN/Log.pm
> @@ -298,7 +298,7 @@ sub cmd_show_log {
>  			get_author_info($c, $1, $2, $3);
>  		} elsif (/^${esc_color}(?:tree|parent|committer) /o) {
>  			# ignore
> -		} elsif (/^${esc_color}:\d{6} \d{6} $::sha1_short/o) {
> +		} elsif (/^${esc_color}:\d{6} \d{6} $::oid_short/o) {
>  			push @{$c->{raw}}, $_;
>  		} elsif (/^${esc_color}[ACRMDT]\t/) {
>  			# we could add $SVN->{svn_path} here, but that requires

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).

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.

-Peff