Uh oh!
There was an error while loading. Please reload this page.
Tar: Use indexer setter instead of Add on ExtendedAttributes dictionary - #76404
Conversation
ghost
commented
Sep 29, 2022
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFixes #75215 Minimal fix for the reported scenario. Note: There's another issue with how we deal with e.g: [Fact]publicvoidQuickTest(){Dictionary<string,string>ea=new();ea["path"]="foo";PaxTarEntrypaxEntry=newPaxTarEntry(TarEntryType.RegularFile,"bar",ea);Console.WriteLine(paxEntry.Name);// prints barConsole.WriteLine(paxEntry.ExtendedAttributes["path"]);// prints foo}
|
| if (!ExtendedAttributes.ContainsKey(PaxEaMTime)) | ||
| { | ||
| ExtendedAttributes.Add(PaxEaMTime, TarHelpers.GetTimestampStringFromDateTimeOffset(_mTime)); | ||
| } |
There was a problem hiding this comment.
Just pointing out some other issues in this code:
- someone uses the copy ctor. passing the extended attributes from the other entry.
- on the new entry, you set ModificationTime.
- pass the new entry to TarWriter.WriteEntry.
The modification time will be neglected due to this check. @carlossanlop please advice if this is something we should address in this PR.
There was a problem hiding this comment.
please advice if this is something we should address in this PR.
Yes. We must always update the dictionary value for mtime using the value from the ModificationTime property. So the if condition should be removed.
There was a problem hiding this comment.
Do you recall why was added in the first place?
Uh oh!
There was an error while loading. Please reload this page.
| { | ||
| ExtendedAttributes.Add(PaxEaSize, _size.ToString()); | ||
| ExtendedAttributes[PaxEaSize] = _size.ToString(); | ||
| } |
There was a problem hiding this comment.
We could add an else here, in case the data stream gets reduced below the max allowed size, in which case, we can remove the PaxEaSize value from the dictionary entry.
There was a problem hiding this comment.
In my other PR which was replaced by this one, I had a fake stream implementation that simulated having 99_999_999 length. You can reuse that to test this.
| if (!string.IsNullOrEmpty(_linkName)) | ||
| { | ||
| Debug.Assert(_typeFlag is TarEntryType.SymbolicLink or TarEntryType.HardLink); |
jozkee
commented
Oct 3, 2022
/backport to release/7.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3177692797 |
@jozkee backporting to release/7.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Use indexer setter instead of Add on ExtendedAttributes dictionary
Using index info to reconstruct a base tree...
M src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
M src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
M src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Tests.cs
M src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.Tests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.Tests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.Tests.cs
Auto-merging src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Tests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Tests.cs
Auto-merging src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
CONFLICT (content): Merge conflict in src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
Auto-merging src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Use indexer setter instead of Add on ExtendedAttributes dictionary
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
…ry (#76404) * Use indexer setter instead of Add on ExtendedAttributes dictionary * Add roundtrip tests * Fix TryAddStringField and always set mtime
* Use UTF8 encoding on Tar string fields * Slice destination on Checksum * Use Encoding.GetByteCount as fast path * Use escape sequences on hardcoded UTF8 characters * Fix ustar prefix logic and throw if name would be truncated * Address feedback * Fix truncation and prefix logic * Fix nits * Add async tests * Add tests for unseekable streams * Address feedback * Tar: Use indexer setter instead of Add on ExtendedAttributes dictionary (#76404) * Use indexer setter instead of Add on ExtendedAttributes dictionary * Add roundtrip tests * Fix TryAddStringField and always set mtime Co-authored-by: David Cantu <dacantu@microsoft.com>
Fixes#75215
Minimal fix for the reported scenario.
Note: There's another issue with how we deal with
ExtendedAttributesand public properties. There's no syncronization between both, which may lead to unexpected results when writing aPaxTarEntry, see #76405.