feat: make signs significant for byte offsets

[ErichDonGubler]

I do NOT consider this ready to merge because documentation doesn't teach about signs and the new distinction that now exists between byte COUNTS, where a positive integer is the goal (like before), and byte OFFSETS, where sign and relativity are now significant. I'm not sure what the best approach to teaching about this would be, and wanted to file this PR in this incomplete state to get feedback and thoughts.

What these changes allow is for relative and, in particular, negative byte offsets to be significant and useful in the context of seeking backwards from a seekable source like a file. For instance, if one wishes to view the last block of a file to view a footer (not uncommon in custom binary formats), it's now possible to do:

$ hexyl --skip=-1block

...instead of whatever gymnastics users may have done with other commands to calculate the backwards offset as a positive forward offset manually. ~~N.B. that this, unfortunately, will fail:

$ hexyl --skip -1block

...because clap interprets -1block as a separate, unrecognized flag.~~

EDIT: a solution has been found for allowing offset flags and their values to be separate and still parse negative values correctly via allow_hyphen_values, but I have a concern about diagnostics.

[sharkdp]

This looks great - thank you very much (again)!

I'm not sure what the best approach to teaching about this would be

If a command-line option is harder to understand, I am a fan of (potentially longer) descriptive --help texts that show a few examples. It's often easier to adapt an example to your own use case as compared to interpreting a formal specification.

it's now possible to do:

$ hexyl --skip=-1block

:tada:

...because clap interprets -1block as a separate, unrecognized flag.

Maybe this would help: https://docs.rs/clap/2.33.1/clap/enum.ArgSettings.html#variant.AllowLeadingHyphen ? There is also "allow negative numbers", but I'm afraid that won't work here due to the trailing unit.

[ErichDonGubler]

I added a commit that uses allow_hyphen_values for --skip and --display-offset, but I'm blanching at the diagnostics regression that happens here:

$ cargo run -- --skip --block-size 2
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target\debug\hexyl.exe --skip --block-size 2`
Error: The system cannot find the file specified. (os error 2)
error: process didn't exit successfully: `target\debug\hexyl.exe --skip --block-size 2` (exit code: 1)

...as opposed to the more helpful text before:

$ cargo run -- --skip --block-size 2
   Compiling hexyl v0.8.0 (C:\Users\K0RYU\workspace\hexyl)
    Finished dev [unoptimized + debuginfo] target(s) in 1.40s
     Running `target\debug\hexyl.exe --skip --block-size 2`
error: The argument '--skip <N>' requires a value but none was supplied

USAGE:
    hexyl.exe --block-size <SIZE> --border <STYLE> --color <WHEN> --skip <N>

For more information try --help
error: process didn't exit successfully: `target\debug\hexyl.exe --skip --block-size 2` (exit code: 1)

Since these changes are forwards-compatible anyway, maybe it'd be best to defer thinking about allow_hyphen_values until somebody complains and provides a better case than I can here?

[ErichDonGubler]

I've also added some (teeny) docs indicated where negative values are allowed. There's not a lot of complexity in thinking about just those -- positive relative offsets aren't really exposed anywhere (yet), but they will be when and if range notation is accepted in the form I understand it (i.e., --range 1block:+2block).

[sharkdp]

I added a commit that uses allow_hyphen_values for --skip and --display-offset, but I'm blanching at the diagnostics regression that happens here:

Due to allow_hyphen_values being activated globally, clap interprets --block-size as the VALUE for the --skip option:

--skip --block-size 2

This leaves 2 as the positional argument, causing hexyl to try and open the file 2.

This sort of behavior is probably why clap has an "allow negative numbers" option in addition. Maybe we should just go with the --option=… style (in the documentation - which we do already) and disable "allow_hyphen_values" again?

[ErichDonGubler]

Yeah, I'm very much in favor of deferring this parsing problem for later, since whatever solution if viable should in my mind have no regressions. Meanwhile, this problem is perfectly forwards compatible for now. It's a lot easier to simply document the behavior that is very easy to work around right now.

[sharkdp]

Ok, so let's remove .allow_hyphen_values(true) for now... or did I misinterpret your answer? :smile:

[ErichDonGubler]

@sharkdp: You interpreted correctly, sorry, I just haven't made this a priority yet what with starting a new job. I should be able to fix this in the next few days, though.

[sharkdp]

There is absolutely no hurry (and never will be). Thank you for the updates.

[ErichDonGubler]

I've removed the commit enabling allow_hyphen_values and rebased into a single commit atop master. I think this is good to go!

[ErichDonGubler]

Looks like CI is failing for x86_64-pc-windows-gnu because the target isn't installed? Seems unrelated to what is being changed here.

[sharkdp]

Looks like CI is failing for x86_64-pc-windows-gnu because the target isn't installed?

This has happened several times and I'm not entirely sure why. I fixed it for now by switching (back) to *-msvc instead of *-gnu.

[sharkdp]

Thank you for the updates!