I don‘t understand the “pkg/archive: remove "whiteout for non-empty file" error handling” commit, I think the original code was correct.

If the converter returns a new whiteout header, the quoted code calls WriteHeader on the original hdr, and then immediately follows WriteHeader(theNewWhiteout). In that case, the Size > 0 situation (as you say) would cause an invalid archive to be created — so why does that commit turns that problematic situation from an error into a warning?

See https://github.com/containers/storage/blob/main/pkg/archive/archive.go#L634,

			if err := ta.TarWriter.WriteHeader(hdr); err != nil {
				return err
			}
			if hdr.Typeflag == tar.TypeReg && hdr.Size > 0 {
				return fmt.Errorf("tar: cannot use whiteout for non-empty file")
			}
			hdr = wo

the check flags a problematic situation, but it should have been made before writing the header with Size > 0, because the current approach breaks the entire archive without flagging an error to the caller. I don't know why it was written this way; given the state of this code, I suspect carelessness. Removing the check doesn't make a test fail, and moby/moby has this erroneous bit until today.

It was introduced in moby/moby@daa7019 . It already ignore the error return from addFile.

for us, we have to decide between:

• write the header, write file, then write whiteout
• ignore the whiteout if file.size >0
• ignore the file + whiteout if file.size > 0
• fail the archive creation

I can't tell which one is the right solution , but clearly the current code is broken.

looking a bit further, the only WhiteoutConverter is the one in archive_linux.go. Its ConvertWrite only takes action on directories and char devices, so this condition can never trigger.

I think we are in agreement that writing a whiteout header with .Size > 0 would cause a corrupt file to be created. And that detecting that only after the WriteHeader is not ideal (but I think it doesn’t really matter as long as the overall work reports a failure.)

Where the disconnect seems to be is that previously the code reported a failure, and now it fails and produces incorrect output. I think that’s a move in the wrong direction?!

the only WhiteoutConverter is the one in archive_linux.go. Its ConvertWrite only takes action on directories and char devices, so this condition can never trigger.

Yes. But, sadly, it’s a public API, so we can’t change WhiteoutConverter itself.

Anyway, I think I’d be fine with just code comments (on the caller, noting that the only possible implementation never sets Size > 0, and on the whiteout implementation, reminding that the implementation must preserve that property) — but keeping the check with the hard failure would provide a stronger guarantee, and the code ~already exists…

So I think just moving the error check before the WriteHeader might be best here.

Yes. But, sadly, it’s a public API, so we can’t change WhiteoutConverter itself.

the converter is a public API, but it is only selected through TarOptions.WhiteoutFormat, whose interpretation in GetWhiteoutConverter we control.

I did some more reading. The AUFS and overlayfs file systems have different ways of indicating deletions. This functionality is converts between them (where overlayfs -> aufs may introduce 2 entries for a single directory). The assumption therefore is that deletions are never marked by a non-empty files, and that a single whiteout in one format may only expand to 2 entries (without content) in the other format.

I'll add a comment.

Where the disconnect seems to be is that previously the code reported a failure,

it returned an error, but then that error only resulted in a log message.

nit: may use markup while at it, i.e.

so that go doc will link to this function.

done.

Generally speaking, it makes no sense to use backticks in docstrings.

removed. (I was following conventions from rest of file.)

nit: missing period

done.

nit: missing period

done.

nit: can use for i := range 10

done.