steid

docs: two commands read git's exit status, and both comments now say so

`grep` and `merge_base` were written on separate branches and each call site
claimed to be *the* exception to this module's "a non-zero exit is always a fault"
rule. After the merge both claims sit in one file, and a reader meeting the second
one has to work out which is wrong.

Neither is: the rule has exactly two exemptions, `run_allowing` is the only way
either reaches a command, and the module header now says that up front. The two
constants stay separate — what makes each legible is the question it answers, not
the number, and both are 1.

Comments only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J18ViwAfdswUCMb2DXJZFG

JamesPatrickGill authored 14 hours agoparentc3ca6daBrowse filese63c831485327203c4f7175e962876b21ca9ef48

1 file changed+27 −18

src/infrastructure/git_query.rs+27 −18View file

		@@ -27,6 +27,11 @@
27	27	//! The listing and content commands are only ever reached after `--batch-check` has
28	28	//! confirmed the object and its type, and are handed the resolved object id rather than
29	29	//! the user's revision, so their failure modes are genuinely faults.
	30	+//!
	31	+//! Two commands are exempt, and only those two: `git grep` and `git merge-base`,
	32	+//! whose exit 1 git's own manual promises as "matched nothing" and "no common ancestor".
	33	+//! Both go through [`run_allowing`], which is the only place a non-zero exit is ever
	34	+//! accepted, and each names the status it accepts at the call site.
30	35
31	36	use std::{
32	37	ffi::OsStr,
		@@ -405,10 +410,11 @@ impl GitQuery for DiskGitQuery {
405	410	) -> Result<Vec<GrepHit>, GitQueryError> {
406	411	let repo = self.repo_path(handle, name);
407	412
408		− // The one command here whose exit status is an answer: `git grep` exits 1
409		− // when it found nothing, which is not a failure. Everything else in this module
410		− // keeps the rule that a non-zero exit is a fault; this is the exception, and it
411		− // is spelled out in the call rather than hidden in the helper.
	413	+ // One of the two commands here whose exit status is an answer — the other
	414	+ // is `merge_base`. `git grep` exits 1 when it found nothing, which is not a
	415	+ // failure. Everything else in this module keeps the rule that a non-zero exit
	416	+ // is a fault; the exception is spelled out in the call rather than hidden in
	417	+ // the helper.
412	418	//
413	419	// `-F` fixed strings, `-I` skips binary files, `-n --column` locate the match,
414	420	// and `-z` makes the output parseable — see `parse_grep_output`. The query is
		@@ -526,12 +532,13 @@ impl GitQuery for DiskGitQuery {
526	532	) -> Result<Option<ObjectId>, GitQueryError> {
527	533	let repo = self.repo_path(handle, name);
528	534
529		− // The one exception to this module's rule that a non-zero exit is a fault.
530		− // `git merge-base` documents exit 1 as "no merge base found" and reserves 128
531		− // for real errors, so the two are distinguishable here in a way they are not
532		− // for `rev-parse` — which is why the rule exists at all. Two histories with no
533		− // common ancestor is a thing a compare page must be able to say, and the
534		− // arguments are resolved object ids, so there is nothing else exit 1 can mean.
	535	+ // **The other exception to this module's rule that a non-zero exit is a
	536	+ // fault**, `grep` being the first. `git merge-base` documents exit 1 as "no
	537	+ // merge base found" and reserves 128 for real errors, so the two are
	538	+ // distinguishable here in a way they are not for `rev-parse` — which is why the
	539	+ // rule exists at all. Two histories with no common ancestor is a thing a compare
	540	+ // page must be able to say, and the arguments are resolved object ids, so there
	541	+ // is nothing else exit 1 can mean.
535	542	let output = run_allowing(
536	543	&repo,
537	544	[
		@@ -1133,13 +1140,15 @@ where
1133	1140	run_within(repo, args, GIT_TIMEOUT).await
1134	1141	}
1135	1142
	1143	+// The two exit statuses this module reads as answers rather than as faults. Both are 1
	1144	+// and they are separate constants on purpose: what makes each of them legible is the
	1145	+// question it answers, not the number. See [`run_allowing`], which is the only way
	1146	+// either reaches a command.
	1147	+
1136	1148	/// What `git grep` exits with when it matched nothing.
1137		−///
1138		−/// A value, not a failure — the one place in this module where git's exit status
1139		−/// carries an answer. Named so the exception is legible at the call site.
1140	1149	const NO_MATCHES: i32 = 1;
1141	1150
1142		−/// `git merge-base`'s exit status for two commits that share no ancestor — documented
	1151	+/// What `git merge-base` exits with for two commits that share no ancestor — documented
1143	1152	/// as an answer, with 128 reserved for real errors.
1144	1153	const NO_COMMON_ANCESTOR: i32 = 1;
1145	1154
		@@ -1147,10 +1156,10 @@ const NO_COMMON_ANCESTOR: i32 = 1;
1147	1156	///
1148	1157	/// Exists for `git grep` and `git merge-base` alone — grep's 1 is "matched nothing" and
1149	1158	/// merge-base's 1 is "no common ancestor", both promised by git's manual. Every other
1150		−/// command here is asked about something
1151		−/// `cat-file --batch-check` has already confirmed exists, which is what makes the
1152		−/// module's "a non-zero exit is always a fault" rule hold; grep is the one command
1153		−/// whose whole job is to find nothing sometimes.
	1159	+/// command here is asked about something `cat-file --batch-check` has already confirmed
	1160	+/// exists, which is what makes the module's "a non-zero exit is always a fault" rule
	1161	+/// hold. These two are asked questions whose answer can legitimately be "there is
	1162	+/// none".
1154	1163	async fn run_allowing<I, S>(repo: &Path, args: I, allowed: &[i32]) -> Result<Output, GitQueryError>
1155	1164	where
1156	1165	I: IntoIterator<Item = S>,