[PATCH] gitweb: add a setting to control the tabstop width

[PATCH] gitweb: add a setting to control the tabstop width

[Charles Bailey]

Not everyone uses the same tab width. gitweb learns a new setting to
control the tabstop width. The configuration can be set globally and
on a per project basis. The default is 8, preserving existing
behaviour. The configuration variable name is borrowed from the vim
setting with the same behaviour.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

The untabify function seems the sensible place to make the change. As
untabify is called once per line from various different locations it
also makes sense to cache the result of the config lookup in a package
variable, though this makes the change slightly less neat.

This change should have a minimal impact on performance but it would
appreciate some more eyes and ideally some performance testing on
heavier systems than my own. 

 gitweb/gitweb.perl |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 922dee9..cdabe37 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -108,6 +108,12 @@ our $mimetypes_file = undef;
 # could be even 'utf-8' for the old behavior)
 our $fallback_encoding = 'latin1';
 
+# variable to keep track of the the current tabstop width
+# this is a package variable as the natural place to check the feature is in
+# the untabify function, but as the function is called once per line we don't
+# want to have to recheck the config for each line
+our $tabstop_width;
+
 # rename detection options for git-diff and git-diff-tree
 # - default is '-M', with the cost proportional to
 #   (number of removed files) * (number of new files).
