feat(dotfile): add advanced merge support for config files#104
feat(dotfile): add advanced merge support for config files#104
Conversation
Implement custom Save method for GhosttyApp that: - Parses key-value config format with comment preservation - Merges remote and local configs with local priority - Shows detailed diff in dry-run mode - Preserves formatting including blank lines and comments This enhancement enables intelligent config merging where local settings always take precedence while incorporating new keys from remote configs. Fixes #101 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @AnnatarHe's task —— View job PR Review ResultsI've completed a comprehensive review of the Ghostty config merge implementation. Here's my detailed feedback: Todo List:
✅ Strengths
🐛 Potential Bugs & Issues
🚨 Security Concerns
⚡ Performance Considerations
📝 Code Quality Improvements
❌ Critical Missing: Test CoverageThis is the biggest concern. The PR adds 189 lines of complex logic with zero test coverage:
Recommended Test Cases: func TestParseGhosttyConfig(t *testing.T) {
// Test cases for comments, blank lines, key=value, edge cases
}
func TestMergeGhosttyConfigs(t *testing.T) {
// Test local priority, new key addition, existing key preservation
}
func TestGhosttyAppSave(t *testing.T) {
// Test dry-run mode, actual save, error conditions
}🔧 Recommendations
📊 Overall AssessmentThe implementation is functionally sound and well-structured, but lacks test coverage which is critical for config file manipulation logic. The core merge algorithm is correct, but there are some edge cases in parsing that should be addressed. Recommendation: Request tests before merge - This type of file manipulation logic is prone to edge case bugs and needs thorough testing. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @AnnatarHe, 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 introduces an advanced configuration merging system for dotfiles, specifically starting with Ghostty terminal configurations. The primary goal is to provide a robust mechanism for synchronizing configuration files by intelligently merging remote settings with local ones, ensuring local preferences are maintained, new remote keys are incorporated, and original file formatting is preserved. This feature also includes a dry-run capability to preview changes safely, aiming to resolve issue #101.
Highlights
- Intelligent Config Merging: Implemented a custom
Savemethod forGhosttyAppto intelligently merge configuration files, addressing issue #101. - Key-Value Parsing: Introduced smart parsing of config files into key-value pairs, preserving comments and blank lines.
- Local Priority: Ensured local configuration settings always take precedence over remote settings during merges.
- New Key Incorporation: Automatically adds new keys from remote configurations if they do not exist locally.
- Dry-Run Support: Added a dry-run mode to preview changes and show a detailed diff before modifying any files.
- Format Preservation: Maintained original formatting, including whitespace and comments, throughout the merge process.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces an intelligent merging mechanism for Ghostty configuration files, which is a great enhancement. The implementation correctly prioritizes local settings and incorporates new keys from remote configurations while preserving comments and blank lines. My review includes a few suggestions to improve robustness and maintainability: one to fully preserve file formatting as intended, another to refactor some redundant logic in the dry-run mode, and a third to improve error handling to ensure failures are correctly reported.
| var result []string | ||
| for _, line := range lines { | ||
| if line.isComment || line.isBlank { | ||
| result = append(result, line.raw) | ||
| } else { | ||
| result = append(result, fmt.Sprintf("%s = %s", line.key, line.value)) | ||
| } | ||
| } | ||
| return strings.Join(result, "\n") |
There was a problem hiding this comment.
The current implementation re-formats all key-value pairs using fmt.Sprintf("%s = %s", line.key, line.value). This alters the original formatting (e.g., spacing around =) and could strip inline comments. The PR description states that a goal is to "Maintain original formatting including whitespace and comments".
To better adhere to this goal, you can leverage the raw field of the configLine struct, which stores the original line from the file. By using line.raw for all lines, you ensure that formatting is perfectly preserved for existing lines. This also simplifies the function.
var result []string
for _, line := range lines {
result = append(result, line.raw)
}
return strings.Join(result, "\n")| func (g *GhosttyApp) Save(ctx context.Context, files map[string]string, isDryRun bool) error { | ||
| for path, remoteContent := range files { | ||
| expandedPath, err := g.expandPath(path) | ||
| if err != nil { | ||
| logrus.Warnf("Failed to expand path %s: %v", path, err) | ||
| continue | ||
| } | ||
|
|
||
| // Read existing local content if file exists | ||
| var localContent string | ||
| if existingBytes, err := os.ReadFile(expandedPath); err == nil { | ||
| localContent = string(existingBytes) | ||
| } else if !os.IsNotExist(err) { | ||
| logrus.Warnf("Failed to read existing file %s: %v", expandedPath, err) | ||
| continue | ||
| } | ||
|
|
||
| // Parse both configs | ||
| localLines := g.parseGhosttyConfig(localContent) | ||
| remoteLines := g.parseGhosttyConfig(remoteContent) | ||
|
|
||
| // Merge configs (local has priority) | ||
| mergedLines := g.mergeGhosttyConfigs(localLines, remoteLines) | ||
| mergedContent := g.formatGhosttyConfig(mergedLines) | ||
|
|
||
| // Check if there are any differences | ||
| if localContent == mergedContent { | ||
| logrus.Infof("No changes needed for %s", expandedPath) | ||
| continue | ||
| } | ||
|
|
||
| if isDryRun { | ||
| // In dry-run mode, show the diff | ||
| fmt.Printf("\n📄 %s:\n", expandedPath) | ||
| fmt.Println("--- Changes to be applied ---") | ||
|
|
||
| // Show added keys (from remote) | ||
| remoteKeys := make(map[string]string) | ||
| for _, line := range remoteLines { | ||
| if !line.isComment && !line.isBlank && line.key != "" { | ||
| remoteKeys[line.key] = line.value | ||
| } | ||
| } | ||
|
|
||
| localKeys := make(map[string]string) | ||
| for _, line := range localLines { | ||
| if !line.isComment && !line.isBlank && line.key != "" { | ||
| localKeys[line.key] = line.value | ||
| } | ||
| } | ||
|
|
||
| // Show new keys from remote | ||
| hasChanges := false | ||
| for key, value := range remoteKeys { | ||
| if _, exists := localKeys[key]; !exists { | ||
| fmt.Printf("+ %s = %s (from remote)\n", key, value) | ||
| hasChanges = true | ||
| } | ||
| } | ||
|
|
||
| if !hasChanges { | ||
| fmt.Println("No new keys from remote") | ||
| } | ||
|
|
||
| fmt.Println("--- End of changes ---") | ||
| continue | ||
| } | ||
|
|
||
| // Ensure directory exists | ||
| dir := filepath.Dir(expandedPath) | ||
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| logrus.Warnf("Failed to create directory %s: %v", dir, err) | ||
| continue | ||
| } | ||
|
|
||
| // Write merged content | ||
| if err := os.WriteFile(expandedPath, []byte(mergedContent), 0644); err != nil { | ||
| logrus.Warnf("Failed to save file %s: %v", expandedPath, err) | ||
| } else { | ||
| logrus.Infof("Saved merged config to %s", expandedPath) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The Save function iterates through a map of files, but if an error occurs (e.g., path expansion, file read, file write), it logs a warning and uses continue to proceed to the next file. The function then returns nil at the end, regardless of whether errors occurred. This can cause the calling command to report success even if some or all files failed to save.
To provide more accurate feedback on the operation's success, it would be better to collect any errors that occur within the loop and return a consolidated error at the end. This ensures that failures are propagated up and can be handled appropriately (e.g., by exiting with a non-zero status code).
Example of aggregating errors:
func (g *GhosttyApp) Save(...) error {
var errs []error
for path, remoteContent := range files {
// ... on error
if err != nil {
errs = append(errs, fmt.Errorf("failed to process %s: %w", path, err))
logrus.Warnf("Failed to process %s: %v", path, err)
continue
}
// ...
}
if len(errs) > 0 {
// Using a modern approach to join errors
return errors.Join(errs...)
}
return nil
}| remoteKeys := make(map[string]string) | ||
| for _, line := range remoteLines { | ||
| if !line.isComment && !line.isBlank && line.key != "" { | ||
| remoteKeys[line.key] = line.value | ||
| } | ||
| } | ||
|
|
||
| localKeys := make(map[string]string) | ||
| for _, line := range localLines { | ||
| if !line.isComment && !line.isBlank && line.key != "" { | ||
| localKeys[line.key] = line.value | ||
| } | ||
| } | ||
|
|
||
| // Show new keys from remote | ||
| hasChanges := false | ||
| for key, value := range remoteKeys { | ||
| if _, exists := localKeys[key]; !exists { | ||
| fmt.Printf("+ %s = %s (from remote)\n", key, value) | ||
| hasChanges = true | ||
| } | ||
| } |
There was a problem hiding this comment.
The dry-run block re-creates localKeys and remoteKeys maps and iterates through them to find new keys. This logic is redundant because mergeGhosttyConfigs already performs a similar operation to identify which remote keys are new.
To improve efficiency and reduce code duplication, you could refactor mergeGhosttyConfigs to also return the list of configLines that were added from the remote config. The Save method could then use this list directly for the dry-run output.
Example refactoring:
// In mergeGhosttyConfigs
func (g *GhosttyApp) mergeGhosttyConfigs(...) ([]configLine, []configLine) {
// ...
var addedLines []configLine
for _, remoteLine := range remoteLines {
if !localKeys[remoteLine.key] {
merged = append(merged, remoteLine)
addedLines = append(addedLines, remoteLine)
}
}
return merged, addedLines
}
// In Save method
mergedLines, addedLines := g.mergeGhosttyConfigs(localLines, remoteLines)
// ...
if isDryRun {
// ...
for _, line := range addedLines {
fmt.Printf("+ %s = %s (from remote)\n", line.key, line.value)
}
// ...
}This change would make the code more DRY (Don't Repeat Yourself) and slightly more performant.
1 participant
Summary
Description
This PR addresses issue #101 by implementing an advanced merge feature for dotfile configurations, starting with Ghostty terminal config files.
Features
Implementation Details
The new
Savemethod inGhosttyApp:This approach can be extended to other dotfile applications that use similar key-value configuration formats.
Test plan
Fixes #101
🤖 Generated with Claude Code