From 83f8f49eb629cc4592d1e3d17b1862a4a6551fba Mon Sep 17 00:00:00 2001
From: Colin Casey <casey.colin@gmail.com>
Date: Thu, 14 Sep 2023 11:22:29 -0300
Subject: [PATCH] PR feedback

* updated error message
* using iterator instead of loop for finding buildpack dirs logic
* added a quick test to ensure that ignores work
* updated changelog entry
---
 CHANGELOG.md                                  |  3 +-
 libcnb-cargo/tests/integration_test.rs        | 41 +++++++++++++++++++
 .../src/buildpack_dependency_graph.rs         |  2 +-
 libcnb-package/src/lib.rs                     | 26 ++++++------
 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index db59f78a..697d0e02 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,8 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `libcnb-cargo`:
   - No longer outputs paths for non-libcnb.rs and non-meta buildpacks. ([#657](https://github.com/heroku/libcnb.rs/pull/657))
   - Build output for humans changed slightly, output intended for machines/scripting didn't change. ([#657](https://github.com/heroku/libcnb.rs/pull/657))
-- `libcnb-package`
-  - dropped `ignore` argument from `find_buildpack_dirs` that allowed for paths to be filtered out of the directory traversal. This function will now respect standard ignore files (like `.gitignore`) while traversing. 
+  - When performing buildpack detection, standard ignore files (`.ignore` and `.gitignore`) will be respected 
 
 ## [0.14.0] - 2023-08-18
 
diff --git a/libcnb-cargo/tests/integration_test.rs b/libcnb-cargo/tests/integration_test.rs
index 6abb7cfe..fe517167 100644
--- a/libcnb-cargo/tests/integration_test.rs
+++ b/libcnb-cargo/tests/integration_test.rs
@@ -219,6 +219,47 @@ fn package_command_error_when_run_in_project_with_no_buildpacks() {
     );
 }
 
+#[test]
+#[ignore = "integration test"]
+fn package_command_respects_ignore_files() {
+    let fixture_dir = copy_fixture_to_temp_dir("multiple_buildpacks").unwrap();
+
+    // when a git folder is not present, .ignore should be used
+    let ignore_file = fixture_dir.path().join(".ignore");
+    fs::write(&ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap();
+
+    let output = Command::new(CARGO_LIBCNB_BINARY_UNDER_TEST)
+        .args(["libcnb", "package", "--release"])
+        .current_dir(fixture_dir.path())
+        .output()
+        .unwrap();
+
+    //assert_ne!(output.status.code(), Some(0));
+    assert_eq!(
+        String::from_utf8_lossy(&output.stderr),
+        "🚚 Preparing package directory...\n🖥\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\n🏗\u{fe0f} Building buildpack dependency graph...\n🔀 Determining build order...\n❌ No buildpacks found!\n"
+    );
+
+    fs::remove_file(ignore_file).unwrap();
+
+    // when a git folder is not present, .gitignore can be used
+    fs::create_dir(fixture_dir.path().join(".git")).unwrap();
+    let ignore_file = fixture_dir.path().join(".gitignore");
+    fs::write(ignore_file, "meta-buildpacks\nbuildpacks\n").unwrap();
+
+    let output = Command::new(CARGO_LIBCNB_BINARY_UNDER_TEST)
+        .args(["libcnb", "package", "--release"])
+        .current_dir(fixture_dir.path())
+        .output()
+        .unwrap();
+
+    //assert_ne!(output.status.code(), Some(0));
+    assert_eq!(
+        String::from_utf8_lossy(&output.stderr),
+        "🚚 Preparing package directory...\n🖥\u{fe0f} Gathering Cargo configuration (for x86_64-unknown-linux-musl)\n🏗\u{fe0f} Building buildpack dependency graph...\n🔀 Determining build order...\n❌ No buildpacks found!\n"
+    );
+}
+
 fn validate_packaged_buildpack(packaged_buildpack_dir: &Path, buildpack_id: &BuildpackId) {
     assert!(packaged_buildpack_dir.join("buildpack.toml").exists());
     assert!(packaged_buildpack_dir.join("package.toml").exists());
diff --git a/libcnb-package/src/buildpack_dependency_graph.rs b/libcnb-package/src/buildpack_dependency_graph.rs
index a696288d..d99b35f0 100644
--- a/libcnb-package/src/buildpack_dependency_graph.rs
+++ b/libcnb-package/src/buildpack_dependency_graph.rs
@@ -84,7 +84,7 @@ fn build_libcnb_buildpack_dependency_graph_node(
 
 #[derive(thiserror::Error, Debug)]
 pub enum BuildBuildpackDependencyGraphError {
-    #[error("IO error while finding buildpack directories: {0}")]
+    #[error("Error while finding buildpack directories: {0}")]
     FindBuildpackDirectories(ignore::Error),
     #[error("Cannot read buildpack descriptor: {0}")]
     ReadBuildpackDescriptorError(TomlFileError),
diff --git a/libcnb-package/src/lib.rs b/libcnb-package/src/lib.rs
index 855180b2..96aea209 100644
--- a/libcnb-package/src/lib.rs
+++ b/libcnb-package/src/lib.rs
@@ -109,18 +109,20 @@ fn create_file_symlink<P: AsRef<Path>, Q: AsRef<Path>>(
 /// Will return an `Err` if any I/O errors happen while walking the file system or any parsing errors
 /// from reading a gitignore file.
 pub fn find_buildpack_dirs(start_dir: &Path) -> Result<Vec<PathBuf>, ignore::Error> {
-    let mut buildpack_dirs = vec![];
-    for result in ignore::Walk::new(start_dir) {
-        match result {
-            Ok(entry) => {
-                if entry.path().is_dir() && entry.path().join("buildpack.toml").exists() {
-                    buildpack_dirs.push(entry.path().to_path_buf());
-                }
-            }
-            Err(error) => return Err(error),
-        }
-    }
-    Ok(buildpack_dirs)
+    ignore::Walk::new(start_dir)
+        .collect::<Result<Vec<_>, _>>()
+        .map(|entries| {
+            entries
+                .iter()
+                .filter_map(|entry| {
+                    if entry.path().is_dir() && entry.path().join("buildpack.toml").exists() {
+                        Some(entry.path().to_path_buf())
+                    } else {
+                        None
+                    }
+                })
+                .collect()
+        })
 }
 
 /// Returns the path of the root workspace directory for a Rust Cargo project. This is often a useful