Concurrent 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: |
Derek Robert Price <dprice>![]() |
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>![]() |
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:
EITHER
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!
OR
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 <rahul@wandisco.com> writes:
I have known sites which favored this method of
CVS should never discard what might be important text
Had an admin configured your new option, they
Or, do I still misunderstand the feature?
A user is looking at a file and accidentally
my $VERSION =
It should be noted that this 'spurious' difference
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$
My feeling is that a careless user needs to accept
However, I suppose that I can see where an
So, if you want to propose a contrib/kwdiff
I hope I have understood the case correctly. |
Mark D. Baushke <mdb>![]() |
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>![]() |
Tue 15 Nov 2005 09:26:10 PM UTC, comment #13: FEATURE branch already has the ability to disable
To disable it totally, use the CVSROOT/config line:
KeywordExpand=i
to disable just the expansion of $Id$ and $Header$, use
KeywordExpand=eId,Header
So, a CVS administrator who wants to use $Id: ... $ lines
KeywordExpand=eId
and not worry about it as all modules in the CVS project
|
Mark D. Baushke <mdb>![]() |
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:
DefaultKeywordMode=ko
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>![]() |
Tue 15 Nov 2005 04:32:03 PM UTC, comment #11: Derek Wrote:
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 ?
Regards,
|
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>![]() |
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>![]() |
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.
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] http://ximbiot.com/cvs/manual/cvs-1.11.21/cvs_12.html#SEC97
[2] http://ximbiot.com/cvs/manual/cvs-1.12.13/cvs_12.html#SEC101
|
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>![]() |
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>![]() |
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 <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>![]() |
Fri 28 Oct 2005 09:28:53 PM UTC, original submission:
THE PROBLEM
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
On the initial commit, the server returns the file (if keyword expansion is not turned off) and expands Id as -
File: foo
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)
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
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 FIX
The patch uses a space-time efficient diff'ing algorithm that does not require the entire file
As part of the patch, we have added new tests to sanity.sh. We have also fixed testcases
In the future, it may make more sense to create a unit test
This patch enables the keyword sensitive diff algorithm only during commits, but
The patch has been developed off the 1.11.21 branch. It can easily be enhanced
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> |
Depends on the following items: None found
Items that depend on this one: None found
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 | Status | None | ![]() |
Wont Do |
Open/Closed | Open | ![]() |
Closed | ||
2005-10-28 | b_rahul | Attached File | - | ![]() |
Added kwdiff.tbz2, #5393 |
Carbon-Copy | - | ![]() |
Added -email is unavailable- |
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.