@@ -275,6 +281,18 @@ our %feature = (
 	'forks' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# Tabstop width.  Controls the number of spaces to which tabs are
+	# expanded. Default is 8.
+	# To change system wide add the following to $GITWEB_CONFIG
+	# $feature{'tabstop'}{'default'} = [8];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'tabstop'}{'override'} = 1;
+	# and in project config gitweb.tabstop = <width>
+	'tabstop' => {
+		'sub' => \&feature_tabstop,
+		'override' => 0,
+		'default' => [8]},
 );
 
 sub gitweb_check_feature {
@@ -340,6 +358,11 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_tabstop {
+	my ($val) = git_get_project_config('tabstop', '--int');
+	return defined($val) ? ($val) : ($_[0])
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -832,8 +855,12 @@ sub unquote {
 sub untabify {
 	my $line = shift;
 
+	if (!defined($tabstop_width)) {
+		($tabstop_width) = gitweb_check_feature('tabstop');
+	}
+
 	while ((my $pos = index($line, "\t")) != -1) {
-		if (my $count = (8 - ($pos % 8))) {
+		if (my $count = ($tabstop_width - ($pos % $tabstop_width))) {
 			my $spaces = ' ' x $count;
 			$line =~ s/\t/$spaces/;
 		}
-- 
1.5.4.3.432.g5ecfc


-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Jakub Narebski]

Charles Bailey <charles@hashpling.org> writes:

> Not everyone uses the same tab width. gitweb learns a new setting to
> control the tabstop width. The configuration can be set globally and
> on a per project basis. The default is 8, preserving existing
> behaviour. The configuration variable name is borrowed from the vim
> setting with the same behaviour.

Good idea. Very nice change.
 
> Signed-off-by: Charles Bailey <charles@hashpling.org>
> ---
> 
> The untabify function seems the sensible place to make the change. As
> untabify is called once per line from various different locations it
> also makes sense to cache the result of the config lookup in a package
> variable, though this makes the change slightly less neat.

Since b201927 (gitweb: Read repo config using 'git config -z -l')
repository config is cached in %config hash (per repository), so
I don't think global / package variable $tabstop_width is really
needed...
 
> This change should have a minimal impact on performance but it would
> appreciate some more eyes and ideally some performance testing on
> heavier systems than my own. 

...but it would be better if you have checked at least on your system
if it does affect performance or not.

[...]

+our $tabstop_width;

I think I would write "our $tabstop_width = 8;" here.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Charles Bailey]

On Mon, Mar 03, 2008 at 11:33:28PM +0100, Jakub Narebski wrote:
> Charles Bailey <charles@hashpling.org> writes:
> > 
> > The untabify function seems the sensible place to make the change. As
> > untabify is called once per line from various different locations it
> > also makes sense to cache the result of the config lookup in a package
> > variable, though this makes the change slightly less neat.
> 
> Since b201927 (gitweb: Read repo config using 'git config -z -l')
> repository config is cached in %config hash (per repository), so
> I don't think global / package variable $tabstop_width is really
> needed...

Fair point, although we still save the cost of some 'is the config
variable overrideable and if so is it overriden' logic.  Untabify is a
once per line call which is more frequesnt than most gitweb config
checking calls.

> > This change should have a minimal impact on performance but it would
> > appreciate some more eyes and ideally some performance testing on
> > heavier systems than my own. 
> 
> ...but it would be better if you have checked at least on your system
> if it does affect performance or not.
> 

Not noticeably (on an old AMD Duron 900MHz), but my tests have been
unscientific.

> [...]
> 
> +our $tabstop_width;
> 
> I think I would write "our $tabstop_width = 8;" here.

Currently, I use the fact that it is initially 'undef' to know that I
haven't checked the config yet. The config is then checked on the
first time through untabify.

Charles.

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Jakub Narebski]

Jakub Narebski wrote:

> +our $tabstop_width;
> 
> I think I would write "our $tabstop_width = 8;" here.

I'm sorry. Please drop this part. Seting it to undef is needed
if it is used as cache (as: not read in), while if we use %config
it is simply not needed.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> +	# Tabstop width.  Controls the number of spaces to which tabs are
> +	# expanded. Default is 8.
> +	# To change system wide add the following to $GITWEB_CONFIG
> +	# $feature{'tabstop'}{'default'} = [8];
> +	# To have project specific config enable override in $GITWEB_CONFIG
> +	# $feature{'tabstop'}{'override'} = 1;
> +	# and in project config gitweb.tabstop = <width>
> +	'tabstop' => {
> +		'sub' => \&feature_tabstop,
> +		'override' => 0,
> +		'default' => [8]},

Some people say "Tabs are 8 characters, and thus indentations are also 8
characters.  There are heretic movements that try to make indentations 4
(or even 2!)  characters deep, and that is akin to trying to define the
value of PI to be 3."  Some people disagree.

But while viewing what is etched in the history, it does not hurt anybody
else if the viewer uses different tab width.  Choice is good.

However, a choice made by the hosting service that runs gitweb would not
help individual viewers with different tab-width taste.  Neither does
configuration that is per-repository.  Participants of the same project
would want to view contents with different tab-width.

Perhaps the tabstop "feature" should control _if_ the tab width of the
material gitweb feeds can be tailored at all (i.e. boolean).  And when
enabled, it would leave the choice of non-8 tab width to the browser (the
way to maintain per-client choice could be cookies or extra parameters, I
do not really care the details), and use that preferred tab-width in the
untabify function.

On the other hand, maybe the tab-width customization is not about user
preference but what tab-width was used when the contents were created.  In
such a case, probably the right thing to do would be to look at the
tab-width hints embedded in the file.  In such a case, probably the tab
width setting need to be per-path (e.g. *.c files may use standard 8,
while *.py may use heretic 4).  Again, site-wide or repository-wide
configuration would not help.

In short, I do not like the patch, not because I do not like customizable
tab-width, but because I think the customizability the patch offers is of
the wrong kind and too limited to be useful.

P.S.

It might be interesting to come up with a heuristics to _guess_ the tab
width used by the content creator by looking at the contents, by the way.
There obviously are Emacs "Local Variables" and "-*-" lines and equivalent
clues vim would leave, but you could probably also use indentation levels
as a cue.

And perhaps teach the underlying git commands a special flag to expand
tabs on the output.

	"git cat-file --expand=auto blob Makefile"
	"git diff --expand=8 HEAD^..HEAD frotz.c"

;-)

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Jakub Narebski]

Charles Bailey wrote:
> On Mon, Mar 03, 2008 at 11:33:28PM +0100, Jakub Narebski wrote:
>> Charles Bailey <charles@hashpling.org> writes:
>>> 
>>> The untabify function seems the sensible place to make the change. As
>>> untabify is called once per line from various different locations it
>>> also makes sense to cache the result of the config lookup in a package
>>> variable, though this makes the change slightly less neat.
>> 
>> Since b201927 (gitweb: Read repo config using 'git config -z -l')
>> repository config is cached in %config hash (per repository), so
>> I don't think global / package variable $tabstop_width is really
>> needed...
> 
> Fair point, although we still save the cost of some 'is the config
> variable overrideable and if so is it overriden' logic.  Untabify is a
> once per line call which is more frequesnt than most gitweb config
> checking calls.

Good enough.

One think I'd worry about is interaction with mod_perl (or FastCGI),
namely if $tabstop_width wouldn't get stale information.  Perhaps
writing

  our $tabstop_width = undef;

as initializer would be enough.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Martin Langhoff]

