fix: handle overrides on paths that later became symlinks

[cachebag]

Closes #1138

Problem Description

The crux of the original problem as seen in #1138, was laid out as such...

Overrides are stored under the directory's resolved path, and lookups resolve the current directory the same way.

If an overridden directory is later replaced by a symlink (say a project moved, and an old path symlinked to the new one), the old entry keeps the old path and can never match again.

This emits 3 main consequences:

1. cd into the symlink doesn't use that entry.
2. list shows the dead entry as normal, and --nonexistent won't remove it.
3. unset --path <symlink> removes the target's override instead. The only way to get rid of the dead entry is editing settings.toml by hand.

Proposed Solution

There are a few different ways we can approach this.

We can store or match overrides by the unresolved path (or $PWD). That would fix (1), but it changes how every override works, and would need a migration, and $PWD depends on the shell.

We can also keep storage and lookup: essentially, only fix how dead entries are reported and removed. That fixes (2) and (3).

Options for (3):

• Never resolve symlinks in unset --path. This breaks symmetry with set --path , which stores the target's path.
• Try the symlink's own entry first, then fall back to the target.

Options for (2):

• Flag entries whose path is itself a symlink (chosen).
• Compare each entry with its fully resolved path. That also catches a parent directory becoming a symlink, but it's riskier with Windows \?\ paths and case-insensitive filesystems.

This PR employs the following:

Try the symlink's own entry first, then fall back to the target

And it makes the most sense to me given the problem space.

Impact

There is not much fallout of anything other than the fact that unset --path <symlink> now removes the symlink's own entry if there is one. Before, it silently removed the target's override. Otherwise the behavior is the same.

list and also marks these entries "(not a directory)" and --nonexistent removes them. But this is safe because lookups resolve symlinks, so such entries can never match anything.

Follow-up work

If we wanted, we could extend --nonexistent to entries whose parent directory became a symlink. This would need careful Windows coverage, since the new tests are Unix-only.

Performance

Validation

Validated through:

Each fix follows a test commit that pins today's behavior, and the fix commit flips that snapshot.

[rami3l]

@cachebag Hi, thanks for this patch!

Looking at #1138, it seems like it refers to a design question rather than a bug. Plus there are 3 questions being raised there, and it is not immediately clear which of the 3 you are addressing here.

Given that, before starting the review process, I would like to know from your PR description more info about the proposed change, such as:

1. A general background of the design problem you are trying to solve;
2. A short summary of possible solutions in the problem domain;
3. The final decisions that you took in this PR and their rationale;
4. Impacts of this change to the user, if applicable;
5. Future work that could happen following this PR, if any.

For a concrete example of the report format that you may use, please refer to #4947.

[cachebag]

@cachebag Hi, thanks for this patch!

Looking at #1138, it seems like it refers to a design question rather than a bug. Plus there are 3 questions being raised there, and it is not immediately clear which of the 3 you are addressing here.

Given that, before starting the review process, I would like to know from your PR description more info about the proposed change, such as:
or a concrete example of the report format that you may use, please refer to #4947.

Absolutely. Sorry about the confusion. I've updated the PR description with the requested information.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rami3l]

@cachebag Thanks a whole lot for your report!

Looking at the current situation, it seems that the underlying issue is surprising similar to #4947, where the value saved to settings.toml has always been prematurely canonicalized.

For this part, maybe we shouldn't do the canonicalization at all as long as the resulting path is valid, and instead we just join CWD and the provided path upon writing? I know it's a breaking change, but it should be quite minor, I'd say at the same level as #4947.

However, we should also consider that since we are storing a dictionary rather than a string, there is another layer where we look for the CWD-keyed entry in:

overrides: BTreeMap<String, String>,

For the key identity part, we already import same_file, for which you can check if two directories are equivalent by converting paths to platform-aware handles first (it works on case-insensitive APFS for example¹). Adding some overhead for the right behavior seems to be justifiable in this case?

I'd like to know your opinions on that matter, and if I got anything wrong here please don't hesitate to correct me.

---

Also cc @ChrisDenton: I'm wondering if the issue with false positives on ReFS¹ is worth worrying about on our side regarding our usage of same_file in general, as I have never personally used ReFS before.

