From f623403fab17806e8c651a3ded56f864d2a8c42a Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Thu, 28 Sep 2023 15:16:14 +0100
Subject: [PATCH 1/9] Add helper to test canonical case

---
 R/util.R                   | 21 +++++++++++++++++++++
 tests/testthat/test-util.R | 14 ++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/R/util.R b/R/util.R
index ee829461..98cb7747 100644
--- a/R/util.R
+++ b/R/util.R
@@ -599,3 +599,24 @@ pretty_bytes <- function(n) {
   }
   paste(prettyNum(round(n, 1), big.mark = ","), unit)
 }
+
+
+file_canonical_case <- function(path, workdir) {
+  if (length(path) != 1) {
+    return(vcapply(path, file_canonical_case, workdir, USE.NAMES = FALSE))
+  }
+  stopifnot(!fs::is_absolute_path(path))
+  path_split <- tolower(fs::path_split(path)[[1]])
+  base <- workdir
+  ret <- character(length(path_split))
+  for (i in seq_along(path_split)) {
+    pos <- dir(base)
+    j <- which(path_split[[i]] == tolower(pos))
+    if (length(j) != 1) {
+      return(NA_character_)
+    }
+    ret[[i]] <- pos[[j]]
+    base <- file.path(base, pos[[j]])
+  }
+  paste(ret, collapse = "/")
+}
diff --git a/tests/testthat/test-util.R b/tests/testthat/test-util.R
index 0d112df8..3ea032e8 100644
--- a/tests/testthat/test-util.R
+++ b/tests/testthat/test-util.R
@@ -292,3 +292,17 @@ test_that("can collapse with special last case", {
   expect_equal(collapse(x[1], " or "), "x")
   expect_equal(collapse(x[0], " or "), "")
 })
+
+
+test_that("can convert files to canonical case", {
+  tmp <- withr::local_tempdir()
+  p <- file.path(tmp, "a", "b", "c")
+  fs::dir_create(dirname(p))
+  file.create(p)
+  expect_equal(file_canonical_case("a/b/c", tmp), "a/b/c")
+  expect_equal(file_canonical_case("a//b//c", tmp), "a/b/c")
+  expect_equal(file_canonical_case("a/B/c", tmp), "a/b/c")
+  expect_equal(file_canonical_case("A/B/C", tmp), "a/b/c")
+  expect_equal(file_canonical_case("A/win~1/C", tmp), NA_character_)
+  expect_equal(file_canonical_case(c("a/b/c", "a/b/d"), tmp), c("a/b/c", NA))
+})

From 4d14e7672b7ae022e7fed402326a644dccb71a21 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Thu, 28 Sep 2023 16:41:20 +0100
Subject: [PATCH 2/9] Add new assertion function

---
 R/util_assert.R                   | 32 ++++++++++++++++++++--
 tests/testthat/test-util-assert.R | 44 +++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/R/util_assert.R b/R/util_assert.R
index 96107fc5..301fe3cb 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -4,9 +4,9 @@ assert_scalar <- function(x, name = deparse(substitute(x))) {
   }
 }
 
-assert_character <- function(x, name = deparse(substitute(x))) {
+assert_character <- function(x, name = deparse(substitute(x)), call = NULL) {
   if (!is.character(x)) {
-    stop(sprintf("'%s' must be character", name), call. = FALSE)
+    cli::cli_abort("'{name}' must be character", call = call)
   }
 }
 
@@ -60,6 +60,34 @@ assert_file_exists <- function(x, workdir = NULL, name = "File") {
   }
 }
 
