The function _ripIfNotRipped in cd.py is broken

[drjaska]

Whipper looks to have a design which can continue ripping CDs which have been partially ripped when there are .flac or .wav files in the right path without a .log file to indicate a finished rip.

However the current logic is severely missing important information.

Currently the function _ripIfNotRipped takes an argument for the track number to rip. It creates a new trackResult object for it and figures out what the file path for the track should be. If it finds a flac/wav file already there then it compares the found file's CRC32 to.. a non-existant CRC32 of None.

As the CRC32 checksum comparison is always a mismatch even for good tracks all tracks are always reripped and no previous tracks are ever reused.

For the function to function it should know the CRC32 to expect for the current track, without ripping the track once again.

I do not know how this should be fixed.

The easy option would be to not do CRC32 checksum comparisons and assume that all existing files are good copies, they should still be verified but AccurateRip checksums but in the case that those are absent we would be blindly trusting the existing files.

[drjaska]

If we would just essentially skip the check with the following patch

diff --git a/whipper/common/program.py b/whipper/common/program.py
index 46d28f1..bc46a15 100644
--- a/whipper/common/program.py
+++ b/whipper/common/program.py
@@ -556,9 +556,15 @@ class Program:
             else:
                 raise
 
-        ret = trackResult.testcrc == t.checksum
-        logger.debug('verifyTrack: track result crc %r, file crc %r, '
-                     'result %r', trackResult.testcrc, t.checksum, ret)
+        if trackResult.testcrc is not None:
+            ret = trackResult.testcrc == t.checksum
+            logger.debug('verifyTrack: track result crc %r, file crc %r, '
+                         'result %r', trackResult.testcrc, t.checksum, ret)
+        else:
+            ret = True
+            logger.debug('verifyTrack: No track result crc, file crc %r, '
+                         'skipping verification with a pass', t.checksum)
+
         return ret
 
     def ripTrack(self, runner, trackResult, offset, device, taglist,

File checksums remain the same and I don't notice other notable differences than these in the log file:

   8:
     Filename: filepathhere
-    Peak level: 0.951263
+    Peak level: 0.0
     Pre-emphasis:
-    Extraction speed: 6.0 X
-    Extraction quality: 100.00 %
-    Test CRC: 78A7357A
-    Copy CRC: 78A7357A

I'll later see about peak calculation and CRC stuff. We should probably note that the track was pre-existing.

[drjaska]

Getting the peak seems harder but copy and test CRCs can probably be read from t.checksum in there as if it is a whipper flac it has already been verified at least once already. If it isn't a whipper flac then user error?

[drjaska]

I don't understand why in _ripIfNotRipped there is a comment # FIXME: optionally allow overriding reripping.

Is the CRC comparison supposed to work and the comment is there to say that it should be possible to always/never rerip?