Perl warning in git-svn (git v1.5.3-rc7-16-ge340d7d)

> 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;

> "Robert Newson" <robert.newson@gmail.com> 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 <normalperson@yhbt.net> 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...
>

> 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 <gitster@pobox.com> wrote:
> > Eric Wong <normalperson@yhbt.net> 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...
> >
>

> 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 <gitster@pobox.com> wrote:
> > Eric Wong <normalperson@yhbt.net> 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 <gitster@pobox.com> 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...

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

>> 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 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"

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

> On Fri, 31 Aug 2007, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 ;-)

> 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 <gitster@pobox.com>
> ---
> 
>   Junio C Hamano <gitster@pobox.com> 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);