Skip to content

Conversation

@qyot27
Copy link
Contributor

@qyot27 qyot27 commented Feb 9, 2026

Addresses #58 by setting up an option to copy the entire disc.

Presumably, cdda_disc_firstsector and cdda_disc_lastsector are the appropriate functions. I'm not sure if there needs to be checks accounting for the offset or whether those function names are extremely literal and aren't subject to needing offset adjusting.

The name of the option and the short invocation are fully open to bikeshedding. I just tried to find names which made sense or weren't being used already.

@rocky rocky requested review from boretom, enzo1982, eshattow, hvr, pbatard, pjcreath, rocky and scdbackup and removed request for rocky February 9, 2026 00:50
@rocky
Copy link
Collaborator

rocky commented Feb 9, 2026

Looks good to me. Thanks!

I'll leave this open for a few days in case any others care to comment. If no further comments, I will merge this before the end of the week.

@eshattow
Copy link
Collaborator

eshattow commented Feb 9, 2026

The description in usage does not seem correct to me, if a span is specified this would disable the span. The usage advice does not say that.

@rocky
Copy link
Collaborator

rocky commented Feb 9, 2026

The description in usage does not seem correct to me, if a span is specified this would disable the span. The usage advice does not say that.

@eshattow: Thanks for the observation. I suspect it might be helpful if you suggested a change in wording. Thanks in advance.. Nevermind, I see a change is in the works... Is this okay?

@qyot27
Copy link
Contributor Author

qyot27 commented Feb 12, 2026

I've pushed a minor cosmetic change to make the previous commit a bit more readable. Let me know if any/all of the commits should be squashed or left separate.

Copy link
Collaborator

@eshattow eshattow left a comment

Choose a reason for hiding this comment

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

Any additional objection I have is overly complex to implement, i.e. introducing a special span token instead of a dedicated commandline flag argument. I think in some situations "copy all sectors" is not a correct description when the span is all sectors yet nothing is copied. If I was a user wanting to copy all sectors then that language makes it sure easier to find what I want to do exactly. LGTM this is easy to understand and readable.

@rocky
Copy link
Collaborator

rocky commented Feb 12, 2026

Any additional objection I have is overly complex to implement, i.e. introducing a special span token instead of a dedicated commandline flag argument. I think in some situations "copy all sectors" is not a correct description when the span is all sectors yet nothing is copied. If I was a user wanting to copy all sectors then that language makes it sure easier to find what I want to do exactly. LGTM this is easy to understand and readable.

Thanks for the review and sharing your detailed thoughts. Feel free to note this as an issue (or better fix in a PR). You could also note this as a Discussion item if you want.

@rocky rocky merged commit 6c0a46b into libcdio:master Feb 12, 2026
2 checks passed
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.

3 participants