hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor code for improved readability and maintainability

Browse source 
- 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.
This commit is contained in:
Joe 2026-02-17 21:05:28 -06:00
parent 91c599ecca
commit 6dec06865f
7 changed files with 111 additions and 96 deletions
Download patch file Download diff file Expand all files Collapse all files

7
plugin/src/App/init.lua
View file

@ -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,
},

3
plugin/src/PatchTree.lua
View file

@ -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

41
plugin/src/SHA1.luau
View file

@ -33,49 +33,54 @@ 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.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))
),
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

5
src/change_processor.rs
View file

@ -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 {

49
src/git.rs
View file

@ -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<Mutex<RojoTree>>,
project_root: &Path,
) -> Option<GitMetadata> {
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)?;
if changed_files.is_empty() {
return Some(GitMetadata {
pub fn compute_git_metadata(tree_handle: &Arc<Mutex<RojoTree>>, 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 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()
);
}

41
src/web/api.rs
View file

@ -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,35 +741,37 @@ impl ApiService {
let stage_ids: HashSet<Ref> = request.stage_ids.into_iter().collect();
let paths_to_stage: Vec<PathBuf> = 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<PathBuf> = 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);
}
}
}
msgpack_ok(WriteResponse { session_id })
}

37
tests/tests/git_sync_defaults.rs
View file

@ -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