Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.path.difference: init #209375

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 138 additions & 1 deletion lib/path/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ let
inherit (builtins)
isString
isPath
isAttrs
dirOf
baseNameOf
typeOf
seq
split
match
;
Expand All @@ -18,6 +23,8 @@ let
all
concatMap
foldl'
take
drop
;

inherit (lib.strings)
Expand All @@ -33,10 +40,16 @@ let
isValid
;

inherit (lib.attrsets)
mapAttrsToList
listToAttrs
nameValuePair
;

# Return the reason why a subpath is invalid, or `null` if it's valid
subpathInvalidReason = value:
if ! isString value then
"The given value is of type ${builtins.typeOf value}, but a string was expected"
"The given value is of type ${typeOf value}, but a string was expected"
else if value == "" then
"The given string is empty"
else if substring 0 1 value == "/" then
Expand Down Expand Up @@ -100,6 +113,16 @@ let
# An empty string is not a valid relative path, so we need to return a `.` when we have no components
(if components == [] then "." else concatStringsSep "/" components);

# Takes a Nix path value and deconstructs it into the filesystem root
# (generally `/`) and a subpath
deconstructPath =
Comment on lines +116 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Takes a Nix path value and deconstructs it into the filesystem root
# (generally `/`) and a subpath
deconstructPath =
# Takes a Nix path and splits it into the filesystem root (generally `/`) and a subpath
splitPath =

Simpler, shorter name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitPath would also be a name for something like strings.split "/"

splitPathFromRoot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just splitting it from the root but also splitting the path itself. Ultimately this is an internal variable name, it doesn't matter much as long as we don't expose it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure matters less than an exported name, but for readability (= maintainability) it's relevant nonetheless. The suggestion is merely to (slightly) reduce cognitive load of reading a long word with many syllables. It arguably eases readability by only a small amount, but we have a long leverage on the future here. In any case, I'm not pushing this one at all, just trying to keep things at the amazing level of quality that you set out with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not be held to this standard all the time, yes I'm a perfectionist, but at some level it's just not worth it. There will always be things that aren't immediately obvious from just the name, in which case programmers can read comments, the source code, run the code themselves, etc.

But it's also entirely subjective which name is best. In this case, I think deconstructPath is still the best name, and me being the original author of the code should give that opinion some weight.

In particular, I also think deconstructPath being non-obvious is even a good thing, because what the function does is in fact non-obvious and non-trivial, I'd rather people look at the definition and comment instead of assuming they know what it means.

let
recurse = components: path:
# If the parent of a path is the path itself, then it's a filesystem root
if path == dirOf path then { root = path; inherit components; }
else recurse ([ (baseNameOf path) ] ++ components) (dirOf path);
in recurse [];

