On Wed, 29 Sep 2010, Magnus Hagander wrote:
> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
>> Magnus Hagander <magnus@xxxxxxxxxxxx> writes:
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index a85e2f6..ef92a4f 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -1465,9 +1465,11 @@ sub unquote {
>>> # escape tabs (convert tabs to spaces)
>>> sub untabify {
>>> my $line = shift;
>>> + my $tabwidth = git_get_project_config('tabwidth', '--int');
>>
>> Note that untabify() is called once for each _line_ in a file or a
>> diff...
>
> Ha, that's what I get for thinking it was too easy. It actually was :-)
>
>
>> This has acceptable performance only because gitweb config is cached
>> in %config hash by git_get_project_config() subroutine.
>>
>>
>> I'm not sure if it wouldn't be better to have $tabwidth be passed as
>> an (optional) argument to untabify(), and calculated either in calling
>> sites for untabify(), or be calculated per-request and save in a
>> global variable.
>
> Given that it's cached, will it actually make a big difference?
Well, I agree that with config cached it could be left like this...
but I would like very much to perhaps have a comment about this, so other
people don't have to wonder.
>>> + $tabwidth = 8 if ($tabwidth <= 0);
>>
>> git_get_project_config('tabwidth', '--int') can return 'undef' if a
>> configuration key does not exist, resulting in
>>
>> Use of uninitialized value in numeric le (<=) at
>>
>> warning in web server logs.
>
> Ah, I knew that would go somewhere. Interestingly enough, it doesn't
> show up in the logs of the server I run it on now. But still should be
> fixed.
Whether such warning shows in web server logs might depend on whether
you are running gitweb under mod_perl, or as plain CGI script.
Nevertheless it is a good practice to check if a change passess
appropriate tests from git testsuite; t9500-gitweb-standalone-no-errors
should detect this.
Simply use
+ $tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);
or
+ $tabwidth = 8 if (!$tabwidth || $tabwidth <= 0);
(though second version is more cryptic).
P.S. If it is not a %feature, we might want to add description of
gitweb.tabwidth to the "Per-repository gitweb configuration" section
in gitweb/README (as next to last item)
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html