Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
- From
Petr Baudis <pasky@suse.cz>
- Date
- Jul 12, 2008, 13:45 UTC
- Message-ID
- <20080712134553.GC10151@machine.or.cz>
- In-Reply-To
- <200807121435.49151.jnareb@gmail.com>
On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
Show 29 quoted lines
> On Sat, Jul 12, 2008, Petr Baudis wrote:
> > Or if you mean foreach use, it's trivial to
> >
> > foreach (grep { /^gitweb\./ } keys %$config)
> >
> > or provide a method of Git::Config that will return this list, but it
> > does not seem appropriate to have specific Git::Config instances for
> > this. (Besides, if the script also needs to access a single variable
> > from a different section, it will need to re-parse the config again.)
> >
> > So I think your approach would be good only if there are multiple
> > methods of Git::Config that would operate on the whole config and needed
> > a way to be restricted; is that the case?
>
> On the other hand if one uses (assuming now Git::Config object API)
>
> $conf = $repo->get_config('section');
>
> one could access configuration variable values _without_ prefixing
> them with subsection name, i.e.
>
> $conf->get('variable');
>
> and not
>
> $conf->get('section.variable');
>
>
> I'm not sure if it is much of an improvement.I would propose not to go with this feature yet and add it only when justified by accompanied cleanups of real code using this.
> We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and > (in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can > have $c->get(<VAR>), $c->get_bool(<VAR>) etc.
I think I prefer the terser approach of value/bool/int now since it is shorter and seems to stay pretty unambiguous. Besides, this way we can easily make mutators by just adding second optional argument to these methods.
Show 18 quoted lines
> The former plays better with
>
> $r->get_config()->bool('gitweb.blame');
>
> but this is nevertheless not recommended workflow; you can use
>
> $r->config_bool('gitweb.blame');
>
> and it would be faster (unless some Perl/OO trickery with singletons
> and the like).
>
> Recommended workflow (code) is
>
> $c = $r->get_config();
> ...
> $c->get_bool('gitweb.blame');
>
> or something like that.I thought that the get_config() method would be reusing existing Git::Config instances no-matter-what? (Otherwise, you can get into rather tricky issues like one part of the code modifying the config and the other not noticing the change, etc. And I can se no benefit. If you want to reload the config, we might have method like reload(), but I don't think that would be very useful except perhaps in some kind of mod_perl'd gitweb scenario.)
Show 17 quoted lines
> >>> Also, having accessors for special types lets you return undef when
> >>> the type really isn't defined, instead of e.g. true with current
> >>> config_val_bool, which is clearly bogus and requires complicated code
> >>> on the user side.
> >>
> >> I don't follow you. Didn't we agree on casting an error when variable
> >> is not of given type?
> >
> > Sorry, s/type really/variable really/. According to your original code,
> >
> > config_val_bool(undef)
> >
> > would return true, while the undef could be from both non-existent and
> > unassigned variable. (This 'unassigned variables' case is really
> > annoying to handle.)
>
> That is why you should check exists($config{<VAR>}) first.And that was my point when I talked about complicated code on the user side.
But we're arguing about nothing, really, except the exact amount of coolness of the Git::Config approach. :-)
-- Petr "Pasky" Baudis GNU, n. An animal of South Africa, which in its domesticated state resembles a horse, a buffalo and a stag. In its wild condition it is something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce