Skip to content

Conversation

@strickyak
Copy link
Collaborator

It should not be different from the sector size
on other tracks.

Also a unit test that breaks when the bug exists.

FIXES #28

It should not be different from the sector size
on other tracks.

Also a unit test that breaks when the bug exists.

FIXES #28
@tlindner
Copy link
Member

I feel sectorsTrack0 needs better handling when unset. Which is the cause of the underlying problem.
I also feel adding the testing of the os9 command to librbftest was a step too far. My own idea of a fix is in PR #31.

@strickyak
Copy link
Collaborator Author

Tim, this bug was a regression, and as I pointed out on #toolshed, it breaks coco-shelf,
so I prioritized it, stayed up late, and wrote a simple one-line fix and simple demonstration test
to prevent further regressions.

Your correct response, if I did my work correctly, is to approve it,
so we check it in and fix the coco-shelf.

If you've got ideas how to improve the code, that can go in later.
You are making other changes, like re-ordering libraries, that I can't clearly say
this is right or not. And if those extra things contain bugs, and have to get rolled back,
then it regresses coco-shelf again.

That's why (1) the simplest-thing-possible fix and (2) other code refactorings & improvements,
should not be mixed up together in a single PR.

Is it the case there are 4 commits in your PR because the first was incorrect?

Requesting approval for my PR. Then we can discuss yours.

@tlindner
Copy link
Member

It is ok for me to ask for improvements to a PR. Nonetheless consider this approved, and I'll move on.

@tlindner tlindner merged commit 99b2412 into main May 27, 2025
2 checks passed
@strickyak
Copy link
Collaborator Author

Thanks, Tim. I know it's just a hobby and we're all in it for fun.

Still, it's best to (1) fix regressions immediately, especially when it breaks other infrastructure
(many of us depend on coco-shelf), and to (2) separate "The simplest fix possible" from further
refactorings, so that latter can roll back if it has problems.

I'll volunteer to adapt your PR for check in, now that refactorings are good,
unless you want to do it.

thanks again! -- strick9

strickyak added a commit that referenced this pull request May 27, 2025
This simplifies how the sectorsTrack0 variable is used,
giving it an "unset" value of 0.

It moves a test that forks the "os9 format"
command into a new file unittest/os9commandtest.c

Fixes the order of libraries, as needed in automated testing.

This PR was originally
#31
but the fundamental bug fix was made in
#29
so it has been adapted.
tlindner pushed a commit that referenced this pull request May 29, 2025
* fix Track0 sector size in formatting Hard Drive.

It should not be different from the sector size on other tracks.

Also a unit test that breaks when the bug exists.

FIXES #28

* Tim's refactor of track0 sectors for os9 format

This simplifies how the sectorsTrack0 variable is used, giving it an "unset" value of 0.

It moves a test that forks the "os9 format" command into a new file unittest/os9commandtest.c

Fixes the order of libraries, as needed in automated testing.

This PR was originally #31 but the fundamental bug fix was made in #29 so it has been adapted.
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.

os9 format Broken for hard drive (with -l65000)

3 participants