in /* No rec! Add dependencies on this file at the top. */ {

/* Append a subpath string to a path.
Expand Down Expand Up @@ -149,6 +172,120 @@ in /* No rec! Add dependencies on this file at the top. */ {
${subpathInvalidReason subpath}'';
path + ("/" + subpath);

/* Determine the difference between multiple paths, including the longest
common prefix and the individual suffixes between them

The input is an attribute set of paths, where the keys are only for user
convenience and can be chosen arbitrarily. The returned attribute set
contains two attributes:

- `commonPrefix`: A path value containing the common prefix between all the
given paths.

- `suffix`: An attribute set of normalised subpaths (see
`lib.path.subpath.normalise`). The keys are the same
as were given as the input, they can be used to easily match up the
suffixes to the inputs.

Throws an error if all paths don't share the same filesystem root.

Laws:

- The input paths can be recovered by appending each suffix to the common ancestor

forall paths, result = difference paths.
mapAttrs (_: append result.commonPrefix) result.suffix == paths

- The _longest_ common prefix is returned

forall paths, result = difference paths.
! exists longerPrefix. hasProperPrefix result.commonPrefix longerPrefix && all (hasPrefix longerPrefix) (attrValues paths)
Comment on lines +199 to +202
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this PR weakly depends on #210423 because of the docs here. The deconstructPath function is also duplicated/reusable between these two PRs


- Suffixes are normalised

forall paths, result = difference paths.
mapAttrs (_: subpath.normalise) result.suffix == result.suffix

Type:
difference :: AttrsOf Path -> { commonPrefix :: Path, suffix :: AttrsOf String }

Example:
difference { foo = ./foo; bar = ./bar; }
=> { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./bar"; }; }

difference { foo = ./foo; bar = ./.; }
=> { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./."; }; }

difference { foo = ./foo; bar = ./foo; }
=> { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./."; }; }

difference { foo = ./foo; bar = ./foo/bar; }
=> { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./bar"; }; }
*/
difference =
# The attribute set of paths to calculate the difference between
paths:
let
# Deconstruct every path into its root and subpath
deconstructedPaths = mapAttrsToList (name: value:
# Check each item to be an actual path
assert assertMsg (isPath value) "lib.path.difference: Attribute ${name} is of type ${typeOf value}, but a path was expected";
deconstructPath value // { inherit name value; }
) paths;

firstPath =
assert assertMsg
(paths != {})
"lib.path.difference: An empty attribute set was given, but a non-empty attribute set was expected";
head deconstructedPaths;

# The common root to all paths, errors if there are different roots
commonRoot =
# Fast happy path in case all roots are the same
if all (path: path.root == firstPath.root) deconstructedPaths then firstPath.root
# Slower sad path when that's not the case and we need to throw an error
else foldl'
(skip: path:
if path.root == firstPath.root then skip
else throw ''
lib.path.difference: Filesystem roots must be the same for all paths, but paths with different roots were given:
${firstPath.name} = ${toString firstPath.value} (root ${toString firstPath.root})
${toString path.name} = ${toString path.value} (root ${toString path.root})''
)
null
deconstructedPaths;

commonAncestorLength =
let
recurse = index:
let firstComponent = elemAt firstPath.components index; in
if all (path:
# If all paths have another level of components
length path.components > index
# And they all match
&& elemAt path.components index == firstComponent
) deconstructedPaths
then recurse (index + 1)
else index;
in
# Ensure that we have a common root before trying to find a common ancestor
# If we didn't do this one could evaluate `relativePaths` without an error even when there's no common root
seq commonRoot (recurse 0);

commonPrefix = commonRoot
+ ("/" + joinRelPath (take commonAncestorLength firstPath.components));

suffix = listToAttrs (map (path:
nameValuePair path.name (joinRelPath (drop commonAncestorLength path.components))
) deconstructedPaths);
in
assert assertMsg
(isAttrs paths)
"lib.path.difference: The given argument is of type ${typeOf paths}, but an attribute set was expected";
roberth marked this conversation as resolved.
Show resolved Hide resolved
{
inherit commonPrefix suffix;
};

/* Whether a value is a valid subpath string.

- The value is a string
Expand Down
34 changes: 33 additions & 1 deletion lib/path/tests/unit.nix
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{ libpath }:
let
lib = import libpath;
inherit (lib.path) append subpath;
inherit (lib.path) append difference subpath;

cases = lib.runTests {
# Test examples from the lib.path.append documentation
Expand Down Expand Up @@ -40,6 +40,38 @@ let
expected = false;
};

# Test examples from the documentation
testDifferenceExample1 = {
expr = difference { foo = ./foo; bar = ./bar; };
expected = { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./bar"; }; };
};
testDifferenceExample2 = {
expr = difference { foo = ./foo; bar = ./.; };
expected = { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./."; }; };
};
testDifferenceExample3 = {
expr = difference { foo = ./foo; bar = ./foo; };
expected = { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./."; }; };
};
testDifferenceExample4 = {
expr = difference { foo = ./foo; bar = ./foo/bar; };
expected = { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./bar"; }; };
};
# Test that the invalid cases cause throws
testDifferenceNonAttrset = {
expr = (builtins.tryEval (difference 10).commonPrefix).success;
expected = false;
};
testDifferenceEmpty = {
expr = (builtins.tryEval (difference { }).commonPrefix).success;
expected = false;
};
testDifferenceNonPath = {
expr = (builtins.tryEval (difference { a = 10; }).commonPrefix).success;
expected = false;
};
# We can't test for multiple roots with the current Nix version though

# Test examples from the lib.path.subpath.isValid documentation
testSubpathIsValidExample1 = {
expr = subpath.isValid null;
Expand Down