From add8d2643cbd5a53f2bddbf5a783da50b826ccbe Mon Sep 17 00:00:00 2001
From: Paul Lietar <pl2113@ic.ac.uk>
Date: Tue, 2 Jul 2024 17:44:35 +0100
Subject: [PATCH] Make files writable when they are exported.

Since [orderly2#141], orderly marks all files in the archive as
read-only. Similarly, files in the file store have always been
read-only. This helps prevent accidental corruption by users.

However this has had the undesirable side-effect of copying the files
with the read-only bit set, even when exporting for interactive use by
the user.

One scenario where this has come up is a user running
`orderly_dependency` interactively twice. They would expect the second
call to succeed and overwrite the existing files. However this was not
the case, and instead the second call fails as the read-only files
cannot be overwritten. The user ends up having to manually delete these
files (or mark them writable) everytime they want to update their copy.

This change marks all files that are exported as writable. This does not
apply to calls to `orderly_dependency` from a job context, in which case
the files are still read-only.

[orderly2#141]: https://github.com/mrc-ide/orderly2/pull/141
---
 R/outpack_root.R                      |  5 +++++
 tests/testthat/test-outpack-helpers.R | 30 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/R/outpack_root.R b/R/outpack_root.R
index f075aec7..973664b0 100644
--- a/R/outpack_root.R
+++ b/R/outpack_root.R
@@ -72,6 +72,11 @@ file_export <- function(root, id, there, here, dest, overwrite, call = NULL) {
     }
     fs::file_copy(there_full, here_full, overwrite)
   }
+
+  # Files in the archive and file store are (generally) read-only.
+  # When exporting for interactive use, it's easier on the user if we make them
+  # writable again.
+  fs::file_chmod(here_full, "u+w")
 }
 
 
diff --git a/tests/testthat/test-outpack-helpers.R b/tests/testthat/test-outpack-helpers.R
index 191cd13b..634f76fb 100644
--- a/tests/testthat/test-outpack-helpers.R
+++ b/tests/testthat/test-outpack-helpers.R
@@ -157,3 +157,33 @@ test_that("good error message if file not found in packet", {
       i = "For 'd.txt' did you mean 'a.txt', 'b.txt' or 'c.txt'",
       i = "Remember that all orderly paths are case sensitive"))
 })
+
+
+test_that("Can overwrite when copying files from packet", {
+  root <- create_temporary_root(use_file_store = TRUE)
+
+  id1 <- create_random_packet(root)
+  id2 <- create_random_packet(root)
+
+  # Just a bit of a sanity check. The two packets are random, so we'd expect
+  # them to have different contents.
+  expect_false(identical(
+    readRDS(file.path(root$path, "archive", "data", id1, "data.rds")),
+    readRDS(file.path(root$path, "archive", "data", id2, "data.rds"))))
+
+  dst <- temp_file()
+
+  suppressMessages(
+    orderly_copy_files(id1, files = "data.rds", dest = dst, root = root))
+
+  expect_identical(
+    readRDS(file.path(dst, "data.rds")),
+    readRDS(file.path(root$path, "archive", "data", id1, "data.rds")))
+
+  suppressMessages(
+    orderly_copy_files(id2, files = "data.rds", dest = dst, root = root))
+
+  expect_identical(
+    readRDS(file.path(dst, "data.rds")),
+    readRDS(file.path(root$path, "archive", "data", id2, "data.rds")))
+})