From 65049f3269e0c4f7704114220e54f34629349acc Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 27 Jun 2024 17:22:18 +0100 Subject: [PATCH 1/4] Improve handling of empty or detached Git repositories. When orderly runs a report from a Git repository, it records information about the current state of the repository in the packet metadata. This includes the current commit hash and the name of the current branch. It is possible for either of these to not exist, in which case we need to make sure orderly behaves gracefully. The commit hash will be missing on a brand new repository which hasn't had any commits yet (or equivalently, an existing repository where `git checkout --orphan` was used. The branch name would be missing if the repository is in a "detached HEAD" state, in other words the user checked out a specific commit hash instead of a named branch. The first situation, of an empty repository, was not supported by orderly at all. It would throw an error and fail to run the report entirely. In the second case, when on a detached HEAD, the Git library we use reports the branch name as "HEAD", and that is what we were recording in the metadata. This is a bit misleading since it is not an actual branch name. Both cases have been modified to work without errors and to omit the missing values from the metadata. --- R/outpack_misc.R | 12 +++++++-- tests/testthat/test-outpack-git.R | 42 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/R/outpack_misc.R b/R/outpack_misc.R index 58871c2b..3c3be020 100644 --- a/R/outpack_misc.R +++ b/R/outpack_misc.R @@ -44,8 +44,16 @@ git_info <- function(path) { if (is.null(repo)) { return(NULL) } - list(sha = gert::git_commit_id(repo = repo), - branch = gert::git_branch(repo = repo), + + branch <- gert::git_branch(repo = repo) + if (identical(branch, "HEAD")) { + # HEAD isn't a valid branch name, and instead is what gets returned when a + # detached head was checked out. + branch <- NULL + } + + list(sha = ignore_errors(gert::git_commit_id(repo = repo)), + branch = branch, url = gert::git_remote_list(repo = repo)$url) } diff --git a/tests/testthat/test-outpack-git.R b/tests/testthat/test-outpack-git.R index 97cc3860..bdc3ae9c 100644 --- a/tests/testthat/test-outpack-git.R +++ b/tests/testthat/test-outpack-git.R @@ -68,3 +68,45 @@ test_that("store no information into packet, if no git found", { expect_true("git" %in% names(meta)) expect_null(meta$git) }) + + +test_that("store git information when on a detached HEAD", { + root <- create_temporary_root() + path_src <- create_temporary_simple_src() + git_info <- helper_add_git(path_src) + + ## gert has no API to checkout a particular commit, only named branches + ## https://github.com/r-lib/gert/issues/147 + system2("git", c("-C", path_src, "checkout", "-q", git_info$sha)) + + suppressMessages({ + p <- outpack_packet_start(path_src, "example", root = root) + id <- p$id + outpack_packet_run(p, "script.R") + outpack_packet_end(p) + }) + + meta <- orderly_metadata(id, root = root$path) + expect_mapequal(meta$git, + list(sha = git_info$sha, + url = git_info$url)) +}) + + +test_that("handle empty git repository correctly", { + root <- create_temporary_root() + path_src <- create_temporary_simple_src() + + gert::git_init(path_src) + gert::git_remote_add("https://example.com/git", repo = path_src) + + suppressMessages({ + p <- outpack_packet_start(path_src, "example", root = root) + id <- p$id + outpack_packet_run(p, "script.R") + outpack_packet_end(p) + }) + + meta <- orderly_metadata(id, root = root$path) + expect_mapequal(meta$git, list(url = "https://example.com/git")) +}) From 64b7b2acc2a18bf4f8b0bd2ba151ff6321c66b29 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Thu, 27 Jun 2024 18:04:09 +0100 Subject: [PATCH 2/4] Encode missing values as null instead of omitting them. --- R/outpack_misc.R | 14 ++++++++------ inst/schema/outpack/git.json | 4 ++-- tests/testthat/test-outpack-git.R | 5 ++++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/R/outpack_misc.R b/R/outpack_misc.R index 3c3be020..e1fa0c3a 100644 --- a/R/outpack_misc.R +++ b/R/outpack_misc.R @@ -45,16 +45,18 @@ git_info <- function(path) { return(NULL) } + sha <- tryCatch(gert::git_commit_id(repo = repo), + error = function(e) NA) + branch <- gert::git_branch(repo = repo) - if (identical(branch, "HEAD")) { - # HEAD isn't a valid branch name, and instead is what gets returned when a + if (is.null(branch) || identical(branch, "HEAD")) { + # NULL can be returned when working in a repo that has no commits yet. + # "HEAD" isn't a valid branch name and instead is what gets returned when a # detached head was checked out. - branch <- NULL + branch <- NA } - list(sha = ignore_errors(gert::git_commit_id(repo = repo)), - branch = branch, - url = gert::git_remote_list(repo = repo)$url) + list(sha = sha, branch = branch, url = gert::git_remote_list(repo = repo)$url) } diff --git a/inst/schema/outpack/git.json b/inst/schema/outpack/git.json index e5bfbdf4..5fe44ac2 100644 --- a/inst/schema/outpack/git.json +++ b/inst/schema/outpack/git.json @@ -7,11 +7,11 @@ "type": "object", "properties": { "sha": { - "type": "string", + "type": ["string", "null"], "pattern": "^[0-9a-f]+$" }, "branch": { - "type": "string" + "type": ["string", "null"] }, "url": { "type": "array", diff --git a/tests/testthat/test-outpack-git.R b/tests/testthat/test-outpack-git.R index bdc3ae9c..0e680a57 100644 --- a/tests/testthat/test-outpack-git.R +++ b/tests/testthat/test-outpack-git.R @@ -89,6 +89,7 @@ test_that("store git information when on a detached HEAD", { meta <- orderly_metadata(id, root = root$path) expect_mapequal(meta$git, list(sha = git_info$sha, + branch = NULL, url = git_info$url)) }) @@ -108,5 +109,7 @@ test_that("handle empty git repository correctly", { }) meta <- orderly_metadata(id, root = root$path) - expect_mapequal(meta$git, list(url = "https://example.com/git")) + expect_mapequal(meta$git, list(sha = NULL, + branch = NULL, + url = "https://example.com/git")) }) From 13ee4a7ce317988cea310fbe5978185441de365e Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Mon, 8 Jul 2024 14:51:41 +0100 Subject: [PATCH 3/4] Update schema --- inst/schema/outpack/README.md | 4 ++-- inst/schema/outpack/git.json | 3 ++- inst/schema/outpack/metadata.json | 7 +++---- inst/schema/outpack/relative-path.json | 9 +++++++++ 4 files changed, 16 insertions(+), 7 deletions(-) create mode 100644 inst/schema/outpack/relative-path.json diff --git a/inst/schema/outpack/README.md b/inst/schema/outpack/README.md index 852daa76..83ac244e 100644 --- a/inst/schema/outpack/README.md +++ b/inst/schema/outpack/README.md @@ -1,8 +1,8 @@ # Imported from outpack * Schema version 0.1.1 -* Imported on 2023-08-17 10:00:21.068664 -* From outpack @ ba2bb5bf44a56b3c0ce78128fa419375df109fe3 (main) +* Imported on 2024-07-10 10:46:08.587491 +* From outpack @ f521a290636c01c777d6550bca78e0a8e0f051f8 (main) Do not make changes to files here, they will be overwritten Run ./scripts/update_schemas to update diff --git a/inst/schema/outpack/git.json b/inst/schema/outpack/git.json index 5fe44ac2..9161b1fc 100644 --- a/inst/schema/outpack/git.json +++ b/inst/schema/outpack/git.json @@ -19,5 +19,6 @@ "type": "string" } } - } + }, + "required": ["url", "sha", "branch"] } diff --git a/inst/schema/outpack/metadata.json b/inst/schema/outpack/metadata.json index 85fd1272..7488e413 100644 --- a/inst/schema/outpack/metadata.json +++ b/inst/schema/outpack/metadata.json @@ -60,7 +60,7 @@ "properties": { "path": { "description": "The path of the file", - "type": "string" + "$ref": "relative-path.json" }, "hash": { "$ref": "hash.json" @@ -88,17 +88,16 @@ }, "files": { "type": "array", - "minItems": 1, "items": { "type": "object", "properties": { "here": { "description": "The path of the file in this packet", - "type": "string" + "$ref": "relative-path.json" }, "there": { "description": "The path of the file within the upstream packet", - "type": "string" + "$ref": "relative-path.json" } }, "required": ["here", "there"] diff --git a/inst/schema/outpack/relative-path.json b/inst/schema/outpack/relative-path.json new file mode 100644 index 00000000..1c27adee --- /dev/null +++ b/inst/schema/outpack/relative-path.json @@ -0,0 +1,9 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "File path", + "description": "A relative cross-platform file path", + "version": "0.1.1", + + "type": "string", + "pattern": "^([^<>:\"/\\\\|?*\\x00-\\x1f]+/)*[^<>:\"/\\\\|?*\\x00-\\x1f]+$" +} From c8ac3d09ce7994bf3604af581fa499b8f4ea3f5e Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Wed, 10 Jul 2024 10:48:57 +0100 Subject: [PATCH 4/4] Bump version --- DESCRIPTION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6ecb1305..f9ce50cb 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.20 +Version: 1.99.21 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"), email = "rich.fitzjohn@gmail.com"), person("Robert", "Ashton", role = "aut"),