# Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) 18 messages from 2007-08-31 to 2007-08-31. Participants: Robert Newson, Junio C Hamano, Eric Wong, Johannes Schindelin, Sam Vilain. Thread: https://gitlist.dev/t/9721 ## Robert Newson, 2007-08-31 12:58 Subject: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46aeb24f0708310558t2defc547v483586f116d8b8ac@mail.gmail.com> URL: https://gitlist.dev/e/46aeb24f0708310558t2defc547v483586f116d8b8ac%40mail.gmail.com ``` Hi, The latest head of git gives me this when doing most operations, this also happens with the rc7 experimental Debian package. It's annoying because it prints this line hundreds of times for each call to 'log', for example "Use of uninitialized value in string eq at blah/git/git-svn line 826." and this fixes it; diff --git a/git-svn.perl b/git-svn.perl index 4e325b7..3d0c76d 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -808,7 +808,7 @@ sub cmt_metadata { sub working_head_info { my ($head, $refs) = @_; my ($fh, $ctx) = command_output_pipe('log', $head); - my $hash; + my $hash = ""; my %max; while (<$fh>) { if ( m{^commit ($::sha1)$} ) { ``` ## Junio C Hamano, 2007-08-31 14:52 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <7vveav21uv.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vveav21uv.fsf%40gitster.siamese.dyndns.org In-Reply-To: <46aeb24f0708310558t2defc547v483586f116d8b8ac@mail.gmail.com> ``` "Robert Newson" writes: > The latest head of git gives me this when doing most operations, this > also happens with the rc7 experimental Debian package. It's annoying > because it prints this line hundreds of times for each call to 'log', > for example > > "Use of uninitialized value in string eq at blah/git/git-svn line 826." > > and this fixes it; Curious. I wonder how can it trigger. Presimably, that while (<$fh>) loop is reading from git-log, and the first line would look like "commit [0-9a-f]{40}" and will set $hash, do "next". Which means the variable should have been initialized by the time the part that complains about string eq (which I think is "if ($c && $c eq $hash)" comparison) is reached. ``` ## Eric Wong, 2007-08-31 15:21 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <20070831152153.GA30745@muzzle> URL: https://gitlist.dev/e/20070831152153.GA30745%40muzzle In-Reply-To: <7vveav21uv.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Robert Newson" writes: > > > The latest head of git gives me this when doing most operations, this > > also happens with the rc7 experimental Debian package. It's annoying > > because it prints this line hundreds of times for each call to 'log', > > for example > > > > "Use of uninitialized value in string eq at blah/git/git-svn line 826." > > > > and this fixes it; > > Curious. I wonder how can it trigger. > > Presimably, that while (<$fh>) loop is reading from git-log, and > the first line would look like "commit [0-9a-f]{40}" and will > set $hash, do "next". Which means the variable should have been > initialized by the time the part that complains about string eq > (which I think is "if ($c && $c eq $hash)" comparison) is > reached. This could be a sign of a bigger problem. Does git-log read .git/config and that could potentially change its default output format? A quick scan of the docs say "no". I remember using git-rev-list in the original code because git-log was (is still?) considered porcelain and less suitable for automated parsing... -- Eric Wong ``` ## Robert Newson, 2007-08-31 15:31 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46aeb24f0708310831n3c16dbbeke173b6f0cdfa7792@mail.gmail.com> URL: https://gitlist.dev/e/46aeb24f0708310831n3c16dbbeke173b6f0cdfa7792%40mail.gmail.com In-Reply-To: <20070831152153.GA30745@muzzle> ``` The difference between the version I'm using (1.5.2.4) and this broken version shows us what's happened. The $hash value was initialised in the loop, and now it isn't. Running 'git-svn log' in my git-svn'ed repo shows this up every time, it's easy to reproduce. my ($head, $refs) = @_; - my ($fh, $ctx) = command_output_pipe('rev-list', $head); - while (my $hash = <$fh>) { - chomp($hash); - my ($url, $rev, $uuid) = cmt_metadata($hash); + my ($fh, $ctx) = command_output_pipe('log', $head); + my $hash = ""; + my %max; + while (<$fh>) { + if ( m{^commit ($::sha1)$} ) { + unshift @$refs, $hash if $hash and $refs; + $hash = $1; + next; + } ``` ## Junio C Hamano, 2007-08-31 16:09 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <7vfy1z1y9u.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfy1z1y9u.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20070831152153.GA30745@muzzle> ``` Eric Wong writes: > This could be a sign of a bigger problem. > > Does git-log read .git/config and that could potentially change > its default output format? A quick scan of the docs say "no". "diff.color = always" could break it I would imagine... ``` ## Robert Newson, 2007-08-31 16:18 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46aeb24f0708310918u29fa5d17r9878658e68702fca@mail.gmail.com> URL: https://gitlist.dev/e/46aeb24f0708310918u29fa5d17r9878658e68702fca%40mail.gmail.com In-Reply-To: <7vfy1z1y9u.fsf@gitster.siamese.dyndns.org> ``` I have these settings in ~/.gitconfig; [color] branch = true diff = true pager = true status = true not a problem in 1.5.2.4. On 8/31/07, Junio C Hamano wrote: > Eric Wong writes: > > > This could be a sign of a bigger problem. > > > > Does git-log read .git/config and that could potentially change > > its default output format? A quick scan of the docs say "no". > > "diff.color = always" could break it I would imagine... > ``` ## Robert Newson, 2007-08-31 16:25 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46aeb24f0708310925n707244d0nf8f58efda0145ac@mail.gmail.com> URL: https://gitlist.dev/e/46aeb24f0708310925n707244d0nf8f58efda0145ac%40mail.gmail.com In-Reply-To: <46aeb24f0708310918u29fa5d17r9878658e68702fca@mail.gmail.com> ``` Confirmed, if I switch 'true' to 'auto', the problem goes away. It's a side-effect but an ugly one. On 8/31/07, Robert Newson wrote: > I have these settings in ~/.gitconfig; > > [color] > branch = true > diff = true > pager = true > status = true > > not a problem in 1.5.2.4. > > On 8/31/07, Junio C Hamano wrote: > > Eric Wong writes: > > > > > This could be a sign of a bigger problem. > > > > > > Does git-log read .git/config and that could potentially change > > > its default output format? A quick scan of the docs say "no". > > > > "diff.color = always" could break it I would imagine... > > > ``` ## Robert Newson, 2007-08-31 16:25 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46aeb24f0708310925x62d9f92fsb2e0938f343f7aea@mail.gmail.com> URL: https://gitlist.dev/e/46aeb24f0708310925x62d9f92fsb2e0938f343f7aea%40mail.gmail.com In-Reply-To: <46aeb24f0708310918u29fa5d17r9878658e68702fca@mail.gmail.com> ``` Confirmed, if I switch 'true' to 'auto', the problem goes away. It's a side-effect but an ugly one. On 8/31/07, Robert Newson wrote: > I have these settings in ~/.gitconfig; > > [color] > branch = true > diff = true > pager = true > status = true > > not a problem in 1.5.2.4. > > On 8/31/07, Junio C Hamano wrote: > > Eric Wong writes: > > > > > This could be a sign of a bigger problem. > > > > > > Does git-log read .git/config and that could potentially change > > > its default output format? A quick scan of the docs say "no". > > > > "diff.color = always" could break it I would imagine... > > > ``` ## Junio C Hamano, 2007-08-31 20:50 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <7v4pifzawc.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pifzawc.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20070831152153.GA30745@muzzle> ``` Eric Wong writes: > Junio C Hamano wrote: > ... >> Curious. I wonder how can it trigger. >> >> Presimably, that while (<$fh>) loop is reading from git-log, and >> the first line would look like "commit [0-9a-f]{40}" and will >> set $hash, do "next". Which means the variable should have been >> initialized by the time the part that complains about string eq >> (which I think is "if ($c && $c eq $hash)" comparison) is >> reached. > > This could be a sign of a bigger problem. > > Does git-log read .git/config and that could potentially change > its default output format? A quick scan of the docs say "no". > > I remember using git-rev-list in the original code because git-log was > (is still?) considered porcelain and less suitable for automated > parsing... Yes, git-log is Porcelain, and it is subject to this kind of breakage. I was almost going to suggest us to change "*.color = true" to mean 'auto'. Because git can internally use pager and has a way for the user to control enabling/disabling colors when the pager is used, there is no _logical_ reason to enable pager when the output is not going to a tty. However, people from CVS/SVN background are used to type: $ scm log | less because they are not used to our "by default we page" setup, and it is very understandable that they would want "*.color = true" in their configuration honored in such a usage. We probably should do two things to resolve this. * Protect our scripts. When parsing from "git log" and any other Porcelain, explicitly give --no-color. * Educate users. Tool output, even from Porcelain, are not to be colored or molested in any other way, when going to anywhere other than a tty. Say in the FAQ "if you are tempted to say *.color = true, stop. Learn not to type the extra '| less' and let the internal pager take care of paging for you and you will be much happier". Additionally we could do the following, but if we do the above two properly, there should be no need to: * Declare "*.color = true" will mean "*.color = auto" in the next version, now. * Actually switch "*.color = true" to mean "*.color = auto" in the next version (not 1.5.3 but the one after that). * Perhaps introduce "*.color = always" at the same time we do the above switch, but I think that has the same issue as the current "true" has, so I doubt this step is needed. ``` ## Junio C Hamano, 2007-08-31 21:22 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <7vd4x3xuu3.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4x3xuu3.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7v4pifzawc.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > I was almost going to suggest us to change "*.color = true" to > mean 'auto'. Because git can internally use pager and has a way > for the user to control enabling/disabling colors when the pager > is used, there is no _logical_ reason to enable pager when the > output is not going to a tty. Gaah, sorry; I meant "no logical reason to enable _color_ when the output is not going to a tty". ``` ## Junio C Hamano, 2007-08-31 21:29 Subject: git-svn: Protect against "diff.color = true". Message-ID: <7v4pifxuia.fsf_-_@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pifxuia.fsf_-_%40gitster.siamese.dyndns.org In-Reply-To: <7v4pifzawc.fsf@gitster.siamese.dyndns.org> ``` If the configuration of the user has "diff.color = true", the output from "log" we invoke internally added color codes, which broke the parser. Signed-off-by: Junio C Hamano --- Junio C Hamano writes: > We probably should do two things to resolve this. > > * Protect our scripts. When parsing from "git log" and any > other Porcelain, explicitly give --no-color. Here is my attempt -- I do not have an easy access to SVN repo to interoperate with, so a testing by real-world users and an Ack is appreciated. I think some fix for this issue (not necessarily this patch) should be in 1.5.3 final. git-svn.perl | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 4e325b7..98218da 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -807,7 +807,7 @@ sub cmt_metadata { sub working_head_info { my ($head, $refs) = @_; - my ($fh, $ctx) = command_output_pipe('log', $head); + my ($fh, $ctx) = command_output_pipe('log', '--no-color', $head); my $hash; my %max; while (<$fh>) { @@ -2072,7 +2072,7 @@ sub rebuild { return; } print "Rebuilding $db_path ...\n"; - my ($log, $ctx) = command_output_pipe("log", $self->refname); + my ($log, $ctx) = command_output_pipe("log", '--no-color', $self->refname); my $latest; my $full_url = $self->full_url; remove_username($full_url); ``` ## Johannes Schindelin, 2007-08-31 21:38 Subject: Re: git-svn: Protect against "diff.color = true". Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708312237340.28586%40racer.site In-Reply-To: <7v4pifxuia.fsf_-_@gitster.siamese.dyndns.org> ``` Hi, On Fri, 31 Aug 2007, Junio C Hamano wrote: > I do not have an easy access to SVN repo > to interoperate with, so a testing by real-world users and an > Ack is appreciated. I just tested on a busybox clone: Works as expected. Without your patch, I get the uninitialised values, with your patch it is fine. ACK. Ciao, Dscho ``` ## Junio C Hamano, 2007-08-31 21:46 Subject: Re: git-svn: Protect against "diff.color = true". Message-ID: <7vtzqfwf5c.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzqfwf5c.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > On Fri, 31 Aug 2007, Junio C Hamano wrote: > >> I do not have an easy access to SVN repo >> to interoperate with, so a testing by real-world users and an >> Ack is appreciated. > > I just tested on a busybox clone: Works as expected. Without your patch, > I get the uninitialised values, with your patch it is fine. > > ACK. Thanks. It's customary that the privilege to issue Ack is reserved to the primary owner of the code. We are a relatively small friendly community and it is not a big deal, but if you ever work on the kernel, be somewhat more careful. People are picky over there on such details. ``` ## Johannes Schindelin, 2007-08-31 21:53 Subject: Re: git-svn: Protect against "diff.color = true". Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708312251510.28586%40racer.site In-Reply-To: <7vtzqfwf5c.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 31 Aug 2007, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Fri, 31 Aug 2007, Junio C Hamano wrote: > > > >> I do not have an easy access to SVN repo > >> to interoperate with, so a testing by real-world users and an > >> Ack is appreciated. > > > > I just tested on a busybox clone: Works as expected. Without your patch, > > I get the uninitialised values, with your patch it is fine. > > > > ACK. > > Thanks. > > It's customary that the privilege to issue Ack is reserved to > the primary owner of the code. We are a relatively small > friendly community and it is not a big deal, but if you ever > work on the kernel, be somewhat more careful. People are picky > over there on such details. Happily, we are a much friendlier bunch here ;-) Besides, since I feel we're really close to 1.5.3 now, I thought that you might want to here as many positive votes as you can get. But yes, I'll keep that in mind ;-) Ciao, Dscho ``` ## Eric Wong, 2007-08-31 21:58 Subject: Re: git-svn: Protect against "diff.color = true". Message-ID: <20070831215752.GA31033@untitled> URL: https://gitlist.dev/e/20070831215752.GA31033%40untitled In-Reply-To: <7v4pifxuia.fsf_-_@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > If the configuration of the user has "diff.color = true", the > output from "log" we invoke internally added color codes, which > broke the parser. > > Signed-off-by: Junio C Hamano > --- > > Junio C Hamano writes: > > > We probably should do two things to resolve this. > > > > * Protect our scripts. When parsing from "git log" and any > > other Porcelain, explicitly give --no-color. > > Here is my attempt -- I do not have an easy access to SVN repo > to interoperate with, so a testing by real-world users and an > Ack is appreciated. I think some fix for this issue (not > necessarily this patch) should be in 1.5.3 final. Works for me here, although switching back to git-rev-list (--pretty=raw) would make me more comfortable. Acked-by: Eric Wong > git-svn.perl | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/git-svn.perl b/git-svn.perl > index 4e325b7..98218da 100755 > --- a/git-svn.perl > +++ b/git-svn.perl > @@ -807,7 +807,7 @@ sub cmt_metadata { > > sub working_head_info { > my ($head, $refs) = @_; > - my ($fh, $ctx) = command_output_pipe('log', $head); > + my ($fh, $ctx) = command_output_pipe('log', '--no-color', $head); > my $hash; > my %max; > while (<$fh>) { > @@ -2072,7 +2072,7 @@ sub rebuild { > return; > } > print "Rebuilding $db_path ...\n"; > - my ($log, $ctx) = command_output_pipe("log", $self->refname); > + my ($log, $ctx) = command_output_pipe("log", '--no-color', $self->refname); > my $latest; > my $full_url = $self->full_url; > remove_username($full_url); -- Eric Wong ``` ## Junio C Hamano, 2007-08-31 22:14 Subject: Re: git-svn: Protect against "diff.color = true". Message-ID: <7vps13wdw1.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vps13wdw1.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > On Fri, 31 Aug 2007, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> ... >> > I just tested on a busybox clone: Works as expected. Without your patch, >> > I get the uninitialised values, with your patch it is fine. >> > >> > ACK. >> >> Thanks. >> >> It's customary that the privilege to issue Ack is reserved to >> the primary owner of the code. We are a relatively small >> friendly community and it is not a big deal, but if you ever >> work on the kernel, be somewhat more careful. People are picky >> over there on such details. > > Happily, we are a much friendlier bunch here ;-) > > Besides, since I feel we're really close to 1.5.3 now, I thought that you > might want to here as many positive votes as you can get. > > But yes, I'll keep that in mind ;-) Oh, I did not mean to sound like your testing does not count. I was just cautioning about using the word "Ack". I will have a "Tested-by: " line with your name on it, and I am hoping I can get an Ack from Eric but it is such an "obviously correct" looking change, so ... ``` ## Sam Vilain, 2007-08-31 23:47 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: <46D8A886.5060701@vilain.net> URL: https://gitlist.dev/e/46D8A886.5060701%40vilain.net In-Reply-To: <7vd4x3xuu3.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: >> I was almost going to suggest us to change "*.color = true" to >> mean 'auto'. Because git can internally use pager and has a way >> for the user to control enabling/disabling colors when the pager >> is used, there is no _logical_ reason to enable pager when the >> output is not going to a tty. >> > > Gaah, sorry; I meant "no logical reason to enable _color_ when > the output is not going to a tty". Sure there is, when the output is "less -R" Sam. ``` ## Johannes Schindelin, 2007-08-31 23:51 Subject: Re: Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d) Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709010049490.28586%40racer.site In-Reply-To: <46D8A886.5060701@vilain.net> ``` Hi, On Sat, 1 Sep 2007, Sam Vilain wrote: > Junio C Hamano wrote: > >> I was almost going to suggest us to change "*.color = true" to > >> mean 'auto'. Because git can internally use pager and has a way > >> for the user to control enabling/disabling colors when the pager > >> is used, there is no _logical_ reason to enable pager when the > >> output is not going to a tty. > >> > > > > Gaah, sorry; I meant "no logical reason to enable _color_ when > > the output is not going to a tty". > > Sure there is, when the output is "less -R" This argument was already raised by Junio himself. But your _option_ to use "git log | less -R" does not make it a good option. Since "git log" is so much more elegant already. Hth, Dscho ```