sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

Take advantage of skip behavior from #93 for `--seek` for non-`Seek` `Input`s #95

Closed ErichDonGubler closed 4 years ago

ErichDonGubler commented 4 years ago

Hard dependency on #93, less hard on #94.

Prior to this commit, Input::seek would fail under the following conditions:

  1. For most reasonable I/O failure conditions, like a file being moved or truncated, or the filesystem being interrupted, or...voodoo. /me waves hands in the air
  2. When attempting to call File::seek on a file backed by a pipe (i.e., a FIFO or socket), as noted by the ESPIPE error check.
  3. When called on a Stdin variant, because StdinLock doesn't implement Seek -- conceptually similar to case 2.

With the lib commit, 2 and 3 are changed to simulate a SeekFrom::Current(positive_value) by throwing away positive_value bytes by std::io::copying into std::io::sink. With the commit changing the binary, we use SeekFrom::Current (instead of SeekFrom::Start), since the "current" offset will always be 0 anyway. Speaking in terms of the command line, doing something like this failed before:

echo 'asdf' | hexyl --skip 2 # hoping to just print 'df'
# Errors out, because we can't `seek` on `Input::Stdin`

...but now it should work as intended.


This is technically a breaking change because it halves the upper range of accepted values for byte counts. I thought that fine under the assumption it will be rare for users to ever want to specify something in the upper half of u64's value range, especially considering that this can pave the way for having negative byte offsets (instead of just counts).

ErichDonGubler commented 4 years ago

I don't quite understand how the "only support seeking forward with a relative offset" would be triggered?

Uh, do you mean how we'd exercise the success path, or the error message with similar contents? The explanation I have will answer for both, so...I'll just get on with it:

Prior to this commit, Input::seek would fail under the following conditions:

  1. For most reasonable I/O failure conditions, like a file being moved or truncated, or the filesystem being interrupted, or...voodoo. /me waves hands in the air
  2. When attempting to call File::seek on a file backed by a pipe (i.e., a FIFO or socket), as noted by the ESPIPE error check.
  3. When called on a Stdin variant, because StdinLock doesn't implement Seek -- conceptually similar to case 2.

With the lib commit, 2 and 3 are changed to simulate a SeekFrom::Current(positive_value) by throwing away positive_value bytes by std::io::copying into std::io::sink. With the commit changing the binary, we use SeekFrom::Current (instead of SeekFrom::Start), since the "current" offset will always be 0 anyway. Speaking in terms of the command line, doing something like this failed before:

echo 'asdf' | hexyl --skip 2 # hoping to just print 'df'
# Errors out, because we can't `seek` on `Input::Stdin`

...but now it should work as intended.

EDIT: I've put the above into the OP, and I'll also put it into the binary commit too.

ErichDonGubler commented 4 years ago

When writing the error message, I tried to write the most concise and correct explanation...but that's a terrible heuristic for diagnostics. If there's something better, let's bend the curve! I'm not so sure I'm the best to come up with a descriptive error message, though, since I've gotten so close to the problem. I'll think about alternatives.

sharkdp commented 4 years ago

Speaking in terms of the command line, doing something like this failed before:

echo 'asdf' | hexyl --skip 2 # hoping to just print 'df'
# Errors out, because we can't `seek` on `Input::Stdin`

...but now it should work as intended.

This is the part I understood. But how could the "only support seeking forward with a relative offset" error be triggered by a user of the hexyl command line application?

ErichDonGubler commented 4 years ago

But how could the "only support seeking forward with a relative offset" error be triggered by a user of the hexyl command line application?

I can see why that's not clear from my answer. This error condition is only accessible from using hexyl::input::Input right now -- the binary's usage doesn't expose this error path. It may once negative byte offsets become a thing though!