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