Re: [PATCH] svn: use correct variable name for short OID

From: Jeff King <peff@peff.net>
Date: Oct 22, 2020, 03:00 UTC
Message-ID: <20201022030051.GB1480820@coredump.intra.peff.net>
In-Reply-To: <20201022011811.853465-1-sandals@crustytoothpaste.net>

On Thu, Oct 22, 2020 at 01:18:11AM +0000, brian m. carlson wrote:

Show 16 quoted lines

> The commit 9ab33150a0 ("perl: create and switch variables for hash
> constants", 2020-06-22) converted each instance of the variable
> $sha1_short into $oid_short in the Subversion code, since git-svn now
> understands SHA-256.  However, one conversion was missed.
> 
> As a result, Perl complains about the use of this variable:
> 
>   Use of uninitialized value $sha1_short in regexp compilation at
>   /usr/lib64/perl5/vendor_perl/5.30.3/Git/SVN/Log.pm line 301, <$fh>
>   line 6.
> 
> Because we're parsing raw diff output here, the likelihood is very low
> that we'll actually misparse the data, since the only lines we're going
> to get starting with colons are the ones we're expecting.  Even if we
> had a newline in a path, we'd end up with a quoted path.  Our regex is
> just less strict than we'd like it to be.

I agree this is unlikely to matter much in the happy path, but I wondered how confused things could get. I'd never looked at this code before, but it looks like we take git-log @args from the user. So:

  git svn log --format=":123456 123456 foo"

gets mis-parsed. But not only is that exceedingly unlikely in the first place, AFAICT the command was never meant to allow arbitrary formats anyway. It's expecting its own "--pretty=raw" to be respected, so the command above is broken even with your fix.

None of that changes the fix, which is obviously correct, but I wondered if we ought to have better test coverage here. And I've convinced myself the answer is "no"; there's no reasonable-to-test functional impact of this bug or its fix (aside from generating the warning, but it would be silly to write a test for this one warning; if we do anything it should be to complain about any warnings during the test run).

-Peff