Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 3, 2010, 18:43 UTC
Message-ID: <201006032043.14071.jnareb@gmail.com>
In-Reply-To: <AANLkTimoA95U0vivTzrc0XZ8i6q-SfCFA6RgMWK67OWl@mail.gmail.com>

Pavan Kumar Sunkara wrote:

> 2010/6/3 Jakub Narebski <jnareb@gmail.com>:
>> On Tue, 3 Jun 2010, Petr Baudis wrote:

Show 18 quoted lines

>>>> +
>>>> +use Gitweb::Config;
>>>>
>>>>  BEGIN {
>>>>     CGI->compile() if $ENV{'MOD_PERL'};
>>>>  }
>>>>
>>>> -our $version = "++GIT_VERSION++";
>>>> +$version = "++GIT_VERSION++";
>>
>> This change is not necessary.
>>
>>  our $version = "++GIT_VERSION++";
>>
>> would keep working even if '$version' is declared in other module and
>> exported by this module (is imported into current scope).
> 
> Ok. Will change it.

[...]

Show 7 quoted lines

>> I would say
>>
>>        our $GIT = "++GIT_BINDIR++/git";
> 
> But, I think when we start reading the code, it would seem that 'our
> $GIT' implies that it is a variable created locally rather than an
> exported variable from Gitweb::Config module.

From `perldoc -f our`:

  An "our" declares the listed variables to be valid globals within the
  enclosing block, file, or "eval".  That is, it has the same scoping
  rules as a "my" declaration, but does not create a local variable.

  [...] The package in which the variable is entered is determined at
  the point of the declaration, [...]

So if the variable already exists in given scope, for example if the variable was imported from other package, the 'our' declaration would be a no-op.

Show 5 quoted lines

> 
> Even though it increases the patch size, I don't think it will be much
> of a concern when it comes to good redability of code.
> 
> Jakub: Can you reply, what you think about this argument ?

But I agree that first, 'our $var' seems to imply that it is _new_ variable declared in current scope, and second if we make a typo in variable name it wouldn't be detected as different from exported variable: 'our' will create new variable.

So I agree that removing 'our' is a good idea, especially together with putting all variables that should be there in Gitweb::Config together with comments, even if they are configured during build process.

Perhaps those declarations in Gitweb::Config should have in-line comment that they are defined in gitweb.cgi / gitweb.perl?

-- 
Jakub Narebski
Poland