Add -vcs_ignore flag via libgit2#162
Conversation
tavianator
left a comment
There was a problem hiding this comment.
Left some comments but there is a major design flaw with the current implementation. To work properly it needs to support multiple active repos simultaneously. If you want I can sketch out the ideas I had for that but it would be a major refactor.
| git_libgit2_init(); | ||
| return 0; |
There was a problem hiding this comment.
| git_libgit2_init(); | |
| return 0; | |
| return git_libgit2_init(); |
| oniguruma) | ||
| LDLIB=-lonig | ||
| ;; | ||
| libgit2) |
There was a problem hiding this comment.
Should be sorted alphabetically
| liburing \ | ||
| oniguruma | ||
| oniguruma \ | ||
| libgit2 |
There was a problem hiding this comment.
Should be sorted alphabetically
| --with-libselinux --without-libselinux | ||
| --with-liburing --without-liburing | ||
| --with-oniguruma --without-oniguruma | ||
| --with-libgit2 --without-libgit2 |
There was a problem hiding this comment.
Should be sorted alphabetically
| #ifdef BFS_WITH_LIBGIT2 | ||
| #include <git2.h> | ||
| #endif |
There was a problem hiding this comment.
bfs coding style is to use #if for these, not #ifdef, so that #define BFS_WITH_LIBGIT2 false works as expected
| #ifdef BFS_WITH_LIBGIT2 | |
| #include <git2.h> | |
| #endif | |
| #if BFS_WITH_LIBGIT2 | |
| # include <git2.h> | |
| #endif |
|
|
||
| #ifdef BFS_WITH_LIBGIT2 | ||
| /** The git repository for the current root. */ | ||
| struct git_repository *git_repo; |
There was a problem hiding this comment.
This is the wrong place to store this. There is only one bfs_ctx, but there can be multiple git repos active at a time. Consider
$ bfs src/repo1 src/repo2or even just bfs src/repo in the presence of git submodules.
| } | ||
|
|
||
| /** | ||
| * Parse -ignore-vcs. |
There was a problem hiding this comment.
| * Parse -ignore-vcs. | |
| * Parse -ignore_vcs. |
| } | ||
|
|
||
| ctx->argc = argc; | ||
| ctx->ignore_vcs = false; |
There was a problem hiding this comment.
Don't think you need this, it's already set to false in bfs_ctx_new()
| # Detect whether -ignore_vcs has any effect (i.e., built with libgit2) | ||
| invoke_bfs . -name '*_file' -print >"$OUT.base" || fail | ||
| invoke_bfs . -ignore_vcs -name '*_file' -print >"$OUT.ign" || fail | ||
| if cmp -s "$OUT.base" "$OUT.ign"; then |
There was a problem hiding this comment.
You don't need to do this, just do
invoke_bfs -quit -ignore_vcs || skipat the top
|
Thanks for the review.
Is your idea still using libgit2? Like checking for repo in each dir? The way I use this tool, I don't care about nested repos, but I understand it's not as expected. Also, is the whole |
libgit2 is fine. I was originally going to write my own The hard part is to find somewhere to store the
My idea is to unify these into something like
I want
Yes it's needed, all of bfs's dependencies are optional. |
001f759 to
d20b422
Compare
|
Hi, this iteration provided by Gemini 2.5 (and myself). I ran it against my projects folder, and it provides the same result as running I noticed a couple of differences with
The number of test cases could be increased to almost infinity (symlinks, repos, .gitignores everywhere, etc.), but I'm pretty happy with it based on real-world testing. |
|
Looks like the case without Any comments before I go around fixing that? I paid zero attention to the fact that there's other algorithms for traversing (bfs|dfs|ids|eds), and did not check compatibility with any of those so far. |
Right. You'll need to make it raise an error here: https://github.com/tavianator/bfs/pull/162/files#diff-041fe350636d21dd5c64d5bc8ea741855ea2f15a9dca7e0132a083fcd141ce54R1617
I'll give it a more thorough review soon.
I think it should be fine. |
Fixes #42
This is entirely vibe coded, I don't really know C that well. It seemed to work, I didn't benchmark it (not really relevant for my use cases), I doubt this:
is working. I'm posting this in hopes that someone takes it the rest of the way. Or, with guidance, I can do that too but I'm not gonna be guessing blindly here.