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

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

[Robert Newson]

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)$} ) {

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

[Junio C Hamano]

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

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> "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.

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

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

[Robert Newson]

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

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

[Junio C Hamano]

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

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

[Robert Newson]

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

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

[Robert Newson]

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 <robert.newson@gmail.com> 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 <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...
> >
>

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

[Robert Newson]

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 <robert.newson@gmail.com> 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 <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...
> >
>

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

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

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.

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

[Junio C Hamano]

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

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

[Sam Vilain]

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.

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

[Johannes Schindelin]

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

git-svn: Protect against "diff.color = true".

[Junio C Hamano]

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

Re: git-svn: Protect against "diff.color = true".

[Johannes Schindelin]

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

Re: git-svn: Protect against "diff.color = true".

[Junio C Hamano]

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.

Re: git-svn: Protect against "diff.color = true".

[Johannes Schindelin]

Hi,

On Fri, 31 Aug 2007, Junio C Hamano wrote:

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

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

Re: git-svn: Protect against "diff.color = true".

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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

Re: git-svn: Protect against "diff.color = true".

[Eric Wong]

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

Works for me here, although switching back to git-rev-list
(--pretty=raw) would make me more comfortable.

Acked-by: Eric Wong <normalperson@yhbt.net>

>  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