Fix Path field being ignored when File field is also present

packages_make_target() built the download/cache target from the
File field first, and only fell back to Path when File was NA. This
meant that PACKAGES entries with both fields set had Path silently
dropped instead of combined, producing repodir/file instead of the
correct repodir/path/file.

Handle all four combinations explicitly: both present, only File,
only Path, and neither.

See r-universe-org/help#715

Author: Jeroen Ooms

NEWS.md

@@ -1,5 +1,11 @@
 # pkgcache (development version)
 
+* Fixed a bug where the `Path` field of a `PACKAGES` entry was silently
+  ignored when the entry also had a `File` field. The target/download
+  URL for such entries is now correctly derived from both fields,
+  as `<repodir>/<path>/<file>`. See
+  https://github.com/r-universe-org/help/issues/715.
+
 * New `PKG_USE_BIOCONDUCTOR` environment variable and new
   `pkg.use_bioconductor` option to opt out from automatic Bioconductor
   support.

R/packages-gz.R

@@ -359,34 +359,41 @@ packages_make_target <- function(
   res <- rep(NA_character_, length(package))
   ext <- get_cran_extension(platform)
 
-  ## 'File' field, if present
-  if (!is.null(file)) {
-    wh <- !is.na(file)
-    if (any(wh)) {
-      res[wh] <- paste0(repodir, "/", file[wh])
-    }
+  have_file <- if (is.null(file)) rep(FALSE, length(package)) else !is.na(file)
+  have_path <- if (is.null(path)) rep(FALSE, length(package)) else !is.na(path)
+
+  ## Both 'Path' and 'File' fields are present: the file named in 'File'
+  ## lives in the subdirectory named in 'Path'.
+  wh <- have_path & have_file
+  if (any(wh)) {
+    res[wh] <- paste0(repodir, "/", path[wh], "/", file[wh])
   }
 
-  ## 'Path' field, if present
-  if (!is.null(path)) {
-    wh <- is.na(res) & !is.na(path)
-    if (any(wh)) {
-      res[wh] <- paste0(
-        repodir,
-        "/",
-        path[wh],
-        "/",
-        package[wh],
-        "_",
-        version[wh],
-        ext[wh]
-      )
-    }
+  ## Only 'File' field is present, use it as is, relative to 'repodir'
+  wh <- have_file & !have_path
+  if (any(wh)) {
+    res[wh] <- paste0(repodir, "/", file[wh])
+  }
+
+  ## Only 'Path' field is present, it is a subdirectory of 'repodir', and
+  ## the file name is the default one, from the package name and version
+  wh <- have_path & !have_file
+  if (any(wh)) {
+    res[wh] <- paste0(
+      repodir,
+      "/",
+      path[wh],
+      "/",
+      package[wh],
+      "_",
+      version[wh],
+      ext[wh]
+    )
   }
 
-  ## Otherwise default
-  if (anyNA(res)) {
-    wh <- is.na(res)
+  ## Neither is present, use the default target path
+  wh <- !have_file & !have_path
+  if (any(wh)) {
     res[wh] <- paste0(repodir, "/", package[wh], "_", version[wh], ext[wh])
   }
 

tests/testthat/test-packages-gz.R

@@ -23,6 +23,9 @@ test_that("packages_make_target", {
     c("src/contrib/foo", "src/contrib/bar")
   )
 
+  # When both 'File' and 'Path' are given, they must be combined into
+  # repodir/path/file, instead of dropping 'Path'. See
+  # https://github.com/r-universe-org/help/issues/715
   expect_equal(
     packages_make_target(
       "source",
@@ -32,7 +35,7 @@ test_that("packages_make_target", {
       c("foo", "bar"),
       c("1", "2")
     ),
-    c("src/contrib/foo", "src/contrib/bar")
+    c("src/contrib/1/foo", "src/contrib/2/bar")
   )
 
   expect_equal(
@@ -47,6 +50,8 @@ test_that("packages_make_target", {
     c("src/contrib/foo/p1_1.0.tar.gz", "src/contrib/bar/p2_2.0.tar.gz")
   )
 
+  # Mix of: both 'File' and 'Path' present, only 'Path' present, and
+  # neither present.
   expect_equal(
     packages_make_target(
       "source",
@@ -57,7 +62,7 @@ test_that("packages_make_target", {
       c("foox", "bar", NA)
     ),
     c(
-      "src/contrib/foo",
+      "src/contrib/foox/foo",
       "src/contrib/bar/p2_2.0.tar.gz",
       "src/contrib/p3_3.0.tar.gz"
     )
@@ -282,6 +287,37 @@ test_that("rbind_expand", {
   expect_identical(m$d, c(NA, NA, 1L, 2L))
 })
 
+test_that("read_packages_file combines Path and File fields", {
+  # Regression test for https://github.com/r-universe-org/help/issues/715
+  # When a PACKAGES entry has both a 'Path' and a 'File' field, the
+  # resulting target/download URL must combine them as repodir/path/file,
+  # instead of silently dropping 'Path'.
+  pkgs_file <- test_temp_file()
+  cat(
+    "Package: foo\n",
+    "Version: 1.0.0\n",
+    "Path: Archive/foo\n",
+    "File: foo_1.0.0.tar.gz\n",
+    "\n",
+    file = pkgs_file,
+    sep = ""
+  )
+
+  data <- read_packages_file(
+    pkgs_file,
+    mirror = "https://example.com",
+    repodir = "src/contrib",
+    platform = "source",
+    rversion = "*"
+  )
+
+  expect_equal(data$pkgs$target, "src/contrib/Archive/foo/foo_1.0.0.tar.gz")
+  expect_equal(
+    data$pkgs$sources[[1]],
+    "https://example.com/src/contrib/Archive/foo/foo_1.0.0.tar.gz"
+  )
+})
+
 test_that("empty PACKAGES file", {
   pkgs <- test_temp_file()
   data <- read_packages_file(