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

From: Magnus Hagander <magnus@hagander.net>
Date: Sep 29, 2010, 08:39 UTC
Message-ID: <AANLkTikMjVQgEzLQ5Z95cmb5fkQ5iSzqfA4T=D1zzy=j@mail.gmail.com>
In-Reply-To: <m34odagioh.fsf@localhost.localdomain>

On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:

Show 17 quoted lines

> Magnus Hagander <magnus@hagander.net> writes:
>
>> Allow the gitweb.tabwidth option to control how many spaces a tab
>> gets expanded to.
>>
>> Signed-off-by: Magnus Hagander <magnus@hagander.net>
>
> This might be a good idea, but the solution looks like it includes
> some unnecessary performance hit (see coment below).
>
>> ---
>>
>> In the PostgreSQL project, we're using 4-space tabs, but we have other projects
>> as well on our gitweb server, so we need to be able to control this on a
>> per-project basis.
>
> Some of this comment should IMHO make it into commit message, e.g.

Check.

Show 12 quoted lines

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

Show 8 quoted lines

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

In order to put it on all the callsites to untabify() it needs to for example go as a parameter to format_diff_line to make any difference (or it'd still be called once per line), so that would be quite a bit of pollution I think.

Putting it as a global variable is certainly an option - but I didn't find a pattern of something else to follow for that, so I figured it was frowned upon?

> We might want to turn 'tabwidth' into a feature, though that is
> probably overengineering.

What advantage(s) would it give? (I admit I'm not particularly familiar with the code - this was just the easiest way to get it done quickly)

Show 8 quoted lines

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

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/