Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Adds support for .gitignore at repository root #420

Closed
wants to merge 1 commit into from
Closed

Adds support for .gitignore at repository root #420

wants to merge 1 commit into from

Conversation

osklyar
Copy link

@osklyar osklyar commented Jun 12, 2017

This PR adds basic support for .gitignore files. The code uses github.com/monochromegane/go-gitignore, which, however, could be easily ported into a local package to avoid an additional external dependency.

The PR covers 99% of the normal .gitignore usage, however, the following limitations as compared to the standard git client exist:

  • only .gitignore files at the root of the repository are considered, files located deeper in the tree structure are not taken into account
  • files matching the exclusion pattern in the .gitignore will be considered not present on the file system, so if forcefully committed, they may be shown as a change (has not been investigated).

The effect of the change is easily seen in the following example using the go-git based client from github.com/silvertern/geat (WIP):

Older state, without the PR:

➜  geat git:(01172b3) geat status
.: [email protected]:silvertern/geat.git [origin] HEAD Changed
?? .idea/workspace.xml
?? .idea/geat.iml
?? .idea/modules.xml
?? .idea/vcs.xml
?? .idea/libraries/GOPATH__geat_.xml
?? .idea/misc.xml
?? vendor/github.com/caarlos0/spin/.gitignore
?? vendor/github.com/caarlos0/spin/.travis.yml
?? vendor/github.com/caarlos0/spin/README.md
?? vendor/github.com/caarlos0/spin/spin_test.go
?? vendor/github.com/caarlos0/spin/Makefile
?? vendor/github.com/caarlos0/spin/spin.go
?? Gopkg.lock

With the PR:

➜  geat git:(master) dep ensure
➜  geat git:(master) go install
➜  geat git:(master) geat status
.: [email protected]:silvertern/geat.git [origin] master Clean

➜  geat git:(master) echo "change" >> Gopkg.toml 
➜  geat git:(master) ✗ geat status                
.: [email protected]:silvertern/geat.git [origin] master Changed
 M Gopkg.toml

Addresses #307, #340

@smola smola requested review from smola, mcuadros and alcortesm June 13, 2017 12:19
.gitignore Outdated
@@ -1 +1,4 @@
coverage.out
vendor/
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid adding content related to your choice of tools and editors to the project.

I suggest using a global $HOME/.gitignore file as an alternative.

git config --global core.excludesfile $HOME/.gitignore

@codecov
Copy link

codecov bot commented Jun 14, 2017

Codecov Report

Merging #420 into master will decrease coverage by 0.61%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
- Coverage    77.8%   77.18%   -0.62%     
==========================================
  Files         124      124              
  Lines        9019     9020       +1     
==========================================
- Hits         7017     6962      -55     
- Misses       1229     1300      +71     
+ Partials      773      758      -15
Impacted Files Coverage Δ
utils/merkletrie/filesystem/node.go 68.57% <100%> (+2.94%) ⬆️
plumbing/transport/ssh/common.go 0% <0%> (-50.91%) ⬇️
plumbing/transport/ssh/auth_method.go 33.33% <0%> (-24.77%) ⬇️
plumbing/transport/git/common.go 70.73% <0%> (-10.23%) ⬇️
remote.go 70.81% <0%> (-0.1%) ⬇️
plumbing/transport/file/client.go 100% <0%> (+6.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6ee806...ad35977. Read the comment docs.

@osklyar
Copy link
Author

osklyar commented Jun 14, 2017

@alcortesm done, changes to .gitignore removed

Copy link
Contributor

@mcuadros mcuadros left a comment

Choose a reason for hiding this comment

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

Maybe the filesystem abstraction is not the best place for Implementing the .gitignore.
The right place IMHO is the Worktree, because in git you have some operations such as git add --force where the .gitignore is ignored. So you need to be aware of the ignored files at the Worktree level.

@osklyar
Copy link
Author

osklyar commented Jun 14, 2017

@mcuadros The limitation has been listed in the original PR message, so it is a known limitation. I would generally agree with you for the change to be complete, but it would be much more involved and I would rather suggest doing it in a second iteration. I would not be able to offer a PR now for a change as suggested for the reasons of time constraints.

@mcuadros
Copy link
Contributor

@osklyar what I am asking is for move this to Worktree, not about implement git add force. Otherwise we will need to redo this PR as soon as we need the force, or any other similar feature.

@osklyar
Copy link
Author

osklyar commented Jun 14, 2017

@mcuadros Ok, fair enough. If it is more or less a move of the code I will investigate and try to provide the corresponding change.

@osklyar
Copy link
Author

osklyar commented Jun 14, 2017

Ok, I now know how to introduce this change in the Worktree directly, however, I have found out that the third-party gitignore pattern matcher that I've been using the change is incorrect on some patterns. Therefore, I will close this PR, work on a change to have a clean pattern matcher in the Worktree and then submit a new one.

@osklyar osklyar closed this Jun 14, 2017
@osklyar osklyar mentioned this pull request Jun 15, 2017
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.

3 participants