{"thread":{"id":"22861","subject":"gitweb problem?","startedAt":"2010-03-01T06:54:29Z","lastAt":"2010-03-02T04:40:08Z","messageCount":6,"participants":["Eli Barzilay","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"135924","messageId":"m34ol0wmze.fsf@winooski.ccs.neu.edu","threadId":"22861","inReplyTo":null,"subject":"gitweb problem?","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2010-03-01T06:54:29Z","receivedAt":"2010-03-01T06:54:29Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"Whenever I view the toplevel gitweb page (running as a cgi script\nunder apache), but not when in a specific repo, I get this in my error\nlog:\n\ngitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.\nfatal: error processing config file(s)\ngitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.\ngitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.\n\n(taken verbatim from the apache error log, removed uninteresting line\nprefixes.)\n\nI'm using the pathinfo option, so perhaps there is a problem with that\nsetup?\n\nLooking at the source, the last two line numbers are in\n`git_get_project_config' -- so my guess is that the code is trying to\nget the options from the repository config file even when showing the\ntoplevel page.  Based on this, and also guessing that $git_dir is\nunset when viewing the toplevel page, I added\n\n\treturn unless (defined $git_dir);\n\nto the top (of the `git_get_project_config' function), and I get no\nwarnings and everything works as it should.\n\n(Disclaimer: I can barely read perl, and I'm a git newbie, so all of\nthis can be due to some other stupid mistake.)\n\n-- \n          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:\n                    http://barzilay.org/                   Maze is Life!\n"},{"id":"135936","messageId":"m3bpf8mj5k.fsf@localhost.localdomain","threadId":"22861","inReplyTo":"m34ol0wmze.fsf@winooski.ccs.neu.edu","subject":"Re: gitweb problem?","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-03-01T10:24:13Z","receivedAt":"2010-03-01T10:24:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Eli Barzilay <eli@barzilay.org> writes:\n\n> Whenever I view the toplevel gitweb page (running as a cgi script\n> under apache), but not when in a specific repo, I get this in my error\n> log:\n> \n> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.\n> fatal: error processing config file(s)\n> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.\n> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.\n> \n> (taken verbatim from the apache error log, removed uninteresting line\n> prefixes.)\n\nIn the future (or if my diagnosis would turn out to be incorrect)\ncould you please show which lines are those (in *your* gitweb.cgi),\nor at least provide gitweb version?  Changes to gitweb can render\nsuch line numbers invalid.\n \n> I'm using the pathinfo option, so perhaps there is a problem with that\n> setup?\n\nIn the future (or if my diagnosis would turn out to be incorrect) could\nyou please include relevant parts of $GITWEB_CONFIG (by default it is\ngitweb_config.perl), at least which features you have enabled, and how\nthey are configured?\n\n> \n> Looking at the source, the last two line numbers are in\n> `git_get_project_config' -- so my guess is that the code is trying to\n> get the options from the repository config file even when showing the\n> toplevel page.  Based on this, and also guessing that $git_dir is\n> unset when viewing the toplevel page, I added\n> \n> \treturn unless (defined $git_dir);\n> \n> to the top (of the `git_get_project_config' function), and I get no\n> warnings and everything works as it should.\n> \n> (Disclaimer: I can barely read perl, and I'm a git newbie, so all of\n> this can be due to some other stupid mistake.)\n\nThat looks like lack of hardening against pilot error.  The\ngit_get_project_config should never be run when $git_dir is not set, as it\nis meant to access *project* config.  \n\nBut at the top of git_project_list_body subroutine, which is responsible for\ngenerating toplevel page with list of projects, we have:\n\tmy $check_forks = gitweb_check_feature('forks');\nand a bit later\n\tmy $show_ctags = gitweb_check_feature('ctags');\n\nNow both of those features are marked as not supporting project specific\noverride.  It might be that you by accident set $feature{XXX}{'override'}\nto true... but I might be mistaken.\n\n\nIt would be strange that this bug was not detected by t9500 test...\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"135942","messageId":"m3zl2suun5.fsf@winooski.ccs.neu.edu","threadId":"22861","inReplyTo":"m3bpf8mj5k.fsf@localhost.localdomain","subject":"Re: gitweb problem?","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2010-03-01T11:51:58Z","receivedAt":"2010-03-01T11:51:58Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Eli Barzilay <eli@barzilay.org> writes:\n>\n>> Whenever I view the toplevel gitweb page (running as a cgi script\n>> under apache), but not when in a specific repo, I get this in my error\n>> log:\n>> \n>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.\n>> fatal: error processing config file(s)\n>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.\n>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.\n>> \n>> (taken verbatim from the apache error log, removed uninteresting line\n>> prefixes.)\n>\n> In the future (or if my diagnosis would turn out to be incorrect)\n> could you please show which lines are those (in *your* gitweb.cgi),\n> or at least provide gitweb version?  Changes to gitweb can render\n> such line numbers invalid.\n\nSorry, I should have mentioned that this is from 1.7.0 -- my file is\nnot modified, and built with no changes to the defaults except for\nprefix=/usr/local.  In any case, the script -- with the test that I\nadded -- is at http://tmp.barzilay.org/x1 .\n\n\n>> I'm using the pathinfo option, so perhaps there is a problem with\n>> that setup?\n>\n> In the future (or if my diagnosis would turn out to be incorrect)\n> could you please include relevant parts of $GITWEB_CONFIG (by\n> default it is gitweb_config.perl), at least which features you have\n> enabled, and how they are configured?\n\nhttp://tmp.barzilay.org/x2 (but see below).\n\n\n>> Looking at the source, the last two line numbers are in\n>> `git_get_project_config' -- so my guess is that the code is trying to\n>> get the options from the repository config file even when showing the\n>> toplevel page.  Based on this, and also guessing that $git_dir is\n>> unset when viewing the toplevel page, I added\n>> \n>> \treturn unless (defined $git_dir);\n>> \n>> to the top (of the `git_get_project_config' function), and I get no\n>> warnings and everything works as it should.\n>> \n>> (Disclaimer: I can barely read perl, and I'm a git newbie, so all of\n>> this can be due to some other stupid mistake.)\n>\n> That looks like lack of hardening against pilot error.  The\n> git_get_project_config should never be run when $git_dir is not set,\n> as it is meant to access *project* config.\n>\n> But at the top of git_project_list_body subroutine, which is\n> responsible for generating toplevel page with list of projects, we\n> have:\n> \tmy $check_forks = gitweb_check_feature('forks');\n> and a bit later\n> \tmy $show_ctags = gitweb_check_feature('ctags');\n>\n> Now both of those features are marked as not supporting project specific\n> override.  It might be that you by accident set $feature{XXX}{'override'}\n> to true... but I might be mistaken.\n\nNo, that's not it.  I've tracked all calls to\n`git_get_project_config', and the two offending calls are the ones in\n`feature_snapshot' and `feature_avatar'.  I then verified that\ncommenting out these two lines in my config\n\n  $feature{'snapshot'}{'override'} = 1;\n  $feature{'avatar'}{'override'} = 1;\n\navoids the calls -- and the comments indicate that it should be fine\nto do that.\n\n-- \n          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:\n                    http://barzilay.org/                   Maze is Life!\n"},{"id":"135944","messageId":"m37hpwma9u.fsf@localhost.localdomain","threadId":"22861","inReplyTo":"m3zl2suun5.fsf@winooski.ccs.neu.edu","subject":"Re: gitweb problem?","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-03-01T13:40:06Z","receivedAt":"2010-03-01T13:40:06Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Eli Barzilay <eli@barzilay.org> writes:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>> Eli Barzilay <eli@barzilay.org> writes:\n>>\n>>> Whenever I view the toplevel gitweb page (running as a cgi script\n>>> under apache), but not when in a specific repo, I get this in my error\n>>> log:\n>>> \n>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.\n>>> fatal: error processing config file(s)\n>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.\n>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.\n[...]\n>>> Looking at the source, the last two line numbers are in\n>>> `git_get_project_config' -- so my guess is that the code is trying to\n>>> get the options from the repository config file even when showing the\n>>> toplevel page.  Based on this, and also guessing that $git_dir is\n>>> unset when viewing the toplevel page, I added\n>>> \n>>> \treturn unless (defined $git_dir);\n>>> \n>>> to the top (of the `git_get_project_config' function), and I get no\n>>> warnings and everything works as it should.\n\nBTW. the first line number is in git_cmd invoked (called) from\ngit_get_project_config.\n\n[...]\n> No, that's not it.  I've tracked all calls to\n> `git_get_project_config', and the two offending calls are the ones in\n> `feature_snapshot' and `feature_avatar'.  I then verified that\n> commenting out these two lines in my config\n> \n>   $feature{'snapshot'}{'override'} = 1;\n>   $feature{'avatar'}{'override'} = 1;\n> \n> avoids the calls -- and the comments indicate that it should be fine\n> to do that.\n\nAh, now I have found it.  This is a bug in gitweb.\n\nThe problem is in the following fragment:\n\n  # path to the current git repository\n  our $git_dir;\n  $git_dir = \"$projectroot/$project\" if $project;\n\n  # list of supported snapshot formats\n  our @snapshot_fmts = gitweb_get_feature('snapshot');\n  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n\nFor the toplevel gitweb page, which is the list of projects, $project\nis not defined, therefore $git_dir is neither.  gitweb_get_feature()\nsubroutine calls git_get_project_config() if project specific override\nis turned on... but we don't have project here.\n\nThere are (at least) three possible solutions:\n\n1. Harden gitweb_get_feature() so that it doesn't call\n   git_get_project_config() if $project (and therefore $git_dir) is not\n   defined; there is no project for project specific config.\n\n2. Harden git_get_project_config() like you did in your fix, returning\n   early if $git_dir is not defined.\n\n3. Harden git_cmd() so that it doesn't add \"--git-dir=$git_dir\" if\n   $git_dir is not defined, and change git_get_project_config() so that\n   it doesn't even try to access $git_dir if it is not defined.\n\n   \t# get config\n   \tif (!defined $config_file ||\n   \t    $config_file ne \"$git_dir/config\") {\n   \t\t%config = git_parse_project_config('gitweb');\n   \t\t$config_file = \"$git_dir/config\";\n   \t}\n\n\nThe last solution, while requiring most work, has the advantage of being\nable to do override via user (~apache/.gitconfig) or system (/etc/gitconfig)\ngitweb config file.  But I am not sure if it is worth it...\n\n\nAs to why t9500 test didn't detect that: there is no test for project list\npage when features are configured to be overridable...\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"135984","messageId":"20100301215040.32574.8149.stgit@localhost.localdomain","threadId":"22861","inReplyTo":"m37hpwma9u.fsf@localhost.localdomain","subject":"[PATCH] gitweb: Fix project-specific feature override behavior","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-03-01T21:51:34Z","receivedAt":"2010-03-01T21:51:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This commit fixes a bug in processing project-specific override in\na situation when there is no project, e.g. for the projects list page.\n\nWhen 'snapshot' feature had project specific config override enabled\nby putting\n  $feature{'snapshot'}{'override'} = 1;\n\n(or equivalent) in $GITWEB_CONFIG, and when viewing toplevel gitweb\npage, which means the projects list page (to be more exact this\nhappens for any project-less action), gitweb would put the following\nPerl warnings in error log:\n\n  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2065.\n  fatal: error processing config file(s)\n  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2221.\n  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2218.\n\n\nThe problem is in the following fragment of code:\n\n  # path to the current git repository\n  our $git_dir;\n  $git_dir = \"$projectroot/$project\" if $project;\n\n  # list of supported snapshot formats\n  our @snapshot_fmts = gitweb_get_feature('snapshot');\n  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n\nFor the toplevel gitweb page, which is the list of projects, $project is not\ndefined, therefore neither is $git_dir.  gitweb_get_feature() subroutine\ncalls git_get_project_config() if project specific override is turned\non... but we don't have project here.\n\n\nThose errors mentioned above occur in the following fragment of code in\ngit_get_project_config():\n\n  \t# get config\n  \tif (!defined $config_file ||\n  \t    $config_file ne \"$git_dir/config\") {\n  \t\t%config = git_parse_project_config('gitweb');\n  \t\t$config_file = \"$git_dir/config\";\n  \t}\n\ngit_parse_project_config() calls git_cmd() which has '--git-dir='.$git_dir\n\n\nThere are (at least) three possible solutions:\n1. Harden gitweb_get_feature() so that it doesn't call\n   git_get_project_config() if $project (and therefore $git_dir) is not\n   defined; there is no project for project specific config.\n2. Harden git_get_project_config() like you did in your fix, returning early\n   if $git_dir is not defined.\n3. Harden git_cmd() so that it doesn't add \"--git-dir=$git_dir\" if $git_dir\n   is not defined, and change git_get_project_config() so that it doesn't\n   even try to access $git_dir if it is not defined.\n\nThis commit implements both 1.) and 2.), i.e. gitweb_get_feature() doesn't\ncall project-specific override if $git_dir is not defined (if there is no\nproject), and git_get_project_config() returns early if $git_dir is not\ndefined.\n\n\nAdd a test for this bug to t/t9500-gitweb-standalone-no-errors.sh test.\n\nReported-by: Eli Barzilay <eli@barzilay.org>\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is a fix, with much too long commit message ;-)\n\n gitweb/gitweb.perl                     |    9 ++++++++-\n t/t9500-gitweb-standalone-no-errors.sh |   16 ++++++++++++----\n 2 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1ce4de1..d02734e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -467,7 +467,11 @@ sub gitweb_get_feature {\n \t\t$feature{$name}{'sub'},\n \t\t$feature{$name}{'override'},\n \t\t@{$feature{$name}{'default'}});\n-\tif (!$override) { return @defaults; }\n+\t# project specific override is possible only if we have project\n+\tour $git_dir; # global variable, declared later\n+\tif (!$override || !defined $git_dir) {\n+\t\treturn @defaults;\n+\t}\n \tif (!defined $sub) {\n \t\twarn \"feature $name is not overridable\";\n \t\treturn @defaults;\n@@ -2225,6 +2229,9 @@ sub config_to_multi {\n sub git_get_project_config {\n \tmy ($key, $type) = @_;\n \n+\t# do we have project\n+\treturn unless (defined $project && defined $git_dir);\n+\n \t# key sanity check\n \treturn unless ($key);\n \t$key =~ s/^gitweb\\.//;\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex 0fe9beb..4f2b9b0 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -591,14 +591,22 @@ test_debug 'cat gitweb.log'\n # ----------------------------------------------------------------------\n # gitweb config and repo config\n \n-cat >>gitweb_config.perl <<EOF\n+cat >>gitweb_config.perl <<\\EOF\n \n-\\$feature{'blame'}{'override'} = 1;\n-\\$feature{'snapshot'}{'override'} = 1;\n-\\$feature{'avatar'}{'override'} = 1;\n+# turn on override for each overridable feature\n+foreach my $key (keys %feature) {\n+\tif ($feature{$key}{'sub'}) {\n+\t\t$feature{$key}{'override'} = 1;\n+\t}\n+}\n EOF\n \n test_expect_success \\\n+\t'config override: projects list (implicit)' \\\n+\t'gitweb_run'\n+test_debug 'cat gitweb.log'\n+\n+test_expect_success \\\n \t'config override: tree view, features not overridden in repo config' \\\n \t'gitweb_run \"p=.git;a=tree\"'\n test_debug 'cat gitweb.log'\n"},{"id":"135998","messageId":"m3vddfuyjb.fsf@winooski.ccs.neu.edu","threadId":"22861","inReplyTo":"m37hpwma9u.fsf@localhost.localdomain","subject":"Re: gitweb problem?","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2010-03-02T04:40:08Z","receivedAt":"2010-03-02T04:40:08Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> BTW. the first line number is in git_cmd invoked (called) from\n> git_get_project_config.\n\n(Oh yeah, I forgot about that one, sorry.)\n\n> [...]\n>\n> The last solution, while requiring most work, has the advantage of\n> being able to do override via user (~apache/.gitconfig) or system\n> (/etc/gitconfig) gitweb config file.  But I am not sure if it is\n> worth it...\n\nThanks for the fix!  As for the point above, isn't the whole point of\nthe feature system to do this kind of toplevel configuration possible?\n(If so, it sounds like a concrete explanation for why it's not worth\nthe effort: it only makes it possible to specify the defaults in a\ntoplevel .gitconfig file, which could be nice but probably confusing.)\n\n-- \n          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:\n                    http://barzilay.org/                   Maze is Life!\n"}]}