Skip to content
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

Check if file is ignored according to git #98

Merged
merged 4 commits into from
Dec 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,137 changes: 1,112 additions & 25 deletions Cargo.lock

Large diffs are not rendered by default.

15 changes: 8 additions & 7 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "bacon"
version = "2.2.8"
version = "2.3.0-dev"
authors = ["dystroy <denys.seguret@gmail.com>"]
repository = "https://github.com/Canop/bacon"
description = "background rust compiler"
Expand All @@ -9,24 +9,25 @@ keywords = ["rust", "background", "compiler", "watch", "inotify"]
license = "AGPL-3.0"
categories = ["command-line-utilities", "development-tools"]
readme = "README.md"
rust-version = "1.59"
rust-version = "1.65"

[dependencies]
anyhow = "1.0.57"
anyhow = "1.0.68"
cargo_metadata = "0.14.0"
clap = { version = "3.1", features = ["derive"] }
cli-log = "2.0.0"
coolor = "=0.5.0"
crokey = "0.4.1"
crossbeam = "0.8.1"
crossbeam = "0.8.2"
directories-next = "2.0.0"
git-repository = "0.30.2"
lazy-regex = "2.3.1"
notify = "=5.0.0-pre.14"
notify = "5.0.0"
serde = { version = "1.0.136", features = ["derive"] }
termimad = "0.20.4"
termimad = "0.20.6"
tokio = { version = "1.17.0", default-features = false, features = ["net", "sync", "process", "rt", "macros", "io-util"] }
toml = "0.5.8"
unicode-width = "0.1.9"
unicode-width = "0.1.10"
vte = "0.8"

