wip

Author: benartuso

src/cli/src/cmd_setup.rs

@@ -805,40 +805,35 @@ pub fn pull() -> Command {
 
 pub fn diff() -> Command {
     Command::new(DIFF)
-        .about("Compare two files against each other or against versions. The first parameter can be one of three things 1) another file 2) a commit hash 3) a branch name. If the first parameter is a revision it will compare the second parameter path to that version of the file.")
-        .arg(Arg::new("FILE_OR_REVISION").required(true))
-        .arg(Arg::new("PATH").required(false))
-}
-
-pub fn compare() -> Command {
-    Command::new(COMPARE)
-        .about("Compare two tabular files with some schematic overlap. The two resource paramaters can be specified by filepath or `file:revision` syntax.")
+        .about("Compare two files against each other or against versions. The two resource paramaters can be specified by filepath or `file:revision` syntax.")
         .arg(Arg::new("RESOURCE1")
             .required(true)
             .help("First resource, in format `file` or `file:revision`")
             .index(1)
         )
         .arg(Arg::new("RESOURCE2")
-            .required(true)
-            .help("Second resource, in format `file` or `file:revision`")
+            .required(false)
+            .help("Second resource, in format `file` or `file:revision`. If left blank, RESOURCE1 will be compared with HEAD.")
             .index(2))
         .arg(Arg::new("keys")
-            .required(true)
+            .required(false)
             .long("keys")
+            .short("k")
             .help("Comma-separated list of columns to compare on. If not specified, all columns are used for comparison.")
             .use_value_delimiter(true)
             .action(clap::ArgAction::Set))
         .arg(Arg::new("targets")
-            .required(true)
+            .required(false)
             .long("targets")
+            .short("t")
             .help("Comma-separated list of columns in which to view changes. If not specified, all columns are viewed")
             .use_value_delimiter(true)
             .action(clap::ArgAction::Set))
         .arg(Arg::new("output")
             .required(false)
             .long("output")
             .short('o')
-            .help("Output directory path to write the results of the comparison. Will write both match.csv (rows with same keys and targets) and diff.csv (rows with different targets between files")
+            .help("Output directory path to write the results of the comparison. Will write both match.csv (rows with same keys and targets) and diff.csv (rows with different targets between files.")
             .action(clap::ArgAction::Set))
 }
 

src/cli/src/dispatch.rs

@@ -501,53 +501,84 @@ pub async fn pull(remote: &str, branch: &str, all: bool) -> Result<(), OxenError
     Ok(())
 }
 
