chadtmiller.com/knotless
git that lives in your Atmosphere account
1
10
Unscoped exclusive labels coexist, and a failed retarget changes nothing
.changeset/client-quirks.mdnew+7−0
| @@ -0,0 +1,7 @@ | ||
--- | ||
default: patch | ||
--- | ||
| ||
# Exclusive labels with no scope stand together, and a failed retarget changes nothing | ||
| ||
An exclusive label with no `/` in its name no longer takes other such labels off: only labels in the same scope, such as `priority/high` and `priority/low`, rival each other. `knotless retarget` with both `--base` and `--head` checks the head first, so one refused for its head no longer moves the pull's base. Several workflow files with names that are not valid each give their own error in the one `invalid` check, rather than a duplicate-name error. `knotless queue` and `knotless uses update` no longer panic on a commit ID shorter than 12 characters, and the gateway takes SHA-256 commit IDs as pushes do. | ||
crates/knotless/src/cli.rs+14−2
| @@ -1222,7 +1222,7 @@ async fn command( | ||
} | ||
for p in crate::uses::update(ctx, &dir, &only).await? { | ||
let was = p.was.as_deref().unwrap_or("-"); | ||
let (now, commit) = (&p.locked.version, &p.locked.commit[..12]); | ||
let (now, commit) = (&p.locked.version, abbrev(&p.locked.commit)); | ||
writeln!(out, "{}\t{was}\t{now}\t{commit}", p.uses)?; | ||
} | ||
} | ||
| @@ -2257,7 +2257,7 @@ async fn command( | ||
let (at, state) = match (&w.place, &w.batch) { | ||
(Some(p), Some(b)) => ( | ||
p.position.to_string(), | ||
format!("{} at {}", b.state, &b.commit[..12]), | ||
format!("{} at {}", b.state, abbrev(&b.commit)), | ||
), | ||
(Some(p), None) if !w.preflight.is_empty() => ( | ||
p.position.to_string(), | ||
| @@ -2488,10 +2488,22 @@ fn bot_flag(flag: &str) -> Option<bool> { | ||
} | ||
} | ||
| ||
/// The first 12 characters of a commit's ID, or all of a shorter one, as a | ||
/// record may hold one. | ||
fn abbrev(commit: &str) -> &str { | ||
commit.get(..12).unwrap_or(commit) | ||
} | ||
| ||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
| ||
#[test] | ||
fn a_short_commit_is_shown_whole() { | ||
assert_eq!(abbrev("0123456789abcdef"), "0123456789ab"); | ||
assert_eq!(abbrev("abc"), "abc"); | ||
} | ||
| ||
#[test] | ||
fn logins_behind_what_knotless_asks_for_say_so() { | ||
let dir = tempfile::tempdir().unwrap(); | ||
crates/knotless/src/forge.rs+50−29
| @@ -572,12 +572,10 @@ fn labelled( | ||
let Some(when) = standing(&label.name) else { | ||
continue; | ||
}; | ||
let beaten = label.exclusive | ||
&& repo.labels.iter().any(|other| { | ||
other.exclusive | ||
&& other.scope() == label.scope() | ||
&& standing(&other.name).is_some_and(|w| w > when) | ||
}); | ||
let beaten = repo | ||
.labels | ||
.iter() | ||
.any(|other| other.rivals(label) && standing(&other.name).is_some_and(|w| w > when)); | ||
if !beaten { | ||
out.push(label.name.clone()); | ||
} | ||
| @@ -1915,9 +1913,11 @@ pub async fn label( | ||
let at = records::now(); | ||
let mine = t.relabel(&mine, &me, subject, |record| { | ||
for label in defined { | ||
let rivals = t.record.labels.iter().filter(|other| { | ||
!removed && label.exclusive && other.exclusive && other.scope() == label.scope() | ||
}); | ||
let rivals = t | ||
.record | ||
.labels | ||
.iter() | ||
.filter(|other| !removed && other.rivals(label)); | ||
for other in rivals { | ||
record.say(&other.name, &at, true); | ||
} | ||
| @@ -2614,10 +2614,18 @@ pub async fn retarget( | ||
if item.state == "merged" { | ||
bail!("{pull} is merged"); | ||
} | ||
if let Some(base) = base | ||
&& t.snap.find_ref(&format!("refs/heads/{base}")).is_none() | ||
{ | ||
bail!("{target} has no branch {base}"); | ||
} | ||
// The head is checked before the base is written, so a retarget that | ||
// fails changes neither. | ||
let moved = match head { | ||
Some(head) => Some(new_head(ctx, &t, pull, value, head).await?), | ||
None => None, | ||
}; | ||
if let Some(base) = base { | ||
if t.snap.find_ref(&format!("refs/heads/{base}")).is_none() { | ||
bail!("{target} has no branch {base}"); | ||
} | ||
let record = StateRecord { | ||
repo: t.reference(), | ||
subject: pull.to_string(), | ||
| @@ -2632,29 +2640,42 @@ pub async fn retarget( | ||
t.write(ctx, nsid::STATE, records::typed(nsid::STATE, &record)) | ||
.await?; | ||
} | ||
if let Some(head) = head { | ||
let (mine, me) = t.mine(ctx).await?; | ||
let (author, _, rkey) = parse_uri(pull)?; | ||
if author != me.did { | ||
bail!("only the author of {pull} can change its head"); | ||
} | ||
let mut record: PullRecord = serde_json::from_value(value)?; | ||
let source = Target::open(ctx, &repo_url(&record.head.repo.uri)?).await?; | ||
if source | ||
.snap | ||
.find_ref(&format!("refs/heads/{head}")) | ||
.is_none() | ||
{ | ||
bail!("{} has no branch {head}", source.url); | ||
} | ||
record.head.branch = head.to_string(); | ||
if let Some((mine, me, rkey, record)) = moved { | ||
let value = records::typed(nsid::PULL, &record); | ||
mine.apply(vec![remote::update(nsid::PULL, rkey, value)], &me.auth()) | ||
mine.apply(vec![remote::update(nsid::PULL, &rkey, value)], &me.auth()) | ||
.await?; | ||
} | ||
Ok(()) | ||
} | ||
| ||
/// The pull record `pull` (read as `value`) becomes with its head branch | ||
/// `head`, with where its author writes it: only its author may, and only | ||
/// to a branch its head repository has. | ||
async fn new_head( | ||
ctx: &Ctx, | ||
t: &Target, | ||
pull: &str, | ||
value: Value, | ||
head: &str, | ||
) -> Result<(Remote, Session, String, PullRecord)> { | ||
let (mine, me) = t.mine(ctx).await?; | ||
let (author, _, rkey) = parse_uri(pull)?; | ||
if author != me.did { | ||
bail!("only the author of {pull} can change its head"); | ||
} | ||
let mut record: PullRecord = serde_json::from_value(value)?; | ||
let source = Target::open(ctx, &repo_url(&record.head.repo.uri)?).await?; | ||
if source | ||
.snap | ||
.find_ref(&format!("refs/heads/{head}")) | ||
.is_none() | ||
{ | ||
bail!("{} has no branch {head}", source.url); | ||
} | ||
record.head.branch = head.to_string(); | ||
Ok((mine, me, rkey.to_string(), record)) | ||
} | ||
| ||
/// Pushes `updates` to the target as `me`: directly as its owner, through | ||
/// the gateway as a collaborator. Fails on the first ref refused. | ||
pub(crate) async fn push_as( | ||
crates/knotless/src/gateway.rs+5−2
| @@ -520,9 +520,11 @@ pub async fn refusal( | ||
Ok(refused.map(str::to_string)) | ||
} | ||
| ||
/// Whether a commit's tip is a hash git can name, as a gateway requires. | ||
/// Whether a commit's tip is a hash git can name, SHA-1 or SHA-256, in | ||
/// lowercase hex, as a gateway requires. | ||
pub fn is_sha(text: &str) -> bool { | ||
text.len() == 40 && text.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) | ||
(text.len() == 40 || text.len() == 64) | ||
&& text.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) | ||
} | ||
| ||
#[cfg(test)] | ||
| @@ -539,6 +541,7 @@ mod tests { | ||
assert_eq!(header.repo, "r"); | ||
assert_eq!(pack, b"PACK"); | ||
assert!(is_sha(&"a".repeat(40)) && !is_sha("main") && !is_sha(&"A".repeat(40))); | ||
assert!(is_sha(&"a".repeat(64)) && !is_sha(&"a".repeat(41))); | ||
} | ||
| ||
#[test] | ||
crates/knotless/src/push.rs+1−6
| @@ -43,11 +43,6 @@ impl Update { | ||
} | ||
} | ||
| ||
fn is_sha(text: &str) -> bool { | ||
(text.len() == 40 || text.len() == 64) | ||
&& text.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) | ||
} | ||
| ||
/// Per ref, `Ok` or the reason it was refused, in git's words. | ||
pub type Report = Vec<(String, std::result::Result<(), String>)>; | ||
| ||
| @@ -96,7 +91,7 @@ pub async fn push_by( | ||
let previous = snap.find_ref(&update.dst).cloned(); | ||
let from = previous.as_ref().and_then(|r| r.value.target.clone()); | ||
let to = match &update.src { | ||
Some(src) if is_sha(src) => Some(src.clone()), | ||
Some(src) if crate::gateway::is_sha(src) => Some(src.clone()), | ||
Some(src) => Some(git.rev_parse(src).await?), | ||
None => None, | ||
}; | ||
crates/knotless/src/records.rs+21−0
| @@ -401,6 +401,12 @@ impl Label { | ||
pub fn scope(&self) -> Option<&str> { | ||
self.name.rsplit_once('/').map(|(scope, _)| scope) | ||
} | ||
| ||
/// Whether only one of it and `other` stands at a time: both are | ||
/// exclusive, in the same scope. A label with no scope rivals none. | ||
pub fn rivals(&self, other: &Label) -> bool { | ||
self.exclusive && other.exclusive && self.scope().is_some() && self.scope() == other.scope() | ||
} | ||
} | ||
| ||
/// An account that puts labels of some scopes on pulls and issues, below | ||
| @@ -1372,6 +1378,21 @@ mod tests { | ||
use super::*; | ||
use serde_json::json; | ||
| ||
/// Exclusive labels rival only those of their own scope, and a label | ||
/// with no scope rivals none. | ||
#[test] | ||
fn exclusive_labels_rival_within_a_scope() { | ||
let label = |name: &str, exclusive: bool| Label { | ||
exclusive, | ||
..Label::new(name) | ||
}; | ||
let high = label("priority/high", true); | ||
assert!(high.rivals(&label("priority/low", true))); | ||
assert!(!high.rivals(&label("priority/low", false))); | ||
assert!(!high.rivals(&label("kind/bug", true))); | ||
assert!(!label("urgent", true).rivals(&label("blocked", true))); | ||
} | ||
| ||
/// An organization's members push beside its collaborators, each named | ||
/// once, and are never written into the record. | ||
#[test] | ||
crates/knotless/src/triage.rs+1−2
| @@ -136,8 +136,7 @@ impl Triage { | ||
pub async fn judge(&self, p: &Pending, choice: &Choice) -> Result<Vec<String>> { | ||
let mut chosen: Vec<&Label> = Vec::new(); | ||
for label in &p.labels { | ||
let rival = |c: &&Label| c.exclusive && c.scope() == label.scope(); | ||
let rivalled = label.exclusive && chosen.iter().any(rival); | ||
let rivalled = chosen.iter().any(|c| c.rivals(label)); | ||
if choice.labels.contains(&label.name) && !rivalled { | ||
chosen.push(label); | ||
} | ||
crates/knotless/src/workflow.rs+10−2
| @@ -570,12 +570,20 @@ pub async fn load(git: &Git, sha: &str) -> Result<Vec<Loaded>> { | ||
Ok(w) => w.name.clone(), | ||
Err(_) => stem(&entry.name).to_string(), | ||
}; | ||
if !valid_name(&name) { | ||
let fallback = !valid_name(&name); | ||
if fallback { | ||
name = "invalid".into(); | ||
} | ||
// Both would file their checks under one key: neither runs, and | ||
// that check says why. | ||
// that check says why. Files whose names are no workflow's, which | ||
// fall back to the one name, each say what is wrong with them. | ||
match loaded.iter_mut().find(|l| l.name == name) { | ||
Some(first) if fallback && first.workflow.is_err() => { | ||
let was = first.workflow.as_ref().err().map(|e| format!("{e:#}")); | ||
let now = workflow.err().map(|e| format!("{e:#}")); | ||
let (was, now) = (was.unwrap_or_default(), now.unwrap_or_default()); | ||
first.workflow = Err(anyhow::anyhow!("{was}\n{now}")); | ||
} | ||
Some(first) => { | ||
first.workflow = Err(anyhow::anyhow!( | ||
"more than one workflow is named {name}, {} among them", | ||
crates/knotless-run/tests/run.rs+9−0
| @@ -972,6 +972,7 @@ fn what_goes_wrong_is_reported() { | ||
("twin-a.yml", "name: twin\nsteps: []\n"), | ||
("twin-b.yml", "name: twin\nsteps: []\n"), | ||
("bad name!.yml", "{"), | ||
("worse name!.yml", "name: also bad\nsteps: []\n"), | ||
] { | ||
std::fs::write(dir.join(file), text).unwrap(); | ||
} | ||
| @@ -1031,6 +1032,14 @@ fn what_goes_wrong_is_reported() { | ||
.starts_with("bad name!.yml: "), | ||
"{invalid}" | ||
); | ||
// A second file named no workflow's says its own error beside it. | ||
assert!( | ||
invalid["error"] | ||
.as_str() | ||
.unwrap() | ||
.contains("\nworse name!.yml: a workflow's name is"), | ||
"{invalid}" | ||
); | ||
| ||
// A branch without the directory has no workflows to run. | ||
owner.git(&work, &["checkout", "-q", "--orphan", "bare"]); | ||
crates/knotless-serve/tests/forge.rs+6−0
| @@ -276,6 +276,12 @@ fn forks_pulls_reviews_and_merges() { | ||
fails(&owner, &bad); | ||
} | ||
ok(&owner, &["retarget", &url, &release, "--base", "stable"]); | ||
// One whose head cannot change, as it is not the owner's pull, moves | ||
// its base nowhere either: it still merges into stable below. | ||
let both = [ | ||
"retarget", &url, &release, "--base", "main", "--head", "two", | ||
]; | ||
fails(&owner, &both); | ||
fails(&contrib, &["retarget", &url, &clash, "--head", "nope"]); | ||
ok(&contrib, &["retarget", &url, &clash, "--head", "two"]); | ||
let issue = ok(&contrib, &["issue", &url, "Not a pull"]); | ||