[profile.release]
Expand Down
1 change: 1 addition & 0 deletions bacon.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ command = [
"-A", "clippy::match_like_matches_macro",
"-A", "clippy::neg_multiply",
"-A", "clippy::vec_init_then_push",
"-A", "clippy::manual_clamp",
]
need_stdout = false
watch = ["tests", "benches", "examples"]
Expand Down
22 changes: 18 additions & 4 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,25 @@ pub fn run(
event_source: &EventSource,
) -> Result<Option<JobRef>> {
let keybindings = mission.settings.keybindings.clone();
let mut ignorer = time!(Info, mission.ignorer());
let (watch_sender, watch_receiver) = bounded(0);
let mut watcher = notify::recommended_watcher(move |res| match res {
Ok(_) => {
debug!("notify event received");
let mut watcher = notify::recommended_watcher(move |res: notify::Result<notify::Event>| match res {
Ok(we) => {
debug!("notify event: {we:?}");
if let Some(ignorer) = ignorer.as_mut() {
match time!(Info, ignorer.excludes_all(&we.paths)) {
Ok(true) => {
debug!("all excluded");
return;
}
Ok(false) => {
debug!("at least one is included");
}
Err(e) => {
warn!("exclusion check failed: {e}");
}
}
}
if let Err(e) = watch_sender.send(()) {
debug!("error when notifying on inotify event: {}", e);
}
Expand Down Expand Up @@ -63,7 +78,6 @@ pub fn run(
event_source.unblock(false);
}
recv(watch_receiver) -> _ => {
debug!("got a watcher event");
if let Err(e) = executor.start(state.new_task()) {
debug!("error sending task: {}", e);
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/command_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl CommandResult {
match self {
Self::Report(report) => {
let path = mission.bacon_locations_path();
let mut file = File::create(&path)?;
let mut file = File::create(path)?;
report.write_to(&mut file, mission)?;
}
Self::Failure(_) => { }
Expand Down
70 changes: 70 additions & 0 deletions src/ignorer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
use {
anyhow::{Context, Result},
git_repository::{
self as git,
prelude::FindExt,
Repository,
},
std::path::{Path, PathBuf},
};

/// An object able to tell whether a file is excluded
/// by gitignore rules
pub struct Ignorer {
repo: Repository,
}

impl Ignorer {

/// Create an Ignorer from any directory path: the closest
/// surrounding git repository will be found (if there's one)
/// and its gitignore rules used.
///
/// root_path is assumed to exist and be a directory
pub(crate) fn new(root_path: &Path) -> Result<Self> {
let repo = git::discover(root_path)?;
Ok(Self { repo })
}

/// Tell whether the given path is excluded according to
/// either the global gitignore rules or the ones of the repository
pub fn excludes(&mut self, file_path: &Path) -> Result<bool> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think as it stands, each path will trigger reading all gitignore files. They are indeed held in the excludes() data structure and ideally it is kept. I see how this isn't possible right now, and believe that the current reference is likely a premature optimization rather than a necessity. This will change for sure - and it's done in the latest main, which should greatly improve performance as the cache can actually reuse state if it's kept around.

Copy link
Owner Author

@Canop Canop Dec 29, 2022

Choose a reason for hiding this comment

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

the current reference is likely a premature optimization rather than a necessity

I feel you. I fell in the same trap in my first libs too, and it's a pain to fix.

TBH checking whether a file is excluded takes less than 1 ms right now and that's fine for bacon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will think about this - the number one usage of lifetimes in structs is platforms, they add some data around an operation and perform it, keeping a reference to their originating Repository. As long as these are basically free, I think they can be created on demand with the Repository being cloned to where it is needed - that's the intent.

If they aren't free though, like the Cache here, I think it's good advice to rather clone the Repository into it to make it standalone, or do whatever else it takes. I think some useful rule emerges from this experience and I will put it into words in DEVELOPMENT.md to make it official.

Copy link
Owner Author

@Canop Canop Dec 30, 2022

Choose a reason for hiding this comment

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

BTW @Byron I don't know if you may find this interesting, but I've also implemented a gitignoring stack (stacking the parsed gitignore files as I imagine you do): https://github.com/Canop/broot/blob/main/src/git/ignore.rs#L155

This was done for broot with very specific performance concerns (breathfirst tree diving).
The reasons I didn't take that for bacon were

  1. I didn't want to implement myself looking for the global gitignore rules (I use git2 in broot for that)
  2. I wanted to try gitoxide for other programs of mine (and maybe replacing git2 with gitoxide in my current programs)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sharing! I love the perceived simplicity of the git-ignore implementation, it fits in 240 lines after all :)!

With the Cache type unchained from the lifetime in main you would now be able to reuse it for each lookup and that should yield much better performance.

From a correctness point of view, it's probably (hopefully) a good idea to use gitoxide even if the performance is just similar, as I tried my best to validate the implementation against git with many many test cases. Of course I hope you won't take my word for it and validate it yourself, gitoxide strives to yield the same results as git.

Thus, I hope you will end up using gitoxide in more of your projects and if there is anything preventing that, I'd love to know to get a chance to fix it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

gitignoring is only part of using git, and I definitely don't want to expand into building a general git crate while there's already an ambitious project, so it's my clear intention to try and use gitoxide ;)

let worktree = self.repo.worktree().context("a worktree should exist")?;
let index = worktree.index()?;

// the "Cache" is the structure allowing checking exclusion
let mut cache = worktree.excludes(&index, None)?;

// cache.at_path panics if not provided a path relative
// to the work directory, so we compute the relative path
let Some(work_dir) = self.repo.work_dir() else {
return Ok(false);
};
let Ok(relative_path) = file_path.strip_prefix(work_dir) else {
return Ok(false);
};

// cache.at_path panics if the relative path is empty, so
// we must check that
if relative_path.as_os_str().is_empty() {
return Ok(true);
};

let platform = cache.at_path(relative_path, Some(file_path.is_dir()), |oid, buf| {
self.repo.objects.find_blob(oid, buf)
})?;

Ok(platform.is_excluded())
}

/// Return Ok(false) when at least one file is included (i.e. we should
/// execute the job)
pub fn excludes_all(&mut self, paths: &[PathBuf]) -> Result<bool> {
for path in paths {
if !self.excludes(path)? {
return Ok(false);
}
}
Ok(true)
}
}
4 changes: 4 additions & 0 deletions src/job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ pub struct Job {
/// for "cargo run" jobs
#[serde(default)]
pub allow_warnings: bool,

/// whether gitignore rules must be applied
pub apply_gitignore: Option<bool>,
}

static DEFAULT_ARGS: &[&str] = &["--color", "always"];
Expand All @@ -59,6 +62,7 @@ impl Job {
need_stdout: false,
on_success: None,
allow_warnings: false,
apply_gitignore: None,
}
}
}
2 changes: 1 addition & 1 deletion src/job_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl<'c> JobStack<'c> {


pub fn pick_job(&mut self, job_ref: &JobRef) -> Result<Option<(ConcreteJobRef, Job)>> {
info!("PICKING JOB {job_ref:?}");
debug!("picking job {job_ref:?}");
let concrete = match job_ref {
JobRef::Default => self.package_config.default_job.clone(),
JobRef::Initial => self.initial_job().clone(),
Expand Down
1 change: 0 additions & 1 deletion src/line.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ impl Line {
pub fn location(&self) -> Option<&str> {
match self.line_type {
LineType::Location => {
info!("CON: {:#?}", &self.content);
self.content.strings.get(2).map(|ts| ts.raw.as_str())
}
_ => None,
Expand Down
2 changes: 2 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ mod executor;
mod failure;
mod help_line;
mod help_page;
mod ignorer;
mod internal;
mod job;
mod job_ref;
Expand Down Expand Up @@ -46,6 +47,7 @@ pub use {
failure::*,
help_line::*,
help_page::*,
ignorer::*,
internal::*,
job::*,
job_ref::*,
Expand Down
23 changes: 22 additions & 1 deletion src/mission.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub struct Mission<'s> {
pub job_name: String,
pub cargo_execution_directory: PathBuf,
pub workspace_root: PathBuf,
job: Job,
pub job: Job,
files_to_watch: Vec<PathBuf>,
directories_to_watch: Vec<PathBuf>,
pub settings: &'s Settings,
Expand Down Expand Up @@ -77,6 +77,27 @@ impl<'s> Mission<'s> {
})
}

/// Return an Ignorer if required by the job's settings
/// and if the mission takes place in a git repository
pub fn ignorer(&self) -> Option<Ignorer> {
match self.job.apply_gitignore {
Some(false) => {
debug!("No gitignorer because of settings");
None
}
_ => { // by default we apply gitignore rules
match Ignorer::new(&self.workspace_root) {
Ok(ignorer) => Some(ignorer),
Err(e) => {
// might be normal, eg not in a git repo
debug!("Failed to initialise git ignorer: {e}");
None
}
}
}
}
}

/// Return the path to the bacon-locations file
pub fn bacon_locations_path(&self) -> PathBuf {
self.workspace_root.join(".bacon-locations")
Expand Down
1 change: 1 addition & 0 deletions website/docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ watch | yes | A list of directories that will be watched if the job is run on a
need_stdout | yes |whether we need to capture stdout too (stderr is always captured). Default is `false`
on_success | yes | the action to run when there's no error, warning or test failures
allow_warnings | yes | if true, the action is considered a success even when there are warnings. Default is `false` but the standard `run` job is configured with `allow_warnings=false`
apply_gitignore | yes | if true (which is default) the job isn't triggered when the modified file is excluded by gitignore rules

Example:

Expand Down