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

Allow multiple search-and-replace paths #93

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ErichDonGubler
Copy link

@ErichDonGubler ErichDonGubler commented May 15, 2022

Intended to resolve #51, pending discussion (there's some trade-offs!). See individual commits for more details on changes.

Open questions:

  • What new tests should be added? Cases that I see:
    • The case mentioned in the first commit (refactor: skip duplicate canonicalized paths) is a good candidate.

Do our best to limit visiting paths to at most once, in preparation for
allowing multiple paths to be specified in a search-and-replace
operation. If we didn't do this, multiple path cases like this would be
handled incorrectly:

```
$ cat a.txt
foo

$ # We want to replace `foo` with `foobar`...
ruplacer foo foobar a.txt a.txt
<snip>

$ # ...but we got `foobarbar` instead:
$ cat a.txt
foobarbar
```

It's important to note what the trade-off for deduplication is. For each
path, we now call `std::fs::canonicalize`, and put the canonicalized
path in a set built using the the [`patricia_tree` crate] for somewhat
efficiently storage.

Interestingly, the above is also a potential issue with following
symlinks, which has never been allowed before. Perhaps this change could
make it sufficiently safe to add an option for following symlinks?

[`patricia_tree` crate]: https://docs.rs/patricia_tree/0.3.1/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept multiple paths on the command line
1 participant