Repository: casey/just
Pull request #436 (2019-05-12 16:10:30)

Ignore justfile name case while searching

casey: I notice that this is a draft, so let me know when you'd like me to take a look.
shevtsiv: This is a draft because I haven't covered this features with tests yet, but I plan to leave this code in the first commit almost unchanged, so feel free to review this part, while I still working on test coverage. Once tests are done I'll mark this request as ready for review, so it can be reviewed then as well.
casey: Cool, just added some comments. I'm not sure where search should go, but I think it should probably be in its own file, like `src/search.rs`
shevtsiv: @casey I've added some test coverage for the justfile search function. Although this is work in progress, I would be grateful for review. Currently tests are located in the same file that search function and I'm not sure if it is a good idea. Also, I don't know if we should remove the old integration tests, because it is tests though and maybe it would be better to leave them as well for confidence that everything works properly. 
Another spot I'm worried about is `#[cfg(unix)]` above the `multiple_candidates` test function. I've used this attribute to disable this test running on Windows since it has case insensitive file system and there is no differences between `JustFile` and `justfile`, for example. But I'm not sure if it is a right way because I've also seen manual checking for the file system case sensitivity in the other tests(`test_capitalization_priority`).
casey: Great! I'm traveling the moment, so forgive me if it takes a few days before I get the chance to do a proper review.

Leaving the current integration tests in place is probably a good idea. They won't hurt, and they'll catch cases where the search code is correct, but it isn't being called correctly.

I'll be back home on Sunday, so I should be able to give you proper feedback soon after that.
shevtsiv: It seems `test_capitalization_priority` test is failing now. I suppose it's because we do not support prioritization anymore. If I am right, should I just delete this test? 
casey: Yup, that's right, we can just delete that test.
casey: It looks like the tests are green! Is this ready for another round of review?
shevtsiv: > It looks like the tests are green! Is this ready for another round of review?

I think so!
casey: Looks good!

Pushed a few additional commits to the branch:

