On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Magnus Hagander <magnus@xxxxxxxxxxxx> writes:
>
>> Allow the gitweb.tabwidth option to control how many spaces a tab
>> gets expanded to.
>>
>> Signed-off-by: Magnus Hagander <magnus@xxxxxxxxxxxx>
>
> 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/
--
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