Fix handling contents added after header creation

[elbe0046]

Second attempt at fixing alexcrichton#282, updated with the PR feedback: alexcrichton#283 (comment).

Just undoes the first set of changes and then adds 378fdb2.

Oh sorry I can generalize what I'm thinking a bit from "application layer" to "callers of this function". This is almost surely a bug in append_dir_all and it's fine to fix there, I just don't think it should be fixed in append which takes a generic input stream

What is here moves the changes up from append to append_path_with_name and append_file. At present I'm not really sure how to move things up higher to append_dir_all - at least without it being a more invasive set of changes.

Secondly, there now isn't really any point at which I can inject the issue reliably in a test situation. Which doesn't feel great to try to land this without adding test coverage.

Hoping to get some feedback from folks here, especially if people have ideas on if / how to address these two points before resubmitting for review upstream.

[AldaronLau]

I would change this private function:

fn append_file(
    dst: &mut dyn Write,
    path: &Path,
    file: &mut fs::File, // change to &mut dyn Read
    mode: HeaderMode,
) -> io::Result<()> {
    let stat = file.metadata()?;
    append_fs(dst, path, &stat, file, mode, None)
}

Then in add the call to take within append_dir_all():

…
        } else {
            #[cfg(unix)]
            {
                let stat = fs::metadata(&src)?;
                if !stat.is_file() {
                    append_special(dst, &dest, &stat, mode)?;
                    continue;
                }
            }
            // Add take() call here:
            append_file(dst, &dest, &mut fs::File::open(src)?, mode)?;
        }
…

I think that's all you need.

[AldaronLau]

As for a test, I'm thinking you'd have to create a temporary directory.

• Write in temp dir to /hello.txt, "Hello"
• Use notify crate / inotify to watch for metadata check event
• Immediately write to /hello.txt ", world" upon receiving event
• With the fs event catching mechanism in place, call append_dir_all()

Unfortunately, I don't believe it's possible to make a more reliable test without actually intercepting the calls to the filesystem (which would probably require something like web assembly sandboxing and WASI-interception magic).

[elbe0046]

I would change this private function:

fn append_file(
    dst: &mut dyn Write,
    path: &Path,
    file: &mut fs::File, // change to &mut dyn Read
    mode: HeaderMode,
) -> io::Result<()> {
    let stat = file.metadata()?;
    append_fs(dst, path, &stat, file, mode, None)
}

Then in add the call to take within append_dir_all():

…
        } else {
            #[cfg(unix)]
            {
                let stat = fs::metadata(&src)?;
                if !stat.is_file() {
                    append_special(dst, &dest, &stat, mode)?;
                    continue;
                }
            }
            // Add take() call here:
            append_file(dst, &dest, &mut fs::File::open(src)?, mode)?;
        }
…

I think that's all you need.

This is a nice approach and we may end up going this route. I could see there being some confusion around a file being append_file but then lifting the restriction of it being a File, so a rename may be in order. I think this also misses the append_path_with_name case unless I'm missing something?

As for a test, I'm thinking you'd have to create a temporary directory.

Write in temp dir to /hello.txt, "Hello"
Use notify crate / inotify to watch for metadata check event
Immediately write to /hello.txt ", world" upon receiving event
With the fs event catching mechanism in place, call append_dir_all()
Unfortunately, I don't believe it's possible to make a more reliable test without actually intercepting the calls to the filesystem (which would probably require something like web assembly sandboxing and WASI-interception magic).

Right I think a reliable test flow would require some fairly heavy lifting in terms of changes introduced. Probably beyond the scope of what would make sense for the fix PR but worth keeping in mind I think.

[AldaronLau]

This is a nice approach and we may end up going this route. I could see there being some confusion around a file being append_file but then lifting the restriction of it being a File, so a rename may be in order. I think this also misses the append_path_with_name case unless I'm missing something?

I think since it's still expected to be used internally as a function for appending files, I personally wouldn't do a rename.

My understanding is that append_path_with_name() is not used by append_dir_all(), and I assumed we were only fixing the one function because of Alex Crichton's comment about which layer the fix should be made.