Handle symlinks embedding into binlogs - approach #2 #8306
Conversation
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
I love the simplification!
One remaining meta-question: is "don't embed empty files" the correct behavior? "File was present but empty" feels like useful information, but I don't recall the motivation for the prior behavior.
@KirillOsenkov may also have an opinion.
|
Actually that's what I'm thinking too - perhaps it's easier to just allow empty files? I can't remember if there's a valid reason to filter them out anymore... |
|
Perfect! Seems there is no compelling reason to special case empty files (actually the opposite is preffered) - so let's simplify a bit more. |
|
|
||
| using Stream entryStream = OpenArchiveEntry(filePath); | ||
| using FileStream content = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete); | ||
| using Stream entryStream = OpenArchiveEntry(filePath); |
There was a problem hiding this comment.
Does this ordering matter?
There was a problem hiding this comment.
I'd revert it just to be safe and keep the change minimal, but at a glance seems benign.
Forgind
added
the
merge-when-branch-open
5 participants
Fixes #6773
Context
Supersedes #8213 and #8282
Symlinked files were not embedded into binlog - previous solution was too much focused on symlinks and hence requried nontrivial code to properly distinguish aymlinks with available contnet.
Alternate approach - proceed with adding file as soon as it has available content
Testing
Preexisting unit test is excercising the scenario. Added a case for empty file - that still should not be added.