From 66a2631df9f50d3a582ef1384ecf9ae81058c10f Mon Sep 17 00:00:00 2001 From: piegames Date: Fri, 2 May 2025 10:14:17 +0200 Subject: [PATCH 1/7] small error handling improvemnent Printing errors to log doesn't work well when used as a library --- src/git.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/git.rs b/src/git.rs index 6ac2c71..ceb8ffd 100644 --- a/src/git.rs +++ b/src/git.rs @@ -635,17 +635,14 @@ async fn fetch_remote(args: &[&str]) -> Result> { .await .context("Failed waiting for git ls-remote subprocess")?; if !process.status.success() { - log::error!("git ls-remote failed. stderr output:"); - String::from_utf8_lossy(&process.stderr) - .split('\n') - .for_each(|line| log::error!("> {}", line)); anyhow::bail!( - "git ls-remote failed with exit code {}", + "git ls-remote failed with exit code {}\n{}", process .status .code() .map(|code| code.to_string()) - .unwrap_or_else(|| "None".into()) + .unwrap_or_else(|| "None".into()), + String::from_utf8_lossy(&process.stderr) ); } log::debug!("git ls-remote stdout:"); From ddb925e70846e41ca74b6038a346d5fbdfecae6b Mon Sep 17 00:00:00 2001 From: piegames Date: Tue, 6 May 2025 16:50:58 +0200 Subject: [PATCH 2/7] check all URLs beforehand This typically gives way better error message for little performance cost --- src/git.rs | 10 ++++++---- src/lib.rs | 14 ++++++++++++++ src/nix.rs | 5 +++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/git.rs b/src/git.rs index ceb8ffd..054336c 100644 --- a/src/git.rs +++ b/src/git.rs @@ -624,7 +624,9 @@ impl RemoteInfo { } /// Convenience wrapper around calling `git ls-remote` -async fn fetch_remote(args: &[&str]) -> Result> { +async fn fetch_remote(url: &str, args: &[&str]) -> Result> { + check_url(url).await?; + log::debug!("Executing `git ls-remote {}`", args.join(" ")); let process = Command::new("git") // Disable any interactive login attempts, failing gracefully instead @@ -674,7 +676,7 @@ async fn fetch_remote(args: &[&str]) -> Result> { pub async fn fetch_ref(repo: &Url, ref_: impl AsRef) -> Result { let ref_ = ref_.as_ref(); - let mut remotes = fetch_remote(&["--refs", repo.as_str(), ref_]) + let mut remotes = fetch_remote(repo.as_str(), &["--refs", repo.as_str(), ref_]) .await .with_context(|| format!("Failed to get revision from remote for {} {}", repo, ref_))?; @@ -698,7 +700,7 @@ pub async fn fetch_branch_head(repo: &Url, branch: impl AsRef) -> Result Result> { - let remotes = fetch_remote(&["--refs", repo.as_str(), "refs/tags/*"]) + let remotes = fetch_remote(repo.as_str(), &["--refs", repo.as_str(), "refs/tags/*"]) .await .with_context(|| format!("Failed to list tags for {}", repo))?; @@ -706,7 +708,7 @@ pub async fn fetch_tags(repo: &Url) -> Result> { } pub async fn fetch_default_branch(repo: &Url) -> Result { - let remotes = fetch_remote(&["--symref", repo.as_str(), "HEAD"]) + let remotes = fetch_remote(repo.as_str(), &["--symref", repo.as_str(), "HEAD"]) .await .with_context(|| format!("Failed to resolve default branch for {}", repo))?; diff --git a/src/lib.rs b/src/lib.rs index f30521d..3fb73bf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,6 +47,20 @@ where Ok(serde_json::from_str(&response)?) } +/// Issue a http HEAD request to an URL as a quick sanity check for its validity. +/// Doing this beforehand adds little overhead and greatly improves error messages +async fn check_url(url: &str) -> anyhow::Result<()> { + log::debug!("Checking {url}"); + let response = build_client()?.head(url).send().await?; + /* Some servers don't like HEAD and will give us "405 Method Not Allowed" for that, + * this has nothing to do with out sanity check and can safely be ignored. + */ + if response.status() != reqwest::StatusCode::from_u16(405).unwrap() { + response.error_for_status()?; + } + anyhow::Ok(()) +} + /// The main trait implemented by all pins /// /// It comes with two associated types, `Version` and `Hashes`. Together, each of these types diff --git a/src/nix.rs b/src/nix.rs index 0bde5b2..947a4b2 100644 --- a/src/nix.rs +++ b/src/nix.rs @@ -1,3 +1,4 @@ +use crate::check_url; use anyhow::{Context, Result}; use log::debug; @@ -9,6 +10,8 @@ pub struct PrefetchInfo { pub async fn nix_prefetch_tarball(url: impl AsRef) -> Result { let url = url.as_ref(); + check_url(url).await?; + log::debug!( "Executing `nix-prefetch-url --unpack --name source --type sha256 {}`", url @@ -44,6 +47,8 @@ pub async fn nix_prefetch_git( submodules: bool, ) -> Result { let url = url.as_ref(); + check_url(url).await?; + log::debug!( "Executing: `nix-prefetch-git {}{} {}`", if submodules { From dd32d914d555dfd54fe6e6ecec510f8154f727c8 Mon Sep 17 00:00:00 2001 From: piegames Date: Thu, 24 Apr 2025 13:46:35 +0200 Subject: [PATCH 3/7] git: Refactoring around repositories --- src/cli.rs | 53 +++++++---------- src/flake.rs | 46 ++++++++------- src/git.rs | 154 +++++++++++------------------------------------- src/niv.rs | 11 +++- src/versions.rs | 6 +- 5 files changed, 93 insertions(+), 177 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 95efbc6..b8ee0e6 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -123,16 +123,13 @@ pub struct GitHubAddOpts { impl GitHubAddOpts { pub fn add(&self) -> Result<(Option, Pin)> { + let repository = git::Repository::github(&self.owner, &self.repository); + Ok(( Some(self.repository.clone()), match &self.more.branch { Some(branch) => { - let pin = git::GitPin::github( - &self.owner, - &self.repository, - branch.clone(), - self.more.submodules, - ); + let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); let version = self .more .at @@ -142,9 +139,8 @@ impl GitHubAddOpts { (pin, version).into() }, None => { - let pin = git::GitReleasePin::github( - &self.owner, - &self.repository, + let pin = git::GitReleasePin::new( + repository, self.more.pre_releases, self.more.version_upper_bound.clone(), self.more.release_prefix.clone(), @@ -177,18 +173,13 @@ impl ForgejoAddOpts { }, _ => Err(err), })?; + let repository = git::Repository::forgejo(server_url, &self.owner, &self.repository); Ok(( Some(self.repository.clone()), match &self.more.branch { Some(branch) => { - let pin = git::GitPin::forgejo( - server_url, - &self.owner, - &self.repository, - branch.clone(), - self.more.submodules, - ); + let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); let version = self .more .at @@ -198,10 +189,8 @@ impl ForgejoAddOpts { (pin, version).into() }, None => { - let pin = git::GitReleasePin::forgejo( - server_url, - &self.owner, - &self.repository, + let pin = git::GitReleasePin::new( + repository, self.more.pre_releases, self.more.version_upper_bound.clone(), self.more.release_prefix.clone(), @@ -244,6 +233,11 @@ pub struct GitLabAddOpts { impl GitLabAddOpts { pub fn add(&self) -> Result<(Option, Pin)> { + let repository = git::Repository::gitlab( + self.repo_path.join("/"), + Some(self.server.clone()), + self.private_token.clone(), + ); Ok(( Some(self.repo_path .last() @@ -251,23 +245,19 @@ impl GitLabAddOpts { .clone()), match &self.more.branch { Some(branch) => { - let pin = git::GitPin::gitlab( - self.repo_path.join("/"), + let pin = git::GitPin::new( + repository, branch.clone(), - Some(self.server.clone()), - self.private_token.clone(), self.more.submodules, ); let version = self.more.at.as_ref().map(|at| git::GitRevision::new(at.clone())).transpose()?; (pin, version).into() }, None => { - let pin = git::GitReleasePin::gitlab( - self.repo_path.join("/"), - Some(self.server.clone()), + let pin = git::GitReleasePin::new( + repository, self.more.pre_releases, self.more.version_upper_bound.clone(), - self.private_token.clone(), self.more.release_prefix.clone(), self.more.submodules, ); @@ -315,12 +305,13 @@ impl GitAddOpts { Some(seg) => seg.to_owned(), }; let name = name.strip_suffix(".git").unwrap_or(&name); + let repository = git::Repository::git(url); Ok(( Some(name.to_owned()), match &self.more.branch { Some(branch) => { - let pin = git::GitPin::git(url, branch.clone(), self.more.submodules); + let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); let version = self .more .at @@ -330,8 +321,8 @@ impl GitAddOpts { (pin, version).into() }, None => { - let pin = git::GitReleasePin::git( - url, + let pin = git::GitReleasePin::new( + repository, self.more.pre_releases, self.more.version_upper_bound.clone(), self.more.release_prefix.clone(), diff --git a/src/flake.rs b/src/flake.rs index 3e4420d..57b4522 100644 --- a/src/flake.rs +++ b/src/flake.rs @@ -76,32 +76,36 @@ impl FlakePin { // TODO: parsing the query string to retrieve servers other than // gitlab.com is not supported for now, but could be added. let branch = self.fetch_default_branch("https://gitlab.com").await?; - git::GitPin::gitlab( - format!( - "{}/{}", - self.locked - .owner - .context("missing field owner in gitlab flake input")?, - self.locked - .repo - .context("missing field repo in gitlab flake input")? + git::GitPin::new( + git::Repository::gitlab( + format!( + "{}/{}", + self.locked + .owner + .context("missing field owner in gitlab flake input")?, + self.locked + .repo + .context("missing field repo in gitlab flake input")? + ), + None, + None, ), branch, - None, - None, false, ) .into() }, Github => { let branch = self.fetch_default_branch("https://github.com").await?; - git::GitPin::github( - self.locked - .owner - .context("missing owner field in github flake input")?, - self.locked - .repo - .context("missing field repo in github flake input")?, + git::GitPin::new( + git::Repository::github( + self.locked + .owner + .context("missing owner field in github flake input")?, + self.locked + .repo + .context("missing field repo in github flake input")?, + ), branch, false, ) @@ -112,8 +116,10 @@ impl FlakePin { if let Some(shortened) = ref_.strip_prefix("refs/heads/") { ref_ = shortened.to_string(); } - git::GitPin::git( - self.locked.url.context("missing url on git flake input")?, + git::GitPin::new( + git::Repository::git( + self.locked.url.context("missing url on git flake input")?, + ), ref_, false, ) diff --git a/src/git.rs b/src/git.rs index 054336c..b05e591 100644 --- a/src/git.rs +++ b/src/git.rs @@ -118,6 +118,34 @@ pub enum Repository { } impl Repository { + pub fn git(url: url::Url) -> Self { + Self::Git { url } + } + + pub fn github(owner: impl Into, repo: impl Into) -> Self { + Repository::GitHub { + owner: owner.into(), + repo: repo.into(), + } + } + + pub fn forgejo(server: Url, owner: impl Into, repo: impl Into) -> Self { + Repository::Forgejo { + server, + owner: owner.into(), + repo: repo.into(), + } + } + + pub fn gitlab(repo_path: String, server: Option, private_token: Option) -> Self { + let server = server.unwrap_or_else(|| "https://gitlab.com/".parse().unwrap()); + Repository::GitLab { + repo_path, + server, + private_token, + } + } + /// Get the URL to the represented Git repository fn git_url(&self) -> Result { Ok(match self { @@ -269,61 +297,9 @@ impl diff::Diff for GitPin { } impl GitPin { - pub fn git(url: Url, branch: String, submodules: bool) -> Self { - Self { - repository: Repository::Git { url }, - branch, - submodules, - } - } - - pub fn github( - owner: impl Into, - repo: impl Into, - branch: String, - submodules: bool, - ) -> Self { - Self { - repository: Repository::GitHub { - owner: owner.into(), - repo: repo.into(), - }, - branch, - submodules, - } - } - - pub fn forgejo( - server: Url, - owner: impl Into, - repo: impl Into, - branch: String, - submodules: bool, - ) -> Self { + pub fn new(repository: Repository, branch: String, submodules: bool) -> Self { Self { - repository: Repository::Forgejo { - server, - owner: owner.into(), - repo: repo.into(), - }, - branch, - submodules, - } - } - - pub fn gitlab( - repo_path: String, - branch: String, - server: Option, - private_token: Option, - submodules: bool, - ) -> Self { - Self { - repository: Repository::GitLab { - repo_path, - server: server.unwrap_or_else(|| "https://gitlab.com/".parse().unwrap()), - private_token, - }, + repository, branch, submodules, } @@ -426,79 +402,15 @@ impl diff::Diff for GitReleasePin { } impl GitReleasePin { - pub fn git( - url: Url, + pub fn new( + repository: Repository, pre_releases: bool, version_upper_bound: Option, release_prefix: Option, submodules: bool, ) -> Self { Self { - repository: Repository::Git { url }, - pre_releases, - version_upper_bound, - release_prefix, - submodules, - } - } - - pub fn github( - owner: impl Into, - repo: impl Into, - pre_releases: bool, - version_upper_bound: Option, - release_prefix: Option, - submodules: bool, - ) -> Self { - Self { - repository: Repository::GitHub { - owner: owner.into(), - repo: repo.into(), - }, - pre_releases, - version_upper_bound, - release_prefix, - submodules, - } - } - - pub fn forgejo( - server: Url, - owner: impl Into, - repo: impl Into, - pre_releases: bool, - version_upper_bound: Option, - release_prefix: Option, - submodules: bool, - ) -> Self { - Self { - repository: Repository::Forgejo { - server, - owner: owner.into(), - repo: repo.into(), - }, - pre_releases, - version_upper_bound, - release_prefix, - submodules, - } - } - - pub fn gitlab( - repo_path: String, - server: Option, - pre_releases: bool, - version_upper_bound: Option, - private_token: Option, - release_prefix: Option, - submodules: bool, - ) -> Self { - Self { - repository: Repository::GitLab { - repo_path, - server: server.unwrap_or_else(|| "https://gitlab.com/".parse().unwrap()), - private_token, - }, + repository, pre_releases, version_upper_bound, release_prefix, diff --git a/src/niv.rs b/src/niv.rs index c1bd544..e34271e 100644 --- a/src/niv.rs +++ b/src/niv.rs @@ -23,8 +23,15 @@ impl TryFrom for Pin { fn try_from(niv: NivPin) -> Result { Ok(match niv.owner { - None => git::GitPin::git(niv.repo.parse()?, niv.branch, false).into(), - Some(owner) => git::GitPin::github(&owner, &niv.repo, niv.branch, false).into(), + None => { + git::GitPin::new(git::Repository::git(niv.repo.parse()?), niv.branch, false).into() + }, + Some(owner) => git::GitPin::new( + git::Repository::github(&owner, &niv.repo), + niv.branch, + false, + ) + .into(), }) } } diff --git a/src/versions.rs b/src/versions.rs index dd42b6b..02a9fe8 100644 --- a/src/versions.rs +++ b/src/versions.rs @@ -276,13 +276,13 @@ mod test { NixPins { pins: btreemap![ "nixos-mailserver".into() => Pin::Git { - input: git::GitPin::git("https://gitlab.com/simple-nixos-mailserver/nixos-mailserver.git".parse().unwrap(), "nixos-21.11".into(), false), + input: git::GitPin::new(git::Repository::git("https://gitlab.com/simple-nixos-mailserver/nixos-mailserver.git".parse().unwrap()), "nixos-21.11".into(), false), version: Some(git::GitRevision::new("6e3a7b2ea6f0d68b82027b988aa25d3423787303".into()).unwrap()), hashes: Some(git::OptionalUrlHashes { url: None, hash: "1i56llz037x416bw698v8j6arvv622qc0vsycd20lx3yx8n77n44".into() } ), frozen: Frozen::default(), }, "nixpkgs".into() => Pin::Git { - input: git::GitPin::github("nixos", "nixpkgs", "nixpkgs-unstable".into(), false), + input: git::GitPin::new(git::Repository::github("nixos", "nixpkgs"), "nixpkgs-unstable".into(), false), version: Some(git::GitRevision::new("5c37ad87222cfc1ec36d6cd1364514a9efc2f7f2".into()).unwrap()), hashes: Some(git::OptionalUrlHashes { url: Some("https://github.com/nixos/nixpkgs/archive/5c37ad87222cfc1ec36d6cd1364514a9efc2f7f2.tar.gz".parse().unwrap()), hash: "1r74afnalgcbpv7b9sbdfbnx1kfj0kp1yfa60bbbv27n36vqdhbb".into() }), frozen: Frozen::default(), @@ -294,7 +294,7 @@ mod test { frozen: Frozen::default(), }, "youtube-dl".into() => Pin::GitRelease { - input: git::GitReleasePin::github("ytdl-org", "youtube-dl", false, None, None, false), + input: git::GitReleasePin::new(git::Repository::github("ytdl-org", "youtube-dl"), false, None, None, false), version: Some(GenericVersion { version: "youtube-dl 2021.12.17".into() }), hashes: None, frozen: Frozen::default(), From 8230f25bb01f6a36147c09355f78d17b35b31bd4 Mon Sep 17 00:00:00 2001 From: piegames Date: Fri, 2 May 2025 11:39:07 +0200 Subject: [PATCH 4/7] cli: Refactor and deduplicate git pin creation Dependency injection for the win! Now moisturize me! I feel DRY --- src/cli.rs | 141 +++++++++++++---------------------------------------- 1 file changed, 33 insertions(+), 108 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index b8ee0e6..2b3b20b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -112,6 +112,35 @@ pub struct GenericGitAddOpts { pub submodules: bool, } +impl GenericGitAddOpts { + fn add(&self, repository: git::Repository) -> Result { + Ok(match &self.branch { + Some(branch) => { + let pin = git::GitPin::new(repository, branch.clone(), self.submodules); + let version = self + .at + .as_ref() + .map(|at| git::GitRevision::new(at.clone())) + .transpose()?; + (pin, version).into() + }, + None => { + let pin = git::GitReleasePin::new( + repository, + self.pre_releases, + self.version_upper_bound.clone(), + self.release_prefix.clone(), + self.submodules, + ); + let version = self.at.as_ref().map(|at| GenericVersion { + version: at.clone(), + }); + (pin, version).into() + }, + }) + } +} + #[derive(Debug, Parser)] pub struct GitHubAddOpts { pub owner: String, @@ -125,34 +154,7 @@ impl GitHubAddOpts { pub fn add(&self) -> Result<(Option, Pin)> { let repository = git::Repository::github(&self.owner, &self.repository); - Ok(( - Some(self.repository.clone()), - match &self.more.branch { - Some(branch) => { - let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); - let version = self - .more - .at - .as_ref() - .map(|at| git::GitRevision::new(at.clone())) - .transpose()?; - (pin, version).into() - }, - None => { - let pin = git::GitReleasePin::new( - repository, - self.more.pre_releases, - self.more.version_upper_bound.clone(), - self.more.release_prefix.clone(), - self.more.submodules, - ); - let version = self.more.at.as_ref().map(|at| GenericVersion { - version: at.clone(), - }); - (pin, version).into() - }, - }, - )) + Ok((Some(self.repository.clone()), self.more.add(repository)?)) } } @@ -175,34 +177,7 @@ impl ForgejoAddOpts { })?; let repository = git::Repository::forgejo(server_url, &self.owner, &self.repository); - Ok(( - Some(self.repository.clone()), - match &self.more.branch { - Some(branch) => { - let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); - let version = self - .more - .at - .as_ref() - .map(|at| git::GitRevision::new(at.clone())) - .transpose()?; - (pin, version).into() - }, - None => { - let pin = git::GitReleasePin::new( - repository, - self.more.pre_releases, - self.more.version_upper_bound.clone(), - self.more.release_prefix.clone(), - self.more.submodules, - ); - let version = self.more.at.as_ref().map(|at| GenericVersion { - version: at.clone(), - }); - (pin, version).into() - }, - }, - )) + Ok((Some(self.repository.clone()), self.more.add(repository)?)) } } @@ -243,30 +218,7 @@ impl GitLabAddOpts { .last() .ok_or_else(|| anyhow::format_err!("GitLab repository path must at least have one element (usually two: owner, repo)"))? .clone()), - match &self.more.branch { - Some(branch) => { - let pin = git::GitPin::new( - repository, - branch.clone(), - self.more.submodules, - ); - let version = self.more.at.as_ref().map(|at| git::GitRevision::new(at.clone())).transpose()?; - (pin, version).into() - }, - None => { - let pin = git::GitReleasePin::new( - repository, - self.more.pre_releases, - self.more.version_upper_bound.clone(), - self.more.release_prefix.clone(), - self.more.submodules, - ); - let version = self.more.at.as_ref().map(|at| GenericVersion { - version: at.clone(), - }); - (pin, version).into() - }, - }, + self.more.add(repository)?, )) } } @@ -307,34 +259,7 @@ impl GitAddOpts { let name = name.strip_suffix(".git").unwrap_or(&name); let repository = git::Repository::git(url); - Ok(( - Some(name.to_owned()), - match &self.more.branch { - Some(branch) => { - let pin = git::GitPin::new(repository, branch.clone(), self.more.submodules); - let version = self - .more - .at - .as_ref() - .map(|at| git::GitRevision::new(at.clone())) - .transpose()?; - (pin, version).into() - }, - None => { - let pin = git::GitReleasePin::new( - repository, - self.more.pre_releases, - self.more.version_upper_bound.clone(), - self.more.release_prefix.clone(), - self.more.submodules, - ); - let version = self.more.at.as_ref().map(|at| GenericVersion { - version: at.clone(), - }); - (pin, version).into() - }, - }, - )) + Ok((Some(name.to_owned()), self.more.add(repository)?)) } } From 2a7a0e9262c6634ee734f9e633c84221c27265a3 Mon Sep 17 00:00:00 2001 From: piegames Date: Fri, 2 May 2025 14:09:47 +0200 Subject: [PATCH 5/7] git: Disable interactive host key prompting --- src/git.rs | 1 + src/nix.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/git.rs b/src/git.rs index b05e591..921b2b2 100644 --- a/src/git.rs +++ b/src/git.rs @@ -543,6 +543,7 @@ async fn fetch_remote(url: &str, args: &[&str]) -> Result> { let process = Command::new("git") // Disable any interactive login attempts, failing gracefully instead .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_SSH_COMMAND", "ssh -o StrictHostKeyChecking=yes") .arg("ls-remote") .args(args) .output() diff --git a/src/nix.rs b/src/nix.rs index 947a4b2..2248079 100644 --- a/src/nix.rs +++ b/src/nix.rs @@ -66,6 +66,7 @@ pub async fn nix_prefetch_git( let output = output // Disable any interactive login attempts, failing gracefully instead .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_SSH_COMMAND", "ssh -o StrictHostKeyChecking=yes") .arg(url) .arg(git_ref.as_ref()) .output() From 1fad9027f51a2a6f5d8568f4dff96e58dd7dc8ee Mon Sep 17 00:00:00 2001 From: piegames Date: Fri, 2 May 2025 16:44:45 +0200 Subject: [PATCH 6/7] git: Resolve ambiguity with GitHub API Currently pins fail when trying to fetch a release on a repo that has a branch with the same name --- src/git.rs | 38 ++++++++++++++++++++++++++++++++++++-- test.nix | 6 +++--- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/git.rs b/src/git.rs index 921b2b2..334e9d1 100644 --- a/src/git.rs +++ b/src/git.rs @@ -229,7 +229,7 @@ impl Repository { Repository::Git { .. } => None, Repository::GitHub { owner, repo } => Some( format!( - "{github_api}/repos/{owner}/{repo}/tarball/{tag}", + "{github_api}/repos/{owner}/{repo}/tarball/refs/tags/{tag}", github_api = get_github_api_url(), owner = owner, repo = repo, @@ -938,7 +938,7 @@ mod test { ReleasePinHashes { revision: "35be5b2b2c3431de1100996487d53134f658b866".into(), url: Some( - "https://api.github.com/repos/jstutters/MidiOSC/tarball/v1.1" + "https://api.github.com/repos/jstutters/MidiOSC/tarball/refs/tags/v1.1" .parse() .unwrap() ), @@ -948,6 +948,40 @@ mod test { Ok(()) } + // That repo has a tag and a branch with the same name, and the naive endpoint for + // GitHub which usually works then returns + // { + // "message": "'0.2.1' has multiple possibilities: https://github.com/alexfedosov/AFHorizontalDayPicker/tarball/refs/heads/0.2.1, https://github.com/alexfedosov/AFHorizontalDayPicker/tarball/refs/tags/0.2.1", + // "documentation_url": "https://docs.github.com/rest/repos/contents#download-a-repository-archive-tar", + // "status": "300" + // } + #[tokio::test] + async fn test_github_release_ambiguous() -> Result<()> { + let pin = GitReleasePin { + repository: Repository::github("alexfedosov", "AFHorizontalDayPicker"), + pre_releases: false, + version_upper_bound: None, + release_prefix: None, + submodules: false, + }; + let version = GenericVersion { + version: "0.2.1".into(), + }; + assert_eq!( + pin.fetch(&version).await?, + ReleasePinHashes { + revision: "ca59ad1dc1b55108f1d17f20bdf443aad3e2f0f5".into(), + url: Some( + "https://api.github.com/repos/alexfedosov/AFHorizontalDayPicker/tarball/refs/tags/0.2.1" + .parse() + .unwrap() + ), + hash: "0arqpja90n3yy767x0ckwg4biqm4igcpa0vznvx3daaywjkb1v7v".into(), + } + ); + Ok(()) + } + #[tokio::test] async fn test_forgejo_update() -> Result<()> { let pin = GitPin { diff --git a/test.nix b/test.nix index caf5533..f61a4c6 100644 --- a/test.nix +++ b/test.nix @@ -338,7 +338,7 @@ let ln -s ${gitRepo} "${repoPath}.git" # Mock the releases - tarballPath="api/repos/${repoPath}/tarball" + tarballPath="api/repos/${repoPath}/tarball/refs/tags" mkdir -p $tarballPath archivePath="${repoPath}/archive" mkdir -p $archivePath @@ -604,7 +604,7 @@ in # Check version and url eq "$(jq -r .pins.bar.version npins/sources.json)" "v0.2" eq "$(jq -r .pins.bar.revision npins/sources.json)" "$(resolveGitCommit ${gitRepo} v0.2)" - eq "$(jq -r .pins.bar.url npins/sources.json)" "http://localhost:8000/api/repos/foo/bar/tarball/v0.2" + eq "$(jq -r .pins.bar.url npins/sources.json)" "http://localhost:8000/api/repos/foo/bar/tarball/refs/tags/v0.2" ''; }; @@ -695,7 +695,7 @@ in eq "$(jq -r .pins.foo2.version npins/sources.json)" "v0.5" eq "$(jq -r .pins.foo.revision npins/sources.json)" "$(resolveGitCommit ${repositories."owner/foo"})" eq "$(jq -r .pins.foo2.revision npins/sources.json)" "$(resolveGitCommit ${repositories."owner/foo"})" - eq "$(jq -r .pins.foo.url npins/sources.json)" "http://localhost:8000/api/repos/owner/foo/tarball/v0.5" + eq "$(jq -r .pins.foo.url npins/sources.json)" "http://localhost:8000/api/repos/owner/foo/tarball/refs/tags/v0.5" # release pins with submodules don't have a URL eq "$(jq -r .pins.foo2.url npins/sources.json)" "null" ''; From a419d89b5739e6e43b57931212c3aa5c60c0f562 Mon Sep 17 00:00:00 2001 From: piegames Date: Fri, 9 May 2025 15:46:52 +0200 Subject: [PATCH 7/7] version: Generalize mechanism to allow for non-trivial version upgrades Inspired by code by Tom Hubrecht in #139 --- src/versions.rs | 87 +++++++++++++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 32 deletions(-) diff --git a/src/versions.rs b/src/versions.rs index 02a9fe8..003402f 100644 --- a/src/versions.rs +++ b/src/versions.rs @@ -54,38 +54,57 @@ pub fn upgrade(mut pins_raw: Map) -> Result { ) })?; - /* This is where the upgrading happens (at the moment we don't have any versions to upgrade from) */ - match version { - 0 => { - let pins = pins_raw - .get_mut("pins") - .and_then(Value::as_object_mut) - .ok_or_else(|| anyhow::format_err!("sources.json must contain a `pins` object"))?; - for (name, pin) in pins.iter_mut() { - upgrade_v0_pin( - name, - pin.as_object_mut() - .ok_or_else(|| anyhow::format_err!("Pin {} must be an object", name))?, - ) - .context(anyhow::format_err!( - "Pin {} could not be upgraded to the latest format version", - name - ))?; - } - }, - // All these versions are already handled by serde default fields - 1 | 2 | 3 | 4 => { - log::info!("There is nothing to do"); - }, - 5 => { - log::info!("sources.json is already up to date"); - }, - unknown => { - anyhow::bail!( - "Unknown format version {}, maybe try updating the application?", - unknown - ); - }, + /* A generic wrapper that updates all pins individually with a provided upgrade function. + * This can be used in all cases where only the pin structure and not the overall file structure + * changes, which should actually be most cases. + */ + fn generic_upgrader( + pins_raw: &mut Map, + update_pin_fn: fn(&str, &mut Map) -> Result<()>, + ) -> Result<()> { + let pins = pins_raw + .get_mut("pins") + .and_then(Value::as_object_mut) + .ok_or_else(|| anyhow::format_err!("sources.json must contain a `pins` object"))?; + for (name, pin) in pins.iter_mut() { + update_pin_fn( + name, + pin.as_object_mut() + .ok_or_else(|| anyhow::format_err!("Pin {} must be an object", name))?, + ) + .context(anyhow::format_err!("Pin {} could not be upgraded", name))?; + } + Ok(()) + } + + /* Registry for version upgrade closures. Every uprade is registered for a version and will + * modify `pins_raw` to be of its following version. + * Most version upgrades are handled by serde default fields and don't need any special treatment. + * They are omitted here; Only non-trivial upgrades should be inserted. + */ + type Upgrader = Box) -> Result<()>>; + let version_upgraders: BTreeMap = [( + 0, + Box::new(|pins_raw: &mut Map| generic_upgrader(pins_raw, upgrade_v0_pin)) + as Upgrader, + )] + .into_iter() + .collect(); + + /* Some quick version checks to provide better user feedback */ + if version > LATEST { + anyhow::bail!( + "Unknown format version {}, maybe try updating the application?", + version + ); + } else if version == LATEST { + log::info!("sources.json is already up to date"); + } else { + for (v, upgrader) in version_upgraders.range(version..) { + log::info!("Upgrading to v{}", v + 1); + upgrader(&mut pins_raw)?; + } + log::info!("Upgrade complete"); } /* Set the new version */ @@ -107,6 +126,10 @@ macro_rules! rename { }} } +/* v0→v1. This upgrade changes the structure of git pins from a Git/GitHub/GitHubRelease split + * to a Git/GitRelease split where both kinds of pin can handle all types of repositories (GitHub or not) + * via the `Repository` struct. + */ fn upgrade_v0_pin(name: &str, raw_pin: &mut Map) -> Result<()> { log::debug!("Updating {} to v1", name);