mononoke: add generate_placeholder_diff option

Summary:
Let's use functionality added in D20389226 so that we can generate
diffs even for large files.

I've contemplated between two approaches: either "silently" generate
placeholder diff for files that are over the limit or add a new option
where client can request these placeholders. I choose the latter for a few
reasons:

1) To me option #1 might cause surprises e.g. diffing a single large file
doesn't fail, but diffing two files whose size is (LIMIT / 2 + 1) will fail.
Option #2 let's the client be very explicit about what it needs - and it also
won't return a placeholder when the actual content is expected!
2) Option #2 makes the client think about what it wants to be returned,
and it seems in line with source control thrift API design in general i.e.
commit_file_diffs is not trivial to use by design to because force client to
think about edge cases. And it seems that forcing client to think about additional
important edge case (i.e. large file) makes sense.
3) The code change is simpler with option #2

I also thought about adding generate_placeholder_diff parameter to CommitFileDiffsParams,
but that makes integration with scs_helper.py harder.

Reviewed By: markbt

Differential Revision: D20417787

fbshipit-source-id: ab9b32fd7a4768043414ed7d8bf39e3c6f4eb53e

Author: StanislavGlebik

eden/mononoke/mononoke_api/src/changeset_path.rs

@@ -290,35 +290,62 @@ impl ChangesetPathContext {
     }
 }
 
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub enum UnifiedDiffMode {
+    Inline,
+    /// Content is not fetched - instead a placeholder diff like
+    ///
+    /// diff --git a/file.txt b/file.txt
+    /// Binary file file.txt has changed
+    ///
+    /// is generated
+    OmitContent,
+}
+
 /// Renders the diff (in the git diff format) against some other path.
 /// Provided with copy_info will render the diff as copy or move as requested.
