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] Fix the list if symlink is in the same directory #174

Merged

Conversation

at-grandpa
Copy link
Contributor

@at-grandpa at-grandpa commented May 15, 2019

Fix the list if symlink is in the same directory.

test dir

$ pwd
/tmp/ghq_test
$ tree -a -L 3
.
├── main_dir
│   ├── a_dir
│   │   └── .git
│   ├── h_dir -> /tmp/ghq_test/sym_dir/h_dir/
│   └── z_dir
│       └── .git
└── sym_dir
    └── h_dir
        └── .git

9 directories, 0 files

ghq root

$ ghq root
/tmp/ghq_test

test

before (latest version)

$ ghq -v
ghq version 0.12.2 (rev:6e7f3c8)
$ ghq list
main_dir/a_dir
main_dir/h_dir
sym_dir/h_dir

main_dir/z_dir is not found.

after (this PR version)

setup.

$ pwd
/path/to/github.com/at-grandpa/ghq
$ git branch
* fix-list-symlink-in-same-directory
  master
$ make build
go get  -d
go mod tidy
go build  -ldflags="-s -w -X main.revision=360ea7a"

ghq list.

$ ./ghq list
main_dir/a_dir
main_dir/h_dir
main_dir/z_dir
sym_dir/h_dir

main_dir/z_dir is found.

Why?

If filepath.Walk returns filepath.SkipDir at symlink time, it will not reference subsequent files in the same directory. Therefore, it was not output to the list. So I have decided not to return filepath.SkipDir for symlinks.

@Songmu
Copy link
Member

Songmu commented May 15, 2019

Thank you! This is an acceptable change.

However, I think it is better to store whether it is a symlink or not into variable such as isSymlink and return nil or filepath.SkipDir by referring it at the end of the func block, rather than commonizing LocalRepositoryFromFile,

How do you think?

@at-grandpa
Copy link
Contributor Author

Thank you for your advice.

I was also worried about it. I thought it would be difficult to understand if I increased the new variables, so I chose commonizing LocalRepositoryFromFile.

But the following morning, I thought it was unnecessary commonality. And the return location is in the middle of the func block. So it is difficult to understand.

I find it easier to use isSymlink.

I'll fix it.

@at-grandpa
Copy link
Contributor Author

Oh..
Travis CI failed...

Go:tip

(snip)

# golang.org/x/xerrors
../../../../pkg/mod/golang.org/x/xerrors@v0.0.0-20190510150013-5403a72a6aaf/adaptor_go1_13.go:16:14: undefined: errors.Frame
../../../../pkg/mod/golang.org/x/xerrors@v0.0.0-20190510150013-5403a72a6aaf/format_go1_13.go:12:18: undefined: errors.Formatter
FAIL	github.com/motemen/ghq [build failed]

(snip)

I don't think it has anything to do with this correction.
Please help me...

@Songmu
Copy link
Member

Songmu commented May 16, 2019

This error is due to the problem that errors.Printer etc. was reverted by the following change, but xerrors haven't followed up with it yet.

golang/go@3e2c522

Since ghq is a tool, not a library, there is no need to keep track of the HEAD of the Go language development.
Could you remove the -tip line in .travis.yml?

@Songmu
Copy link
Member

Songmu commented May 16, 2019

Adjusted on my side and merged.

I'm grad for your contribution. It is a good job, test code is also sufficient.

Thank you!

@Songmu Songmu merged commit 41ef114 into x-motemen:master May 16, 2019
Songmu added a commit that referenced this pull request May 16, 2019
## [v0.12.3](v0.12.2...v0.12.3) (2019-05-16)

* [fix] Ignore files which seems to system hidden file in walking [#176](#176) ([Songmu](https://github.com/Songmu))
* [fix] Fix the list if symlink is in the same directory [#174](#174) ([at-grandpa](https://github.com/at-grandpa))
* [refactoring] introduce Songmu/gitconfig [#175](#175) ([Songmu](https://github.com/Songmu))
* [refactoring] Get ghq.completeUser strictly as a boolean value [#172](#172) ([Songmu](https://github.com/Songmu))
@at-grandpa
Copy link
Contributor Author

@Songmu Thank you! I'm glad I could contribute.

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