Re: [Patch] Encode invalid chars in /proc/registry entries

[Christian Franke <Christian dot Franke at t-online dot de>]

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
> this.  

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 
("/") itself.

To be convinced, please try

$ find /proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Cygnus\ Solutions 
>some.file

(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.
>
>   

Agree :-)


Christian