Skip to content

Conversation

@OwenCochell
Copy link
Contributor

Hello again,

After some production testing, I have found an issue with the flac fix command I included in my previous change. Long story short, it is including an extra set of quotes in the path when preforming a fix, meaning the flac command will be given a non-existent path. This causes a pretty big error message when attempting to fix flac files.

This PR fixes the issue, and makes the flac fix command align better with the others. Let me know if there are any issues here, hopefully this fix should be very simple.

Thanks,
Owen Cochell

@geigerzaehler
Copy link
Owner

LGTM! Do you have the time to add a test for this as well?

Copy link
Contributor Author

@OwenCochell OwenCochell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I have made the change to the test cases to make sure flac fixing is correct.

However, I had to change the truncated flac file. It had an MD5 hash built in that caused decoding to fail. After the fiac file was fixed, the new hash did not match with the old one in the file, which causes the decoder to fail and the test does not complete. To fix this, I have zeroed out the hash, meaning the flac command will not complain about the integrity issue and allows the fix operation to complete correctly.

The implication of this change is your deliberately broken flac file no longer has a proper hash. The biggest side effect from this is the flac checker will now emit a warning about the unset MD5 hash, which IMO is not a big deal as this file emits warnings anyway. After the fix, the MD5 hash is properly set and any further check commands report no issues. All test cases are passing ok.

This is a relatively big change to your test files, so I understand if this is not up to standard. A possible alternative is I could craft a valid flac file with an unset MD5 hash and use that instead for this test. This would prevent the need for existing test files to be altered. If that sounds good let me know and I can get that going.

Thanks,
Owen Cochell

@geigerzaehler geigerzaehler merged commit adf5a59 into geigerzaehler:main Nov 4, 2025
8 of 9 checks passed
@geigerzaehler
Copy link
Owner

Great job solving the testing issue. I don’t mind the changes at all. For testing purposes they are totally fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants