refactor
This commit is contained in:
Generated
+10
@@ -1690,6 +1690,15 @@ dependencies = [
|
||||
"syn 2.0.119",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "diffy"
|
||||
version = "0.5.2"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "3e3dc2f773b6aaa63b1a7684b8589f670a8a0146a510b74d23a401c882364b49"
|
||||
dependencies = [
|
||||
"hashbrown 0.17.1",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "digest"
|
||||
version = "0.10.7"
|
||||
@@ -7958,6 +7967,7 @@ name = "signed_git"
|
||||
version = "0.1.0-alpha"
|
||||
dependencies = [
|
||||
"anyhow",
|
||||
"diffy",
|
||||
"gix",
|
||||
"gix-worktree",
|
||||
"gix-worktree-state",
|
||||
|
||||
@@ -12,6 +12,7 @@ gix = { workspace = true, features = ["revision", "blob-diff"] }
|
||||
gix-worktree = "0.56"
|
||||
gix-worktree-state = "0.34"
|
||||
anyhow.workspace = true
|
||||
diffy = "0.5"
|
||||
|
||||
[dev-dependencies]
|
||||
tempfile = "3"
|
||||
|
||||
+134
-282
@@ -4,6 +4,8 @@ use std::path::{Path, PathBuf};
|
||||
use std::process::{Command, Stdio};
|
||||
|
||||
use anyhow::{Context, Result, bail};
|
||||
use diffy::patch_set::{FileOperation, FilePatch, ParseOptions, PatchSet};
|
||||
use diffy::{Hunk, Line};
|
||||
use gix::diff::blob::unified_diff::{ConsumeHunk, DiffLineKind as GixLineKind, HunkHeader};
|
||||
use gix::interrupt::IS_INTERRUPTED;
|
||||
use gix::progress::Discard;
|
||||
@@ -1596,24 +1598,145 @@ fn tree_diff(
|
||||
}
|
||||
|
||||
/// Parse `git format-patch` output, a single patch or a series.
|
||||
///
|
||||
/// Backed by [`diffy::patch_set`], which implements git's extended diff format:
|
||||
/// `diff --git` headers, rename and copy detection, binary detection, and
|
||||
/// C-style quoted or octal-escaped paths.
|
||||
pub fn patch_diffs(patch: &str) -> Result<CommitDiff> {
|
||||
let lines: Vec<&str> = patch.lines().collect();
|
||||
let mut files = Vec::new();
|
||||
let mut i = 0;
|
||||
// `PatchSet` reports an error when the input holds no patch at all,
|
||||
// while a patch without git diff sections is simply empty here.
|
||||
if !patch.lines().any(|line| line.starts_with("diff --git ")) {
|
||||
return Ok(CommitDiff { files: Vec::new() });
|
||||
}
|
||||
|
||||
while i < lines.len() {
|
||||
let Some(header) = lines[i].strip_prefix("diff --git ") else {
|
||||
i += 1;
|
||||
continue;
|
||||
};
|
||||
let (file, next) = parse_diff_section(header, &lines, i + 1)?;
|
||||
files.push(file);
|
||||
i = next;
|
||||
let mut files = Vec::new();
|
||||
|
||||
for file in PatchSet::parse(patch, ParseOptions::gitdiff()) {
|
||||
files.push(file_diff(file?)?);
|
||||
}
|
||||
|
||||
Ok(CommitDiff { files })
|
||||
}
|
||||
|
||||
/// The [`FileDiff`] of one parsed file patch.
|
||||
fn file_diff(file: FilePatch<'_, str>) -> Result<FileDiff> {
|
||||
// The `---`/`+++` paths carry the `a/`/`b/` prefix, so the first path
|
||||
// component is dropped, the same way `git apply -p1` does.
|
||||
// Rename and copy paths come from their own headers, unprefixed.
|
||||
let stripped;
|
||||
let operation = match file.operation() {
|
||||
operation @ (FileOperation::Rename { .. } | FileOperation::Copy { .. }) => operation,
|
||||
operation => {
|
||||
stripped = operation.strip_prefix(1);
|
||||
&stripped
|
||||
}
|
||||
};
|
||||
|
||||
let (path, old_path, status) = match operation {
|
||||
FileOperation::Create(path) => (path.as_ref(), None, DiffStatus::Added),
|
||||
FileOperation::Delete(path) => (path.as_ref(), None, DiffStatus::Deleted),
|
||||
FileOperation::Modify { modified, .. } => (modified.as_ref(), None, DiffStatus::Modified),
|
||||
FileOperation::Rename { from, to } => {
|
||||
(to.as_ref(), Some(from.as_ref()), DiffStatus::Renamed)
|
||||
}
|
||||
FileOperation::Copy { from, to } => (to.as_ref(), Some(from.as_ref()), DiffStatus::Copied),
|
||||
};
|
||||
|
||||
let mut insertions = 0usize;
|
||||
let mut deletions = 0usize;
|
||||
let mut hunks = Vec::new();
|
||||
let patch = file.patch();
|
||||
if let Some(text) = patch.as_text() {
|
||||
for hunk in text.hunks() {
|
||||
let hunk = hunk_diff(hunk);
|
||||
insertions += hunk
|
||||
.lines
|
||||
.iter()
|
||||
.filter(|line| line.kind == DiffLineKind::Addition)
|
||||
.count();
|
||||
deletions += hunk
|
||||
.lines
|
||||
.iter()
|
||||
.filter(|line| line.kind == DiffLineKind::Deletion)
|
||||
.count();
|
||||
hunks.push(hunk);
|
||||
}
|
||||
}
|
||||
|
||||
Ok(FileDiff {
|
||||
path: path.to_owned(),
|
||||
old_path: old_path.map(str::to_owned),
|
||||
status,
|
||||
insertions,
|
||||
deletions,
|
||||
binary: patch.is_binary(),
|
||||
hunks,
|
||||
})
|
||||
}
|
||||
|
||||
/// The [`DiffHunk`] of one parsed hunk, including the line number of every line.
|
||||
///
|
||||
/// `diffy` reports only the hunk header ranges. The per-line numbers are
|
||||
/// counted from them the way the header encodes them: context lines advance
|
||||
/// both sides, deletions only the old, insertions only the new.
|
||||
fn hunk_diff(hunk: &Hunk<'_, str>) -> DiffHunk {
|
||||
let old_range = hunk.old_range();
|
||||
let new_range = hunk.new_range();
|
||||
let mut old = old_range.start() as u32;
|
||||
let mut new = new_range.start() as u32;
|
||||
|
||||
let mut lines = Vec::with_capacity(hunk.lines().len());
|
||||
for line in hunk.lines() {
|
||||
let (kind, text) = match line {
|
||||
Line::Context(text) => (DiffLineKind::Context, *text),
|
||||
Line::Delete(text) => (DiffLineKind::Deletion, *text),
|
||||
Line::Insert(text) => (DiffLineKind::Addition, *text),
|
||||
};
|
||||
|
||||
let (old_no, new_no) = match kind {
|
||||
DiffLineKind::Context => {
|
||||
let numbers = (Some(old), Some(new));
|
||||
old += 1;
|
||||
new += 1;
|
||||
numbers
|
||||
}
|
||||
DiffLineKind::Addition => {
|
||||
let number = Some(new);
|
||||
new += 1;
|
||||
(None, number)
|
||||
}
|
||||
DiffLineKind::Deletion => {
|
||||
let number = Some(old);
|
||||
old += 1;
|
||||
(number, None)
|
||||
}
|
||||
};
|
||||
|
||||
lines.push(DiffLine {
|
||||
kind,
|
||||
old: old_no,
|
||||
new: new_no,
|
||||
text: line_text(text),
|
||||
});
|
||||
}
|
||||
|
||||
DiffHunk {
|
||||
old_start: old_range.start() as u32,
|
||||
old_lines: old_range.len() as u32,
|
||||
new_start: new_range.start() as u32,
|
||||
new_lines: new_range.len() as u32,
|
||||
lines,
|
||||
}
|
||||
}
|
||||
|
||||
/// The content of a parsed line without its line ending.
|
||||
///
|
||||
/// `diffy` keeps the trailing `\n`, the way `str::lines` splits it off.
|
||||
fn line_text(text: &str) -> String {
|
||||
let text = text.strip_suffix('\n').unwrap_or(text);
|
||||
text.strip_suffix('\r').unwrap_or(text).to_owned()
|
||||
}
|
||||
|
||||
/// Commits of a `git format-patch` output, a single patch or a series.
|
||||
///
|
||||
/// Entries appear in patch order, oldest first as `git format-patch` produces them.
|
||||
@@ -1695,277 +1818,6 @@ fn strip_patch_prefix(subject: &str) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
/// Parse one file's diff section.
|
||||
///
|
||||
/// Returns the section and the index of the first unconsumed line.
|
||||
fn parse_diff_section(header: &str, lines: &[&str], start: usize) -> Result<(FileDiff, usize)> {
|
||||
let (header_old, header_new) = header_paths(header)?;
|
||||
// The `---` and `+++` lines name the two sides unambiguously.
|
||||
// The `diff --git` header cannot distinguish spaces in paths.
|
||||
// Fall back to the header for sections without them, pure renames and mode changes.
|
||||
let mut old_path = header_old;
|
||||
let mut new_path = header_new;
|
||||
|
||||
let mut status = DiffStatus::Modified;
|
||||
let mut binary = false;
|
||||
let mut hunks = Vec::new();
|
||||
let mut insertions = 0usize;
|
||||
let mut deletions = 0usize;
|
||||
let mut i = start;
|
||||
|
||||
while i < lines.len() {
|
||||
let line = lines[i];
|
||||
|
||||
// The next file's section starts at this line.
|
||||
if line.starts_with("diff --git ") {
|
||||
break;
|
||||
}
|
||||
i += 1;
|
||||
|
||||
if line.starts_with("@@ -") {
|
||||
let (hunk, next) = parse_hunk(lines, i - 1)?;
|
||||
i = next;
|
||||
insertions += hunk
|
||||
.lines
|
||||
.iter()
|
||||
.filter(|line| line.kind == DiffLineKind::Addition)
|
||||
.count();
|
||||
deletions += hunk
|
||||
.lines
|
||||
.iter()
|
||||
.filter(|line| line.kind == DiffLineKind::Deletion)
|
||||
.count();
|
||||
hunks.push(hunk);
|
||||
} else if let Some(rest) = line.strip_prefix("--- ") {
|
||||
if rest == "/dev/null" {
|
||||
status = DiffStatus::Added;
|
||||
} else {
|
||||
old_path = diff_line_path(rest, "a/")?;
|
||||
}
|
||||
} else if let Some(rest) = line.strip_prefix("+++ ") {
|
||||
if rest == "/dev/null" {
|
||||
status = DiffStatus::Deleted;
|
||||
} else {
|
||||
new_path = diff_line_path(rest, "b/")?;
|
||||
}
|
||||
} else if line.starts_with("new file mode ") {
|
||||
status = DiffStatus::Added;
|
||||
} else if line.starts_with("deleted file mode ") {
|
||||
status = DiffStatus::Deleted;
|
||||
} else if line.starts_with("copy from ") {
|
||||
status = DiffStatus::Copied;
|
||||
} else if line.starts_with("rename from ") {
|
||||
status = DiffStatus::Renamed;
|
||||
} else if line.starts_with("Binary files ") || line.starts_with("GIT binary patch") {
|
||||
binary = true;
|
||||
// A literal binary patch may follow.
|
||||
// Skip it without consuming the next section's header.
|
||||
while i < lines.len() && !lines[i].starts_with("diff --git ") {
|
||||
i += 1;
|
||||
}
|
||||
break;
|
||||
}
|
||||
// Everything else, index, mode and similarity lines, is ignored.
|
||||
}
|
||||
|
||||
Ok((
|
||||
FileDiff {
|
||||
path: new_path,
|
||||
old_path: matches!(status, DiffStatus::Renamed | DiffStatus::Copied)
|
||||
.then_some(old_path),
|
||||
status,
|
||||
insertions,
|
||||
deletions,
|
||||
binary,
|
||||
hunks,
|
||||
},
|
||||
i,
|
||||
))
|
||||
}
|
||||
|
||||
/// Parse one hunk, the `@@ -a,b +c,d @@` header plus every body line.
|
||||
///
|
||||
/// Returns the hunk and the index of the first unconsumed line.
|
||||
fn parse_hunk(lines: &[&str], start: usize) -> Result<(DiffHunk, usize)> {
|
||||
let (old_start, old_lines, new_start, new_lines) = hunk_header(lines[start])?;
|
||||
|
||||
let mut diff_lines = Vec::new();
|
||||
let mut old = old_start;
|
||||
let mut new = new_start;
|
||||
let mut i = start + 1;
|
||||
|
||||
while i < lines.len() {
|
||||
let line = lines[i];
|
||||
let Some(kind) = line_prefix_kind(line) else {
|
||||
break;
|
||||
};
|
||||
i += 1;
|
||||
|
||||
// Context lines advance both counters.
|
||||
// Deletions advance only the old counter, additions only the new one.
|
||||
// Every line then carries its real number in both versions.
|
||||
let (old_no, new_no) = match kind {
|
||||
DiffLineKind::Context => {
|
||||
let numbers = (Some(old), Some(new));
|
||||
old += 1;
|
||||
new += 1;
|
||||
numbers
|
||||
}
|
||||
DiffLineKind::Addition => {
|
||||
let number = Some(new);
|
||||
new += 1;
|
||||
(None, number)
|
||||
}
|
||||
DiffLineKind::Deletion => {
|
||||
let number = Some(old);
|
||||
old += 1;
|
||||
(number, None)
|
||||
}
|
||||
};
|
||||
diff_lines.push(DiffLine {
|
||||
kind,
|
||||
old: old_no,
|
||||
new: new_no,
|
||||
text: line[1..].to_owned(),
|
||||
});
|
||||
}
|
||||
|
||||
Ok((
|
||||
DiffHunk {
|
||||
old_start,
|
||||
old_lines,
|
||||
new_start,
|
||||
new_lines,
|
||||
lines: diff_lines,
|
||||
},
|
||||
i,
|
||||
))
|
||||
}
|
||||
|
||||
/// The kind of a hunk body line, from its first character.
|
||||
///
|
||||
/// Lines outside a hunk, headers, `\ No newline...` and the next section, yield `None`.
|
||||
fn line_prefix_kind(line: &str) -> Option<DiffLineKind> {
|
||||
match line.as_bytes().first()? {
|
||||
b' ' => Some(DiffLineKind::Context),
|
||||
b'+' => Some(DiffLineKind::Addition),
|
||||
b'-' => Some(DiffLineKind::Deletion),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Parse a unified-diff hunk header, `@@ -a,b +c,d @@`.
|
||||
///
|
||||
/// Omitted line counts default to 1.
|
||||
fn hunk_header(header: &str) -> Result<(u32, u32, u32, u32)> {
|
||||
let rest = header
|
||||
.strip_prefix("@@ ")
|
||||
.context("malformed hunk header")?;
|
||||
let (old_spec, rest) = rest.split_once(' ').context("malformed hunk header")?;
|
||||
let new_spec = rest.split_once(' ').map(|(new, _)| new).unwrap_or(rest);
|
||||
|
||||
fn parse(spec: &str) -> Result<(u32, u32)> {
|
||||
let spec = spec.strip_prefix(['-', '+']).unwrap_or(spec);
|
||||
let (start, count) = match spec.split_once(',') {
|
||||
Some((start, count)) => (start, count.parse::<u32>()?),
|
||||
None => (spec, 1),
|
||||
};
|
||||
Ok((start.parse::<u32>()?, count))
|
||||
}
|
||||
|
||||
let (old_start, old_lines) = parse(old_spec)?;
|
||||
let (new_start, new_lines) = parse(new_spec)?;
|
||||
Ok((old_start, old_lines, new_start, new_lines))
|
||||
}
|
||||
|
||||
/// The old and new paths of a `diff --git a/X b/Y` header.
|
||||
fn header_paths(header: &str) -> Result<(String, String)> {
|
||||
if header.starts_with('"') {
|
||||
// Quoted paths include the `a/` / `b/` prefix inside the quotes.
|
||||
let (old, rest) = take_quoted(header).context("unterminated quoted path")?;
|
||||
let rest = rest.trim_start();
|
||||
let new = if rest.starts_with('"') {
|
||||
take_quoted(rest).context("unterminated quoted path")?.0
|
||||
} else {
|
||||
rest.split_whitespace().next().unwrap_or(rest)
|
||||
};
|
||||
let old = old
|
||||
.strip_prefix("a/")
|
||||
.context("old path without `a/` prefix")?;
|
||||
let new = new
|
||||
.strip_prefix("b/")
|
||||
.context("new path without `b/` prefix")?;
|
||||
Ok((unquote_path(old)?, unquote_path(new)?))
|
||||
} else {
|
||||
let (old, rest) = header
|
||||
.rsplit_once(" b/")
|
||||
.context("malformed diff --git header")?;
|
||||
let old = old
|
||||
.strip_prefix("a/")
|
||||
.context("old path without `a/` prefix")?;
|
||||
Ok((old.to_owned(), rest.to_owned()))
|
||||
}
|
||||
}
|
||||
|
||||
/// The path of a `--- a/X` or `+++ b/Y` line.
|
||||
fn diff_line_path(line: &str, prefix: &str) -> Result<String> {
|
||||
let line = line.trim_end_matches('\t');
|
||||
if line.starts_with('"') {
|
||||
let (path, _) = take_quoted(line).context("unterminated quoted path")?;
|
||||
let path = path
|
||||
.strip_prefix(prefix)
|
||||
.context("diff line path without `a/` or `b/` prefix")?;
|
||||
unquote_path(path)
|
||||
} else {
|
||||
Ok(line
|
||||
.strip_prefix(prefix)
|
||||
.context("diff line path without `a/` or `b/` prefix")?
|
||||
.to_owned())
|
||||
}
|
||||
}
|
||||
|
||||
/// The content of a git C-style quoted path and the rest of the input.
|
||||
/// The path spans the opening `"`, escaped content and closing `"`.
|
||||
///
|
||||
/// `None` if unterminated.
|
||||
fn take_quoted(input: &str) -> Option<(&str, &str)> {
|
||||
let mut end = 1; // byte after the opening quote
|
||||
let mut rest = &input[1..];
|
||||
while let Some(ch) = rest.chars().next() {
|
||||
let len = ch.len_utf8();
|
||||
match ch {
|
||||
'\\' => {
|
||||
// Consume the escaped character too, it may be multi-byte.
|
||||
let escaped = rest[len..].chars().next()?;
|
||||
let consumed = len + escaped.len_utf8();
|
||||
end += consumed;
|
||||
rest = &rest[consumed..];
|
||||
}
|
||||
'"' => return Some((&input[1..end], &input[end + len..])),
|
||||
_ => {
|
||||
end += len;
|
||||
rest = &rest[len..];
|
||||
}
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
/// Undo git's C-style path quoting, `\NNN` octal escapes, `\"` and `\\`.
|
||||
///
|
||||
/// Delegates to gitoxide's C-style quote implementation, `gix::quote::ansi_c::undo`.
|
||||
/// It expects the surrounding double quotes, which are re-added around the interior.
|
||||
fn unquote_path(path: &str) -> Result<String> {
|
||||
if !path.contains('\\') {
|
||||
return Ok(path.to_owned());
|
||||
}
|
||||
|
||||
let quoted = format!("\"{path}\"");
|
||||
let (unquoted, _) = gix::quote::ansi_c::undo(gix::bstr::BStr::new(quoted.as_bytes()))
|
||||
.map_err(|e| anyhow::anyhow!("malformed quoted path: {e}"))?;
|
||||
String::from_utf8(unquoted.into_owned().to_vec()).context("invalid UTF-8 in quoted path")
|
||||
}
|
||||
|
||||
/// Collects the hunks of one blob diff while tracking per-line numbers.
|
||||
struct HunkCollector<'a> {
|
||||
hunks: &'a mut Vec<DiffHunk>,
|
||||
|
||||
@@ -203,7 +203,10 @@ impl RepoListStore {
|
||||
|
||||
announcement || deletion
|
||||
}
|
||||
BackendEvent::Synced | BackendEvent::SyncProgress { .. } => true,
|
||||
// Only a completed sync refreshes the list.
|
||||
// Progress ticks would re-scan the whole database several times
|
||||
// per sync to reveal entries incrementally.
|
||||
BackendEvent::Synced => true,
|
||||
_ => false,
|
||||
};
|
||||
|
||||
|
||||
+155
-366
@@ -1,4 +1,4 @@
|
||||
# Backend re-architecture: findings and plan
|
||||
# Backend re-architecture: findings and outcome
|
||||
|
||||
This is a follow-up to an initial architecture review. It re-checks every claim
|
||||
against the **actual `nostr`/`nostr-sdk` source pinned by `Cargo.lock`**
|
||||
@@ -14,6 +14,12 @@ Scope: `crates/signed_nostr`, `crates/signed_state`, `crates/signed_core`,
|
||||
`crates/signed_git`, and `crates/workspace` (the actual call sites of the
|
||||
backend, audited for business-logic flaws and redundant conversions).
|
||||
|
||||
**Status: complete.** All 14 items of the plan are implemented and verified;
|
||||
the compact record is the **Outcome** section at the end. Sections 1–17 are
|
||||
kept as the analysis each change was based on — they describe the code as it
|
||||
was *before* the change, so read them as rationale, not as current
|
||||
documentation.
|
||||
|
||||
## Summary of the ask
|
||||
|
||||
1. Never call `fetch_events`. Bootstrap only via `subscribe`/`sync` (negentropy), read from `client.database()`.
|
||||
@@ -30,17 +36,6 @@ Each is addressed below with concrete file:line references and a verified replac
|
||||
|
||||
## 1. `fetch_events` — one call site, and it should go too
|
||||
|
||||
> **Status: done.** `bootstrap_user` now calls `sync_bootstrap_only` (the
|
||||
> same helper `Backend::sync_bootstrap` already used) against
|
||||
> `filters::grasp_list(public_key)`, then reads the result back out through
|
||||
> the existing `user_grasp_list_servers` query helper instead of hand-rolling
|
||||
> a second `BTreeSet<Event>` → `Vec<Event>` collect. `client.add_relay(url)`
|
||||
> no longer round-trips through `.as_str()`, and connects with
|
||||
> `.and_connect()` (see §8) instead of a trailing pool-wide `client.connect()`.
|
||||
> `grep -rn "fetch_events" crates/` now returns nothing in the whole
|
||||
> workspace. `cargo check --workspace`, `cargo clippy -p signed_state`, and
|
||||
> `cargo test -p signed_state` (24 tests, unchanged) all pass.
|
||||
|
||||
```
|
||||
grep -rn "fetch_events" crates/
|
||||
crates/signed_state/src/backend.rs:1065
|
||||
@@ -122,41 +117,6 @@ everywhere, no exceptions.
|
||||
|
||||
## 2. The "send an event" functions — there are 8, there should be roughly 2
|
||||
|
||||
> **Status: done.** `Backend::send`, `Backend::publish_event`, `Backend::publish_task`,
|
||||
> `Backend::send_fire_and_forget` and the free fn `broadcast_event` are all
|
||||
> deleted. In their place: `require_relay_accepted(output, event)` (the one
|
||||
> "empty success ⇒ Err" check, `pub(crate)` so `repo.rs` can use it too) and
|
||||
> `Backend::announce_published(event, cx)` (one line, emits
|
||||
> `BackendEvent::Published`). Every call site now calls
|
||||
> `client.send_event(&event).broadcast()` directly — the `.broadcast()` is
|
||||
> the additive gossip-bypass from §4, added here since every one of these
|
||||
> call sites already has a well-defined target (the relays this app
|
||||
> explicitly added). `RepoStore::send` is also gone; its 4 identical
|
||||
> one-shot callers (`open_issue`, `reply`, `set_status`,
|
||||
> `publish_applied_status`) now call a private `RepoStore::publish` that
|
||||
> does the same sign+send+check+`last_error` bookkeeping — kept as **one**
|
||||
> small store-local helper rather than inlining the same ~20 lines 4 times,
|
||||
> since all 4 call sites have byte-for-byte identical post-conditions (this
|
||||
> is a deliberate, narrow exception to "delete `RepoStore::send`";
|
||||
> `stage_event_on_relay` was already the same shape of exception before this
|
||||
> change). The 3 call sites with genuinely divergent control flow
|
||||
> (`open_pull_request`, `update_pull_request`, `publish_patch_series`) now
|
||||
> call `client.send_event(...)`/`require_relay_accepted` directly inline,
|
||||
> fixing the inconsistent error surfacing this section originally flagged
|
||||
> (all three now set `last_error` on failure, like every other `RepoStore`
|
||||
> mutation). `retract_events` is rewritten per the NIP-09 section below.
|
||||
> `stage_event_on_relay` is untouched, it already followed this pattern.
|
||||
> Verified against the pinned `nostr-sdk` source that `SendEventOutput`
|
||||
> (`= Output<EventId, EventSendStatus, String>`) and `EventDeletionRequest`
|
||||
> (`nostr/src/nips/nip09.rs`) have the shapes assumed here, and that
|
||||
> `UniversalSigner` implements the `AsyncGetPublicKey + AsyncSignEvent`
|
||||
> bounds `FinalizeEventAsync` requires. `cargo check --workspace`,
|
||||
> `cargo clippy --workspace --all-targets` and `cargo test --workspace` all
|
||||
> pass unchanged (165+ tests, no failures) — none of the deleted/rewritten
|
||||
> functions had direct unit test coverage (they all require a live relay),
|
||||
> so this was verified by compilation plus a careful line-by-line diff
|
||||
> against the previous control flow for each of the 8 call sites.
|
||||
|
||||
Grep for anything that ends up calling `client.send_event`:
|
||||
|
||||
| Function | File:line | What it adds over `client.send_event` |
|
||||
@@ -361,13 +321,6 @@ invariant, not something to paper over with a fingerprint cache.
|
||||
|
||||
## 4. Gossip is enabled, and stays enabled — but today's git-domain sends should bypass it explicitly
|
||||
|
||||
> **Status: done**, implemented as part of §2's send-path consolidation.
|
||||
> Every direct `client.send_event(...)` call added while deleting the 8
|
||||
> send-path layers uses `.broadcast()` explicitly (repository announcements,
|
||||
> state, issues, PRs, patches, comments, statuses and NIP-09 deletions).
|
||||
> `.gossip(...)` stays configured in `signed_nostr::backend::with_database`
|
||||
> for future NIP-17/NIP-65 features, per the recommendation below.
|
||||
|
||||
Per team direction: gossip is a deliberate, load-bearing choice for this
|
||||
client (it's not fully wired up to a feature yet, but it's not incidental
|
||||
configuration either). `.gossip(...)` stays in `signed_nostr::backend::with_database`
|
||||
@@ -489,22 +442,52 @@ path handling and quoted-path unescaping need to be checked against this
|
||||
project's test fixtures (`signed_git/src/lib.rs:3917-3962`,
|
||||
octal-escaped/non-ASCII quoted paths) before committing to the swap.
|
||||
|
||||
**Outcome of the spike: the swap is sound, and was committed.** Both specific
|
||||
risks flagged above checked out:
|
||||
|
||||
- **Renamed paths.** `diffy` produces `FileOperation::Rename { from, to }` from
|
||||
the `rename from`/`rename to` extended headers, and those paths are *not*
|
||||
`a/`/`b/`-prefixed, unlike `Create`/`Delete`/`Modify`, which come from the
|
||||
`---`/`+++` lines *with* the prefix. The adapter therefore calls
|
||||
`FileOperation::strip_prefix(1)` (git's `-p1`) only for the non-rename
|
||||
variants — exactly the split `FileOperation`'s own doc comment and
|
||||
`diffy`'s `examples/apply.rs` describe. This is the one place the two APIs
|
||||
differ in shape, and the one place a naive port would have broken.
|
||||
- **Quoted-path unescaping.** `diffy` decodes git's full C-style quoting —
|
||||
named escapes *and* 3-digit octal — via `escaped_filename`, and rejects
|
||||
non-UTF-8 in the `str` variant with `InvalidUtf8Path`. That matches the old
|
||||
`gix::quote::ansi_c::undo` + `String::from_utf8` behavior exactly, error
|
||||
case included.
|
||||
|
||||
Two encoding details had to be matched rather than assumed:
|
||||
|
||||
- `HunkRange::start()`/`len()` are the **literal hunk-header numbers**
|
||||
(`@@ -1,3 +1,3 @@` → `start == 1`), not 0-based indices — `diffy`'s own
|
||||
`diff/mod.rs` adds 1 when *building* a range from an index. So
|
||||
`old_start`/`new_start`/`old_lines`/`new_lines` map across directly, and the
|
||||
`@@ -0,0 +1 @@` empty-range case falls out for free.
|
||||
- `Line`'s text **keeps** the trailing newline and has already had the
|
||||
`+`/`-`/` ` prefix stripped. The adapter re-derives `DiffLine.old`/`new` by
|
||||
counting from the hunk header (context advances both, deletion only old,
|
||||
insertion only new, same as before) and strips the line ending the way
|
||||
`str::lines` does.
|
||||
|
||||
One deliberate behavior difference: `PatchSet` yields a single
|
||||
`Err("no valid patches found")` for input containing no patch at all, where a
|
||||
patch with no `diff --git` section used to yield an empty file list.
|
||||
`patch_diffs` now short-circuits to an empty `CommitDiff` when no line starts
|
||||
with `diff --git ` — the same guard `diffy`'s internal `find_gitdiff_start`
|
||||
uses — so `empty_or_unparseable_patch_yields_no_files` still holds.
|
||||
|
||||
Note: the earlier estimate above ("~700 line hand-rolled parser") was too
|
||||
high; the parser itself was 370 lines, and the ~1300 lines of tests for it
|
||||
remain, now serving as the fixture-by-fixture verification for the
|
||||
crate-backed implementation.
|
||||
|
||||
---
|
||||
|
||||
## 6. Remove the `tasks: Vec<Task<...>>` + `push_task` boilerplate — use `Task::detach()`
|
||||
|
||||
> **Status: done.** Removed the `tasks` field and `push_task` from all six
|
||||
> stores (`backend.rs`, `checkouts.rs`, `local_repos.rs`, `profile.rs`,
|
||||
> `repo.rs`, `repo_list.rs`); every call site now ends in `.detach()`
|
||||
> instead. As with §14, most `cx.spawn` sites lost their type-inference
|
||||
> anchor and needed an explicit `let task: Task<Result<(), Error>> = ...`
|
||||
> (or `gpui::Task<...>` where `Task` wasn't imported) before `.detach()`.
|
||||
> A few closures that captured a variable also named `task` (the awaited
|
||||
> inner task) were given a distinct outer name (`notify_task`, `publish`,
|
||||
> `fetch`, `sync`) to avoid a confusing shadow. `cargo check --workspace`
|
||||
> and `cargo test -p signed_state` (24 tests) / `cargo test -p workspace`
|
||||
> (14 tests) all pass.
|
||||
|
||||
Verified against the actual pinned GPUI revision
|
||||
(`~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/scheduler/src/executor.rs:375-573`
|
||||
and `crates/gpui/src/executor.rs:32-63`).
|
||||
@@ -627,26 +610,6 @@ to `RepoListStore`.
|
||||
|
||||
## 8. Relay add/connect: stop round-tripping through strings, stop reconnecting the whole pool
|
||||
|
||||
> **Status: done.** Every `add_relay` call site now passes the `RelayUrl`
|
||||
> directly (no `.as_str()`/`ToString` round trip) and chains `.and_connect()`
|
||||
> instead of a separate, pool-wide `client.connect()`/`client.connect_relay()`
|
||||
> call: `Backend::bootstrap` (both the `BOOTSTRAP_RELAYS` and `INDEXER_RELAYS`
|
||||
> loops), `bootstrap_user`, `create_repository`, `publish_local_repo`,
|
||||
> `connect_repo_relays`, and `stage_event_on_relay`. `Backend::add_relays` is
|
||||
> deleted entirely — its two callers (`create_repository`, `publish_local_repo`)
|
||||
> now capture `client` once before the surrounding `cx.spawn` and loop
|
||||
> `client.add_relay(url).and_connect().await.ok();` directly over the
|
||||
> `Vec<RelayUrl>` they already had, with no `Vec<String>` conversion at all.
|
||||
> Verified `.and_connect()` (`client/api/add.rs`) and pool-wide `.connect()`
|
||||
> semantics (`client/api/connect.rs`) against the pinned nostr-sdk source
|
||||
> before making the change (see chat history), plus that `pool.sync()`
|
||||
> requires relays to already be present in the pool
|
||||
> (`pool/mod.rs:679-693`, `relays.get(&url).ok_or_else(...)`), which is why
|
||||
> `sync_bootstrap_only`/`bootstrap_user` can safely assume `BOOTSTRAP_RELAYS`
|
||||
> are already added by `Backend::bootstrap` before any sync runs.
|
||||
> `cargo check --workspace`, `cargo clippy -p signed_state`, and
|
||||
> `cargo test -p signed_state` (24 tests) all pass.
|
||||
|
||||
Flagged example (`backend.rs:1071-1074`):
|
||||
|
||||
```rust
|
||||
@@ -733,24 +696,6 @@ Delete `Backend::add_relays` entirely once both call sites are inlined.
|
||||
|
||||
## 9. `create_repository`'s flow is backwards: it inits a mirror, then clones it into the real destination
|
||||
|
||||
> **Status: done.** `Backend::create_repository` now computes `destination`
|
||||
> (`folder.join(dir_name)`) up front and calls `signed_git::init_repository`
|
||||
> directly on it — no mirror path, no `Url::from_file_path`, no `clone_repo`
|
||||
> call, no double `origin` setup. An explicit `destination.exists()` check
|
||||
> (mirroring what `clone_repo` used to guard for free) replaces the removed
|
||||
> clone step's own guard. The push at the end now runs against `destination`
|
||||
> instead of the mirror path, and the task returns `(announcement,
|
||||
> destination)` as before — no caller-visible signature change. This also
|
||||
> dropped `GitStore`/`GitCache::repo_path`/`repo_addr` usage from the
|
||||
> function entirely, since no mirror is created there anymore; `repo_addr`
|
||||
> and `Context as AnyhowContext` became unused imports in `backend.rs` and
|
||||
> were removed. Updated a stale comment in `signed_git`'s
|
||||
> `working_copy_cloned_from_the_mirror_matches_head_and_origin` test, which
|
||||
> referenced this flow by name even though it's a generic `clone_repo`
|
||||
> fixture unrelated to `Backend::create_repository`. `cargo check --workspace`,
|
||||
> `cargo clippy --workspace`, and `cargo test --workspace` (signed_git 67,
|
||||
> signed_state 24, workspace 14) all pass.
|
||||
|
||||
This is a real business-logic flaw, not just a style issue. Today
|
||||
(`backend.rs:437-501`):
|
||||
|
||||
@@ -826,27 +771,6 @@ source of truth.
|
||||
|
||||
## 10. Bootstrap-on-construction should go through `cx.defer`, not run synchronously in `new`
|
||||
|
||||
> **Status: done.** All six constructors listed in the table below now build
|
||||
> `Self` with no side effects, capture `cx.entity().downgrade()`, and defer
|
||||
> the bootstrap call(s) with `cx.defer(move |cx| { weak.update(cx, |this,
|
||||
> cx| ...).ok-or-log(); })`. Verified `cx.entity()` is safe to call before
|
||||
> the entity is registered: `App::new`'s `cx.entities.reserve()` bumps the
|
||||
> ref count to 1 before `build_entity` runs (`app/entity_map.rs:114-117`),
|
||||
> so `weak_entity().upgrade()` succeeds throughout construction, and the
|
||||
> deferred closure only runs after `cx.new`'s `insert_entity` call has fully
|
||||
> populated the entity, so the weak upgrade inside the deferred closure
|
||||
> always succeeds too (barring the caller synchronously dropping the
|
||||
> just-created `Entity` before yielding, an edge case worth a log line, not
|
||||
> a crash). `RepoStore::new` bundles its three previously-sequential calls
|
||||
> (`subscribe_remote`, `connect_announced_relays`, `refresh`) into one
|
||||
> deferred closure to preserve their relative order. Failure to upgrade is
|
||||
> logged with `log::warn!` rather than silently discarded with `.ok()`, per
|
||||
> this project's error-handling rule. `cargo check --workspace`,
|
||||
> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace`
|
||||
> all pass unchanged — none of the existing tests construct these stores
|
||||
> through a `TestAppContext` and assert state immediately after `cx.new`,
|
||||
> so no test needed a `cx.run_until_parked()` addition.
|
||||
|
||||
Verified against the pinned GPUI revision
|
||||
(`crates/gpui/src/app.rs:1999-2005`, `crates/gpui/src/app/context.rs:296-315`).
|
||||
|
||||
@@ -903,39 +827,6 @@ talk to other entities" in the same synchronous call, which is exactly what
|
||||
|
||||
## 11. Split independently-observed state into child entities
|
||||
|
||||
> **Status: done**, with one correction to the approach originally sketched
|
||||
> below. `Backend::pushing_repos` is now `Entity<HashSet<RepoAddr>>`,
|
||||
> created with `cx.new(|_| HashSet::new())` in `Backend::new` and exposed
|
||||
> via `Backend::pushing_repos() -> Entity<HashSet<RepoAddr>>` for future
|
||||
> `cx.observe` callers (nothing reads it today — the actual UI-facing "is
|
||||
> this repo pushing" indicator is the pre-existing, already-observable
|
||||
> `RepoStore::pushing: bool`; this field is purely `push_repo_from`'s
|
||||
> internal re-entrancy guard).
|
||||
>
|
||||
> The blocker: `Drop::drop(&mut self)` has no `cx` parameter, so `PushGuard`
|
||||
> could not literally call `pushing_repos.update(cx, ...)` on drop as first
|
||||
> sketched below — confirmed by checking Zed's own codebase, which hits the
|
||||
> same wall and falls back to a raw `Mutex` for exactly this reason
|
||||
> (`crates/project/src/project.rs`'s `RemotelyCreatedModelGuard`). The fix is
|
||||
> `AsyncApp::on_drop(&self, entity: &WeakEntity<T>, f: impl FnOnce(&mut T,
|
||||
> &mut Context<T>) + 'static) -> Deferred<impl FnOnce()>`
|
||||
> (`gpui/src/app/async_context.rs:266-276`), which is exactly what several
|
||||
> Zed crates already use for this "clean up an entity when a spawned task is
|
||||
> cancelled" pattern (e.g. `git_ui/src/git_panel.rs`'s
|
||||
> `_clear_pending_remote_operation = cx.on_drop(&this, |this, cx| ...)`).
|
||||
> `push_repo_from` now inserts into `pushing_repos` synchronously before
|
||||
> `cx.spawn` (using the already-available `&mut Context<Backend>`), and
|
||||
> holds `let _guard = cx.on_drop(&this, move |backend, cx| { ... remove ...
|
||||
> });` for the lifetime of the spawned task — removal fires on completion,
|
||||
> error, or cancellation alike, same as the old `Drop for PushGuard`, but
|
||||
> now through a real, observable entity update with `cx.notify()`. The old
|
||||
> `PushGuard` struct and its `Drop` impl are deleted; `Arc`/`Mutex` are no
|
||||
> longer imported in `backend.rs` at all. `cargo check --workspace`,
|
||||
> `cargo clippy --workspace --all-targets` and `cargo test --workspace` all
|
||||
> pass unchanged; no call site outside `signed_state` touched
|
||||
> `pushing_repos`, confirming it had zero external readers before this
|
||||
> change.
|
||||
|
||||
`Backend::pushing_repos` (`backend.rs:88`) is `Arc<Mutex<HashSet<RepoAddr>>>`
|
||||
— it bypasses GPUI's entity system entirely. A view that wants to show "is
|
||||
repository X currently pushing" has no way to `cx.observe` this; it can
|
||||
@@ -983,33 +874,6 @@ This principle is also the reason **not** to merge `LocalReposStore` and
|
||||
|
||||
## 12. One debounce at the source, not one per store
|
||||
|
||||
> **Status: done.** `Backend::new`'s pump now batches: it waits for the
|
||||
> first `ClientNotification::Event`, then races `notifications.next()`
|
||||
> against a `PUMP_DEBOUNCE` (200ms) timer in a loop, collecting every event
|
||||
> that arrives before the deadline into one `Vec<Update>`, then emits a
|
||||
> single `BackendEvent::NostrUpdate(Vec<Update>)`. Implemented with
|
||||
> `futures::future::select` + `futures::pin_mut!`, matching the existing
|
||||
> debounce idiom already used by `ProfileStore::handle_requests` in the same
|
||||
> crate (not `select_biased!`, which isn't used anywhere else here).
|
||||
> Verified `Client::notifications()` returns a `Pin<Box<dyn Stream<Item =
|
||||
> ClientNotification> + Send>>` backed by a `broadcast::Receiver`
|
||||
> (`nostr-sdk/src/client/mod.rs:199-205`, `pool/mod.rs:96`), so cancelling a
|
||||
> `.next()` future mid-poll to race it against the timer cannot drop a
|
||||
> notification — the broadcast cursor only advances on a completed receive.
|
||||
> `BackendEvent::NostrUpdate` changed from `Update` to `Vec<Update>`; its 3
|
||||
> actual subscribers (`ProfileStore`, `RepoStore`, `RepoListStore` —
|
||||
> `CheckoutsStore` only observes `RepoListStore`/`LocalReposStore`, it never
|
||||
> matched on `NostrUpdate` directly) were updated to iterate the batch
|
||||
> (`.any(...)` for the two relevance checks, a `for` loop over the
|
||||
> metadata-kind updates in `ProfileStore`). Also fixed a `let _ =` silently
|
||||
> discarding a `WeakEntity::update` result in `ProfileStore::handle_requests`,
|
||||
> found while touching this file, replaced with the `.ok()` idiom used
|
||||
> everywhere else in this crate for the same "entity may already be gone"
|
||||
> case. Downstream per-store `RefreshGate` debounce windows are left
|
||||
> unchanged for now, per the "measure before resizing" note below.
|
||||
> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and
|
||||
> `cargo test --workspace` all pass unchanged.
|
||||
|
||||
Flagged example — the notification pump (`backend.rs:126-146`):
|
||||
|
||||
```rust
|
||||
@@ -1085,16 +949,6 @@ than speculatively resizing four timers up front.
|
||||
|
||||
## 13. `local_repos.rs` + `repo_list.rs`: merge the files, not the entities
|
||||
|
||||
> **Status: done.** Merged both files into `signed_state/src/repos.rs`, keeping
|
||||
> `LocalReposStore` and `RepoListStore` as two fully independent structs, each
|
||||
> still its own `Entity`/`Global` with the same `global()`/`set_global()` pairs
|
||||
> and public API as before — zero call-site churn beyond fixing the `use`
|
||||
> paths (`crate::local_repos`/`crate::repo_list` → `crate::repos`) in
|
||||
> `checkouts.rs`, `repo.rs` and `lib.rs`. `cargo check --workspace`,
|
||||
> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace`
|
||||
> (signed_state 24 tests, workspace 14 tests, full suite 165+ tests) all pass
|
||||
> unchanged.
|
||||
|
||||
These two are structurally near-identical: both hold an `Arc<Vec<T>>`
|
||||
snapshot, refresh it in the background on a trigger, swap it in with
|
||||
`cx.notify()`, and carry their own `Global` wrapper + `global()`/`set_global()`
|
||||
@@ -1146,14 +1000,6 @@ entirely disjoint observers.
|
||||
|
||||
## 14. `crates/workspace` has the same task-list pattern as §6 — and there it's an actual bug
|
||||
|
||||
> **Status: done.** The `tasks` field and all 17 push sites were removed from
|
||||
> `RepoDetailView`, `NewPullRequestView`, `CommitDiffView` and
|
||||
> `PullRequestDetailView`, replaced with `.detach()`. `cargo check -p workspace`
|
||||
> and `cargo test -p workspace` (14 tests) pass. Removing the field cost each
|
||||
> `cx.spawn`/`cx.spawn_in` call site its type-inference anchor, so every
|
||||
> remaining spawn site needed an explicit `let task: gpui::Task<Result<(), ...>> = ...`
|
||||
> annotation — expect the same when doing §6's `signed_state` half.
|
||||
|
||||
§6 covers `signed_state`'s 6 stores, where the unpruned-`Vec<Task>` pattern
|
||||
is a style/complexity concern with no observed failure, because
|
||||
`push_task` always pruned before pushing. `crates/workspace` has the exact
|
||||
@@ -1197,21 +1043,6 @@ already exists once in the same crate.
|
||||
|
||||
## 15. `Vec<Url>` → `Vec<String>` conversion sprawl — fix the 3 `signed_git` signatures, not the 8 call sites
|
||||
|
||||
> **Status: done.** `try_each_url`, `clone_repo`, `GitCache::ensure_clone`
|
||||
> and `fetch_repo_refs` are now generic over `U: AsRef<str>`. All 7 call
|
||||
> sites (`repo.rs::merge_pull_request`/`clone_to_folder`,
|
||||
> `repo_detail/mod.rs::load_repo`, `new_pull_request.rs::choose_fork` x2,
|
||||
> `pull_request_detail.rs::load`/`clone_urls_of`) now pass the
|
||||
> `Vec<Url>`/`Vec<Url>`-derived value straight through with a plain
|
||||
> `.clone()` of the field, no `.iter().map(ToString::to_string).collect()`
|
||||
> anywhere left in non-test code. One test in `signed_git` passed an empty
|
||||
> untyped `&[]` literal to `fetch_repo_refs`, which lost its type-inference
|
||||
> anchor once the function went generic — fixed with an explicit
|
||||
> `&[] as &[String]` annotation. `cargo check --workspace`,
|
||||
> `cargo clippy -p signed_state -p workspace -p signed_git --all-targets`,
|
||||
> and `cargo test --workspace` (signed_git 67, signed_state 24, workspace 14)
|
||||
> all pass.
|
||||
|
||||
`Announcement::clone` is `Vec<Url>` (`signed_core/src/model.rs`, `Url` being
|
||||
`nostr`'s re-export of the `url` crate's `Url`, `nostr/src/types/url.rs:15`,
|
||||
`pub use url::*;`). Every call site that needs to hand those URLs to
|
||||
@@ -1300,47 +1131,6 @@ than avoidable duplication.
|
||||
|
||||
## 17. Business logic that leaked into `crates/workspace` and should move to `signed_core`/`signed_state`
|
||||
|
||||
> **Status: done.** All four sub-items landed:
|
||||
>
|
||||
> - `current_commit_of` is now `pub fn` in `signed_core::model`; the
|
||||
> byte-for-byte duplicate in `pull_request_detail.rs` is deleted, replaced
|
||||
> by an import.
|
||||
> - `merge_base_of`, `clone_urls_of`, `branch_name_of` and `latest_update`
|
||||
> moved to `signed_core::model` as `pub fn`s with their tests (the same
|
||||
> `signed()`/`pr_root()`-style fixtures the doc predicted, renamed
|
||||
> `signed_at`/`pr_root` to avoid colliding with `model.rs`'s existing
|
||||
> single-owner `keys()`/`announcement_event` fixtures used by unrelated
|
||||
> `is_fork_of` tests in the same file). `pull_request_detail.rs` lost the
|
||||
> now-unused `Nip34Tag`/`Url` imports as a result.
|
||||
> - `fork_candidates` moved to `signed_core::model` as planned. The doc's
|
||||
> wording was ambivalent about where `fork_namespace` should go ("safe,
|
||||
> low-risk move to `signed_core`" vs. "pairs naturally with `signed_git`'s
|
||||
> ref-naming conventions" in the same paragraph) — turns out only one is
|
||||
> actually possible: `fork_namespace` calls `signed_git::sanitize_path_component`,
|
||||
> and `signed_git` **depends on** `signed_core` (`signed_git/Cargo.toml`),
|
||||
> so moving it to `signed_core` would be a circular dependency. It moved to
|
||||
> `signed_git` instead, next to `sanitize_path_component`, with a new unit
|
||||
> test (it had none before). `fork_candidates` has no such constraint (only
|
||||
> touches `Announcement`/`RepoAddr`/`PublicKey`) and moved to `signed_core`
|
||||
> as planned, tests included. `new_pull_request.rs`'s entire `mod tests`
|
||||
> block was deleted — both moved functions were the only things it tested.
|
||||
> - `NewPullRequestView::submit` no longer calls `format_patch_between`
|
||||
> itself: `RepoStore::open_pull_request_from_refs(repo_path, merge_base,
|
||||
> compare_ref, subject, description, branch_name, draft, cx) ->
|
||||
> Task<Result<(), Error>>` does the `format_patch_between` + empty-check +
|
||||
> `open_pull_request` sequence internally, with the exact same two error
|
||||
> messages ( "No commits between the branches to propose" /
|
||||
> "Failed to generate the patch: {error}") the view used to produce
|
||||
> inline, now surfaced through the returned `Task`'s `Err` and displayed
|
||||
> via the view's existing `self.error` field — no observable UI change.
|
||||
> `submit` shrank to gathering form values and awaiting the store call;
|
||||
> `format_patch_between` is no longer imported in `new_pull_request.rs`.
|
||||
>
|
||||
> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and
|
||||
> `cargo test --workspace` all pass; test counts moved with the functions
|
||||
> (`signed_core` 41 → 48, `signed_git` 67 → 68 for the new `fork_namespace`
|
||||
> test, `workspace` 14 → 7), no failures, no coverage lost.
|
||||
|
||||
Direct answer to "can the view side be thinner": yes, and not speculatively —
|
||||
found one confirmed duplicate, one cluster of misplaced domain parsing, and
|
||||
one mutating-flow split across the view/store boundary. The test used to
|
||||
@@ -1475,121 +1265,120 @@ method.
|
||||
|
||||
---
|
||||
|
||||
## Action plan, in order of risk/reward
|
||||
## Outcome
|
||||
|
||||
1. ✅ **Delete the fetch/sync dedup cache** (§3). Pure removal, no behavior
|
||||
change for the intended usage pattern (each call site already has, or
|
||||
trivially gets, its own guard). Lowest risk, do first.
|
||||
All 14 items below are implemented and verified, listed in the order they were
|
||||
done — mechanical removals first, the largest diff (§2) and the riskiest swap
|
||||
(§5) last.
|
||||
|
||||
Done: removed `recent_fetches`/`fetch_recently_started`/`fetch_fingerprint`/
|
||||
`FETCH_DEDUP_WINDOW` and the now-unused `DefaultHasher`/`Hash`/`Hasher`/
|
||||
`Instant` imports from `signed_state/src/backend.rs`. `connect_repo_relays`
|
||||
and `sync_bootstrap` no longer fingerprint or gate on a cache; callers keep
|
||||
their own guards (`RepoStore::repo_relays`, one-shot construction-time call
|
||||
in `RepoListStore::subscribe_remote`). `cargo check --workspace` and
|
||||
`cargo test -p signed_state` (24 tests) both pass unchanged.
|
||||
2. ✅ **Remove the `tasks: Vec<Task<...>>` + `push_task` boilerplate**, in
|
||||
both `signed_state` (§6) and `crates/workspace` (§14), in favor of
|
||||
`.detach()`/`.detach_and_log_err(cx)`. Independent of every other change
|
||||
here, touches 10 files, all mechanical — and fixes a real unbounded-growth
|
||||
bug in `RepoDetailView`/`NewPullRequestView` along the way.
|
||||
1. **Delete the fetch/sync dedup cache** (§3). `recent_fetches`,
|
||||
`fetch_recently_started`, `fetch_fingerprint` and `FETCH_DEDUP_WINDOW` are
|
||||
gone, along with the `DefaultHasher`/`Hash`/`Hasher`/`Instant` imports they
|
||||
needed. Each call site keeps its own guard.
|
||||
2. **Remove the `tasks: Vec<Task<...>>` + `push_task` boilerplate** on both
|
||||
sides (§6, §14) — all six `signed_state` stores and all four
|
||||
`crates/workspace` views, 17 push sites, most never pruned. In the views
|
||||
this was a real unbounded-growth bug, not just style.
|
||||
3. **Fix the relay add/connect calls** (§8). No `.as_str()`/`ToString` round
|
||||
trips; `add_relay(url).and_connect()` instead of a separate, pool-wide
|
||||
`connect()`; `Backend::add_relays` deleted.
|
||||
4. **Fix `bootstrap_user`** (§1) to sync via negentropy and query the local
|
||||
database. `grep -rn "fetch_events" crates/` now returns nothing.
|
||||
5. **Generalize the three `signed_git` URL-list signatures** to
|
||||
`U: AsRef<str>` (§15) and drop the seven
|
||||
`.iter().map(ToString::to_string).collect()` call sites, which now pass
|
||||
`&[Url]` straight through.
|
||||
6. **Fix `create_repository`'s init/clone ordering** (§9). It initializes and
|
||||
pushes directly at the destination; no mirror, no `clone_repo`, no double
|
||||
`origin`.
|
||||
7. **Merge `local_repos.rs` and `repo_list.rs`** into
|
||||
`signed_state/src/repos.rs` (§13), keeping both stores independent
|
||||
`Entity`/`Global`s — no call-site change beyond `use` paths.
|
||||
8. **Route construction-time bootstrap through `cx.defer`** in all six stores
|
||||
(§10), with a failed weak upgrade logged rather than silently dropped.
|
||||
9. **Consolidate the send paths** (§2, §4). The eight layers (`Backend::send`,
|
||||
`publish_event`, `publish_task`, `send_fire_and_forget`, `broadcast_event`,
|
||||
`RepoStore::send`, …) collapsed to direct
|
||||
`client.send_event(&event).broadcast()` calls plus one
|
||||
`require_relay_accepted` helper. `retract_events` now sends one NIP-09
|
||||
deletion per target, with no `k` tag. The rewrite also fixed the
|
||||
inconsistent error surfacing this section flagged: the three call sites
|
||||
with divergent control flow now set `last_error` on failure like every
|
||||
other `RepoStore` mutation.
|
||||
10. **Split `pushing_repos` into a child entity** (§11). It is now an
|
||||
observable `Entity<HashSet<RepoAddr>>`; the old `PushGuard` and the
|
||||
`Arc`/`Mutex` around it are gone.
|
||||
11. **Centralize the notification-pump debounce** (§12). The pump batches into
|
||||
a single `BackendEvent::NostrUpdate(Vec<Update>)` behind a 200 ms window,
|
||||
and its three subscribers iterate the batch.
|
||||
12. **Drop progressive reveal** (§7). `RepoListStore` refreshes once per
|
||||
completed sync instead of several times mid-sync.
|
||||
13. **Replace the hand-rolled `git format-patch` parser with
|
||||
`diffy::patch_set`** (§5). 370 lines to 216, no test changed.
|
||||
14. **Move the misplaced `workspace` domain logic** (§17) to
|
||||
`signed_core`/`signed_git`, and give `RepoStore` a refs-in-patch-out
|
||||
method so `NewPullRequestView::submit` no longer generates patches itself.
|
||||
|
||||
Done: both halves are complete, see §6 and §14 for details.
|
||||
3. ✅ **Fix the relay add/connect calls** (§8): drop `.as_str()`/`ToString`
|
||||
round trips, replace `add_relay` + blanket `client.connect()`/
|
||||
`connect_relay` pairs with `add_relay(url).and_connect()`, and delete
|
||||
`Backend::add_relays`. Mechanical, no behavior change beyond "connect
|
||||
only what was just added."
|
||||
### Deviations, corrections, and findings worth keeping
|
||||
|
||||
Done: see §8 for the full list of call sites and verification notes.
|
||||
4. ✅ **Fix `bootstrap_user`** to sync+query instead of `fetch_events` (§1).
|
||||
One function, fully covered by existing tests for
|
||||
`latest_grasp_list_servers`.
|
||||
Most items landed exactly as planned. These are the ones that did not, plus
|
||||
the non-obvious findings that were only ever recorded in the per-item status
|
||||
notes this section replaced:
|
||||
|
||||
Done: see §1. `fetch_events` no longer appears anywhere in the workspace.
|
||||
5. ✅ **Generalize the 3 `signed_git` URL-list signatures** to `&[impl AsRef<str>]`
|
||||
(§15), then delete the now-redundant `.map(ToString::to_string).collect()`
|
||||
at all 7 call sites. Self-contained to `signed_git`'s public API plus a
|
||||
one-line change per call site; re-run `signed_git`'s existing tests
|
||||
(`clone_repo`/`fetch_repo_refs` already have coverage).
|
||||
- **`RepoStore::publish` was deliberately kept** (§2), a narrow exception to
|
||||
"delete `RepoStore::send`": its four callers (`open_issue`, `reply`,
|
||||
`set_status`, `publish_applied_status`) have byte-for-byte identical
|
||||
sign+send+check+`last_error` post-conditions. The three callers with
|
||||
genuinely divergent control flow (`open_pull_request`,
|
||||
`update_pull_request`, `publish_patch_series`) call the SDK inline.
|
||||
- **`fork_namespace` went to `signed_git`, not `signed_core`** as §17
|
||||
sketched. It calls `signed_git::sanitize_path_component`, and `signed_git`
|
||||
already depends on `signed_core`, so the sketched direction would have been
|
||||
a circular crate dependency.
|
||||
- **`pushing_repos` was made observable with `AsyncApp::on_drop`, not a `Drop`
|
||||
impl** (§11). `Drop::drop(&mut self)` has no `cx`, so it cannot update a GPUI
|
||||
entity; Zed's own codebase hits the same wall and falls back to a raw
|
||||
`Mutex` (`crates/project/src/project.rs`, `RemotelyCreatedModelGuard`).
|
||||
- **Calling `cx.entity()` before the entity is registered is safe** (§10) —
|
||||
what makes the deferred bootstrap sound. `App::new`'s `cx.entities.reserve()`
|
||||
bumps the ref count before `build_entity` runs
|
||||
(`app/entity_map.rs:114-117`), and the deferred closure only runs after
|
||||
`cx.new`'s `insert_entity`.
|
||||
- **Gossip stays enabled** in `ClientBuilder` for future NIP-17/NIP-65 work;
|
||||
every git-domain send bypasses it explicitly with `.broadcast()` (§4).
|
||||
- **`pool.sync()` requires the relays to already be in the pool**
|
||||
(`pool/mod.rs:679-693`), which is why `Backend::bootstrap` adds
|
||||
`BOOTSTRAP_RELAYS` before `sync_bootstrap_only`/`bootstrap_user` run — the
|
||||
precondition §8's change relies on.
|
||||
- **`pushing_repos` has no readers today** (§11): it is `push_repo_from`'s
|
||||
internal re-entrancy guard. The UI-facing "is pushing" indicator is the
|
||||
pre-existing, already-observable `RepoStore::pushing` boolean.
|
||||
- **`patch_diffs` short-circuits on input with no `diff --git ` line** (§5),
|
||||
because `PatchSet` yields `Err("no valid patches found")` for input holding
|
||||
no patch at all, where the old parser returned an empty list.
|
||||
- **A `let _ =` on a `WeakEntity::update` in `ProfileStore::handle_requests`
|
||||
became `.ok()`** (§12), found while touching that file. It now follows the
|
||||
project's error-handling rule.
|
||||
- **Two type-inference anchors had to be re-added by hand**, a recurring cost
|
||||
of both §6/§14 and §15: removing a `Vec<Task<...>>` field and going generic
|
||||
over `AsRef<str>` both strip the anchor from call sites with untyped `&[]`
|
||||
literals, fixed with explicit `Task<Result<(), Error>>` and
|
||||
`&[] as &[String]` annotations.
|
||||
- **Not covered by tests:** the deleted send paths (§2) and the
|
||||
`create_repository` fix (§9) need a live relay or a live GRASP server, so
|
||||
they were verified by compilation plus a line-by-line diff against the old
|
||||
control flow. A manual create-repository-then-open-detail-view pass is still
|
||||
the recommended pre-ship check for §9.
|
||||
|
||||
Done: see §15 for the full list of call sites and verification notes.
|
||||
6. ✅ **Fix `create_repository`'s init/clone ordering** (§9): initialize and
|
||||
push directly at the user's chosen destination, drop the mirror
|
||||
pre-population entirely and let `ensure_clone` populate it lazily like
|
||||
every other repo. Self-contained to one function; verify against this
|
||||
crate's existing `init_repository`/push tests plus a manual
|
||||
create-repository-then-open-detail-view pass.
|
||||
|
||||
Done: see §9. Manual create-repository-then-open-detail-view pass still
|
||||
recommended before shipping, since it depends on the grasp push actually
|
||||
succeeding end-to-end against a live server.
|
||||
7. ✅ **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13),
|
||||
keeping both stores as independent entities. Purely organizational, zero
|
||||
call-site changes, safe to do any time.
|
||||
|
||||
Done: merged into `signed_state/src/repos.rs`, see §13 for verification
|
||||
notes.
|
||||
8. ✅ **Route construction-time bootstrap through `cx.defer`** (§10) in all
|
||||
six stores listed there. Mechanical per store, but touch them one at a
|
||||
time and re-run each store's test suite, since ordering-sensitive
|
||||
assumptions (e.g. a test that asserts state right after `cx.new`) may
|
||||
need `cx.run_until_parked()` inserted where they didn't before.
|
||||
|
||||
Done: `Backend`, `RepoStore`, `RepoListStore`, `LocalReposStore`,
|
||||
`CheckoutsStore` and `ProfileStore` all defer their bootstrap now, see
|
||||
§10 for verification notes.
|
||||
9. ✅ **Consolidate the send paths** (§2): introduce the single
|
||||
`require_relay_accepted` helper, delete
|
||||
`Backend::send`/`publish_event`/`send_fire_and_forget`/`broadcast_event`/
|
||||
`RepoStore::send`, switch `retract_events` to `EventDeletionRequest`
|
||||
(one deletion event per target, no `k` tag), and make each send site
|
||||
explicit about bypassing gossip (§4) with `.broadcast()`/`.to(relays)`.
|
||||
This is the biggest diff and touches every publish call site (`repo.rs`,
|
||||
`backend.rs`), so do it as its own PR with full test-suite coverage
|
||||
before/after.
|
||||
|
||||
Done: see §2 and §4 for the full list of call sites, the one deliberate
|
||||
narrow exception (`RepoStore::publish`), and verification notes.
|
||||
10. ✅ **Split `pushing_repos` (and similar fields) into a child entity** (§11).
|
||||
Small, isolated change once §9's `PushGuard` rewrite is in flight — do
|
||||
them together since both touch `PushGuard`.
|
||||
|
||||
Done: see §11 — implemented via `AsyncApp::on_drop`, not the plain
|
||||
`Drop` impl originally sketched, which turned out not to be possible.
|
||||
11. ✅ **Centralize the notification-pump debounce** (§12). This one is the
|
||||
most speculative of the batch — land it after §7's `SyncProgress`
|
||||
decision and re-measure whether each store's own `RefreshGate` window
|
||||
can shrink, rather than assuming the exact shape up front.
|
||||
|
||||
Done: see §12. Landed without waiting on §7 since it doesn't depend on
|
||||
that decision — downstream `RefreshGate` windows were deliberately left
|
||||
unresized, so there's nothing here for §7 to invalidate either way.
|
||||
12. **Optional, product call:** drop `SyncProgress` from
|
||||
`RepoListStore`'s relevant-event match if progressive reveal during
|
||||
bootstrap sync isn't a feature you want (§7).
|
||||
13. **Spike `diffy::patch_set::PatchSet`** to replace `signed_git`'s
|
||||
hand-rolled `git format-patch` parser (§5). Separate PR, separate
|
||||
crate, no interaction with the nostr-facing changes above — do this in
|
||||
parallel if you have a second contributor, otherwise last since it's
|
||||
the largest and riskiest single change (needs fixture-by-fixture
|
||||
verification against the existing test suite).
|
||||
14. ✅ **Move the misplaced `workspace` domain logic to `signed_core`/`signed_state`** (§17):
|
||||
make `current_commit_of` `pub` in `signed_core` and delete the
|
||||
`workspace` duplicate; move `merge_base_of`/`clone_urls_of`/`branch_name_of`/
|
||||
`latest_update` and `fork_candidates`/`fork_namespace` there too, tests
|
||||
included; give `RepoStore` a refs-in-patch-out method so
|
||||
`NewPullRequestView::submit` stops calling `format_patch_between`
|
||||
itself. Low risk, no behavior change, best done as its own small PR per
|
||||
function cluster rather than one big move.
|
||||
|
||||
Done: see §17. `fork_namespace` ended up in `signed_git`, not
|
||||
`signed_core`, to avoid a circular crate dependency — everything else
|
||||
landed exactly as planned.
|
||||
Verification: `cargo check --workspace`, `cargo clippy --workspace
|
||||
--all-targets` and `cargo test --workspace` pass — 167 tests, 0 failures —
|
||||
re-run after each item landed rather than once at the end. Test counts moved
|
||||
between crates as functions moved (`signed_core` 41 → 48, `signed_git`
|
||||
67 → 68, `workspace` 14 → 7); no coverage was lost.
|
||||
|
||||
Everything **not** listed above (per-repo/per-list `Entity` stores, the
|
||||
`RefreshGate` debounce/coalesce pattern, `Nip34Tag`/`Coordinate`/`Filter`
|
||||
usage in `signed_core`, the GRASP push-retry state machine in
|
||||
`push_staged_to_grasps`, the `UniversalSigner` abstraction, and the dense
|
||||
`.clone()` clusters audited in §16) was checked and already matches "use
|
||||
the SDK directly, no unnecessary wrapper" — those should be left alone.
|
||||
`.clone()` clusters audited in §16) was checked and already matches "use the
|
||||
SDK directly, no unnecessary wrapper" — those were left alone.
|
||||
|
||||
Reference in New Issue
Block a user