Re: [PATCH] BUG/MEDIUM: init: don't use environment locale
Hi, Le 19/05/2016 03:11, Maciej Katafiasz a écrit : On 18 May 2016 at 16:18, Cyril Bonté wrote: Le 19/05/2016 00:34, Maciej Katafiasz a écrit : While potentially confusing, forcing it to C is also confusing and prevents people from actually exploiting locale should they want to, and traditionally the Unix approach was to let the administrator get the locale right, so it's also more consistent with how other software does it. Which ones ? As previously said, nor apache httpd and nginx do that with their respective "include" directive, at least. Why would we want to make it differently ? Postgresql for example [...] Sorry but no, it doesn't. The "include_dir" directive in PosgreSQL uses readdir() which doesn't have any sorting guarantee, fills an array with the filenames, then calls qsort() on the arrray with strcmp() as the comparator function. They also document this in their documentation : "Multiple files within an include directory are processed in file name order (according to C locale rules, i.e. numbers before letters, and uppercase letters before lowercase ones)." and they have the good idea to provide some good practices about naming convention. -- Cyril Bonté
Re: [PATCH] BUG/MEDIUM: init: don't use environment locale
On Wed, May 18, 2016 at 11:13:38PM +0200, Maxime de Roucy wrote: > This patch remove setlocale from the main function. (...) Thank you Maxime, I've merged it. I've amended your commit message to mention the ID of the commit which introduced it in order to avoid future confusion from people who fear a change of behaviour. Thanks! Willy
Re: [PATCH] BUG/MEDIUM: init: don't use environment locale
On 18 May 2016 at 16:18, Cyril Bonté wrote: > Le 19/05/2016 00:34, Maciej Katafiasz a écrit : >> While potentially >> confusing, forcing it to C is also confusing and prevents people from >> actually exploiting locale should they want to, and traditionally the >> Unix approach was to let the administrator get the locale right, so >> it's also more consistent with how other software does it. > > > Which ones ? As previously said, nor apache httpd and nginx do that with > their respective "include" directive, at least. Why would we want to make it > differently ? Postgresql for example, and many other commonly used pieces of unix code. Like shell, as was originally pointed out. >> It's >> documented that way too, so your patch constitutes a design change, >> which I feel should be argued for with a bit more rationale. > > > It only reverts to the original design. Right, I got confused about the context of the change, and missed the fact it was within the config dir patch only. Still, both approaches have their downsides, as also mentioned. That said, not touching existing configs is probably the better side to err on, so I withdraw my objections. I'd spend a bit more text on explaining the implications of collation in C locale vs user locale though, change "LC_COLLATE=C" in the docs to "C locale" because the code does not actually consult LC_COLLATE, clarify what "lexical order" means in the context of multiple directories (ie. whether all files found are sorted together before including, or only within the containing directory, with the order of directories passed determining the order of inclusion), and that directories are *not* scanned recursively (which is my understanding of the intended design). Cheers, Maciej
Re: [PATCH] BUG/MEDIUM: init: don't use environment locale
Le 19/05/2016 00:34, Maciej Katafiasz a écrit : On 18 May 2016 at 14:13, Maxime de Roucy wrote: This patch remove setlocale from the main function. Some regex may have different behaviours depending on the locale. Some LUA scripts may change their behaviour too (http://lua-users.org/wiki/LuaLocales). I'm not convinced that is a good change. It should probably be left up to the environment to get the locale right. On the contrary, I'm more convinced its a good one. While potentially confusing, forcing it to C is also confusing and prevents people from actually exploiting locale should they want to, and traditionally the Unix approach was to let the administrator get the locale right, so it's also more consistent with how other software does it. Which ones ? As previously said, nor apache httpd and nginx do that with their respective "include" directive, at least. Why would we want to make it differently ? It's documented that way too, so your patch constitutes a design change, which I feel should be argued for with a bit more rationale. It only reverts to the original design. -- Cyril Bonté
Re: [PATCH] BUG/MEDIUM: init: don't use environment locale
On 18 May 2016 at 14:13, Maxime de Roucy wrote: > > This patch remove setlocale from the main function. > > Some regex may have different behaviours depending on the > locale. Some LUA scripts may change their behaviour too > (http://lua-users.org/wiki/LuaLocales). I'm not convinced that is a good change. It should probably be left up to the environment to get the locale right. While potentially confusing, forcing it to C is also confusing and prevents people from actually exploiting locale should they want to, and traditionally the Unix approach was to let the administrator get the locale right, so it's also more consistent with how other software does it. It's documented that way too, so your patch constitutes a design change, which I feel should be argued for with a bit more rationale.
