[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: loginfo processing (logmsg.c) bug

From: Derek Robert Price
Subject: Re: loginfo processing (logmsg.c) bug
Date: Mon, 19 Aug 2002 11:34:21 -0400
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020606

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

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.



Email: derek@ximbiot.com

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]