More refactoring on partial commit graph module

Summary:
Minor changes

- Extracting the logic to get all relevant changesets for a path to a single function, because in the latter diffs each path might have a different changeset context as its head (i.e. the diff from which we start the history lookup).
- Defining a data type for the output of the partial commit graph module, instead of using a tuple.

Reviewed By: mitrandir77

Differential Revision: D49270520

fbshipit-source-id: d48db4a9c6b4ec6a0bcf592975422f29ec83d657

eden/mononoke/git/gitexport/src/gitexport_tools/lib.rs

@@ -47,7 +47,8 @@ use sql::rusqlite::Connection as SqliteConnection;
 use test_repo_factory::TestRepoFactory;
 
 pub use crate::partial_commit_graph::build_partial_commit_graph_for_export;
-pub use crate::partial_commit_graph::ChangesetParents;
+use crate::partial_commit_graph::ChangesetParents;
+pub use crate::partial_commit_graph::GitExportGraphInfo;
 
 pub const MASTER_BOOKMARK: &str = "master";
 
@@ -57,12 +58,12 @@ pub const MASTER_BOOKMARK: &str = "master";
 pub async fn rewrite_partial_changesets(
     fb: FacebookInit,
     source_repo_ctx: RepoContext,
-    changesets: Vec<ChangesetContext>,
-    changeset_parents: &ChangesetParents,
+    graph_info: GitExportGraphInfo,
     export_paths: Vec<NonRootMPath>,
 ) -> Result<RepoContext> {
     let ctx: &CoreContext = source_repo_ctx.ctx();
-
+    let changesets = graph_info.changesets;
+    let changeset_parents = &graph_info.parents_map;
     let logger = ctx.logger();
 
     info!(logger, "Copying changesets to temporary repo...");

eden/mononoke/git/gitexport/src/gitexport_tools/partial_commit_graph.rs

@@ -14,7 +14,6 @@ use futures::stream::StreamExt;
 use futures::stream::TryStreamExt;
 use futures::stream::{self};
 use itertools::Itertools;
-use mononoke_api::changeset_path::ChangesetPathHistoryContext;
 use mononoke_api::changeset_path::ChangesetPathHistoryOptions;
 use mononoke_api::ChangesetContext;
 use mononoke_types::ChangesetId;
@@ -25,7 +24,12 @@ use slog::trace;
 use slog::Logger;
 
 pub type ChangesetParents = HashMap<ChangesetId, Vec<ChangesetId>>;
-pub type PartialGraphInfo = (Vec<ChangesetContext>, ChangesetParents);
+
+#[derive(Debug)]
+pub struct GitExportGraphInfo {
+    pub changesets: Vec<ChangesetContext>,
+    pub parents_map: ChangesetParents,
+}
 
 /// Given a list of paths and a changeset, return a commit graph
 /// containing only commits that are ancestors of the changeset and have
@@ -39,32 +43,21 @@ pub async fn build_partial_commit_graph_for_export(
     // Consider history until the provided timestamp, i.e. all commits in the
     // graph will have its creation time greater than or equal to it.
     oldest_commit_ts: Option<i64>,
-) -> Result<PartialGraphInfo> {
+) -> Result<GitExportGraphInfo> {
     info!(logger, "Building partial commit graph for export...");
 
-    let cs_path_hist_ctxs: Vec<ChangesetPathHistoryContext> = stream::iter(paths)
-        .then(|p| async { cs_ctx.path_with_history(p).await })
-        .try_collect::<Vec<_>>()
-        .await?;
-
     let cs_path_history_options = ChangesetPathHistoryOptions {
         follow_history_across_deletions: true,
         until_timestamp: oldest_commit_ts,
         ..Default::default()
     };
 
-    // Get each path's history as a vector of changesets
-    let history_changesets: Vec<Vec<ChangesetContext>> = try_join_all(
-        try_join_all(
-            cs_path_hist_ctxs
-                .iter()
-                .map(|csphc| csphc.history(cs_path_history_options)),
-        )
-        .await?
-        .into_iter()
-        .map(|stream| stream.try_collect()),
-    )
-    .await?;
+    let history_changesets: Vec<Vec<ChangesetContext>> = stream::iter(paths)
+        .then(|p| async {
+            get_relevant_changesets_for_single_path(p, &cs_ctx, &cs_path_history_options).await
+        })
+        .try_collect::<Vec<_>>()
+        .await?;
 
     let (sorted_changesets, parents_map) =
         merge_cs_lists_and_build_parents_map(logger, history_changesets).await?;
@@ -80,7 +73,28 @@ pub async fn build_partial_commit_graph_for_export(
     trace!(logger, "changeset messages: {0:#?}", cs_msgs);
 
     info!(logger, "Partial commit graph built!");
-    Ok((sorted_changesets, parents_map))
+    Ok(GitExportGraphInfo {
+        parents_map,
+        changesets: sorted_changesets,
+    })
+}
+
+/// Get all changesets that affected the provided path up to a specific head
+/// commit.
+async fn get_relevant_changesets_for_single_path(
+    path: NonRootMPath,
+    head_cs: &ChangesetContext,
+    cs_path_history_opts: &ChangesetPathHistoryOptions,
+) -> Result<Vec<ChangesetContext>> {
+    let cs_path_hist_ctx = head_cs.path_with_history(path).await?;
+
+    let changesets: Vec<ChangesetContext> = cs_path_hist_ctx
+        .history(*cs_path_history_opts)
+        .await?
+        .try_collect()
+        .await?;
+
+    Ok(changesets)
 }
 
 /// Given a list of changeset lists, merge, dedupe and sort them topologically

eden/mononoke/git/gitexport/src/main.rs

@@ -144,19 +144,19 @@ async fn async_main(app: MononokeApp) -> Result<(), Error> {
         .map(|p| TryFrom::try_from(p.as_os_str()))
         .collect::<Result<Vec<NonRootMPath>>>()?;
 
-    let (changesets, cs_parents) = build_partial_commit_graph_for_export(
+    let graph_info = build_partial_commit_graph_for_export(
         logger,
         export_paths.clone(),
         cs_ctx,
         args.oldest_commit_ts,
     )
     .await?;
 
-    trace!(logger, "changesets: {:#?}", changesets);
-    trace!(logger, "changeset parents: {:?}", cs_parents);
+    trace!(logger, "changesets: {:#?}", &graph_info.changesets);
+    trace!(logger, "changeset parents: {:?}", &graph_info.parents_map);
 
     let temp_repo_ctx =
-        rewrite_partial_changesets(app.fb, repo_ctx, changesets, &cs_parents, export_paths).await?;
+        rewrite_partial_changesets(app.fb, repo_ctx, graph_info, export_paths).await?;
 
     let temp_master_csc = temp_repo_ctx
         .resolve_bookmark(

eden/mononoke/git/gitexport/tests/gitexport_tools/commit_rewrite/lib.rs

@@ -18,6 +18,7 @@ use anyhow::Result;
 use fbinit::FacebookInit;
 use futures::future::try_join_all;
 use gitexport_tools::rewrite_partial_changesets;
+use gitexport_tools::GitExportGraphInfo;
 use gitexport_tools::MASTER_BOOKMARK;
 use mononoke_api::BookmarkFreshness;
 use mononoke_api::BookmarkKey;
@@ -71,11 +72,15 @@ async fn test_rewrite_partial_changesets(fb: FacebookInit) -> Result<(), Error>
         (J, vec![I]),
     ]);
 
+    let graph_info = GitExportGraphInfo {
+        parents_map: relevant_changeset_parents,
+        changesets: relevant_changesets.clone(),
+    };
+
     let temp_repo_ctx = rewrite_partial_changesets(
         fb,
         source_repo_ctx.clone(),
-        relevant_changesets.clone(),
-        &relevant_changeset_parents,
+        graph_info,
         vec![export_dir.clone(), second_export_dir.clone()],
     )
     .await?;
@@ -123,15 +128,20 @@ async fn test_rewriting_fails_with_irrelevant_changeset(fb: FacebookInit) -> Res
     let broken_changeset_parents =
         HashMap::from([(A, vec![]), (C, vec![A]), (D, vec![C]), (E, vec![D])]);
 
+    let graph_info = GitExportGraphInfo {
+        parents_map: broken_changeset_parents.clone(),
+        changesets: broken_changeset_list,
+    };
+
     let error = rewrite_partial_changesets(
         fb,
         source_repo_ctx.clone(),
-        broken_changeset_list.clone(),
-        &broken_changeset_parents,
+        graph_info,
         vec![export_dir.clone()],
     )
     .await
     .unwrap_err();
+
     assert_eq!(
         error.to_string(),
         "internal error: Commit wasn't rewritten because it had no signficant changes"

eden/mononoke/git/gitexport/tests/gitexport_tools/partial_commit_graph/lib.rs

@@ -64,17 +64,18 @@ async fn test_partial_commit_graph_for_single_export_path(fb: FacebookInit) -> R
         .await?
         .ok_or(anyhow!("Couldn't find master bookmark in source repo."))?;
 
-    let (relevant_css, parents_map) =
+    let graph_info =
         build_partial_commit_graph_for_export(&logger, vec![export_dir], master_cs, None).await?;
 
-    let relevant_cs_ids = relevant_css
+    let relevant_cs_ids = graph_info
+        .changesets
         .iter()
         .map(ChangesetContext::id)
         .collect::<Vec<_>>();
 
     assert_eq!(expected_cs_ids, relevant_cs_ids);
 
-    assert_eq!(expected_parent_map, parents_map);
+    assert_eq!(expected_parent_map, graph_info.parents_map);
 
     Ok(())
 }
@@ -172,22 +173,23 @@ async fn test_partial_commit_graph_for_multiple_export_paths(fb: FacebookInit) -
         .await?
         .ok_or(anyhow!("Couldn't find master bookmark in source repo."))?;
 
-    let (relevant_css, parents_map) = build_partial_commit_graph_for_export(
+    let graph_info = build_partial_commit_graph_for_export(
         &logger,
         vec![export_dir, second_export_dir],
         master_cs,
         None,
     )
     .await?;
 
-    let relevant_cs_ids = relevant_css
+    let relevant_cs_ids = graph_info
+        .changesets
         .iter()
         .map(ChangesetContext::id)
         .collect::<Vec<_>>();
 
     assert_eq!(expected_cs_ids, relevant_cs_ids);
 
-    assert_eq!(expected_parent_map, parents_map);
+    assert_eq!(expected_parent_map, graph_info.parents_map);
 
     Ok(())
 }
@@ -245,22 +247,23 @@ async fn test_oldest_commit_ts_option(fb: FacebookInit) -> Result<()> {
 
     let oldest_ts = fifth_cs.author_date().await?.timestamp();
 
-    let (relevant_css, parents_map) = build_partial_commit_graph_for_export(
+    let graph_info = build_partial_commit_graph_for_export(
         &logger,
         vec![export_dir, second_export_dir],
         master_cs,
         Some(oldest_ts),
     )
     .await?;
 
-    let relevant_cs_ids = relevant_css
+    let relevant_cs_ids = graph_info
+        .changesets
         .iter()
         .map(ChangesetContext::id)
         .collect::<Vec<_>>();
 
     assert_eq!(expected_cs_ids, relevant_cs_ids);
 
-    assert_eq!(expected_parent_map, parents_map);
+    assert_eq!(expected_parent_map, graph_info.parents_map);
 
     Ok(())
 }