{"thread":{"id":"12465","subject":"[PATCH] gitweb: add a setting to control the tabstop width","startedAt":"2008-03-03T22:11:59Z","lastAt":"2008-03-04T08:41:20Z","messageCount":10,"participants":["Charles Bailey","Jakub Narebski","Junio C Hamano","Martin Langhoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70843","messageId":"20080303221159.GA6875@hashpling.org","threadId":"12465","inReplyTo":null,"subject":"[PATCH] gitweb: add a setting to control the tabstop width","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-03-03T22:11:59Z","receivedAt":"2008-03-03T22:11:59Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Not everyone uses the same tab width. gitweb learns a new setting to\ncontrol the tabstop width. The configuration can be set globally and\non a per project basis. The default is 8, preserving existing\nbehaviour. The configuration variable name is borrowed from the vim\nsetting with the same behaviour.\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nThe untabify function seems the sensible place to make the change. As\nuntabify is called once per line from various different locations it\nalso makes sense to cache the result of the config lookup in a package\nvariable, though this makes the change slightly less neat.\n\nThis change should have a minimal impact on performance but it would\nappreciate some more eyes and ideally some performance testing on\nheavier systems than my own. \n\n gitweb/gitweb.perl |   29 ++++++++++++++++++++++++++++-\n 1 files changed, 28 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 922dee9..cdabe37 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -108,6 +108,12 @@ our $mimetypes_file = undef;\n # could be even 'utf-8' for the old behavior)\n our $fallback_encoding = 'latin1';\n \n+# variable to keep track of the the current tabstop width\n+# this is a package variable as the natural place to check the feature is in\n+# the untabify function, but as the function is called once per line we don't\n+# want to have to recheck the config for each line\n+our $tabstop_width;\n+\n # rename detection options for git-diff and git-diff-tree\n # - default is '-M', with the cost proportional to\n #   (number of removed files) * (number of new files).\n@@ -275,6 +281,18 @@ our %feature = (\n \t'forks' => {\n \t\t'override' => 0,\n \t\t'default' => [0]},\n+\n+\t# Tabstop width.  Controls the number of spaces to which tabs are\n+\t# expanded. Default is 8.\n+\t# To change system wide add the following to $GITWEB_CONFIG\n+\t# $feature{'tabstop'}{'default'} = [8];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'tabstop'}{'override'} = 1;\n+\t# and in project config gitweb.tabstop = <width>\n+\t'tabstop' => {\n+\t\t'sub' => \\&feature_tabstop,\n+\t\t'override' => 0,\n+\t\t'default' => [8]},\n );\n \n sub gitweb_check_feature {\n@@ -340,6 +358,11 @@ sub feature_pickaxe {\n \treturn ($_[0]);\n }\n \n+sub feature_tabstop {\n+\tmy ($val) = git_get_project_config('tabstop', '--int');\n+\treturn defined($val) ? ($val) : ($_[0])\n+}\n+\n # checking HEAD file with -e is fragile if the repository was\n # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n # and then pruned.\n@@ -832,8 +855,12 @@ sub unquote {\n sub untabify {\n \tmy $line = shift;\n \n+\tif (!defined($tabstop_width)) {\n+\t\t($tabstop_width) = gitweb_check_feature('tabstop');\n+\t}\n+\n \twhile ((my $pos = index($line, \"\\t\")) != -1) {\n-\t\tif (my $count = (8 - ($pos % 8))) {\n+\t\tif (my $count = ($tabstop_width - ($pos % $tabstop_width))) {\n \t\t\tmy $spaces = ' ' x $count;\n \t\t\t$line =~ s/\\t/$spaces/;\n \t\t}\n-- \n1.5.4.3.432.g5ecfc\n\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"},{"id":"70845","messageId":"200803032333.29426.jnareb@gmail.com","threadId":"12465","inReplyTo":"20080303221159.GA6875@hashpling.org","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-03T22:33:28Z","receivedAt":"2008-03-03T22:33:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> Not everyone uses the same tab width. gitweb learns a new setting to\n> control the tabstop width. The configuration can be set globally and\n> on a per project basis. The default is 8, preserving existing\n> behaviour. The configuration variable name is borrowed from the vim\n> setting with the same behaviour.\n\nGood idea. Very nice change.\n \n> Signed-off-by: Charles Bailey <charles@hashpling.org>\n> ---\n> \n> The untabify function seems the sensible place to make the change. As\n> untabify is called once per line from various different locations it\n> also makes sense to cache the result of the config lookup in a package\n> variable, though this makes the change slightly less neat.\n\nSince b201927 (gitweb: Read repo config using 'git config -z -l')\nrepository config is cached in %config hash (per repository), so\nI don't think global / package variable $tabstop_width is really\nneeded...\n \n> This change should have a minimal impact on performance but it would\n> appreciate some more eyes and ideally some performance testing on\n> heavier systems than my own. \n\n...but it would be better if you have checked at least on your system\nif it does affect performance or not.\n\n[...]\n\n+our $tabstop_width;\n\nI think I would write \"our $tabstop_width = 8;\" here.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"70848","messageId":"20080303225200.GA8145@hashpling.org","threadId":"12465","inReplyTo":"200803032333.29426.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-03-03T22:52:00Z","receivedAt":"2008-03-03T22:52:00Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Mar 03, 2008 at 11:33:28PM +0100, Jakub Narebski wrote:\n> Charles Bailey <charles@hashpling.org> writes:\n> > \n> > The untabify function seems the sensible place to make the change. As\n> > untabify is called once per line from various different locations it\n> > also makes sense to cache the result of the config lookup in a package\n> > variable, though this makes the change slightly less neat.\n> \n> Since b201927 (gitweb: Read repo config using 'git config -z -l')\n> repository config is cached in %config hash (per repository), so\n> I don't think global / package variable $tabstop_width is really\n> needed...\n\nFair point, although we still save the cost of some 'is the config\nvariable overrideable and if so is it overriden' logic.  Untabify is a\nonce per line call which is more frequesnt than most gitweb config\nchecking calls.\n\n> > This change should have a minimal impact on performance but it would\n> > appreciate some more eyes and ideally some performance testing on\n> > heavier systems than my own. \n> \n> ...but it would be better if you have checked at least on your system\n> if it does affect performance or not.\n> \n\nNot noticeably (on an old AMD Duron 900MHz), but my tests have been\nunscientific.\n\n> [...]\n> \n> +our $tabstop_width;\n> \n> I think I would write \"our $tabstop_width = 8;\" here.\n\nCurrently, I use the fact that it is initially 'undef' to know that I\nhaven't checked the config yet. The config is then checked on the\nfirst time through untabify.\n\nCharles.\n"},{"id":"70849","messageId":"200803032352.48614.jnareb@gmail.com","threadId":"12465","inReplyTo":"200803032333.29426.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-03T22:52:48Z","receivedAt":"2008-03-03T22:52:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\n> +our $tabstop_width;\n> \n> I think I would write \"our $tabstop_width = 8;\" here.\n\nI'm sorry. Please drop this part. Seting it to undef is needed\nif it is used as cache (as: not read in), while if we use %config\nit is simply not needed.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"70851","messageId":"7vhcfnfljw.fsf@gitster.siamese.dyndns.org","threadId":"12465","inReplyTo":"20080303221159.GA6875@hashpling.org","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-03T23:13:39Z","receivedAt":"2008-03-03T23:13:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> +\t# Tabstop width.  Controls the number of spaces to which tabs are\n> +\t# expanded. Default is 8.\n> +\t# To change system wide add the following to $GITWEB_CONFIG\n> +\t# $feature{'tabstop'}{'default'} = [8];\n> +\t# To have project specific config enable override in $GITWEB_CONFIG\n> +\t# $feature{'tabstop'}{'override'} = 1;\n> +\t# and in project config gitweb.tabstop = <width>\n> +\t'tabstop' => {\n> +\t\t'sub' => \\&feature_tabstop,\n> +\t\t'override' => 0,\n> +\t\t'default' => [8]},\n\nSome people say \"Tabs are 8 characters, and thus indentations are also 8\ncharacters.  There are heretic movements that try to make indentations 4\n(or even 2!)  characters deep, and that is akin to trying to define the\nvalue of PI to be 3.\"  Some people disagree.\n\nBut while viewing what is etched in the history, it does not hurt anybody\nelse if the viewer uses different tab width.  Choice is good.\n\nHowever, a choice made by the hosting service that runs gitweb would not\nhelp individual viewers with different tab-width taste.  Neither does\nconfiguration that is per-repository.  Participants of the same project\nwould want to view contents with different tab-width.\n\nPerhaps the tabstop \"feature\" should control _if_ the tab width of the\nmaterial gitweb feeds can be tailored at all (i.e. boolean).  And when\nenabled, it would leave the choice of non-8 tab width to the browser (the\nway to maintain per-client choice could be cookies or extra parameters, I\ndo not really care the details), and use that preferred tab-width in the\nuntabify function.\n\nOn the other hand, maybe the tab-width customization is not about user\npreference but what tab-width was used when the contents were created.  In\nsuch a case, probably the right thing to do would be to look at the\ntab-width hints embedded in the file.  In such a case, probably the tab\nwidth setting need to be per-path (e.g. *.c files may use standard 8,\nwhile *.py may use heretic 4).  Again, site-wide or repository-wide\nconfiguration would not help.\n\nIn short, I do not like the patch, not because I do not like customizable\ntab-width, but because I think the customizability the patch offers is of\nthe wrong kind and too limited to be useful.\n\nP.S.\n\nIt might be interesting to come up with a heuristics to _guess_ the tab\nwidth used by the content creator by looking at the contents, by the way.\nThere obviously are Emacs \"Local Variables\" and \"-*-\" lines and equivalent\nclues vim would leave, but you could probably also use indentation levels\nas a cue.\n\nAnd perhaps teach the underlying git commands a special flag to expand\ntabs on the output.\n\n\t\"git cat-file --expand=auto blob Makefile\"\n\t\"git diff --expand=8 HEAD^..HEAD frotz.c\"\n\n;-)\n\n\n\n"},{"id":"70858","messageId":"200803040108.58243.jnareb@gmail.com","threadId":"12465","inReplyTo":"20080303225200.GA8145@hashpling.org","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-04T00:08:57Z","receivedAt":"2008-03-04T00:08:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Charles Bailey wrote:\n> On Mon, Mar 03, 2008 at 11:33:28PM +0100, Jakub Narebski wrote:\n>> Charles Bailey <charles@hashpling.org> writes:\n>>> \n>>> The untabify function seems the sensible place to make the change. As\n>>> untabify is called once per line from various different locations it\n>>> also makes sense to cache the result of the config lookup in a package\n>>> variable, though this makes the change slightly less neat.\n>> \n>> Since b201927 (gitweb: Read repo config using 'git config -z -l')\n>> repository config is cached in %config hash (per repository), so\n>> I don't think global / package variable $tabstop_width is really\n>> needed...\n> \n> Fair point, although we still save the cost of some 'is the config\n> variable overrideable and if so is it overriden' logic.  Untabify is a\n> once per line call which is more frequesnt than most gitweb config\n> checking calls.\n\nGood enough.\n\nOne think I'd worry about is interaction with mod_perl (or FastCGI),\nnamely if $tabstop_width wouldn't get stale information.  Perhaps\nwriting\n\n  our $tabstop_width = undef;\n\nas initializer would be enough.\n-- \nJakub Narebski\nPoland\n"},{"id":"70885","messageId":"46a038f90803031935s7957a700nb5d3ba09b6575423@mail.gmail.com","threadId":"12465","inReplyTo":"7vhcfnfljw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2008-03-04T03:35:38Z","receivedAt":"2008-03-04T03:35:38Z","isPatch":true,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>  Some people say \"Tabs are 8 characters, and thus indentations are also 8\n>  characters.  There are heretic movements that try to make indentations 4\n>  (or even 2!)  characters deep, and that is akin to trying to define the\n>  value of PI to be 3.\"  Some people disagree.\n\nAnd on the web, people use CSS to sort these disagreements amicably...\nAs a web apps guy, adding a setting for something like this, and then\nchanging the output feels _very_ weird, as it breaks with a lot of\nstuff that Just Works in the HTTP+HTML world even for users that view\nit differently... like caching :-)\n\ncheers,\n\n\n\nmartin\n"},{"id":"70902","messageId":"200803040919.44925.jnareb@gmail.com","threadId":"12465","inReplyTo":"46a038f90803031935s7957a700nb5d3ba09b6575423@mail.gmail.com","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-03-04T08:19:43Z","receivedAt":"2008-03-04T08:19:43Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 4 Mar 2008, Martin Langhoff wrote:\n> On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>  Some people say \"Tabs are 8 characters, and thus indentations are also 8\n>>  characters.  There are heretic movements that try to make indentations 4\n>>  (or even 2!)  characters deep, and that is akin to trying to define the\n>>  value of PI to be 3.\"  Some people disagree.\n> \n> And on the web, people use CSS to sort these disagreements amicably...\n> As a web apps guy, adding a setting for something like this, and then\n> changing the output feels _very_ weird, as it breaks with a lot of\n> stuff that Just Works in the HTTP+HTML world even for users that view\n> it differently... like caching :-)\n\nThe problem with using CSS to select tabstop width is twofold. First,\nit has to work correctly also in text browsers like lynx, elinks, w3m.\nSecond, it is tabstop size, not the width of tab character; I'm not\nsure if it is possible to implement it in CSS.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"70907","messageId":"20080304083602.GA13585@hashpling.org","threadId":"12465","inReplyTo":"7vhcfnfljw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-03-04T08:36:02Z","receivedAt":"2008-03-04T08:36:02Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Mar 03, 2008 at 03:13:39PM -0800, Junio C Hamano wrote:\n> Some people say \"Tabs are 8 characters, and thus indentations are also 8\n> characters.  There are heretic movements that try to make indentations 4\n> (or even 2!)  characters deep, and that is akin to trying to define the\n> value of PI to be 3.\"  Some people disagree.\n> \n> But while viewing what is etched in the history, it does not hurt anybody\n> else if the viewer uses different tab width.  Choice is good.\n> \n> However, a choice made by the hosting service that runs gitweb would not\n> help individual viewers with different tab-width taste.  Neither does\n> configuration that is per-repository.  Participants of the same project\n> would want to view contents with different tab-width.\n\nI don not want to, nor shall I, argue about what interpretation of\nASCII HT is correct. I do want to argue that choice is good, and\nfurthermore that choice at the project level is appropriate.\n\nPersonally, I do not agree that participants of the same project would\nwant to view contents with different tab-width - or if they do, that\nthey shouldn't ;-) .\n\nI work on, or contribute to, a number of projects. I have a preference\nfor indentation level of source, but I try to match the conventions of\nthe project to which I am contributing. If tabs are used, and if so\nhow wide they are, are important project conventions.\n\nFor some projects they are mandatory standards.\n\nFor some sets of conventions the width of a tab is not important, but\nfor many they are.\n\nYou can have the convention that tabs shall not be used, but you\ncannot have a convention that tabs shall or can be used in conjunction\nwith a convention about maximum line length without and agreement on\ntab-width.\n\ngit-mergetool.sh exhibits an example. If you look at the file with\ntabs set to 4 then the gvimdiff 'case' in the merge_file shell\nfunction *looks* correctly indented, but in other cases, the contents\nof the if statements do not look indented when they should be.\n\nIf you look at git-mergetool.sh with tabs set to 8, then the other\ncases look correctly indented, but the gvimdiff case is over-indented.\n\nClearly, what happened is that a user edited a file that should have\nbeen viewed with ts=8 as ts=4 and the result is now 'wrong' in\ndifferent ways in different places.\n\n[ Aside: this particular inconsistency is fixed in pu :-) ]\n\n> On the other hand, maybe the tab-width customization is not about user\n> preference but what tab-width was used when the contents were created.  In\n> such a case, probably the right thing to do would be to look at the\n> tab-width hints embedded in the file.  In such a case, probably the tab\n> width setting need to be per-path (e.g. *.c files may use standard 8,\n> while *.py may use heretic 4).  Again, site-wide or repository-wide\n> configuration would not help.\n\nThis, I certainly agree with more. Tab width is about choice at\ncontent creation. My argument is that choice at content creation\nshould follow project-wide conventions. I agree that my patch doesn't\nprovide a totally flexible solution but I do believe that there are\nprojects that either use one type of source file - or at least only one\ntab width - which would benefit from the patch.\n\nI run a gitweb.cgi which has a handful of projects, one of which uses\na popular IDE on a popular proprietary OS and uses a ts=4 convention\nand another which uses ts=8 convention. The patch was driven by this\nrequirement and a (possibly erroneous) feeling that I probably wasn't\nalone.\n\n> In short, I do not like the patch, not because I do not like customizable\n> tab-width, but because I think the customizability the patch offers is of\n> the wrong kind and too limited to be useful.\n\nWhereas I believe it provides some useful customization, the\ncustomization is appropriate in many situations and the costs\n(performance and code complexity) are minimal. Obviously, integration\nis your call, but the patch will certainly stay on my own http server.\n\n> P.S.\n> \n> It might be interesting to come up with a heuristics to _guess_ the tab\n> width used by the content creator by looking at the contents, by the way.\n> There obviously are Emacs \"Local Variables\" and \"-*-\" lines and equivalent\n> clues vim would leave, but you could probably also use indentation levels\n> as a cue.\n\nPerhaps. I don't really care for -*- style fluff but that's just\npersonal. The heuristics could be tricky to get right, probably\nimpossible in the case of users with tab conventions mangling the same\nfile :-) .\n\n> And perhaps teach the underlying git commands a special flag to expand\n> tabs on the output.\n> \n> \t\"git cat-file --expand=auto blob Makefile\"\n> \t\"git diff --expand=8 HEAD^..HEAD frotz.c\"\n> \n> ;-)\n> \n\nAnd I'd argue that expanding tabs was the job of the terminal and not\ncore git, but hey :-) !\n\nCharles.\n"},{"id":"70909","messageId":"20080304084120.GB13585@hashpling.org","threadId":"12465","inReplyTo":"200803040919.44925.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: add a setting to control the tabstop width","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-03-04T08:41:20Z","receivedAt":"2008-03-04T08:41:20Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Tue, Mar 04, 2008 at 09:19:43AM +0100, Jakub Narebski wrote:\n> On Tue, 4 Mar 2008, Martin Langhoff wrote:\n> > On Tue, Mar 4, 2008 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >>  Some people say \"Tabs are 8 characters, and thus indentations are also 8\n> >>  characters.  There are heretic movements that try to make indentations 4\n> >>  (or even 2!)  characters deep, and that is akin to trying to define the\n> >>  value of PI to be 3.\"  Some people disagree.\n> > \n> > And on the web, people use CSS to sort these disagreements amicably...\n> > As a web apps guy, adding a setting for something like this, and then\n> > changing the output feels _very_ weird, as it breaks with a lot of\n> > stuff that Just Works in the HTTP+HTML world even for users that view\n> > it differently... like caching :-)\n> \n> The problem with using CSS to select tabstop width is twofold. First,\n> it has to work correctly also in text browsers like lynx, elinks, w3m.\n> Second, it is tabstop size, not the width of tab character; I'm not\n> sure if it is possible to implement it in CSS.\n\nI do understand Martin's point, it does feel a little wrong performing\na styling action in the html generation but, as Jakub says, I don't\nbelieve there is a good CSS solution to the problem.\n\nMy patch doesn't change this issue (expanding tabs in the html\ngeneration), it just makes it a little more flexible.\n"}]}