A push to a landed proposal's bookmark says so; its page lists the changes since landing (closes #329); pin OxygenFlow main (oxygen's push_notes) 668b2ad7495c
- Author
- Andy McCall @andymccall <[email protected]>
- via
- agent
agent:claude@andymccall <[email protected]> · keyless, under your grant for changes,refs,seal · key c5631451921b
- When
- · 2026-10-27 01:33 UTC
- Signature
- ◈ sealed by agent:claude@andymccall
- Hash
668b2ad7495ce300ca09b4436b280dfd9a141febdf0f93ee420ac08c9f9b6fd5- Depends on
- 0b20f1ec4007 15d3f6f7871b 16626c0d69c0 19bb31ff1f5c 27babfa16580 380996db84e5 4f37afcc3583 7c06f5b28ade a7e6e8e6094c ac713df7b38b b5668108487f ba0a55bb5b3c d23464c0ac60 d7e2c2754807 de505df5cf8d
- Depended on by
- 951aed50c2f5 5caa65fd51e7
- In bookmarks
- main
- In tags
- v0.9.11 v0.9.15 v0.9.16 v0.9.19 v0.9.20 v0.9.21 v0.9.22 v0.9.23 v0.9.24 v0.9.7 v0.9.8 v0.9.9
- Edits
- 21 atoms, 0 tree operations, 7 files
Browse the tree at this change
modified CHANGELOG.md +1 −0
| … | 3 unchanged lines |
||
| 4 | 4 | ||
| 5 | 5 | ## Unreleased |
|
| 6 | 6 | ||
| 7 | + | - **A push to a landed proposal's bookmark says so (#329).** The changes reach no target until a new proposal takes them, and the push was silent: now it is answered `its proposal #N has landed in TARGET, and these changes are not on TARGET: open a new proposal with oh proposal create --from BOOKMARK` (oxygen's `Backend::push_notes`). The landed proposal's page lists them as changes since it landed, with a link to a new proposal; it no longer shows them as its own, and no longer offers to delete a bookmark that holds them. |
|
| 7 | 8 | - **Creating a repository no longer stalls the hub on a busy disk (#335).** A bare init makes about sixteen fsyncs, and on Earth's shared disk each waited behind every other write: one create took 26 s and held an async worker the whole time. Creates from the API, the web and drops now run on a blocking thread, as forks and bucket creates already did. |
|
| 8 | 9 | - **A Diffusion bump pins the upstream's default bookmark (#332).** A bump kept the bookmark name of the pin it replaced, so a subrepo first pinned at an upstream proposal's bookmark still named it after the proposal landed and Diffusion moved the pin to the default bookmark's state; a checkout then counted the subrepo as on another bookmark. A bump now names the upstream's default bookmark, unless it keeps pinned changes that bookmark lacks. |
|
| 9 | 10 | - **Listing landed proposals is quick, and pages (#331).** `oh proposal list --state landed` on a repository with a few hundred proposals timed out: the API worked out every proposal's projected state again, landed and closed ones too. Those now come from what was recorded, and the list takes `limit` (`oh proposal list --limit N`, 30 by default), newest first. |
|
| … | 353 unchanged lines |
modified crates/hub/src/db/proposals.rs +1 −1
| … | 3 unchanged lines |
||
| 4 | 4 | use super::{now_ms, Pool}; |
|
| 5 | 5 | use sqlx::FromRow; |
|
| 6 | 6 | ||
| 7 | - | #[derive(Clone, Debug, FromRow)] |
|
| 7 | + | #[derive(Clone, Debug, Default, FromRow)] |
|
| 8 | 8 | pub struct ProposalRow { |
|
| 9 | 9 | pub id: i64, |
|
| 10 | 10 | pub repo_id: i64, |
|
| … | 439 unchanged lines |
modified crates/hub/src/protocol.rs +79 −0
| … | 415 unchanged lines |
||
| 416 | 416 | } |
|
| 417 | 417 | } |
|
| 418 | 418 | ||
| 419 | + | /// A push that moved the bookmark of a proposal that has landed, with |
|
| 420 | + | /// none open from it now, says so beside the bookmark (#329): those |
|
| 421 | + | /// changes reach no target until a new proposal takes them. |
|
| 422 | + | fn push_notes(&self, name: &str, report: &PushReport, _headers: &HeaderMap) -> Vec<(String, String)> { |
|
| 423 | + | let Some(parsed) = RepoName::parse(name) else { return Vec::new() }; |
|
| 424 | + | if report.updated.is_empty() || crate::wiki::parent_of(&parsed).1 { |
|
| 425 | + | return Vec::new(); |
|
| 426 | + | } |
|
| 427 | + | let Ok(row) = self.row(&parsed) else { return Vec::new() }; |
|
| 428 | + | let Ok(proposals) = self.block(db::proposals::list(&self.pool, row.id, &[])) else { return Vec::new() }; |
|
| 429 | + | let moved: Vec<&str> = report.updated.iter().map(|u| u.name.as_str()).collect(); |
|
| 430 | + | landed_notes(&proposals, row.id, &moved) |
|
| 431 | + | } |
|
| 432 | + | ||
| 419 | 433 | fn after_push(&self, name: &str, report: &PushReport, headers: &HeaderMap) { |
|
| 420 | 434 | // The keys that signed what it brought were used (#286): linked |
|
| 421 | 435 | // ones show it under Settings › Keys. |
|
| … | 239 unchanged lines |
||
| 661 | 675 | enum Guard { |
|
| 662 | 676 | Read(#[allow(dead_code)] crate::repos::ReadGuard), |
|
| 663 | 677 | Write(#[allow(dead_code)] crate::repos::WriteGuard), |
|
| 678 | + | } |
|
| 679 | + | ||
| 680 | + | /// For each bookmark in `moved` whose latest proposal in this repository |
|
| 681 | + | /// (`repo_id`) has landed and none is open from it, a note with what to do |
|
| 682 | + | /// (#329). |
|
| 683 | + | fn landed_notes(proposals: &[db::proposals::ProposalRow], repo_id: i64, moved: &[&str]) -> Vec<(String, String)> { |
|
| 684 | + | let mut notes = Vec::new(); |
|
| 685 | + | for bookmark in moved { |
|
| 686 | + | let from: Vec<&db::proposals::ProposalRow> = proposals |
|
| 687 | + | .iter() |
|
| 688 | + | .filter(|p| p.source_kind == "bookmark" && p.source_repo_id.is_none_or(|id| id == repo_id) && p.source_bookmark == *bookmark) |
|
| 689 | + | .collect(); |
|
| 690 | + | if from.iter().any(|p| p.state == "open") { |
|
| 691 | + | continue; |
|
| 692 | + | } |
|
| 693 | + | if let Some(p) = from.iter().filter(|p| p.state == "landed").max_by_key(|p| p.landed_ms) { |
|
| 694 | + | notes.push(( |
|
| 695 | + | bookmark.to_string(), |
|
| 696 | + | format!( |
|
| 697 | + | "its proposal #{} has landed in {}, and these changes are not on {}: open a new proposal with `oh proposal create --from {bookmark}`", |
|
| 698 | + | p.number, p.target, p.target |
|
| 699 | + | ), |
|
| 700 | + | )); |
|
| 701 | + | } |
|
| 702 | + | } |
|
| 703 | + | notes |
|
| 704 | + | } |
|
| 705 | + | ||
| 706 | + | #[cfg(test)] |
|
| 707 | + | mod landed_tests { |
|
| 708 | + | use super::*; |
|
| 709 | + | ||
| 710 | + | fn proposal(number: i64, state: &str, source: &str, landed_ms: i64) -> db::proposals::ProposalRow { |
|
| 711 | + | db::proposals::ProposalRow { |
|
| 712 | + | id: number, |
|
| 713 | + | repo_id: 1, |
|
| 714 | + | number, |
|
| 715 | + | state: state.into(), |
|
| 716 | + | target: "develop".into(), |
|
| 717 | + | source_kind: "bookmark".into(), |
|
| 718 | + | source_bookmark: source.into(), |
|
| 719 | + | landed_ms, |
|
| 720 | + | ..Default::default() |
|
| 721 | + | } |
|
| 722 | + | } |
|
| 723 | + | ||
| 724 | + | #[test] |
|
| 725 | + | fn a_push_to_a_landed_proposals_bookmark_says_what_to_do() { |
|
| 726 | + | let ps = [proposal(3, "landed", "audio-irq", 10), proposal(5, "landed", "audio-irq", 20), proposal(7, "open", "camera", 0)]; |
|
| 727 | + | let notes = landed_notes(&ps, 1, &["audio-irq", "camera", "fresh"]); |
|
| 728 | + | assert_eq!(notes.len(), 1, "{notes:?}"); |
|
| 729 | + | assert_eq!(notes[0].0, "audio-irq"); |
|
| 730 | + | assert!( |
|
| 731 | + | notes[0].1.contains("#5 has landed in develop") && notes[0].1.contains("oh proposal create --from audio-irq"), |
|
| 732 | + | "{}", |
|
| 733 | + | notes[0].1 |
|
| 734 | + | ); |
|
| 735 | + | // An open proposal from it takes the changes: nothing to say. |
|
| 736 | + | let reopened = [proposal(3, "landed", "audio-irq", 10), proposal(9, "open", "audio-irq", 0)]; |
|
| 737 | + | assert!(landed_notes(&reopened, 1, &["audio-irq"]).is_empty()); |
|
| 738 | + | // A fork's proposal names another repository's bookmark. |
|
| 739 | + | let mut fork = proposal(4, "landed", "audio-irq", 10); |
|
| 740 | + | fork.source_repo_id = Some(2); |
|
| 741 | + | assert!(landed_notes(&[fork], 1, &["audio-irq"]).is_empty()); |
|
| 742 | + | } |
|
| 664 | 743 | } |
modified crates/hub/src/web/proposals.rs +39 −17
| … | 418 unchanged lines |
||
| 419 | 419 | /// whose source has changes, none of them missing from the target, was |
|
| 420 | 420 | /// landed by some other route. |
|
| 421 | 421 | pub source_has_changes: bool, |
|
| 422 | + | /// A landed proposal's bookmark's changes that are not on its target: |
|
| 423 | + | /// pushed after it landed, so no proposal carries them (#329). |
|
| 424 | + | pub since_landing: Vec<ChangeView>, |
|
| 422 | 425 | } |
|
| 423 | 426 | ||
| 424 | 427 | /// Compute the projection and everything derived from it. |
|
| … | 48 unchanged lines |
||
| 473 | 476 | owners: Vec::new(), |
|
| 474 | 477 | landed_already: row.state == "landed", |
|
| 475 | 478 | source_has_changes: !hashes.is_empty(), |
|
| 479 | + | since_landing: Vec::new(), |
|
| 476 | 480 | hashes, |
|
| 477 | 481 | row, |
|
| 478 | 482 | }) |
|
| … | 50 unchanged lines |
||
| 529 | 533 | let sealing = pl::sealing_keys(r, &p.hashes)?.into_iter().map(|k| k.to_hex()).collect(); |
|
| 530 | 534 | let landed_already = !p.hashes.is_empty() && p.hashes.iter().all(|h| target_loaded.state.contains(h)); |
|
| 531 | 535 | // A landed proposal's changes are in its target now: it shows |
|
| 532 | - | // the ones it landed, as recorded when it landed. |
|
| 533 | - | let (changes, hashes) = if r2.state == "landed" && p.hashes.is_empty() { |
|
| 534 | - | let hashes: Vec<ChangeHash> = r2.projected_changes().iter().filter_map(|h| ChangeHash::from_hex(h)).collect(); |
|
| 536 | + | // the ones it landed, as recorded when it landed (none when it |
|
| 537 | + | // landed by a push before any projection recorded them). |
|
| 538 | + | // Whatever its bookmark has that the target lacks cannot be |
|
| 539 | + | // part of what landed: it came after, and is the page's to point |
|
| 540 | + | // at, not the proposal's (#329). |
|
| 541 | + | let recorded: Vec<ChangeHash> = if r2.state == "landed" { |
|
| 542 | + | r2.projected_changes().iter().filter_map(|h| ChangeHash::from_hex(h)).collect() |
|
| 543 | + | } else { |
|
| 544 | + | Vec::new() |
|
| 545 | + | }; |
|
| 546 | + | let (changes, hashes, since_landing) = if r2.state == "landed" { |
|
| 535 | 547 | let labels = HashMap::new(); |
|
| 536 | - | let changes = hashes |
|
| 548 | + | let changes = recorded |
|
| 537 | 549 | .iter() |
|
| 538 | 550 | .filter_map(|h| Some(view::change_view_pub(r, h, &r.store().get_change_meta(h).ok()?, false, &labels))) |
|
| 539 | 551 | .collect(); |
|
| 540 | - | (changes, hashes) |
|
| 552 | + | let since: Vec<ChangeView> = p.changes.into_iter().filter(|c| !recorded.iter().any(|h| h.to_hex() == c.hash)).collect(); |
|
| 553 | + | (changes, recorded, since) |
|
| 541 | 554 | } else { |
|
| 542 | - | (p.changes, p.hashes) |
|
| 555 | + | (p.changes, p.hashes, Vec::new()) |
|
| 543 | 556 | }; |
|
| 544 | 557 | Ok(Loaded { |
|
| 545 | 558 | row: r2, |
|
| … | 15 unchanged lines |
||
| 561 | 574 | owners, |
|
| 562 | 575 | landed_already, |
|
| 563 | 576 | source_has_changes, |
|
| 577 | + | since_landing, |
|
| 564 | 578 | }) |
|
| 565 | 579 | }) |
|
| 566 | 580 | .await?; |
|
| … | 206 unchanged lines |
||
| 773 | 787 | /// The landed proposal's source bookmark, when it is this repository's, |
|
| 774 | 788 | /// still there, and the viewer may delete it (oxygenhub#86). |
|
| 775 | 789 | deletable_source: Option<String>, |
|
| 790 | + | /// Changes its bookmark gained after it landed, on no target (#329). |
|
| 791 | + | since_landing: Vec<ChangeView>, |
|
| 776 | 792 | can_land: bool, |
|
| 777 | 793 | can_land_over_checks: bool, |
|
| 778 | 794 | can_auto_land: bool, |
|
| … | 94 unchanged lines |
||
| 873 | 889 | super::attribute(app, &mut changes.iter_mut().collect::<Vec<_>>()).await; |
|
| 874 | 890 | let me = viewer.account(); |
|
| 875 | 891 | let can_edit = me.is_some_and(|a| Some(a.id) == p.author_id || role >= Role::Maintain); |
|
| 876 | - | let deletable_source = |
|
| 877 | - | if p.state == "landed" && p.source_kind == "bookmark" && role >= Role::Write && p.source_bookmark != row.default_bookmark { |
|
| 878 | - | let b = p.source_bookmark.clone(); |
|
| 879 | - | app.repos |
|
| 880 | - | .read(&repo, move |r, _| Ok(r.view()?.bookmarks.contains_key(&b))) |
|
| 881 | - | .await |
|
| 882 | - | .unwrap_or(false) |
|
| 883 | - | .then(|| p.source_bookmark.clone()) |
|
| 884 | - | } else { |
|
| 885 | - | None |
|
| 886 | - | }; |
|
| 892 | + | // Not while it holds work that came after (#329). |
|
| 893 | + | let deletable_source = if p.state == "landed" |
|
| 894 | + | && p.source_kind == "bookmark" |
|
| 895 | + | && role >= Role::Write |
|
| 896 | + | && p.source_bookmark != row.default_bookmark |
|
| 897 | + | && loaded.since_landing.is_empty() |
|
| 898 | + | { |
|
| 899 | + | let b = p.source_bookmark.clone(); |
|
| 900 | + | app.repos |
|
| 901 | + | .read(&repo, move |r, _| Ok(r.view()?.bookmarks.contains_key(&b))) |
|
| 902 | + | .await |
|
| 903 | + | .unwrap_or(false) |
|
| 904 | + | .then(|| p.source_bookmark.clone()) |
|
| 905 | + | } else { |
|
| 906 | + | None |
|
| 907 | + | }; |
|
| 887 | 908 | let can_land = role >= Role::Write && p.is_open() && v.blockers.is_empty(); |
|
| 888 | 909 | // Not landable yet: anyone who may land can set it to land when its |
|
| 889 | 910 | // checks pass (#261). |
|
| … | 71 unchanged lines |
||
| 961 | 982 | ignore_ws: false, |
|
| 962 | 983 | conflicts: loaded.conflicts, |
|
| 963 | 984 | conflicted: loaded.conflicted, |
|
| 985 | + | since_landing: loaded.since_landing, |
|
| 964 | 986 | comments, |
|
| 965 | 987 | placed, |
|
| 966 | 988 | outdated, |
|
| … | 1168 unchanged lines |
modified crates/hub/tests/proposals.rs +25 −0
| … | 638 unchanged lines |
||
| 639 | 639 | p = db::proposals::get(&app.db, 1, 1).await.unwrap().unwrap(); |
|
| 640 | 640 | } |
|
| 641 | 641 | assert_eq!((p.state.as_str(), p.landed_via.as_str()), ("landed", "push"), "{p:?}"); |
|
| 642 | + | ||
| 643 | + | // #329: more work pushed to the landed proposal's bookmark is told |
|
| 644 | + | // that it reaches main only through a new proposal. |
|
| 645 | + | client.bookmark_switch("feature", false).unwrap(); |
|
| 646 | + | write(work.path(), "a.txt", b"one\ntwo\nthree\n"); |
|
| 647 | + | client.snapshot("t").unwrap(); |
|
| 648 | + | client.commit("add three", &[]).unwrap(); |
|
| 649 | + | let results = client.push_bookmark("origin", "feature", false).unwrap(); |
|
| 650 | + | let feature = results.iter().find(|r| r.name == "feature").expect("the feature bookmark's result"); |
|
| 651 | + | assert!(feature.ok, "{results:?}"); |
|
| 652 | + | assert!( |
|
| 653 | + | feature.reason.contains("proposal #1 has landed in main") && feature.reason.contains("oh proposal create --from feature"), |
|
| 654 | + | "{results:?}" |
|
| 655 | + | ); |
|
| 656 | + | // The landed proposal's page still lists what it landed, and points at |
|
| 657 | + | // the change that came after. |
|
| 658 | + | let page = get(app, "/andy/demo/proposal/1", Some(&andy)).await; |
|
| 659 | + | assert!(page.body.contains("has 1 change since this landed"), "{}", page.body); |
|
| 660 | + | assert!(page.body.contains("add three") && page.body.contains("proposals/new?source=feature&target=main"), "{}", page.body); |
|
| 661 | + | let at = page.body.find("Proposal landed").expect("the landed outcome"); |
|
| 662 | + | let outcome = &page.body[at..at + 600]; |
|
| 663 | + | // It landed by a push before any projection recorded its changes, so it |
|
| 664 | + | // claims none of the new one as its own, and its bookmark is not |
|
| 665 | + | // offered for deletion while it holds work. |
|
| 666 | + | assert!(!outcome.contains("1 change into") && !outcome.contains("can be safely deleted"), "{outcome}"); |
|
| 642 | 667 | } |
|
| 643 | 668 | ||
| 644 | 669 | /// #140: a proposal is closed and reopened through the API, as the page's |
|
| … | 495 unchanged lines |
replaced deps/oxygenflow +0 −0
No line changes.
modified templates/proposal.html +2 −1
| … | 20 unchanged lines |
||
| 21 | 21 | {% if let Some(e) = error %}<p class="error">{{ e }}</p>{% endif %} |
|
| 22 | 22 | {% if conflicted %}<div class="strip bad">Landing would carry {{ conflicts.len()|plural("conflict", "conflicts") }}. Any collaborator can push a resolution change to the source; the proposal follows it. <a href="?tab=files#conflicts">See them</a></div>{% endif %} |
|
| 23 | 23 | {% if landed_already && p.state != "landed" %}<div class="strip warn">Every change is already in {{ p.target }}.</div>{% endif %} |
|
| 24 | + | {% if p.state == "landed" && !since_landing.is_empty() %}<div class="strip warn"><code>{{ p.source_bookmark }}</code> has {{ since_landing.len()|plural("change", "changes") }} since this landed, not on <code>{{ p.target }}</code>: {% for c in since_landing %}<a href="{{ repo.url }}/change/{{ c.hash }}"><code>{{ c.hash|short }}</code></a> {{ c.title }}{% if !loop.last %}, {% endif %}{% endfor %}. <a href="{{ repo.url }}/proposals/new?source={{ p.source_bookmark }}&target={{ p.target }}">Open a new proposal</a> to take {% if since_landing.len() == 1 %}it{% else %}them{% endif %} there.</div>{% endif %} |
|
| 24 | 25 | <div class="bar"> |
|
| 25 | 26 | <span class="modes"> |
|
| 26 | 27 | <a href="?tab=conversation"{% if tab == "conversation" %} class="on"{% endif %}>conversation ({{ comments.len() + reviews.len() }})</a> |
|
| … | 100 unchanged lines |
||
| 127 | 128 | <span class="tl-badge" aria-hidden="true"><svg viewBox="0 0 16 16" width="18" height="18"><path fill="currentColor" d="M5.45 5.15A3.5 3.5 0 0 0 8.75 7.5h1.94a2.25 2.25 0 1 1 0 1.5H8.75a5 5 0 0 1-3.5-1.43v3.04a2.25 2.25 0 1 1-1.5 0V5.39a2.25 2.25 0 1 1 1.7-.24ZM4.5 2.75a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm8.25-4.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/></svg></span> |
|
| 128 | 129 | <div class="tl-card outcome-card"> |
|
| 129 | 130 | <div><strong>Proposal landed</strong> |
|
| 130 | - | <p class="small">{% if !p.landed_by.is_empty() %}<a href="/{{ p.landed_by }}">{{ p.landed_by }}</a> landed{% else %}Landed{% endif %} {{ changes.len()|plural("change", "changes") }} into <a class="lbl lbl-bm" href="{{ repo.url }}/tree/{{ p.target }}">{{ p.target }}</a>{% if p.landed_ms > 0 %} <time title="{{ p.landed_ms|when_i }}">{{ p.landed_ms|ago_i }}</time>{% endif %}.{% if let Some(b) = deletable_source %} The <code>{{ b }}</code> bookmark can be safely deleted.{% endif %}</p></div> |
|
| 131 | + | <p class="small">{% if !p.landed_by.is_empty() %}<a href="/{{ p.landed_by }}">{{ p.landed_by }}</a> landed{% else %}Landed{% endif %}{% if !changes.is_empty() %} {{ changes.len()|plural("change", "changes") }}{% endif %} into <a class="lbl lbl-bm" href="{{ repo.url }}/tree/{{ p.target }}">{{ p.target }}</a>{% if p.landed_ms > 0 %} <time title="{{ p.landed_ms|when_i }}">{{ p.landed_ms|ago_i }}</time>{% endif %}.{% if let Some(b) = deletable_source %} The <code>{{ b }}</code> bookmark can be safely deleted.{% endif %}</p></div> |
|
| 131 | 132 | {% if let Some(b) = deletable_source %}<form method="post" action="{{ repo.url }}/bookmarks/delete"><input type="hidden" name="_csrf" value="{{ shell.csrf }}"><input type="hidden" name="bookmark" value="{{ b }}"><button type="submit" data-busy="Deleting">Delete bookmark</button></form>{% endif %} |
|
| 132 | 133 | </div> |
|
| 133 | 134 | </div> |
|
| … | 132 unchanged lines |