Re: [gentoo-portage-dev] [PATCH] FEATURES=case-insensitive-fs for bug #524236

2014-11-13 Thread Alexander Berntsen
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 13/11/14 02:22, Zac Medico wrote:
 +if case-insensitive-fs in portage.settings.features:
 +FIND_EXTANT_CONFIGS = \
 +FIND_EXTANT_CONFIGS.replace(-name '._cfg, -iname '._cfg)
 +
Splitting inside the replace will look nicer following PEP indentation
(as you won't need the '\').

 +Use case\-insensitive file name comparisions when merging and unmerging
 +files.
 +.TP
Maybe mention a) that most people can ignore this option, and b) who
it's actually for. Kind of in the kernel option help style.


In general I don't like this patch. It handles a bunch of cases separately by 
doing lower(), when I think instead it should be handled implicitly. The data 
should be in a structure such that it knows whether it is supposed to be upper 
or lowercase, and whatever's dealing with it should deal with it accordingly, 
rather than checking is this case insensitive? OK lowercase it before sending 
it wherever.

But if you think this is the best way, I'm not going to stand in the way of 
this patch.
- -- 
Alexander
berna...@gentoo.org
https://secure.plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlRkiB0ACgkQRtClrXBQc7UudAD/V1r4AR5zA54Xz+LHBVNt0bnn
uQ9w+146L8WYK6pDN9gBAJxZbQREeOwxKSDjluZ1lq9XARf1rh/Eqzl58wIc8I4K
=c1Em
-END PGP SIGNATURE-



Re: [gentoo-portage-dev] [PATCH] FEATURES=case-insensitive-fs for bug #524236

2014-11-13 Thread Zac Medico
On 11/13/2014 02:29 AM, Alexander Berntsen wrote:
 On 13/11/14 02:22, Zac Medico wrote:
 +if case-insensitive-fs in portage.settings.features:
 +FIND_EXTANT_CONFIGS = \
 +FIND_EXTANT_CONFIGS.replace(-name '._cfg, -iname '._cfg)
 +
 Splitting inside the replace will look nicer following PEP indentation
 (as you won't need the '\').

Okay, I'll re-format it as you've suggested.

 +Use case\-insensitive file name comparisions when merging and unmerging
 +files.
 +.TP
 Maybe mention a) that most people can ignore this option, and b) who
 it's actually for. Kind of in the kernel option help style.

Okay, I'll add something about it only being needed for case-insensitive
file systems, which are usually not used.

 
 In general I don't like this patch. It handles a bunch of cases separately
 by doing lower(), when I think instead it should be handled implicitly.
 The data should be in a structure such that it knows whether it is supposed
 to be upper or lowercase, and whatever's dealing with it should deal with
 it accordingly, rather than checking is this case insensitive? OK
 lowercase it before sending it wherever.

Aside from the ConfigProtect constructor, which has a new
case_insensitive keyword parameter, all affected methods handle case
transformations implicitly, as far as API consumers are concerned.

However, we could improve efficiency for some usage patterns by
providing an alternative to dblink.getcontents that is oriented toward
case-insensitive handling. For example, every single
dblink._match_contents call currently has to transform all names to
lowercase, and generate a reverse mapping from lowercase back to
preserved case. The dblink._match_contents method would be more
efficient if we created an alternative to dblink.getcontents that
handled the transformations and cached the results. I intentionally did
not implement this optimization yet, since it's probably better to do it
in a separate patch, rather than complicate the current patch.

 But if you think this is the best way, I'm not going to stand in the way of 
 this patch.

As discussed above, think the current approach is pretty reasonable,
though I would like to optimize it with a separate patch.
-- 
Thanks,
Zac