Re: gitweb problem?

[Jakub Narebski <jnareb@gmail.com>]

Eli Barzilay <eli@barzilay.org> writes:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Eli Barzilay <eli@barzilay.org> writes:
>>
>>> Whenever I view the toplevel gitweb page (running as a cgi script
>>> under apache), but not when in a specific repo, I get this in my error
>>> log:
>>> 
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.
>>> fatal: error processing config file(s)
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.
[...]
>>> Looking at the source, the last two line numbers are in
>>> `git_get_project_config' -- so my guess is that the code is trying to
>>> get the options from the repository config file even when showing the
>>> toplevel page.  Based on this, and also guessing that $git_dir is
>>> unset when viewing the toplevel page, I added
>>> 
>>> 	return unless (defined $git_dir);
>>> 
>>> to the top (of the `git_get_project_config' function), and I get no
>>> warnings and everything works as it should.

BTW. the first line number is in git_cmd invoked (called) from
git_get_project_config.

[...]
> No, that's not it.  I've tracked all calls to
> `git_get_project_config', and the two offending calls are the ones in
> `feature_snapshot' and `feature_avatar'.  I then verified that
> commenting out these two lines in my config
> 
>   $feature{'snapshot'}{'override'} = 1;
>   $feature{'avatar'}{'override'} = 1;
> 
> avoids the calls -- and the comments indicate that it should be fine
> to do that.

Ah, now I have found it.  This is a bug in gitweb.

The problem is in the following fragment:

  # path to the current git repository
  our $git_dir;
  $git_dir = "$projectroot/$project" if $project;

  # list of supported snapshot formats
  our @snapshot_fmts = gitweb_get_feature('snapshot');
  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);

For the toplevel gitweb page, which is the list of projects, $project
is not defined, therefore $git_dir is neither.  gitweb_get_feature()
subroutine calls git_get_project_config() if project specific override
is turned on... but we don't have project here.

There are (at least) three possible solutions:

1. Harden gitweb_get_feature() so that it doesn't call
   git_get_project_config() if $project (and therefore $git_dir) is not
   defined; there is no project for project specific config.

2. Harden git_get_project_config() like you did in your fix, returning
   early if $git_dir is not defined.

3. Harden git_cmd() so that it doesn't add "--git-dir=$git_dir" if
   $git_dir is not defined, and change git_get_project_config() so that
   it doesn't even try to access $git_dir if it is not defined.

   	# get config
   	if (!defined $config_file ||
   	    $config_file ne "$git_dir/config") {
   		%config = git_parse_project_config('gitweb');
   		$config_file = "$git_dir/config";
   	}


The last solution, while requiring most work, has the advantage of being
able to do override via user (~apache/.gitconfig) or system (/etc/gitconfig)
gitweb config file.  But I am not sure if it is worth it...


As to why t9500 test didn't detect that: there is no test for project list
page when features are configured to be overridable...

-- 
Jakub Narebski
Poland
ShadeHawk on #git