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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symlink review #9384

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Symlink review #9384

wants to merge 4 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 19, 2023

Motivation

Did #9363 (review) myself because we were optimizing for a quick release.

Context

Priorities

Add 馃憤 to pull requests you find important.

They do not involve base-2 exponential effects, so they should be
Normal base-10 round numbers.
@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world labels Nov 19, 2023
while (true) {
// Basic cycle/depth limit to avoid infinite loops.
if (++followCount >= maxFollow)
throw Error("too many levels of symbolic links while traversing the path '%s'; assuming it leads to a cycle after following %d indirections", this->to_string(), maxFollow);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw Error("too many levels of symbolic links while traversing the path '%s'; assuming it leads to a cycle after following %d indirections", this->to_string(), maxFollow);
throw Error("cycle detected following symlink '%s'", this->to_string());

After 1000 links we can safely assume there's a cycle, so no need to be very verbose.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

As discussed with @roberth in real time, it seems like this code is missing some deleted lines, because shouldn't resolve symlinks have something to do with follow symlinks? (and there is already some symlink code in libeval too I thought.)

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 22, 2024

Ah I see followSymlinks is really quite different, because it is only dereferencing the tip. Not so obvious after all how to share code between them in a worthwhile way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants