{"thread":{"id":"25272","subject":"[PATCH] Allow gitweb tab width to be set per project.","startedAt":"2010-09-28T11:35:09Z","lastAt":"2010-10-18T10:41:04Z","messageCount":8,"participants":["Magnus Hagander","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"151899","messageId":"1285673709-24924-1-git-send-email-magnus@hagander.net","threadId":"25272","inReplyTo":null,"subject":"[PATCH] Allow gitweb tab width to be set per project.","fromName":"Magnus Hagander","fromEmail":"magnus@hagander.net","sentAt":"2010-09-28T11:35:09Z","receivedAt":"2010-09-28T11:35:09Z","isPatch":true,"sender":{"key":"magnus@hagander.net","avatar":"https://gravatar.com/avatar/ff0a145db1e59aea05d26e41254a059649fb7aaf33210f1a48fc2a70f79c9583?d=mp&s=160"},"body":"Allow the gitweb.tabwidth option to control how many spaces a tab\ngets expanded to.\n\nSigned-off-by: Magnus Hagander <magnus@hagander.net>\n---\n\nIn the PostgreSQL project, we're using 4-space tabs, but we have other projects\nas well on our gitweb server, so we need to be able to control this on a\nper-project basis.\n\n\n gitweb/gitweb.perl |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a85e2f6..ef92a4f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1465,9 +1465,11 @@ sub unquote {\n # escape tabs (convert tabs to spaces)\n sub untabify {\n \tmy $line = shift;\n+\tmy $tabwidth = git_get_project_config('tabwidth', '--int');\n+\t$tabwidth = 8 if ($tabwidth <= 0);\n \n \twhile ((my $pos = index($line, \"\\t\")) != -1) {\n-\t\tif (my $count = (8 - ($pos % 8))) {\n+\t\tif (my $count = ($tabwidth - ($pos % $tabwidth))) {\n \t\t\tmy $spaces = ' ' x $count;\n \t\t\t$line =~ s/\\t/$spaces/;\n \t\t}\n-- \n1.7.0.4\n"},{"id":"151904","messageId":"m34odagioh.fsf@localhost.localdomain","threadId":"25272","inReplyTo":"1285673709-24924-1-git-send-email-magnus@hagander.net","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-28T12:25:05Z","receivedAt":"2010-09-28T12:25:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Magnus Hagander <magnus@hagander.net> writes:\n\n> Allow the gitweb.tabwidth option to control how many spaces a tab\n> gets expanded to.\n> \n> Signed-off-by: Magnus Hagander <magnus@hagander.net>\n\nThis might be a good idea, but the solution looks like it includes\nsome unnecessary performance hit (see coment below).\n\n> ---\n> \n> In the PostgreSQL project, we're using 4-space tabs, but we have other projects\n> as well on our gitweb server, so we need to be able to control this on a\n> per-project basis.\n\nSome of this comment should IMHO make it into commit message, e.g.\n\n  Different project scan use different tab widths, even on the same\n  gitweb server.\n\nOr something like that.\n\n> \n> \n>  gitweb/gitweb.perl |    4 +++-\n>  1 files changed, 3 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a85e2f6..ef92a4f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1465,9 +1465,11 @@ sub unquote {\n>  # escape tabs (convert tabs to spaces)\n>  sub untabify {\n>  \tmy $line = shift;\n> +\tmy $tabwidth = git_get_project_config('tabwidth', '--int');\n\nNote that untabify() is called once for each _line_ in a file or a\ndiff...\n\nThis has acceptable performance only because gitweb config is cached\nin %config hash by git_get_project_config() subroutine.\n\n\nI'm not sure if it wouldn't be better to have $tabwidth be passed as\nan (optional) argument to untabify(), and calculated either in calling\nsites for untabify(), or be calculated per-request and save in a\nglobal variable.\n\nWe might want to turn 'tabwidth' into a feature, though that is\nprobably overengineering.\n\n> +\t$tabwidth = 8 if ($tabwidth <= 0);\n\ngit_get_project_config('tabwidth', '--int') can return 'undef' if a\nconfiguration key does not exist, resulting in\n\n  Use of uninitialized value in numeric le (<=) at \n\nwarning in web server logs.\n\n>  \n>  \twhile ((my $pos = index($line, \"\\t\")) != -1) {\n> -\t\tif (my $count = (8 - ($pos % 8))) {\n> +\t\tif (my $count = ($tabwidth - ($pos % $tabwidth))) {\n>  \t\t\tmy $spaces = ' ' x $count;\n>  \t\t\t$line =~ s/\\t/$spaces/;\n>  \t\t}\n> -- \n> 1.7.0.4\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"152031","messageId":"AANLkTikMjVQgEzLQ5Z95cmb5fkQ5iSzqfA4T=D1zzy=j@mail.gmail.com","threadId":"25272","inReplyTo":"m34odagioh.fsf@localhost.localdomain","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Magnus Hagander","fromEmail":"magnus@hagander.net","sentAt":"2010-09-29T08:39:27Z","receivedAt":"2010-09-29T08:39:27Z","isPatch":true,"sender":{"key":"magnus@hagander.net","avatar":"https://gravatar.com/avatar/ff0a145db1e59aea05d26e41254a059649fb7aaf33210f1a48fc2a70f79c9583?d=mp&s=160"},"body":"On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:\n> Magnus Hagander <magnus@hagander.net> writes:\n>\n>> Allow the gitweb.tabwidth option to control how many spaces a tab\n>> gets expanded to.\n>>\n>> Signed-off-by: Magnus Hagander <magnus@hagander.net>\n>\n> This might be a good idea, but the solution looks like it includes\n> some unnecessary performance hit (see coment below).\n>\n>> ---\n>>\n>> In the PostgreSQL project, we're using 4-space tabs, but we have other projects\n>> as well on our gitweb server, so we need to be able to control this on a\n>> per-project basis.\n>\n> Some of this comment should IMHO make it into commit message, e.g.\n\nCheck.\n\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index a85e2f6..ef92a4f 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1465,9 +1465,11 @@ sub unquote {\n>>  # escape tabs (convert tabs to spaces)\n>>  sub untabify {\n>>       my $line = shift;\n>> +     my $tabwidth = git_get_project_config('tabwidth', '--int');\n>\n> Note that untabify() is called once for each _line_ in a file or a\n> diff...\n\nHa, that's what I get for thinking it was too easy. It actually was :-)\n\n\n> This has acceptable performance only because gitweb config is cached\n> in %config hash by git_get_project_config() subroutine.\n>\n>\n> I'm not sure if it wouldn't be better to have $tabwidth be passed as\n> an (optional) argument to untabify(), and calculated either in calling\n> sites for untabify(), or be calculated per-request and save in a\n> global variable.\n\nGiven that it's cached, will it actually make a big difference?\n\nIn order to put it on all the callsites to untabify() it needs to for\nexample go as a parameter to format_diff_line to make any difference\n(or it'd still be called once per line), so that would be quite a bit\nof pollution I think.\n\nPutting it as a global variable is certainly an option - but I didn't\nfind a pattern of something else to follow for that, so I figured it\nwas frowned upon?\n\n\n> We might want to turn 'tabwidth' into a feature, though that is\n> probably overengineering.\n\nWhat advantage(s) would it give? (I admit I'm not particularly\nfamiliar with the code - this was just the easiest way to get it done\nquickly)\n\n\n>> +     $tabwidth = 8 if ($tabwidth <= 0);\n>\n> git_get_project_config('tabwidth', '--int') can return 'undef' if a\n> configuration key does not exist, resulting in\n>\n>  Use of uninitialized value in numeric le (<=) at\n>\n> warning in web server logs.\n\nAh, I knew that would go somewhere. Interestingly enough, it doesn't\nshow up in the logs of the server I run it on now. But still should be\nfixed.\n\n-- \n Magnus Hagander\n Me: http://www.hagander.net/\n Work: http://www.redpill-linpro.com/\n"},{"id":"152035","messageId":"201009291122.01272.jnareb@gmail.com","threadId":"25272","inReplyTo":"AANLkTikMjVQgEzLQ5Z95cmb5fkQ5iSzqfA4T=D1zzy=j@mail.gmail.com","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-29T09:22:00Z","receivedAt":"2010-09-29T09:22:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 29 Sep 2010, Magnus Hagander wrote:\n> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:\n>> Magnus Hagander <magnus@hagander.net> writes:\n\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index a85e2f6..ef92a4f 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -1465,9 +1465,11 @@ sub unquote {\n>>>  # escape tabs (convert tabs to spaces)\n>>>  sub untabify {\n>>>       my $line = shift;\n>>> +     my $tabwidth = git_get_project_config('tabwidth', '--int');\n>>\n>> Note that untabify() is called once for each _line_ in a file or a\n>> diff...\n> \n> Ha, that's what I get for thinking it was too easy. It actually was :-)\n> \n> \n>> This has acceptable performance only because gitweb config is cached\n>> in %config hash by git_get_project_config() subroutine.\n>>\n>>\n>> I'm not sure if it wouldn't be better to have $tabwidth be passed as\n>> an (optional) argument to untabify(), and calculated either in calling\n>> sites for untabify(), or be calculated per-request and save in a\n>> global variable.\n> \n> Given that it's cached, will it actually make a big difference?\n\nWell, I agree that with config cached it could be left like this...\nbut I would like very much to perhaps have a comment about this, so other\npeople don't have to wonder.\n \n\n>>> +     $tabwidth = 8 if ($tabwidth <= 0);\n>>\n>> git_get_project_config('tabwidth', '--int') can return 'undef' if a\n>> configuration key does not exist, resulting in\n>>\n>>  Use of uninitialized value in numeric le (<=) at\n>>\n>> warning in web server logs.\n> \n> Ah, I knew that would go somewhere. Interestingly enough, it doesn't\n> show up in the logs of the server I run it on now. But still should be\n> fixed.\n\nWhether such warning shows in web server logs might depend on whether\nyou are running gitweb under mod_perl, or as plain CGI script.\nNevertheless it is a good practice to check if a change passess\nappropriate tests from git testsuite; t9500-gitweb-standalone-no-errors\nshould detect this.\n\nSimply use\n\n    +     $tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);\n\nor\n\n    +     $tabwidth = 8 if (!$tabwidth || $tabwidth <= 0);\n\n(though second version is more cryptic).\n\n\nP.S. If it is not a %feature, we might want to add description of\ngitweb.tabwidth to the \"Per-repository gitweb configuration\" section\nin gitweb/README (as next to last item)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"152234","messageId":"AANLkTimPte3eQMuCE3NTS=03Vv+Q2-nnu8BmXq=4YCbA@mail.gmail.com","threadId":"25272","inReplyTo":"201009291122.01272.jnareb@gmail.com","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Magnus Hagander","fromEmail":"magnus@hagander.net","sentAt":"2010-10-01T11:56:12Z","receivedAt":"2010-10-01T11:56:12Z","isPatch":true,"sender":{"key":"magnus@hagander.net","avatar":"https://gravatar.com/avatar/ff0a145db1e59aea05d26e41254a059649fb7aaf33210f1a48fc2a70f79c9583?d=mp&s=160"},"body":"On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Wed, 29 Sep 2010, Magnus Hagander wrote:\n>> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:\n>>> Magnus Hagander <magnus@hagander.net> writes:\n>\n>>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>>> index a85e2f6..ef92a4f 100755\n>>>> --- a/gitweb/gitweb.perl\n>>>> +++ b/gitweb/gitweb.perl\n>>>> @@ -1465,9 +1465,11 @@ sub unquote {\n>>>>  # escape tabs (convert tabs to spaces)\n>>>>  sub untabify {\n>>>>       my $line = shift;\n>>>> +     my $tabwidth = git_get_project_config('tabwidth', '--int');\n>>>\n>>> Note that untabify() is called once for each _line_ in a file or a\n>>> diff...\n>>\n>> Ha, that's what I get for thinking it was too easy. It actually was :-)\n>>\n>>\n>>> This has acceptable performance only because gitweb config is cached\n>>> in %config hash by git_get_project_config() subroutine.\n>>>\n>>>\n>>> I'm not sure if it wouldn't be better to have $tabwidth be passed as\n>>> an (optional) argument to untabify(), and calculated either in calling\n>>> sites for untabify(), or be calculated per-request and save in a\n>>> global variable.\n>>\n>> Given that it's cached, will it actually make a big difference?\n>\n> Well, I agree that with config cached it could be left like this...\n> but I would like very much to perhaps have a comment about this, so other\n> people don't have to wonder.\n\nCheck.\n\n>>>> +     $tabwidth = 8 if ($tabwidth <= 0);\n>>>\n>>> git_get_project_config('tabwidth', '--int') can return 'undef' if a\n>>> configuration key does not exist, resulting in\n>>>\n>>>  Use of uninitialized value in numeric le (<=) at\n>>>\n>>> warning in web server logs.\n>>\n>> Ah, I knew that would go somewhere. Interestingly enough, it doesn't\n>> show up in the logs of the server I run it on now. But still should be\n>> fixed.\n>\n> Whether such warning shows in web server logs might depend on whether\n> you are running gitweb under mod_perl, or as plain CGI script.\n> Nevertheless it is a good practice to check if a change passess\n> appropriate tests from git testsuite; t9500-gitweb-standalone-no-errors\n> should detect this.\n\nGood point. Now I just need to figure out how to be able to run the\ntests :-) I guess I should just set off a job to build the whole tree,\nand then it will just work..\n\n\n> Simply use\n>\n>    +     $tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);\n>\n> or\n>\n>    +     $tabwidth = 8 if (!$tabwidth || $tabwidth <= 0);\n>\n> (though second version is more cryptic).\n\nYeah, i definitely prefer the first one - then again, I'm not really a\nperl guy...\n\n\n\n> P.S. If it is not a %feature, we might want to add description of\n> gitweb.tabwidth to the \"Per-repository gitweb configuration\" section\n> in gitweb/README (as next to last item)\n\nOk. Will add that. Want me to send a new patch with these things included?\n\n\n-- \n Magnus Hagander\n Me: http://www.hagander.net/\n Work: http://www.redpill-linpro.com/\n"},{"id":"152265","messageId":"201010012302.47269.jnareb@gmail.com","threadId":"25272","inReplyTo":"AANLkTimPte3eQMuCE3NTS=03Vv+Q2-nnu8BmXq=4YCbA@mail.gmail.com","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-01T21:02:45Z","receivedAt":"2010-10-01T21:02:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 1 Oct 2010, Magnus Hagander wrote:\n> On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:\n>> On Wed, 29 Sep 2010, Magnus Hagander wrote:\n>>> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:\n>>>> Magnus Hagander <magnus@hagander.net> writes:\n\n>> Nevertheless it is a good practice to check if a change passess\n>> appropriate tests from git testsuite; t9500-gitweb-standalone-no-errors\n>> should detect this.\n> \n> Good point. Now I just need to figure out how to be able to run the\n> tests :-) I guess I should just set off a job to build the whole tree,\n> and then it will just work..\n \nTo test other parts of git, you need to first compile, and then run tests\n(e.g. by running \"make test\" after \"make\").  Gitweb tests check the \nsource version (for historical reason, namely that there were no gitweb\ntarget in main makefile, and gitweb didn't get compiled by default),\nso in that case you need to compile git once (to satisfy test suite),\nand then run e.g.\n\n # (cd t && ./t9500-gitweb-standalone-no-errors.sh)\n \nfrom the top directory of git repository.\n\n>> P.S. If it is not a %feature, we might want to add description of\n>> gitweb.tabwidth to the \"Per-repository gitweb configuration\" section\n>> in gitweb/README (as next to last item)\n> \n> Ok. Will add that. Want me to send a new patch with these things included?\n\nYes, please do.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"153709","messageId":"AANLkTimOxim60=gMNZu3n_AztZR7mPAOzg1NevJA2m+4@mail.gmail.com","threadId":"25272","inReplyTo":"201010012302.47269.jnareb@gmail.com","subject":"Re: [PATCH] Allow gitweb tab width to be set per project.","fromName":"Magnus Hagander","fromEmail":"magnus@hagander.net","sentAt":"2010-10-18T10:31:11Z","receivedAt":"2010-10-18T10:31:11Z","isPatch":true,"sender":{"key":"magnus@hagander.net","avatar":"https://gravatar.com/avatar/ff0a145db1e59aea05d26e41254a059649fb7aaf33210f1a48fc2a70f79c9583?d=mp&s=160"},"body":"On Fri, Oct 1, 2010 at 23:02, Jakub Narebski <jnareb@gmail.com> wrote:\n> On Fri, 1 Oct 2010, Magnus Hagander wrote:\n>> On Wed, Sep 29, 2010 at 11:22, Jakub Narebski <jnareb@gmail.com> wrote:\n>>> On Wed, 29 Sep 2010, Magnus Hagander wrote:\n>>>> On Tue, Sep 28, 2010 at 14:25, Jakub Narebski <jnareb@gmail.com> wrote:\n>>>>> Magnus Hagander <magnus@hagander.net> writes:\n>\n>>> Nevertheless it is a good practice to check if a change passess\n>>> appropriate tests from git testsuite; t9500-gitweb-standalone-no-errors\n>>> should detect this.\n>>\n>> Good point. Now I just need to figure out how to be able to run the\n>> tests :-) I guess I should just set off a job to build the whole tree,\n>> and then it will just work..\n>\n> To test other parts of git, you need to first compile, and then run tests\n> (e.g. by running \"make test\" after \"make\").  Gitweb tests check the\n> source version (for historical reason, namely that there were no gitweb\n> target in main makefile, and gitweb didn't get compiled by default),\n> so in that case you need to compile git once (to satisfy test suite),\n> and then run e.g.\n>\n>  # (cd t && ./t9500-gitweb-standalone-no-errors.sh)\n>\n> from the top directory of git repository.\n\nYeah, I got that working a while ago. Just had to build the whole thing.\n\n\n>>> P.S. If it is not a %feature, we might want to add description of\n>>> gitweb.tabwidth to the \"Per-repository gitweb configuration\" section\n>>> in gitweb/README (as next to last item)\n>>\n>> Ok. Will add that. Want me to send a new patch with these things included?\n>\n> Yes, please do.\n\nSorry, this one ended up filed under the completely wrong tag in my\nin-<real-life>-memory-bugtracker, forgot all about it. New patch\nforthcoming in a couple of minutes - just need to complete the tests\nagain.\n\n-- \n Magnus Hagander\n Me: http://www.hagander.net/\n Work: http://www.redpill-linpro.com/\n"},{"id":"153710","messageId":"1287398464-22721-1-git-send-email-magnus@hagander.net","threadId":"25272","inReplyTo":"AANLkTimOxim60=gMNZu3n_AztZR7mPAOzg1NevJA2m+4@mail.gmail.com","subject":"[PATCH] Allow gitweb tab width to be set per project.","fromName":"Magnus Hagander","fromEmail":"magnus@hagander.net","sentAt":"2010-10-18T10:41:04Z","receivedAt":"2010-10-18T10:41:04Z","isPatch":true,"sender":{"key":"magnus@hagander.net","avatar":"https://gravatar.com/avatar/ff0a145db1e59aea05d26e41254a059649fb7aaf33210f1a48fc2a70f79c9583?d=mp&s=160"},"body":"Allow the gitweb.tabwidth option to control how many spaces a tab\ngets expanded to.\n\nSigned-off-by: Magnus Hagander <magnus@hagander.net>\n---\n gitweb/README      |    4 ++++\n gitweb/gitweb.perl |    6 +++++-\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/README b/gitweb/README\nindex bf3664f..d2e4a1d 100644\n--- a/gitweb/README\n+++ b/gitweb/README\n@@ -312,6 +312,10 @@ You can use the following files in repository:\n    repository's owner. It is displayed in the project list and summary\n    page. If it's not set, filesystem directory's owner is used\n    (via GECOS field / real name field from getpwiud(3)).\n+ * gitweb.tabwidth\n+   You can use the gitweb.tabwidth repository configuration variable to set\n+   the number of spaces that tabs should be expanded to, instead of the\n+   default 8.\n  * various gitweb.* config variables (in config)\n    Read description of %feature hash for detailed list, and some\n    descriptions.\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8d7e4c5..66c258f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1478,9 +1478,13 @@ sub unquote {\n # escape tabs (convert tabs to spaces)\n sub untabify {\n \tmy $line = shift;\n+\t# git_get_project_config caches the value for us, so it's ok\n+\t# to call it once for each line.\n+\tmy $tabwidth = git_get_project_config('tabwidth', '--int');\n+\t$tabwidth = 8 if (!defined $tabwidth || $tabwidth <= 0);\n \n \twhile ((my $pos = index($line, \"\\t\")) != -1) {\n-\t\tif (my $count = (8 - ($pos % 8))) {\n+\t\tif (my $count = ($tabwidth - ($pos % $tabwidth))) {\n \t\t\tmy $spaces = ' ' x $count;\n \t\t\t$line =~ s/\\t/$spaces/;\n \t\t}\n-- \n1.7.0.4\n"}]}