Footnotes

1. There are some quirks about ReFS with this crate, but ripgrep would fail the same time as we do, see https://github.com/BurntSushi/same-file/issues/63. ↩ ↩²

[cachebag]

@rami3l
thank you! and thank you for mentioning #4947. i think your diagnosis is spot on here.

maybe we shouldn't do the canonicalization at all as long as the resulting path is valid, and instead we just join CWD and the provided path upon writing?

i agree here. actually i think everything you are mentioning is far better as a solution than mine. under this model an entry on a symlink isn't stale, it just resolves at lookup time. so my last commit (list/--nonexistent treating symlinks as nonexistent) is technically wrong, and the remove_override fallback isn't needed anymore since keys are no longer resolved.

nicely enough, it also turns out that paths that only differ by case on case-insensitive filesystems (APFS), and override set --path with a relative nonexistent path, which i believe currently get stored as a relative key that can never match.

re: writes:

• what would you count as "valid" here? right now, set --path happily accepts a nonexistent path.
• cwd.join() keeps .. segments. we probably want to normalize before storing, though collapsing .. right after a symlink changes what the path points to...i have to think about that more.

there is another layer where we look for the CWD-keyed entry in

for lookup i'm thinking that we can exact string match first. it's cheap and keeps existing (canonical) keys working the same after these changes are merged.

else, we can do a same-dir match via same_file::Handle. since it's Eq + Hash, we can open each key once into a map and each ancestor once, so it's depth + n opens instead of depth × n.

Adding some overhead for the right behavior seems to be justifiable in this case?

I don't know actually, I have to also think about this more. This runs on every proxy invocation, and the exact-match fast path only helps in dirs that actually have an override. in every other dir we'd open every key on every cargo call, so a key sitting on an unreachable network mount could stall all of them.

SO I think yes? But, I would love to measure the overhead before/after if that helps inform the decision though. Let me know if you think otherwise.

This is the rough shape I have in mind as far as commits, please let me know if it makes sense to you:

1. tests pinning the current behavior
2. stop canonicalizing on write
3. same-dir lookup
4. unset / --nonexistent semantics
5. flip the tests

[rami3l]

override set --path with a relative nonexistent path, which i believe currently get stored as a relative key that can never match.

• what would you count as "valid" here? right now, set --path happily accepts a nonexistent path.
• cwd.join() keeps .. segments. we probably want to normalize before storing, though collapsing .. right after a symlink changes what the path points to...i have to think about that more.

@cachebag Wow that's really messy, thanks for pointing out that subtle detail.

I'm thinking about 3 cases then:

1. rustup override set: Just proceed normally. Save the current path as-is (maybe it's already canonicalized? to be confirmed) or something among those lines. We want the least disruptive change here.
2. rustup override set --path=<absolute>: Save the given path as-is. Or maybe you can try to collapse .. etc if there is an obvious way of doing it... But don't do too much on top of that.
3. rustup override set --path=<relative>: Canonicalize first and then save. If the relative path doesn't exist, the canonicalization will fail but that shouldn't matter as you have reported above.

for lookup i'm thinking that we can exact string match first. it's cheap and keeps existing (canonical) keys working the same after these changes are merged.

If you mean going through literal tests first and then fall back to same_file tests, my intuition would be that since most of literal tests will fail (because they are prone to false negatives and thus need to fall back), it's probably better to just not add them.

I would love to measure the overhead before/after if that helps inform the decision though. Let me know if you think otherwise.

I'd be glad if the measurements could be made, and actually I'm also moving on that front but not sure if my patch can land while you are working on this issue. See #5103 and #2626. For the moment being you might really need to do some manual testing with hyperfine, or you may also have a look at #5104 and see if you can make use of the bench suite without the codspeed bench feature.

This is the rough shape I have in mind as far as commits, please let me know if it makes sense to you:

1. tests pinning the current behavior
2. stop canonicalizing on write
3. same-dir lookup
4. unset / --nonexistent semantics
5. flip the tests

Yeah, looks great to me as long as my analysis above sounds reasonable to you :)