Adjusting diff return format; adding additional functionality /w diff baselines

Author: biscuitWizard

vcs-worker/src/operations/object/object_diff_op.rs

@@ -105,7 +105,41 @@ impl ObjectDiffOperation {
     }
 
     /// Get baseline object definition (before the change)
-    fn get_baseline_object(&self, object_name: &str, change: &Change) -> Result<Option<moor_compiler::ObjectDefinition>, ObjectsTreeError> {
+    /// If baseline_change_id is provided, use that specific change as the baseline
+    /// Otherwise, use the default logic (preceding change for merged, or index_change_id for non-merged)
+    fn get_baseline_object(&self, object_name: &str, change: &Change, baseline_change_id: Option<&str>) -> Result<Option<moor_compiler::ObjectDefinition>, ObjectsTreeError> {
+        // If a specific baseline change was requested, use that
+        if let Some(baseline_id) = baseline_change_id {
+            let resolved_baseline_id = self.database.resolve_change_id(baseline_id)?;
+            
+            // Try to find the baseline change in index first
+            let baseline_change = self
+                .database
+                .index()
+                .get_change(&resolved_baseline_id)
+                .map_err(|e| ObjectsTreeError::SerializationError(e.to_string()))?;
+            
+            if let Some(base_change) = baseline_change {
+                return self.get_object_at_change(object_name, &base_change);
+            } else {
+                // Try workspace if not in index
+                let baseline_change = self
+                    .database
+                    .workspace()
+                    .get_workspace_change(&resolved_baseline_id)
+                    .map_err(|e| ObjectsTreeError::SerializationError(e.to_string()))?;
+                
+                if let Some(base_change) = baseline_change {
+                    return self.get_object_at_change(object_name, &base_change);
+                } else {
+                    return Err(ObjectsTreeError::SerializationError(format!(
+                        "Baseline change '{}' not found in index or workspace",
+                        resolved_baseline_id
+                    )));
+                }
+            }
+        }
+        
         // If change is merged, we need to get the state before this change
         if change.status == ChangeStatus::Merged {
             // Get the change order and find the position of this change
@@ -159,12 +193,12 @@ impl ObjectDiffOperation {
     }
 
     /// Process the object diff request
-    fn process_object_diff(&self, change_id: &str, object_name: &str) -> Result<Vec<VerbDiff>, ObjectsTreeError> {
+    fn process_object_diff(&self, object_name: &str, change_id: &str, baseline_change_id: Option<&str>) -> Result<Vec<VerbDiff>, ObjectsTreeError> {
         // Resolve short or full hash to full hash
         let resolved_change_id = self.database.resolve_change_id(change_id)?;
         info!(
-            "Computing object diff for '{}' at change ID '{}'",
-            object_name, resolved_change_id
+            "Computing object diff for '{}' at change ID '{}' (baseline: {:?})",
+            object_name, resolved_change_id, baseline_change_id
         );
 
         // Get the change
@@ -191,7 +225,7 @@ impl ObjectDiffOperation {
 
         // Get current and baseline object definitions
         let current_obj = self.get_object_at_change(object_name, &change)?;
-        let baseline_obj = self.get_baseline_object(object_name, &change)?;
+        let baseline_obj = self.get_baseline_object(object_name, &change, baseline_change_id)?;
 
 
         let mut verb_diffs = Vec::new();
@@ -319,17 +353,23 @@ impl Operation for ObjectDiffOperation {
 
     fn parameters(&self) -> Vec<OperationParameter> {
         vec![
+            OperationParameter {
+                name: "object_name".to_string(),
+                description: "The name of the object to diff (e.g., '$player', '#123')"
+                    .to_string(),
+                required: true,
+            },
             OperationParameter {
                 name: "change_id".to_string(),
                 description: "The change ID (short or long Blake3 hash) to compare against"
                     .to_string(),
                 required: true,
             },
             OperationParameter {
-                name: "object_name".to_string(),
-                description: "The name of the object to diff (e.g., '$player', '#123')"
+                name: "baseline_change_id".to_string(),
+                description: "Optional baseline change ID to compare from. If not provided, uses the preceding change for merged changes or index_change_id for non-merged changes."
                     .to_string(),
-                required: true,
+                required: false,
             },
         ]
     }
@@ -338,18 +378,26 @@ impl Operation for ObjectDiffOperation {
         vec![
             OperationExample {
                 description: "Get diff for an object at a specific change".to_string(),
-                moocode: r#"diff = worker_request("vcs", {"object/diff", "abc123def456", "$player"});
+                moocode: r#"diff = worker_request("vcs", {"object/diff", "$player", "abc123def456"});
 // Returns a list of verb diffs showing what changed"#.to_string(),
                 http_curl: Some(r#"curl -X POST http://localhost:8081/api/object/diff \
   -H "Content-Type: application/json" \
-  -d '{"operation": "object/diff", "args": ["abc123def456", "$player"]}'"#.to_string()),
+  -d '{"operation": "object/diff", "args": ["$player", "abc123def456"]}'"#.to_string()),
             },
             OperationExample {
                 description: "Use short hash for convenience".to_string(),
-                moocode: r#"diff = worker_request("vcs", {"object/diff", "abc123", "obj123"});
+                moocode: r#"diff = worker_request("vcs", {"object/diff", "obj123", "abc123"});
 // Short hashes are automatically resolved to full hashes"#.to_string(),
                 http_curl: None,
             },
+            OperationExample {
+                description: "Compare against a specific baseline change".to_string(),
+                moocode: r#"diff = worker_request("vcs", {"object/diff", "$player", "abc123def456", "baseline123"});
+// Compares $player at change abc123def456 against the state at baseline123"#.to_string(),
+                http_curl: Some(r#"curl -X POST http://localhost:8081/api/object/diff \
+  -H "Content-Type: application/json" \
+  -d '{"operation": "object/diff", "args": ["$player", "abc123def456", "baseline123"]}'"#.to_string()),
+            },
         ]
     }
 
@@ -363,18 +411,20 @@ impl Operation for ObjectDiffOperation {
 
     fn execute(&self, args: Vec<String>, _user: &User) -> Var {
         // For RPC calls, we expect the args to contain:
-        // args[0] = change_id
-        // args[1] = object_name
+        // args[0] = object_name
+        // args[1] = change_id
+        // args[2] = baseline_change_id (optional)
 
         if args.len() < 2 {
-            error!("Object diff operation requires change_id and object_name");
-            return v_error(E_INVARG.msg("Change ID and object name are required"));
+            error!("Object diff operation requires object_name and change_id");
+            return v_error(E_INVARG.msg("Object name and change ID are required"));
         }
 
-        let change_id = args[0].clone();
-        let object_name = args[1].clone();
+        let object_name = args[0].clone();
+        let change_id = args[1].clone();
+        let baseline_change_id = args.get(2).map(|s| s.as_str());
 
-        match self.process_object_diff(&change_id, &object_name) {
+        match self.process_object_diff(&object_name, &change_id, baseline_change_id) {
             Ok(verb_diffs) => {
                 info!("Object diff operation completed successfully");
                 // Convert each VerbDiff to MOO Var and return as list

vcs-worker/src/types.rs

@@ -423,33 +423,57 @@ pub struct VerbDiff {
     pub hunks: Vec<DiffHunk>,
 }
 
-impl DiffLine {
-    /// Convert to MOO Var format
-    pub fn to_moo_var(&self) -> Var {
-        let (line_type, content) = match self {
-            DiffLine::Context(line) => ("context", line),
-            DiffLine::Added(line) => ("added", line),
-            DiffLine::Removed(line) => ("removed", line),
-        };
-        
-        let mut pairs = Vec::new();
-        pairs.push((moor_var::v_str("type"), moor_var::v_str(line_type)));
-        pairs.push((moor_var::v_str("line"), moor_var::v_str(content)));
-        moor_var::v_map(&pairs)
-    }
-}
 
 impl DiffHunk {
     /// Convert to MOO Var format
+    /// Groups consecutive lines of the same type together for efficiency
     pub fn to_moo_var(&self) -> Var {
         let mut pairs = Vec::new();
         pairs.push((moor_var::v_str("old_start"), moor_var::v_int(self.old_start as i64)));
         pairs.push((moor_var::v_str("old_lines"), moor_var::v_int(self.old_lines as i64)));
         pairs.push((moor_var::v_str("new_start"), moor_var::v_int(self.new_start as i64)));
         pairs.push((moor_var::v_str("new_lines"), moor_var::v_int(self.new_lines as i64)));
 
-        let lines: Vec<Var> = self.lines.iter().map(|line| line.to_moo_var()).collect();
-        pairs.push((moor_var::v_str("lines"), moor_var::v_list(&lines)));
+        // Group consecutive lines of the same type together
+        let mut grouped_lines = Vec::new();
+        let mut current_group: Option<(String, Vec<String>)> = None;
+        
+        for line in &self.lines {
+            let (line_type, content) = match line {
+                DiffLine::Context(line) => ("context", line.clone()),
+                DiffLine::Added(line) => ("added", line.clone()),
+                DiffLine::Removed(line) => ("removed", line.clone()),
+            };
+            
+            match &mut current_group {
+                Some((group_type, lines)) if group_type == line_type => {
+                    // Same type, add to current group
+                    lines.push(content);
+                }
+                _ => {
+                    // Different type or first line, finalize previous group and start new one
+                    if let Some((group_type, lines)) = current_group.take() {
+                        let line_vars: Vec<Var> = lines.iter().map(|l| moor_var::v_str(l)).collect();
+                        let mut group_pairs = Vec::new();
+                        group_pairs.push((moor_var::v_str("type"), moor_var::v_str(&group_type)));
+                        group_pairs.push((moor_var::v_str("lines"), moor_var::v_list(&line_vars)));
+                        grouped_lines.push(moor_var::v_map(&group_pairs));
+                    }
+                    current_group = Some((line_type.to_string(), vec![content]));
+                }
+            }
+        }
+        
+        // Don't forget the last group
+        if let Some((group_type, lines)) = current_group {
+            let line_vars: Vec<Var> = lines.iter().map(|l| moor_var::v_str(l)).collect();
+            let mut group_pairs = Vec::new();
+            group_pairs.push((moor_var::v_str("type"), moor_var::v_str(&group_type)));
+            group_pairs.push((moor_var::v_str("lines"), moor_var::v_list(&line_vars)));
+            grouped_lines.push(moor_var::v_map(&group_pairs));
+        }
+        
+        pairs.push((moor_var::v_str("lines"), moor_var::v_list(&grouped_lines)));
 
         moor_var::v_map(&pairs)
     }

vcs-worker/tests/common/client.rs

@@ -157,14 +157,32 @@ impl VcsTestClient {
     /// Get object diff for a specific change
     pub async fn object_diff(
         &self,
-        change_id: &str,
         object_name: &str,
+        change_id: &str,
     ) -> Result<Value, Box<dyn std::error::Error>> {
         self.rpc_call(
             "object/diff",
             vec![
+                Value::String(object_name.to_string()),
                 Value::String(change_id.to_string()),
+            ],
+        )
+        .await
+    }
+
+    /// Get object diff with optional baseline change ID
+    pub async fn object_diff_with_baseline(
+        &self,
+        object_name: &str,
+        change_id: &str,
+        baseline_change_id: &str,
+    ) -> Result<Value, Box<dyn std::error::Error>> {
+        self.rpc_call(
+            "object/diff",
+            vec![
                 Value::String(object_name.to_string()),
+                Value::String(change_id.to_string()),
+                Value::String(baseline_change_id.to_string()),
             ],
         )
         .await

vcs-worker/tests/operations/object/object_diff_tests.rs

@@ -47,7 +47,7 @@ end"#;
     // Test diff with full change ID
     println!("\nTesting diff with full change ID...");
     let result = client
-        .object_diff(&change_id, "test_object")
+        .object_diff("test_object", &change_id)
         .await
         .expect("Failed to get object diff");
 
@@ -89,7 +89,7 @@ end"#;
     // Test diff with short change ID
     println!("\nTesting diff with short change ID...");
     let result = client
-        .object_diff(short_id, "test_object")
+        .object_diff("test_object", short_id)
         .await
         .expect("Failed to get object diff with short ID");
 
@@ -115,7 +115,7 @@ async fn test_object_diff_nonexistent_change() {
 
     println!("Test: Object diff with non-existent change should return error");
 
-    let result = client.object_diff("nonexistent123", "test_object").await;
+    let result = client.object_diff("test_object", "nonexistent123").await;
 
     println!("Result: {:?}", result);
 
@@ -149,7 +149,7 @@ async fn test_object_diff_nonexistent_object() {
 
     let (change_id, _) = db.require_top_change();
 
-    let result = client.object_diff(&change_id, "nonexistent_object").await;
+    let result = client.object_diff("nonexistent_object", &change_id).await;
 
     println!("Result: {:?}", result);
 
@@ -184,7 +184,7 @@ async fn test_object_diff_missing_arguments() {
     println!("Test: Object diff with missing arguments should return error");
 
     // Test with missing object name (empty string)
-    let result = client.object_diff("some_change_id", "").await;
+    let result = client.object_diff("", "some_change_id").await;
 
     // Should return a successful response with an error message in the result
     let response = result.expect("Expected successful HTTP response");
@@ -228,7 +228,7 @@ end"#;
         .await
         .expect("Failed to update object v1");
 
-    let (change_id_1, _) = db.require_top_change();
+    let (_change_id_1, _) = db.require_top_change();
 
     // Submit the first change
     println!("\nSubmitting first change...");
@@ -262,7 +262,7 @@ end"#;
     // Now test the diff between the two changes
     println!("\nTesting diff between changes...");
     let result = client
-        .object_diff(&change_id_2, "test_object_v2")
+        .object_diff("test_object_v2", &change_id_2)
         .await
         .expect("Failed to get object diff");
 
@@ -300,16 +300,24 @@ end"#;
                     assert!(first_hunk.get("new_start").is_some(), "Expected new_start in hunk");
                     assert!(first_hunk.get("lines").is_some(), "Expected lines in hunk");
 
-                    if let Some(lines) = first_hunk.get("lines").and_then(|v| v.as_array()) {
-                        println!("✅ Found {} lines in hunk", lines.len());
-                        assert!(!lines.is_empty(), "Expected at least one line in hunk");
+                    if let Some(line_groups) = first_hunk.get("lines").and_then(|v| v.as_array()) {
+                        println!("✅ Found {} line groups in hunk", line_groups.len());
+                        assert!(!line_groups.is_empty(), "Expected at least one line group in hunk");
 
-                        // Check line types
-                        for (i, line) in lines.iter().enumerate() {
-                            if let Some(line_type) = line.get("type").and_then(|v| v.as_str()) {
-                                println!("Line {}: type={}", i, line_type);
-                                assert!(matches!(line_type, "context" | "added" | "removed"), 
-                                       "Expected line type to be context, added, or removed, got: {}", line_type);
+                        // Check line group structure (new grouped format)
+                        for (i, group) in line_groups.iter().enumerate() {
+                            if let Some(group_type) = group.get("type").and_then(|v| v.as_str()) {
+                                println!("Group {}: type={}", i, group_type);
+                                assert!(matches!(group_type, "context" | "added" | "removed"), 
+                                       "Expected group type to be context, added, or removed, got: {}", group_type);
+                                
+                                // Check that the group has a "lines" array
+                                if let Some(lines_in_group) = group.get("lines").and_then(|v| v.as_array()) {
+                                    println!("  Group {} has {} lines", i, lines_in_group.len());
+                                    assert!(!lines_in_group.is_empty(), "Expected at least one line in group");
+                                } else {
+                                    panic!("Expected 'lines' array in group, got: {:?}", group);
+                                }
                             }
                         }
                     }