Item945: RcsLite corrupts file's history

Priority:	CurrentState:	AppliesTo:	Component:	WaitingFor:
Urgent	Closed	Engine	RcsLite

Steps to reproduce:

1. Adjust $Foswiki::cfg{StoreImpl} to RcsWrap
2. Create a topic and make some changes forcing new revisions
3. Check history. It works.
4. Change $Foswiki::cfg{StoreImpl} to RcsLite
5. Make at least one more revision to the topic
6. Check history. It works. (but the history file is already corrupted)
7. Change $Foswiki::cfg{StoreImpl} back to RcsWrap
8. Check history. BOOM

If you try to run rlog at command line, you get a error like:

$ rlog data/Sandbox/TestTopic0.txt

RCS file: data/Sandbox/TestTopic0.txt,v
Working file: data/Sandbox/TestTopic0.txt
head: 1.4
branch:
locks: strict
access list:
symbolic names:
keyword substitution: o
total revisions: 4;     selected revisions: 4
description:
none
rlog: data/Sandbox/TestTopic0.txt,v: bad diff output line:
@
rlog aborted

RcsLite adds a blank line before @ character (@ marks the end of a revision data).

Thank you very much, Juraj, for giving this feedback!

(Refactored to make SMART. Check revision 11 to see all previous trace.)

-- GilmarSantosJr - 07 Feb 2009

I added a unit test (test_Item945 in RcsTests.pm) that checks compatibility of histories between RcsWrap and RcsLite, and it works fine. Without knowing exactly what changes you made to cause this failure there's not much more I can do. Gilmar, please take a look at that unit test and see if you can make it fail.

-- CrawfordCurrie - 08 Feb 2009

added Michael as he's indicated he had a similar issue.

Oh, and me too - I saw this last week! hopefully one of us will find time.

-- SvenDowideit - 08 Feb 2009

numRevisions works cause it calls rlog -h, that doesn't parse the entire file. I modified the test, so the created revisions are the same (2 from RcsWrap and 2 from RcsLite) and, after each new created revision, check history and revision info (to force the file to be parsed).

-- GilmarSantosJr - 08 Feb 2009

I have been using rcs lite for too long. As a consequence I plain had to throw away ALL of my ,v files as they were corrupted in ways I was not able to figure out how to repair them

That' said we might need a way to repair broken ,v files as people might get into the same situation as I was.

-- MichaelDaum - 08 Feb 2009

Don't throw away your ,v files, Michael! The corruption exposed by this task is an extra \n in some fields. I think we should provide a repair script along the fix to this issue (if we confirm the problem is always an extra \n).

-- GilmarSantosJr - 08 Feb 2009

I agree fix-it code should be possible, and will be desired - I think I'll be needing it in the next month or 2

good job on the unit tests

-- SvenDowideit - 08 Feb 2009

I found two issues with the code:

1. The stored log message always terminate with one, and only one, \n (even if the original message has multiple or none \n. This is the way rcs works. Verify at command line).
   • The \n is not part of the log message
   • The stored log message should match /@((\n?[^\n])+\n)?@/
     • rcs always stores log messages according to this regex, but it doesn't complain if it finds a message that doesn't match
2. RcsLite wrongly added an extra empty line for each transformation

The history file stores the latest revision unmodified and the previous ones as reverse diffs: apply the diff stored at revision head - 1 to head and you'll get head - 1 text. Apply diff stored at revision head - 2 to text of revision head - 1 and you'll get head - 2 text and so on. To add a revision, the old head text is compared to the new text and replaced by the diff. This is what I called transformation.

The fix script should:

• Canonicalize log messages
• Remove the extra empty line (if it exists) from revision diffs
  • We know that an empty line is extra based on the lines counter of the operation.

For now I improved the tests and fixed the issue. No time to work on the script. Feel free to develop it

-- GilmarSantosJr - 09 Feb 2009

In SVN-Change 2424 at the end of the _addChunk method the line

$out .= "\n";

was removed, but now there is a problem: RcsLite creates corrupt files (whenever a "d"-change is the last change in a revision): Example

text
@d1 1
a1 1
d3 1@

Now the newline is missing after "d3 1"; at least rlog says:

rlog: ./RcsTest.txt,v: bad diff output line: d3 1@
rlog aborted

I think in this method for "d"-changes we have to use:

$$out .= 'd' . ( $start + 1 ) . ' ' . $nLines."\n";

Because in my opinion after every "d"-change a newline is needed.

-- ChristianLudwig - 19 Feb 2009

Oh, thank you ChristianLudwig. I was starting to get crazy. I tried this on Lenny and OpenSolaris, and it failed. EugenMayer tried it on Intrepid, and it worked. I guess he just didn't make a d change, and this worked out.

I'll try to fix it properly, and also to fix the unit test too.

-- OlivierRaginel - 20 Feb 2009

We're still missing the migration script, but in fact, we might not need it. If you simply use RcsLite to make a new revision on your topic(s), it should rewrite it properly. And it should be able to read the buggy topics too.

So the script should simply be a loop on the topics, using the latest RcsLite, and writing it back.

I could write it, but I'd rather first wait for a few of the other players involved to confirm this works.

Patch resides in Foswikirev:2584 (did a few other commits to split things up).

-- OlivierRaginel - 21 Feb 2009

This all sounds scarily familiar; I had previously (back in TWiki 4.1 time) fixed several other similar errors. There really needs to be a more thorough unit test, that covers all the delta operators that RcsLite (and RCS itself) can generate.

-- CrawfordCurrie - 23 Feb 2009

Which was exactly why I re-wrote the unit test the way I did it.

Here, you can easily expand it so that it generates whatever you want, with whatever RCS implementation you want, by adding lines to the @historyItem945 array in UnitTestContrib/test/unit/RcsTests.pm

As I have no clue what they are, or how they are triggered, I only did what I could.

• First parameter is the implementation (remember, the tests will be run also by each implementation, and the resulted ,v will be compared too)
• Second parameter is the entire text countained in the topic, thus the diff will be made with the previous line
• Third and Fourth parameters are the user and comment
• Sixth parameter is the time

In brief, they will be passed as-is to the addRevisionFromText method of the implementation chosen as first parameter.

-- OlivierRaginel - 23 Feb 2009

I am closing this bug report and adding it to the 1.0.1 release fixes.

If there are additional tasks after 1.0.2 open a new report.

I do this to have a closed report in the release note. It is an urgent important bug to report fixed.

-- KennethLavrsen - 24 Feb 2009

Added repair script; tools/repair_rcs_lite.pl; see Item3212

-- CrawfordCurrie - 16 Jun 2009