Skip to content

Conversation

losnappas
Copy link

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:

it should be an on/off thing regardless of where it appears on the command line.

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.

Copy link
Owner

@tavianator tavianator left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +4 to +5
git_libgit2_init();
return 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
git_libgit2_init();
return 0;
return git_libgit2_init();

@@ -80,6 +80,9 @@ for LIB; do
oniguruma)
LDLIB=-lonig
;;
libgit2)
Copy link
Owner

Choose a reason for hiding this comment

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

Should be sorted alphabetically

@@ -67,4 +67,5 @@ ALL_PKGS := \
libcap \
libselinux \
liburing \
oniguruma
oniguruma \
libgit2
Copy link
Owner

Choose a reason for hiding this comment

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

Should be sorted alphabetically

@@ -53,6 +53,7 @@ External dependencies are auto-detected by default, but you can build --with or
--with-libselinux --without-libselinux
--with-liburing --without-liburing
--with-oniguruma --without-oniguruma
--with-libgit2 --without-libgit2
Copy link
Owner

Choose a reason for hiding this comment

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

Should be sorted alphabetically

Comment on lines +27 to +29
#ifdef BFS_WITH_LIBGIT2
#include <git2.h>
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

bfs coding style is to use #if for these, not #ifdef, so that #define BFS_WITH_LIBGIT2 false works as expected

Suggested change
#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;
Copy link
Owner

Choose a reason for hiding this comment

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

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/repo2

or even just bfs src/repo in the presence of git submodules.

@@ -1584,6 +1584,14 @@ static struct bfs_expr *parse_hidden(struct bfs_parser *parser, int arg1, int ar
return parse_nullary_test(parser, eval_hidden);
}

/**
* Parse -ignore-vcs.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* Parse -ignore-vcs.
* Parse -ignore_vcs.

@@ -3838,6 +3853,7 @@ struct bfs_ctx *bfs_parse_cmdline(int argc, char *argv[]) {
}

ctx->argc = argc;
ctx->ignore_vcs = false;
Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to do this, just do

invoke_bfs -quit -ignore_vcs || skip

at the top

@losnappas
Copy link
Author

losnappas commented Aug 23, 2025

Thanks for the review.

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.

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 #if BFS_WITH_LIBGIT2 needed/wanted, or can we just assume it exists?

@tavianator
Copy link
Owner

Thanks for the review.

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.

Is your idea still using libgit2? Like checking for repo in each dir?

libgit2 is fine. I was originally going to write my own .gitignore parser because I thought libgit2's license wasn't compatible with bfs, but actually it seems okay.

The hard part is to find somewhere to store the struct git_repository for every active path. Right now there are a couple different representations of active paths:

  • struct bftw_file is private to bftw.c. There is a separate one for each path component, with parent pointers.
  • struct BFTW is public, but there's only one alive at a time.

My idea is to unify these into something like struct bfs_path. This new type would chain like bftw_file, making it easy to hang extra data off each path component. So if you're at path/to/repo/sub/dir, you could grab the repo like path->parent->parent->repo.

The way I use this tool, I don't care about nested repos, but I understand it's not as expected.

I want bfs to work pretty much exactly like fd does regarding .gitignore support. If I merge an incomplete implementation that only supports one active repo, improving it in the future becomes a backwards-incompatible behaviour change. I try very hard to minimize those.

Also, is the whole #if BFS_WITH_LIBGIT2 needed/wanted, or can we just assume it exists?

Yes it's needed, all of bfs's dependencies are optional.

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.

Respect VCS ignore files
2 participants