-
Notifications
You must be signed in to change notification settings - Fork 325
feat(cli): Add an rg-equivalent CLI interface for fff#561
feat(cli): Add an rg-equivalent CLI interface for fff #561markovejnovic wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is up for debate whether we want to keep it this way or not.
I wanted to avoid adding another dependency to fff that is only used in the ipc code-path, but with optimizers being what they are, maybe that's not so bad.
either way, i chose this path of having two CaseModes (one in fff and one in fff-ipc-domain), but open to discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can have optional dependencies - this is absolutely fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to stop passing paths as strings. much better bet is to pass pathbufs as deep as possible until the fff barrier.
3a207ad to
9969bba
Compare
Bootstrap the CLI layer for FFF: - fff-ipc-domain: wire types and IPC protocol (Unix socket, bincode) - fff-daemon: background search daemon with session pooling, rg-compatible output formatting, and ANSI color matching - fff-rg: ripgrep-compatible CLI frontend with daemon/fallback searcher backends Includes 120 e2e tests: - 95 comparison tests (fff-rg vs rg side-by-side) across inline, heading, vimgrep, context, color, quiet, count, regex, unicode, and edge-case modes using test-case crate for parametrization - 25 synthetic repo scale tests (50/200/500 files) verifying match counts, line numbers, output formats, concurrency, and per-needle findability without rg comparison
9969bba to
c27a5ff
Compare
@dmtrKovalenko
dmtrKovalenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nits on the code I need to go through the way it actually works one more time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would require a major version bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we should get all the unix specific mode into a separate file cause I would love this to work on windows at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use From trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use our log crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not intermix imports and constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should restructure the crates to be somethjing like this
cli/
daemon
ffd
frg
ipc
this iwll make it much easier to keep backward compatibility with those tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we need mimaloc here - can drop a dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for tests I would prefer tests in the big repo using proptest on various flags + queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure we need it - if it's on a path Command will resolve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls do not override
Uh oh!
There was an error while loading. Please reload this page.
This PR adds a CLI interface to
fff. After talking with @dmtrKovalenko, we decided that the best architecture is to have anfffdaemon run in the background to keep the index hot. The command that the user interacts with isfff-rg, which talks to this daemon.Before we proceed, it's worth looking at the architecture diagram here:
Let's walk over each component:
fff-rgfff-rgcan search either through talking through thefff-daemon, or through shelling torg. It picks thefff-daemonif it believes to be working within a git repo. Otherwise, it will fall back torg, and if the user doesn't have it installed, it will abort.The core reasoning for this is that holding an
fff-daemonalive for directories that don't need to be indexed makes no sense, as you'll hold a very large amount of RAM in memory for an effectively one-off search.fff-daemonThe
fff-daemonholds within it aquery-service. Thisquery-serviceis responsible for accepting connections from new clients and then passing the request down to thesession-pool.The
session-poolis a pool of "active" sessions, ie. active git repositories for which we have anfffindex in memory. A couple notable facts:IPC protocol
I spent a good bit of time thinking through how best to handle the IPC protocol, and the epiphany I had is that we don't actually need to shuttle results between the daemon and the client. There may be hundreds, if not thousands of strings we'd need to copy, so if we can avoid it, that would be awesome.
On UNIX, you can pass file descriptors by using SCM_RIGHTS between processes. This enables us to take the stdout fd of the client, pass it to the daemon, and have the daemon directly write to the client's stdout. Neat!
The downside of this approach is that
SCM_RIGHTSrequires a unix domain socket, which means that passing the fd needs to happen over that. I evaluated:iceoryx2for all the other message passing, but ultimately I couldn't justify the maintenance burden. There's a ton of complexity added in figuring synchronizing the req-rep flow of themmaped iceoryx2 channels and the req-rep of the fd transfer, so it made no sense.This lives in the
crates/cli/fff-ipc-domaincrate.Holes in the implementation