On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  Some people say "Tabs are 8 characters, and thus indentations are also 8
>  characters.  There are heretic movements that try to make indentations 4
>  (or even 2!)  characters deep, and that is akin to trying to define the
>  value of PI to be 3."  Some people disagree.

And on the web, people use CSS to sort these disagreements amicably...
As a web apps guy, adding a setting for something like this, and then
changing the output feels _very_ weird, as it breaks with a lot of
stuff that Just Works in the HTTP+HTML world even for users that view
it differently... like caching :-)

cheers,



martin

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Jakub Narebski]

On Tue, 4 Mar 2008, Martin Langhoff wrote:
> On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>  Some people say "Tabs are 8 characters, and thus indentations are also 8
>>  characters.  There are heretic movements that try to make indentations 4
>>  (or even 2!)  characters deep, and that is akin to trying to define the
>>  value of PI to be 3."  Some people disagree.
> 
> And on the web, people use CSS to sort these disagreements amicably...
> As a web apps guy, adding a setting for something like this, and then
> changing the output feels _very_ weird, as it breaks with a lot of
> stuff that Just Works in the HTTP+HTML world even for users that view
> it differently... like caching :-)

The problem with using CSS to select tabstop width is twofold. First,
it has to work correctly also in text browsers like lynx, elinks, w3m.
Second, it is tabstop size, not the width of tab character; I'm not
sure if it is possible to implement it in CSS.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Charles Bailey]

On Mon, Mar 03, 2008 at 03:13:39PM -0800, Junio C Hamano wrote:
> Some people say "Tabs are 8 characters, and thus indentations are also 8
> characters.  There are heretic movements that try to make indentations 4
> (or even 2!)  characters deep, and that is akin to trying to define the
> value of PI to be 3."  Some people disagree.
> 
> But while viewing what is etched in the history, it does not hurt anybody
> else if the viewer uses different tab width.  Choice is good.
> 
> However, a choice made by the hosting service that runs gitweb would not
> help individual viewers with different tab-width taste.  Neither does
> configuration that is per-repository.  Participants of the same project
> would want to view contents with different tab-width.

I don not want to, nor shall I, argue about what interpretation of
ASCII HT is correct. I do want to argue that choice is good, and
furthermore that choice at the project level is appropriate.

Personally, I do not agree that participants of the same project would
want to view contents with different tab-width - or if they do, that
they shouldn't ;-) .

I work on, or contribute to, a number of projects. I have a preference
for indentation level of source, but I try to match the conventions of
the project to which I am contributing. If tabs are used, and if so
how wide they are, are important project conventions.

For some projects they are mandatory standards.

For some sets of conventions the width of a tab is not important, but
for many they are.

You can have the convention that tabs shall not be used, but you
cannot have a convention that tabs shall or can be used in conjunction
with a convention about maximum line length without and agreement on
tab-width.

