Improve the ParseSink interface
#9653
Conversation
|
|
||
| if (mode == Mode::Executable) | ||
| sink.isExecutable(); |
There was a problem hiding this comment.
This was a bad hack that no longer works. The changes to this file are to make a better solution instead.
There was a problem hiding this comment.
This file will be easier to review with whitespace disabled
| @@ -171,93 +171,105 @@ static void parse(ParseSink & sink, Source & source, const Path & path) | |||
| s = readString(source); | |||
| if (s != "(") throw badArchive("expected open tag"); | |||
|
|
|||
| enum { tpUnknown, tpRegular, tpDirectory, tpSymlink } type = tpUnknown; | |||
There was a problem hiding this comment.
The change I did to the parser is basically to replace this extra state variable with control flow. This is needed so that the "content" and "executable" parsing happens within the callback. I then changed the others to keep the code design consistent (and get rid of this enum entirely, as opposed to confusingly just getting rid of tpRegular).
| @@ -9,27 +9,38 @@ | |||
| namespace nix { | |||
|
|
|||
| /** | |||
| * \todo Fix this API, it sucks. | |||
There was a problem hiding this comment.
This was originally your comment, @edolstra, but I removed it as I am now decently happy with this interface :)
| if (archiveSettings.useCaseHack) { | ||
| auto i = names.find(name); | ||
| if (i != names.end()) { | ||
| debug("case collision between '%1%' and '%2%'", i->first, name); | ||
| name += caseHackSuffix; | ||
| name += std::to_string(++i->second); | ||
| } else | ||
| names[name] = 0; | ||
| } |
There was a problem hiding this comment.
thought: This could be factored out into a sink to sink transformation that performs the renaming.
| if (name <= prevName) | ||
| throw Error("NAR directory is not sorted"); | ||
| prevName = name; | ||
| if (archiveSettings.useCaseHack) { |
There was a problem hiding this comment.
(pre-existing)
This is only necessary when writing to a store that needs it.
It must not be done in other situations, e.g. when writing NAR contents to Git trees.
| virtual void createDirectory(const Path & path) = 0; | ||
|
|
||
| virtual void createRegularFile(const Path & path) = 0; | ||
| virtual void receiveContents(std::string_view data) = 0; | ||
| virtual void isExecutable() = 0; |
There was a problem hiding this comment.
Should isExecutable be called before, during or after the sink operations?
There was a problem hiding this comment.
I think any order is fine, actually.
There was a problem hiding this comment.
What if a NAR has multiple "executable" strings per file? That'd not be canonical. It doesn't quite smell right, but I don't have a suggestion for this PR.
Co-authored-by: Robert Hensing <[email protected]>
More invariants are enforced in the type, and less state needs to be stored in the main sink itself. The method here is roughly that known as "session types". Co-authored-by: Robert Hensing <[email protected]>
Ericson2314
force-pushed
the
improve-parse-sink
branch
from
fc6a155 to
6365bbf
Compare
github-actions
bot
added
the
store
Motivation
More invariants are enforced in the type, and less state needs to be stored in the main sink itself. The method here is roughly that known as "session types".
Context
The separation of concerns I would like to see in #9485 depends on this step.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.