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

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 28, 2010, 12:25 UTC
Message-ID: <m34odagioh.fsf@localhost.localdomain>
In-Reply-To: <1285673709-24924-1-git-send-email-magnus@hagander.net>

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