From 599df867a184aa1ce7b62ad8cad381174de90312 Mon Sep 17 00:00:00 2001 From: Daniel Scherzer Date: Mon, 25 May 2026 17:26:38 -0700 Subject: [PATCH] [Result migration] Don't return options for `StringArray` iterators Instead, treat a `None` as indicative of a bug in the code, and panic with a clear message that something went wrong. Also update the documentation of `StringArray::iter()` to reflect the correct iterated type. Fixes #1263 --- examples/tag.rs | 2 +- src/remote.rs | 2 +- src/string_array.rs | 34 ++++++++++++++++++++++++++-------- tests/add_extensions.rs | 12 ++++++------ tests/get_extensions.rs | 10 +++++----- tests/remove_extensions.rs | 2 +- 6 files changed, 40 insertions(+), 22 deletions(-) diff --git a/examples/tag.rs b/examples/tag.rs index 921863bee3..86fa9aca11 100644 --- a/examples/tag.rs +++ b/examples/tag.rs @@ -65,7 +65,7 @@ fn run(args: &Args) -> Result<(), Error> { } else if args.flag_list { let pattern = args.arg_pattern.as_ref().map(|s| &s[..]).unwrap_or("*"); for name in repo.tag_names(Some(pattern))?.iter() { - let name = name.expect("Not invalid utf8").expect("Not None"); + let name = name.expect("Not invalid utf8"); let obj = repo.revparse_single(name)?; if let Some(tag) = obj.as_tag() { diff --git a/src/remote.rs b/src/remote.rs index 0037b53975..448de0e1e2 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -886,7 +886,7 @@ mod tests { assert_eq!(remotes.len(), 1); assert_eq!(remotes.get(0), Ok(Some("origin"))); assert_eq!(remotes.iter().count(), 1); - assert_eq!(remotes.iter().next().unwrap(), Ok(Some("origin"))); + assert_eq!(remotes.iter().next().unwrap(), Ok("origin")); } origin.connect(Direction::Push).unwrap(); diff --git a/src/string_array.rs b/src/string_array.rs index 8d68616cfc..d320193ac3 100644 --- a/src/string_array.rs +++ b/src/string_array.rs @@ -52,8 +52,8 @@ impl StringArray { /// Returns an iterator over the strings contained within this array. /// - /// The iterator yields `Option<&str>` as it is unknown whether the contents - /// are utf-8 or not. + /// The iterator yields `Result<&str, Error>` as it is unknown whether the + /// contents are utf-8 or not. pub fn iter(&self) -> Iter<'_> { Iter { range: 0..self.len(), @@ -92,7 +92,7 @@ impl Binding for StringArray { } impl<'a> IntoIterator for &'a StringArray { - type Item = Result, Error>; + type Item = Result<&'a str, Error>; type IntoIter = Iter<'a>; fn into_iter(self) -> Self::IntoIter { self.iter() @@ -100,17 +100,35 @@ impl<'a> IntoIterator for &'a StringArray { } impl<'a> Iterator for Iter<'a> { - type Item = Result, Error>; - fn next(&mut self) -> Option, Error>> { - self.range.next().map(|i| self.arr.get(i)) + type Item = Result<&'a str, Error>; + fn next(&mut self) -> Option> { + // Convert from Result, Error> to just + // Result<&str, Error> - if Ok(None) was returned by self.arr.get() + // then something went wrong with the array and panicking is the + // right decision, see discussion on #1263. + self.range.next().map(|i| { + self.arr + .get(i) + .transpose() + .expect("StringArray should have values for all indices in its size") + }) } fn size_hint(&self) -> (usize, Option) { self.range.size_hint() } } impl<'a> DoubleEndedIterator for Iter<'a> { - fn next_back(&mut self) -> Option, Error>> { - self.range.next_back().map(|i| self.arr.get(i)) + fn next_back(&mut self) -> Option> { + // Convert from Result, Error> to just + // Result<&str, Error> - if Ok(None) was returned by self.arr.get() + // then something went wrong with the array and panicking is the + // right decision, see discussion on #1263. + self.range.next_back().map(|i| { + self.arr + .get(i) + .transpose() + .expect("StringArray should have values for all indices in its size") + }) } } impl<'a> FusedIterator for Iter<'a> {} diff --git a/tests/add_extensions.rs b/tests/add_extensions.rs index bf14a25cd7..bd455c3477 100644 --- a/tests/add_extensions.rs +++ b/tests/add_extensions.rs @@ -16,16 +16,16 @@ fn test_add_extensions() -> Result<(), Error> { assert_eq!( extensions, [ - Some("custom"), - Some("noop"), + "custom", + "noop", // The objectformat extension was added in 1.6 - Some("objectformat"), + "objectformat", // The preciousobjects extension was added in 1.9 - Some("preciousobjects"), + "preciousobjects", // The relativeworktrees extension was added in 1.9.4 - Some("relativeworktrees"), + "relativeworktrees", // The worktreeconfig extension was added in 1.8 - Some("worktreeconfig") + "worktreeconfig" ] ); diff --git a/tests/get_extensions.rs b/tests/get_extensions.rs index 731d98781a..b7dd110ca2 100644 --- a/tests/get_extensions.rs +++ b/tests/get_extensions.rs @@ -12,15 +12,15 @@ fn test_get_extensions() -> Result<(), Error> { assert_eq!( extensions, [ - Some("noop"), + "noop", // The objectformat extension was added in 1.6 - Some("objectformat"), + "objectformat", // The preciousobjects extension was added in 1.9 - Some("preciousobjects"), + "preciousobjects", // The relativeworktrees extension was added in 1.9.4 - Some("relativeworktrees"), + "relativeworktrees", // The worktreeconfig extension was added in 1.8 - Some("worktreeconfig") + "worktreeconfig" ] ); diff --git a/tests/remove_extensions.rs b/tests/remove_extensions.rs index 422cdf6a3c..0376e74ea1 100644 --- a/tests/remove_extensions.rs +++ b/tests/remove_extensions.rs @@ -22,7 +22,7 @@ fn test_remove_extensions() -> Result<(), Error> { let extensions: Result, Error> = extensions.iter().collect(); let extensions = extensions.unwrap(); - assert_eq!(extensions, [Some("custom"), Some("other")]); + assert_eq!(extensions, ["custom", "other"]); Ok(()) }