Skip to content

fix: only update .gitattributes if needed, skip if already up to date #24124

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

Merged
pelikhan merged 1 commit into from
Apr 2, 2026
Merged

fix: only update .gitattributes if needed, skip if already up to date #24124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/cli/add_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,13 @@ func addWorkflowsWithTracking(workflows []*ResolvedWorkflow, tracker *FileTracke
// Ensure .gitattributes is configured unless flag is set
if !opts.NoGitattributes {
addLog.Print("Configuring .gitattributes")
if err := ensureGitAttributes(); err != nil {
if updated, err := ensureGitAttributes(); err != nil {
addLog.Printf("Failed to configure .gitattributes: %v", err)
if opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err)))
}
// Don't fail the entire operation if gitattributes update fails
} else if opts.Verbose {
} else if updated && opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes"))
}
}
Expand Down
40 changes: 21 additions & 19 deletions pkg/cli/add_workflow_compilation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func compileWorkflowWithRefresh(filePath string, verbose bool, quiet bool, engin
addWorkflowCompilationLog.Print("Compilation completed successfully")

// Ensure .gitattributes marks .lock.yml files as generated
if err := ensureGitAttributes(); err != nil {
if _, err := ensureGitAttributes(); err != nil {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err)))
}
Expand Down Expand Up @@ -75,15 +75,16 @@ func compileWorkflowWithTrackingAndRefresh(filePath string, verbose bool, quiet

addWorkflowCompilationLog.Printf("Lock file %s exists: %v", lockFile, lockFileExists)

// Check if .gitattributes exists before ensuring it
gitRoot, err := gitutil.FindGitRoot()
if err != nil {
return err
}
gitAttributesPath := filepath.Join(gitRoot, ".gitattributes")
gitAttributesExists := false
if _, err := os.Stat(gitAttributesPath); err == nil {
gitAttributesExists = true
// Check if .gitattributes exists before compilation so we know whether to
// use TrackCreated or TrackModified if ensureGitAttributes modifies it later.
gitRoot, gitRootErr := gitutil.FindGitRoot()
gitAttributesPath := ""
gitAttributesExisted := false
if gitRootErr == nil {
gitAttributesPath = filepath.Join(gitRoot, ".gitattributes")
if _, err := os.Stat(gitAttributesPath); err == nil {
gitAttributesExisted = true
}
}

// Track the lock file before compilation
Expand All @@ -93,13 +94,6 @@ func compileWorkflowWithTrackingAndRefresh(filePath string, verbose bool, quiet
tracker.TrackCreated(lockFile)
}

// Track .gitattributes file before modification
if gitAttributesExists {
tracker.TrackModified(gitAttributesPath)
} else {
tracker.TrackCreated(gitAttributesPath)
}

// Create compiler with auto-detected version and action mode
compiler := workflow.NewCompiler(
workflow.WithVerbose(verbose),
Expand All @@ -112,11 +106,19 @@ func compileWorkflowWithTrackingAndRefresh(filePath string, verbose bool, quiet
return err
}

// Ensure .gitattributes marks .lock.yml files as generated
if err := ensureGitAttributes(); err != nil {
// Ensure .gitattributes marks .lock.yml files as generated; only track it if it was actually
// modified. Errors here are non-fatal — gitattributes update failure does not prevent the
// compiled workflow from being usable.
if updated, err := ensureGitAttributes(); err != nil {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err)))
}
} else if updated && gitRootErr == nil {
if gitAttributesExisted {
tracker.TrackModified(gitAttributesPath)
} else {
tracker.TrackCreated(gitAttributesPath)
}
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/compile_file_operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func compileModifiedFilesWithDependencies(compiler *workflow.Compiler, depGraph
// Ensure .gitattributes marks .lock.yml files as generated
// Only update if we successfully compiled workflows or have action cache entries
if successCount > 0 || hasActionCacheEntries {
if err := ensureGitAttributes(); err != nil {
if _, err := ensureGitAttributes(); err != nil {
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err)))
}
Expand Down
13 changes: 9 additions & 4 deletions pkg/cli/compile_post_processing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,21 @@ func updateGitAttributes(successCount int, actionCache *workflow.ActionCache, ve
// Only update if we successfully compiled workflows or have action cache entries
if successCount > 0 || hasActionCacheEntries {
compilePostProcessingLog.Printf("Updating .gitattributes (compiled=%d, actionCache=%v)", successCount, hasActionCacheEntries)
if err := ensureGitAttributes(); err != nil {
updated, err := ensureGitAttributes()
if err != nil {
compilePostProcessingLog.Printf("Failed to update .gitattributes: %v", err)
if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err)))
}
return err
}
compilePostProcessingLog.Printf("Successfully updated .gitattributes")
if verbose {
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated .gitattributes to mark .lock.yml files as generated"))
if updated {
compilePostProcessingLog.Printf("Successfully updated .gitattributes")
if verbose {
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Updated .gitattributes to mark .lock.yml files as generated"))
}
} else {
compilePostProcessingLog.Print(".gitattributes already up to date")
}
} else {
compilePostProcessingLog.Print("Skipping .gitattributes update (no compiled workflows and no action cache entries)")
Expand Down
13 changes: 7 additions & 6 deletions pkg/cli/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,13 @@ func stageWorkflowChanges() {
}
}