- Rebased onto current master
-  I found `str::eq_ignore_ascii_case` in the standard library, so I removed the dependency on the caseless crate
- Organized imports in common.rs
- The old code would also change the working directory to the parent directory of the found justfile. The new code changed this, but I somehow didn't have a test for this which would have caught this. I added a test and changed the code to change the working directory, as before.
-Put the `justfile` string in a constantly, for slighly better readability
- "JuStFiLe" is, I think, more correctly referred to as [spongebob case](https://knowyourmeme.com/memes/mocking-spongebob), so I changed the name of the test.
- Reworked how some of the search errors are formatted

I also updated the docs to reflect the new functionality. Once the tests pass I'll merge this in.

Patch:
diff --git a/README.adoc b/README.adoc
index 3050eedc..363260d0 100644
--- a/README.adoc
+++ b/README.adoc
@@ -11,7 +11,7 @@ image:https://img.shields.io/badge/Say%20Thanks-!-1EAEDB.svg[say thanks,link=htt
 
 (非官方中文文档,link:https://github.com/chinanf-boy/just-zh[这里],快看过来!)
 
-Commands are stored in a file called `justfile` or `Justfile` with syntax inspired by `make`:
+Commands are stored in a file called `justfile` with syntax inspired by `make`:
 
 ```make
 build:
@@ -126,7 +126,9 @@ another-recipe:
     @echo 'This is another recipe.'
 ```
 
-When you invoke `just` it looks for a `justfile` in the current directory and upwards, so you can invoke it from any subdirectory of your project.
+When you invoke `just` it looks for file `justfile` in the current directory and upwards, so you can invoke it from any subdirectory of your project.
+
+The search for a `justfile` is case insensitive, so any case, like `Justfile`, `JUSTFILE`, or `JuStFiLe`, will work.
 
 Running `just` with no arguments runs the first recipe in the `justfile`:
 
@@ -744,7 +746,7 @@ if exists("did_load_filetypes")
 endif
 
 augroup filetypedetect
-  au BufNewFile,BufRead Justfile,justfile setf make
+  au BufNewFile,BufRead justfile setf make
 augroup END
 ```
 
diff --git a/src/common.rs b/src/common.rs
index a3b27dd2..2ac27e5d 100644
--- a/src/common.rs
+++ b/src/common.rs
@@ -20,41 +20,27 @@ pub(crate) use log::warn;
 pub(crate) use tempdir::TempDir;
 pub(crate) use unicode_width::UnicodeWidthChar;
 
+// Modules
+pub(crate) use crate::search;
+
+// Functions
 pub(crate) use crate::{
-  alias::Alias,
-  alias_resolver::AliasResolver,
-  assignment_evaluator::AssignmentEvaluator,
-  assignment_resolver::AssignmentResolver,
-  color::Color,
-  compilation_error::CompilationError,
-  compilation_error_kind::CompilationErrorKind,
-  configuration::Configuration,
-  expression::Expression,
-  fragment::Fragment,
-  function::Function,
-  function_context::FunctionContext,
-  functions::Functions,
-  interrupt_guard::InterruptGuard,
-  interrupt_handler::InterruptHandler,
-  justfile::Justfile,
-  lexer::Lexer,
   load_dotenv::load_dotenv,
   misc::{default, empty},
-  parameter::Parameter,
-  parser::Parser,
-  position::Position,
-  recipe::Recipe,
-  recipe_context::RecipeContext,
-  recipe_resolver::RecipeResolver,
-  runtime_error::RuntimeError,
-  shebang::Shebang,
-  state::State,
-  string_literal::StringLiteral,
-  token::Token,
-  token_kind::TokenKind,
-  use_color::UseColor,
-  variables::Variables,
-  verbosity::Verbosity,
+};
+
+// Structs and enums
+pub(crate) use crate::{
+  alias::Alias, alias_resolver::AliasResolver, assignment_evaluator::AssignmentEvaluator,
+  assignment_resolver::AssignmentResolver, color::Color, compilation_error::CompilationError,
+  compilation_error_kind::CompilationErrorKind, configuration::Configuration,
+  expression::Expression, fragment::Fragment, function::Function,
+  function_context::FunctionContext, functions::Functions, interrupt_guard::InterruptGuard,
+  interrupt_handler::InterruptHandler, justfile::Justfile, lexer::Lexer, parameter::Parameter,
+  parser::Parser, position::Position, recipe::Recipe, recipe_context::RecipeContext,
+  recipe_resolver::RecipeResolver, runtime_error::RuntimeError, search_error::SearchError,
+  shebang::Shebang, state::State, string_literal::StringLiteral, token::Token,
+  token_kind::TokenKind, use_color::UseColor, variables::Variables, verbosity::Verbosity,
 };
 
 pub type CompilationResult<'a, T> = Result<T, CompilationError<'a>>;
diff --git a/src/lib.rs b/src/lib.rs
index 2c19f867..2ec3471b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -43,6 +43,8 @@ mod recipe_context;
 mod recipe_resolver;
 mod run;
 mod runtime_error;
+mod search;
+mod search_error;
 mod shebang;
 mod state;
 mod string_literal;
diff --git a/src/run.rs b/src/run.rs
index 35d5d9e3..75d018ed 100644
--- a/src/run.rs
+++ b/src/run.rs
@@ -273,44 +273,30 @@ pub fn run() {
       );
     }
   } else {
-    let name;
-    'outer: loop {
-      for candidate in &["justfile", "Justfile"] {
-        match fs::metadata(candidate) {
-          Ok(metadata) => {
-            if metadata.is_file() {
-              name = *candidate;
-              break 'outer;
-            }
-          }
-          Err(error) => {
-            if error.kind() != io::ErrorKind::NotFound {
-              die!("Error fetching justfile metadata: {}", error)
-            }
-          }
+    let current_dir = match env::current_dir() {
+      Ok(current_dir) => current_dir,
+      Err(io_error) => die!("Error getting current dir: {}", io_error),
+    };
+    match search::justfile(&current_dir) {
+      Ok(name) => {
+        if matches.is_present("EDIT") {
+          edit(name);
         }
-      }
+        text = fs::read_to_string(&name)
+          .unwrap_or_else(|error| die!("Error reading justfile: {}", error));
 
-      match env::current_dir() {
-        Ok(pathbuf) => {
-          if pathbuf.as_os_str() == "/" {
-            die!("No justfile found.");
-          }
-        }
-        Err(error) => die!("Error getting current dir: {}", error),
-      }
+        let parent = name.parent().unwrap();
 
-      if let Err(error) = env::set_current_dir("..") {
-        die!("Error changing directory: {}", error);
+        if let Err(error) = env::set_current_dir(&parent) {
+          die!(
+            "Error changing directory to {}: {}",
+            parent.display(),
+            error
+          );
+        }
       }
+      Err(search_error) => die!("{}", search_error),
     }
-
-    if matches.is_present("EDIT") {
-      edit(name);
-    }
-
-    text =
-      fs::read_to_string(name).unwrap_or_else(|error| die!("Error reading justfile: {}", error));
   }
 
   let justfile = Parser::parse(&text).unwrap_or_else(|error| {
diff --git a/src/search.rs b/src/search.rs
new file mode 100644
index 00000000..d851728f
--- /dev/null
+++ b/src/search.rs
@@ -0,0 +1,163 @@
+use crate::common::*;
+use std::fs;
+use std::path::{Path, PathBuf};
+
+const FILENAME: &str = "justfile";
+
+pub fn justfile(directory: &Path) -> Result<PathBuf, SearchError> {
+  let mut candidates = Vec::new();
+  let dir = fs::read_dir(directory).map_err(|io_error| SearchError::Io {
+    io_error,
+    directory: directory.to_owned(),
+  })?;
+  for entry in dir {
+    let entry = entry.map_err(|io_error| SearchError::Io {
+      io_error,
+      directory: directory.to_owned(),
+    })?;
+    if let Some(name) = entry.file_name().to_str() {
+      if name.eq_ignore_ascii_case(FILENAME) {
+        candidates.push(entry.path());
+      }
+    }
+  }
+  if candidates.len() == 1 {
+    Ok(candidates.pop().unwrap())
+  } else if candidates.len() > 1 {
+    Err(SearchError::MultipleCandidates { candidates })
+  } else if let Some(parent_dir) = directory.parent() {
+    justfile(parent_dir)
+  } else {
+    Err(SearchError::NotFound)
+  }
+}
+
+#[cfg(test)]
+mod tests {
+  use super::*;
+  use std::fs;
+  use tempdir::TempDir;
+
+  #[test]
+  fn not_found() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    match search::justfile(tmp.path()) {
+      Err(SearchError::NotFound) => {
+        assert!(true);
+      }
+      _ => panic!("No justfile found error was expected"),
+    }
+  }
+
+  #[test]
+  fn multiple_candidates() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    let mut path = tmp.path().to_path_buf();
+    path.push(FILENAME);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    path.push(FILENAME.to_uppercase());
+    if let Ok(_) = fs::File::open(path.as_path()) {
+      // We are in case-insensitive file system
+      return;
+    }
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    match search::justfile(path.as_path()) {
+      Err(SearchError::MultipleCandidates { .. }) => {
+        assert!(true);
+      }
+      _ => panic!("Multiple candidates error was expected"),
+    }
+  }
+
+  #[test]
+  fn found() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    let mut path = tmp.path().to_path_buf();
+    path.push(FILENAME);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    match search::justfile(path.as_path()) {
+      Ok(_path) => {
+        assert!(true);
+      }
+      _ => panic!("No errors were expected"),
+    }
+  }
+
+  #[test]
+  fn found_spongebob_case() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    let mut path = tmp.path().to_path_buf();
+    let spongebob_case = FILENAME
+      .chars()
+      .enumerate()
+      .map(|(i, c)| {
+        if i % 2 == 0 {
+          c.to_ascii_uppercase()
+        } else {
+          c
+        }
+      })
+      .collect::<String>();
+    path.push(spongebob_case);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    match search::justfile(path.as_path()) {
+      Ok(_path) => {
+        assert!(true);
+      }
+      _ => panic!("No errors were expected"),
+    }
+  }
+
+  #[test]
+  fn found_from_inner_dir() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    let mut path = tmp.path().to_path_buf();
+    path.push(FILENAME);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    path.push("a");
+    fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
+    path.push("b");
+    fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
+    match search::justfile(path.as_path()) {
+      Ok(_path) => {
+        assert!(true);
+      }
+      _ => panic!("No errors were expected"),
+    }
+  }
+
+  #[test]
+  fn found_and_stopped_at_first_justfile() {
+    let tmp = TempDir::new("just-test-justfile-search")
+      .expect("test justfile search: failed to create temporary directory");
+    let mut path = tmp.path().to_path_buf();
+    path.push(FILENAME);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    path.push("a");
+    fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
+    path.push(FILENAME);
+    fs::write(&path, "default:\n\techo ok").unwrap();
+    path.pop();
+    path.push("b");
+    fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
+    match search::justfile(path.as_path()) {
+      Ok(found_path) => {
+        path.pop();
+        path.push(FILENAME);
+        assert_eq!(found_path, path);
+      }
+      _ => panic!("No errors were expected"),
+    }
+  }
+}
diff --git a/src/search_error.rs b/src/search_error.rs
new file mode 100644
index 00000000..ff82de7a
--- /dev/null
+++ b/src/search_error.rs
@@ -0,0 +1,62 @@
+use std::{fmt, io, path::PathBuf};
+
+use crate::misc::And;
+
+pub enum SearchError {
+  MultipleCandidates {
+    candidates: Vec<PathBuf>,
+  },
+  Io {
+    directory: PathBuf,
+    io_error: io::Error,
+  },
+  NotFound,
+}
+
+impl fmt::Display for SearchError {
+  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    match self {
+      SearchError::Io {
+        directory,
+        io_error,
+      } => write!(
+        f,
+        "I/O error reading directory `{}`: {}",
+        directory.display(),
+        io_error
+      ),
+      SearchError::MultipleCandidates { candidates } => write!(
+        f,
+        "Multiple candidate justfiles found in `{}`: {}",
+        candidates[0].parent().unwrap().display(),
+        And(
+          &candidates
+            .iter()
+            .map(|candidate| format!("`{}`", candidate.file_name().unwrap().to_string_lossy()))
+            .collect::<Vec<String>>()
+        ),
+      ),
+      SearchError::NotFound => write!(f, "No justfile found"),
+    }
+  }
+}
+
+#[cfg(test)]
+mod tests {
+  use super::*;
+
+  #[test]
+  fn multiple_candidates_formatting() {
+    let error = SearchError::MultipleCandidates {
+      candidates: vec![
+        PathBuf::from("/foo/justfile"),
+        PathBuf::from("/foo/JUSTFILE"),
+      ],
+    };
+
+    assert_eq!(
+      error.to_string(),
+      "Multiple candidate justfiles found in `/foo`: `justfile` and `JUSTFILE`"
+    )
+  }
+}


Test patch:
diff --git a/tests/search.rs b/tests/search.rs
index bbb05a4e..56058c9e 100644
--- a/tests/search.rs
+++ b/tests/search.rs
@@ -62,38 +62,6 @@ fn test_capitalized_justfile_search() {
   search_test(path, &[]);
 }
 
-#[test]
-fn test_capitalization_priority() {
-  let tmp = TempDir::new("just-test-justfile-search")
-    .expect("test justfile search: failed to create temporary directory");
-  let mut path = tmp.path().to_path_buf();
-  path.push("justfile");
-  fs::write(&path, "default:\n\techo ok").unwrap();
-  path.pop();
-  path.push("Justfile");
-  fs::write(&path, "default:\n\techo fail").unwrap();
-  path.pop();
-
-  // if we see "default\n\techo fail" in `justfile` then we're running
-  // in a case insensitive filesystem, so just bail
-  path.push("justfile");
-  if fs::read_to_string(&path).unwrap() == "default:\n\techo fail" {
-    return;
-  }
-  path.pop();
-
-  path.push("a");
-  fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
-  path.push("b");
-  fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
-  path.push("c");
-  fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
-  path.push("d");
-  fs::create_dir(&path).expect("test justfile search: failed to create intermediary directory");
-
-  search_test(path, &[]);
-}
-
 #[test]
 fn test_upwards_path_argument() {
   let tmp = TempDir::new("just-test-justfile-search")
diff --git a/tests/working_directory.rs b/tests/working_directory.rs
index 669ff42b..6becfc78 100644
--- a/tests/working_directory.rs
+++ b/tests/working_directory.rs
@@ -5,7 +5,7 @@ use tempdir::TempDir;
 /// Test that just runs with the correct working directory when invoked with
 /// `--justfile` but not `--working-directory`
 #[test]
-fn justfile_without_working_directory() -> Result<(), Box<Error>> {
+fn justfile_without_working_directory() -> Result<(), Box<dyn Error>> {
   let tmp = TempDir::new("just-integration")?;
   let justfile = tmp.path().join("justfile");
   let data = tmp.path().join("data");
@@ -30,3 +30,35 @@ fn justfile_without_working_directory() -> Result<(), Box<Error>> {
 
   Ok(())
 }
+
+/// Test that just invokes commands from the directory in which the justfile is found
+#[test]
+fn change_working_directory_to_justfile_parent() -> Result<(), Box<dyn Error>> {
+  let tmp = TempDir::new("just-integration")?;
+
+  let justfile = tmp.path().join("justfile");
+  fs::write(
+    &justfile,
+    "foo = `cat data`\ndefault:\n echo {{foo}}\n cat data",
+  )?;
+
+  let data = tmp.path().join("data");
+  fs::write(&data, "found it")?;
+
+  let subdir = tmp.path().join("subdir");
+  fs::create_dir(&subdir)?;
+
+  let output = Command::new(executable_path("just"))
+    .current_dir(subdir)
+    .output()?;
+
+  if !output.status.success() {
+    panic!("just invocation failed: {}", output.status)
+  }
+
+  let stdout = String::from_utf8(output.stdout).unwrap();
+
+  assert_eq!(stdout, "found it\nfound it");
+
+  Ok(())
+}