Skip to content

fix(files): fixes #78, correctly handle .gitignore #83

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

Merged
merged 2 commits into from
Jan 25, 2022
Merged

fix(files): fixes #78, correctly handle .gitignore #83

merged 2 commits into from
Jan 25, 2022

Conversation

jgottzen
Copy link
Contributor

Plenary's scandir only checks for .gitignore files in the directories it
scans. Since directories are only scanned when opened any .gitignore in
any parent directory isn't applied.

Implements handling of .gitignore files anywhere in the tree. The parsing
and checking logic is copied from the Plenary implementation.

@jgottzen
Copy link
Contributor Author

This is the draft I mentioned in #78, cleaned up.

It checks any directory scanned for a .gitignore file and only creates a file item if the file/directory isn't ignored.

There are mainly three cases that isn't handled right now:

  1. If the tree root is set to a sub-directory of a git repo, no git ignore filter is applied since none is found. This could easily be remedied by traversing upwards until a .git directory is found.
  2. Git submodules are treated like any other directory, meaning the parent repo .gitignore will also apply.
  3. Dotfiles-management, similar to 1, but since there will most likely be no .git directory upwards in the tree that solution will not suffice. Traversing upwards and stopping at the $HOME directory and apply any .gitignore there can be a solution.

@cseickel
Copy link
Contributor

  1. To get the gitignore from a sub folder, I do have a function in the utils module which I just modified in a separate branch to accept a path other than cwd:
    M.get_git_project_root = function(path)
  2. I think using the above method would be effective for submodules as well, but I am not actively using any right now to test this with.
  3. I'm not sure how a dotfiles management is different from any other git project. I use dotbot and it's just a standard git repo for me. Would a home directory gitignore take effect if a git project does not have it's own?

@cseickel
Copy link
Contributor

One major issue I notice with this is that it is storing the filters as global state. We can easily have multiple trees open in different tabs which each need their own state. I would make gitignore.parse() return the patterns table instead of storing it in the module, which can then be stored in the state object and passed back as an argument to the is_ignored() function.

@jgottzen
Copy link
Contributor Author

Yeah, will do.

I think manually parsing out .gitignore files is problematic, and will cause a lot of issues. Relying on git to do it is a lot safer, and easier, i.e. git status --porcelain --ignored=matching.

@cseickel
Copy link
Contributor

Absolutely, I think the git status --porcelain --ignored=matching is the way to go. I was worried about speed at first but I tried it out on my monorepo from work and it seems pretty efficient.

You likely noticed this as well, but just to be sure: I noticed is that when a directory is ignored, it is only returning the directory and not the file contents, which is great because something like node_modules would be huge. This means that we probably build two kinds of filters out of this result, a file entry that is an exact match and a directory entry which is a prefix match.

Thanks for working on this, I think the end result is going to be great!

@levouh
Copy link
Contributor

levouh commented Jan 23, 2022

I think the git status --porcelain --ignored=matching is the way to go

+1 to this. I've seen git status take ages and I am not entirely sure why, but usually doing something like git fsck or running it again (assuming there is some caching going on?) results in speedups for subsequent runs.

Regardless, it definitely seems better to let git do it as it will obviously handle things that the plenary.nvim implementation misses. I made this (rather naive) PR to plenary.nvim as the one there doesn't handle global ignore. Whichever route you end up going here, supporting that would be awesome 😉

@jgottzen
Copy link
Contributor Author

Changed the implementation to use git status --ignored=matching instead.

There's no need to use separate filters for directories and files, it can easily be handled as one filter.

I also moved the status flagging from utils.lua to the new neo-tree/git/init.lua file. But that should be consolidated into one call since they basically do the same thing, but that's for another time.

Plenary's scandir only checks for .gitignore files in the directories it
scans. Since directories are only scanned when opened any .gitignore in
any parent directory isn't applied.

Implements parsing the output of `git status --ignored=matching`.
@jgottzen jgottzen marked this pull request as ready for review January 24, 2022 11:55
@cseickel
Copy link
Contributor

It looks great, but when I tried it out I keep getting this error on every navigate/refresh:

[neo-tree.nvim] [ERROR 12:27:24] ...packer/start/neo-tree.nvim/lua/neo-tree/events/queue.lua:144: Error in event 
handler for event before_render[filesystem. before_render]: vim/shared.lua:0: s: expected string, got nil

I'm not sure what causes it yet.

@cseickel
Copy link
Contributor

Of course, the problem went away when I added some log messages, and now it's not repeating. I will keep using this throughout my workday and see what happens.

@jgottzen
Copy link
Contributor Author

@levouh I'm not sure about git status taking a long time, but I've included --no-optional-locks, see git(1) GIT_OPTIONAL_LOCKS for more info.

@cseickel
Copy link
Contributor

I found the problem! The error shows reliably when in a repository with no changes. Classic Heisenbug, as soon as you start working on it, it goes away.

@jgottzen
Copy link
Contributor Author

Weird, that worked for me previously, but when I tested it now, I had the same error as you. I must have screwed up my testing somehow.

Just pushed a fix for it.

@cseickel cseickel merged commit 1020de3 into nvim-neo-tree:main Jan 25, 2022
@jgottzen jgottzen deleted the gitignore branch January 25, 2022 14:06
@cseickel
Copy link
Contributor

It is so awesome to finally have a functional gitgnore option. I don't think any of the trees I have used in the past did this correctly. I'll use the it for the day and release tonight if there are no issues.

Thanks @jgottzen !

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.

3 participants