// ensureGitAttributes ensures that .gitattributes contains the entry to mark .lock.yml files as generated
func ensureGitAttributes() error {
// ensureGitAttributes ensures that .gitattributes contains the entry to mark .lock.yml files as generated.
// It returns true if the file was modified, false if it was already up to date.
func ensureGitAttributes() (bool, error) {
gitLog.Print("Ensuring .gitattributes is updated")
gitRoot, err := gitutil.FindGitRoot()
if err != nil {
return err // Not in a git repository, skip
return false, err // Not in a git repository, skip
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

The inline comment // Not in a git repository, skip is misleading because this branch returns a non-nil error (which some callers treat as fatal). Consider rewording to reflect that the error is propagated so callers can decide whether to skip or fail (or adjust behavior to truly “skip” by returning (false, nil) if that’s the intent).

Suggested change
return false, err // Not in a git repository, skip
return false, err // Propagate error (e.g., when not in a git repository) so caller can decide how to handle it

Copilot uses AI. Check for mistakes.
}

gitAttributesPath := filepath.Join(gitRoot, ".gitattributes")
Expand Down Expand Up @@ -265,18 +266,18 @@ func ensureGitAttributes() error {

if !modified {
gitLog.Print(".gitattributes already contains required entries")
return nil
return false, nil
}

// Write back to file with owner-only read/write permissions (0600) for security best practices
content := strings.Join(lines, "\n")
if err := os.WriteFile(gitAttributesPath, []byte(content), 0600); err != nil {
gitLog.Printf("Failed to write .gitattributes: %v", err)
return fmt.Errorf("failed to write .gitattributes: %w", err)
return false, fmt.Errorf("failed to write .gitattributes: %w", err)
}

gitLog.Print("Successfully updated .gitattributes")
return nil
return true, nil
}

// stageGitAttributesIfChanged stages .gitattributes if it was modified
Expand Down
25 changes: 23 additions & 2 deletions pkg/cli/gitattributes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,31 +42,37 @@ func TestEnsureGitAttributes(t *testing.T) {
name string
existingContent string
expectedContent string
expectUpdated bool
}{
{
name: "creates new gitattributes file",
existingContent: "",
expectedContent: ".github/workflows/*.lock.yml linguist-generated=true merge=ours",
expectUpdated: true,
},
{
name: "adds entry to existing file",
existingContent: "*.generated linguist-generated=true\n",
expectedContent: "*.generated linguist-generated=true\n\n.github/workflows/*.lock.yml linguist-generated=true merge=ours",
expectUpdated: true,
},
{
name: "does not duplicate existing entry",
existingContent: ".github/workflows/*.lock.yml linguist-generated=true merge=ours\n",
expectedContent: ".github/workflows/*.lock.yml linguist-generated=true merge=ours",
expectUpdated: false,
},
{
name: "does not duplicate entry with different order",
existingContent: "*.md linguist-documentation=true\n.github/workflows/*.lock.yml linguist-generated=true merge=ours\n*.txt text=auto\n",
expectedContent: "*.md linguist-documentation=true\n.github/workflows/*.lock.yml linguist-generated=true merge=ours\n*.txt text=auto",
expectUpdated: false,
},
{
name: "updates old format entry",
existingContent: "*.md linguist-documentation=true\n.github/workflows/*.lock.yml linguist-generated=true\n*.txt text=auto\n",
expectedContent: "*.md linguist-documentation=true\n.github/workflows/*.lock.yml linguist-generated=true merge=ours\n*.txt text=auto",
expectUpdated: true,
},
}

Expand All @@ -85,11 +91,15 @@ func TestEnsureGitAttributes(t *testing.T) {
}

// Call the function
err := ensureGitAttributes()
updated, err := ensureGitAttributes()
if err != nil {
t.Fatalf("ensureGitAttributes() returned error: %v", err)
}

if updated != tt.expectUpdated {
t.Errorf("ensureGitAttributes() updated=%v, want %v", updated, tt.expectUpdated)
}

// Check that file exists
if _, err := os.Stat(gitAttributesPath); os.IsNotExist(err) {
t.Fatalf("Expected .gitattributes file to exist")
Expand All @@ -116,6 +126,17 @@ func TestEnsureGitAttributes(t *testing.T) {
if strings.Contains(string(content), ".github/workflows/*.campaign.g.md") {
t.Errorf("Did not expect .gitattributes to contain '.github/workflows/*.campaign.g.md' (should be in .gitignore)")
}

// Verify that calling again when already up to date returns updated=false
if tt.expectUpdated {
updatedAgain, errAgain := ensureGitAttributes()
if errAgain != nil {
t.Fatalf("ensureGitAttributes() second call returned error: %v", errAgain)
}
if updatedAgain {
t.Errorf("ensureGitAttributes() second call updated=true, want false (already up to date)")
}
}
Comment on lines +131 to +139
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

This “second call” assertion only runs when the first call returned updated=true. To actually verify the stated behavior “calling again when already up to date returns updated=false”, it should also run for cases where the initial file was already correct (expectUpdated=false). Consider always doing a second call at the end of each test case and asserting updatedAgain == false (and that the content is unchanged).

Suggested change
if tt.expectUpdated {
updatedAgain, errAgain := ensureGitAttributes()
if errAgain != nil {
t.Fatalf("ensureGitAttributes() second call returned error: %v", errAgain)
}
if updatedAgain {
t.Errorf("ensureGitAttributes() second call updated=true, want false (already up to date)")
}
}
updatedAgain, errAgain := ensureGitAttributes()
if errAgain != nil {
t.Fatalf("ensureGitAttributes() second call returned error: %v", errAgain)
}
if updatedAgain {
t.Errorf("ensureGitAttributes() second call updated=true, want false (already up to date)")
}
// Verify that the content is unchanged after the second call
contentAfter, errAfter := os.ReadFile(gitAttributesPath)
if errAfter != nil {
t.Fatalf("Failed to read .gitattributes after second call: %v", errAfter)
}
if string(contentAfter) != string(content) {
t.Errorf("Expected .gitattributes content to remain unchanged after second call.\nBefore:\n%s\n\nAfter:\n%s", string(content), string(contentAfter))
}

Copilot uses AI. Check for mistakes.
})
}
}
Expand All @@ -142,7 +163,7 @@ func TestEnsureGitAttributesNotInGitRepo(t *testing.T) {
}

// Call ensureGitAttributes in non-git directory
err = ensureGitAttributes()
_, err = ensureGitAttributes()
if err == nil {
t.Errorf("Expected error when not in git repository, got nil")
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/cli/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,10 @@ func InitRepository(opts InitOptions) error {

// Configure .gitattributes
initLog.Print("Configuring .gitattributes")
if err := ensureGitAttributes(); err != nil {
if updated, err := ensureGitAttributes(); err != nil {
initLog.Printf("Failed to configure .gitattributes: %v", err)
return fmt.Errorf("failed to configure .gitattributes: %w", err)
}
if opts.Verbose {
} else if updated && opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes"))
}

Expand Down
Loading