[Tar] Fill in the file size even if the file is empty. by sobczyk · Pull Request #106409 · dotnet/runtime (original) (raw)

This matches standard behavior.
See #95354

This matches standard behavior.

This was referenced

Aug 14, 2024

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

This was referenced

Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. Can you please add a test based on the repro code in your issue description? #95354 (comment)

@sobczyk I see your comment in the issue:

the change is basically a one liner, the problem is that the unit tests are a bit messy I'll provide the fix today or tomorrow, and will try to do some UT, but that looked more difficult than the fix

Let me know if you have any questions or I can be of assistance. I'll be happy to help.

@carlossanlop Ok, I added the repro code as an unit test.
I was wondering whether _size field should be nullable?
I'm guessing it was 0 since older .NET did not have nullables.
If _size would be nullable the check for _dataStream would not be necessary,
though the fix would change more lines.

@dotnet-policy-service agree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor suggestion (disposing the test types). Otherwise looks good.

I see your test only verifies Pax, which is fine for this case. I would be surprised if the issue happens only in certain formats instead of in all of them, as the affected code lines are shared among all types.

Comment on lines +358 to +360

MemoryStream emptyData = new(0);
MemoryStream output = new();
TarWriter archive = new(output, TarEntryFormat.Pax);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you only see the problem in Pax, or is this the only format that you tested?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one I tested, but the problem would apply equally to all, since small sizes are encoded the same way.

@carlossanlop Ok, I added the repro code as an unit test. I was wondering whether _size field should be nullable? I'm guessing it was 0 since older .NET did not have nullables. If _size would be nullable the check for _dataStream would not be necessary, though the fix would change more lines.

It was not nullable on purpose. I don't recall the specific reasons at the moment but this was intended. System.Formats.Tar was introduced in .NET 7. Nullables already existed in that version, so that was not the reason.

Co-authored-by: Carlos Sánchez López 1175054+carlossanlop@users.noreply.github.com

Is the code OK?

Yes. I was just waiting for the CI to finish.

The failure is known and pre-existing: #104905

Ready to merge. Thank you for your contribution, @sobczyk!

}

[Fact]
void Verify_Size_RegularFile_Empty()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the test methods should be public

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be included in .NET 9?

It's only in main right now. But we could consider requesting approval for backporting to RC2.

Did you encounter a case where you need it, @am11?

If it is going in GA release, that would be nice. I haven't stumbled upon it, but I was thinking about implementing an msbuild task based on System.Formats.Tar (to replace dotnet/arcade#15038 (comment)), so I thought of checking up on the status of (this relatively new lib) support. :)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})