0

I am copying files from one zip file to another in certain circumstances. I am wondering if there is a better way to do it than what I came up with:

using FileStream sourceFileStream = new FileStream(source.FileName, FileMode.Open);
using FileStream targetFileStream = new FileStream(target.FileName, FileMode.Open, FileAccess.ReadWrite);
using ZipArchive sourceZip = new ZipArchive(sourceFileStream, ZipArchiveMode.Read);
using ZipArchive targetZip = new ZipArchive(targetFileStream, ZipArchiveMode.Update);
ZipArchiveEntry sourceEntry = sourceZip.GetEntry(filePathInArchive);
if (sourceEntry == null) 
    return;
ZipArchiveEntry targetEntry = targetZip.GetEntry(filePathInArchive);
if (targetEntry != null) 
    targetEntry.Delete();
targetZip.CreateEntry(filePathInArchive);
targetEntry = targetZip.GetEntry(filePathInArchive);
if (targetEntry != null)
{
    Stream writer = targetEntry.Open();
    Stream reader = sourceEntry.Open();

    int b;
    do
    {
        b = reader.ReadByte();
        writer.WriteByte((byte)b);
    } while (b != -1);


    writer.Close();
    reader.Close();
}

Tips and suggestions would be appreciated.

Magnetron
  • 7,495
  • 1
  • 25
  • 41
a.rlx
  • 54
  • 6
  • 1
    Surely this will corrupt the file.. you go and write the "-1" you receive meaning "end of stream" onto the file transferred. Consider `while(true) .. if(b==-1)break;`? – Caius Jard Oct 07 '21 at 13:57
  • What do you mean by "better?" – Robert Harvey Oct 07 '21 at 14:05
  • 1
    Your code seems to be missing some braces. The `using` statements will work all the way up to `using ZipArchive targetZip`, at which point the only line of code that will be affected by the `using` statements is `ZipArchiveEntry sourceEntry = ...` Put in your braces. – Robert Harvey Oct 07 '21 at 14:06
  • 1
    We strictly know that target zip exists and doesn't creating new? – Auditive Oct 07 '21 at 14:08
  • 1
    @RobertHarvey Starting in C# 8, you don't need the braces, and the scope of `using` is until the end of the current scope. – Magnetron Oct 07 '21 at 14:15
  • @Magnetron: Good to know, but for clarity I think I would still put in the final brace pair. – Robert Harvey Oct 07 '21 at 14:17
  • 2
    Have you looked at [`Stream.CopyTo`](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.copyto?view=net-5.0) instead of copying by hand? – Magnetron Oct 07 '21 at 14:19
  • @RobertHarvey PS: the feature is called *using declaration* opposing to the classic *using statement*, that need braces. You can read more [here](https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-8#using-declarations) – Magnetron Oct 07 '21 at 14:22
  • Ah, yes. I see the semicolons. I've seen this before, just forgotten it was there. – Robert Harvey Oct 07 '21 at 14:23
  • Thank you for the tips, everyone. I was able to clean up and improve the code considerably. – a.rlx Oct 08 '21 at 06:44

2 Answers2

2

You can iterate each entry from source archive with opening its streams and using Stream.CopyTo write source entry content to target entry.

From C# 8.0 it looks compact and works fine:

static void CopyZipEntries(string sourceZipFile, string targetZipFile)
{
    using FileStream sourceFS = new FileStream(sourceZipFile, FileMode.Open);
    using FileStream targetFS = new FileStream(targetZipFile, FileMode.Open);

    using ZipArchive sourceZIP = new ZipArchive(sourceFS, ZipArchiveMode.Read, false, Encoding.GetEncoding(1251));
    using ZipArchive targetZIP = new ZipArchive(targetFS, ZipArchiveMode.Update, false, Encoding.GetEncoding(1251));

    foreach (ZipArchiveEntry sourceEntry in sourceZIP.Entries)
    {
        // 'is' is replacement for 'null' check
        if (targetZIP.GetEntry(sourceEntry.FullName) is ZipArchiveEntry existingTargetEntry)
            existingTargetEntry.Delete();

        using (Stream targetEntryStream = targetZIP.CreateEntry(sourceEntry.FullName).Open())
        {
            sourceEntry.Open().CopyTo(targetEntryStream);
        }
    }
}

With earlier than C# 8.0 versions it works fine too, but more braces needed:

static void CopyZipEntries(string sourceZipFile, string targetZipFile)
{
    using (FileStream sourceFS = new FileStream(sourceZipFile, FileMode.Open))
    {
        using (FileStream targetFS = new FileStream(targetZipFile, FileMode.Open))
        {
            using (ZipArchive sourceZIP = new ZipArchive(sourceFS, ZipArchiveMode.Read, false, Encoding.GetEncoding(1251)))
            {
                using (ZipArchive targetZIP = new ZipArchive(targetFS, ZipArchiveMode.Update, false, Encoding.GetEncoding(1251)))
                {
                    foreach (ZipArchiveEntry sourceEntry in sourceZIP.Entries)
                    {
                        if (targetZIP.GetEntry(sourceEntry.FullName) is ZipArchiveEntry existingTargetEntry)
                        {
                            existingTargetEntry.Delete();
                        }

                        using (Stream target = targetZIP.CreateEntry(sourceEntry.FullName).Open())
                        {
                            sourceEntry.Open().CopyTo(target);
                        }
                    }
                }
            }
        }
    }
}

For single specified file copy just replace bottom part from foreach loop to if condition:

static void CopyZipEntry(string fileName, string sourceZipFile, string targetZipFile)
{
    // ...

    // It means specified file exists in source ZIP-archive
    // and we can copy it to target ZIP-archive
    if (sourceZIP.GetEntry(fileName) is ZipArchiveEntry sourceEntry) 
    {
        if (targetZIP.GetEntry(sourceEntry.FullName) is ZipArchiveEntry existingTargetEntry)
            existingTargetEntry.Delete();

        using (Stream targetEntryStream = targetZIP.CreateEntry(sourceEntry.FullName).Open())
        {
            sourceEntry.Open().CopyTo(targetEntryStream);
        }
    }
    else
        MessageBox.Show("Source ZIP-archive doesn't contains file " + fileName);
}
Auditive
  • 1,607
  • 1
  • 7
  • 13
0

Thanks to the input so far, I cleaned up and improved the code. I think this looks cleaner and more reliable.

//Making sure files exist etc before this part...
string filePathInArchive = source.GetFilePath(fileId);

using FileStream sourceFileStream = new FileStream(source.FileName, FileMode.Open);
using FileStream targetFileStream = new FileStream(target.FileName, FileMode.Open, FileAccess.ReadWrite);
using ZipArchive sourceZip = new ZipArchive(sourceFileStream, ZipArchiveMode.Read, false );
using ZipArchive targetZip = new ZipArchive(targetFileStream, ZipArchiveMode.Update, false);

ZipArchiveEntry sourceEntry = sourceZip.GetEntry(filePathInArchive);

if (sourceEntry != null)
{
    if (targetZip.GetEntry(filePathInArchive) is { } existingTargetEntry)
    {
        existingTargetEntry.Delete();
    }

    using var targetEntryStream = targetZip.CreateEntry(sourceEntry.FullName).Open();
    sourceEntry.Open().CopyTo(targetEntryStream);
}
a.rlx
  • 54
  • 6