-pub async fn diff(commit_id: Option<&str>, path: &str, remote: bool) -> Result<(), OxenError> {
-    let repo_dir = env::current_dir().unwrap();
-    let repository = LocalRepository::from_dir(&repo_dir)?;
-    let path = Path::new(path);
-
-    let result = if remote {
-        command::remote::diff(&repository, commit_id, path).await?
-    } else {
-        command::diff(&repository, commit_id, path)?
-    };
-    println!("{result}");
-    Ok(())
-}
-
-pub fn compare(
+#[allow(clippy::too_many_arguments)]
+pub async fn diff(
     file_1: PathBuf,
     revision_1: Option<&str>,
-    file_2: PathBuf,
+    file_2: Option<PathBuf>,
     revision_2: Option<&str>,
     keys: Vec<String>,
     targets: Vec<String>,
     output: Option<PathBuf>,
+    is_remote: bool,
 ) -> Result<(), OxenError> {
     let repo_dir = env::current_dir().unwrap();
     let repository = LocalRepository::from_dir(&repo_dir)?;
     check_repo_migration_needed(&repository)?;
 
     let current_commit = api::local::commits::head_commit(&repository)?;
     // For revision_1 and revision_2, if none, set to current_commit
-    let revision_1 = revision_1.unwrap_or(current_commit.id.as_str());
-    let revision_2 = revision_2.unwrap_or(current_commit.id.as_str());
+    // let revision_1 = revision_1.unwrap_or(current_commit.id.as_str());
+    // let revision_2 = revision_2.unwrap_or(current_commit.id.as_str());
 
     let commit_1 = api::local::revisions::get(&repository, revision_1)?
         .ok_or_else(|| OxenError::revision_not_found(revision_1.into()))?;
     let commit_2 = api::local::revisions::get(&repository, revision_2)?
         .ok_or_else(|| OxenError::revision_not_found(revision_2.into()))?;
 
-    let cpath_1 = CommitPath {
-        commit: commit_1,
-        path: file_1,
+    // TODONOW: might be able to clean this logic up - pull out into function so we can early return and be less confusing
+    let (cpath_1, cpath_2) = if let Some(file_2) = file_2 {
+        let cpath_1 = if let Some(revison) = revision_1 {
+            let commit_1 = api::local::revisions::get(&repository, revison)?;
+            CommitPath {
+                commit: commit_1,
+                path: file_1.clone(),
+            }
+        } else {
+            CommitPath {
+                commit: None,
+                path: file_1.clone(),
+            }
+        };
+
+        let cpath_2 = if let Some(revison) = revision_2 {
+            let commit = api::local::revisions::get(&repository, revison)?;
+
+            CommitPath {
+                commit,
+                path: file_2,
+            }
+        } else {
+            CommitPath {
+                commit: None,
+                path: file_2,
+            }
+        };
+
+        (cpath_1, cpath_2)
+    } else {
+        // If no file2, compare with file1 at head.
+        let commit = Some(api::local::commits::head_commit(&repository)?);
+
+        (
+            CommitPath {
+                commit: None,
+                path: file_1.clone(),
+            },
+            CommitPath {
+                commit,
+                path: file_1.clone(),
+            },
+        )
     };
-    let cpath_2 = CommitPath {
-        commit: commit_2,
-        path: file_2,
+
+    let result = if is_remote {
+        command::remote::diff(&repository, revision_1, &file_1).await?
+    } else {
+        command::compare(&repository, cpath_1, cpath_2, keys, targets, output)?
     };
 
-    command::compare(&repository, cpath_1, cpath_2, keys, targets, output)?;
+    println!("{result}");
     Ok(())
 }
 

src/cli/src/main.rs

@@ -21,7 +21,6 @@ async fn main() {
         .subcommand(cmd_setup::clone())
         .subcommand(cmd_setup::commit_cache())
         .subcommand(cmd_setup::commit())
-        .subcommand(cmd_setup::compare())
         .subcommand(cmd_setup::config())
         .subcommand(cmd_setup::create_remote())
         .subcommand(cmd_setup::df())
@@ -56,13 +55,12 @@ async fn main() {
             parse_and_run::compute_commit_cache(sub_matches).await
         }
         Some((cmd_setup::COMMIT, sub_matches)) => parse_and_run::commit(sub_matches).await,
-        Some((cmd_setup::COMPARE, sub_matches)) => parse_and_run::compare(sub_matches).await,
+        Some((cmd_setup::DIFF, sub_matches)) => parse_and_run::diff(sub_matches).await,
         Some((cmd_setup::CONFIG, sub_matches)) => parse_and_run::config(sub_matches),
         Some((cmd_setup::CREATE_REMOTE, sub_matches)) => {
             parse_and_run::create_remote(sub_matches).await
         }
         Some((cmd_setup::DF, sub_matches)) => parse_and_run::df(sub_matches),
-        Some((cmd_setup::DIFF, sub_matches)) => parse_and_run::diff(sub_matches).await,
         Some((cmd_setup::DOWNLOAD, sub_matches)) => parse_and_run::download(sub_matches).await,
         Some((cmd_setup::INIT, sub_matches)) => parse_and_run::init(sub_matches).await,
         Some((cmd_setup::INFO, sub_matches)) => parse_and_run::info(sub_matches),

src/cli/src/parse_and_run.rs

@@ -943,72 +943,82 @@ pub async fn pull(sub_matches: &ArgMatches) {
     }
 }
 
-pub async fn remote_diff(sub_matches: &ArgMatches) {
+pub async fn diff(sub_matches: &ArgMatches) {
     let is_remote = true;
     p_diff(sub_matches, is_remote).await
 }
 
-pub async fn compare(sub_matches: &ArgMatches) {
+async fn p_diff(sub_matches: &ArgMatches, is_remote: bool) {
     let resource1 = sub_matches
         .get_one::<String>("RESOURCE1")
         .expect("required");
-    let resource2 = sub_matches
-        .get_one::<String>("RESOURCE2")
-        .expect("required");
+    let resource2 = sub_matches.get_one::<String>("RESOURCE2");
 
     let (file1, revision1) = parse_file_and_revision(resource1);
-    let (file2, revision2) = parse_file_and_revision(resource2);
 
     let file1 = PathBuf::from(file1);
-    let file2 = PathBuf::from(file2);
+
+    let (file2, revision2) = match resource2 {
+        Some(resource) => {
+            let (file, revision) = parse_file_and_revision(resource);
+            (Some(PathBuf::from(file)), Some(revision))
+        }
+        None => (None, None),
+    };
 
     let keys: Vec<String> = match sub_matches.get_many::<String>("keys") {
         Some(values) => values.cloned().collect(),
         None => Vec::new(),
     };
 
-    let targets: Vec<String> = match sub_matches.get_many::<String>("targets") {
+    let maybe_targets = sub_matches.get_many::<String>("targets");
+
+    let targets = match maybe_targets {
         Some(values) => values.cloned().collect(),
         None => Vec::new(),
     };
 
     let output = sub_matches.get_one::<String>("output").map(PathBuf::from);
 
-    match dispatch::compare(file1, revision1, file2, revision2, keys, targets, output) {
+    match dispatch::diff(
+        file1, revision1, file2, revision2, keys, targets, output, is_remote,
+    )
+    .await
+    {
         Ok(_) => {}
         Err(err) => {
             eprintln!("{err}")
         }
     }
 }
 
-pub async fn diff(sub_matches: &ArgMatches) {
-    let is_remote = false;
-    p_diff(sub_matches, is_remote).await
-}
-
-async fn p_diff(sub_matches: &ArgMatches, is_remote: bool) {
-    // First arg is optional
-    let file_or_commit_id = sub_matches
-        .get_one::<String>("FILE_OR_REVISION")
-        .expect("required");
-    let path = sub_matches.get_one::<String>("PATH");
-    if let Some(path) = path {
-        match dispatch::diff(Some(file_or_commit_id), path, is_remote).await {
-            Ok(_) => {}
-            Err(err) => {
-                eprintln!("{err}")
-            }
-        }
-    } else {
-        match dispatch::diff(None, file_or_commit_id, is_remote).await {
-            Ok(_) => {}
-            Err(err) => {
-                eprintln!("{err}")
-            }
-        }
-    }
-}
+// pub async fn diff(sub_matches: &ArgMatches) {
+//     let is_remote = false;
+//     p_diff(sub_matches, is_remote).await
+// }
+
+// async fn p_diff(sub_matches: &ArgMatches, is_remote: bool) {
+//     // First arg is optional
+//     let file_or_commit_id = sub_matches
+//         .get_one::<String>("FILE_OR_REVISION")
+//         .expect("required");
+//     let path = sub_matches.get_one::<String>("PATH");
+//     if let Some(path) = path {
+//         match dispatch::diff(Some(file_or_commit_id), path, is_remote).await {
+//             Ok(_) => {}
+//             Err(err) => {
+//                 eprintln!("{err}")
+//             }
+//         }
+//     } else {
+//         match dispatch::diff(None, file_or_commit_id, is_remote).await {
+//             Ok(_) => {}
+//             Err(err) => {
+//                 eprintln!("{err}")
+//             }
+//         }
+//     }
+// }
 
 pub async fn clone(sub_matches: &ArgMatches) {
     let url = sub_matches.get_one::<String>("URL").expect("required");