blame: make replacement parent lookup fallible

Summary: Blame derivation should arrange for all interesting commits to be available in the map of replacement parents.  However, it seems this might not always be the case.  Make the lookup fallible so that we fail derivation (rather than crashing) and allow us to find the cause more easily.

Reviewed By: mzr

Differential Revision: D75864222

fbshipit-source-id: a09a2749f5a24403c4b1861f1e4cb0de94c8d337

Author: markbt

eden/mononoke/mononoke_types/BUCK

@@ -37,6 +37,7 @@ rust_library(
         "fbsource//third-party/rust:derivative",
         "fbsource//third-party/rust:either",
         "fbsource//third-party/rust:faster-hex",
+        "fbsource//third-party/rust:fn-error-context",
         "fbsource//third-party/rust:futures",
         "fbsource//third-party/rust:gix-date",
         "fbsource//third-party/rust:gix-hash",

eden/mononoke/mononoke_types/Cargo.toml

@@ -30,6 +30,7 @@ derived_data_type_if = { version = "0.1.0", path = "../derived_data/if" }
 either = "1.5"
 faster-hex = "0.6.1"
 fbthrift = { version = "0.0.1+unstable", git = "https://github.com/facebook/fbthrift.git", branch = "main" }
+fn-error-context = "0.2"
 futures = { version = "0.3.30", features = ["async-await", "compat"] }
 futures_watchdog = { version = "0.1.0", path = "../common/futures_watchdog" }
 gix-date = "0.10.1"

eden/mononoke/mononoke_types/src/blame_v2.rs

@@ -26,6 +26,7 @@ use blobstore::Loadable;
 use blobstore::LoadableError;
 use context::CoreContext;
 use fbthrift::compact_protocol;
+use fn_error_context::context;
 use futures_watchdog::WatchdogExt;
 use sorted_vector_map::SortedVectorMap;
 use thiserror::Error;
@@ -321,19 +322,25 @@ pub enum BlameParentId<Id> {
 impl<Id: Eq + Hash> BlameParentId<Id> {
     /// Convert this blame parent into an indexed blame parent, using the provided
     /// mapping from changeset id to index.
-    pub fn indexed<IdKey: Eq + Hash>(
+    pub fn indexed<IdKey: Eq + Hash + std::fmt::Debug>(
         &self,
         replacement_parent_indexes: &HashMap<IdKey, u32>,
-    ) -> BlameParentIndex
+    ) -> Result<BlameParentIndex>
     where
         Id: Borrow<IdKey>,
     {
         match self {
             BlameParentId::ChangesetParent(index) => {
-                BlameParentIndex::ChangesetParent(*index as u32)
+                Ok(BlameParentIndex::ChangesetParent(*index as u32))
             }
             BlameParentId::ReplacementParent(csid) => {
-                BlameParentIndex::ReplacementParent(replacement_parent_indexes[csid.borrow()])
+                let csid = csid.borrow();
+                Ok(BlameParentIndex::ReplacementParent(
+                    replacement_parent_indexes
+                        .get(csid)
+                        .copied()
+                        .ok_or_else(|| anyhow!("No replacement parent index for {csid:?}"))?,
+                ))
             }
         }
     }
@@ -626,6 +633,7 @@ impl BlameData {
     }
 
     /// Merge other blame data into this blame for a merge changeset.
+    #[context("Failed to merge blame data for {merge_csid}")]
     fn merge(&mut self, merge_csid: ChangesetId, others: &[BlameData]) -> Result<()> {
         // Remove the csid index for the merge changeset; we will re-assign it
         // once all other blames have been merged in.
@@ -708,7 +716,14 @@ impl BlameData {
         for blame_line in self.merge_lines(merge_csid, others) {
             let path_index = path_indexes[blame_line.path];
             let csid_index = csid_indexes[blame_line.changeset_id];
-            let parent = blame_line.parent(&path_indexes, &replacement_parent_indexes);
+            let parent = blame_line
+                .parent(&path_indexes, &replacement_parent_indexes)
+                .with_context(|| {
+                    format!(
+                        "Failed to get parent for blame range at offset {}",
+                        blame_line.offset
+                    )
+                })?;
             match merged_ranges.last_mut() {
                 None if blame_line.offset != 0 => {
                     return Err(anyhow!(
@@ -962,6 +977,7 @@ impl BlameData {
         Ok(result)
     }
 
+    #[context("Failed to apply mutable change to blame")]
     fn apply_mutable_change(&mut self, original_blame: &Self, mutated_blame: &Self) -> Result<()> {
         // Sort order of changesets is not guaranteed to be the same, which rules out Iterator::eq
         let my_csids: HashSet<_> = self.csids.values().collect();
@@ -1116,10 +1132,18 @@ impl BlameData {
                             format!("Unknown path {} - should not be possible", line.path)
                         })?;
                         let offset = offset as u32;
-                        let parent = line.parent.map(|parent| {
-                            parent
-                                .to_range_parent_index(&path_to_index, &replacement_parent_to_index)
-                        });
+                        let parent = line
+                            .parent
+                            .map(|parent| {
+                                parent.to_range_parent_index(
+                                    &path_to_index,
+                                    &replacement_parent_to_index,
+                                )
+                            })
+                            .transpose()
+                            .with_context(|| {
+                                format!("Failed to get parent index for range at offset {offset}")
+                            })?;
 
                         let range = BlameRangeIndex {
                             offset,
@@ -1391,13 +1415,13 @@ impl<'a> BlameLineParent<'a> {
         &self,
         path_indexes: &HashMap<NonRootMPath, u32>,
         replacement_parent_indexes: &HashMap<&ChangesetId, u32>,
-    ) -> BlameRangeParentIndex {
-        BlameRangeParentIndex {
-            parent_index: self.parent.indexed(replacement_parent_indexes),
+    ) -> Result<BlameRangeParentIndex> {
+        Ok(BlameRangeParentIndex {
+            parent_index: self.parent.indexed(replacement_parent_indexes)?,
             offset: self.offset,
             length: self.length,
             renamed_from_path_index: self.renamed_from_path.map(|p| path_indexes[p]),
-        }
+        })
     }
 
     /// Returns true if this parent matches the parent of another range.
@@ -1464,15 +1488,25 @@ impl<'a> BlameLine<'a> {
         &self,
         path_indexes: &HashMap<NonRootMPath, u32>,
         replacement_parent_indexes: &HashMap<&ChangesetId, u32>,
-    ) -> Option<BlameRangeParentIndex> {
+    ) -> Result<Option<BlameRangeParentIndex>> {
         self.parent
             .as_ref()
-            .map(|parent_range| BlameRangeParentIndex {
-                parent_index: parent_range.parent.indexed(replacement_parent_indexes),
-                offset: parent_range.offset,
-                length: parent_range.length,
-                renamed_from_path_index: parent_range.renamed_from_path.map(|p| path_indexes[p]),
+            .map(|parent_range| {
+                Ok(BlameRangeParentIndex {
+                    parent_index: parent_range.parent.indexed(replacement_parent_indexes)?,
+                    offset: parent_range.offset,
+                    length: parent_range.length,
+                    renamed_from_path_index: parent_range
+                        .renamed_from_path
+                        .map(|p| {
+                            path_indexes.get(p).copied().ok_or_else(|| {
+                                anyhow!("Failed to get path index for renamed-from path {p}")
+                            })
+                        })
+                        .transpose()?,
+                })
             })
+            .transpose()
     }
 }
 

eden/mononoke/scs/scs_methods/src/methods/commit_path.rs

@@ -378,66 +378,71 @@ impl SourceControlServiceImpl {
             .map_err(|e| MononokeError::InvalidRequest(e.to_string()))?;
         let lines = stream::iter(lines.enumerate())
             .yield_periodically()
-            .map(|(line, blame_line)| -> Result<_, thrift::RequestError> {
-                let commit_id_index =
-                    commit_id_indexes
+            .map(
+                |(line, blame_line)| -> Result<_, scs_errors::ServiceError> {
+                    let commit_id_index = commit_id_indexes
                         .get(blame_line.changeset_id)
                         .ok_or_else(|| {
                             scs_errors::commit_not_found(format!(
                                 "failed to resolve commit: {}",
                                 blame_line.changeset_id
                             ))
                         })?;
-                let (author, date, message, title) =
-                    info.get(blame_line.changeset_id).ok_or_else(|| {
-                        scs_errors::commit_not_found(format!(
-                            "failed to resolve commit: {}",
-                            blame_line.changeset_id
-                        ))
-                    })?;
-                let mut thrift_blame_line = thrift::BlameCompactLine {
-                    line: (line + 1) as i32,
-                    contents: None,
-                    commit_id_index: *commit_id_index as i32,
-                    path_index: paths.insert(&blame_line.path.to_string()) as i32,
-                    author_index: authors.insert(author) as i32,
-                    date_index: dates.insert(Cow::Borrowed(date)) as i32,
-                    origin_line: (blame_line.origin_offset + 1) as i32,
-                    title_index: None,
-                    message_index: None,
-                    ..Default::default()
-                };
-                if option_include_contents {
-                    if let Some(content_line) = content_iter.next() {
-                        thrift_blame_line.contents =
-                            Some(String::from_utf8_lossy(content_line).into_owned());
+                    let (author, date, message, title) =
+                        info.get(blame_line.changeset_id).ok_or_else(|| {
+                            scs_errors::commit_not_found(format!(
+                                "failed to resolve commit: {}",
+                                blame_line.changeset_id
+                            ))
+                        })?;
+                    let mut thrift_blame_line = thrift::BlameCompactLine {
+                        line: (line + 1) as i32,
+                        contents: None,
+                        commit_id_index: *commit_id_index as i32,
+                        path_index: paths.insert(&blame_line.path.to_string()) as i32,
+                        author_index: authors.insert(author) as i32,
+                        date_index: dates.insert(Cow::Borrowed(date)) as i32,
+                        origin_line: (blame_line.origin_offset + 1) as i32,
+                        title_index: None,
+                        message_index: None,
+                        ..Default::default()
+                    };
+                    if option_include_contents {
+                        if let Some(content_line) = content_iter.next() {
+                            thrift_blame_line.contents =
+                                Some(String::from_utf8_lossy(content_line).into_owned());
+                        }
                     }
-                }
-                if option_include_title {
-                    thrift_blame_line.title_index = Some(titles.insert(title) as i32);
-                }
-                if option_include_message {
-                    thrift_blame_line.message_index = Some(messages.insert(message) as i32);
-                }
-                if option_include_parent {
-                    if let Some(parent) = &blame_line.parent {
-                        match parent.parent.indexed(&replacement_parents) {
-                            BlameParentIndex::ChangesetParent(index) => {
-                                thrift_blame_line.parent_index = Some(index as i32);
-                            }
-                            BlameParentIndex::ReplacementParent(index) => {
-                                thrift_blame_line.replacement_parent_index = Some(index as i32);
+                    if option_include_title {
+                        thrift_blame_line.title_index = Some(titles.insert(title) as i32);
+                    }
+                    if option_include_message {
+                        thrift_blame_line.message_index = Some(messages.insert(message) as i32);
+                    }
+                    if option_include_parent {
+                        if let Some(parent) = &blame_line.parent {
+                            match parent
+                                .parent
+                                .indexed(&replacement_parents)
+                                .map_err(scs_errors::internal_error)?
+                            {
+                                BlameParentIndex::ChangesetParent(index) => {
+                                    thrift_blame_line.parent_index = Some(index as i32);
+                                }
+                                BlameParentIndex::ReplacementParent(index) => {
+                                    thrift_blame_line.replacement_parent_index = Some(index as i32);
+                                }
                             }
+                            thrift_blame_line.parent_start_line = Some((parent.offset + 1) as i32);
+                            thrift_blame_line.parent_range_length = Some(parent.length as i32);
+                            thrift_blame_line.parent_path_index = parent
+                                .renamed_from_path
+                                .map(|path| paths.insert(&path.to_string()) as i32);
                         }
-                        thrift_blame_line.parent_start_line = Some((parent.offset + 1) as i32);
-                        thrift_blame_line.parent_range_length = Some(parent.length as i32);
-                        thrift_blame_line.parent_path_index = parent
-                            .renamed_from_path
-                            .map(|path| paths.insert(&path.to_string()) as i32);
                     }
-                }
-                Ok(thrift_blame_line)
-            })
+                    Ok(thrift_blame_line)
+                },
+            )
             .try_collect::<Vec<_>>()
             .await?;