patchConcurrent Versions System - Patches: patch #4573, Fix for keyword expansion...


You are not allowed to post comments on this tracker with your current authentication level.

patch #4573: Fix for keyword expansion problem/mis-feature during commit

Submitter:  Rahul Bhargava <b_rahul>
Submitted:  Fri 28 Oct 2005 09:28:53 PM UTC
Votes:  100  
Category:  None Priority:  9 - Immediate
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Fixed Release:  None Fixed Feature Release:  None

( Jump to the original submission )

Thu 16 Feb 2006 02:17:21 PM UTC, comment #19: 

Pat, in your use cases, the user cannot "[control the server] metadata", spuriously or otherwise.  The server will still substitute the correct keyword information on checkout, provided the user did not actually delete the keywords from the file, which this patch would not prevent.

The difference is that, currently, if a misguided user checks in a change after editing the keyword information in the file, it can still be recovered.  After this patch, this would no longer be possible.  The "very simple guarantee" guarantee we are attempting to provide is that user changes cannot be permanently lost.

Also, your second example, where a bug fix from a contractor contains differing RCS keyword information can be fixed by any of several simple procedural fixes:  1.  Have your contractor send a diff rather than copies of every file, 2. import drops from your contractor into a special tree and generate the same diffs yourself, 3. use vendor branches to import code drops from your contractor.

Derek Robert Price <dprice>
Project Administrator
Sat 11 Feb 2006 12:56:50 AM UTC, comment #18: 

I think Mark's example about lost PERL code is a compelling argument for leaving the status quo. Your patch bypasses the inconvenience of extraneous checkins at the expense of potentially losing valuable information.

Applying the wrong -k options to a file is an extremely common mistake, which even experienced CVS users can make.

I think the correct decision has been made.

Jim Hyslop <jhyslop>
Project Member
Sat 11 Feb 2006 12:03:39 AM UTC, comment #17: 

It is really unfortunate that this bug will not be fixed. There has been no response to the case I made two and a half months ago FOR fixing the bug. It absolutely blows my mind that the CVS maintainers will not even consider implementing this patch as a non-default option.

Since it looks like the CVS maintainers will never fix this problem, I had to recommend to my company that other alternatives should be evaluated, and if a suitable alternative is found, we should switch. After a careful evaluation of several open-source and commercial alternatives, we have decided to switch to Subversion.

Yes - the Subversion team got it right. Keyword expansion is Subversion works as CVS would IF the proposed patch were incorporated.

Patrick Richardson <patjr>
Tue 22 Nov 2005 10:19:52 PM UTC, comment #16: 

Here, for what it's worth is an end-user's perspective:

In the context of a given file, I want one of two things:

RCS keywords are disabled - $DATE: xxx$ is data - not metadata. I absolutely DO NOT WANT the server mucking with what may appear to be an RCS keyword. For these files, I want a very simple guarantee - each new revision represents a change (any change) to the file.

Example 1. I am writing an article on RCS keywords

Example 2. My strange, in-house scripting language has syntax elements that can easily be confused for RCS keywords.

I can get this today with the appropriate sticky -k option. No problem!

RCS keywords are enabled - $DATE: xxx$ is metadata - not data. I want the server AND ONLY THE SERVER controlling the content of the metadata. I want a dependable mechanism to protect the repository from spurious (usually accidental) user changes to the metadata. For these files, I want a very simple guarantee - each new revision represents a change to the file DATA, not METADATA.

Example 1. I have scripts that analyze the RCS metadata in the repository, and publish information to a relational database. This information is then used in generating reports including some elaborate productivity statistics. We don't want records of spurious checkins influencing these reports.

Example 2. Jane had received a bug fix from a contractor. The cover letter accompanying the code drop said that only three files have changed. The contractor uses an RCS-savvy source-code repository (perforce, cvs ...) Jane checked out a clean sandbox, untarred the contractor's code-drop over that, subjected the result to our QA processes, and did a global checkin. Yikes - although the bug fix affected only three files, all 723 files were revved up, because of differences in RCS keyword content. Every engineer trying to check in a change encountered a conflict, even if the change was not to one of the three files changed by the contractor. When they synced up with the repository, they saw 720 spurious updates. The proposed patch here would have been a god-send.

