[Patch] Encode invalid chars in /proc/registry entries
Fri Nov 16 20:49:00 GMT 2007
Christopher Faylor wrote:
> On Fri, Nov 16, 2007 at 08:35:48PM +0100, Christian Franke wrote:
>> Christopher Faylor wrote:
>>>>> Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not
>>>>> actually tested. Therefore, no changelog provided yet.
>>>> Thanks for this patch. Apart from the missing ChangeLog I'm inclined
>>>> to apply it to the upcoming 1.5.25 release, but I don't like to have it
>>>> in HEAD as is.
>>> I'm not so sure it's appropriate for either yet.
>>> Isn't it possible to use at least some of the managed mode functions
>>> which deal with munging characters to do some of encoding? It seems
>>> like the patch duplicates some of the functionality from path.cc.
>>> I realize that the registry is sort of the opposite of a managed mount
>>> but it seems like the encoding functions might be potentially used in
>>> reverse for this.
>> I actually consulted path.cc before starting the patch but did not find
>> any function which provides the required functionality OOTB.
>> Therefore, I solved the tradeoff between "reuse" and "do not change
>> working code if you don't have time for thorough regression testing" by
>> the latter :-)
> I'm sorry but "reuse" is a fairly important concept in a project like
Agree. But there is always this tradeoff when fixing bugs.
> The proc functions, in particular, have been prone to NIH and I
> don't want to see even more there if we can possibly help it.
> So, I'll reiterate my suggestion that you look at, e.g.,
> mount_item::fnmunge and possibly think about generalizing it if it isn't
> quite up to the task.
OK, I will do this for a possible HEAD patch. But not for this patch,
due to increased regression risk.
fnmunge() is different: The chars '/' and '\' are not considered special
but special names are handled.
The inverse fnunmunge() decodes each %XX sequence, which I didn't want
to avoid introducing alias names.
> I'll also go on record as advocating that this not be part of a bugfix
> release. It seems too much like a last minute change to me.
Sorry, but I disagree. My patch does not introduce a nice-to-have
feature. Returning non-POSIX conforming names in readdir() is IMO not a
minor issue. And Cygwin adds the worst possible registry entry name
To be convinced, please try
$ find /proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Cygnus\ Solutions
(Own risk, it may result in a bluescreen on XP SP2)
This should be fixed in a bugfix release when a reasonable tested patch
with low regression-risk is available (which is IMO the case now :-)
> Getting it into cvs main, however, seems like a good idea.
More information about the Cygwin-patches