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

feat: list files by git when possible #133

Merged
merged 21 commits into from
Sep 5, 2022

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Sep 2, 2022

Although we try to use ignore pattern present by a .gitignore file, it's possible that users have a global git ignore settings or manually ignore a file.

We can use git command to list files when runs under a git worksapce, and if there's no git workspace, we don't have to see .gitignore. Normally, it's via directly download and all ignored files already excluded. If users want to respect to the ignore pattern, they can simply git init.

cc @kezhenxu94 @wu-sheng @fgksgf @spacewander

Reference:

@wu-sheng wu-sheng added this to the 0.5.0 milestone Sep 2, 2022
@wu-sheng wu-sheng added the enhancement New feature or request label Sep 2, 2022
@kezhenxu94
Copy link
Member

Although we try to use ignore pattern present by a .gitignore file, it's possible that users have a global git ignore settings or manually ignore a file.

We can use git command to list files when runs under a git worksapce,

This is a good idea, but it looks like the implementation in this PR only lists files that have been committed into the codebase, however, in most cases, I'm using the tool in the following workflow:

  • In a git repo, checkout a new branch.
  • Do modifications, for example, touch a-new-file.yaml.
  • ... many other new files.
  • license-eye h f to fix all files that are missing license headers.
  • git add . && git commit -m'...'.

In this PR, nothing will be fixed, even if I run license-eye h f after git add ..

This also breaks users who used this tool as a git pre-commit hook.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

@kezhenxu94 good point. Let me try to find a way to list staged/unstaged files.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

@kezhenxu94 fixed at 18af4c4.

pkg/header/check.go Outdated Show resolved Hide resolved
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically looks good to me, except that it doesn't resolve the problem as the PR description claimed, it's possible that users have a global git ignore settings., the files contained in global .gitignore are still checked.

Reproduce:

skywalking-eyes pr/tisonkun/133 % g
On branch pr/tisonkun/133
Your branch is up to date with 'tisonkun/list-files-by-git'.

nothing to commit, working tree clean

skywalking-eyes pr/tisonkun/133 % g config --global core.excludesFile       
~/.gitignore

skywalking-eyes pr/tisonkun/133 % tail -n1 ~/.gitignore 
.tool-versions

skywalking-eyes pr/tisonkun/133 % touch .tool-versions

skywalking-eyes pr/tisonkun/133 % go run cmd/license-eye/main.go h c
INFO Loading configuration from file: .licenserc.yaml 
INFO Totally checked 726 files, valid: 59, invalid: 1, ignored: 666, fixed: 0 
ERROR the following files don't have a valid license header: 
.tool-versions 
ERROR one or more files does not have a valid license header 
exit status 1

In terms of this case, it is neither better or worse than before so I'm OK if you want to stop at this point or continue to resolve that case. @tisonkun

pkg/header/check.go Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

This patch fixes my case under incubator-kvrocks:

➜  incubator-kvrocks git: ~/Brittani/skywalking-eyes/bin/darwin/license-eye -c tools/ci/licenserc.yml header check
INFO Loading configuration from file: tools/ci/licenserc.yml 
INFO Totally checked 250 files, valid: 212, invalid: 0, ignored: 38, fixed: 0 
➜  incubator-kvrocks git: ~/go/bin/license-eye -c tools/ci/licenserc.yml header check
INFO Loading configuration from file: tools/ci/licenserc.yml 
INFO Totally checked 4279 files, valid: 212, invalid: 2045, ignored: 2022, fixed: 0 
INFO GITHUB_TOKEN is not set, license-eye won't comment on the pull request 
ERROR the following files don't have a valid license header: 
cmake-build-debug/_deps/gtest-src/googletest/scripts/test/Makefile
cmake-build-debug/_deps/luajit-src/src/Makefile
...
cmake-build-debug/_deps/lz4-src/contrib/snap/snapcraft.yaml 
ERROR one or more files does not have a valid license header

... where:

cat $(git config --global core.excludesFile)
# JetBrains
.idea
*.iml
cmake-build-*

# VS Code
.vscode

# macOS
.DS_Store

# Ruby
.ruby-version

I'll try to dig it out a bit :)

@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

@kezhenxu94 I think we can handle the returned FileStatus. Previously, I assume that all ignored files don't occur in the status list, but it seems not true :)

@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

@kezhenxu94 it's a go-git wart. Workaround at 2687d4e.

There're still some issues about .gitignore files in nested path go-git/go-git#522. But previously we also only handle the gitignore file at root path of the repo. If someone else asks for it, they can nudge the upstream fix or switch to a patching mod replacement. I'll leave it as is for now.

Signed-off-by: tison <[email protected]>
@tisonkun
Copy link
Member Author

tisonkun commented Sep 4, 2022

This patch fixes my case

I spot that it's about another issue on the master branch. I'll create an issue for it.

UPDATE - apache/skywalking#9560

kezhenxu94
kezhenxu94 previously approved these changes Sep 5, 2022
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still a culprit in the go-git upstream, it doesn't expand the tilde (~) for the core.excludesfile, which I think it should, so this still doesn't solve my problem.

skywalking-eyes pr/tisonkun/133 % g config --global core.excludesfile
~/.gitignore

Also left some minor comment, the PR is good enough for me.

pkg/header/check.go Outdated Show resolved Hide resolved
pkg/header/check.go Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

it doesn't expand the tilde (~) for the core.excludesfile

I dug the code a bit. Finally, I notice that it's the Golang stdlib that causes the issue:

func main() {
	_, err := os.OpenFile("~/.gitignore", 0, 0666)
	fmt.Printf("%v", err)
}

// open ~/.gitignore: no such file or directory

@kezhenxu94 kezhenxu94 merged commit 7fca702 into apache:main Sep 5, 2022
@tisonkun tisonkun deleted the list-files-by-git branch September 5, 2022 03:20
@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

Set up a cross-reference to go-git/go-git#578. If upstream resolves this issue, we can upgrade to fix the ~ in path issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants