Potential bug in gitweb 1.7.2

Potential bug in gitweb 1.7.2

[Julio Lajara]

Hi all, I recently had git/gitweb (1.7.0.4) setup on my Ubuntu 10.04 box 
and decided to add ppa:git-core/ppa to get up to 1.7.2. After I did so, 
my previously working gitweb stopped functioning and only shows a blank 
page. Im using apache/cgi and the following error is what I found in my 
apache error log:

[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call 
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 
3401., referer: http://192.168.1.142/gitweb/


 From the latest source:
cat -n gitweb.perl | grep 3401
   3401        if (defined $cgi->http('HTTP_ACCEPT') &&

Thanks,

Re: Potential bug in gitweb 1.7.2

[Jonathan Nieder]

Hi Julio,

Julio Lajara wrote:

> Hi all, I recently had git/gitweb (1.7.0.4) setup on my Ubuntu 10.04
> box and decided to add ppa:git-core/ppa to get up to 1.7.2. After I
> did so, my previously working gitweb stopped functioning

Indeed, the rules for installation have changed with v1.7.2, so
the packaging may need to be adjusted[1] to match (see
gitweb/INSTALL).  Please report this to the Ubuntu git team.

(I am not sure what the proper way to do that is.
https://bugs.launchpad.net/ubuntu/+source/git/+filebug will at
least reach the right people.)

Hope that helps,
Jonathan

[1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5

Re: Potential bug in gitweb 1.7.2

[Anders Kaseorg]

On Mon, 26 Jul 2010, Jonathan Nieder wrote:
> Indeed, the rules for installation have changed with v1.7.2, so the 
> packaging may need to be adjusted[1] to match (see gitweb/INSTALL).  
> Please report this to the Ubuntu git team.
> 
> [1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5

Thanks for the report.  But, the 1.7.2-0ppa1 package in the git-core PPA 
was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1, 
commit f84e1eb) with upstream v1.7.2, and updating 
debian/versions.upstream and debian/changelog.upstream.  It already 
includes that commit.

Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some 
repositories into /var/cache/git, and go to http://localhost/gitweb/ , it 
works correctly for me.

The error that Julio originally posted:

> > [Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call 
> > method "http" on an undefined value at /usr/share/gitweb/index.cgi 
> > line 3401., referer: http://192.168.1.142/gitweb/

looks very strange, and does not seem like it would result from just a 
packaging problem.

Anders

Re: Potential bug in gitweb 1.7.2

[Julio Lajara]

Hi Anders, thanks for the input. I will begin chasing down the issue in 
my config then, but its strange that it would stop working as the exact 
same configuration works without issue in 1.7.0.4 . Ill follow up with 
what I find.

Thanks,

On 07/26/2010 05:27 AM, Anders Kaseorg wrote:
> On Mon, 26 Jul 2010, Jonathan Nieder wrote:
>    
>> Indeed, the rules for installation have changed with v1.7.2, so the
>> packaging may need to be adjusted[1] to match (see gitweb/INSTALL).
>> Please report this to the Ubuntu git team.
>>
>> [1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5
>>      
> Thanks for the report.  But, the 1.7.2-0ppa1 package in the git-core PPA
> was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1,
> commit f84e1eb) with upstream v1.7.2, and updating
> debian/versions.upstream and debian/changelog.upstream.  It already
> includes that commit.
>
> Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some
> repositories into /var/cache/git, and go to http://localhost/gitweb/ , it
> works correctly for me.
>
> The error that Julio originally posted:
>
>    
>>> [Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
>>> method "http" on an undefined value at /usr/share/gitweb/index.cgi
>>> line 3401., referer: http://192.168.1.142/gitweb/
>>>        
> looks very strange, and does not seem like it would result from just a
> packaging problem.
>
> Anders
>
>    

Re: Potential bug in gitweb 1.7.2

[Jakub Narebski]

On Mon, 26 Jul 2010, Julio Lajara wrote:

> Hi Anders, thanks for the input. I will begin chasing down the issue in 
> my config then, but its strange that it would stop working as the exact 
> same configuration works without issue in 1.7.0.4 . Ill follow up with 
> what I find.
> 
> On 07/26/2010 05:27 AM, Anders Kaseorg wrote:

>> The error that Julio originally posted:
>>
>>    
>>>> [Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
>>>> method "http" on an undefined value at /usr/share/gitweb/index.cgi
>>>> line 3401., referer: http://192.168.1.142/gitweb/
>>        
>> looks very strange, and does not seem like it would result from just a
>> packaging problem.

Julio, could you show us the neigbourhood of line 3401 in your index.cgi
file?  What is this index.cgi file - git doesn't have it?

-- 
Jakub Narebski
Poland

Re: Potential bug in gitweb 1.7.2

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 26, 2010 at 13:36, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jul 2010, Julio Lajara wrote:
>
>> Hi Anders, thanks for the input. I will begin chasing down the issue in
>> my config then, but its strange that it would stop working as the exact
>> same configuration works without issue in 1.7.0.4 . Ill follow up with
>> what I find.
>>
>> On 07/26/2010 05:27 AM, Anders Kaseorg wrote:
>
>>> The error that Julio originally posted:
>>>
>>>
>>>>> [Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
>>>>> method "http" on an undefined value at /usr/share/gitweb/index.cgi
>>>>> line 3401., referer: http://192.168.1.142/gitweb/
>>>
>>> looks very strange, and does not seem like it would result from just a
>>> packaging problem.
>
> Julio, could you show us the neigbourhood of line 3401 in your index.cgi
> file?  What is this index.cgi file - git doesn't have it?

His index.cgi is just gitweb.cgi under a different name. Line 3401 is
the same as gitweb/gitweb.cgi in the 1.7.2 release.

Re: Potential bug in gitweb 1.7.2

[Julio Lajara]

Hi, Ive managed to narrow down the issue to a line in the gitolite's 
contrib/gitweb/gitweb.conf that I appended to the default 
/etc/gitweb.conf . It was working with 1.7.0.4 but 1.7.2 doesnt like 
this line:

$ENV{GL_USER} = $cgi->remote_user || "gitweb";

I corrected it by just setting it equal to gitweb and all is well now.

Thanks,

On 07/26/2010 09:39 AM, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jul 26, 2010 at 13:36, Jakub Narebski<jnareb@gmail.com>  wrote:
>    
>> On Mon, 26 Jul 2010, Julio Lajara wrote:
>>
>>      
>>> Hi Anders, thanks for the input. I will begin chasing down the issue in
>>> my config then, but its strange that it would stop working as the exact
>>> same configuration works without issue in 1.7.0.4 . Ill follow up with
>>> what I find.
>>>
>>> On 07/26/2010 05:27 AM, Anders Kaseorg wrote:
>>>        
>>      
>>>> The error that Julio originally posted:
>>>>
>>>>
>>>>          
>>>>>> [Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
>>>>>> method "http" on an undefined value at /usr/share/gitweb/index.cgi
>>>>>> line 3401., referer: http://192.168.1.142/gitweb/
>>>>>>              
>>>> looks very strange, and does not seem like it would result from just a
>>>> packaging problem.
>>>>          
>> Julio, could you show us the neigbourhood of line 3401 in your index.cgi
>> file?  What is this index.cgi file - git doesn't have it?
>>      
> His index.cgi is just gitweb.cgi under a different name. Line 3401 is
> the same as gitweb/gitweb.cgi in the 1.7.2 release.
>    

Re: Potential bug in gitweb 1.7.2

[Jakub Narebski]

On Mon, 26 Jul 2010, Julio Lajara wrote:

> Hi, Ive managed to narrow down the issue to a line in the gitolite's 
> contrib/gitweb/gitweb.conf that I appended to the default 
> /etc/gitweb.conf . It was working with 1.7.0.4 but 1.7.2 doesnt like 
> this line:
> 
> $ENV{GL_USER} = $cgi->remote_user || "gitweb";
> 
> I corrected it by just setting it equal to gitweb and all is well now.

I think the following commit is the reason that this stopped working
869d588 (gitweb: Move evaluate_gitweb_config out of run_request, 2010-07-05)

>From its commit message:

    gitweb: Move evaluate_gitweb_config out of run_request
    
    Move evaluate_gitweb_config() and evaluate_git_version() out of
    run_request() to run(), making them not run one for each request.
    This changes how git behaves in FastCGI case.
    
    This change makes it impossible to have config which changes with
    request, but I don't think anyone relied on such (hidden action)
    behavior.

I guess I was wrong about that.  I wonder if it would be possible to 
re-enable this feature (which I think is needed to be able to use
$cgi->remote_user) but without having all pay the [slight] performance
penalty of including (and I think parsing) config file once per each
request.

-- 
Jakub Narebski
Poland

Re: Potential bug in gitweb 1.7.2

[Jonathan Nieder]

Hi Anders,

Anders Kaseorg wrote:

> the 1.7.2-0ppa1 package in the git-core PPA 
> was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1, 
> commit f84e1eb) with upstream v1.7.2, and updating 
> debian/versions.upstream and debian/changelog.upstream.  It already 
> includes that commit.

Good to know.  Is there a public git repository for the PPA?  (I am
asking selfishly, because it would be convenient to be able to tell
what commit each version corresponds to and to steal changes
back.)

> Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some 
> repositories into /var/cache/git, and go to http://localhost/gitweb/ , it 
> works correctly for me.

Hm, I should have tried that myself.

Thanks.

Re: Potential bug in gitweb 1.7.2

[Anders Kaseorg]

On Mon, 26 Jul 2010, Jonathan Nieder wrote:
> Good to know.  Is there a public git repository for the PPA?  (I am 
> asking selfishly, because it would be convenient to be able to tell what 
> commit each version corresponds to and to steal changes back.)

There is now:
  git://andersk.mit.edu/git.git
  http://andersk.mit.edu/gitweb/git.git
(The most recent version is on the ppa branch, but there are tags going 
back through 1.7.0.1-1~ppa1.)

Anders

Re: Potential bug in gitweb 1.7.2

[Jonathan Nieder]

Anders Kaseorg wrote:

>   git://andersk.mit.edu/git.git

Thanks!

FWIW (because this does not seem to have been advertised widely)
there's a mailing list to coordinate work on the Debian git packages.

Anyone interested can send mail (with any content) to
<debian-package-git-help@list.smarden.org> for a list of supported
commands or <debian-package-git-subscribe@list.smarden.org> to
subscribe.

[PATCH/RFC] gitweb: allow configurations that change with each request

[Jonathan Nieder]

gitolite's contrib/gitweb/gitweb.conf includes:

	$ENV{GL_USER} = $cgi->remote_user || "gitweb";

which is useful for setups where a user has to be authenticated
to access certain repos.  Perhaps other typical configurations
change per session in other ways, too.

v1.7.2-rc2~6 (gitweb: Move evaluate_gitweb_config out of run_request,
2010-07-05) broke such configurations for a speedup, by loading
the configuration once per FastCGI process.

Probably in the end there should be a way to specify in the
configuration whether a particular installation wants the speedup or
the flexibility.  But for now it is easier to just undo the relevant
change.

This partially reverts commit 869d58813b24c74e84c9388041eafcef40cb51e4.

Reported-by: Julio Lajara <julio.lajara@alum.rpi.edu>
Analysis-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jakub Narebski wrote:

> I wonder if it would be possible to 
> re-enable this feature (which I think is needed to be able to use
> $cgi->remote_user) but without having all pay the [slight] performance
> penalty of including (and I think parsing) config file once per each
> request.

I dunno.  Maybe this would be a good place to start.

 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e0e9532..300c4b1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1060,8 +1060,12 @@ sub run_request {
 	reset_timer();
 
 	evaluate_uri();
+	evaluate_gitweb_config();
 	check_loadavg();
 
+	# $projectroot and $projects_list might be set in gitweb config file
+	$projects_list ||= $projectroot;
+
 	evaluate_query_params();
 	evaluate_path_info();
 	evaluate_and_validate_params();
@@ -1109,12 +1113,8 @@ sub evaluate_argv {
 
 sub run {
 	evaluate_argv();
-	evaluate_gitweb_config();
 	evaluate_git_version();
 
-	# $projectroot and $projects_list might be set in gitweb config file
-	$projects_list ||= $projectroot;
-
 	$pre_listen_hook->()
 		if $pre_listen_hook;
 
-- 
1.7.2.1.544.ga752d.dirty

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

[Jakub Narebski]

On Sat, 31 Jul 2010, Jonathan Nieder wrote:

> gitolite's contrib/gitweb/gitweb.conf includes:
> 
> 	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
> 
> which is useful for setups where a user has to be authenticated
> to access certain repos.  Perhaps other typical configurations
> change per session in other ways, too.
> 
> v1.7.2-rc2~6 (gitweb: Move evaluate_gitweb_config out of run_request,
> 2010-07-05) broke such configurations for a speedup, by loading
> the configuration once per FastCGI process.
> 
> Probably in the end there should be a way to specify in the
> configuration whether a particular installation wants the speedup or
> the flexibility.  But for now it is easier to just undo the relevant
> change.
> 
> This partially reverts commit 869d58813b24c74e84c9388041eafcef40cb51e4.

Why only *partially* reverts...

...ah, I see, I did "while at it" change fixing timer and number of git
commands info for persistent environments (i.e. gitweb run as FastCGI
script).
 
> Reported-by: Julio Lajara <julio.lajara@alum.rpi.edu>
> Analysis-by: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Reluctantly-acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
> Jakub Narebski wrote:
> 
> > I wonder if it would be possible to 
> > re-enable this feature (which I think is needed to be able to use
> > $cgi->remote_user) but without having all pay the [slight] performance
> > penalty of including (and I think parsing) config file once per each
> > request.
> 
> I dunno.  Maybe this would be a good place to start.

Currently this is non-issue; the eventual performance penalty is I guess
very small.

The problem would be with adding caching support to gitweb.  While
default GitwebCache::* caching engine should have low startup cost, 
the CHI generic caching interface that one might want to use instead
uses Moose (Mouse with Any::Moose?) for OOP, which imposes some startup
cost.  One enables and configures output caching in gitweb config, so
if gitweb config would be read once per run then cache interface could
be started once per run, not once per request.
 
Nevertheless this change is backwards incompatibile change, and should
probably wait for 1.7.3 (pity that 1.7.2 was so recently released).


One solution I can think of (still backwards incompatibile) would be to
provide $per_request_config variable, which would hold anonymous sub
with parts of config that need to be done per request (this should work
with global variables (our), but I think it wouldn't work with lexical
variables (my)).  For example gitolite's contrib/gitweb/gitweb.conf would
then include:

  $per_request_config = sub {
  	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
  }

What do you think about it?

>  gitweb/gitweb.perl |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e0e9532..300c4b1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1060,8 +1060,12 @@ sub run_request {
>  	reset_timer();
>  
>  	evaluate_uri();
> +	evaluate_gitweb_config();
>  	check_loadavg();
>  
> +	# $projectroot and $projects_list might be set in gitweb config file
> +	$projects_list ||= $projectroot;
> +
>  	evaluate_query_params();
>  	evaluate_path_info();
>  	evaluate_and_validate_params();
> @@ -1109,12 +1113,8 @@ sub evaluate_argv {
>  
>  sub run {
>  	evaluate_argv();
> -	evaluate_gitweb_config();
>  	evaluate_git_version();
>  
> -	# $projectroot and $projects_list might be set in gitweb config file
> -	$projects_list ||= $projectroot;
> -
>  	$pre_listen_hook->()
>  		if $pre_listen_hook;
>  

That reminds me: '$projects_list ||= $projectroot;' line should be put,
I think, inside evaluate_gitweb_config().  But that of course should not
be done in this patch.

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

[Jonathan Nieder]

Jakub Narebski wrote:

> One solution I can think of (still backwards incompatibile) would be to
> provide $per_request_config variable, which would hold anonymous sub
> with parts of config that need to be done per request (this should work
> with global variables (our), but I think it wouldn't work with lexical
> variables (my)).  For example gitolite's contrib/gitweb/gitweb.conf would
> then include:
> 
>   $per_request_config = sub {
>   	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
>   }

How would that interact with caching?  i.e., how would whatever caching
engine is used learn that $cgi->remote_user is part of the key needed to
uniquely determine a response?

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

[Jakub Narebski]

Jonathan Nieder wrote:
> Jakub Narebski wrote:
> 
> > One solution I can think of (still backwards incompatibile) would be to
> > provide $per_request_config variable, which would hold anonymous sub
> > with parts of config that need to be done per request (this should work
> > with global variables (our), but I think it wouldn't work with lexical
> > variables (my)).  For example gitolite's contrib/gitweb/gitweb.conf would
> > then include:
> > 
> >   $per_request_config = sub {
> >   	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
> >   }
> 
> How would that interact with caching?  i.e., how would whatever caching
> engine is used learn that $cgi->remote_user is part of the key needed to
> uniquely determine a response?

Well, obviously caching itself, including generating cache key, would be
done per-request.  It is only *initializing* cache (i.e. require + 
+ $cache->new()) that should be done once per run and not once per 
request.

But nevermind, this might be not much of a problem.  I'll start with
cache re-initialized on each request for start.

-- 
Jakub Narebski
Poland