To summarize, I am asking for clear-cut exclusive ownership of RCS keywords - either ONLY the user or ONLY the server should be allowed to dictate their content for a given file. The default keyword-handling behavior of CVS does not give me this clean sepatation today - an RCS keyword field is neither strictly metadata nor strictly data. CVS with this patch gives me exactly what I want. With this patch, I can ensure that an RCS keyword is strictly metadata by default.

In this message, I am making the case for a clean model where an RCS keyword is either just metadata or just data. If there is a case for the mixed model, i.e., the status quo - one based on plausible use-cases, not historical precedent - it has not been made. Even if such use-cases exist, why would one object if this patch can be enabled or disabled by a simple configuration option?

Finally, another nuisance for us is interference from RCS keywords when we merge branches. It would be cool if the work done in this patch could be extended in the future to disregard metadata differences when merging CVS branches.

Patrick Richardson <patjr>
Tue 15 Nov 2005 10:33:20 PM UTC, comment #15: 

Rahul Bhargava <> writes:

> We are already aware of this feature. If you
> read the initial post, we are not talking about
> disabling keywords or excluding certain keywords
> using CVS from feature branch.

I guess I missed the initial context. My apologies
for not paying closer attention.

> We are talking about the mis-feature that causes
> CVS to bump up revisions when a user commits a
> file which only differs in keywords.

This is not a mis-feature. It is the correct and
documented behavior.

I have known sites which favored this method of
doing 'forced' commits over use of the 'cvs commit
-f' option. Granted, I would hope those folks have
now moved to using 'cvs import -X' for the
purpose, but nevertheless, it was in operational
use explicitly.

CVS should never discard what might be important text
out of a file during a commit.

> The proposed patch preserves the admin's ability to use
> keywords without creating spurious versions on the tree.

Hmmm... This strikes me as an undesirable feature.

> We speak for 100s of companies using CVS today who would
> like to get the submitted patch as an optional feature in
> CVS.

That is an impressive number of companies. Do they all really
believe that CVS should be told to discard text they have put
into the file never to be able to retreive it again?

> No we are not asking that default behaviour be changed.


> Iff an admin wants the behaviour we outlined in
> the initial post they can enable ignoring of
> keywords during classification phase of commit.

This seems like a dangerous knob to me. I have
seen more than one person using a PERL script who
has accidentally 'LOST' text because it turned out that
the text for the program they had working got munged
because CVS RCS keyword expansion broke their code
on checkout. They were able to retrieve the text
by doing a 'cvs update -ko -p file > file' and then
having a 'cvs admin -ko file' and committing without
any further loss of time or money.

Had an admin configured your new option, they
would have completely lost the information inside
of the keywords for all time.

Or, do I still misunderstand the feature?

> If an admin/user wants to use $..$ for their own
> needs they do not have to turn on the feature!

You lost me. I do not understand this sentence.

> All works as before. Again this is being
> proposed as an optional behavior, no need to
> change semantics if you don't want.

Let us look at user visible impact and tell me what
you are trying to accomplish.

A user is looking at a file and accidentally
modifies the $Id: ...$ text to use '1.14' instead
of '1.141' and saves it. The line looks like this:

    sprintf("%s", q$Id: file,v 1.14 2005/09/12 21:03:09 mdb Exp $
            =~ /(\d+\.\d+)/);

It should be noted that this 'spurious' difference
is actually a highly significant one that might
cause problems to users of the script who need
features that behave different in 1.14 of the file
versus 1.141 of the file.

What is the expected behavior for the following

  - cvs update

    Does the user see that a change has been made to the file?

  - cvs update -C

    Does the user's accidental change get reset?

  - cvs diff

    Does the user see that a change has been made to the file?

  - cvs commit

    If the change is just the change to the $Id$
    line, one presumes that the commit to the file
    would not occur. However, if the file
    contained an additional change to the text,
    would the text between the $Id: ... $ also be
    committed, or would it be silently lost?

My feeling is that a careless user needs to accept
the consequences of his actions and that the
behavior of CVS continues to be correct.

However, I suppose that I can see where an
administrator may find it desirable to know that a
commit only has changes to the RCS keywords in a
file and possibly make a decision as a part of the
commitinfo trigger concerning this situation to
enable them to fail the commit of the file in this

