Implement gzungetc

[brian-pane]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libz-rs-sys/src/gz.rs	98.66%	1 Missing ⚠️

Flag	Coverage Δ
fuzz-compress	?
fuzz-decompress	?
test-aarch64-apple-darwin	88.43% <98.66%> (+0.68%)	⬆️
test-x86_64-apple-darwin	86.17% <5.33%> (-0.43%)	⬇️
test-x86_64-unknown-linux-gnu	88.00% <4.00%> (-0.52%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
libz-rs-sys/src/gz.rs	92.11% <98.66%> (+0.84%)	⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[folkertdev]

where does this 1 come from? I'm having some trouble mapping this back to the C original

    if (state->x.next == state->out) {
        unsigned char *src = state->out + state->x.have;
        unsigned char *dest = state->out + (state->size << 1);
        while (src > state->out)
            *--dest = *--src;
        state->x.next = dest;
    }

[brian-pane]

Thanks for catching that. I misread the C original as handling the case where there is 1 free byte at the end of the buffer, but it actually covers all cases between 0 and state->size << 1 free bytes. I'll update the PR to shift the content to the very end of the buffer to match the C version.

[folkertdev]

I'm still not confident that the current implementation is right, especially given that the test did not flag the problem earlier.

This bit of logic is tricky, so while I appreciate the from-first-principles reasoning, I think it would also be useful to add why/how this maps back to the C source code, and to make sure that the tests properly exercise this logic.

I also tried running the test locally but with use libz_sys::*; at the top, overwriting the imports with the C version of those functions, and that fails (I commented out the line using gzbuffer, which libz_sys does not export, it then fails on assert_eq!(unsafe { gzungetc('-' as c_int, file) }, -1);, which apparently returns 45 using the C version.

[brian-pane]

I think I can address why the test didn't flag the problem in the previous version: it was a performance regression, rather than a functionality bug. zlib-ng shifts the content as far right as possible, which avoids a quadratic cost if the caller invokes gzungetc repeatedly. My original implementation, which shifted just one byte to the right, had the same functional result (the character passed to gzungetc was pushed onto the front of the buffered content) but would be very inefficient if gzungetc was called repeatedly in that state.

[brian-pane]

gzbuffer plays an important role in the test case: it ensures that the buffer is small enough so the various gzread and gzungetc operations can fill and drain it. The test case sets up 16 bytes of data in the buffer before the line assert_eq!(unsafe { gzungetc('-' as c_int, file) }, -1); to exercise the full-buffer case. But without calling gzbuffer, the default buffer size in zlib-ng is something like 128KB. So the gzungetc call on that line will succeed, returning 45 (ascii for -).

I'll add some more comments to the test code to document the buffer arithmetic that the tests are using.