Re: [PATCH] Allow gitweb tab width to be set per project.

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 29 Sep 2010, Magnus Hagander wrote:
> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:
>> Magnus Hagander <magnus@hagander.net> 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