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();

build/pkgconf.sh Outdated
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

build/prelude.mk Outdated
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

configure Outdated
--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

src/ctx.c Outdated
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

src/ctx.h Outdated

#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.

src/parse.c Outdated
}

/**
* 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.

}

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.

@losnappas
Copy link
Author

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 fd . [my_projects_dir] --exclude .git --hidden.

I noticed a couple of differences with fd, but those aren't bad:

  • following a symlink into a dir with .gitignore but no repository doesn't respect the .gitignore (fd foes)
  • .git folder is ignored automatically, unlike fd

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.

@losnappas losnappas requested a review from tavianator October 16, 2025 10:26
@losnappas
Copy link
Author

losnappas commented Oct 28, 2025

Looks like the case without libgit2 is failing, meaning these lines: invoke_bfs -quit -ignore_vcs || skip are not skipping. I now see there's a bunch of stuff like BFS_CAN_CHECK_ACL that I should implement for this flag, too.

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.

@tavianator
Copy link
Owner

Looks like the case without libgit2 is failing, meaning these lines: invoke_bfs -quit -ignore_vcs || skip are not skipping. I now see there's a bunch of stuff like BFS_CAN_CHECK_ACL that I should implement for this flag, too.

Right. You'll need to make it raise an error here: https://github.com/tavianator/bfs/pull/162/files#diff-041fe350636d21dd5c64d5bc8ea741855ea2f15a9dca7e0132a083fcd141ce54R1617

Any comments before I go around fixing that?

I'll give it a more thorough review soon.

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.

I think it should be fine. bfs|dfs share most of the code, and ids|eds are just wrappers around dfs mostly.

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