threads / patch / 25272

patchAllow gitweb tab width to be set per project.

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

## tl;dr

8 messages between Sep 28, 2010 and Oct 18, 2010. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Magnus Hagander· Sep 28, 2010, 11:35 UTC · lore

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(-)
Show changes to gitweb/gitweb.perl +3 −1
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
Jakub Narebski· Sep 28, 2010, 12:25 UTC · re: Magnus Hagander · lore

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

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

Show 5 quoted lines
> ---
> 
> 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.
Show 14 quoted lines
> 
> 
>  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.
Show 10 quoted lines
>  
>  	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
Magnus Hagander· Sep 29, 2010, 08:39 UTC · re: Jakub Narebski · lore

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

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/
Jakub Narebski· Sep 29, 2010, 09:22 UTC · re: Magnus Hagander · lore

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

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:
Show 26 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 :-)
> 
> 
>> 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.
 
Show 12 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.

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
Magnus Hagander· Oct 1, 2010, 11:56 UTC · re: Jakub Narebski · lore

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

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

Show 9 quoted lines
> 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/
Jakub Narebski· Oct 1, 2010, 21:02 UTC · re: Magnus Hagander · lore

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

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:
Show 7 quoted lines
>> 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.
Show 5 quoted lines
>> 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
Magnus Hagander· Oct 18, 2010, 10:31 UTC · re: Jakub Narebski · lore

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

On Fri, Oct 1, 2010 at 23:02, Jakub Narebski <jnareb@gmail.com> wrote:
Show 24 quoted lines
> 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.
Show 7 quoted lines
>>> 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/
Magnus Hagander· Oct 18, 2010, 10:41 UTC · re: Magnus Hagander · lore

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(-)
Show changes to 2 files +9 −1

gitweb/README, gitweb/gitweb.perl

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

← back to recent threads