lib.fileset: Representation for empty file sets without a base path

[infinisil]

Description of changes

Changes the internal fileset structure to also be able to represent empty file sets without any base path. This can then be used as the return value of unions [].

Once the lib.fileset.intersect combinator from the full WIP combinators is added, this will also be suitable as a return value for e.g. intersect /foo /bar.

Notably the new empty singleton value is not exposed in the interface, because I don't know of any use case for it. It could be added later if a use case emerges.

This work is sponsored by Antithesis ✨

Things done

• Added tests
• Benchmarked (minor change, not significant)

[fricklerhandwerk]

I'm not sure about the approach here. I understand that having a value to represent unions [] would be nice to have, but the special case clutters the code quite a bit, moving it past the sweet spot of readability, and that particular edge case seems really not that important. Also I don't see why intersect /foo /bar can't return { base = /; tree = null; }. The overall change is therefore of questionable utility to me.

[infinisil]

Also I don't see why intersect /foo /bar can't return { base = /; tree = null; }`.

The motivation comes from this example, which should work:

toSource {
  root = ./foo;
  fileset = intersect ./foo ./.;
}

If intersect ./foo ./. would use the common prefix (./.) as the base path, toSource would complain that the base path is outside of the root, when clearly that's not what the user would expect. So the base path of intersect ./foo ./. should instead be ./foo.

Extending that, this should also succeed without an error:

toSource {
  root = ./foo;
  fileset = intersect ./foo ./bar;
}

though this would give an empty result, but that's as one would expect.

(this will definitely have to go into the design document)

[roberth]

I'd rather have a working abstraction than a leaky (broken) one that is "extra readable".

[fricklerhandwerk]

So the base path of intersect ./foo ./. should instead be ./foo.

Agreed that the combination of intersect ./foo ./. and toSource { root = ./foo; } as you describe it may be confusing, but not convinced about intersect in isolation. Just from looking at it, it's not evident what the base path should be, and intuitively I'd say it's their common prefix. If you want it to be the longest path in the resulting set, it should be documented very visibly. Maybe even guard it by an invariant that the base path is never shortened in operations?

Only then it seems inevitable to produce a special kind of value for intersect ./foo ./bar.

@roberth: Fully agreed. A particularly involved implementation is still better with a well-articulated rationale.

[infinisil]

Just from looking at it, it's not evident what the base path should be, and intuitively I'd say it's their common prefix. If you want it to be the longest path in the resulting set, it should be documented very visibly.

The base path is mostly an invisible implementation detail, users shouldn't have to know about it. The only interaction of it with the user is in this error message:

nix-repl> fileset.toSource { root = ./foo; fileset = ./.; }
error: lib.fileset.toSource: `fileset` could contain files in /home/user/src,
  which is not under the `root` (/home/user/src/foo). Potential solutions:
   - Set `root` to /home/user/src or any directory higher up.
     This changes the layout of the resulting store path.
   - Set `fileset` to a file set that cannot contain files outside the `root` (/home/user/src/foo).
     This could change the files included in the result.

This error should only be thrown when it's relevant, meaning when there could be some file outside the root in the fileset.

intersect ./foo ./. can never contain files outside ./foo, so it should not thrown this error. Similarly, intersect ./foo ./bar can never contain any files at all, so it should not throw that error either.

Maybe even guard it by an invariant that the base path is never shortened in operations?

Comparatively, union needs the common prefix path as the base path, it's a kind of duality! union ./foo ./bar can contain files from either, so the base path should be ./..

[infinisil]

I now updated this to use _internalIsEmptyBaseless, I liked this one the most. I'd rather not bikeshed over the name any longer, this is not exposed in the API so it can also be changed later if desired.

Furthermore, I added a section to the design document explaining why such a value is needed and the problem with alternatives. Valentin also confirmed in a meeting that he agrees with this being needed.

I think this is ready to be merged, and unless there are any complaints in the next couple days I'll merge this myself.

[infinisil]

In PM's @fricklerhandwerk made the suggestion to call it ..withoutBase or ..noBase instead. That sounds good to me, so I now changed it to _internalIsEmptyWithoutBase.

[roberth]

One off-topic suggestion. LGTM!

[roberth]

Should this be in a file like fileset/internal/README.md?

[infinisil]

I view lib/fileset/README.md as the internal contributor documentation, where I think this makes sense to have. It's not part of the public interface, but you need to know about it to contribute, which can mean to change the internals

[roberth]

It should link to the user docs.

[infinisil]

Now done in #258832 (already merged myself)