+
+assert_file_exists2 <- function(files, workdir, name, call = NULL) {
+  assert_character(files, call = call)
+  err <- !file_exists(files, workdir = workdir)
+  if (any(err)) {
+    n <- cli::qty(sum(err))
+    cli::cli_abort(
+      c("{name}{n}{?s} {?does/do} not exist: {collapseq(files[err])}",
+        i = "Looked within directory '{workdir}'"),
+      call = call)
+  }
+
+  files_canonical <- file_canonical_case(files, workdir)
+  err <- fs::path(files) != files_canonical
+  if (any(err)) {
+    n <- cli::qty(sum(err))
+    hint_case <- sprintf("For '%s', did you mean '%s'?",
+                         files[err], files_canonical[err])
+    cli::cli_abort(
+      c("{name}{n}{?s} {?does/do} not exist: {collapseq(files[err])}",
+        set_names(hint_case, "i"),
+        i = paste("If you don't use the canonical case for a file, your code",
+                  "is not portable across different platforms"),
+        i = "Looked within directory '{workdir}'"),
+      call = call)
+  }
+}
+
 assert_is_directory <- function(x, workdir = NULL, name = "Directory") {
   assert_file_exists(x, workdir, name)
   path <- if (is.null(workdir)) x else file.path(workdir, x)
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index 7a4d4574..e704b40f 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -51,6 +51,50 @@ test_that("assert_file_exists", {
 })
 
 
+test_that("assert_file_exists2 works checks if files exist", {
+  tmp <- withr::local_tempdir()
+  file.create(file.path(tmp, "c"))
+  expect_error(assert_file_exists2("a", tmp, "File"),
+               "File does not exist: 'a'")
+  expect_error(assert_file_exists2(c("a", "b"), tmp, "File"),
+               "Files do not exist: 'a', 'b'")
+  expect_error(assert_file_exists2(c("a", "b", "c", "d"), tmp, "File"),
+               "Files do not exist: 'a', 'b', 'd'")
+  expect_silent(assert_file_exists2("c", tmp, "File"))
+})
+
+
+test_that("assert_file_exists2 informs about case mismatch", {
+  testthat::skip_if_not_installed("mockery")
+  mock_file_exists <- mockery::mock(TRUE, cycle = TRUE)
+  mockery::stub(assert_file_exists2, "file_exists", mock_file_exists)
+
+  tmp <- withr::local_tempdir()
+  file.create(file.path(tmp, "a"))
+  fs::dir_create(file.path(tmp, "b/c"))
+  file.create(file.path(tmp, "b/c/d"))
+
+  err <- expect_error(
+    assert_file_exists2("A", tmp, "File"),
+    "File does not exist: 'A'")
+  expect_length(err$body, 3)
+  expect_equal(names(err$body), c("i", "i", "i"))
+  expect_equal(err$body[[1]], "For 'A', did you mean 'a'?")
+  expect_match(err$body[[2]], "If you don't use the canonical case for a file")
+  expect_match(err$body[[3]], "Looked within directory '.+'")
+
+  err <- expect_error(
+    assert_file_exists2(c("A", "b/C/d"), tmp, "File"),
+    "Files do not exist: 'A', 'b/C/d'")
+  expect_length(err$body, 4)
+  expect_equal(names(err$body), c("i", "i", "i", "i"))
+  expect_equal(err$body[[1]], "For 'A', did you mean 'a'?")
+  expect_equal(err$body[[2]], "For 'b/C/d', did you mean 'b/c/d'?")
+  expect_match(err$body[[3]], "If you don't use the canonical case for a file")
+  expect_match(err$body[[4]], "Looked within directory '.+'")
+})
+
+
 test_that("assert_is_directory", {
   path <- tempfile(tmpdir = normalise_path(tempdir()))
   expect_error(assert_is_directory(path), "Directory does not exist")

From a94871e9d427f86109ae08f6df694159e0177bf6 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Thu, 28 Sep 2023 16:41:29 +0100
Subject: [PATCH 3/9] Use assertion function within resource helper

---
 R/metadata.R              | 25 ++++++++++---------------
 tests/testthat/test-run.R | 12 ++++++++++++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/R/metadata.R b/R/metadata.R
index a4e4b307..42d72ff6 100644
--- a/R/metadata.R
+++ b/R/metadata.R
@@ -149,25 +149,20 @@ static_orderly_description <- function(args) {
 ##'
 ##' @export
 orderly_resource <- function(files) {
-  ## TODO: an error here needs to throw a condition that we can easily
-  ## handle and or defer; that's not too hard to do though - convert
-  ## the error into something with a special class, perhaps make it a
-  ## warning in normal R and then register a handler for it in the
-  ## main run.
-  assert_character(files)
-
   p <- get_active_packet()
-  if (is.null(p)) {
-    assert_file_exists(files)
-    files_expanded <- expand_dirs(files, ".")
-  } else {
-    src <- p$orderly2$src
-    assert_file_exists(files, workdir = src)
-    files_expanded <- expand_dirs(files, src)
+  src <- if (is.null(p)) "." else p$orderly2$src
+  assert_file_exists2(files, workdir = src, name = "Resource file",
+                      call = environment())
+  files_expanded <- expand_dirs(files, src)
+  if (!is.null(p)) {
     if (p$orderly2$strict$enabled) {
       copy_files(src, p$path, files_expanded)
     } else {
-      assert_file_exists(files, workdir = p$path)
+      ## Above we're looking in the underlying source directory, here
+      ## we're looking within the running directory; it's not obvious
+      ## when this second case would fail, really.
+      assert_file_exists2(files, workdir = p$path, name = "Resource file",
+                          call = environment())
     }
     outpack_packet_file_mark(p, files_expanded, "immutable")
     p$orderly2$resources <- c(p$orderly2$resources, files_expanded)
diff --git a/tests/testthat/test-run.R b/tests/testthat/test-run.R
index 1fa8a83d..c7949688 100644
--- a/tests/testthat/test-run.R
+++ b/tests/testthat/test-run.R
@@ -1221,3 +1221,15 @@ test_that("can read about dependencies", {
                                          query = "latest",
                                          files = c(input.rds = "data.rds"))))
 })
+
+
+test_that("nice error if resource file not found", {
+  path <- test_prepare_orderly_example("explicit")
+  envir <- new.env()
+  unlink(file.path(path, "src", "explicit", "data.csv"))
+  err <- expect_error(
+    orderly_run_quietly("explicit", root = path, envir = envir),
+    "Resource file does not exist: 'data.csv'")
+  expect_match(err$parent$body[[1]],
+               "Looked within directory '.+/src/explicit'")
+})

From 5e19fe81048882e0e1138da20b7999116fa46048 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 12:05:05 +0100
Subject: [PATCH 4/9] Use new assertion in more places

---
 R/cleanup.R  |  2 +-
 R/config.R   |  4 ++--
 R/metadata.R | 13 ++++++-------
 R/read.R     |  5 +++--
 R/run.R      |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/R/cleanup.R b/R/cleanup.R
index 5750a335..60f2db9e 100644
--- a/R/cleanup.R
+++ b/R/cleanup.R
@@ -116,7 +116,7 @@ orderly_cleanup_status <- function(name = NULL, root = NULL, locate = TRUE) {
     path <- file.path(root_path, "src", name)
   }
 
-  info <- orderly_read(path)
+  info <- orderly_read(path, call = environment())
   files <- withr::with_dir(
     path,
     dir(all.files = TRUE, recursive = TRUE, no.. = TRUE))
diff --git a/R/config.R b/R/config.R
index 1868a036..8801b13f 100644
--- a/R/config.R
+++ b/R/config.R
@@ -1,7 +1,7 @@
 orderly_config_read <- function(path, call = NULL) {
   filename <- file.path(path, "orderly_config.yml")
-  assert_file_exists(basename(filename), workdir = path,
-                     name = "Orderly configuration")
+  assert_file_exists2(basename(filename), workdir = path,
+                      name = "Orderly configuration", call)
   raw <- yaml_read(filename)
 
   if (!is.null(raw)) {
diff --git a/R/metadata.R b/R/metadata.R
index 42d72ff6..94d1d1eb 100644
--- a/R/metadata.R
+++ b/R/metadata.R
@@ -316,7 +316,9 @@ orderly_shared_resource <- function(...) {
   files <- validate_shared_resource(list(...), environment())
   ctx <- orderly_context(rlang::caller_env())
 
-  files <- copy_shared_resource(ctx$root_src, ctx$path, ctx$config, files)
+  files <- copy_shared_resource(ctx$root_src, ctx$path, ctx$config, files,
+                                environment())
+
   if (ctx$is_active) {
     outpack_packet_file_mark(ctx$packet, files$here, "immutable")
     ctx$packet$orderly2$shared_resources <-
@@ -345,9 +347,7 @@ validate_shared_resource <- function(args, call) {
 }
 
 
-copy_shared_resource <- function(path_root, path_dest, config, files) {
-  ## This used to be configurable in orderly1, but almost everyone
-  ## just kept it as 'global'. We might make it configurable later.
+copy_shared_resource <- function(path_root, path_dest, config, files, call) {
   shared_dir <- "shared"
   shared_path <- file.path(path_root, shared_dir)
   if (!is_directory(shared_path)) {
@@ -359,9 +359,8 @@ copy_shared_resource <- function(path_root, path_dest, config, files) {
   here <- names(files)
   there <- unname(files)
 
-  assert_file_exists(
-    there, workdir = shared_path,
-    name = sprintf("Shared resources in '%s'", shared_path))
+  assert_file_exists2(there, workdir = shared_path, name = "Shared resource",
+                      call = call)
   src <- file.path(shared_path, there)
   dst <- file.path(path_dest, here)
 
diff --git a/R/read.R b/R/read.R
index cc55e69f..c1d1a177 100644
--- a/R/read.R
+++ b/R/read.R
@@ -1,5 +1,6 @@
-orderly_read <- function(path) {
-  assert_file_exists("orderly.R", workdir = path)
+orderly_read <- function(path, call = NULL) {
+  assert_file_exists2("orderly.R", name = "Orderly file", workdir = path,
+                      call = call)
   orderly_read_r(file.path(path, "orderly.R"))
 }
 
diff --git a/R/run.R b/R/run.R
index 2e30d07a..e2590ec3 100644
--- a/R/run.R
+++ b/R/run.R
@@ -176,7 +176,7 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,
   assert_is(envir, "environment")
 
   src <- file.path(root_src, "src", name)
-  dat <- orderly_read(src)
+  dat <- orderly_read(src, environment())
   parameters <- check_parameters(parameters, dat$parameters, environment())
   orderly_validate(dat, src)
 

From 192bd1ac760cfb58a0189fc35839ecf09dfe3f33 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 12:54:14 +0100
Subject: [PATCH 5/9] Expand to include relative check

---
 R/outpack_config.R                |  4 ++--
 R/outpack_helpers.R               |  2 +-
 R/outpack_metadata.R              |  3 +--
 R/outpack_packet.R                |  4 ++--
 R/util_assert.R                   | 35 ++++++++++++++++++++++---------
 tests/testthat/helper-outpack.R   |  1 -
 tests/testthat/test-util-assert.R | 20 ++++++++++--------
 7 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/R/outpack_config.R b/R/outpack_config.R
index f03825c4..01da4f76 100644
--- a/R/outpack_config.R
+++ b/R/outpack_config.R
@@ -187,7 +187,7 @@ config_set_path_archive <- function(value, root) {
     path_archive_old <- file.path(root$path, config$core$path_archive)
     if (fs::dir_exists(path_archive_old)) {
       path_archive_new <- file.path(root$path, value)
-      assert_relative_path(value, name = "path_archive")
+      assert_relative_path(value, name = "path_archive", workdir = root$path)
       assert_directory_does_not_exist(path_archive_new)
       fs::dir_copy(path_archive_old, path_archive_new)
       fs::dir_delete(path_archive_old)
@@ -195,7 +195,7 @@ config_set_path_archive <- function(value, root) {
     config$core$path_archive <- value
   } else {
     path_archive <- file.path(root$path, value)
-    assert_relative_path(value, name = "path_archive")
+    assert_relative_path(value, name = "path_archive", workdir = root$path)
     assert_directory_does_not_exist(path_archive)
     tryCatch({
       fs::dir_create(path_archive)
diff --git a/R/outpack_helpers.R b/R/outpack_helpers.R
index 603117f5..d5a9db7a 100644
--- a/R/outpack_helpers.R
+++ b/R/outpack_helpers.R
@@ -150,7 +150,7 @@ orderly_copy_files <- function(..., files, dest, overwrite = TRUE,
 
 
 plan_copy_files <- function(root, id, there, here, call = NULL) {
-  assert_relative_path(there, no_dots = TRUE)
+  assert_relative_path(there, name = "File", workdir = id, call = NULL)
   validate_packet_has_file(root, id, there, call)
   is_dir <- grepl("/$", there)
   if (any(is_dir)) {
diff --git a/R/outpack_metadata.R b/R/outpack_metadata.R
index 2ad6210d..349d77a5 100644
--- a/R/outpack_metadata.R
+++ b/R/outpack_metadata.R
@@ -76,8 +76,7 @@ outpack_metadata_create <- function(path, name, id, time, files,
   if (is.null(files)) {
     files <- dir(path, recursive = TRUE, all.files = TRUE, no.. = TRUE)
   } else {
-    assert_relative_path(files, no_dots = TRUE)
-    assert_file_exists(files, workdir = path)
+    assert_file_exists2(files, name = "File", workdir = path)
   }
 
   if (length(file_ignore) > 0) {
diff --git a/R/outpack_packet.R b/R/outpack_packet.R
index 86ceeaf7..f42b5c55 100644
--- a/R/outpack_packet.R
+++ b/R/outpack_packet.R
@@ -325,8 +325,8 @@ outpack_packet_file_mark <- function(packet, files, status) {
   status <- match_value(status, c("immutable", "ignored"))
   packet <- check_current_packet(packet)
 
-  assert_relative_path(files, no_dots = TRUE)
-  assert_file_exists(files, workdir = packet$path)
+  assert_file_exists2(files, workdir = packet$path, name = "File",
+                      call = environment())
 
   ## TODO: these are exclusive categories because we later return a
   ## 1:1 mapping of file to status
diff --git a/R/util_assert.R b/R/util_assert.R
index 301fe3cb..acb0e4e3 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -62,6 +62,8 @@ assert_file_exists <- function(x, workdir = NULL, name = "File") {
 
 
 assert_file_exists2 <- function(files, workdir, name, call = NULL) {
+  assert_relative_path(files, name, workdir, call)
+
   assert_character(files, call = call)
   err <- !file_exists(files, workdir = workdir)
   if (any(err)) {
@@ -73,9 +75,12 @@ assert_file_exists2 <- function(files, workdir, name, call = NULL) {
   }
 
   files_canonical <- file_canonical_case(files, workdir)
-  err <- fs::path(files) != files_canonical
+  err <- is.na(files_canonical) | fs::path(files) != files_canonical
   if (any(err)) {
     n <- cli::qty(sum(err))
+    if (any(is.na(files_canonical))) {
+      browser()
+    }
     hint_case <- sprintf("For '%s', did you mean '%s'?",
                          files[err], files_canonical[err])
     cli::cli_abort(
@@ -98,17 +103,27 @@ assert_is_directory <- function(x, workdir = NULL, name = "Directory") {
   }
 }
 
-assert_relative_path <- function(x, no_dots = FALSE,
-                                 name = deparse(substitute(x))) {
-  err <- fs::is_absolute_path(x)
+assert_relative_path <- function(files, name, workdir, call = NULL) {
+  err <- fs::is_absolute_path(files)
   if (any(err)) {
-    stop(sprintf("'%s' must be relative %s",
-                 name, ngettext(length(x), "path", "paths")),
-         call. = FALSE)
+    n <- cli::qty(sum(err))
+    ## TODO: try and relativise - use path_has_parent and path_rel
+    cli::cli_abort(
+      c("{name}{n}{?s} must be relative path{?s}",
+        set_names(files[err], "x"),
+        i = "Path was relative to directory '{workdir}'"),
+      call = call)
   }
-  if (no_dots && any(grepl("..", x, fixed = TRUE))) {
-    stop(sprintf("'%s' must not contain '..' path components", name),
-         call. = FALSE)
+
+  err <- vlapply(fs::path_split(files), function(x) any(x == ".."))
+  if (any(err)) {
+    n <- cli::qty(sum(err))
+    ## TODO: try and elide these, where possible
+    cli::cli_abort(
+      c("{name}{n}{?s} must not contain '..' (parent directory) components",
+        set_names(files[err], "x"),
+        i = "Path was relative to directory '{workdir}'"),
+      call = call)
   }
 }
 
diff --git a/tests/testthat/helper-outpack.R b/tests/testthat/helper-outpack.R
index 152d9236..ff2dbe2f 100644
--- a/tests/testthat/helper-outpack.R
+++ b/tests/testthat/helper-outpack.R
@@ -161,7 +161,6 @@ outpack_packet_run <- function(packet, script, envir = NULL) {
     envir <- new.env(parent = .GlobalEnv)
   }
   packet <- check_current_packet(packet)
-  assert_relative_path(script, no_dots = TRUE)
   assert_file_exists(script, workdir = packet$path, name = "Script")
   withr::with_dir(packet$path,
                   source_echo(script, envir = envir, echo = FALSE))
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index e704b40f..28faabb0 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -106,19 +106,21 @@ test_that("assert_is_directory", {
 
 
 test_that("assert_relative_path", {
-  expect_error(assert_relative_path(getwd()),
-               "'getwd()' must be relative path",
+  workdir <- getwd()
+  expect_error(assert_relative_path(getwd(), "File", workdir),
+               "File must be relative path",
                fixed = TRUE)
-  expect_silent(assert_relative_path("relpath"))
+  expect_silent(assert_relative_path("relpath", "File", workdir))
+  expect_silent(assert_relative_path("a/b/c", "File", workdir))
 
-  expect_silent(
-    assert_relative_path("../my/path"))
   expect_error(
-    assert_relative_path("../my/path", TRUE),
-    "must not contain '..' path components")
+    assert_relative_path("../my/path", "File", workdir),
+    "must not contain '..' (parent directory) components",
+    fixed = TRUE)
   expect_error(
-    assert_relative_path("my/../../path", TRUE),
-    "must not contain '..' path components")
+    assert_relative_path("my/../../path", "File", workdir),
+    "must not contain '..' (parent directory) components",
+    fixed = TRUE)
 })
 
 

From bdb1889a59dbf6bf1fd5a2ce9b67898ecea185bb Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 15:00:05 +0100
Subject: [PATCH 6/9] Update tests

---
 R/outpack_config.R                   | 4 ++--
 R/util_assert.R                      | 2 +-
 tests/testthat/test-outpack-config.R | 4 ++--
 tests/testthat/test-util-assert.R    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/R/outpack_config.R b/R/outpack_config.R
index 01da4f76..e15115b3 100644
--- a/R/outpack_config.R
+++ b/R/outpack_config.R
@@ -187,7 +187,7 @@ config_set_path_archive <- function(value, root) {
     path_archive_old <- file.path(root$path, config$core$path_archive)
     if (fs::dir_exists(path_archive_old)) {
       path_archive_new <- file.path(root$path, value)
-      assert_relative_path(value, name = "path_archive", workdir = root$path)
+      assert_relative_path(value, name = "'path_archive'", workdir = root$path)
       assert_directory_does_not_exist(path_archive_new)
       fs::dir_copy(path_archive_old, path_archive_new)
       fs::dir_delete(path_archive_old)
@@ -195,7 +195,7 @@ config_set_path_archive <- function(value, root) {
     config$core$path_archive <- value
   } else {
     path_archive <- file.path(root$path, value)
-    assert_relative_path(value, name = "path_archive", workdir = root$path)
+    assert_relative_path(value, name = "'path_archive'", workdir = root$path)
     assert_directory_does_not_exist(path_archive)
     tryCatch({
       fs::dir_create(path_archive)
diff --git a/R/util_assert.R b/R/util_assert.R
index acb0e4e3..28372ea2 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -109,7 +109,7 @@ assert_relative_path <- function(files, name, workdir, call = NULL) {
     n <- cli::qty(sum(err))
     ## TODO: try and relativise - use path_has_parent and path_rel
     cli::cli_abort(
-      c("{name}{n}{?s} must be relative path{?s}",
+      c("{name}{n}{?s} must be {?a/} relative path{?s}",
         set_names(files[err], "x"),
         i = "Path was relative to directory '{workdir}'"),
       call = call)
diff --git a/tests/testthat/test-outpack-config.R b/tests/testthat/test-outpack-config.R
index 68a0fa5a..e94524f5 100644
--- a/tests/testthat/test-outpack-config.R
+++ b/tests/testthat/test-outpack-config.R
@@ -271,7 +271,7 @@ test_that("Archive is not added if file store is corrupt", {
 test_that("Validates path_archive", {
   root <- create_temporary_root(path_archive = NULL, use_file_store = TRUE)
   expect_error(orderly_config_set(core.path_archive = "/archive", root = root),
-               "'path_archive' must be relative path")
+               "'path_archive' must be a relative path")
   expect_null(root$config$core$path_archive)
 
   dir.create(file.path(root$path, "archive"))
@@ -281,7 +281,7 @@ test_that("Validates path_archive", {
 
   orderly_config_set(core.path_archive = "new-archive", root = root)
   expect_error(orderly_config_set(core.path_archive = "/archive", root = root),
-               "'path_archive' must be relative path")
+               "'path_archive' must be a relative path")
   expect_error(orderly_config_set(core.path_archive = "archive", root = root),
                "Directory already exists")
   expect_equal(root$config$core$path_archive, "new-archive")
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index 28faabb0..317905c2 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -108,7 +108,7 @@ test_that("assert_is_directory", {
 test_that("assert_relative_path", {
   workdir <- getwd()
   expect_error(assert_relative_path(getwd(), "File", workdir),
-               "File must be relative path",
+               "File must be a relative path",
                fixed = TRUE)
   expect_silent(assert_relative_path("relpath", "File", workdir))
   expect_silent(assert_relative_path("a/b/c", "File", workdir))

From 6d522a8ef1b4d92a6453a333afe60149a605d2e7 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 15:48:42 +0100
Subject: [PATCH 7/9] Rename and reorganise

---
 R/config.R                        |  4 ++--
 R/metadata.R                      | 12 ++++++------
 R/outpack_metadata.R              |  2 +-
 R/outpack_packet.R                |  4 ++--
 R/read.R                          |  4 ++--
 R/util_assert.R                   | 24 ++++++++++++------------
 tests/testthat/helper-outpack.R   |  1 -
 tests/testthat/test-util-assert.R | 23 +++++++++++------------
 8 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/R/config.R b/R/config.R
index 8801b13f..f8c2f7a5 100644
--- a/R/config.R
+++ b/R/config.R
@@ -1,7 +1,7 @@
 orderly_config_read <- function(path, call = NULL) {
   filename <- file.path(path, "orderly_config.yml")
-  assert_file_exists2(basename(filename), workdir = path,
-                      name = "Orderly configuration", call)
+  assert_file_exists_relative(basename(filename), workdir = path,
+                              name = "Orderly configuration", call = call)
   raw <- yaml_read(filename)
 
   if (!is.null(raw)) {
diff --git a/R/metadata.R b/R/metadata.R
index 94d1d1eb..7a1eb5b6 100644
--- a/R/metadata.R
+++ b/R/metadata.R
@@ -151,8 +151,8 @@ static_orderly_description <- function(args) {
 orderly_resource <- function(files) {
   p <- get_active_packet()
   src <- if (is.null(p)) "." else p$orderly2$src
-  assert_file_exists2(files, workdir = src, name = "Resource file",
-                      call = environment())
+  assert_file_exists_relative(files, workdir = src, name = "Resource file",
+                              call = environment())
   files_expanded <- expand_dirs(files, src)
   if (!is.null(p)) {
     if (p$orderly2$strict$enabled) {
@@ -161,8 +161,8 @@ orderly_resource <- function(files) {
       ## Above we're looking in the underlying source directory, here
       ## we're looking within the running directory; it's not obvious
       ## when this second case would fail, really.
-      assert_file_exists2(files, workdir = p$path, name = "Resource file",
-                          call = environment())
+      assert_file_exists_relative(files, workdir = p$path,
+                                  name = "Resource file", call = environment())
     }
     outpack_packet_file_mark(p, files_expanded, "immutable")
     p$orderly2$resources <- c(p$orderly2$resources, files_expanded)
@@ -359,8 +359,8 @@ copy_shared_resource <- function(path_root, path_dest, config, files, call) {
   here <- names(files)
   there <- unname(files)
 
-  assert_file_exists2(there, workdir = shared_path, name = "Shared resource",
-                      call = call)
+  assert_file_exists_relative(there, workdir = shared_path,
+                              name = "Shared resource file", call = call)
   src <- file.path(shared_path, there)
   dst <- file.path(path_dest, here)
 
diff --git a/R/outpack_metadata.R b/R/outpack_metadata.R
index 349d77a5..3691735d 100644
--- a/R/outpack_metadata.R
+++ b/R/outpack_metadata.R
@@ -76,7 +76,7 @@ outpack_metadata_create <- function(path, name, id, time, files,
   if (is.null(files)) {
     files <- dir(path, recursive = TRUE, all.files = TRUE, no.. = TRUE)
   } else {
-    assert_file_exists2(files, name = "File", workdir = path)
+    assert_file_exists_relative(files, name = "File", workdir = path)
   }
 
   if (length(file_ignore) > 0) {
diff --git a/R/outpack_packet.R b/R/outpack_packet.R
index f42b5c55..ff422b18 100644
--- a/R/outpack_packet.R
+++ b/R/outpack_packet.R
@@ -325,8 +325,8 @@ outpack_packet_file_mark <- function(packet, files, status) {
   status <- match_value(status, c("immutable", "ignored"))
   packet <- check_current_packet(packet)
 
-  assert_file_exists2(files, workdir = packet$path, name = "File",
-                      call = environment())
+  assert_file_exists_relative(files, workdir = packet$path, name = "File",
+                              call = environment())
 
   ## TODO: these are exclusive categories because we later return a
   ## 1:1 mapping of file to status
diff --git a/R/read.R b/R/read.R
index c1d1a177..99fe674d 100644
--- a/R/read.R
+++ b/R/read.R
@@ -1,6 +1,6 @@
 orderly_read <- function(path, call = NULL) {
-  assert_file_exists2("orderly.R", name = "Orderly file", workdir = path,
-                      call = call)
+  assert_file_exists_relative("orderly.R", name = "Orderly file",
+                              workdir = path, call = call)
   orderly_read_r(file.path(path, "orderly.R"))
 }
 
diff --git a/R/util_assert.R b/R/util_assert.R
index 28372ea2..d67c3171 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -51,17 +51,18 @@ assert_is <- function(x, what, name = deparse(substitute(x))) {
   }
 }
 
-assert_file_exists <- function(x, workdir = NULL, name = "File") {
-  err <- !file_exists(x, workdir = workdir)
+assert_file_exists <- function(files, name = "File", call = NULL) {
+  err <- !file.exists(files)
   if (any(err)) {
-    msg <- squote(x[err])
-    stop(sprintf("%s does not exist: %s", name, paste(msg, collapse = ", ")),
-         call. = FALSE)
+    n <- cli::qty(sum(err))
+    cli::cli_abort(
+      "{name}{n}{?s} {?does/do} not exist: {collapseq(files[err])}",
+      call = call)
   }
 }
 
 
-assert_file_exists2 <- function(files, workdir, name, call = NULL) {
+assert_file_exists_relative <- function(files, workdir, name, call = NULL) {
   assert_relative_path(files, name, workdir, call)
 
   assert_character(files, call = call)
@@ -93,13 +94,12 @@ assert_file_exists2 <- function(files, workdir, name, call = NULL) {
   }
 }
 
-assert_is_directory <- function(x, workdir = NULL, name = "Directory") {
-  assert_file_exists(x, workdir, name)
-  path <- if (is.null(workdir)) x else file.path(workdir, x)
+assert_is_directory <- function(path, name = "Directory", call = NULL) {
+  assert_scalar_character(path)
+  assert_file_exists(path, name = name, call = call)
   if (!is_directory(path)) {
-    stop(sprintf("Path exists but is not a directory: %s",
-                 paste(x, collapse = ", ")),
-         call. = FALSE)
+    cli::cli_abort("Path exists but is not a directory: {path}",
+                   call = call)
   }
 }
 
diff --git a/tests/testthat/helper-outpack.R b/tests/testthat/helper-outpack.R
index ff2dbe2f..12eff2a5 100644
--- a/tests/testthat/helper-outpack.R
+++ b/tests/testthat/helper-outpack.R
@@ -161,7 +161,6 @@ outpack_packet_run <- function(packet, script, envir = NULL) {
     envir <- new.env(parent = .GlobalEnv)
   }
   packet <- check_current_packet(packet)
-  assert_file_exists(script, workdir = packet$path, name = "Script")
   withr::with_dir(packet$path,
                   source_echo(script, envir = envir, echo = FALSE))
 }
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index 317905c2..98dfd017 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -44,30 +44,29 @@ test_that("assert_is", {
 test_that("assert_file_exists", {
   tmp <- normalise_path(tempdir())
   path <- tempfile(tmpdir = tmp)
-  expect_error(assert_file_exists(path), "File does not exist")
+  expect_error(assert_file_exists(path, "File"), "File does not exist")
   file.create(path)
-  expect_silent(assert_file_exists(path))
-  expect_silent(assert_file_exists(basename(path), workdir = tmp))
+  expect_silent(assert_file_exists(path, "File"))
 })
 
 
-test_that("assert_file_exists2 works checks if files exist", {
+test_that("assert_file_exists_relative works checks if files exist", {
   tmp <- withr::local_tempdir()
   file.create(file.path(tmp, "c"))
-  expect_error(assert_file_exists2("a", tmp, "File"),
+  expect_error(assert_file_exists_relative("a", tmp, "File"),
                "File does not exist: 'a'")
-  expect_error(assert_file_exists2(c("a", "b"), tmp, "File"),
+  expect_error(assert_file_exists_relative(c("a", "b"), tmp, "File"),
                "Files do not exist: 'a', 'b'")
-  expect_error(assert_file_exists2(c("a", "b", "c", "d"), tmp, "File"),
+  expect_error(assert_file_exists_relative(c("a", "b", "c", "d"), tmp, "File"),
                "Files do not exist: 'a', 'b', 'd'")
-  expect_silent(assert_file_exists2("c", tmp, "File"))
+  expect_silent(assert_file_exists_relative("c", tmp, "File"))
 })
 
 
-test_that("assert_file_exists2 informs about case mismatch", {
+test_that("assert_file_exists_relative informs about case mismatch", {
   testthat::skip_if_not_installed("mockery")
   mock_file_exists <- mockery::mock(TRUE, cycle = TRUE)
-  mockery::stub(assert_file_exists2, "file_exists", mock_file_exists)
+  mockery::stub(assert_file_exists_relative, "file_exists", mock_file_exists)
 
   tmp <- withr::local_tempdir()
   file.create(file.path(tmp, "a"))
@@ -75,7 +74,7 @@ test_that("assert_file_exists2 informs about case mismatch", {
   file.create(file.path(tmp, "b/c/d"))
 
   err <- expect_error(
-    assert_file_exists2("A", tmp, "File"),
+    assert_file_exists_relative("A", tmp, "File"),
     "File does not exist: 'A'")
   expect_length(err$body, 3)
   expect_equal(names(err$body), c("i", "i", "i"))
@@ -84,7 +83,7 @@ test_that("assert_file_exists2 informs about case mismatch", {
   expect_match(err$body[[3]], "Looked within directory '.+'")
 
   err <- expect_error(
-    assert_file_exists2(c("A", "b/C/d"), tmp, "File"),
+    assert_file_exists_relative(c("A", "b/C/d"), tmp, "File"),
     "Files do not exist: 'A', 'b/C/d'")
   expect_length(err$body, 4)
   expect_equal(names(err$body), c("i", "i", "i", "i"))

From 210039ee1181834a94a736a6f6ddaa801453d1ce Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 16:08:11 +0100
Subject: [PATCH 8/9] Fix coverage hole

---
 R/util_assert.R                   | 8 +++-----
 tests/testthat/test-util-assert.R | 9 +++++++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/R/util_assert.R b/R/util_assert.R
index d67c3171..f8091943 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -78,12 +78,10 @@ assert_file_exists_relative <- function(files, workdir, name, call = NULL) {
   files_canonical <- file_canonical_case(files, workdir)
   err <- is.na(files_canonical) | fs::path(files) != files_canonical
   if (any(err)) {
-    n <- cli::qty(sum(err))
-    if (any(is.na(files_canonical))) {
-      browser()
-    }
+    i <- err & !is.na(files_canonical)
     hint_case <- sprintf("For '%s', did you mean '%s'?",
-                         files[err], files_canonical[err])
+                         files[i], files_canonical[i])
+    n <- cli::qty(sum(err))
     cli::cli_abort(
       c("{name}{n}{?s} {?does/do} not exist: {collapseq(files[err])}",
         set_names(hint_case, "i"),
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index 98dfd017..12a3c9a7 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -91,6 +91,15 @@ test_that("assert_file_exists_relative informs about case mismatch", {
   expect_equal(err$body[[2]], "For 'b/C/d', did you mean 'b/c/d'?")
   expect_match(err$body[[3]], "If you don't use the canonical case for a file")
   expect_match(err$body[[4]], "Looked within directory '.+'")
+
+  err <- expect_error(
+    assert_file_exists_relative(c("A", "b/X/d"), tmp, "File"),
+    "Files do not exist: 'A', 'b/X/d'")
+  expect_length(err$body, 3)
+  expect_equal(names(err$body), c("i", "i", "i"))
+  expect_equal(err$body[[1]], "For 'A', did you mean 'a'?")
+  expect_match(err$body[[3]], "If you don't use the canonical case for a file")
+  expect_match(err$body[[4]], "Looked within directory '.+'")
 })
 
 

From 087c84b89d9eff8eb46907b3cbf49419a862f9df Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Fri, 29 Sep 2023 16:12:41 +0100
Subject: [PATCH 9/9] Tidy up old comments

---
 R/util_assert.R                   | 2 --
 tests/testthat/test-util-assert.R | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/R/util_assert.R b/R/util_assert.R
index f8091943..8c310eb8 100644
--- a/R/util_assert.R
+++ b/R/util_assert.R
@@ -105,7 +105,6 @@ assert_relative_path <- function(files, name, workdir, call = NULL) {
   err <- fs::is_absolute_path(files)
   if (any(err)) {
     n <- cli::qty(sum(err))
-    ## TODO: try and relativise - use path_has_parent and path_rel
     cli::cli_abort(
       c("{name}{n}{?s} must be {?a/} relative path{?s}",
         set_names(files[err], "x"),
@@ -116,7 +115,6 @@ assert_relative_path <- function(files, name, workdir, call = NULL) {
   err <- vlapply(fs::path_split(files), function(x) any(x == ".."))
   if (any(err)) {
     n <- cli::qty(sum(err))
-    ## TODO: try and elide these, where possible
     cli::cli_abort(
       c("{name}{n}{?s} must not contain '..' (parent directory) components",
         set_names(files[err], "x"),
diff --git a/tests/testthat/test-util-assert.R b/tests/testthat/test-util-assert.R
index 12a3c9a7..925b5573 100644
--- a/tests/testthat/test-util-assert.R
+++ b/tests/testthat/test-util-assert.R
@@ -98,8 +98,8 @@ test_that("assert_file_exists_relative informs about case mismatch", {
   expect_length(err$body, 3)
   expect_equal(names(err$body), c("i", "i", "i"))
   expect_equal(err$body[[1]], "For 'A', did you mean 'a'?")
-  expect_match(err$body[[3]], "If you don't use the canonical case for a file")
-  expect_match(err$body[[4]], "Looked within directory '.+'")
+  expect_match(err$body[[2]], "If you don't use the canonical case for a file")
+  expect_match(err$body[[3]], "Looked within directory '.+'")
 })