On 01/10/12 15:25, Dan McGee wrote: > On Tue, Jan 10, 2012 at 7:37 AM, jjacky <[email protected]> wrote: >> >> On 01/09/12 19:40, Dan McGee wrote: >>> On Mon, Jan 9, 2012 at 12:09 PM, jjacky <[email protected]> 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 <[email protected]> >>> >>> 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 >
