{"thread":{"id":"54481","subject":"git svn log: Use of uninitialized value $sha1_short","startedAt":"2020-10-21T18:50:06Z","lastAt":"2020-10-22T19:30:06Z","messageCount":11,"participants":["Nikos Chantziaras","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408084","messageId":"rmpve5$q2s$1@ciao.gmane.io","threadId":"54481","inReplyTo":null,"subject":"git svn log: Use of uninitialized value $sha1_short","fromName":"Nikos Chantziaras","fromEmail":"realnc@gmail.com","sentAt":"2020-10-21T18:42:12Z","receivedAt":"2020-10-21T18:50:06Z","isPatch":false,"sender":{"key":"realnc@gmail.com","avatar":null},"body":"Running 'git svn log' in a repository that was cloned from an SVN repo \nresults in this warning:\n\n$ git svn log > /dev/null\nUse of uninitialized value $sha1_short in regexp compilation at \n/usr/lib64/perl5/vendor_perl/5.30.3/Git/SVN/Log.pm line 301, <$fh> line 6.\n\nThis doesn't appear to have any ill effects, but the warning might \nindicate a problem somewhere.\n\n\n[System Info]\ngit version:\ngit version 2.29.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.4.72-gentoo #1 SMP PREEMPT Sat Oct 17 18:27:41 EEST 2020 \nx86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n"},{"id":"408091","messageId":"20201021202642.GA60606@coredump.intra.peff.net","threadId":"54481","inReplyTo":"rmpve5$q2s$1@ciao.gmane.io","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-21T20:26:42Z","receivedAt":"2020-10-21T20:26:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 21, 2020 at 09:42:12PM +0300, Nikos Chantziaras wrote:\n\n> Running 'git svn log' in a repository that was cloned from an SVN repo\n> results in this warning:\n> \n> $ git svn log > /dev/null\n> Use of uninitialized value $sha1_short in regexp compilation at\n> /usr/lib64/perl5/vendor_perl/5.30.3/Git/SVN/Log.pm line 301, <$fh> line 6.\n> \n> This doesn't appear to have any ill effects, but the warning might indicate\n> a problem somewhere.\n\nIt seems to only get mentioned once and never set:\n\n  $ git grep sha1_short perl\n  perl/Git/SVN/Log.pm:            } elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n\nLooks like it got renamed, and this reference was somehow missed?\n\n  $ git log -1 -Ssha1_short perl\n  commit 9ab33150a0d14089d0496dd8354d4a969e849571\n  Author: brian m. carlson <sandals@crustytoothpaste.net>\n  Date:   Mon Jun 22 18:04:12 2020 +0000\n  \n      perl: create and switch variables for hash constants\n      \n      git-svn has several variables for SHA-1 constants, including short hash\n      values and full length hash values.  Since these are no longer SHA-1\n      specific, let's start them with \"oid\" instead of \"sha1\".  Add a\n      constant, oid_length, which is the length of the hash algorithm in use\n      in hex.  We use the hex version because overwhelmingly that's what's\n      used by git-svn.\n  [...]\n\n-Peff\n"},{"id":"408098","messageId":"xmqqwnzj5mq5.fsf@gitster.c.googlers.com","threadId":"54481","inReplyTo":"20201021202642.GA60606@coredump.intra.peff.net","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-21T20:48:50Z","receivedAt":"2020-10-21T20:48:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It seems to only get mentioned once and never set:\n>\n>   $ git grep sha1_short perl\n>   perl/Git/SVN/Log.pm:            } elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n>\n> Looks like it got renamed, and this reference was somehow missed?\n>\n>   $ git log -1 -Ssha1_short perl\n>   commit 9ab33150a0d14089d0496dd8354d4a969e849571\n>   Author: brian m. carlson <sandals@crustytoothpaste.net>\n>   Date:   Mon Jun 22 18:04:12 2020 +0000\n>   \n>       perl: create and switch variables for hash constants\n>       \n>       git-svn has several variables for SHA-1 constants, including short hash\n>       values and full length hash values.  Since these are no longer SHA-1\n>       specific, let's start them with \"oid\" instead of \"sha1\".  Add a\n>       constant, oid_length, which is the length of the hash algorithm in use\n>       in hex.  We use the hex version because overwhelmingly that's what's\n>       used by git-svn.\n>   [...]\n\nLooks that way.  '$::' as opposed to plain '$' threw the replacement\noff the track?\n\n perl/Git/SVN/Log.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git i/perl/Git/SVN/Log.pm w/perl/Git/SVN/Log.pm\nindex 3858fcf27d..9c819188ea 100644\n--- i/perl/Git/SVN/Log.pm\n+++ w/perl/Git/SVN/Log.pm\n@@ -298,7 +298,7 @@ sub cmd_show_log {\n \t\t\tget_author_info($c, $1, $2, $3);\n \t\t} elsif (/^${esc_color}(?:tree|parent|committer) /o) {\n \t\t\t# ignore\n-\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n+\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::oid_short/o) {\n \t\t\tpush @{$c->{raw}}, $_;\n \t\t} elsif (/^${esc_color}[ACRMDT]\\t/) {\n \t\t\t# we could add $SVN->{svn_path} here, but that requires\n"},{"id":"408104","messageId":"20201021212917.GA62005@coredump.intra.peff.net","threadId":"54481","inReplyTo":"xmqqwnzj5mq5.fsf@gitster.c.googlers.com","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-21T21:29:17Z","receivedAt":"2020-10-21T21:29:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 21, 2020 at 01:48:50PM -0700, Junio C Hamano wrote:\n\n> > Looks like it got renamed, and this reference was somehow missed?\n> >\n> >   $ git log -1 -Ssha1_short perl\n> >   commit 9ab33150a0d14089d0496dd8354d4a969e849571\n> >   Author: brian m. carlson <sandals@crustytoothpaste.net>\n> >   Date:   Mon Jun 22 18:04:12 2020 +0000\n> >   \n> >       perl: create and switch variables for hash constants\n> >       \n> >       git-svn has several variables for SHA-1 constants, including short hash\n> >       values and full length hash values.  Since these are no longer SHA-1\n> >       specific, let's start them with \"oid\" instead of \"sha1\".  Add a\n> >       constant, oid_length, which is the length of the hash algorithm in use\n> >       in hex.  We use the hex version because overwhelmingly that's what's\n> >       used by git-svn.\n> >   [...]\n> \n> Looks that way.  '$::' as opposed to plain '$' threw the replacement\n> off the track?\n\nThat was my guess, too, but that commit does convert some other\nreferences of that form. <shrug>\n\n>  perl/Git/SVN/Log.pm | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git i/perl/Git/SVN/Log.pm w/perl/Git/SVN/Log.pm\n> index 3858fcf27d..9c819188ea 100644\n> --- i/perl/Git/SVN/Log.pm\n> +++ w/perl/Git/SVN/Log.pm\n> @@ -298,7 +298,7 @@ sub cmd_show_log {\n>  \t\t\tget_author_info($c, $1, $2, $3);\n>  \t\t} elsif (/^${esc_color}(?:tree|parent|committer) /o) {\n>  \t\t\t# ignore\n> -\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n> +\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::oid_short/o) {\n>  \t\t\tpush @{$c->{raw}}, $_;\n>  \t\t} elsif (/^${esc_color}[ACRMDT]\\t/) {\n>  \t\t\t# we could add $SVN->{svn_path} here, but that requires\n\nYeah, I'm almost certain this is the solution, but it was a little\ndisturbing that no tests catch it. Besides the warning, it probably is a\nfunctional problem (I guess that regex is now overly broad since its\nlast half is blank). But maybe it doesn't matter much. It looks like\nwe're parsing raw diff output from git-log. Short of a really bizarre\n--format parameter, those are the only lines that would match /^:/\nanyway.\n\nThe tests do catch it if we do:\n\ndiff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm\nindex 3858fcf27d..92e223caed 100644\n--- a/perl/Git/SVN/Log.pm\n+++ b/perl/Git/SVN/Log.pm\n@@ -1,6 +1,6 @@\n package Git::SVN::Log;\n use strict;\n-use warnings;\n+use warnings FATAL => qw(all);\n use Git::SVN::Utils qw(fatal);\n use Git qw(command\n            command_oneline\n\nbut:\n\n  - we'd need to do that in each .pm file, as well as git-svn.perl\n\n  - I wonder if it's suitable for production use (i.e., would it become\n    annoying when a newer version of perl issues a harmless warning;\n    right now that's a minor inconvenience, but aborting the whole\n    program might be a show-stopper).\n\nIt would be nice if we could crank up the severity just while running\nthe tests, but I don't think there's an easy built-in way to do that.\nThis seems to work:\n\n  use warnings ($ENV{GIT_PERL_STRICT} ? qw(FATAL all) : ());\n\nthough I'm honestly surprised it does (because \"use\" is generally\nresolved at read/compile time. I guess perl is smart enough to run\nthat code snippet at that point.\n\n-Peff\n"},{"id":"408113","messageId":"20201021222158.GJ490427@camp.crustytoothpaste.net","threadId":"54481","inReplyTo":"xmqqwnzj5mq5.fsf@gitster.c.googlers.com","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-21T22:21:58Z","receivedAt":"2020-10-21T22:22:07Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-21 at 20:48:50, Junio C Hamano wrote:\n> Looks that way.  '$::' as opposed to plain '$' threw the replacement\n> off the track?\n> \n>  perl/Git/SVN/Log.pm | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git i/perl/Git/SVN/Log.pm w/perl/Git/SVN/Log.pm\n> index 3858fcf27d..9c819188ea 100644\n> --- i/perl/Git/SVN/Log.pm\n> +++ w/perl/Git/SVN/Log.pm\n> @@ -298,7 +298,7 @@ sub cmd_show_log {\n>  \t\t\tget_author_info($c, $1, $2, $3);\n>  \t\t} elsif (/^${esc_color}(?:tree|parent|committer) /o) {\n>  \t\t\t# ignore\n> -\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n> +\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::oid_short/o) {\n>  \t\t\tpush @{$c->{raw}}, $_;\n>  \t\t} elsif (/^${esc_color}[ACRMDT]\\t/) {\n>  \t\t\t# we could add $SVN->{svn_path} here, but that requires\n\nYeah, this is correct.  I'll try to get a format patch written up\ntonight with this if nobody gets to it before me.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"408118","messageId":"20201021222901.GK490427@camp.crustytoothpaste.net","threadId":"54481","inReplyTo":"20201021212917.GA62005@coredump.intra.peff.net","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-21T22:29:01Z","receivedAt":"2020-10-21T22:29:41Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-21 at 21:29:17, Jeff King wrote:\n> Yeah, I'm almost certain this is the solution, but it was a little\n> disturbing that no tests catch it. Besides the warning, it probably is a\n> functional problem (I guess that regex is now overly broad since its\n> last half is blank). But maybe it doesn't matter much. It looks like\n> we're parsing raw diff output from git-log. Short of a really bizarre\n> --format parameter, those are the only lines that would match /^:/\n> anyway.\n> \n> The tests do catch it if we do:\n> \n> diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm\n> index 3858fcf27d..92e223caed 100644\n> --- a/perl/Git/SVN/Log.pm\n> +++ b/perl/Git/SVN/Log.pm\n> @@ -1,6 +1,6 @@\n>  package Git::SVN::Log;\n>  use strict;\n> -use warnings;\n> +use warnings FATAL => qw(all);\n>  use Git::SVN::Utils qw(fatal);\n>  use Git qw(command\n>             command_oneline\n> \n> but:\n> \n>   - we'd need to do that in each .pm file, as well as git-svn.perl\n> \n>   - I wonder if it's suitable for production use (i.e., would it become\n>     annoying when a newer version of perl issues a harmless warning;\n>     right now that's a minor inconvenience, but aborting the whole\n>     program might be a show-stopper).\n\nNo, that's not suitable for production use.  Perl does add new warnings\nfrom time to time and breaking things when Perl gets upgraded will\ndefinitely not make us the friends of Linux distros.  Doing this is like\nusing -Werror: fine for your personal development needs, but not\nsuitable for shipping to others.\n\nWe could run \"perl -w\" on each file and look for a single-line output\nwith \"OK\"; that's what we did at a previous job.  However, any change we\nmake here needs to be conditional on DEVELOPER, because otherwise anyone\nwho needs to build an Git with a new version of Perl will potentially\nhave a broken testsuite.\n\n> It would be nice if we could crank up the severity just while running\n> the tests, but I don't think there's an easy built-in way to do that.\n> This seems to work:\n> \n>   use warnings ($ENV{GIT_PERL_STRICT} ? qw(FATAL all) : ());\n> \n> though I'm honestly surprised it does (because \"use\" is generally\n> resolved at read/compile time. I guess perl is smart enough to run\n> that code snippet at that point.\n\nYup, that would run at BEGIN time.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"408134","messageId":"20201022011811.853465-1-sandals@crustytoothpaste.net","threadId":"54481","inReplyTo":"rmpve5$q2s$1@ciao.gmane.io","subject":"[PATCH] svn: use correct variable name for short OID","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-22T01:18:11Z","receivedAt":"2020-10-22T01:18:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The commit 9ab33150a0 (\"perl: create and switch variables for hash\nconstants\", 2020-06-22) converted each instance of the variable\n$sha1_short into $oid_short in the Subversion code, since git-svn now\nunderstands SHA-256.  However, one conversion was missed.\n\nAs a result, Perl complains about the use of this variable:\n\n  Use of uninitialized value $sha1_short in regexp compilation at\n  /usr/lib64/perl5/vendor_perl/5.30.3/Git/SVN/Log.pm line 301, <$fh>\n  line 6.\n\nBecause we're parsing raw diff output here, the likelihood is very low\nthat we'll actually misparse the data, since the only lines we're going\nto get starting with colons are the ones we're expecting.  Even if we\nhad a newline in a path, we'd end up with a quoted path.  Our regex is\njust less strict than we'd like it to be.\n\nHowever, it's obviously undesirable that our code is emitting Perl\nwarnings, so let's convert it to use the proper variable name.\n\nReported-by: Nikos Chantziaras <realnc@gmail.com>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n perl/Git/SVN/Log.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm\nindex 3858fcf27d..9c819188ea 100644\n--- a/perl/Git/SVN/Log.pm\n+++ b/perl/Git/SVN/Log.pm\n@@ -298,7 +298,7 @@ sub cmd_show_log {\n \t\t\tget_author_info($c, $1, $2, $3);\n \t\t} elsif (/^${esc_color}(?:tree|parent|committer) /o) {\n \t\t\t# ignore\n-\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::sha1_short/o) {\n+\t\t} elsif (/^${esc_color}:\\d{6} \\d{6} $::oid_short/o) {\n \t\t\tpush @{$c->{raw}}, $_;\n \t\t} elsif (/^${esc_color}[ACRMDT]\\t/) {\n \t\t\t# we could add $SVN->{svn_path} here, but that requires\n"},{"id":"408135","messageId":"20201022025644.GA1480820@coredump.intra.peff.net","threadId":"54481","inReplyTo":"20201021222901.GK490427@camp.crustytoothpaste.net","subject":"Re: git svn log: Use of uninitialized value $sha1_short","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-22T02:56:44Z","receivedAt":"2020-10-22T02:56:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 21, 2020 at 10:29:01PM +0000, brian m. carlson wrote:\n\n> >   - I wonder if it's suitable for production use (i.e., would it become\n> >     annoying when a newer version of perl issues a harmless warning;\n> >     right now that's a minor inconvenience, but aborting the whole\n> >     program might be a show-stopper).\n> \n> No, that's not suitable for production use.  Perl does add new warnings\n> from time to time and breaking things when Perl gets upgraded will\n> definitely not make us the friends of Linux distros.  Doing this is like\n> using -Werror: fine for your personal development needs, but not\n> suitable for shipping to others.\n\nOK, that matches my general sense.\n\n> We could run \"perl -w\" on each file and look for a single-line output\n> with \"OK\"; that's what we did at a previous job.  However, any change we\n> make here needs to be conditional on DEVELOPER, because otherwise anyone\n> who needs to build an Git with a new version of Perl will potentially\n> have a broken testsuite.\n\nYeah, I've done something similar as well. I agree it would be\npotentially annoying for distros building. But unlike the -Wall/-Werror\ndistinction, where the builder may be annoyed by extra messages but the\nend result is presumably OK, this _does_ affect end users. I.e., if it\ntriggers, it also means the users of your package are going to see\nannoying warnings. The change I'm proposing is just whether that's fatal\nor not.\n\nSo it might actually be of interest to distro builders to know that the\nversion of Git they're building is going to produce annoying warnings.\nAnd the escape hatch there is likely not to turn warnings from fatal to\nnon-fatal, but to suppress the particular warning entirely with a patch\nto the code.\n\nI dunno. I admit I don't really care enough about our perl code in\ngeneral (which I consider mostly legacy; I doubt we'd introduce new perl\nscripts lightly). So it may not even be worth putting a lot of effort\ninto it. But it is unfortunate that this bug _could_ have been caught\nautomatically but wasn't.\n\n-Peff\n"},{"id":"408136","messageId":"20201022030051.GB1480820@coredump.intra.peff.net","threadId":"54481","inReplyTo":"20201022011811.853465-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] svn: use correct variable name for short OID","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-22T03:00:51Z","receivedAt":"2020-10-22T03:00:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 22, 2020 at 01:18:11AM +0000, brian m. carlson wrote:\n\n> The commit 9ab33150a0 (\"perl: create and switch variables for hash\n> constants\", 2020-06-22) converted each instance of the variable\n> $sha1_short into $oid_short in the Subversion code, since git-svn now\n> understands SHA-256.  However, one conversion was missed.\n> \n> As a result, Perl complains about the use of this variable:\n> \n>   Use of uninitialized value $sha1_short in regexp compilation at\n>   /usr/lib64/perl5/vendor_perl/5.30.3/Git/SVN/Log.pm line 301, <$fh>\n>   line 6.\n> \n> Because we're parsing raw diff output here, the likelihood is very low\n> that we'll actually misparse the data, since the only lines we're going\n> to get starting with colons are the ones we're expecting.  Even if we\n> had a newline in a path, we'd end up with a quoted path.  Our regex is\n> just less strict than we'd like it to be.\n\nI agree this is unlikely to matter much in the happy path, but I\nwondered how confused things could get. I'd never looked at this code\nbefore, but it looks like we take git-log @args from the user. So:\n\n  git svn log --format=\":123456 123456 foo\"\n\ngets mis-parsed. But not only is that exceedingly unlikely in the first\nplace, AFAICT the command was never meant to allow arbitrary formats\nanyway. It's expecting its own \"--pretty=raw\" to be respected, so the\ncommand above is broken even with your fix.\n\nNone of that changes the fix, which is obviously correct, but I wondered\nif we ought to have better test coverage here. And I've convinced myself\nthe answer is \"no\"; there's no reasonable-to-test functional impact of\nthis bug or its fix (aside from generating the warning, but it would be\nsilly to write a test for this one warning; if we do anything it should\nbe to complain about any warnings during the test run).\n\n-Peff\n"},{"id":"408141","messageId":"20201022032400.GC1480820@coredump.intra.peff.net","threadId":"54481","inReplyTo":"20201022030051.GB1480820@coredump.intra.peff.net","subject":"Re: [PATCH] svn: use correct variable name for short OID","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-22T03:24:00Z","receivedAt":"2020-10-22T03:24:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 21, 2020 at 11:00:51PM -0400, Jeff King wrote:\n\n> None of that changes the fix, which is obviously correct, but I wondered\n> if we ought to have better test coverage here. And I've convinced myself\n> the answer is \"no\"; there's no reasonable-to-test functional impact of\n> this bug or its fix (aside from generating the warning, but it would be\n> silly to write a test for this one warning; if we do anything it should\n> be to complain about any warnings during the test run).\n\nWe talked about it enough that I was curious how bad the patch would\nlook. So here it is. It does catch the problem in DEVELOPER=1 mode, but\nnot otherwise.\n\nThe fact that we have to touch every perl file is a bit ugly. So I\ndunno. Maybe worth it, or maybe too nasty.\n\n-- >8 --\nSubject: [PATCH] perl: check for perl warnings while running tests\n\nWe set \"use warnings\" in most of our perl code to catch problems. But as\nthe name implies, warnings just emit a message to stderr and don't\notherwise affect the program. So our tests are quite likely to miss that\nwarnings are being spewed, as most of them do not look at stderr.\n\nWe could ask perl to make all warnings fatal, but this is likely\nannoying for non-developers, who would rather have a running program\nwith a warning than something that refuses to work at all.\n\nSo instead, let's teach the perl code to respect an environment variable\n(GIT_PERL_FATAL_WARNINGS) to increase the severity of the warnings. This\ncan be set for day-to-day running if people want to be really pedantic,\nbut the primary use is to trigger it within the test suite.\n\nWe could also trigger that for every test run, but likewise even the\ntests failing may be annoying to distro builders, etc (just as -Werror\nwould be for compiling C code). So we'll tie it to a special test-mode\nvariable (GIT_TEST_PERL_FATAL_WARNINGS) that can be set in the\nenvironment or as a Makefile knob, and we'll automatically turn the knob\nwhen DEVELOPER=1 is set. That should give developers and CI the more\ncareful view without disrupting normal users or packagers.\n\nNote that the mapping from the GIT_TEST_* form to the GIT_* form in\ntest-lib.sh is necessary even if they had the same name: the perl\nscripts need it to be normalized to a perl truth value, and we also have\nto make sure it's exported (we might have gotten it from the\nenvironment, but we might also have gotten it from GIT-BUILD-OPTIONS\ndirectly).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile                          | 3 +++\n config.mak.dev                    | 2 ++\n git-svn.perl                      | 2 +-\n perl/FromCPAN/Error.pm            | 2 +-\n perl/Git.pm                       | 2 +-\n perl/Git/I18N.pm                  | 2 +-\n perl/Git/IndexInfo.pm             | 2 +-\n perl/Git/LoadCPAN.pm              | 2 +-\n perl/Git/LoadCPAN/Error.pm        | 2 +-\n perl/Git/LoadCPAN/Mail/Address.pm | 2 +-\n perl/Git/Packet.pm                | 2 +-\n perl/Git/SVN.pm                   | 2 +-\n perl/Git/SVN/Editor.pm            | 2 +-\n perl/Git/SVN/Fetcher.pm           | 2 +-\n perl/Git/SVN/GlobSpec.pm          | 2 +-\n perl/Git/SVN/Log.pm               | 2 +-\n perl/Git/SVN/Memoize/YAML.pm      | 2 +-\n perl/Git/SVN/Migration.pm         | 2 +-\n perl/Git/SVN/Prompt.pm            | 2 +-\n perl/Git/SVN/Ra.pm                | 2 +-\n perl/Git/SVN/Utils.pm             | 2 +-\n t/test-lib.sh                     | 6 ++++++\n 22 files changed, 30 insertions(+), 19 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 95571ee3fc..4d6f6dc16f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2767,6 +2767,9 @@ ifdef GIT_INTEROP_MAKE_OPTS\n endif\n ifdef GIT_TEST_INDEX_VERSION\n \t@echo GIT_TEST_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_INDEX_VERSION)))'\\' >>$@+\n+endif\n+ifdef GIT_TEST_PERL_FATAL_WARNINGS\n+\t@echo GIT_TEST_PERL_FATAL_WARNINGS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_PERL_FATAL_WARNINGS)))'\\' >>$@+\n endif\n \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n \ndiff --git a/config.mak.dev b/config.mak.dev\nindex 89b218d11a..89db543534 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -45,3 +45,5 @@ ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wno-uninitialized\n endif\n endif\n+\n+GIT_TEST_PERL_FATAL_WARNINGS = YesPlease\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 58f5a7ac8f..70cb5e2a83 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -2,7 +2,7 @@\n # Copyright (C) 2006, Eric Wong <normalperson@yhbt.net>\n # License: GPL v2 or later\n use 5.008;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use strict;\n use vars qw/\t$AUTHOR $VERSION\n \t\t$oid $oid_short $oid_length\ndiff --git a/perl/FromCPAN/Error.pm b/perl/FromCPAN/Error.pm\nindex 8b95e2d73d..d82b71325c 100644\n--- a/perl/FromCPAN/Error.pm\n+++ b/perl/FromCPAN/Error.pm\n@@ -12,7 +12,7 @@\n package Error;\n \n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n \n use vars qw($VERSION);\n use 5.004;\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 10df990959..02eacef0c2 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -9,7 +9,7 @@ package Git;\n \n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n \n use File::Temp ();\n use File::Spec ();\ndiff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm\nindex bfb4fb67a1..2037f387c8 100644\n--- a/perl/Git/I18N.pm\n+++ b/perl/Git/I18N.pm\n@@ -1,7 +1,7 @@\n package Git::I18N;\n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n BEGIN {\n \trequire Exporter;\n \tif ($] < 5.008003) {\ndiff --git a/perl/Git/IndexInfo.pm b/perl/Git/IndexInfo.pm\nindex 2a7b4908f3..9ee054f854 100644\n--- a/perl/Git/IndexInfo.pm\n+++ b/perl/Git/IndexInfo.pm\n@@ -1,6 +1,6 @@\n package Git::IndexInfo;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Git qw/command_input_pipe command_close_pipe/;\n \n sub new {\ndiff --git a/perl/Git/LoadCPAN.pm b/perl/Git/LoadCPAN.pm\nindex e5585e75e8..0c360bc799 100644\n--- a/perl/Git/LoadCPAN.pm\n+++ b/perl/Git/LoadCPAN.pm\n@@ -1,7 +1,7 @@\n package Git::LoadCPAN;\n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n \n =head1 NAME\n \ndiff --git a/perl/Git/LoadCPAN/Error.pm b/perl/Git/LoadCPAN/Error.pm\nindex c6d2c45d80..5d84c20288 100644\n--- a/perl/Git/LoadCPAN/Error.pm\n+++ b/perl/Git/LoadCPAN/Error.pm\n@@ -1,7 +1,7 @@\n package Git::LoadCPAN::Error;\n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Git::LoadCPAN (\n \tmodule => 'Error',\n \timport => 1,\ndiff --git a/perl/Git/LoadCPAN/Mail/Address.pm b/perl/Git/LoadCPAN/Mail/Address.pm\nindex f70a4f064c..340e88a7a5 100644\n--- a/perl/Git/LoadCPAN/Mail/Address.pm\n+++ b/perl/Git/LoadCPAN/Mail/Address.pm\n@@ -1,7 +1,7 @@\n package Git::LoadCPAN::Mail::Address;\n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Git::LoadCPAN (\n \tmodule => 'Mail::Address',\n \timport => 0,\ndiff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm\nindex b75738bed4..d144f5168f 100644\n--- a/perl/Git/Packet.pm\n+++ b/perl/Git/Packet.pm\n@@ -1,7 +1,7 @@\n package Git::Packet;\n use 5.008;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n BEGIN {\n \trequire Exporter;\n \tif ($] < 5.008003) {\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex d1c352f92b..f6f1dc03c6 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -1,6 +1,6 @@\n package Git::SVN;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Fcntl qw/:DEFAULT :seek/;\n use constant rev_map_fmt => 'NH*';\n use vars qw/$_no_metadata\ndiff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm\nindex c961444d4c..47fd048bf2 100644\n--- a/perl/Git/SVN/Editor.pm\n+++ b/perl/Git/SVN/Editor.pm\n@@ -1,7 +1,7 @@\n package Git::SVN::Editor;\n use vars qw/@ISA $_rmdir $_cp_similarity $_find_copies_harder $_rename_limit/;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use SVN::Core;\n use SVN::Delta;\n use Carp qw/croak/;\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 729e5337df..968309e6d6 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -3,7 +3,7 @@ package Git::SVN::Fetcher;\n             $_placeholder_filename @deleted_gpath %added_placeholder\n             $repo_id/;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use SVN::Delta;\n use Carp qw/croak/;\n use File::Basename qw/dirname/;\ndiff --git a/perl/Git/SVN/GlobSpec.pm b/perl/Git/SVN/GlobSpec.pm\nindex a0a8d17621..f2c1e1f6fb 100644\n--- a/perl/Git/SVN/GlobSpec.pm\n+++ b/perl/Git/SVN/GlobSpec.pm\n@@ -1,6 +1,6 @@\n package Git::SVN::GlobSpec;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n \n sub new {\n \tmy ($class, $glob, $pattern_ok) = @_;\ndiff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm\nindex 3858fcf27d..6cd4cdfceb 100644\n--- a/perl/Git/SVN/Log.pm\n+++ b/perl/Git/SVN/Log.pm\n@@ -1,6 +1,6 @@\n package Git::SVN::Log;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Git::SVN::Utils qw(fatal);\n use Git qw(command\n            command_oneline\ndiff --git a/perl/Git/SVN/Memoize/YAML.pm b/perl/Git/SVN/Memoize/YAML.pm\nindex 9676b8f2f7..8fcf6644a1 100644\n--- a/perl/Git/SVN/Memoize/YAML.pm\n+++ b/perl/Git/SVN/Memoize/YAML.pm\n@@ -1,5 +1,5 @@\n package Git::SVN::Memoize::YAML;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use strict;\n use YAML::Any ();\n \ndiff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm\nindex dc90f6a621..ed96ac593e 100644\n--- a/perl/Git/SVN/Migration.pm\n+++ b/perl/Git/SVN/Migration.pm\n@@ -33,7 +33,7 @@ package Git::SVN::Migration;\n #              possible if noMetadata or useSvmProps are set; but should\n #              be no problem for users that use the (sensible) defaults.\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use File::Basename qw/dirname basename/;\ndiff --git a/perl/Git/SVN/Prompt.pm b/perl/Git/SVN/Prompt.pm\nindex e940b08505..de158e848f 100644\n--- a/perl/Git/SVN/Prompt.pm\n+++ b/perl/Git/SVN/Prompt.pm\n@@ -1,6 +1,6 @@\n package Git::SVN::Prompt;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n require SVN::Core;\n use vars qw/$_no_auth_cache $_username/;\n \ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 2cfe055a9a..912e035040 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -1,7 +1,7 @@\n package Git::SVN::Ra;\n use vars qw/@ISA $config_dir $_ignore_refs_regex $_log_window_size/;\n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n use Memoize;\n use Git::SVN::Utils qw(\n \tcanonicalize_url\ndiff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm\nindex 3d1a0933a2..5ca09ab448 100644\n--- a/perl/Git/SVN/Utils.pm\n+++ b/perl/Git/SVN/Utils.pm\n@@ -1,7 +1,7 @@\n package Git::SVN::Utils;\n \n use strict;\n-use warnings;\n+use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();\n \n use SVN::Core;\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ef31f40037..dfad820dd4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -499,6 +499,12 @@ then\n \texport GIT_INDEX_VERSION\n fi\n \n+if test -n \"$GIT_TEST_PERL_FATAL_WARNINGS\"\n+then\n+\tGIT_PERL_FATAL_WARNINGS=1\n+\texport GIT_PERL_FATAL_WARNINGS\n+fi\n+\n # Add libc MALLOC and MALLOC_PERTURB test\n # only if we are not executing the test with valgrind\n if test -n \"$valgrind\" ||\n-- \n2.29.0.579.g559558a5ea\n\n"},{"id":"408187","messageId":"xmqq3626m53d.fsf@gitster.c.googlers.com","threadId":"54481","inReplyTo":"20201022032400.GC1480820@coredump.intra.peff.net","subject":"Re: [PATCH] svn: use correct variable name for short OID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-22T19:29:58Z","receivedAt":"2020-10-22T19:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The fact that we have to touch every perl file is a bit ugly. So I\n> dunno. Maybe worth it, or maybe too nasty.\n\nJust a single pragma per file?  That does not sound too bad at least\nto me.\n\nThanks, queued.\n\n> Note that the mapping from the GIT_TEST_* form to the GIT_* form in\n> test-lib.sh is necessary even if they had the same name: the perl\n> scripts need it to be normalized to a perl truth value, and we also have\n> to make sure it's exported (we might have gotten it from the\n> environment, but we might also have gotten it from GIT-BUILD-OPTIONS\n> directly).\n\nAnd GIT_PERL_FATAL_WARNINGS is cleared together with the other GIT_*\noptions upfront, so here we only need to worry about setting and\nexporting it.  Makes sense.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index ef31f40037..dfad820dd4 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -499,6 +499,12 @@ then\n>  \texport GIT_INDEX_VERSION\n>  fi\n>  \n> +if test -n \"$GIT_TEST_PERL_FATAL_WARNINGS\"\n> +then\n> +\tGIT_PERL_FATAL_WARNINGS=1\n> +\texport GIT_PERL_FATAL_WARNINGS\n> +fi\n> +\n>  # Add libc MALLOC and MALLOC_PERTURB test\n>  # only if we are not executing the test with valgrind\n>  if test -n \"$valgrind\" ||\n"}]}