Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Aug 28, 2025

This will be used by the lsp client with the post-commit hook to retrieve committed functions

PR Type

Enhancement


Description

  • Add support for diffing specific commit

  • Extract common function-inside-lines logic

  • Introduce getOptimizableFunctionsInCommit LSP

  • Refactor uncommitted diff integration


Diagram Walkthrough

flowchart LR
  GDU["Git Diff Utility"] --> FDL["Function Lines Extractor"]
  GDU -- "only_this_commit" --> FDL
  FDL --> LSP["LSP commit endpoint"]
Loading

File Walkthrough

Relevant files
Enhancement
git_utils.py
Support diff for a specific commit                                             

codeflash/code_utils/git_utils.py

  • Add only_this_commit parameter
  • Select diff for specific commit
  • Preserve existing uncommitted diff logic
+8/-2     
functions_to_optimize.py
Extract common logic and add commit functions                       

codeflash/discovery/functions_to_optimize.py

  • Extract get_functions_inside_lines helper
  • Add get_functions_inside_a_commit function
  • Refactor get_functions_within_git_diff return
+12/-3   
beta.py
Add LSP feature for commit functions                                         

codeflash/lsp/beta.py

  • Import new discovery functions
  • Add getOptimizableFunctionsInCommit feature
  • Implement _group_functions_by_file helper
+39/-1   

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Input Validation

Ensure only_this_commit hash is validated and that errors from git.diff are handled to avoid unexpected crashes when an invalid or first-parent commit is provided.

if only_this_commit:
    uni_diff_text = repository.git.diff(
        only_this_commit + "^1", only_this_commit, ignore_blank_lines=True, ignore_space_at_eol=True
    )
elif uncommitted_changes:
Error Handling

Wrap get_functions_inside_a_commit in the LSP handler with try/except to return a structured error response instead of letting exceptions bubble up on invalid commit hashes.

@server.feature("getOptimizableFunctionsInCommit")
def get_functions_in_commit(
    server: CodeflashLanguageServer, params: OptimizableFunctionsInCommitParams
) -> dict[str, str | dict[str, list[str]]]:
    functions = get_functions_inside_a_commit(params.commit_hash)
    file_to_qualified_names = _group_functions_by_file(server, functions)
    return {"functions": file_to_qualified_names, "status": "success"}
Performance Concern

Consider adding caching or incremental parsing in get_functions_inside_lines to avoid reprocessing the entire codebase on each invocation, improving performance for large repositories.

def get_functions_inside_lines(modified_lines: dict[str, list[int]]) -> dict[str, list[FunctionToOptimize]]:
    functions: dict[str, list[FunctionToOptimize]] = {}
    for path_str, lines_in_file in modified_lines.items():
        path = Path(path_str)
        if not path.exists():
            continue
        with path.open(encoding="utf8") as f:
            file_content = f.read()
            try:
                wrapper = cst.metadata.MetadataWrapper(cst.parse_module(file_content))
            except Exception as e:
                logger.exception(e)
                continue
            function_lines = FunctionVisitor(file_path=str(path))
            wrapper.visit(function_lines)
            functions[str(path)] = [
                function_to_optimize
                for function_to_optimize in function_lines.functions
                if (start_line := function_to_optimize.starting_line) is not None
                and (end_line := function_to_optimize.ending_line) is not None
                and any(start_line <= line <= end_line for line in lines_in_file)
            ]
    return functions

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add diff fallback for root commits

Wrap the diff invocation in a try/except to handle commits without parents and
fallback to diffing against the null tree SHA. This prevents GitCommandError on
initial commits. Use the known Git null tree constant.

codeflash/code_utils/git_utils.py [33-36]

-uni_diff_text = repository.git.diff(
-    only_this_commit + "^1", only_this_commit, ignore_blank_lines=True, ignore_space_at_eol=True
-)
+try:
+    uni_diff_text = repository.git.diff(
+        f"{only_this_commit}^1", only_this_commit, ignore_blank_lines=True, ignore_space_at_eol=True
+    )
+except git.GitCommandError:
+    # Fallback for initial commit without parent
+    null_tree = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"
+    uni_diff_text = repository.git.diff(
+        null_tree, only_this_commit, ignore_blank_lines=True, ignore_space_at_eol=True
+    )
Suggestion importance[1-10]: 8

__

Why: The suggestion adds robust error handling for initial commits by falling back to the Git null tree, preventing GitCommandError on root commits.

Medium
Validate commit and compute parent

Resolve only_this_commit to a commit object first to validate the hash and compute
parent SHA. This ensures clearer error handling and supports commits without
parents.

codeflash/code_utils/git_utils.py [33-36]

 if only_this_commit:
+    commit_obj = repository.commit(only_this_commit)
+    parent_sha = commit_obj.parents[0].hexsha if commit_obj.parents else "4b825dc642cb6eb9a060e54bf8d69288fbee4904"
     uni_diff_text = repository.git.diff(
-        only_this_commit + "^1", only_this_commit, ignore_blank_lines=True, ignore_space_at_eol=True
+        parent_sha, commit_obj.hexsha, ignore_blank_lines=True, ignore_space_at_eol=True
     )
Suggestion importance[1-10]: 7

__

Why: Resolving the commit object and determining the parent SHA enhances clarity and gracefully handles commits without parents.

Medium
General
Early skip empty modifications

Skip entries where lines_in_file is empty before file I/O and parsing. This avoids
unnecessary processing for files with no relevant modifications.

codeflash/discovery/functions_to_optimize.py [245-248]

 for path_str, lines_in_file in modified_lines.items():
+    if not lines_in_file:
+        continue
     path = Path(path_str)
     if not path.exists():
         continue
Suggestion importance[1-10]: 4

__

Why: Skipping entries with no modified lines avoids unnecessary file reads and parsing, improving performance.

Low

@mohammedahmed18 mohammedahmed18 requested a review from KRRT7 August 28, 2025 14:55
Copy link
Contributor

@Saga4 Saga4 left a comment

Choose a reason for hiding this comment

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

LGTM

@Saga4 Saga4 merged commit a59b9ed into main Sep 2, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants