Re: [PATCH] gitweb: Use the config file to set repository owner's name.

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 8, 2008, 15:33 UTC
Message-ID: <200802081633.55934.jnareb@gmail.com>
In-Reply-To: <20080208143027.GA707@c3sl.ufpr.br>

I have joined the two emails to reply only once.

On Fri, 8 Feb 2008, Bruno Cesar Ribas wrote:

Show 20 quoted lines

> On Fri, Feb 08, 2008 at 02:55:33AM -0800, Jakub Narebski wrote:
>> Bruno Ribas <ribas@c3sl.ufpr.br> writes:
>>> 
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 8ef2735..e8a43b7 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -1767,7 +1767,12 @@ sub git_get_project_owner {
>>>       if (exists $gitweb_project_owner->{$project}) {
>>>               $owner = $gitweb_project_owner->{$project};
>>>       }
>>> -     if (!defined $owner) {
>>> +
>>> +     if (!defined $owner){
>>> +             $owner = git_get_project_config('owner');
>>> +     }
>>> +
>>> +     if (!$owner) {
>>>               $owner = get_file_owner("$projectroot/$project");
>>>       }

Another comment: why did you change from checking of "!defined $owner" to checking "!$owner"? git_get_project_config('owner') returns undef if gitweb.owner is not defined. With checking for defined we can avoid false positives of owner being "0" (in practice I think this does not matter) or "" (this could happen if somebody doesn't want for project to have owner shown).

Show 5 quoted lines

>> First, I think the empty lines added are not needed.
> 
> I made those empty lines because original code had same empty lines
> above, I just let it to have same pattern, but I can remove. Should I
> remove?!

The idea was for empty lines to separate blocks of code: variables declaration, initialization, finding an owner, and return value. So I think that empty lines are not needed here. There were no empty lines between check for owner in the structure populated by git_get_project_list_from_file() and checking filesystem stat for project directory owner.

By the way, the git_get_project_list_from_file() interface is a bit strange...

> I that last 3lines should be inside the block that we call
> git_get_project_config, don't you think?

No. I think using "if (!defined $foo) { maybe define foo }..." sequence is a good flow.

> I'll resend [...] with $git_dir set.

And with signoff corrected, I assume?

Please try to check if the code works with and without gitweb.owner set before sending new version of the patch...

-- 
Jakub Narebski
Poland