[pacman-dev] Case sensitive options in pacman.conf
Currently, every options in pacman.conf apparently has a fixed case : RootDir = / DBPath = /var/lib/pacman/ CacheDir = /var/cache/pacman/pkg/ LogFile = /var/log/pacman.log HoldPkg = pacman glibc But in fact, pacman does an insensitive comparisons when parsing the file. } else if(!strcmp(key, "CACHEDIR")) { with key = strtoupper("CacheDir") So you can put cAcHeDiR if you like.. Then we found out this insensitive comparison didn't work in some locales, like tr_TR one, where for example, upper(i) != I To work around this, we also added a sensitive comparison : } else if(strcmp(origkey, "CacheDir") == 0 || strcmp(key, "CACHEDIR") == 0) { http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=1cd7567ff8af4... So in fact, in tr_TR locale, only case sensitive options will really work (eg cAcHeDiR won't work). Why do we make a special case for this locale? I think that having to do every single comparisons twice during the config file parsing is really ugly, and more error prone when adding new options. So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :) If that's not acceptable, what about adding a new function, to do something like : } else if(compare(origkey, "CacheDir", "CACHEDIR") == 0) { That still isn't ideal but would at least reduce the duplication a bit.
2008/3/11, Xavier <shiningxc@gmail.com>:
Currently, every options in pacman.conf apparently has a fixed case : RootDir = / DBPath = /var/lib/pacman/ CacheDir = /var/cache/pacman/pkg/ LogFile = /var/log/pacman.log HoldPkg = pacman glibc
But in fact, pacman does an insensitive comparisons when parsing the file. } else if(!strcmp(key, "CACHEDIR")) { with key = strtoupper("CacheDir")
So you can put cAcHeDiR if you like..
Then we found out this insensitive comparison didn't work in some locales, like tr_TR one, where for example, upper(i) != I To work around this, we also added a sensitive comparison : } else if(strcmp(origkey, "CacheDir") == 0 || strcmp(key, "CACHEDIR") == 0) { http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=1cd7567ff8af4...
So in fact, in tr_TR locale, only case sensitive options will really work (eg cAcHeDiR won't work). Why do we make a special case for this locale? I think that having to do every single comparisons twice during the config file parsing is really ugly, and more error prone when adding new options.
So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :)
If that's not acceptable, what about adding a new function, to do something like : } else if(compare(origkey, "CacheDir", "CACHEDIR") == 0) {
That still isn't ideal but would at least reduce the duplication a bit.
"only case sensitive" is ok for me, I'm not that lazy to use Shift. :-P -- Roman Kyrylych (Роман Кирилич)
Currently, every options in pacman.conf apparently has a fixed case : RootDir = / DBPath = /var/lib/pacman/ CacheDir = /var/cache/pacman/pkg/ LogFile = /var/log/pacman.log HoldPkg = pacman glibc
But in fact, pacman does an insensitive comparisons when parsing the file. } else if(!strcmp(key, "CACHEDIR")) { with key = strtoupper("CacheDir")
So you can put cAcHeDiR if you like..
Then we found out this insensitive comparison didn't work in some locales, like tr_TR one, where for example, upper(i) != I To work around this, we also added a sensitive comparison : } else if(strcmp(origkey, "CacheDir") == 0 || strcmp(key, "CACHEDIR") ==
2008/3/11, Xavier <shiningxc@gmail.com>: 0) {
http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=1cd7567ff8af4...
So in fact, in tr_TR locale, only case sensitive options will really work
(eg
cAcHeDiR won't work). Why do we make a special case for this locale? I think that having to do every single comparisons twice during the config file parsing is really ugly, and more error prone when adding new options.
So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :)
If that's not acceptable, what about adding a new function, to do something like : } else if(compare(origkey, "CacheDir", "CACHEDIR") == 0) {
That still isn't ideal but would at least reduce the duplication a bit.
"only case sensitive" is ok for me, I'm not that lazy to use Shift. :-P +1
---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
On Tue, Mar 11, 2008 at 10:10 AM, Xavier <shiningxc@gmail.com> wrote:
So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :)
I originally brought this up to Dan when I made the tr_TR change. I was for case-sensitive compares, but I think Dan had a bit of an issue with it. Dan, do you remember what it was?
On Tue, Mar 11, 2008 at 11:14:17AM -0500, Aaron Griffin wrote:
On Tue, Mar 11, 2008 at 10:10 AM, Xavier <shiningxc@gmail.com> wrote:
So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :)
I originally brought this up to Dan when I made the tr_TR change. I was for case-sensitive compares, but I think Dan had a bit of an issue with it. Dan, do you remember what it was?
15:40 <toofishes> i'm sure it is used by a handful of people 15:41 <toofishes> i know it isn't the prettiest, but i don't have a good reason to get rid of it (as it has been this way for a long time, we just fixed the issues with i <-> I) And vmiklos said it was probably used by many users : http://www.archlinux.org/pipermail/pacman-dev/2007-August/009168.html I doubt that many users use it, since every keywords present in the default pacman.conf have that same case format (RootDir, CacheDir, etc)
On Tue, Mar 11, 2008 at 11:23 AM, Xavier <shiningxc@gmail.com> wrote:
On Tue, Mar 11, 2008 at 11:14:17AM -0500, Aaron Griffin wrote:
On Tue, Mar 11, 2008 at 10:10 AM, Xavier <shiningxc@gmail.com> wrote:
So I would vote for only allowing case sensitive options in pacman.conf, to treat all locales equally, and cleaning the code :)
I originally brought this up to Dan when I made the tr_TR change. I was for case-sensitive compares, but I think Dan had a bit of an issue with it. Dan, do you remember what it was?
15:40 <toofishes> i'm sure it is used by a handful of people 15:41 <toofishes> i know it isn't the prettiest, but i don't have a good reason to get rid of it (as it has been this way for a long time, we just fixed the issues with i <-> I)
And vmiklos said it was probably used by many users : http://www.archlinux.org/pipermail/pacman-dev/2007-August/009168.html
I doubt that many users use it, since every keywords present in the default pacman.conf have that same case format (RootDir, CacheDir, etc)
Glad you brought this up and it got a bunch of responses. Looks like I am a bit in the minority here, and we probably would be OK just doing the CamelCase option names *if* we document this point in pacman.conf.5. Xavier, want to toss a patch into the mix here that cleans up the strcmp() usage AND documents the case-sensitivity in the manpage? -Dan
On Tue, Mar 11, 2008 at 12:59:36PM -0500, Dan McGee wrote:
Glad you brought this up and it got a bunch of responses.
Looks like I am a bit in the minority here, and we probably would be OK just doing the CamelCase option names *if* we document this point in pacman.conf.5.
Xavier, want to toss a patch into the mix here that cleans up the strcmp() usage AND documents the case-sensitivity in the manpage?
Yes, I also felt like it should be mentioned there, and I began to look at the man page, but wasn't sure in which section it belonged. Maybe at the end of the Description section ? Or the beginning of Options? I am not sure. Also, it will conflict with my CleanMethod patch, which one should I do first?
These case insensitive comparisons didn't work in some locales, like tr_TR where upper(i) != I. So a second case sensitive comparison had to be made for each directive. Only keeping case sensitive comparisons make the code cleaner and treat all locales equally. Ref: http://www.archlinux.org/pipermail/pacman-dev/2008-March/011445.html Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- doc/pacman.conf.5.txt | 2 + src/pacman/pacman.c | 52 +++++++++++++++++++----------------------------- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index e8f7454..12df824 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -40,6 +40,8 @@ Include = /etc/pacman.d/core Server = file:///home/pkgs -------- +*NOTE*: Each directive must be in CamelCase. If the case isn't respected, the directive +won't be recognized. For example. noupgrade or NOUPGRADE will not work. Options ------- diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 579474c..308d1dd 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -604,7 +604,7 @@ static int _parseconfig(const char *file, const char *givensection, } } else { /* directive */ - char *key, *upperkey; + char *key; /* strsep modifies the 'line' string: 'key \0 ptr' */ key = line; ptr = line; @@ -617,11 +617,7 @@ static int _parseconfig(const char *file, const char *givensection, file, linenum); return(1); } - /* For each directive, compare to the uppercase and camelcase string. - * This prevents issues with certain locales where characters don't - * follow the toupper() rules we may expect, e.g. tr_TR where i != I. - */ - upperkey = strtoupper(strdup(key)); + /* For each directive, compare to the camelcase string. */ if(section == NULL) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), file, linenum); @@ -629,25 +625,25 @@ static int _parseconfig(const char *file, const char *givensection, } if(ptr == NULL && strcmp(section, "options") == 0) { /* directives without settings, all in [options] */ - if(strcmp(key, "NoPassiveFTP") == 0 || strcmp(upperkey, "NOPASSIVEFTP") == 0) { + if(strcmp(key, "NoPassiveFTP") == 0) { alpm_option_set_nopassiveftp(1); pm_printf(PM_LOG_DEBUG, "config: nopassiveftp\n"); - } else if(strcmp(key, "UseSyslog") == 0 || strcmp(upperkey, "USESYSLOG") == 0) { + } else if(strcmp(key, "UseSyslog") == 0) { alpm_option_set_usesyslog(1); pm_printf(PM_LOG_DEBUG, "config: usesyslog\n"); - } else if(strcmp(key, "ILoveCandy") == 0 || strcmp(upperkey, "ILOVECANDY") == 0) { + } else if(strcmp(key, "ILoveCandy") == 0) { config->chomp = 1; pm_printf(PM_LOG_DEBUG, "config: chomp\n"); - } else if(strcmp(key, "UseColor") == 0 || strcmp(upperkey, "USECOLOR") == 0) { + } else if(strcmp(key, "UseColor") == 0) { config->usecolor = 1; pm_printf(PM_LOG_DEBUG, "config: usecolor\n"); - } else if(strcmp(key, "ShowSize") == 0 || strcmp(upperkey, "SHOWSIZE") == 0) { + } else if(strcmp(key, "ShowSize") == 0) { config->showsize = 1; pm_printf(PM_LOG_DEBUG, "config: showsize\n"); - } else if(strcmp(key, "UseDelta") == 0 || strcmp(upperkey, "USEDELTA") == 0) { + } else if(strcmp(key, "UseDelta") == 0) { alpm_option_set_usedelta(1); pm_printf(PM_LOG_DEBUG, "config: usedelta\n"); - } else if(strcmp(key, "TotalDownload") == 0 || strcmp(upperkey, "TOTALDOWNLOAD") == 0) { + } else if(strcmp(key, "TotalDownload") == 0) { config->totaldownload = 1; pm_printf(PM_LOG_DEBUG, "config: totaldownload\n"); } else { @@ -657,51 +653,46 @@ static int _parseconfig(const char *file, const char *givensection, } } else { /* directives with settings */ - if(strcmp(key, "Include") == 0 || strcmp(upperkey, "INCLUDE") == 0) { + if(strcmp(key, "Include") == 0) { pm_printf(PM_LOG_DEBUG, "config: including %s\n", ptr); _parseconfig(ptr, section, db); /* Ignore include failures... assume non-critical */ } else if(strcmp(section, "options") == 0) { - if(strcmp(key, "NoUpgrade") == 0 - || strcmp(upperkey, "NOUPGRADE") == 0) { + if(strcmp(key, "NoUpgrade") == 0) { setrepeatingoption(ptr, "NoUpgrade", alpm_option_add_noupgrade); - } else if(strcmp(key, "NoExtract") == 0 - || strcmp(upperkey, "NOEXTRACT") == 0) { + } else if(strcmp(key, "NoExtract") == 0) { setrepeatingoption(ptr, "NoExtract", alpm_option_add_noextract); - } else if(strcmp(key, "IgnorePkg") == 0 - || strcmp(upperkey, "IGNOREPKG") == 0) { + } else if(strcmp(key, "IgnorePkg") == 0) { setrepeatingoption(ptr, "IgnorePkg", alpm_option_add_ignorepkg); - } else if(strcmp(key, "IgnoreGroup") == 0 - || strcmp(upperkey, "IGNOREGROUP") == 0) { + } else if(strcmp(key, "IgnoreGroup") == 0) { setrepeatingoption(ptr, "IgnoreGroup", alpm_option_add_ignoregrp); - } else if(strcmp(key, "HoldPkg") == 0 - || strcmp(upperkey, "HOLDPKG") == 0) { + } else if(strcmp(key, "HoldPkg") == 0) { setrepeatingoption(ptr, "HoldPkg", alpm_option_add_holdpkg); - } else if(strcmp(key, "DBPath") == 0 || strcmp(upperkey, "DBPATH") == 0) { + } else if(strcmp(key, "DBPath") == 0) { /* don't overwrite a path specified on the command line */ if(!config->dbpath) { config->dbpath = strdup(ptr); pm_printf(PM_LOG_DEBUG, "config: dbpath: %s\n", ptr); } - } else if(strcmp(key, "CacheDir") == 0 || strcmp(upperkey, "CACHEDIR") == 0) { + } else if(strcmp(key, "CacheDir") == 0) { if(alpm_option_add_cachedir(ptr) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), ptr, alpm_strerrorlast()); return(1); } pm_printf(PM_LOG_DEBUG, "config: cachedir: %s\n", ptr); - } else if(strcmp(key, "RootDir") == 0 || strcmp(upperkey, "ROOTDIR") == 0) { + } else if(strcmp(key, "RootDir") == 0) { /* don't overwrite a path specified on the command line */ if(!config->rootdir) { config->rootdir = strdup(ptr); pm_printf(PM_LOG_DEBUG, "config: rootdir: %s\n", ptr); } - } else if (strcmp(key, "LogFile") == 0 || strcmp(upperkey, "LOGFILE") == 0) { + } else if (strcmp(key, "LogFile") == 0) { if(!config->logfile) { config->logfile = strdup(ptr); pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", ptr); } - } else if (strcmp(key, "XferCommand") == 0 || strcmp(upperkey, "XFERCOMMAND") == 0) { + } else if (strcmp(key, "XferCommand") == 0) { alpm_option_set_xfercommand(ptr); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); } else { @@ -709,7 +700,7 @@ static int _parseconfig(const char *file, const char *givensection, file, linenum, key); return(1); } - } else if(strcmp(key, "Server") == 0 || strcmp(upperkey, "SERVER") == 0) { + } else if(strcmp(key, "Server") == 0) { /* let's attempt a replacement for the current repo */ char *server = strreplace(ptr, "$repo", section); @@ -725,7 +716,6 @@ static int _parseconfig(const char *file, const char *givensection, return(1); } } - free(upperkey); } } fclose(fp); -- 1.5.4.2
On Tue, Mar 11, 2008 at 05:23:20PM +0100, Xavier <shiningxc@gmail.com> wrote:
And vmiklos said it was probably used by many users : http://www.archlinux.org/pipermail/pacman-dev/2007-August/009168.html
I doubt that many users use it, since every keywords present in the default pacman.conf have that same case format (RootDir, CacheDir, etc)
i don't think anybody uses IGNOREPKG but if you type manually (and you know that it's case-insensiteve) then it's possible that you type ignorepkg. speaking of options which are not by default in the pacman config.
On Wed, Mar 12, 2008 at 11:16:58PM +0100, Miklos Vajna wrote:
i don't think anybody uses IGNOREPKG but if you type manually (and you know that it's case-insensiteve) then it's possible that you type ignorepkg. speaking of options which are not by default in the pacman config.
Well, Nagy already proposed to add all options in pacman.conf, but it might not be accepted for the arch pacman package. Even if it isn't, I don't think that's a big problem. pacman will fail saying ignorepkg is an unknown keyword, and the user should be able to figure out his mistake from there.
participants (7)
-
Aaron Griffin
-
Chantry Xavier
-
Dan McGee
-
Miklos Vajna
-
Nagy Gabor
-
Roman Kyrylych
-
Xavier