So, if you want to propose a contrib/kwdiff
directory that contains a portable C89 program
that would let a user add kwdiff to their
environment such that commitinfo could be used to
run it and determine if the file being committed
only had that kind of change, then I would have no
big problems with the idea. Note that I would
probably consider that two files that differ only
in the RCS keywords to be the non-zero exit case
rather than two that are idential...

I hope I have understood the case correctly.

Mark D. Baushke <mdb>
Project Administrator
Tue 15 Nov 2005 09:55:37 PM UTC, comment #14: 

Which 100s of companies would those be?  Perhaps some of their representatives would care to pipe up and provide example use cases as well as explaining why they can't ask their developers to stop changing keyword values?

Anyhow, I am still against this for the reasons I stated before - I believe this patch implements behavior totally contrary to at least a decade of CVS development philosophy as I understand it.  Data should never be thrown away if we can help it (exempting legacy support for rcs commands like `cvs admin -o', though I really don't even like those).

Derek Robert Price <dprice>
Project Administrator
Tue 15 Nov 2005 09:26:10 PM UTC, comment #13: 

FEATURE branch already has the ability to disable
RCS keyword expansion either selectively, or altogether.

To disable it totally, use the CVSROOT/config line:


to disable just the expansion of $Id$ and $Header$, use
the CVSROOT/config line:


So, a CVS administrator who wants to use $Id: ... $ lines
for their own purposes and not have RCS expansion change
anything on them would just need to put


and not worry about it as all modules in the CVS project
would no longer do expansion of that RCS keyword.

Mark D. Baushke <mdb>
Project Administrator
Tue 15 Nov 2005 08:03:03 PM UTC, comment #12: 

This isn't what I meant to suggest.  I meant to suggest that your original patch be thrown away in favor of a patch that implemented:


This would cause the server to continue convert EOLs but not keywords by default for newly added files, which avoids the "bug" you pointed out by disabling keyword processing.

Of course, I'd be all for removing RCS keyword support from CVS entirely, but I'll probably get a lot of flak for saying that.  :)

Derek Robert Price <dprice>
Project Administrator
Tue 15 Nov 2005 04:32:03 PM UTC, comment #11: 

Derek Wrote:

>Perhaps a patch to, instead, set the default keyword mode for a repository or
>project to something other than "-kkv" would be a reasonable compromise?  Such
>a config key could hopefully be processed at the time of commit of a newly
>added file.  Not that I have time to write this, but I am willing to discuss
>it and perhaps apply a complete patch submission if there are no objections.

That's not a bad idea. I will be happy to resubmit the patch with your recommendations:

1. Define a new keyword expansion mode , "-kks" to indicate server-side keyword control.

2. Support an option, "DefaultKeywordExpansion=kks" in CVSROOT/config to set the default keyword expansion mode to -kks

Let me know if that works and I will work on rolling out changes to the original patch ?

Rahul Bhargava,

Rahul Bhargava <b_rahul>
Thu 10 Nov 2005 04:40:14 AM UTC, comment #10: 

Except that this is not a bug.  CVS is working as designed.  One of the primary design goals of all CVS development is to NEVER, EVER, EVER, EVER, EVER, EVER, EVER, EVER, EVER, EVER, throw away data.  Even when merging local changes with changes from the server, CVS first creates a permanent backup file before editing changes into the local copy.

You are proposing that when a user has intentionally modified a file, CVS should intentionally not only ignore the user's change, but overwrite it on the next update.  This goes against most everything else CVS tries to do.

The alternative, the current behavior and status quo, is that the change be checked into the repository before the local version is rewritten.  Thus, if the user decides they are upset about losing the data, they can get it back and presumably do something to protect it from future keyword substitutions.  If they are not upset about losing the data, then they can presumably refrain from making changes inside keyword values in the future.

In addition, what you are defining as a "bug" only occurs when EITHER users are unaware of how CVS expands keywords OR when the keyword mode is set improperly for a file.  I would define this, rather, as "user error", or a management/training issue.  Either way problems can be fixed quickly and users educated when necessary, as noted above.

Perhaps a patch to, instead, set the default keyword mode for a repository or project to something other than "-kkv" would be a reasonable compromise?  Such a config key could hopefully be processed at the time of commit of a newly added file.  Not that I have time to write this, but I am willing to discuss it and perhaps apply a complete patch submission if there are no objections.

Derek Robert Price <dprice>
Project Administrator
Thu 10 Nov 2005 12:18:26 AM UTC, comment #9: 

This bug should be fixed for the following reasons:

1) Todd Denniston's point No. 6 below - "6) if the users need something that will not be overwritten by the VCS then create that, these keywords were not meant for that purpose."

If you believe this, you must conclude that if the only changes to the file are in the RCS keyword fields, then the file should not be considered as changed. It looks like this exactly is what the proposed fix does.

2) Derek's comments # 2 and 8 propose using -k options to defeat server-side keyword expansion in lieu of fixing this bug. I consider that a rather drastic workaround, especially since someone actually took the trouble to fix the bug.

Re. Derek's comment # 6, yes - I have voted for this patch.

Harry Muller <hmuller_adm>
Wed 09 Nov 2005 03:51:05 PM UTC, comment #8: 

Another thought on uneducated users:  not only do they probably only need to be educated about this once, but an admin can correct the keyword mode on an offending file once and never worry about that file again.  Users uneducated about keywords and keyword modes are unlikely to change the keyword substitution mode back to the offending mode.

Derek Robert Price <dprice>
Project Administrator
Wed 09 Nov 2005 03:40:06 PM UTC, comment #7: 

I believe that the behavior should be left as is for the following reasons:

1) it is a core behavior that was inherited from RCS.

2) people who are inexperienced using tools simply need educated, i.e., they should have to read appropriate parts of the manual for any tool they use, and appropriate should be chosen by their task leaders.

3) it is the documented behavior... RCS key words (which is what these are) exist for the explicit reason of having the Version Control System (VCS) set them any time the VCS interacts with the file.
from the Keyword substitution section of the manual[1]:
"Embedded strings of the form $keyword$ and $keyword:...$ in a file are replaced with strings of the form $keyword:value$ whenever you obtain a new revision of the file."
"CVS will automatically ... expand the string as part of the commit operation."

4) in version 1.12.x local keywords can be created for your users to mess with[2].

5) in version 1.12.x keyword expansion can be controlled in a more fine grained approach[3] already.

6) if the users need something that will not be overwritten by the VCS then create that, these keywords were not meant for that purpose.

7) the manual gives appropriate methods to control the substitution, these methods can be put in the users .cvsrc (AFAIK) if the users do not want to remember to add the substitution method of choice each time they type a command.[4][5]

1.12.x manual has the same information for all the keyword information that exists in 1.11.x but also has some added options (from what I could see).


Todd Denniston <tdennist>
Wed 09 Nov 2005 03:01:19 PM UTC, comment #6: 

I'm not ready to change my mind but, please, vote for this patch using the Savannnah interface.  We don't have an official policy about how to deal with user votes yet, but at least it will help track how popular bug fixes and features are.  There are also several other maintainers who could outvote my single rejection vote, though this discussion appears to have failed to attract their attention as of yet.

Derek Robert Price <dprice>
Project Administrator
Wed 09 Nov 2005 03:19:11 AM UTC, comment #5: 

The behavior implemented by the proposed patch is what I would like for our CVS deployment. The workarounds you suggest require user discipline (consistent use of -k* options, for example) and are therefore very problematic for a large team such as ours (we have our fair share of relatively inexperienced CVS users)

As Yeturu indicates, a version of the patch which preserves the default behavior but makes the new behavior available via an option, sounds quite reasonable. I hope you will reconsider your decision.

Harry Muller <hmuller_adm>
Fri 04 Nov 2005 10:21:01 PM UTC, comment #4: 

The behavior implemented in this patch is something an organization could reasonably want. It is usually not practical, especially with a large and diverse development team, to impose a disciplined use of cvs command options across the entire team. Mistakes will happen. It would be much more desirable to implement the desired behavior in the server.

This patch, as delivered, changes the default behavior of the CVS server, and provides a config option to restore the old behavior. If that were reversed; i.e., the default behavior remained the same, and a config option was provided to enable the new behavior, would you be more amenable to accepting the patch?

Yeturu Aahlad <aahlad>
Fri 04 Nov 2005 07:31:42 PM UTC, comment #3: 

The data loss you refer to only occurs in the user's sandbox.  The change is committed to the repository.  If the original user or another user were to checkout/update the file in -kb or -ko modes, then they would see the data they committed.  In fact, this could be used as a workaround for the problem you describe - if only the user in question wants to see their local changes inside key-values after commit, then they should mark text files -ko on checkout/update and binaries -kb.  If all users should see what the user committed inside key-values, then `cvs add' or `cvs admin' should be used to mark the files default keyword mode as -ko or -kb.  Either of these is even a more fine-grained solution than yours, since it allows this behavior to be selected on a file-by-file basis.

If I recall the documentation correctly, keywords are currently defined as being updated at checkout time, but I suspect updating them at commit time is or was considered a feature by some, since it means that no update is needed after a commit of newly added keywords to have the key-value substituted correctly locally.  If this is not documented behavior, then it probably should be.

Derek Robert Price <dprice>
Project Administrator
Fri 04 Nov 2005 04:13:19 PM UTC, comment #2: 

Hi Derek -

You wrote, "Ignoring the user's action is almost certainly not the correct course of action here".  But that is exactly what CVS is doing right now, if you commit a change with just keyword content changed, CVS ignores's the user intent after returning the keyword modified file to CVS user. This is not at CHECKOUT time as you wrote but when the file is returned back to the user on COMMIT. Clearly that violates the contract you are referring to? If the intent is to honor user's action, then the CVS server is obligated to change whatever the user specifes in the keyword line just like normal content? Can they do that currently ? The answer is NO. The user can not change for example the revision number, CVS server will over-write that on COMMIT (not checkout). This completely baffles a CVS users as the revision is bumped up but the changes are wiped out from user sandbox on COMMIT. How does this not lead to ignoring the user's action?

If you read the patch description, the suggested behaviour is optional. If the CVS user doesn't want to change the behaviour they don't have to. We would request you to please reconsider your decision.

Best Regards,
Rahul Bhargava

Rahul Bhargava <b_rahul>
Fri 04 Nov 2005 03:21:50 PM UTC, comment #1: 

I do not think it is CVS's place to second-guess the user here.  It would be one thing if this was happening automatically due to some action on CVS's part, but if the user altered the keyword contents, then we must assume they intended to, regardless of the fact that a normal checkout would overwrite the change.  A checkout using -kb or -ko could still retrieve the committed data.  Ignoring the user's action is almost certainly not the correct course of action here.

I'm closing this issue, but thank you for the suggestion.

Derek Robert Price <dprice>
Project Administrator
Fri 28 Oct 2005 09:28:53 PM UTC, original submission:  

The cvs server will automatically (or, more accurately, as part of the
update run that automatically happens after a commit) expand valid keyword strings in a text file as part of the commit operation. In other words it is the responsibility of the 'server-side' of cvs to fill in the appropriate values for the RCS keywords.

Currently where this breaks down is when the end-user modifies the content of the RCS keywords. For example, let's say a file contains:

File : foo
Line 1: $Id$

On the initial commit, the server returns the file (if keyword expansion is not turned off) and expands Id as -

File: foo
$Id: id,v 1.1 2005/10/28 18:39:46 userX Exp $

If the user accidentally or intentionally changes just the expanded value (no other change) on the client-side to:

File: foo (revision 1.1 in user sandbox)
$Id: id,v 1.9 3005/10/28 18:39:46 userYYY Exp $

and then does a commit, the CVS server as it stands today will be happy to commit a new version but as part of the update run that automatically happens after a commit, will re-expand the keyword and the CVS client will have the following file in the sandbox :

File: foo
$Id: id,v 1.2 2005/10/28 18:46:18 userX Exp $

In other words the user's intent of modifying the keyword content is not honored. This make sense as keyword expansion is owned by the server and a client can not wily-nilly change the keyword content (between the two $, with the exception of $Log$ keyword of-course)

However, this also causes spurious revisions to be created in the CVS repository and this revision is not what the end user expects. The revision metadata within the $..$ keyword expansion is controlled by the server and as seen in the above example, the user can't change it even though his attempt to do so results in a new revision being committed.

We have come across large CVS repositories where lots of such spurious revisions (no content change except keyword) exist on several files. This causes problems downstream when for example doing branch merges, as differences on just keyword expansion can cause the file to be treated as changed.  Creating new revisions is an important step. It has consequence on downstream tools. Per-user stats (how may commits by a given user, for example) are impacted when revisions are created. One way to deal with this problem is to turn off keyword expansion (as available in 1-12-XX feature tree or use -ko for every file that has keywords), but that is a sledgehammer approach that denies the benefits of RCS keywords to the end users.

The patch (off the 1-11-21 tree) that we have attached addresses this bug/mis-feature. It ensures keyword expansion will not  cause  spurious revisions that differ just in keyword content that the CVS server inserts between $..$ markers. With the patch, a CVS user can still use keyword expansion as expected, but
during the file classification phase of a CVS commit, the patch kicks in and triggers a keyword-sensitive
diff algorithm, that we have developed, which will ignore RCS/CVS keyword differences during this phase.
This reduces spurious revision creation. The patch deals with all keywords except the logmsgs after a $Log$ keyword.

The patch uses a space-time efficient diff'ing algorithm that does not require the entire file
to be read into memory to do the keyword canonicalization. It operates on a block at
a time. The algorithm does not need to make multiple passes. As described in the
src comments, the algorithm is implemented as a finite state machine that tracks the
state across block comparisons and can deal with keywords straddled across block
boundaries etc. Right now, the patch is turned on by default. This behavior can be
controlled via a new option that we have added to CVSROOT/config: "IgnoreKeywordDiffsAtCommit".
If set to "no" it will switch back to the old semantics, just in case someone needs that.
The patch also builds a new executable "kwdiff" that can be used for comparing checked-out files
such that keyword differences can be ignored. This is useful when comparing files
between two users each with their own sand-box. It is also used for unit testing the implementation(see below).

As part of the patch, we have added new tests to We have also fixed testcases
that were expecting the old behavior (spurious revisions). To test the algorithm for every
possible corner case, we have also devised a unit test that generates millions of test file
permutations. These are perl based and we didn't integrate them into as it would
change the standalone nature of the script. Memory tools like valgrind have been run on the patch to ensure there are no memory errors.

In the future, it may make more sense to create a unit test
subdir for testing components of cvs (as opposed to the full cvs executable), in which case the kwdiff unit tests
could be checked in there.

This patch enables the keyword sensitive diff algorithm only during commits, but
the CVS community may want to consider it for updates, especially to reduce unnecessary
keyword related conflicts during branch merges. Given how the code is structured it is
relatively easy to turn on and off the keyword canonicalization algorithm for
other cases beyond commits. Today CVS reads the entire RCS file into memory for many
operation. This increases the footprint of the CVS process and can be a scalability bottleneck.
The file chunk management code in kwfilecmp.c shows that it is possible to operate on a
block basis, thus bounding the footprint of the cvs server process, regardless of the size of the file being processed.
Perhaps, in the future, this module could be re-used for eliminating the need to read in the
entire file into memory for many CVS operations.

The patch has been developed off the 1.11.21 branch.  It can easily be enhanced
 for the feature branch to take into account local keyword aliases. This will require changes to a single function ( is_valid_keyword).

Attached bzip2 file contains the kwdiff.patch (actual patch) and the kwdiff-unit-test/ subdir.

We hope the CVS community will find the patch useful.

Rahul Bhargava <b_rahul>


(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #7473:  kwdiff.tbz2 added by b_rahul (14KiB - application/octet-stream - Tar with patch file etc)


Depends on the following items: None found

Items that depend on this one: None found


CC list is empty


There are 100 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.




Follow 7 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2005-11-22 patjr Carbon-Copy- Added patjr
2005-11-09 b_rahul Carbon-Copy- Added b_rahul
2005-11-09 hmuller_adm Carbon-Copy- Added hmuller_adm
2005-11-04 dprice StatusNone Wont Do
    Open/ClosedOpen Closed
2005-10-28 b_rahul Attached File- Added kwdiff.tbz2, #5393
    Carbon-Copy- Added -email is unavailable-

Back to the top

Powered by Savane 3.9