-// (does not do the copy-tracking on its own) async fn unified_diff(
+/// (does not do the copy-tracking on its own)
+/// If `omit_content` is set then unified_diff(...) doesn't fetch content, but just
+/// generates a placeholder diff that says that files differ.
 pub async fn unified_diff(
     // The diff applied to old_path with produce new_path
     old_path: &Option<ChangesetPathContext>,
     new_path: &Option<ChangesetPathContext>,
     copy_info: CopyInfo,
     context_lines: usize,
+    mode: UnifiedDiffMode,
 ) -> Result<UnifiedDiff, MononokeError> {
     // Helper for getting file information.
     async fn get_file_data(
         path: &Option<ChangesetPathContext>,
+        mode: UnifiedDiffMode,
     ) -> Result<Option<xdiff::DiffFile<String, Bytes>>, MononokeError> {
         match path {
             Some(path) => {
                 if let Some(file_type) = path.file_type().await? {
                     let file = path.file().await?.ok_or_else(|| {
                         MononokeError::from(Error::msg("assertion error: file should exist"))
                     })?;
-                    let contents = file.content_concat().await?;
                     let file_type = match file_type {
                         FileType::Regular => xdiff::FileType::Regular,
                         FileType::Executable => xdiff::FileType::Executable,
                         FileType::Symlink => xdiff::FileType::Symlink,
                     };
+                    let contents = match mode {
+                        UnifiedDiffMode::Inline => {
+                            let contents = file.content_concat().await?;
+                            xdiff::FileContent::Inline(contents)
+                        }
+                        UnifiedDiffMode::OmitContent => {
+                            let content_id = file.metadata().await?.content_id;
+                            xdiff::FileContent::Omitted {
+                                content_hash: format!("{}", content_id),
+                            }
+                        }
+                    };
                     Ok(Some(xdiff::DiffFile {
                         path: path.path().to_string(),
-                        contents: xdiff::FileContent::Inline(contents),
+                        contents,
                         file_type,
                     }))
                 } else {
@@ -329,8 +356,10 @@ pub async fn unified_diff(
         }
     }
 
-    let (old_diff_file, new_diff_file) =
-        try_join!(get_file_data(&old_path), get_file_data(&new_path))?;
+    let (old_diff_file, new_diff_file) = try_join!(
+        get_file_data(&old_path, mode),
+        get_file_data(&new_path, mode)
+    )?;
     let is_binary = xdiff::file_is_binary(&old_diff_file) || xdiff::file_is_binary(&new_diff_file);
     let copy_info = match copy_info {
         CopyInfo::None => xdiff::CopyInfo::None,

eden/mononoke/mononoke_api/src/lib.rs

@@ -49,7 +49,7 @@ pub use crate::legacy::get_content_by_path;
 
 pub use crate::changeset::ChangesetContext;
 pub use crate::changeset_path::{
-    unified_diff, ChangesetPathContext, CopyInfo, PathEntry, UnifiedDiff,
+    unified_diff, ChangesetPathContext, CopyInfo, PathEntry, UnifiedDiff, UnifiedDiffMode,
 };
 pub use crate::changeset_path_diff::ChangesetPathDiffContext;
 pub use crate::errors::MononokeError;

eden/mononoke/scs_server/src/methods/commit.rs

@@ -9,7 +9,7 @@ use std::convert::{TryFrom, TryInto};
 
 use context::CoreContext;
 use futures_util::{future, stream, try_join, StreamExt, TryStreamExt};
-use mononoke_api::{unified_diff, ChangesetSpecifier, CopyInfo, MononokePath};
+use mononoke_api::{unified_diff, ChangesetSpecifier, CopyInfo, MononokePath, UnifiedDiffMode};
 use source_control as thrift;
 
 use crate::commit_id::{map_commit_identity, CommitIdExt};
@@ -78,6 +78,11 @@ impl SourceControlServiceImpl {
             .paths
             .into_iter()
             .map(|path_pair| {
+                let mode = if path_pair.generate_placeholder_diff.unwrap_or(false) {
+                    UnifiedDiffMode::OmitContent
+                } else {
+                    UnifiedDiffMode::Inline
+                };
                 Ok((
                     match path_pair.base_path {
                         Some(path) => Some(base_commit.path(&path)?),
@@ -88,14 +93,19 @@ impl SourceControlServiceImpl {
                         None => None,
                     },
                     CopyInfo::from_request(&path_pair.copy_info)?,
+                    mode,
                 ))
             })
             .collect::<Result<Vec<_>, errors::ServiceError>>()?;
 
         // Check the total file size limit
         let flat_paths = paths
             .iter()
-            .flat_map(|(base_path, other_path, _)| vec![base_path, other_path])
+            .filter_map(|(base_path, other_path, _, mode)| match mode {
+                UnifiedDiffMode::OmitContent => None,
+                UnifiedDiffMode::Inline => Some((base_path, other_path)),
+            })
+            .flat_map(|(base_path, other_path)| vec![base_path, other_path])
             .filter_map(|x| x.as_ref());
         let total_input_size: u64 = future::try_join_all(flat_paths.map(|path| async move {
             let r: Result<_, errors::ServiceError> = if let Some(file) = path.file().await? {
@@ -114,8 +124,9 @@ impl SourceControlServiceImpl {
         }
 
         let path_diffs = future::try_join_all(paths.into_iter().map(
-            |(base_path, other_path, copy_info)| async move {
-                let diff = unified_diff(&other_path, &base_path, copy_info, context_lines).await?;
+            |(base_path, other_path, copy_info, mode)| async move {
+                let diff =
+                    unified_diff(&other_path, &base_path, copy_info, context_lines, mode).await?;
                 let r: Result<_, errors::ServiceError> =
                     Ok(thrift::CommitFileDiffsResponseElement {
                         base_path: base_path.map(|p| p.path().to_string()),

eden/mononoke/tests/integration/test-scs-diff.t

@@ -0,0 +1,61 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License found in the LICENSE file in the root
+# directory of this source tree.
+
+  $ . "${TEST_FIXTURES}/library.sh"
+
+Setup config repo:
+  $ setup_common_config
+  $ cd "$TESTTMP"
+
+Setup testing repo for mononoke:
+  $ hg init repo-hg
+  $ cd repo-hg
+  $ setup_hg_server
+
+Helper for making commit:
+  $ function commit() { # the arg is used both for commit message and variable name
+  >   hg commit -Am $1 # create commit
+  >   export COMMIT_$1="$(hg --debug id -i)" # save hash to variable
+  > }
+
+First two simple commits and bookmark:
+  $ echo -e "a\nb\nc\nd\ne" > a
+  $ commit A
+  adding a
+
+  $ echo -e "a\nb\nd\ne\nf" > b
+  $ commit B
+  adding b
+
+A commit with a file change and binary file
+
+  $ echo -e "b\nc\nd\ne\nf" > b
+  $ echo -e "\0 10" > binary
+  $ commit C
+  adding binary
+
+import testing repo to mononoke
+  $ cd ..
+  $ blobimport repo-hg/.hg repo
+
+start SCS server
+  $ start_and_wait_for_scs_server --scuba-log-file "$TESTTMP/scuba.json"
+
+  $ scsc diff --repo repo --hg-commit-id "$COMMIT_A" --hg-commit-id "$COMMIT_B"
+  diff --git a/b b/b
+  new file mode 100644
+  --- /dev/null
+  +++ b/b
+  @@ -0,0 +1,5 @@
+  +a
+  +b
+  +d
+  +e
+  +f
+  $ scsc diff --repo repo --hg-commit-id "$COMMIT_A" --hg-commit-id "$COMMIT_B" --placeholders-only
+  diff --git a/b b/b
+  new file mode 100644
+  Binary file b has changed