This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/compress-reader-cannot-open-a-file-in-bgzf-format-is-this-a-bug/6262/7

Spec would be nice :)

Hi @Sija

The current test is the following link. It already contains tests on Extra field.

https://github.com/crystal-lang/crystal/blob/989b7fb31451951696ea8c2aaf3e6f0cc01d56f0/spec/std/gzip/gzip_spec.cr

Here we use Gzip::Writer to create a gzip stream with an Extra field.

  Gzip::Writer.open(io) do |gzip|
    header = gzip.header
    header.modification_time = SAMPLE_TIME
    header.os = SAMPLE_OS
    header.extra = SAMPLE_EXTRA
    header.name = SAMPLE_NAME
    header.comment = SAMPLE_COMMENT

    io.bytesize.should eq(0)
    gzip.flush
    io.bytesize.should_not eq(0)

    gzip.print SAMPLE_CONTENTS
  end

The created gzip stream is then read using Gzip::Reader.

    Gzip::Reader.open(io) do |gzip|
      header = gzip.header.not_nil!
      header.modification_time.should eq(SAMPLE_TIME)
      header.os.should eq(SAMPLE_OS)
      header.extra.should eq(SAMPLE_EXTRA)
      header.name.should eq(SAMPLE_NAME)
      header.comment.should eq(SAMPLE_COMMENT)

      # Reading zero bytes is OK
      gzip.read(Bytes.empty).should eq(0)

      gzip.gets_to_end.should eq(SAMPLE_CONTENTS)
    end

The original code passed the test. Because it created an incorrect stream with XLEN size of one byte instead of two bytes and read it the wrong way.

This pull request failed the test the first time because I was only correcting the reading XLEN. So I also corrected the writing XLEN as well and now it passes the test.

Even then, should we really add another test? If so, what kind of test should we add?

One idea might be to have a hard-coded slice and check that it can be read by Gzip::Reader.

@oprypin Thanks for linking the issue. I think this also fixes the issue (#8933)

There should be at least one test that ensures correctly reading an extra field, and one for correctly writing an extra field.
Each test should fail without this patch, and succeed with it.
If the existing test succeeds with and without this patch, it's insufficient. It just test that reading and writing are implementing the same format, but not that this is correct.

So it's probably a good idea to include a gzipped file with extra field as a test fixture and make sure we can read it and produce the same file when writing.

@straight-shoota
Understood. Now I would like to add a test code that fails before the change and passes after the change.

(1) Add a small file test.gz with extra fields to spec/std/data .

https://github.com/crystal-lang/crystal/blob/master/spec/std/data

(2) Write some test code like the following

Is my understanding correct?

I created a draft Gzip file for testing.

C code to generate the Gzip file
https://gist.github.com/kojix2/bd9523fdd3c1e7f8be4ce2c91fd17435

Generated gzip file
test.gz