Fix some corner cases in TarReader

[am11]

• Fix reading of tar file with hardlinks.
• Fix reading of tar files with mixed formats (gnu with ustar or pax entries and vice versa).
• Add node-tar fixtures test added in dotnet/runtime-assets repo.

Contributes to #74316.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

• Fix reading of tar file with hardlinks.
• Fix reading of tar files with mixed formats (gnu with ustar or pax entries and vice versa).
• Add node-tar fixtures test added in dotnet/runtime-assets repo.

Contributes to #74316.

Author:	am11
Assignees:	-
Labels:	area-System.IO, community-contribution
Milestone:	-

[carlossanlop]

The fix looks good. I just have a question about the test.

[carlossanlop]

First time I see Record.Exception being used.

What advantage does it have over simply letting this throw?

[am11]

The intent of Record.Exception() followed by Assert.Null() is to be explicit about asserting that 'the method call is not throwing with this fix' (which were throwing without the fix). It also avoids seemingly unused statements to be optimized out by the JIT.

[danmoseley]

If you're consuming or asserting something about entry, Name, and Type the jit cannot remove them, and you're implicitly testing that they don't throw.

Eg you can assert that Name is never null, and that if type is Directory then length is zero.

[danmoseley]

Eg you could do more or less what the test code did and then you won't need to record the exception explicitly

var ms = new MemoryStream();               
Assert.NotEmpty(entry.Name);                     
Assert.True(Enum.IsDefined(entry.EntryType));               
Assert.True(Enum.IsDefined(entry.Format));         
if (entry.EntryType == TarEntryType.Directory)
    continue;
var ds = entry.DataStream;
                       
if (ds != null && ds.Length > 0)
{                            
    ds.CopyTo(ms);
}

[danmoseley]

Cc @stephentoub

[carlossanlop]

LGTM. Thanks for fixing this @am11.

CI failure was a cancellation due to timeout.

@danmoseley I'll prepare the backport manually.

[danmoseley]

Read content into a MemoryStream perhaps, as I did in my test code?

[danmoseley]

If you're consuming or asserting something about entry, Name, and Type the jit cannot remove them, and you're implicitly testing that they don't throw.

Eg you can assert that Name is never null, and that if type is Directory then length is zero.

[danmoseley]

Eg you could do more or less what the test code did and then you won't need to record the exception explicitly

var ms = new MemoryStream();               
Assert.NotEmpty(entry.Name);                     
Assert.True(Enum.IsDefined(entry.EntryType));               
Assert.True(Enum.IsDefined(entry.Format));         
if (entry.EntryType == TarEntryType.Directory)
    continue;
var ds = entry.DataStream;
                       
if (ds != null && ds.Length > 0)
{                            
    ds.CopyTo(ms);
}

[danmoseley]

@carlossanlop my suggestion about backport is for us to fix the various issues first in main, then bring a single back port PR. It will be less confusing. Also because the code has diverged.