feat(gitcommit): add remote, fetch, pull support #27
Conversation
- Add cherry_pick_skip operation - Improve error messages with conflict resolution guidance
- Remove nested output/user_msg/llm_msg wrapper objects - Remove setup handlers from tool definitions - Rename agent to tools in all handler signatures - Remove v17 compatibility code and comments
Summary of ChangesHello @jinzhongjia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great feature enhancement, adding support for more git commands like remote, fetch, and pull, as well as improving the cherry-pick and merge workflows with conflict handling. The introduction of tests using mini.nvim is a fantastic step towards improving code quality and maintainability. I've found a few areas for improvement, mainly around code duplication and a bug in pattern matching for git conflicts. I've also made some suggestions to improve the new test files.
I am having trouble creating individual review comments. Click here to see my feedback.
lua/codecompanion/_extensions/gitcommit/tools/git.lua (962)
The Lua pattern used here to find git conflict markers is incorrect and will not work for multi-line conflict blocks. The . character in Lua patterns does not match newline characters, and the pattern itself seems to have typos (<<<<<<<. and ->). This will fail to detect most, if not all, real-world git conflicts.
A correct implementation would need to handle multi-line matching. Since Lua patterns don't have a simple multi-line flag, you could split the content by lines and process it, or use a more complex pattern that can handle newlines, for example by using string.find in a loop to find start and end markers.
lua/codecompanion/_extensions/gitcommit/tools/ai_release_notes.lua (3-20)
There's duplicated code here. The helper functions is_windows and shell_quote are also defined in lua/codecompanion/_extensions/gitcommit/tools/git.lua. To avoid code duplication and improve maintainability, these functions should be extracted into a shared utility module (e.g., lua/codecompanion/_extensions/gitcommit/utils.lua) and required where needed.
lua/codecompanion/_extensions/gitcommit/tools/validation.lua (152-162)
This implementation of first_error is not as robust as it could be. It iterates from 1 to max_idx, which assumes a dense array-like table. If the validations table is sparse (e.g., {[2] = err1, [4] = err2}), this loop will fail on validations[1]. A more robust approach would be to use ipairs which correctly handles standard Lua sequences, or if you need to support sparse tables, sort the keys first.
for _, result in ipairs(validations) do
if result then
return result
end
end
tests/test_git_edit.lua (44)
The test is hardcoded to expect 28 operations. While this works now, it will break every time a new operation is added, making the tests brittle. It would be more maintainable to get the list of valid operations from the VALID_OPERATIONS table in git_edit.lua and compare the lengths.
tests/test_git_read.lua (44)
The test is hardcoded to expect 18 operations. This is brittle and will require updates whenever operations are added or removed. A more maintainable approach would be to get the list of valid operations from the source file and assert that the schema's enum count matches the source of truth.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent and extensive refactoring of the git-related functionality. The introduction of git_utils.lua for pure functions and command.lua for command building and execution significantly improves the code's structure, testability, and maintainability. The move from Makefile to mise.toml is a great modernization of the development workflow. The addition of a comprehensive test suite is also a major improvement.
I've found a few issues, including a regression in the glob pattern matching and diff filtering, and a small bug in how pcall is used. My comments provide suggestions to address these points. Overall, this is a very strong contribution to the project.
- Fix show_conflict multiline pattern matching (. doesn't match newlines) - Fix glob_to_lua_pattern missing [ and ] character escaping - Fix stage_files/unstage_files losing second return value from pcall - Replace hardcoded operation counts in tests with dynamic checks - Add tests for square bracket escaping in glob patterns - Add tests for sparse array handling in first_error
Add 37 new robustness tests covering: - glob_to_lua_pattern: empty string, unicode, special chars, long input - parse_conflicts: single/multiple/multiline conflicts, malformed input - has_conflicts: basic detection - filter_diff: empty diff, special filenames, unicode, malformed headers - shell_quote_unix/windows: newlines, tabs, shell injection, metacharacters - validation functions: various types without crash, special error messages Extract conflict parsing to git_utils.lua for better testability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the structure, testability, and feature set of the gitcommit extension. The refactoring to introduce git_utils.lua and command.lua for pure functions and command execution is a fantastic move for maintainability. The addition of a comprehensive test suite is a major win for the project's long-term health. I've included a few suggestions to further reduce code duplication and improve robustness, but overall, this is a very high-quality contribution.
- Remove duplicate is_windows/shell_quote from command.lua, import from git_utils - Refactor contributors() to limit results in Lua instead of platform-specific pipes - Simplify filter_diff() by consolidating file path extraction logic
No description provided.