From: Magnus Hagander Date: Wed, 29 Sep 2010 08:39:27 GMT Subject: Re: [PATCH] Allow gitweb tab width to be set per project. Message-ID: In-Reply-To: On Tue, Sep 28, 2010 at 14:25, Jakub Narebski wrote: > Magnus Hagander writes: > >> Allow the gitweb.tabwidth option to control how many spaces a tab >> gets expanded to. >> >> Signed-off-by: Magnus Hagander > > 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. >> 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? 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) >> +     $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/