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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2007, 20:50 UTC
Message-ID: <7v4pifzawc.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20070831152153.GA30745@muzzle>

Eric Wong <normalperson@yhbt.net> writes:

Show 19 quoted lines

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