Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow negative values for --display-offset #145

Closed

Conversation

ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Nov 16, 2021

--display-offset has had an odd relationship with negated inputs over its lifetime. Historical behaviors (and the addition being proposed here) might be easiest to understand in a tabular format.

When Behavior summary Negative offset behavior
First implemented in #72. Absolute position, positive values only. Disallowed.
Refined to allow negatives in #99. Absolute position, including then-new semantics for negatives. Allowed, end offset backwards from EOF (and so only usable on files), prompting #112 for poor diagnostics at the time.
Actual compatibility with xxd desired, made an offset of --skip in #119. Relative offset based on --skip. Disallowed.

This PR proposes to make negative values for --display-offset meaningful again by allowing them again when --skip is specified, adding a negative value to --skip if provided. To use the same format as above:

When Behavior summary Negative offset behavior
This PR. Relative offset based on --skip. Allowed when --skip is specified; underflow results in an error.

More detailed overview of the design and rationale:

  • This stays consistent with current behavior -- the final display offset retains its concept as "--skip + --display-offset".

  • This adds an error case for the display offset underflowing when added to the unsigned --skip offset:

    hexyl --display-offset=-21 --skip 20 .\some_file
    Error: invalid value when applying `--display-offset` to `--skip`
    
    Caused by:
        base (20) - offset (21) < 0
    
  • Bonus: the error case where offset addition overflows now has a nice diagnostic:

    TODO
    
  • If --skip is not specified, it is an error, as before:

    Error: failed to parse `--display-offset` arg "-1" as byte count
    
    Caused by:
        negative offset specified, but only positive offsets (counts) are accepted in this context
    

Resolves #112.


PR checklist:

  • Get deets from PR OP into commit messages for history's sake.
  • Tests

TODO: describe design, why it's interesting, and how much it niggles my brain that I'm considering this our best option.
@sharkdp
Copy link
Owner

sharkdp commented Nov 16, 2021

Sounds great. Thank you very much for looking into this.

@sharkdp
Copy link
Owner

sharkdp commented Nov 13, 2022

I'm closing this due to inactivity. Please feel free to comment in case it should be re-opened.

@sharkdp sharkdp closed this Nov 13, 2022
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.

Argument --display-offset does not accept negative numbers, contrary to its helptext description.
2 participants