[pacman-dev] [PATCH] Fix parsing included files w/ sections
jjacky
i.am.jack.mail at gmail.com
Wed Jan 11 18:55:41 EST 2012
On 01/11/12 23:00, Dan McGee wrote:
> On Wed, Jan 11, 2012 at 12:52 PM, Olivier Brunel
> <i.am.jack.mail at gmail.com> wrote:
>> When parsing Include directives, a new section_t is now
>> used, to ensure it won't "mess" with the current one (since
>> included files can define new sections, which would result
>> in an invalid "change" of the current section) This new
>> section starts "in" the current one (so included files are
>> parsed as expected, and don't need to open any section);
>> IOW sections propagate down the include chain, not back up.
>>
>> This also solves the bug (?) when two repos with the same name are declared,
>> which could happen in the same file or, more likely, through Include-s.
>> Now only one db will be registered, with all servers in it (it used to
>> be as many dbs with the same name as there were sections).
>
> IOW? bug (?)? "quotes" around "random" words? I know I'm nitpicking,
> but this is not always the most readable English, especially to
> non-native speakers (even I had to look up what IOW was).
>
> Imagine coming back to a commit message in two years- if there is
> anything in it that wouldn't make sense, you need to reword/rewrite
> it. That is the purpose of commit messages.
Noted, sorry. (To be honest I am very new at all this, so I probably
lack proper habits/methodologies. I'll do my best to improve this.)
>
>> Signed-off-by: Olivier Brunel <i.am.jack.mail at gmail.com>
>> ---
>> src/pacman/conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 files changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
>> index 2ff16d2..eb3374e 100644
>> --- a/src/pacman/conf.c
>> +++ b/src/pacman/conf.c
>> @@ -628,6 +628,18 @@ struct section_t {
>> alpm_list_t *servers;
>> };
>>
>> +static alpm_db_t *get_db(const char *dbname)
>> +{
>> + alpm_list_t *i;
>> + for(i = alpm_option_get_syncdbs(config->handle); i; i = i->next) {
>> + alpm_db_t *db = i->data;
>> + if(strcmp(alpm_db_get_name(db), dbname) == 0) {
>> + return db;
>> + }
>> + }
>> + return NULL;
>> +}
> Rather than copying this straight out of src/pacman/sync.c, why don't
> you make it visible to the entire compilation unit in util.c/util.h
> (this file) and remove the now unneeded version from sync.c? (for a
> more public function, a longer name might be nice too; something like
> 'get_sync_db' just to be as clear as possible).
>
>> +
>> /**
>> * Wrap up a section once we have reached the end of it. This should be called
>> * when a subsequent section is encountered, or when we have reached the end of
>> @@ -650,13 +662,17 @@ static int finish_section(struct section_t *section, int parse_options)
>> goto cleanup;
>> }
>>
>> - /* if we are not looking at options sections only, register a db */
>> - db = alpm_db_register_sync(config->handle, section->name, section->siglevel);
>> + /* if we are not looking at options sections only, get the db */
>> + db = get_db(section->name);
>> if(db == NULL) {
>> - pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"),
>> - section->name, alpm_strerror(alpm_errno(config->handle)));
>> - ret = 1;
>> - goto cleanup;
>> + /* not registered yet, let's do it */
> The "let's do it" part is not really necessary; I say this because we
> also try to avoid contractions if possible.
>
>> + db = alpm_db_register_sync(config->handle, section->name, section->siglevel);
>> + if(db == NULL) {
>> + pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"),
>> + section->name, alpm_strerror(alpm_errno(config->handle)));
>> + ret = 1;
>> + goto cleanup;
>> + }
>> }
>>
>> for(i = section->servers; i; i = alpm_list_next(i)) {
>> @@ -780,6 +796,7 @@ static int _parseconfig(const char *file, struct section_t *section,
>> glob_t globbuf;
>> int globret;
>> size_t gindex;
>> + struct section_t inc_section;
>>
>> if(value == NULL) {
>> pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"),
>> @@ -816,10 +833,33 @@ static int _parseconfig(const char *file, struct section_t *section,
>> file, linenum, value);
>> break;
>> default:
>> + /* create new section_t for the includes -- to keep ours unaffected */
>> for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) {
>> + /* we use a different section_t for includes, so they can't mess up
>> + * the current one. This section starts "in" the current section ofc */
> ofc?
>
>> + inc_section->name = strdup (section->name);
>> + inc_section->is_options = section->is_options;
>> + inc_section->siglevel = section->siglevel;
>> + inc_section->servers = NULL;
>> pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n",
>> file, linenum, globbuf.gl_pathv[gindex]);
>> - _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1);
>> + _parseconfig(globbuf.gl_pathv[gindex], inc_section, parse_options, depth + 1);
>> + /* now, if this is a repo ... */
>> + if(!parse_options && !section->is_options) {
>> + if (section->siglevel == inc_section->siglevel
>> + && strcmp(section->name, inc_section->name) == 0) {
> This logic seems very very suspect to me; and likely wrong. For example:
>
> [myrepo]
> SigLevel = DatabaseRequired PackageOptional
> Server = foobar.com
> Include = /path/to/myrepo.override
> ----> (jump to new file)
> Server = example.com
> SigLevel = PackageRequred
> ----> EOF, jump back
>
> You're going to lose my updated siglevel configuration in all of this;
> I'm not even sure what is going to happen.
>
> I also don't like the assumption that repos have only "Server" as
> their main setting, I have a few plans to add more options to this in
> the future and don't want parsing to rely on the fact that right now
> only "Server" and "SigLevel" appear there, if possible.
Right. This was to go with the idea that changes should only go down,
not up. Right now since the same section_t is used, they go both ways
indeed, but that can lead to troubles when defining sections in included
files.
Thing is, supporting it (sections in included files) means that you
could have e.g. this:
[myrepo]
SigLevel = DatabaseRequired PackageOptional
Server = foobar.com
Include = /path/to/myrepo.override
----> (jump to new file)
[anotherrepo]
Server = foo.com
[myrepo]
Server = example.com
SigLevel = PackageRequred
----> EOF, jump back
With the patch, the updated siglevel would indeed not be applied.
Currently, pacman would register two dbs by the name "myrepo" -- is that
how it is supposed to work?
Or should everything be remembered until all files have been parsed, and
only then dbs be registered?
-j
>> + /* ... still the same one, we'll just import servers */
>> + alpm_list_t *i;
>> + for(i = inc_section->servers; i; i = alpm_list_next(i)) {
>> + section->servers = alpm_list_add(section->servers, i->data);
>> + }
>> + alpm_list_free(inc_section->servers);
>> + free(inc_section->name);
>> + } else {
>> + /* ... a different one, so let's "finish" it */
>> + finish_section(inc_section, parse_options);
>> + }
>> + }
>> }
>> break;
>> }
>> --
>> 1.7.8.3
>
More information about the pacman-dev
mailing list