On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> 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.
Check.
>>>> + $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.
Good point. Now I just need to figure out how to be able to run the
tests :-) I guess I should just set off a job to build the whole tree,
and then it will just work..
> Simply use
>
> + $tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);
>
> or
>
> + $tabwidth = 8 if (!$tabwidth || $tabwidth <= 0);
>
> (though second version is more cryptic).
Yeah, i definitely prefer the first one - then again, I'm not really a
perl guy...
> 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)
Ok. Will add that. Want me to send a new patch with these things included?
--
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