git-mergetool.sh exhibits an example. If you look at the file with
tabs set to 4 then the gvimdiff 'case' in the merge_file shell
function *looks* correctly indented, but in other cases, the contents
of the if statements do not look indented when they should be.

If you look at git-mergetool.sh with tabs set to 8, then the other
cases look correctly indented, but the gvimdiff case is over-indented.

Clearly, what happened is that a user edited a file that should have
been viewed with ts=8 as ts=4 and the result is now 'wrong' in
different ways in different places.

[ Aside: this particular inconsistency is fixed in pu :-) ]

> On the other hand, maybe the tab-width customization is not about user
> preference but what tab-width was used when the contents were created.  In
> such a case, probably the right thing to do would be to look at the
> tab-width hints embedded in the file.  In such a case, probably the tab
> width setting need to be per-path (e.g. *.c files may use standard 8,
> while *.py may use heretic 4).  Again, site-wide or repository-wide
> configuration would not help.

This, I certainly agree with more. Tab width is about choice at
content creation. My argument is that choice at content creation
should follow project-wide conventions. I agree that my patch doesn't
provide a totally flexible solution but I do believe that there are
projects that either use one type of source file - or at least only one
tab width - which would benefit from the patch.

I run a gitweb.cgi which has a handful of projects, one of which uses
a popular IDE on a popular proprietary OS and uses a ts=4 convention
and another which uses ts=8 convention. The patch was driven by this
requirement and a (possibly erroneous) feeling that I probably wasn't
alone.

> In short, I do not like the patch, not because I do not like customizable
> tab-width, but because I think the customizability the patch offers is of
> the wrong kind and too limited to be useful.

Whereas I believe it provides some useful customization, the
customization is appropriate in many situations and the costs
(performance and code complexity) are minimal. Obviously, integration
is your call, but the patch will certainly stay on my own http server.

> P.S.
> 
> It might be interesting to come up with a heuristics to _guess_ the tab
> width used by the content creator by looking at the contents, by the way.
> There obviously are Emacs "Local Variables" and "-*-" lines and equivalent
> clues vim would leave, but you could probably also use indentation levels
> as a cue.

Perhaps. I don't really care for -*- style fluff but that's just
personal. The heuristics could be tricky to get right, probably
impossible in the case of users with tab conventions mangling the same
file :-) .

> And perhaps teach the underlying git commands a special flag to expand
> tabs on the output.
> 
> 	"git cat-file --expand=auto blob Makefile"
> 	"git diff --expand=8 HEAD^..HEAD frotz.c"
> 
> ;-)
> 

And I'd argue that expanding tabs was the job of the terminal and not
core git, but hey :-) !

Charles.

Re: [PATCH] gitweb: add a setting to control the tabstop width

[Charles Bailey]

On Tue, Mar 04, 2008 at 09:19:43AM +0100, Jakub Narebski wrote:
> On Tue, 4 Mar 2008, Martin Langhoff wrote:
> > On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>  Some people say "Tabs are 8 characters, and thus indentations are also 8
> >>  characters.  There are heretic movements that try to make indentations 4
> >>  (or even 2!)  characters deep, and that is akin to trying to define the
> >>  value of PI to be 3."  Some people disagree.
> > 
> > And on the web, people use CSS to sort these disagreements amicably...
> > As a web apps guy, adding a setting for something like this, and then
> > changing the output feels _very_ weird, as it breaks with a lot of
> > stuff that Just Works in the HTTP+HTML world even for users that view
> > it differently... like caching :-)
> 
> The problem with using CSS to select tabstop width is twofold. First,
> it has to work correctly also in text browsers like lynx, elinks, w3m.
> Second, it is tabstop size, not the width of tab character; I'm not
> sure if it is possible to implement it in CSS.

I do understand Martin's point, it does feel a little wrong performing
a styling action in the html generation but, as Jakub says, I don't
believe there is a good CSS solution to the problem.

My patch doesn't change this issue (expanding tabs in the html
generation), it just makes it a little more flexible.