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

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

[Magnus Hagander]

Allow the gitweb.tabwidth option to control how many spaces a tab
gets expanded to.

Signed-off-by: Magnus Hagander <magnus@hagander.net>
---

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.


 gitweb/gitweb.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

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');
+	$tabwidth = 8 if ($tabwidth <= 0);
 
 	while ((my $pos = index($line, "\t")) != -1) {
-		if (my $count = (8 - ($pos % 8))) {
+		if (my $count = ($tabwidth - ($pos % $tabwidth))) {
 			my $spaces = ' ' x $count;
 			$line =~ s/\t/$spaces/;
 		}
-- 
1.7.0.4

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

[Jakub Narebski]

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.

  Different project scan use different tab widths, even on the same
  gitweb server.

Or something like that.

> 
> 
>  gitweb/gitweb.perl |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> 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...

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.

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

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

>  
>  	while ((my $pos = index($line, "\t")) != -1) {
> -		if (my $count = (8 - ($pos % 8))) {
> +		if (my $count = ($tabwidth - ($pos % $tabwidth))) {
>  			my $spaces = ' ' x $count;
>  			$line =~ s/\t/$spaces/;
>  		}
> -- 
> 1.7.0.4
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

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

[Magnus Hagander]

On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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

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

[Jakub Narebski]

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

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

[Magnus Hagander]

On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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/

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

[Jakub Narebski]

On Fri, 1 Oct 2010, Magnus Hagander wrote:
> On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:
>> 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:

>> 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..
 
To test other parts of git, you need to first compile, and then run tests
(e.g. by running "make test" after "make").  Gitweb tests check the 
source version (for historical reason, namely that there were no gitweb
target in main makefile, and gitweb didn't get compiled by default),
so in that case you need to compile git once (to satisfy test suite),
and then run e.g.

 # (cd t && ./t9500-gitweb-standalone-no-errors.sh)
 
from the top directory of git repository.

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

Yes, please do.

-- 
Jakub Narebski
Poland

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

[Magnus Hagander]

On Fri, Oct 1, 2010 at 23:02, Jakub Narebski <jnareb@gmail.com> wrote:
> On Fri, 1 Oct 2010, Magnus Hagander wrote:
>> On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:
>>> 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:
>
>>> 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..
>
> To test other parts of git, you need to first compile, and then run tests
> (e.g. by running "make test" after "make").  Gitweb tests check the
> source version (for historical reason, namely that there were no gitweb
> target in main makefile, and gitweb didn't get compiled by default),
> so in that case you need to compile git once (to satisfy test suite),
> and then run e.g.
>
>  # (cd t && ./t9500-gitweb-standalone-no-errors.sh)
>
> from the top directory of git repository.

Yeah, I got that working a while ago. Just had to build the whole thing.


>>> 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?
>
> Yes, please do.

Sorry, this one ended up filed under the completely wrong tag in my
in-<real-life>-memory-bugtracker, forgot all about it. New patch
forthcoming in a couple of minutes - just need to complete the tests
again.

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

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

[Magnus Hagander]

Allow the gitweb.tabwidth option to control how many spaces a tab
gets expanded to.

Signed-off-by: Magnus Hagander <magnus@hagander.net>
---
 gitweb/README      |    4 ++++
 gitweb/gitweb.perl |    6 +++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index bf3664f..d2e4a1d 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -312,6 +312,10 @@ You can use the following files in repository:
    repository's owner. It is displayed in the project list and summary
    page. If it's not set, filesystem directory's owner is used
    (via GECOS field / real name field from getpwiud(3)).
+ * gitweb.tabwidth
+   You can use the gitweb.tabwidth repository configuration variable to set
+   the number of spaces that tabs should be expanded to, instead of the
+   default 8.
  * various gitweb.* config variables (in config)
    Read description of %feature hash for detailed list, and some
    descriptions.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8d7e4c5..66c258f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1478,9 +1478,13 @@ sub unquote {
 # escape tabs (convert tabs to spaces)
 sub untabify {
 	my $line = shift;
+	# git_get_project_config caches the value for us, so it's ok
+	# to call it once for each line.
+	my $tabwidth = git_get_project_config('tabwidth', '--int');
+	$tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);
 
 	while ((my $pos = index($line, "\t")) != -1) {
-		if (my $count = (8 - ($pos % 8))) {
+		if (my $count = ($tabwidth - ($pos % $tabwidth))) {
 			my $spaces = ' ' x $count;
 			$line =~ s/\t/$spaces/;
 		}
-- 
1.7.0.4