[Bug]: UpdateMode::Update not working as expected, throwing exception

Gohico commented 5 months ago

bit7z version

4.0.x

Compilation options

No response

7-zip version

v23.01

7-zip shared library used

7z.dll / 7z.so

Compilers

MSVC

Compiler versions

VS2022

Architecture

x86_64

Operating system

Windows

Operating system versions

Win11

Bug description

Hi, While trying to update an existing file within a zip the bit7 lib throws a wrong 'file exits' exception. It seems the 'UpdateMode::Update' gets ignored.

Its easy to reproduce via:

    Bit7zLibrary lib{ "7z.dll" };
    BitArchiveWriter archive{ lib, BitFormat::Zip };
    archive.setUpdateMode(UpdateMode::Update);
    archive.addFile("D:\\OFL.txt");
    archive.compressTo("D:\\Sirin.zip");

I also see the same behavior with the compressor BitFileCompressor and compressor.setUpdateMode(UpdateMode::Update).

The files mentioned in the example exists, can be edited (no permission issue) and I can do all things manually (via 7zip).

Steps to reproduce

Get a zip file from somewhere with at least a single file inside (Sirin.zip as example)
Extract the file and change the content (OFL.txt)
Try to update the file inside the zip with the updated version

Sample:

    Bit7zLibrary lib{ "7z.dll" };
    BitArchiveWriter archive{ lib, BitFormat::Zip };
    archive.setUpdateMode(UpdateMode::Update);
    archive.addFile("D:\\OFL.txt");
    archive.compressTo("D:\\Sirin.zip");

Throws a Failed to create the output file: file exists exception text

Expected behavior

The OFL.txt files within the zip gets updated

Relevant compilation output

Exception text: Failed to create the output file: file exists

Code of Conduct

[X] By submitting this issue, I agree to follow bit7z's Code of Conduct

rikyoz commented 5 months ago

Hi!

While trying to update an existing file within a zip the bit7 lib throws a wrong 'file exits' exception. It seems the 'UpdateMode::Update' gets ignored.

Its easy to reproduce via:
    Bit7zLibrary lib{ "7z.dll" };
    BitArchiveWriter archive{ lib, BitFormat::Zip };
    archive.setUpdateMode(UpdateMode::Update);
    archive.addFile("D:\\OFL.txt");
    archive.compressTo("D:\\Sirin.zip");

This is actually the expected behavior: the UpdateMode is taken into account only when you open an already existing archive in BitArchiveWriter's constructor.

Basically, when you create a new BitArchiveWriter without passing a path to an existing archive (e.g., BitArchiveWriter archive{ lib, BitFormat::Zip } vs BitArchiveWriter archive{ lib, "D:\\Sirin.zip", BitFormat::Zip }, you're telling bit7z that you want to create an entirely new archive, hence the exception in compressTo.

Using BitArchiveWriter archive{ lib, "D:\\Sirin.zip", BitFormat::Zip } should fix the issue.

I understand this behavior might be confusing, so I'll evaluate fixing it in the next version of the library. Thank you for pointing it out.

I also see the same behavior with the compressor BitFileCompressor and compressor.setUpdateMode(UpdateMode::Update).

This issue, instead, is not expected and I cannot replicate it.

Gohico commented 5 months ago

Brilliant!

Thank you very much for the quick reply. For whatever reason I had not seen the ctor with the path - that fixes my problem. I think I hadn't even considered the alternative constructor because I had seen the UpdateMode property - (setUpdateMode) - so I think I never scrolled up fully.

Generally speaking it's the first time I'm using your lib and this is one of the cleanest and most consistent interfaces I've seen in public projects. So thank you very much for this.

I don't think its necessary to change the API - but it would be nice if this use case could be added to the samples in the documentation.

rikyoz commented 5 months ago

Thank you very much for the quick reply. For whatever reason I had not seen the ctor with the path - that fixes my problem. I think I hadn't even considered the alternative constructor because I had seen the UpdateMode property - (setUpdateMode) - so I think I never scrolled up fully.

No problem!

Generally speaking it's the first time I'm using your lib and this is one of the cleanest and most consistent interfaces I've seen in public projects.

Thank you!

So thank you very much for this.

You're welcome!

I don't think its necessary to change the API - but it would be nice if this use case could be added to the samples in the documentation.

I'm not thinking of changing the API but rather its behavior in this specific case, since it seems a reasonable behavior. I still need to evaluate the implications of such change, though.

As a side note, there's the BitArchiveEditor class, which extends BitArchiveWriter, and it's specifically meant to be used for modifying existing archives. For your use case, the code would be as follows:

Bit7zLibrary lib{ "7z.dll" };
BitArchiveEditor archive{ lib, "D:\\Sirin.zip", BitFormat::Zip };
archive.setUpdateMode(UpdateMode::Update); // Cannot be UpdateMode::None, defaults to UpdateMode::Append.
archive.addFile("D:\\OFL.txt");
// Note: applyChanges does not need the path of the output archive, like compressTo,
// since you're editing the archive you passed to the BitArchiveEditor's constructor.
archive.applyChanges();

rikyoz / bit7z