From 291eb0fd8dc2a8666ed37afba69029116d7b79f7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 4 May 2026 10:19:19 -0400 Subject: [PATCH] Fix CI and Documentation issues --- builtin/api/packages.lua | 38 +++-- docs/packages.md | 106 +++++++++++++- src/attach.rs | 164 +++++++++++++++++++--- src/buffer.rs | 5 - src/lua_bindings.rs | 184 +++++++++++++++++-------- src/packages/installer.rs | 227 ++++++++++++++++++++++-------- src/packages/mod.rs | 4 +- tests/m6_5_repl_acceptance.rs | 6 +- tests/m7_3_acceptance.rs | 253 +++++++++++++++++++++++++++++++++- 9 files changed, 826 insertions(+), 161 deletions(-) diff --git a/builtin/api/packages.lua b/builtin/api/packages.lua index 8d6dac4..4aca098 100644 --- a/builtin/api/packages.lua +++ b/builtin/api/packages.lua @@ -15,31 +15,49 @@ --- --- Two accepted shapes: --- ---- - Table with positional address at `[1]`: `{ "github:owner/repo", version = "^1.0.0" }`. ---- The `version` field defaults to `"*"` (any tag) when omitted. The ---- `install_project` variant additionally **requires** `project_root = "..."` ---- (no default; see the field doc below). ---- - Shorthand string `"github:owner/repo@^1.0.0"`. The separator is the **last** `@` in the ---- string, so addresses containing an `@` (SSH shorthand `git@host:path`) parse correctly. ---- The shorthand form is not accepted by `install_project` (no place to put `project_root`). +--- - Table with positional address at `[1]`. The pin is one of three +--- mutually-exclusive fields: +--- - `version = ""` (default; constrains to a tag). +--- - `branch = ""` (HEAD of the named branch at install time). +--- - `commit = ""` (specific revision; full or partial SHA). +--- Specifying more than one of these errors with a "must specify +--- exactly one" message naming every conflicting field. With none +--- specified, the pin defaults to `version = "*"` (any tag). +--- The `install_project` variant additionally **requires** +--- `project_root = "..."` (no default). +--- - Shorthand string `"github:owner/repo@^1.0.0"`. **Version-pin only.** +--- The separator is the **last** `@` in the string, so addresses +--- containing an `@` (SSH shorthand `git@host:path`) parse correctly. +--- Branch and commit pins must use the table form. The shorthand +--- form is also not accepted by `install_project` (no place to put +--- `project_root`). --- --- @class PackageInstallSpec --- @field [1] string Positional address (e.g., `"github:owner/repo"`). --- @field address string|nil Alternative to the positional `[1]`. ---- @field version string|nil Semver constraint (e.g., `"^1.0.0"`, `"=1.2.3"`, `"*"`). Defaults to `"*"`. +--- @field version string|nil Semver constraint (e.g., `"^1.0.0"`, `"=1.2.3"`, `"*"`). Mutually exclusive with `branch` / `commit`. Defaults to `"*"` when none of the three are specified. +--- @field branch string|nil Branch name (e.g., `"main"`). The install resolves the branch's HEAD at install time; not reproducible across time. Mutually exclusive with `version` / `commit`. +--- @field commit string|nil Commit SHA (full or partial). The install pins to that exact revision. Mutually exclusive with `version` / `branch`. --- @field project_root string|nil `install_project` only: REQUIRED project root. Absolute paths used as-is. Relative paths resolve against the directory of the loading `init.lua` (not against CWD). Common patterns: `os.getenv("PMACS_PROJECT")`, or a literal subdirectory like `"."` for "alongside this init.lua". +--- The pin info on an [`InstalledPackage`]. +--- +--- @class InstalledPackagePin +--- @field kind "version"|"branch"|"commit" Which pin kind the user supplied. +--- @field value string The user-supplied value: the semver constraint (e.g. `"^1.0.0"`), the branch name (e.g. `"main"`), or the commit SHA. Echoes the field on `PackageInstallSpec` exactly. + --- A successful-install record returned by `install` and listed by `installed`. --- --- @class InstalledPackage --- @field name string Package name from `pmacs.toml` (e.g., `"samplepkg"` or `"user/samplepkg"`). ---- @field version string Semver of the resolved tag (canonical numeric form, e.g., `"1.2.3"`). ---- @field tag string The tag that was matched (e.g., `"v1.2.3"`). +--- @field version string Manifest-declared version of the installed snapshot (canonical semver, e.g. `"1.2.3"`). +--- @field tag string Resolution descriptor: matched tag (`"v1.2.3"`) for version pins, `"branch:"` for branch pins, `"commit:"` for commit pins. Always non-empty. --- @field commit string 40-character commit SHA of the installed snapshot. --- @field install_path string Absolute on-disk install directory. --- @field entry string Absolute path to the package's `entry` Lua module. --- @field scope "user"|"project" Which scope the package was installed under. --- @field summary string One-line description from the manifest. +--- @field pin InstalledPackagePin Structured pin info (the user's request, distinct from the resolved descriptor). local pmacs = pmacs or {} pmacs.packages = pmacs.packages or {} diff --git a/docs/packages.md b/docs/packages.md index 8df0ec7..f1e58ac 100644 --- a/docs/packages.md +++ b/docs/packages.md @@ -19,12 +19,47 @@ pmacs.packages.install { } ``` -The shorthand string form is also accepted: +The shorthand string form is also accepted (version pins only): ```lua pmacs.packages.install "github:owner/repo@^1.0.0" ``` +### Pin kinds: `version`, `branch`, `commit` + +Each install pins exactly one revision. The spec table chooses the +pin via one of three mutually-exclusive fields: + +```lua +-- Highest semver tag matching the constraint. Recommended default. +pmacs.packages.install { "github:owner/repo", version = "^1.0.0" } + +-- HEAD of the named branch at install time. Not reproducible across +-- time --- the upstream's branch HEAD moves --- so use sparingly. +pmacs.packages.install { "github:owner/repo", branch = "main" } + +-- Exact commit. Reproducible: the same SHA always installs the same +-- snapshot. Useful for pinning to a known-good state before the +-- upstream has tagged a release. +pmacs.packages.install { "github:owner/repo", commit = "abc1234" } +``` + +Mutual exclusion is enforced: a spec table with two of these fields +errors with a "must specify exactly one" message naming every +conflicting field. With none of the three, the pin defaults to +`version = "*"` (any tag). + +The shorthand string form (`"address@^1.0"`) is **version-pin only**. +Branch and commit pins must use the table form because there is no +unambiguous sigil that distinguishes a branch named "main" from a +malformed semver constraint without surprising users. + +For version pins, pmacs additionally cross-checks that the +manifest's `version` field at the matched tag satisfies the user's +constraint, catching upstreams whose tag and `pmacs.toml` disagree. +Branch and commit pins skip that check (the user explicitly asked +for that revision regardless of what the manifest says). + ## `pmacs.packages.install_project { ... }` — project scope Installs to `/.pmacs/packages//`. Project @@ -105,12 +140,77 @@ field, since it has no implicit project context. The change will be relaxation, not breakage: code that explicitly passes `project_root` keeps working unchanged. +## How `require` resolution works + +pmacs uses Lua's standard require machinery, augmented at install +time: + +1. **Path-based search.** Each install prepends + `/?.lua;/?/init.lua` to + `package.path`. Packages with the conventional layout + (`.lua` or `/init.lua`) resolve via this + path with no further machinery — exactly as a hand-written Lua + project would. + +2. **Custom searcher.** When the path-based search misses (e.g. + the manifest declares `entry = "main.lua"` or `entry = + "lib/foo.lua"`), a custom searcher pmacs registered in + `package.searchers` (Lua 5.4) / `package.loaders` (LuaJIT and + Lua 5.1) consults the install roster, finds the matching + package by basename, and returns a loader for the exact entry + path declared in the manifest. + +The searcher iterates the roster in install order, most-recent +first, so a project-scope install of a basename overrides a prior +user-scope install of the same basename — mirroring the +"newer-installs-prepend-to-path" semantics of the path-based +search. + +When `require` cannot find a name through any searcher, the +combined error message names every searcher's contribution; the +custom searcher's contribution looks like: + +``` +no installed pmacs package named 'whatever' +``` + +so a user with a typo can spot it without digging into pmacs's +internals. + ## `pmacs.packages.installed()` Returns an array of records describing every package installed during the current init pass. Each record has the same shape as -`install`'s return value (`name`, `version`, `commit`, -`install_path`, `entry`, `scope`, `summary`). +`install`'s return value: + +```lua +{ + name = "samplepkg", -- manifest's name + version = "1.0.0", -- manifest's declared version + tag = "v1.0.0", -- resolution descriptor (see below) + commit = "abc...", -- full SHA of the installed snapshot + install_path = "...", + entry = "...", + scope = "user", -- or "project" + summary = "...", + pin = { -- structured user request + kind = "version", -- or "branch" or "commit" + value = "^1.0.0", -- echoes the spec field exactly + }, +} +``` + +The `tag` field is a stable, non-empty descriptor: + +- For version pins: the matched tag (`"v1.2.3"`). +- For branch pins: `"branch:"`. +- For commit pins: `"commit:"`. + +The `pin` table is the source of truth for "what did the user +request." The flat fields (`tag`, `version`, `commit`) record the +resolution. They differ for branch/commit pins, where the resolved +commit is what got installed but the user's request was the branch +name or SHA prefix. ## `pmacs.packages.update(...)` diff --git a/src/attach.rs b/src/attach.rs index 0fd56dc..34ecb0f 100644 --- a/src/attach.rs +++ b/src/attach.rs @@ -29,12 +29,16 @@ //! later release will make it configurable. use std::collections::VecDeque; -use std::io::{Read, Write}; +use std::io::{self, Read, Write}; use std::net::Shutdown; use std::os::unix::net::UnixStream; use std::path::PathBuf; use std::process::{Child, Command, Stdio}; -use std::sync::{Arc, Mutex, mpsc}; +use std::sync::{ + Arc, Mutex, + atomic::{AtomicBool, Ordering}, + mpsc, +}; use std::thread; use std::time::Duration; @@ -210,7 +214,8 @@ impl From for AttachError { /// /// Each transport builds its own `AttachIo` from the primitives it has /// available. The local-socket transport (M5.5g) clones a `UnixStream` -/// three ways and uses `shutdown(Read)` for the kick. The SSH +/// three ways and uses a kick-aware reader plus socket shutdown for +/// the kick. The SSH /// transport (M5.7e) takes a child process's `stdout` and `stdin` /// halves and uses `SIGTERM` to the child for the kick. pub(crate) struct AttachIo { @@ -229,10 +234,9 @@ pub(crate) struct AttachIo { /// The reader thread is presumed to be blocked on a read; this /// wakes it. /// - /// Implementations may be destructive (SSH transport `SIGTERM`s - /// the child) or non-destructive (local-socket transport calls - /// `shutdown(Read)`). Callers do not distinguish — by the time - /// the kick runs, the pump has already decided to exit. + /// Implementations may be destructive. Callers do not distinguish + /// — by the time the kick runs, the pump has already decided to + /// exit. pub kick: Box, } @@ -318,21 +322,83 @@ pub fn run_attach(socket_path: PathBuf) -> Result<(), AttachError> { /// Build an [`AttachIo`] for a connected `UnixStream`. /// -/// The kick clones a third handle for `shutdown(Read)`. Cloning may +/// The kick sets a shared flag and clones a third handle for +/// `shutdown(Both)`. Cloning may /// fail (rare — the kernel is out of file descriptors), in which /// case the caller propagates the error before raw mode engages. fn build_local_socket_io(stream: UnixStream) -> Result { let reader = stream.try_clone()?; + reader.set_nonblocking(true)?; let kick_handle = stream.try_clone()?; + let kicked = Arc::new(AtomicBool::new(false)); + let reader_kicked = Arc::clone(&kicked); Ok(AttachIo { - reader: Box::new(reader), + reader: Box::new(KickAwareUnixReader { + stream: reader, + kicked: reader_kicked, + }), writer: Box::new(stream), kick: Box::new(move || { - let _ = kick_handle.shutdown(Shutdown::Read); + kicked.store(true, Ordering::SeqCst); + let _ = kick_handle.shutdown(Shutdown::Both); }), }) } +/// Non-blocking poll-based reader with a kick flag. +/// +/// # Wake semantics +/// +/// This reader has two cooperating wake paths, only one of which is +/// load-bearing: +/// +/// 1. **Atomic flag (correctness):** the reader runs a non-blocking +/// poll loop with a 10ms sleep between iterations. After the kick +/// sets `kicked`, the next loop iteration observes it and returns +/// `Ok(0)`. Worst-case wake latency is one poll cycle (~10ms). +/// This path is platform-independent and is the mechanism the +/// caller relies on for correctness. +/// +/// 2. **`shutdown(Both)` on a sibling clone (best-effort speedup):** +/// if the reader happens to be inside `self.stream.read()` when +/// the kick fires, and the platform honors cross-clone shutdown +/// wakes, the read returns `Ok(0)` immediately and the loop +/// skips its sleep. This path is **not** load-bearing — Unix +/// socket cross-clone shutdown semantics are not portably +/// guaranteed, and any wake it provides is a bonus on top of +/// path 1. +/// +/// In other words: the atomic flag wakes the reader; the shutdown +/// just shaves up to ~10ms off the wake when the platform plays +/// along. Tests asserting wake bounds should treat the budget as +/// "≤ one poll cycle plus scheduler jitter," not as a measure of +/// shutdown latency. +struct KickAwareUnixReader { + stream: UnixStream, + kicked: Arc, +} + +impl Read for KickAwareUnixReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + loop { + match self.stream.read(buf) { + Err(e) + if matches!( + e.kind(), + io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut + ) => + { + if self.kicked.load(Ordering::SeqCst) { + return Ok(0); + } + thread::sleep(Duration::from_millis(10)); + } + other => return other, + } + } + } +} + fn build_capabilities() -> FrontendCapabilities { // The v0.1 TUI implements all of these; we report them honestly // so the daemon doesn't strip features that work fine. @@ -487,7 +553,7 @@ pub(crate) fn run_attach_pair( // reader needs `kick` to wake. // // 2. `kick()` — wake the reader thread, by any means necessary - // (per the kick contract). For local-socket: `shutdown(Read)`. + // (per the kick contract). For local-socket: `shutdown(Both)`. // For SSH: a watchdog that SIGTERMs the child if the EOF // cascade hasn't reached the reader within the watchdog's // grace period. @@ -1350,6 +1416,27 @@ mod tests { } } + /// Test-only `Read` wrapper that flips an `AtomicBool` whenever + /// its inner reader is called. Used to synchronize the test + /// against "the reader thread has entered its read call" without + /// resorting to wall-clock sleeps. + /// + /// External wrapper by design: production types stay free of + /// test-only hooks. The signal fires on every `read` call (not + /// just the first); the test only cares about observing it + /// transition once, so cheap repeated stores are harmless. + struct EnteredReadSignaler { + inner: R, + entered: Arc, + } + + impl Read for EnteredReadSignaler { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + self.entered.store(true, Ordering::Release); + self.inner.read(buf) + } + } + /// Build an `AttachIo` with a kick that increments `counter`. fn pipe_io_with_counting_kick(socket: UnixStream, counter: Arc) -> AttachIo { let reader = socket.try_clone().expect("try_clone reader"); @@ -1459,7 +1546,34 @@ mod tests { } #[test] - fn kick_wakes_blocked_reader_within_one_second() { + fn kick_wakes_blocked_reader() { + // What this test asserts: after `kick()` fires, the reader + // thread terminates. That is, the kick mechanism wakes a + // reader that would otherwise wait on the socket forever. + // + // What this test does NOT assert: a specific wake latency. + // The 5s join bound is intentionally generous so that CI + // scheduler jitter (heavily loaded VM hosts can stall threads + // for hundreds of ms cumulatively) does not turn a correctness + // test into a flake. The steady-state wake budget per + // `KickAwareUnixReader`'s contract is ~10ms (one poll cycle), + // but observing that bound under timing pressure is not what + // this test is for. **Do not tighten the 5s bound back toward + // 1s on the grounds that 5s is much larger than the + // steady-state budget** — the steady-state budget is not what + // is being tested. A real bug (kick mechanism is broken, the + // reader runs forever) hits this bound; CI jitter does not + // come close. + // + // Synchronization: rather than guessing how long the reader + // thread takes to start with `thread::sleep`, the test wraps + // the reader in an `EnteredReadSignaler` that flips an + // `AtomicBool` when the inner reader is first called. The + // test spins on that flag (bounded) so kick fires only once + // we know the reader is actively reading from the socket. + // No wall-clock guesses; no test-only paths in production + // types. + // Hold the daemon side so the kick is the only thing that can // wake the reader. If we let the daemon side close, the reader // sees EOF naturally and we'd be testing nothing. @@ -1471,11 +1585,25 @@ mod tests { kick, } = io; - let (tx, _rx) = mpsc::channel::(); - let reader_handle = thread::spawn(move || run_reader(reader, tx)); + let entered = Arc::new(AtomicBool::new(false)); + let signaling_reader: Box = Box::new(EnteredReadSignaler { + inner: reader, + entered: Arc::clone(&entered), + }); - // Let the reader actually start blocking on its read. - thread::sleep(Duration::from_millis(50)); + let (tx, _rx) = mpsc::channel::(); + let reader_handle = thread::spawn(move || run_reader(signaling_reader, tx)); + + // Wait for the reader to enter its read call. Bounded so a + // never-spawning reader fails the test instead of hanging. + let entry_deadline = Instant::now() + Duration::from_secs(1); + while !entered.load(Ordering::Acquire) { + assert!( + Instant::now() < entry_deadline, + "reader thread did not enter its read call within 1s", + ); + thread::sleep(Duration::from_millis(1)); + } kick(); @@ -1487,8 +1615,8 @@ mod tests { let _ = done_tx.send(()); }); done_rx - .recv_timeout(Duration::from_secs(1)) - .expect("reader thread must exit within 1s after kick"); + .recv_timeout(Duration::from_secs(5)) + .expect("reader thread must exit within 5s after kick"); } #[test] diff --git a/src/buffer.rs b/src/buffer.rs index 79b88a6..e19e2fd 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -702,11 +702,6 @@ impl Buffer { MarkGravity::Right => new_end, } } - } else if pos < end { - match mark.gravity { - MarkGravity::Left => start, - MarkGravity::Right => new_end, - } } else { match mark.gravity { MarkGravity::Left => start, diff --git a/src/lua_bindings.rs b/src/lua_bindings.rs index f7862ec..a6e7ca0 100644 --- a/src/lua_bindings.rs +++ b/src/lua_bindings.rs @@ -59,7 +59,8 @@ use crate::hook::{Hook, HookRegistry}; use crate::key::{display_sequence, parse_sequence}; use crate::keymap_stack::KeymapStack; use crate::packages::{ - Address, Fetcher, InstallError, InstallScope, InstallSpec, InstalledPackage, Installer, + Address, Fetcher, InstallError, InstallPin, InstallScope, InstallSpec, InstalledPackage, + Installer, }; use crate::protocol::{AttachTarget, AttachmentHandle, InstanceIdentity}; use crate::rope::Range; @@ -626,6 +627,21 @@ pub enum BindingError { )] InstallSpecMissingAddress, + /// A `pmacs.packages.install{...}` spec table specified more than + /// one of `version`, `branch`, `commit`. Each install must pin + /// exactly one revision; combining pin kinds is ambiguous (which + /// one wins?). The error message names every conflicting field + /// the spec actually carried. + #[error( + "pmacs.packages.install: spec must specify exactly one of \ + `version`, `branch`, or `commit`; got: {fields}" + )] + InstallSpecConflictingPins { + /// Comma-separated list of the offending field names, in + /// the order they appeared on the table. + fields: String, + }, + /// `install_project` was called without an explicit /// `project_root` field. The CWD-fallback was removed because at /// init time CWD is whatever directory the user happened to @@ -1744,6 +1760,7 @@ fn install_instance_show_binding(lua: &Lua, registry: &SharedRegistry) -> mlua:: }) } +#[allow(clippy::too_many_lines)] fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { let buffer = lua.create_table()?; @@ -1873,7 +1890,7 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result mlua::Result) -> mlua::Result { fn attach_style_overlay_to_visible_windows( lua: &Lua, buffer_id: BufferId, - spans: crate::overlay::SharedBufferStyleSpans, + spans: &crate::overlay::SharedBufferStyleSpans, ) { let Some(core) = lua.app_data_ref::() else { return; @@ -1921,7 +1938,7 @@ fn attach_style_overlay_to_visible_windows( for win in core.windows.values_mut() { if win.buffer_id == buffer_id { win.push_overlay(Box::new(crate::overlay::BufferStyleOverlay::new( - Arc::clone(&spans), + Arc::clone(spans), ))); } } @@ -2055,7 +2072,7 @@ fn install_packages_module(lua: &Lua) -> mlua::Result
{ } /// Register a custom searcher in `package.searchers` (Lua 5.4) / -/// `package.loaders` (Lua 5.1, LuaJIT) that consults the +/// `package.loaders` (Lua 5.1, `LuaJIT`) that consults the /// [`InstalledPackages`] roster at require time. /// /// # Why @@ -2089,7 +2106,7 @@ fn install_packages_module(lua: &Lua) -> mlua::Result
{ /// /// # 5.1 vs 5.4 names /// -/// Lua 5.1 / LuaJIT exposes the searcher list as `package.loaders`; +/// Lua 5.1 / `LuaJIT` exposes the searcher list as `package.loaders`; /// Lua 5.2+ renamed it to `package.searchers`. Both are tables of /// functions with the same callback shape. We probe `searchers` /// first and fall back to `loaders` so the same code works under @@ -2101,51 +2118,44 @@ fn register_package_searcher(lua: &Lua) -> mlua::Result<()> { None => package.get::
("loaders")?, }; - let searcher = lua.create_function( - |lua, name: String| -> mlua::Result { - let Some(slot) = lua.app_data_ref::() else { - // Slot uninstalled (shouldn't happen under - // production wiring, but a defensive nil keeps - // require working under unusual test setups). - return Ok(mlua::Value::Nil); - }; - let snapshot = slot.snapshot(); - // Most-recent-first: a project-scope install of a - // basename overrides a prior user-scope install. - for pkg in snapshot.iter().rev() { - if pkg.install_basename() != name { - continue; - } - let entry = pkg.entry_path(); - let bytes = match std::fs::read(&entry) { - Ok(b) => b, - Err(e) => { - // Searcher convention: a non-function return - // is treated as "not found, here's why" and - // appended to the require error message. - let s = lua.create_string(&format!( - "\n\tinstalled pmacs package '{name}' \ - entry `{}` could not be read: {e}", - entry.display() - ))?; - return Ok(mlua::Value::String(s)); - } - }; - let chunk_name = format!("@{}", entry.display()); - let func = lua - .load(&bytes) - .set_name(&chunk_name) - .into_function()?; - return Ok(mlua::Value::Function(func)); + let searcher = lua.create_function(|lua, name: String| -> mlua::Result { + let Some(slot) = lua.app_data_ref::() else { + // Slot uninstalled (shouldn't happen under + // production wiring, but a defensive nil keeps + // require working under unusual test setups). + return Ok(mlua::Value::Nil); + }; + let snapshot = slot.snapshot(); + // Most-recent-first: a project-scope install of a + // basename overrides a prior user-scope install. + for pkg in snapshot.iter().rev() { + if pkg.install_basename() != name { + continue; } - // No installed package matches. Return a string so Lua - // appends our reason to the aggregate require error. - let s = lua.create_string(&format!( - "\n\tno installed pmacs package named '{name}'" - ))?; - Ok(mlua::Value::String(s)) - }, - )?; + let entry = pkg.entry_path(); + let bytes = match std::fs::read(&entry) { + Ok(b) => b, + Err(e) => { + // Searcher convention: a non-function return + // is treated as "not found, here's why" and + // appended to the require error message. + let s = lua.create_string(format!( + "\n\tinstalled pmacs package '{name}' \ + entry `{}` could not be read: {e}", + entry.display() + ))?; + return Ok(mlua::Value::String(s)); + } + }; + let chunk_name = format!("@{}", entry.display()); + let func = lua.load(&bytes).set_name(&chunk_name).into_function()?; + return Ok(mlua::Value::Function(func)); + } + // No installed package matches. Return a string so Lua + // appends our reason to the aggregate require error. + let s = lua.create_string(format!("\n\tno installed pmacs package named '{name}'"))?; + Ok(mlua::Value::String(s)) + })?; // Append to the searcher list. Lua tables are 1-indexed; the // new searcher runs after every existing searcher (preload, @@ -2184,18 +2194,10 @@ fn parse_lua_install_spec(value: &Value) -> mlua::Result { } }, }; - let version_str: String = t - .get::("version") - .unwrap_or_else(|_| "*".to_string()); let address = Address::parse(&address_str) .map_err(|e| mlua::Error::external(BindingError::from(InstallError::Address(e))))?; - let version = semver::VersionReq::parse(&version_str).map_err(|e| { - mlua::Error::external(BindingError::from(InstallError::InvalidVersionReq { - value: version_str, - cause: e.to_string(), - })) - })?; - Ok(InstallSpec { address, version }) + let pin = parse_install_pin(t)?; + Ok(InstallSpec { address, pin }) } other => Err(mlua::Error::external(BindingError::InstallSpecWrongType { got: other.type_name().to_string(), @@ -2203,6 +2205,57 @@ fn parse_lua_install_spec(value: &Value) -> mlua::Result { } } +/// Parse the pin fields from a `pmacs.packages.install{...}` table. +/// +/// A spec table may carry exactly one of: +/// - `version = ""` (e.g. `"^1.0.0"`, `"=2.3.4"`). +/// - `branch = ""` (e.g. `"main"`). +/// - `commit = ""` (full or partial; the fetcher accepts either). +/// +/// If none are present the pin defaults to `version = "*"` (any +/// tag). If two or more are present the parse fails with +/// [`BindingError::InstallSpecConflictingPins`] naming every field +/// that conflicted. +fn parse_install_pin(t: &Table) -> mlua::Result { + let version: Option = t.get::>("version").unwrap_or(None); + let branch: Option = t.get::>("branch").unwrap_or(None); + let commit: Option = t.get::>("commit").unwrap_or(None); + let mut present: Vec<&'static str> = Vec::new(); + let version = version.filter(|s| !s.is_empty()); + let branch = branch.filter(|s| !s.is_empty()); + let commit = commit.filter(|s| !s.is_empty()); + if version.is_some() { + present.push("version"); + } + if branch.is_some() { + present.push("branch"); + } + if commit.is_some() { + present.push("commit"); + } + if present.len() > 1 { + return Err(mlua::Error::external( + BindingError::InstallSpecConflictingPins { + fields: present.join(", "), + }, + )); + } + if let Some(b) = branch { + return Ok(InstallPin::Branch(b)); + } + if let Some(c) = commit { + return Ok(InstallPin::Commit(c)); + } + let value = version.unwrap_or_else(|| "*".to_string()); + let req = semver::VersionReq::parse(&value).map_err(|e| { + mlua::Error::external(BindingError::from(InstallError::InvalidVersionReq { + value, + cause: e.to_string(), + })) + })?; + Ok(InstallPin::Version(req)) +} + /// Read the required `project_root = "..."` field from the table form /// of `install_project`'s spec. /// @@ -2374,6 +2427,15 @@ fn installed_package_to_lua(lua: &Lua, pkg: &InstalledPackage) -> mlua::Result }`. + // Existing flat fields (`tag`, `version`, `commit`) remain + // populated for backward-compatible introspection; the `pin` + // table is the source of truth for "what did the user request", + // distinct from "what got resolved". + let pin_table = lua.create_table_with_capacity(0, 2)?; + pin_table.set("kind", pkg.pin.kind())?; + pin_table.set("value", pkg.pin.value())?; + t.set("pin", pin_table)?; Ok(t) } diff --git a/src/packages/installer.rs b/src/packages/installer.rs index 158a1e6..b8d9e6a 100644 --- a/src/packages/installer.rs +++ b/src/packages/installer.rs @@ -104,13 +104,71 @@ fn xdg_data_root() -> Result { // InstallSpec // --------------------------------------------------------------------------- -/// A normalized install request: where to fetch and what version to pick. +/// What the user pinned the install to. Mutually exclusive on the +/// Lua side: a spec table may carry exactly one of `version`, +/// `branch`, or `commit`. +/// +/// # Why three kinds +/// +/// - [`Self::Version`] is the default, recommended path: the +/// installer picks the highest semver tag matching the constraint +/// and validates the manifest's declared version against the same +/// constraint. Lockfile reproduction (M7.6) records the resolved +/// commit so a later install at the same constraint yields the +/// same revision. +/// - [`Self::Branch`] follows a moving target. Each install +/// re-resolves the branch's HEAD; the install is *not* +/// reproducible across time. Useful for development against an +/// upstream's `main` or for a private package whose semver +/// discipline is not yet established. +/// - [`Self::Commit`] freezes the install at a specific revision. +/// Useful for pinning to a known-good state before the upstream +/// has tagged a release, or for reproducing a colleague's +/// environment exactly without semver drift. +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum InstallPin { + /// Highest semver tag satisfying the constraint. + Version(VersionReq), + /// HEAD of the named branch at install time. + Branch(String), + /// Specific commit (full or partial SHA; the fetcher accepts + /// either via `git rev-parse`). + Commit(String), +} + +impl InstallPin { + /// Stable string discriminator used at the Lua boundary + /// (`installed_package_to_lua`) and in error messages. + #[must_use] + pub fn kind(&self) -> &'static str { + match self { + Self::Version(_) => "version", + Self::Branch(_) => "branch", + Self::Commit(_) => "commit", + } + } + + /// User-supplied value as a string: the constraint for + /// [`Self::Version`], the branch name for [`Self::Branch`], the + /// SHA for [`Self::Commit`]. + #[must_use] + pub fn value(&self) -> String { + match self { + Self::Version(req) => req.to_string(), + Self::Branch(b) => b.clone(), + Self::Commit(c) => c.clone(), + } + } +} + +/// A normalized install request: where to fetch and how to pin the +/// revision. #[derive(Debug, Clone)] pub struct InstallSpec { /// Address (resolved via `Address::parse`). pub address: Address, - /// Semver constraint to match against the upstream's tags. - pub version: VersionReq, + /// What the user pinned the install to. See [`InstallPin`]. + pub pin: InstallPin, } impl InstallSpec { @@ -126,6 +184,14 @@ impl InstallSpec { /// has no `@` separator and [`InstallError::Address`] / /// [`InstallError::InvalidVersionReq`] for the underlying parse /// failures. + /// + /// The shorthand string form is **version-pin only**. Branch and + /// commit pins must use the Lua table form + /// (`{ "addr", branch = "..." }` / `{ "addr", commit = "..." }`) + /// because there is no concise sigil that disambiguates a + /// branch/commit value from a semver constraint without + /// surprising users (`@main` could be a branch named "main" or + /// a malformed semver --- ambiguous). pub fn parse_shorthand(s: &str) -> Result { let (addr, ver) = s.rsplit_once('@') @@ -142,7 +208,10 @@ impl InstallSpec { value: ver.to_string(), cause: e.to_string(), })?; - Ok(Self { address, version }) + Ok(Self { + address, + pin: InstallPin::Version(version), + }) } } @@ -159,12 +228,26 @@ pub struct InstalledPackage { pub install_path: PathBuf, /// 40-char commit hash of the installed snapshot. pub commit: String, - /// The tag that was matched (e.g., `v1.0.0` or `1.0.0`). + /// A descriptor of what was installed: + /// - For [`InstallPin::Version`]: the matched tag, e.g. `"v1.0.0"`. + /// - For [`InstallPin::Branch`]: `"branch:"`. + /// - For [`InstallPin::Commit`]: `"commit:"`. + /// + /// Always non-empty so Lua callers can use it as a stable + /// "what got installed" label without nil-checking. pub tag: String, - /// The semver value parsed from `tag` (canonical numeric form). + /// Semver version of the installed snapshot. For + /// [`InstallPin::Version`] this is the version parsed from the + /// matched tag; for [`InstallPin::Branch`] / [`InstallPin::Commit`] + /// it falls back to `manifest.version` (the package's declared + /// version at the resolved revision). pub version: Version, /// The install scope this package was installed under. pub scope: InstallScope, + /// What the user originally pinned this install to. Useful for + /// lockfile generation (M7.6) and for surfacing to the Lua + /// `installed()` snapshot. + pub pin: InstallPin, } impl InstalledPackage { @@ -243,33 +326,54 @@ impl Installer { } /// Install one package. See module docs for the step-by-step flow. + #[allow(clippy::too_many_lines)] pub fn install(&self, spec: &InstallSpec) -> Result { let url = spec.address.to_git_url(); let bare = self.fetcher.fetch(&url).map_err(InstallError::Fetch)?; - // 2. Pick best matching tag. - let tags = self.fetcher.list_tags(&bare).map_err(InstallError::Fetch)?; - let chosen = - best_match(&tags, &spec.version).ok_or_else(|| InstallError::NoMatchingVersion { - address: url.clone(), - req: spec.version.to_string(), - available: tags.clone(), - })?; + // Resolve the user's pin to a concrete (commit, tag-descriptor) + // pair. The descriptor is what we display to users in the + // `tag` field of the resulting `InstalledPackage`. + let (commit, tag_descriptor) = match &spec.pin { + InstallPin::Version(req) => { + let tags = self.fetcher.list_tags(&bare).map_err(InstallError::Fetch)?; + let chosen = + best_match(&tags, req).ok_or_else(|| InstallError::NoMatchingVersion { + address: url.clone(), + req: req.to_string(), + available: tags.clone(), + })?; + let commit = self + .fetcher + .resolve(&bare, &RefSpec::Tag(chosen.tag.clone())) + .map_err(InstallError::Fetch)?; + (commit, chosen.tag) + } + InstallPin::Branch(name) => { + let commit = self + .fetcher + .resolve(&bare, &RefSpec::Branch(name.clone())) + .map_err(InstallError::Fetch)?; + (commit, format!("branch:{name}")) + } + InstallPin::Commit(sha) => { + let commit = self + .fetcher + .resolve(&bare, &RefSpec::Commit(sha.clone())) + .map_err(InstallError::Fetch)?; + let short = commit.get(..7).unwrap_or(commit.as_str()).to_string(); + (commit, format!("commit:{short}")) + } + }; - // 3. Resolve to commit. - let commit = self - .fetcher - .resolve(&bare, &RefSpec::Tag(chosen.tag.clone())) - .map_err(InstallError::Fetch)?; - - // 4. Read manifest at this commit so we know the install dir name. + // Read the manifest at this commit so we know the install dir name. let manifest_bytes = self .fetcher .show_blob(&bare, &commit, "pmacs.toml") .map_err(|e| match e { FetchError::GitInvocation { stderr, .. } => InstallError::ManifestMissing { address: url.clone(), - tag: chosen.tag.clone(), + tag: tag_descriptor.clone(), cause: stderr, }, other => InstallError::Fetch(other), @@ -277,42 +381,43 @@ impl Installer { let manifest_str = std::str::from_utf8(&manifest_bytes).map_err(|_| InstallError::ManifestNotUtf8 { address: url.clone(), - tag: chosen.tag.clone(), + tag: tag_descriptor.clone(), })?; let manifest = PackageManifest::from_toml(manifest_str).map_err(InstallError::Manifest)?; // Refuse to install a package whose `pmacs_required` constraint - // does not match the running pmacs version. The manifest field - // is a hard contract, not advisory: if a package declares - // `pmacs_required = ">=2.0.0"` and we're 1.x, the package is - // free to call APIs we do not yet expose, and the failure - // would manifest as a runtime Lua traceback rather than a - // typed install-time error. + // does not match the running pmacs version. Applies to every + // pin kind: a package's declared API requirements are + // independent of how the user pinned the revision. let running_pmacs = running_pmacs_version(); if !manifest.pmacs_required.matches(&running_pmacs) { return Err(InstallError::PmacsVersionIncompatible { address: url.clone(), - tag: chosen.tag.clone(), + tag: tag_descriptor.clone(), required: manifest.pmacs_required.to_string(), running: running_pmacs.to_string(), }); } - // Sanity: the manifest's `version` should equal the resolved tag. - // We don't reject mismatches (some upstreams version-tag asymmetrically), - // but we do require it to satisfy the requested constraint. The tag - // already satisfies the constraint (we chose it that way), so the - // strict check is on the manifest. - if !spec.version.matches(&manifest.version) { - return Err(InstallError::ManifestVersionMismatch { - address: url.clone(), - tag: chosen.tag.clone(), - manifest_version: manifest.version.to_string(), - req: spec.version.to_string(), - }); + // For version pins only: cross-check that the manifest's + // declared version satisfies the constraint. The matched tag + // already satisfies it (we chose it that way); the strict + // check is on the manifest, which catches packages whose tag + // and pmacs.toml version disagree. Branch and commit pins + // skip this check --- the user explicitly asked for that + // revision regardless of what the manifest says. + if let InstallPin::Version(req) = &spec.pin { + if !req.matches(&manifest.version) { + return Err(InstallError::ManifestVersionMismatch { + address: url.clone(), + tag: tag_descriptor.clone(), + manifest_version: manifest.version.to_string(), + req: req.to_string(), + }); + } } - // 5. Archive + extract. + // Archive + extract. let install_root = self.install_root()?; let basename = package_basename(manifest.name.as_str()); let install_path = install_root.join(basename); @@ -326,12 +431,13 @@ impl Installer { match existing { Some(prev) if prev == commit => { return Ok(InstalledPackage { + version: manifest.version.clone(), manifest, install_path, commit, - tag: chosen.tag, - version: chosen.version, + tag: tag_descriptor, scope: self.scope.clone(), + pin: spec.pin.clone(), }); } _ => { @@ -361,12 +467,13 @@ impl Installer { write_install_marker(&install_path, &commit)?; Ok(InstalledPackage { + version: manifest.version.clone(), manifest, install_path, commit, - tag: chosen.tag, - version: chosen.version, + tag: tag_descriptor, scope: self.scope.clone(), + pin: spec.pin.clone(), }) } } @@ -782,7 +889,10 @@ exports = ["samplepkg"] fn shorthand_parses_github_address_with_caret_constraint() { let s = InstallSpec::parse_shorthand("github:user/repo@^1.0.0").unwrap(); assert!(matches!(s.address, Address::Github { .. })); - assert_eq!(s.version.to_string(), "^1.0.0"); + match &s.pin { + InstallPin::Version(req) => assert_eq!(req.to_string(), "^1.0.0"), + other => panic!("expected Version pin, got {other:?}"), + } } #[test] @@ -790,7 +900,10 @@ exports = ["samplepkg"] // SSH shorthand: `git:git@host:path`. The `@` in `git@host` // must not be confused with the version separator. let s = InstallSpec::parse_shorthand("git:git@host:path/repo.git@=1.2.3").unwrap(); - assert_eq!(s.version.to_string(), "=1.2.3"); + match &s.pin { + InstallPin::Version(req) => assert_eq!(req.to_string(), "=1.2.3"), + other => panic!("expected Version pin, got {other:?}"), + } if let Address::Url(u) = s.address { assert_eq!(u, "git@host:path/repo.git"); } else { @@ -876,7 +989,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("^1.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("^1.0").unwrap()), }; let installed = installer.install(&spec).unwrap(); @@ -918,7 +1031,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("=1.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("=1.0.0").unwrap()), }; let installed = installer.install(&spec).unwrap(); assert_eq!(installed.tag, "v1.0.0"); @@ -933,7 +1046,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse(">=2.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse(">=2.0.0").unwrap()), }; let err = installer.install(&spec).unwrap_err(); match err { @@ -953,7 +1066,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("=1.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("=1.0.0").unwrap()), }; let first = installer.install(&spec).unwrap(); // Drop a sentinel; idempotent re-install should not blow it away. @@ -976,14 +1089,14 @@ exports = ["samplepkg"] // First install at 1.0.0. let spec_v1 = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("=1.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("=1.0.0").unwrap()), }; installer.install(&spec_v1).unwrap(); // Second install at 1.1.0 to the same install path: refuse. let spec_v2 = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("=1.1.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("=1.1.0").unwrap()), }; let err = installer.install(&spec_v2).unwrap_err(); assert!(matches!(err, InstallError::AlreadyInstalled { .. })); @@ -1057,7 +1170,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("^1.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("^1.0.0").unwrap()), }; match installer.install(&spec).unwrap_err() { @@ -1086,7 +1199,7 @@ exports = ["samplepkg"] let spec = InstallSpec { address: Address::Url(file_url(&bare)), - version: VersionReq::parse("^1.0.0").unwrap(), + pin: InstallPin::Version(VersionReq::parse("^1.0.0").unwrap()), }; installer .install(&spec) diff --git a/src/packages/mod.rs b/src/packages/mod.rs index 842efe1..2abf0d4 100644 --- a/src/packages/mod.rs +++ b/src/packages/mod.rs @@ -18,5 +18,7 @@ pub mod manifest; pub use address::{Address, AddressError}; pub use fetcher::{FetchError, Fetcher, RefSpec}; -pub use installer::{InstallError, InstallScope, InstallSpec, InstalledPackage, Installer}; +pub use installer::{ + InstallError, InstallPin, InstallScope, InstallSpec, InstalledPackage, Installer, +}; pub use manifest::{DependencySpec, ManifestError, PackageManifest, PackageName}; diff --git a/tests/m6_5_repl_acceptance.rs b/tests/m6_5_repl_acceptance.rs index 7b7b2ed..3456316 100644 --- a/tests/m6_5_repl_acceptance.rs +++ b/tests/m6_5_repl_acceptance.rs @@ -30,9 +30,13 @@ //! marker is `"\n[ exited with code N]\n"`. use pmacs::editor::EditorState; +use std::fmt::Write as _; use std::path::PathBuf; +use std::sync::Mutex; use std::time::{Duration, Instant}; +static PUMP_TEST_LOCK: Mutex<()> = Mutex::new(()); + /// Locate a shell binary for tests that require one. Returns the /// resolved path or `None` if the shell is neither at `PMACS_TEST_` /// nor on `PATH`. Per-test selective skipping (rather than skipping @@ -80,6 +84,7 @@ fn run(chunk: &str) { /// `poll_until` pattern but routes through `tick_processes` so the M6.5 /// after-tick contract is exercised end-to-end. fn run_with_pump(setup_chunk: &str, predicate_chunk: &str, timeout_ms: u64) { + let _guard = PUMP_TEST_LOCK.lock().expect("pump test lock"); let mut editor = EditorState::new(); editor .lua_host @@ -328,7 +333,6 @@ fn m6_5_exit_marker_uses_basename_with_leading_newline() { /// running-state (process must have started before we type) and then /// on history matching the expected output. fn run_shell_smoke_test(shell_path: &std::path::Path, argv_extra: &[&str]) { - use std::fmt::Write as _; let mut argv_lua = String::new(); write!(&mut argv_lua, r#""{}""#, shell_path.display()).unwrap(); for a in argv_extra { diff --git a/tests/m7_3_acceptance.rs b/tests/m7_3_acceptance.rs index f098f8d..8f4bf64 100644 --- a/tests/m7_3_acceptance.rs +++ b/tests/m7_3_acceptance.rs @@ -40,11 +40,7 @@ use tempfile::TempDir; /// (relative to the package root) with the supplied Lua body. /// Returns `(tempdir, bare_path)` --- the tempdir owns both the work /// tree and the bare clone. -fn make_package_with_entry( - name: &str, - entry_path: &str, - entry_body: &str, -) -> (TempDir, PathBuf) { +fn make_package_with_entry(name: &str, entry_path: &str, entry_body: &str) -> (TempDir, PathBuf) { let td = tempfile::tempdir().expect("tempdir"); let work = td.path().join("work"); let bare = td.path().join("upstream.git"); @@ -595,3 +591,250 @@ fn searcher_misses_for_unknown_name_with_pmacs_specific_message() { "error must mention the pmacs searcher's contribution: {msg}" ); } + +// --------------------------------------------------------------------------- +// Reviewer-flagged item 11: branch/commit install pins. +// --------------------------------------------------------------------------- +// +// The fetcher already supports `RefSpec::Branch` and `RefSpec::Commit`; +// item 11 is the Lua-surface plumbing that exposes those resolutions +// to user init.lua. The acceptance shape: a spec table with +// `branch = "..."` or `commit = "..."` (instead of `version = "..."`) +// installs that exact revision. The two are mutually exclusive --- a +// table with both must error. + +/// Build a sample-package bare repo with two tagged versions plus a +/// `feature` branch carrying a third commit. Returns +/// `(tempdir, bare_path, feature_branch_commit_sha)`. Every field +/// caller may need to verify a branch/commit pin came out of the +/// install path correctly. +fn make_branched_sample_package(name: &str) -> (TempDir, PathBuf, String) { + let td = tempfile::tempdir().expect("tempdir"); + let work = td.path().join("work"); + let bare = td.path().join("upstream.git"); + + run_git(&[ + OsStr::new("init"), + OsStr::new("--initial-branch=main"), + work.as_os_str(), + ]); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("config"), + OsStr::new("user.email"), + OsStr::new("test@example.com"), + ]); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("config"), + OsStr::new("user.name"), + OsStr::new("Tester"), + ]); + + // First tagged release on `main`. + write_manifest(&work, name, "1.0.0"); + std::fs::write(work.join("init.lua"), b"return { from = 'main@v1.0.0' }\n") + .expect("write init"); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("add"), + OsStr::new("."), + ]); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("commit"), + OsStr::new("-m"), + OsStr::new("v1.0.0"), + ]); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("tag"), + OsStr::new("v1.0.0"), + ]); + + // Branch off `main` and commit a different init.lua. The branch + // remains untagged --- only a branch ref points at it. + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("checkout"), + OsStr::new("-b"), + OsStr::new("feature"), + ]); + std::fs::write( + work.join("init.lua"), + b"return { from = 'feature-branch' }\n", + ) + .expect("write feature init"); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("add"), + OsStr::new("."), + ]); + run_git(&[ + OsStr::new("-C"), + work.as_os_str(), + OsStr::new("commit"), + OsStr::new("-m"), + OsStr::new("feature work"), + ]); + let feature_sha = git_rev_parse_head(&work); + + // Bare clone after both refs exist. + run_git(&[ + OsStr::new("clone"), + OsStr::new("--bare"), + work.as_os_str(), + bare.as_os_str(), + ]); + + (td, bare, feature_sha) +} + +fn write_manifest(work: &Path, name: &str, version: &str) { + let manifest = format!( + "name = \"{name}\"\n\ + version = \"{version}\"\n\ + summary = \"acceptance fixture\"\n\ + pmacs_required = \">= 0.1.0\"\n\ + entry = \"init.lua\"\n\ + exports = [\"{name}\"]\n" + ); + std::fs::write(work.join("pmacs.toml"), manifest).expect("write pmacs.toml"); +} + +fn git_rev_parse_head(work: &Path) -> String { + let out = Command::new("git") + .arg("-C") + .arg(work) + .arg("rev-parse") + .arg("HEAD") + .env("GIT_TERMINAL_PROMPT", "0") + .env("LC_ALL", "C") + .output() + .expect("git rev-parse spawn"); + assert!(out.status.success(), "git rev-parse failed"); + String::from_utf8(out.stdout) + .expect("rev-parse stdout utf8") + .trim() + .to_string() +} + +#[test] +fn install_with_branch_pin_uses_branch_head() { + let (_pkg_td, bare, _feature_sha) = make_branched_sample_package("samplepkg"); + let url = file_url(&bare); + let (mut host, _cache, _user_root) = host_with_overrides(); + + let script = format!( + r#" + local installed = pmacs.packages.install {{ + "git:{url}", + branch = "feature", + }} + assert(installed.pin.kind == "branch", + "pin.kind must be branch, got " .. tostring(installed.pin.kind)) + assert(installed.pin.value == "feature", + "pin.value must echo the branch name, got " .. tostring(installed.pin.value)) + assert(installed.tag == "branch:feature", + "tag descriptor must be branch:feature, got " .. tostring(installed.tag)) + local mod = require("samplepkg") + assert(mod.from == "feature-branch", + "module body must come from the feature branch's init.lua, got " .. tostring(mod.from)) + "# + ); + host.eval(Some("test"), &script).unwrap_or_else(|e| { + panic!("branch pin install failed: {e}"); + }); +} + +#[test] +fn install_with_commit_pin_uses_exact_revision() { + let (_pkg_td, bare, feature_sha) = make_branched_sample_package("samplepkg"); + let url = file_url(&bare); + let (mut host, _cache, _user_root) = host_with_overrides(); + + let script = format!( + r#" + local installed = pmacs.packages.install {{ + "git:{url}", + commit = "{feature_sha}", + }} + assert(installed.pin.kind == "commit", + "pin.kind must be commit, got " .. tostring(installed.pin.kind)) + assert(installed.pin.value == "{feature_sha}", + "pin.value must echo the SHA, got " .. tostring(installed.pin.value)) + assert(installed.commit == "{feature_sha}", + "resolved commit must equal the pinned SHA, got " .. tostring(installed.commit)) + assert(installed.tag:sub(1, 7) == "commit:", + "tag descriptor must start with commit:, got " .. tostring(installed.tag)) + local mod = require("samplepkg") + assert(mod.from == "feature-branch", + "module body must come from the pinned commit, got " .. tostring(mod.from)) + "# + ); + host.eval(Some("test"), &script).unwrap_or_else(|e| { + panic!("commit pin install failed: {e}"); + }); +} + +#[test] +fn install_with_conflicting_pins_errors_with_field_list() { + // Specifying more than one pin is ambiguous (which one wins?). + // The error must name every conflicting field so the user can + // see which to keep without re-reading the docs. + let (_pkg_td, bare, _) = make_branched_sample_package("samplepkg"); + let url = file_url(&bare); + let (mut host, _cache, _user_root) = host_with_overrides(); + + let script = format!( + r#" + pmacs.packages.install {{ + "git:{url}", + version = "^1.0.0", + branch = "feature", + }} + "# + ); + let err = host + .eval(Some("test"), &script) + .expect_err("conflicting pins must error"); + let msg = err.to_string(); + assert!( + msg.contains("version") && msg.contains("branch"), + "error must name both conflicting fields: {msg}" + ); + assert!( + msg.contains("exactly one"), + "error must explain the mutual-exclusion rule: {msg}" + ); +} + +#[test] +fn install_with_default_version_pin_when_no_pin_field_supplied() { + // The reviewer's wording: existing default is `version = "*"`. + // This test pins that contract: a spec table with no + // version/branch/commit field defaults to "any tag". + let (_pkg_td, bare) = make_sample_package("samplepkg"); + let url = file_url(&bare); + let (mut host, _cache, _user_root) = host_with_overrides(); + + let script = format!( + r#" + local installed = pmacs.packages.install {{ "git:{url}" }} + assert(installed.pin.kind == "version", + "default pin must be a version pin, got " .. tostring(installed.pin.kind)) + assert(installed.pin.value == "*", + "default constraint must be `*`, got " .. tostring(installed.pin.value)) + "# + ); + host.eval(Some("test"), &script).unwrap_or_else(|e| { + panic!("default-pin install failed: {e}"); + }); +}