diff --git a/builtin/runtime/git.lua b/builtin/runtime/git.lua index 9ef69c1..12e6e51 100644 --- a/builtin/runtime/git.lua +++ b/builtin/runtime/git.lua @@ -314,11 +314,33 @@ end --- has three other callers, all of them status-band text, and folding --- the two together would fix this one and break those. --- ---- Exactly one trailing newline is stripped, with an optional preceding ---- carriage return, because that is what git emits. A second newline ---- would be output, not a terminator. +--- Exactly ONE trailing `\n` is stripped, and NOTHING may ride along +--- with it --- not a carriage return, not a second newline. A carriage +--- return is as legal a POSIX path byte as a newline is, so a repository +--- rooted at `/tmp/a\r` makes git print `/tmp/a` `0d` `0a`: the path's +--- own CR, then git's LF terminator. A strip tolerant of `\r?\n$` cannot +--- tell those two bytes apart and takes both, resolving the root as +--- `/tmp/a` --- the same defect this function was written to fix, one +--- byte over. A second newline is output, not a terminator, for the same +--- reason. +--- +--- There is no unambiguous output representation to prefer instead, +--- which was CHECKED against git 2.55 rather than assumed: `-z` is not +--- an option of `git rev-parse` at all. It is absent from the manual, +--- `--parseopt -z` errors with "unknown switch", and in ordinary mode +--- `rev-parse` treats `-z` as an unrecognized FLAG ARGUMENT and echoes a +--- literal `-z\n` onto stdout AHEAD of the toplevel --- so asking for it +--- would corrupt the very output it was meant to disambiguate, silently +--- and with exit code 0. `--show-toplevel` applies no C quoting either, +--- not even under `core.quotePath=true`. Removing the one byte git +--- appended is therefore the whole of the correct answer. +--- +--- Written as an explicit last-byte test rather than a pattern: an +--- anchored Lua pattern is where both of this function's bugs lived. local function strip_output_terminator(text) - return ((text or ""):gsub("\r?\n$", "")) + text = text or "" + if text:sub(-1) == "\n" then return text:sub(1, -2) end + return text end --- A one-line description of why a git invocation failed. @@ -815,9 +837,11 @@ function pmacs.git._deliver_root(request, res) end return end - -- The WHOLE output, minus its terminator --- never the first line. A - -- repository root may contain a newline, and truncating one here would - -- point every command that follows at a directory that does not exist. + -- The WHOLE output, minus its terminator --- never the first line, and + -- never a byte more than git appended. A repository root may contain a + -- newline and may END in a carriage return, and losing either here + -- would point every command that follows at a directory that does not + -- exist. local root = strip_output_terminator(res.stdout) if root == "" then pmacs.editor.set_status("git: rev-parse returned no worktree root") diff --git a/tests/git_status_stage1_acceptance.rs b/tests/git_status_stage1_acceptance.rs index 9099cac..b8eacb6 100644 --- a/tests/git_status_stage1_acceptance.rs +++ b/tests/git_status_stage1_acceptance.rs @@ -1654,32 +1654,27 @@ fn g6_14_the_root_rule_works_where_project_kind_is_not_git() { ); } -/// A repository root containing a **newline** resolves **whole**, and -/// the status command really runs there. -/// -/// A newline is a legal byte in a POSIX path — the fixture below builds -/// one and `git rev-parse --show-toplevel` prints it, terminator and -/// all — so parsing that output with a first-line match truncates -/// `/tmp/…/nl\nroot` to `/tmp/…/nl`, and every command this module runs -/// afterwards gets a `-C` and a cwd naming a directory that does not -/// exist. The right answer is to strip git's final terminator and -/// nothing else. +/// A repository rooted at a directory literally named `leaf` resolves +/// **whole**, and the status command really runs there. /// /// End to end, not at the parser: the directory really is created, the /// real `git` really resolves it, and the assertion is on the cwd of the /// spawn the module actually made. `_last_spawn` is the status /// invocation here, since `rev-parse` runs first and carries no cwd of -/// its own. +/// its own. A root that lost a byte would still SPAWN — with a `-C` and +/// a cwd naming a directory that does not exist — so the panel is +/// checked for a failure row as well as for real ones. /// -/// It rides beside the one-line-status rule rather than replacing it: -/// the helper this uses is deliberately **separate** from `first_line`, -/// whose other three callers all feed the single-line status band and -/// would be corrupted by a multi-line message. -#[test] -fn g6_14c_a_root_containing_a_newline_is_not_truncated() { +/// `open_panel` binds `pmacs.project.set_search_boundary` to the fixture +/// (R8's lesson), which matters most here: these leaf names are exactly +/// the shape that makes a detection walk out of the tempdir hard to +/// read when it goes wrong. +fn assert_root_resolves_whole(leaf: &str) { let (_dir, base) = tempdir(); - let root = base.join("nl\nroot"); - std::fs::create_dir_all(&root).expect("a newline is a legal POSIX path byte"); + let root = base.join(leaf); + std::fs::create_dir_all(&root).unwrap_or_else(|e| { + panic!("a root named {leaf:?} must be creatable — every byte in it is a legal POSIX path byte: {e}") + }); mixed_repo(&root); let mut s = editor(); @@ -1689,7 +1684,7 @@ fn g6_14c_a_root_containing_a_newline_is_not_truncated() { assert_eq!( cwd, root.display().to_string(), - "the resolved root must be the WHOLE path, newline included" + "the resolved root must be the WHOLE path, every byte of {leaf:?} included" ); let text = panel_text(&s); @@ -1714,6 +1709,47 @@ fn g6_14c_a_root_containing_a_newline_is_not_truncated() { ); } +/// A repository root containing a **newline** resolves **whole**, and +/// the status command really runs there. +/// +/// A newline is a legal byte in a POSIX path — the fixture builds one +/// and `git rev-parse --show-toplevel` prints it, terminator and all — +/// so parsing that output with a first-line match truncates +/// `/tmp/…/nl\nroot` to `/tmp/…/nl`, and every command this module runs +/// afterwards gets a `-C` and a cwd naming a directory that does not +/// exist. The right answer is to strip git's final terminator and +/// nothing else. +/// +/// It rides beside the one-line-status rule rather than replacing it: +/// the helper this uses is deliberately **separate** from `first_line`, +/// whose other three callers all feed the single-line status band and +/// would be corrupted by a multi-line message. +#[test] +fn g6_14c_a_root_containing_a_newline_is_not_truncated() { + assert_root_resolves_whole("nl\nroot"); +} + +/// A repository root ending in a **carriage return** resolves whole too +/// — the byte the newline fix's own strip still ate. +/// +/// `\r` is as legal in a POSIX directory name as `\n` is, and it is the +/// byte that makes `\r?\n$` ambiguous: for a root named `trailing\r`, +/// git prints `…/trailing` `0d` `0a`, where the `0d` is the PATH and +/// only the `0a` is the terminator. A strip tolerant of an optional +/// preceding carriage return cannot tell those apart and takes both, +/// resolving the root as `…/trailing` — a directory that does not +/// exist. Only git's final `\n` may be removed. +/// +/// The second case sends the two hazards in together, because a root may +/// hold both and neither fix may mask the other: an embedded newline +/// (which forbids a first-line read) ahead of a trailing carriage return +/// (which forbids an over-eager terminator strip). +#[test] +fn g6_14d_a_root_ending_in_a_carriage_return_is_not_truncated() { + assert_root_resolves_whole("trailing\r"); + assert_root_resolves_whole("nl\nand-trailing\r"); +} + /// A directory outside any repository reports it, rather than opening /// an empty panel or saying nothing. #[test]