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

[fix] Make sure the directory exists with the exact name #145

Merged
merged 2 commits into from
May 5, 2019

Conversation

knu
Copy link
Contributor

@knu knu commented May 1, 2019

Since os.Stat("CVS") successfully picks cvs on a case insensitive filesystem, we need to iterate through directory entries to see if a directory with the exact name exists.

cf. #115 (comment)

This certainly has an impact on performance, so any input is welcome.

Since `os.Stat("CVS")` successfully picks `cvs` on a case insensitive
filesystem, we need to iterate through directory entries to see if a
directory with the exact name exists.

cf. x-motemen#115 (comment)
@knu
Copy link
Contributor Author

knu commented May 1, 2019

Alternatively, you could check for the existence of a file CVS/Repository and omit a walk on a directory. It would still cause a false-positive result if you had a file named repository under cvs/, but does it sound like a fair trade-off?

@Songmu
Copy link
Member

Songmu commented May 2, 2019

Indeed, "CVS" has the possibility of conflicting with a regular directory name because no dots or underscores are there.

However, I do not want to reduce the overall performance for only CVS,
how about checking whether the name matches case sensitively exactly only with CVS and whether there is a Repository file under it?

@Songmu
Copy link
Member

Songmu commented May 5, 2019

I've fixed it by myself so I merge it now.

@Songmu Songmu merged commit 5cdce02 into x-motemen:master May 5, 2019
@Songmu
Copy link
Member

Songmu commented May 5, 2019

After all, I check only the existence of CVS/Repository at #154.

@knu knu deleted the case-sensitive-check branch May 5, 2019 14:11
@Songmu Songmu changed the title Make sure the directory exists with the exact name [fix] Make sure the directory exists with the exact name May 5, 2019
Songmu added a commit that referenced this pull request May 5, 2019
## [v0.11.1](v0.11.0...v0.11.1) (2019-05-05)

* [feature] List vcs option [#155](#155) ([msh5](https://github.com/msh5))
* [testing] add TestDoImport [#156](#156) ([Songmu](https://github.com/Songmu))
* [fix] fix findVCSBackend and add tests [#154](#154) ([Songmu](https://github.com/Songmu))
* [fix] Make sure the directory exists with the exact name [#145](#145) ([knu](https://github.com/knu))
* [bugfix] Fix fossil support [#153](#153) ([Songmu](https://github.com/Songmu))
* [testing] add TestDoList_query [#152](#152) ([Songmu](https://github.com/Songmu))
* [testing] add TestRunInDirSilently [#151](#151) ([Songmu](https://github.com/Songmu))
* [testing] add more tests in TestDoLook [#150](#150) ([Songmu](https://github.com/Songmu))
* [testing] add TestDoLook [#149](#149) ([Songmu](https://github.com/Songmu))
* [refactoring] remove NewFakeRunner which not used [#148](#148) ([Songmu](https://github.com/Songmu))
* [refactoring] Commonize doGet and doImport processing for refactoring [#147](#147) ([Songmu](https://github.com/Songmu))
* [testing] add test for `ghq root` [#146](#146) ([Songmu](https://github.com/Songmu))
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.

2 participants