[pacman-dev] [PATCH 1/2] skip unneeded parsing

[jjacky]

On 01/10/12 15:25, Dan McGee wrote:
> On Tue, Jan 10, 2012 at 7:37 AM, jjacky <i.am.jack.mail at gmail.com> wrote:
>>
>> On 01/09/12 19:40, Dan McGee wrote:
>>> On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail at gmail.com> wrote:
>>>> parsing config file is a two-steps process, one for
>>>> "options" and one for the repos. in each of those,
>>>> there's no need to parse the other section(s), so we'll
>>>> skip it. (most notably, when parsing for "options" it
>>>> would read all included files for repos, while just
>>>> ignoring everything in them)
>>>>
>>>> Signed-off-by: Olivier Brunel <i.am.jack.mail at gmail.com>
>>>
>>> I think this will fail hard in the following scenario:
>>>
>>> pacman.conf follows:
>>> -----
>>> Include /etc/pacman.d/shared-options.conf
>>>
>>> [repo]
>>> ....
>>> -----
>>>
>>> Include directives can occur *anywhere*, not just inside sections.
>>
>> Sorry, I'm back on this (because I work on something that needs to parse
>> pacman.conf)
>>
>> First off, actually your example was invalid, as there cannot be
>> directives outside of any section, pacman would even throw a syntax
>> error ("All directives must belong to a section").
> Ahh, tested and confirmed, duh. Thanks for looking at this.
> 
>> What is true, however, is that inside an included file new sections can
>> be opened. So as I said last time, if a file included in "options" was
>> to define some repos, my patch would fail (since any & all Include
>> directive in repos would be ignored during options pass, and vice versa).
> Correct.
> 
>> However, I feel like I should ask: is it really the expected behavior?
>> That pacman allows/supports sections being defined inside included files?
> 100% correct that you can *define and use them* in an include file,
> yes. In fact, I'm thinking the first "all directives" check should not
> be so strict here, if the first line of an Included file is a
> [section] header, we should be allowing that.
> 
> Say a sysadmin wanted to change repos on 15 machines but couldn't
> share the pacman.conf file across them; it would be much easier if all
> repos were defined in a separate include file and that file could be
> swapped on all the machines.
> 
>> Because this could lead to trouble; for instance, w/ this:
>>
>> -- pacman.conf -------
>> [options]
>> Include = /etc/pacman.d/shared.conf
>> DBPath = /opt/pacman/
>> ----------------------
>>
>> -- shared.conf -------
>> [core]
>> Server = http//foo.bar/
>> ----------------------
>>
>> Then by the time pacman gets to the DBPath directive, it thinks the
>> current section is "core" and therefore will *not* apply that directive.
>> That doesn't seem very right to me?
>>
>> In fact, this would result in a warning: "directive 'DBPath' in section
>> 'core' not recognized."
>>
>> One could also imagine that servers get added to the wrong repo with the
>> same kind of situation. Only then, there would be no warnings!
>>
>> Is this really the expected behavior?
>>
>>
>> A fix for this could be to use a new struct section_t when parsing an
>> included file, so as to not "mess up" the current one. (And my patch
>> would still be invalid, for the same reason it is now.)
> This is my vote, as I agree, the current parsing is not so sane when
> returning back from a file. We should probably only allow sections to
> propagate down the include chain and not back up, as you have
> indicated, so this seems like an acceptable resolution to me.

Alright; So I'll look into this and see to send a patch to do that.

> 
>> Or, to ignore sections in included file, and going with the idea that
>> all directives in an included file belong to the section where the
>> Include directive was in.
>> (In which case I think my patch could work, since e.g. during the
>> options pass there would be no need to process Include directives in
>> repos, and vice versa.)
>>
>> I also feel the later might make things clearer, but I'm sure there
>> might be usage cases I'm not thinking of, where it might be good to
>> define sections inside included files?
>>
>> Any ideas/thoughts on this?
> Can you explain the original rationale behind your patch- was it just
> to save some cycles? The config parsing is extremely fast and that is
> why I had no qualms switching it to doing it twice.

Yes, just to save cycles. It's simply because I was looking at how this
work, and noticed that included files were read twice (one per pass)
while I thought only one time was needed, so I figured I might save a
few "useless" stuff, is all. Never had any problems or anything feel
slow or anything.

-j

> 
> -Dan
>