Re: loginfo processing (logmsg.c) bug

[Derek Robert Price]

Andrey Aristarkhov wrote:

> > -----Original Message-----
> > From: Derek Robert Price [mailto:derek@ximbiot.com]
> >
> >    
> >
> > Because the equivalent of that function call you were able to exclude
> >    
> is
>  
>
> > being called by other people in other scripts.  
> >    
>
> Sorry, I don't understand what you mean. What function call I was able o
> exclude? I didn't exclude any function call. I've found a bug: %{sVv}
> string in 'loginfo' is not correctly substituted (spaces in file names
> are not escaped, as they should be done). I'v add a code which eliminate
> this bug, but it does not affect any other escaping issues in CVS code.

I meant the function call in your script that pieces the filenames back 
together.  More below:

> > If spaces are suddenly
> > escaped, when they upgrade, all of their scripts will break with
> >    
> little
>  
>
> > warning - especially for those sorts who like to skip reading the
> > release notes.  The CVS design goals include avoiding that.
> >    
> I clearly understand that CVS is designed in "plug-n-play" manner and
> even lazy men can use it with minimal knowlege of theory. And, I hope
> you will agree with me, if documentation states that %s expanded into
> file name and %{s} is expanded to a list of files, it should do in a
> such way. In current version of CVS if I commmit two files:
> "readme.txt." and "TO DO.txt", filter script declared to be called will
> take _3_ paramters ("readme.txt", "TO", "DO.txt") as a result of
> expantion %{s}. In my implementation the filter takes _2_ parameters -
> corresponded file names. I didn't see any backaward compatibility issues
> my patch can affect. 
> Hope I've convinced you with my reasons.
>  

Actually, in loginfo, I believe that your script would recieve a single 
parameter of the format "repos file,tag,tag file,tag,tag file,tag,tag 
...", or with %{s}, "path file file file..." so only your script should 
have been doing the argument parsing, not the shell, and I expect your 
script would have had to convert the escaped space ("\ ") back into a 
space (" ").  This is what could break other scripts.

While reading through the source to reassure myself this was the case, I 
stumbled across a workaround to your problem left by Larry Jones in a 
comment back in April:  you can insert extra empty fields into the 
argument string using spaces, e.g. %{ s }.  This would output something 
like, "path ,file, ,file, ,file,...".  Each empty field will insert 
another comma if you need to handle file names with single commas in 
them too, and so on.

> > Well, yes, but in a backwards compatible way.  Things still work after
> > upgrade, except that a bunch of extra warnings message will be printed
> > on stderr urging the users to update their scripts.
> >    
> This is not users' problem. This is CVS admins' problem. Users are just
> CVS clients.
>  

Well, okay, but the users will probably goad the admin into fixing it.  ;)

> I suppose old code could be removed in minor release. Or may be even
> better to remove it in major release (CVS 2.x). Actually CVS IMHO has
> too much configuration files with different formats. I've not seen your
> patch yet but I hope it will really unify format of CVS configuration
> files.
>  

All the *info hooks were unified.  It's been awhile since I looked at 
the patch, but all the *info parsing routes through the same function. 
I also pulled out some of the common options, and made them available 
seperately to all *info hooks - %p for repository, for example.

Derek

--
               *8^)

Email: derek@ximbiot.com

Get CVS support at http://ximbiot.com
--
Old musicians don't die... they just decompose.