Fix #5299 and remove uncesssary unbounded buffer #5696
Conversation
No more buffering in string.
This doesn't fix the bug, but makes the code less difficult to read. Also improve the comments, now that it is clear what part is needed in each code path.
No matter what, we need to resize the buffer to not have any scratch space after we do the `read`. In the end of file case, `got` will be 0 from it's initial value. Before, we forgot to resize in the EOF case with the break. Yes, we know we didn't recieve any data in that case, but we still have the scatch space to undo. Co-Authored-By: Will Fancher <Will.Fancher@Obsidian.Systems>
There was a problem hiding this comment.
Thanks!
It does indeed fix #5299 and makes things nicer to read in the end 👍
There was a problem hiding this comment.
See review comments for some minor touchups to the larger of introduced comments.
Also in interest of not just nitpicking and helping craft this instead here's one possible alternative (attempting to keep/use your language) which maybe is not better:
Pass through the NAR unmodified into
saved, but use parseDump to parse it anyway so we aren't sending arbitrary data unwittingly, and so that we only consume the NAR off offromand not past it.
(We don't trustaddToStoreFromDumpto not eagerly consume the entire stream it's given, past the length of the NAR.)
(personally I have no idea why addtoStoreFromDump is mentioned here, but leaving it in spirit of Chesterson's fence, and I'm very much not suggesting it's not there for a good reason 😄).
Whatever you think is best, hope this helps! 😄.
Tried to not too intrusively clarify we're letting parseDump drive the amount consumed (TeeSource only writing to saved what is read from it in parseDump, since that seems to be the point of this)--behavior that wasn't obvious to me but I'm not super familiar with the Source/Sink bits we've got, but IDK. Mostly "parse it" is not obviously "parse and read a limited amount (one object)" as that's not normally strictly speaking what I think of as the role of a parser. But that is clearly what's expected, for example, in the else side of this and likely elsewhere so 🤷.
Honestly maybe what you have with your selection of the suggested fixups is best! 👍.
|
|
||
| auto dumpSource = sinkToSource([&](Sink & saved) { | ||
| if (method == FileIngestionMethod::Recursive) { | ||
| /* We parse the NAR dump through into `saved` unmodified, |
There was a problem hiding this comment.
We parse the NAR dump
was "We pass the NAR dump" meant instead?
I mean yes we parse it too, but it seems that's the part we're explaining is why we're parsing despite passing it through unmodified. Wouldn't bring it up but there are few other fixups...
| /* We parse the NAR dump through into `saved` unmodified, | ||
| so why all this extra work? We still parse the NAR so | ||
| that we aren't sending arbitrary data to `saved` | ||
| unwittingly`, and we know when the NAR ends so we don't |
There was a problem hiding this comment.
unwittingly`
(Seems like stray backtick?)
and we know when the NAR
This was confusing to me on first read, I want to suggest "and so that we know when the NAR". But then that's a lot of "so"s in a row. Writing is hard haha.
| consume the rest of `from` and can't parse another | ||
| command. (We don't trust `addToStoreFromDump` to not | ||
| eagerly consume the entire stream it's given, past the | ||
| length of the Nar. */ |
There was a problem hiding this comment.
Nar
Nar -> NAR? (for consistency's sake?)
Also, the paren here is unmatched/missing end paren.
|
@dtzWill Would you want to just make another PR rewriting the comment as you fit? I would be happy to review it! |
|
@edolstra @Ericson2314 as shown in #5740, this actually causes noticeable, yet weird problems. |
|
N.B. backport PR is already up at #5721 |
See each commit message for details