From 6dec06865fa1946685324c034496f01f9c366dc0 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Feb 2026 21:05:28 -0600 Subject: [PATCH] Refactor code for improved readability and maintainability - Simplified multi-line function calls in `PatchTree.lua` and `SHA1.luau` for better clarity. - Enhanced the `init.lua` file by formatting the `PatchTree.build` function call across multiple lines. - Updated `change_processor.rs` and `api.rs` to improve the formatting of function calls and enhance code consistency. - Made minor adjustments in test files to streamline assertions and improve readability. These changes contribute to a cleaner codebase and enhance overall maintainability. --- plugin/src/App/init.lua | 7 +++- plugin/src/PatchTree.lua | 3 +- plugin/src/SHA1.luau | 65 +++++++++++++++++--------------- src/change_processor.rs | 5 ++- src/git.rs | 45 +++++++++++----------- src/web/api.rs | 45 +++++++++++----------- tests/tests/git_sync_defaults.rs | 37 ++++++++++-------- 7 files changed, 111 insertions(+), 96 deletions(-) diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 03f89b8b..57633fbf 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -868,7 +868,12 @@ function App:startSession() }) self:setState({ appStatus = AppStatus.Confirming, - patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }, serverInfo.gitMetadata), + patchTree = PatchTree.build( + patch, + instanceMap, + { "Property", "Current", "Incoming" }, + serverInfo.gitMetadata + ), confirmData = { serverInfo = serverInfo, }, diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index dd74006f..465cec26 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -319,8 +319,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders, gitMetadata) if changedIdSet then if not changedIdSet[change.id] then defaultSelection = "pull" - elseif instance:IsA("LuaSourceContainer") - and gitMetadata.scriptCommittedHashes then + elseif instance:IsA("LuaSourceContainer") and gitMetadata.scriptCommittedHashes then local hashes = gitMetadata.scriptCommittedHashes[change.id] if hashes then local source = instance.Source diff --git a/plugin/src/SHA1.luau b/plugin/src/SHA1.luau index a300dd81..bd2f39cc 100644 --- a/plugin/src/SHA1.luau +++ b/plugin/src/SHA1.luau @@ -33,50 +33,55 @@ local function DigestBlocks(Blocks: buffer, Length: number): (number, number, nu end for Index = 64, 316, 4 do - buffer.writeu32(Offsets, Index, bit32.lrotate(bit32.bxor( - buffer.readu32(Offsets, Index - 12), - buffer.readu32(Offsets, Index - 32), - buffer.readu32(Offsets, Index - 56), - buffer.readu32(Offsets, Index - 64) - ), 1)) + buffer.writeu32( + Offsets, + Index, + bit32.lrotate( + bit32.bxor( + buffer.readu32(Offsets, Index - 12), + buffer.readu32(Offsets, Index - 32), + buffer.readu32(Offsets, Index - 56), + buffer.readu32(Offsets, Index - 64) + ), + 1 + ) + ) end local H1, H2, H3, H4, H5 = A, B, C, D, E - + local Temp for Round = 0, 19 do - Temp = bit32.lrotate(H1, 5) + - bit32.band(H2, H3) + bit32.band(-1 - H2, H4) + - H5 + 0x5a827999 + - buffer.readu32(Offsets, Round * 4) - + Temp = bit32.lrotate(H1, 5) + + bit32.band(H2, H3) + + bit32.band(-1 - H2, H4) + + H5 + + 0x5a827999 + + buffer.readu32(Offsets, Round * 4) + H5, H4, H3, H2, H1 = H4, H3, bit32.lrotate(H2, 30), H1, Temp end - + for Round = 20, 39 do - Temp = bit32.lrotate(H1, 5) + - bit32.bxor(H2, H3, H4) + - H5 + 0x6ed9eba1 + - buffer.readu32(Offsets, Round * 4) - + Temp = bit32.lrotate(H1, 5) + bit32.bxor(H2, H3, H4) + H5 + 0x6ed9eba1 + buffer.readu32(Offsets, Round * 4) + H5, H4, H3, H2, H1 = H4, H3, bit32.lrotate(H2, 30), H1, Temp end - + for Round = 40, 59 do - Temp = bit32.lrotate(H1, 5) + - bit32.band(H4, H3) + bit32.band(H2, bit32.bxor(H4, H3)) + - H5 + 0x8f1bbcdc + - buffer.readu32(Offsets, Round * 4) - + Temp = bit32.lrotate(H1, 5) + + bit32.band(H4, H3) + + bit32.band(H2, bit32.bxor(H4, H3)) + + H5 + + 0x8f1bbcdc + + buffer.readu32(Offsets, Round * 4) + H5, H4, H3, H2, H1 = H4, H3, bit32.lrotate(H2, 30), H1, Temp end - + for Round = 60, 79 do - Temp = bit32.lrotate(H1, 5) + - bit32.bxor(H2, H3, H4) + - H5 + 0xca62c1d6 + - buffer.readu32(Offsets, Round * 4) - + Temp = bit32.lrotate(H1, 5) + bit32.bxor(H2, H3, H4) + H5 + 0xca62c1d6 + buffer.readu32(Offsets, Round * 4) + H5, H4, H3, H2, H1 = H4, H3, bit32.lrotate(H2, 30), H1, Temp end diff --git a/src/change_processor.rs b/src/change_processor.rs index 86fc1419..4a27b907 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1822,7 +1822,10 @@ impl JobThreadContext { ); } else if patch_set.stage_ids.contains(&id) { if let Some(ref repo_root) = self.git_repo_root { - crate::git::git_add(repo_root, std::slice::from_ref(write_path)); + crate::git::git_add( + repo_root, + std::slice::from_ref(write_path), + ); } } } else { diff --git a/src/git.rs b/src/git.rs index e39af892..dbc0fdfe 100644 --- a/src/git.rs +++ b/src/git.rs @@ -155,23 +155,29 @@ struct ResolvedInstance { /// Computes git metadata for the two-way sync confirmation UI. /// +/// `repo_root` must be the git repository root (from `git_repo_root()`), +/// cached at session start by `ServeSession`. +/// /// Uses a two-phase approach to avoid holding the tree lock during I/O: /// 1. Run git commands to get changed files (no lock) /// 2. Briefly lock tree to resolve file paths to instance Refs and class names /// 3. Run git show for each changed script and compute hashes (no lock) -pub fn compute_git_metadata( - tree_handle: &Arc>, - project_root: &Path, -) -> Option { - let repo_root_raw = git_repo_root(project_root)?; - let repo_root = std::fs::canonicalize(&repo_root_raw).unwrap_or(repo_root_raw); - let changed_files = git_changed_files(&repo_root)?; +pub fn compute_git_metadata(tree_handle: &Arc>, repo_root: &Path) -> GitMetadata { + let changed_files = match git_changed_files(repo_root) { + Some(files) => files, + None => { + return GitMetadata { + changed_ids: Vec::new(), + script_committed_hashes: HashMap::new(), + }; + } + }; if changed_files.is_empty() { - return Some(GitMetadata { + return GitMetadata { changed_ids: Vec::new(), script_committed_hashes: HashMap::new(), - }); + }; } // Brief lock: resolve changed file paths to instance Refs and class names @@ -207,11 +213,11 @@ pub fn compute_git_metadata( let mut hashes = Vec::with_capacity(2); - if let Some(head_content) = git_show_head(&repo_root, &ri.rel_path) { + if let Some(head_content) = git_show_head(repo_root, &ri.rel_path) { hashes.push(compute_blob_sha1(&head_content)); } - if let Some(staged_content) = git_show_staged(&repo_root, &ri.rel_path) { + if let Some(staged_content) = git_show_staged(repo_root, &ri.rel_path) { let staged_hash = compute_blob_sha1(&staged_content); if hashes.is_empty() || hashes[0] != staged_hash { hashes.push(staged_hash); @@ -223,10 +229,10 @@ pub fn compute_git_metadata( } } - Some(GitMetadata { + GitMetadata { changed_ids, script_committed_hashes, - }) + } } #[cfg(test)] @@ -251,13 +257,7 @@ mod tests { .output() .unwrap(); Command::new("git") - .args([ - "-C", - &dir.to_string_lossy(), - "config", - "user.name", - "Test", - ]) + .args(["-C", &dir.to_string_lossy(), "config", "user.name", "Test"]) .output() .unwrap(); } @@ -382,10 +382,7 @@ mod tests { // Subdirectory should resolve to the same repo root assert_eq!( root.canonicalize().unwrap(), - git_repo_root(dir.path()) - .unwrap() - .canonicalize() - .unwrap() + git_repo_root(dir.path()).unwrap().canonicalize().unwrap() ); } diff --git a/src/web/api.rs b/src/web/api.rs index 3ace00d5..0f1ed75a 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -273,10 +273,9 @@ impl ApiService { Vec::new() }; - let git_metadata = crate::git::compute_git_metadata( - &self.serve_session.tree_handle(), - self.serve_session.root_dir(), - ); + let git_metadata = self.serve_session.git_repo_root().map(|repo_root| { + crate::git::compute_git_metadata(&self.serve_session.tree_handle(), repo_root) + }); msgpack_ok(&ServerInfoResponse { server_version: SERVER_VERSION.to_owned(), @@ -742,33 +741,35 @@ impl ApiService { let stage_ids: HashSet = request.stage_ids.into_iter().collect(); + let paths_to_stage: Vec = if !stage_ids.is_empty() { + let tree = self.serve_session.tree(); + stage_ids + .iter() + .filter_map(|&id| { + tree.get_instance(id).and_then(|inst| { + inst.metadata() + .instigating_source + .as_ref() + .map(|src| src.path().to_path_buf()) + }) + }) + .collect() + } else { + Vec::new() + }; + tree_mutation_sender .send(PatchSet { removed_instances: actually_removed, added_instances, updated_instances, - stage_ids: stage_ids.clone(), + stage_ids, }) .unwrap(); if let Some(repo_root) = self.serve_session.git_repo_root() { - if !stage_ids.is_empty() { - let tree = self.serve_session.tree(); - let paths_to_stage: Vec = stage_ids - .iter() - .filter_map(|&id| { - tree.get_instance(id).and_then(|inst| { - inst.metadata().instigating_source.as_ref().map(|src| { - src.path().to_path_buf() - }) - }) - }) - .collect(); - drop(tree); - - if !paths_to_stage.is_empty() { - crate::git::git_add(repo_root, &paths_to_stage); - } + if !paths_to_stage.is_empty() { + crate::git::git_add(repo_root, &paths_to_stage); } } diff --git a/tests/tests/git_sync_defaults.rs b/tests/tests/git_sync_defaults.rs index 4405eef1..c5f2e92f 100644 --- a/tests/tests/git_sync_defaults.rs +++ b/tests/tests/git_sync_defaults.rs @@ -14,8 +14,8 @@ use std::collections::HashMap; use std::path::Path; use std::process::Command; -use std::{fs, thread}; use std::time::Duration; +use std::{fs, thread}; use librojo::web_api::{AddedInstance, InstanceUpdate, WriteRequest}; use rbx_dom_weak::types::{Ref, Variant}; @@ -297,10 +297,7 @@ fn staged_hash_deduplicated_when_same_as_head() { if let Some(hashes) = meta.script_committed_hashes.get(&module_id) { // When staged differs from HEAD, we get 2 hashes // When staged == HEAD, we'd get 1 hash (deduplicated) - assert!( - hashes.len() <= 2, - "Should have at most 2 hashes" - ); + assert!(hashes.len() <= 2, "Should have at most 2 hashes"); } } } @@ -410,13 +407,17 @@ fn all_script_types_get_hashes() { fn stage_ids_in_write_request_triggers_git_add() { let mut session = TestServeSession::new_with_git("git_sync_defaults", |path| { git_commit_all(path, "initial commit"); - fs::write(path.join("src/ModuleA.luau"), "-- modified for staging test").unwrap(); + fs::write( + path.join("src/ModuleA.luau"), + "-- modified for staging test", + ) + .unwrap(); }); let info = session.wait_to_come_online(); let read = session.get_api_read(info.root_instance_id).unwrap(); - let (module_id, _) = find_instance_by_name(&read.instances, "ModuleA") - .expect("ModuleA should exist"); + let (module_id, _) = + find_instance_by_name(&read.instances, "ModuleA").expect("ModuleA should exist"); // Send a write request with stage_ids containing ModuleA let write_request = WriteRequest { @@ -473,8 +474,8 @@ fn stage_ids_only_stages_specified_files() { let info = session.wait_to_come_online(); let read = session.get_api_read(info.root_instance_id).unwrap(); - let (module_id, _) = find_instance_by_name(&read.instances, "ModuleA") - .expect("ModuleA should exist"); + let (module_id, _) = + find_instance_by_name(&read.instances, "ModuleA").expect("ModuleA should exist"); // Only stage ModuleA, NOT ServerScript let write_request = WriteRequest { @@ -509,14 +510,16 @@ fn source_write_with_stage_ids_stages_after_write() { let info = session.wait_to_come_online(); let read = session.get_api_read(info.root_instance_id).unwrap(); - let (module_id, _) = find_instance_by_name(&read.instances, "ModuleA") - .expect("ModuleA should exist"); + let (module_id, _) = + find_instance_by_name(&read.instances, "ModuleA").expect("ModuleA should exist"); // Write a Source update AND include in stage_ids let mut props = UstrMap::default(); props.insert( ustr("Source"), - Some(Variant::String("-- new source from pull\nreturn {}\n".to_string())), + Some(Variant::String( + "-- new source from pull\nreturn {}\n".to_string(), + )), ); let write_request = WriteRequest { @@ -559,13 +562,15 @@ fn source_write_without_stage_ids_does_not_stage() { let info = session.wait_to_come_online(); let read = session.get_api_read(info.root_instance_id).unwrap(); - let (module_id, _) = find_instance_by_name(&read.instances, "ModuleA") - .expect("ModuleA should exist"); + let (module_id, _) = + find_instance_by_name(&read.instances, "ModuleA").expect("ModuleA should exist"); let mut props = UstrMap::default(); props.insert( ustr("Source"), - Some(Variant::String("-- manual pull, not auto-staged".to_string())), + Some(Variant::String( + "-- manual pull, not auto-staged".to_string(), + )), ); // NO stage_ids