Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Ignore symlinks. You can't move them, and we don't modify them. #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csilvers
Copy link
Member

I don't know that this is strictly necessary: in practice what seems
to happen when you modify a file that's actually a symlink is that we
modify the underlying file (that is, open(f, 'w') opens the file
that the symlink points to, and leaves the actual link untouched).
And when we try to modify the file the second time, under its new
name, then all the codemods have already been applied via the symlink
so it's a noop, and all is good.

But I don't know if that's posix standard, and anyway it seems
confusing. Now we notice symlinks and just skip over them.

Summary:
I don't know that this is strictly necessary: in practice what seems
to happen when you modify a file that's actually a symlink is that we
modify the underlying file (that is, `open(f, 'w')` opens the file
that the symlink points to, and leaves the actual link untouched).
And when we try to modify the file the second time, under its new
name, then all the codemods have already been applied via the symlink
so it's a noop, and all is good.

But I don't know if that's posix standard, and anyway it seems
confusing.  Now we notice symlinks and just skip over them.

Test Plan: make test

Reviewers: benkraft

Subscribers: davidflanagan, carter

Differential Revision: https://phabricator.khanacademy.org/D41041
@csilvers
Copy link
Member Author

@benjaminjkraft sez: "Sadly this will break the (admittedly fragile) way I cached resolve_paths -- it depends on sharing the same path-filter function object across suggestor runs, whereas now we'll have a new one each time (even if the root doesn't change). Honestly the easiest thing if this is an issue is probably to remove the caching; pruning directories makes path resolution fast enough that it doesn't really matter anymore."

@benjaminjkraft
Copy link
Contributor

Yeah. As we discussed I think this is philosophically sensible. I think either removing the caching (assuming perf is ok), or improving it such that it will work right, is a good idea to clean up the code. It's a bigger change (at least in the latter case) but I don't think it will make things net-messier.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants