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

[Dan McGee]

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.

> 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.

-Dan