Skip to content

refactor: refactor commit-info API and GitHub history inconsistency #1657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+350 −506
Open

refactor: refactor commit-info API and GitHub history inconsistency #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d26b97f
refactor: refactor commit-info API and GitHub history inconsistency
AllureCurtain Nov 23, 2025
c08c89b
fix: fix index out of bounds and add docs
AllureCurtain Nov 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 70 additions & 51 deletions ceres/src/api_service/commit_ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,69 @@ use std::{path::PathBuf, sync::Arc};

use git_internal::{
errors::GitError,
hash::SHA1,
internal::object::{
commit::Commit,
tree::{TreeItem, TreeItemMode},
},
internal::object::tree::{TreeItem, TreeItemMode},
};
use tokio::sync::Mutex;

use crate::api_service::{ApiHandler, cache::GitObjectCache, history, tree_ops};
use crate::model::git::{CommitBindingInfo, LatestCommitInfo};

/// Get the latest commit that modified a file or directory.
///
/// This unified function handles both tag-based and commit-based browsing through
/// the `refs` parameter, ensuring consistent behavior across all code paths.
///
/// # Arguments
/// - `handler`: API handler for accessing Git data
/// - `path`: File or directory path to check
/// - `refs`: Optional reference (tag name or commit SHA). If None, uses default HEAD/root.
///
/// # Returns
/// The commit information for the last modification of the specified path.
pub async fn get_latest_commit<T: ApiHandler + ?Sized>(
handler: &T,
path: PathBuf,
refs: Option<&str>,
) -> Result<LatestCommitInfo, GitError> {
// Resolve the starting commit from refs
let start_commit = crate::api_service::resolve_start_commit(handler, refs).await?;

// 1) Try as directory path first
if let Some(tree) = tree_ops::search_tree_by_path(handler, &path, None).await? {
let commit = get_tree_relate_commit(handler, tree.id, path).await?;
if let Some(tree) = tree_ops::search_tree_by_path(handler, &path, refs).await? {
// Special handling for root directory
let (dir_name, parent): (String, &std::path::Path) = if path.as_os_str().is_empty()
|| path == std::path::Path::new(".")
|| path == std::path::Path::new("/")
{
// For root directory, treat it as the tree itself with empty parent
// This commit represents the root tree's last modification
(String::from(""), std::path::Path::new(""))
} else {
// For non-root directories, extract name and parent normally
let dir_name = path
.file_name()
.and_then(|n| n.to_str())
.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
.to_string();
let parent = path
.parent()
.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;
(dir_name, parent)
};

Comment on lines +35 to +54
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug with root directory handling: When handling the root directory (/, ., or empty path), the code creates a TreeItem with an empty string as the name (String::from("")). This item is then used in traverse_commit_history_for_last_modification, which searches for an item with that empty name in the tree items.

However, tree items typically don't have empty names - the root tree itself isn't contained as an item within its parent. This logic appears incorrect and may cause the traversal to fail with a 404 error when querying the root directory's last commit.

Consider either:

  1. Special-casing root directory to return the start_commit directly without traversal
  2. Using the root tree's hash instead of trying to find it as an item in a non-existent parent
Suggested change
let (dir_name, parent): (String, &std::path::Path) = if path.as_os_str().is_empty()
|| path == std::path::Path::new(".")
|| path == std::path::Path::new("/")
{
// For root directory, treat it as the tree itself with empty parent
// This commit represents the root tree's last modification
(String::from(""), std::path::Path::new(""))
} else {
// For non-root directories, extract name and parent normally
let dir_name = path
.file_name()
.and_then(|n| n.to_str())
.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
.to_string();
let parent = path
.parent()
.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;
(dir_name, parent)
};
if path.as_os_str().is_empty()
|| path == std::path::Path::new(".")
|| path == std::path::Path::new("/")
{
// For root directory, the commit that created the tree is the last modification.
let mut commit_info: LatestCommitInfo = start_commit.clone().into();
// If commit has a username binding, prefer showing that username
if let Ok(Some(binding)) = handler
.build_commit_binding_info(&start_commit.id.to_string())
.await
&& !binding.is_anonymous
&& binding.matched_username.is_some()
{
let username = binding.matched_username.unwrap();
commit_info.author = username.clone();
commit_info.committer = username;
}
return Ok(commit_info);
}
// For non-root directories, extract name and parent normally
let dir_name = path
.file_name()
.and_then(|n| n.to_str())
.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
.to_string();
let parent = path
.parent()
.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;

Copilot uses AI. Check for mistakes.
let dir_item = TreeItem::new(TreeItemMode::Tree, tree.id, dir_name);

let cache = Arc::new(Mutex::new(GitObjectCache::default()));

let commit = history::traverse_commit_history_for_last_modification(
handler,
parent,
start_commit.clone(),
&dir_item,
cache,
)
.await?;

let mut commit_info: LatestCommitInfo = commit.clone().into();

// If commit has a username binding, prefer showing that username
Expand All @@ -38,25 +83,11 @@ pub async fn get_latest_commit<T: ApiHandler + ?Sized>(
}

// 2) If not a directory, try as file path
// basic validation for file path
path.file_name()
.and_then(|n| n.to_str())
.ok_or_else(|| GitError::CustomError("Invalid file path".to_string()))?;
let parent = path
.parent()
.ok_or_else(|| GitError::CustomError("Invalid file path".to_string()))?;
// Use unified last-modification logic
let cache = Arc::new(Mutex::new(GitObjectCache::default()));

// parent must be a directory tree that exists
if tree_ops::search_tree_by_path(handler, parent, None)
.await?
.is_none()
{
return Err(GitError::CustomError(
"can't find target parent tree under latest commit".to_string(),
));
};
match history::resolve_latest_commit_for_file_path(handler, &path).await? {
Some(commit) => {
match history::resolve_last_modification_by_path(handler, &path, start_commit, cache).await {
Ok(commit) => {
let mut commit_info: LatestCommitInfo = commit.clone().into();
// If commit has a username binding, prefer showing that username
if let Ok(Some(binding)) = handler
Expand All @@ -71,33 +102,21 @@ pub async fn get_latest_commit<T: ApiHandler + ?Sized>(
}
Ok(commit_info)
}
None => Err(GitError::CustomError(
"[code:404] File not found".to_string(),
)),
}
}

pub async fn get_tree_relate_commit<T: ApiHandler + ?Sized>(
handler: &T,
t_hash: SHA1,
path: PathBuf,
) -> Result<Commit, GitError> {
let file_name = match path.file_name() {
Some(name) => name.to_string_lossy().to_string(),
None => {
return Err(GitError::CustomError("Invalid Path Input".to_string()));
Err(e) => {
// Preserve the original error message for better debugging
tracing::debug!("File not found or error during traversal: {:?}", e);
// If it's already a CustomError with [code:404], preserve it
if let GitError::CustomError(msg) = &e
&& msg.starts_with("[code:404]")
{
return Err(e);
}
// Otherwise wrap with 404 code
Err(GitError::CustomError(
"[code:404] File not found".to_string(),
))
Comment on lines +106 to +117
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Inconsistent error handling pattern: The error handling pattern here is convoluted with nested pattern matching. The code checks if the error is CustomError with [code:404] prefix, preserves it, and otherwise wraps it with a generic 404 message. However, this loses the original error context when wrapping.

Consider simplifying this to:

Err(e) => {
    tracing::debug!("File not found or error during traversal: {:?}", e);
    match e {
        GitError::CustomError(ref msg) if msg.starts_with("[code:404]") => Err(e),
        _ => Err(GitError::CustomError("[code:404] File not found".to_string()))
    }
}

This avoids the let-if pattern and makes the logic clearer.

Suggested change
// Preserve the original error message for better debugging
tracing::debug!("File not found or error during traversal: {:?}", e);
// If it's already a CustomError with [code:404], preserve it
if let GitError::CustomError(msg) = &e
&& msg.starts_with("[code:404]")
{
return Err(e);
}
// Otherwise wrap with 404 code
Err(GitError::CustomError(
"[code:404] File not found".to_string(),
))
tracing::debug!("File not found or error during traversal: {:?}", e);
match e {
GitError::CustomError(ref msg) if msg.starts_with("[code:404]") => Err(e),
_ => Err(GitError::CustomError("[code:404] File not found".to_string())),
}

Copilot uses AI. Check for mistakes.
}
};

let search_item = TreeItem::new(TreeItemMode::Tree, t_hash, file_name);
let cache = Arc::new(Mutex::new(GitObjectCache::default()));
let root_commit = Arc::new(handler.get_root_commit().await);

let parent = match path.parent() {
Some(p) => p,
None => return Err(GitError::CustomError("Invalid Path Input".to_string())),
};
history::traverse_commit_history(handler, parent, root_commit, &search_item, cache).await
}
}

/// Build commit binding information for a given commit SHA
Expand Down
Loading