init: Skip devcontainer.json write when content unchanged#13655
init: Skip devcontainer.json write when content unchanged#13655
Conversation
- Compare normalized JSON before and after to detect actual changes - Skip file write if content is semantically identical - Add test to verify file modification time doesn't change on idempotent runs - Preserves file timestamps when no changes are needed Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR modifies the init --codespaces command to skip writing devcontainer.json when the configuration is unchanged, avoiding spurious git diffs and file timestamp updates.
Changes:
- Added JSON comparison logic to detect when devcontainer.json content is unchanged
- Skip file write when serialized configs are identical
- Enhanced existing test with mtime verification
- Added new test
TestEnsureDevcontainerConfigNoWriteWhenUnchanged
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cli/devcontainer.go | Implements comparison between existing and new configs by serializing both to JSON and checking equality before writing |
| pkg/cli/devcontainer_test.go | Adds mtime checks to existing test and new dedicated test to verify write-skipping behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If file exists, check if content has changed (compare normalized JSON) | ||
| if existingConfig != nil { | ||
| // Serialize the existing config to compare | ||
| existingNormalized, err := json.MarshalIndent(existingConfig, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal existing config for comparison: %w", err) | ||
| } | ||
| existingNormalized = append(existingNormalized, '\n') | ||
|
|
||
| // Compare normalized JSON - if they're the same, skip writing | ||
| if string(newData) == string(existingNormalized) { | ||
| devcontainerLog.Printf("No changes detected, skipping write: %s", devcontainerPath) | ||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "No changes to devcontainer.json\n") | ||
| } | ||
| return nil | ||
| } | ||
| devcontainerLog.Printf("Changes detected, will update file: %s", devcontainerPath) | ||
| } |
There was a problem hiding this comment.
Comparison logic flaw: The comparison compares the newly computed config (after merge operations) against existingConfig. However, due to a shallow copy on line 112 (config = *existingConfig), the pointer fields (Customizations, Build) and map field (Features) are shared between config and existingConfig. When these shared structures are modified through config (lines 114-160), existingConfig is also mutated. This causes the comparison to always succeed because both variables point to the same mutated data, making the skip-write optimization ineffective for detecting actual changes.
A more robust approach would be to compare against the original file content before any mutations, or preserve the original existingConfig through deep copying before mutations.
| func TestEnsureDevcontainerConfigNoWriteWhenUnchanged(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "test-*") | ||
|
|
||
| originalDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatalf("Failed to get current directory: %v", err) | ||
| } | ||
| defer func() { | ||
| _ = os.Chdir(originalDir) | ||
| }() | ||
|
|
||
| if err := os.Chdir(tmpDir); err != nil { | ||
| t.Fatalf("Failed to change to temp directory: %v", err) | ||
| } | ||
|
|
||
| // Initialize git repo | ||
| if err := exec.Command("git", "init").Run(); err != nil { | ||
| t.Skip("Git not available") | ||
| } | ||
|
|
||
| // Configure git and add remote | ||
| exec.Command("git", "config", "user.name", "Test User").Run() | ||
| exec.Command("git", "config", "user.email", "test@example.com").Run() | ||
| exec.Command("git", "remote", "add", "origin", "https://github.com/testorg/testrepo.git").Run() | ||
|
|
||
| // Create initial devcontainer.json | ||
| err = ensureDevcontainerConfig(false, []string{}) | ||
| if err != nil { | ||
| t.Fatalf("Initial ensureDevcontainerConfig() failed: %v", err) | ||
| } | ||
|
|
||
| devcontainerPath := filepath.Join(".devcontainer", "devcontainer.json") | ||
|
|
||
| // Get file info after first write | ||
| firstStat, err := os.Stat(devcontainerPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to stat file after first write: %v", err) | ||
| } | ||
| firstModTime := firstStat.ModTime() | ||
|
|
||
| // Read the first content | ||
| firstContent, err := os.ReadFile(devcontainerPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read file after first write: %v", err) | ||
| } | ||
|
|
||
| // Run again with same parameters - should not write | ||
| err = ensureDevcontainerConfig(false, []string{}) | ||
| if err != nil { | ||
| t.Fatalf("Second ensureDevcontainerConfig() failed: %v", err) | ||
| } | ||
|
|
||
| // Get file info after second run | ||
| secondStat, err := os.Stat(devcontainerPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to stat file after second run: %v", err) | ||
| } | ||
| secondModTime := secondStat.ModTime() | ||
|
|
||
| // Read the second content | ||
| secondContent, err := os.ReadFile(devcontainerPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read file after second run: %v", err) | ||
| } | ||
|
|
||
| // Modification times should be equal (file was not rewritten) | ||
| if !firstModTime.Equal(secondModTime) { | ||
| t.Errorf("File was rewritten when no changes were needed. First modtime: %v, Second modtime: %v", firstModTime, secondModTime) | ||
| } | ||
|
|
||
| // Content should be identical | ||
| if string(firstContent) != string(secondContent) { | ||
| t.Error("File content changed when it should not have") | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for parameter changes: The tests verify that the file isn't rewritten when called with identical parameters, but they don't test whether the file IS correctly updated when parameters change (e.g., different additionalRepos). This is important because the comparison logic has a shallow copy issue that could cause updates to be skipped incorrectly.
Consider adding a test that:
- Creates a devcontainer.json with additionalRepos=[]
- Runs again with additionalRepos=["new/repo"]
- Verifies the file WAS updated and contains the new repo
The
init --codespacescommand unconditionally writesdevcontainer.jsonon every run, creating spurious git diffs and file timestamp changes even when configuration is identical.Changes
TestEnsureDevcontainerConfigNoWriteWhenUnchangedverifying file mtime preservation on idempotent runsImplementation
JSON serialization normalizes map key ordering and formatting, providing reliable semantic equality checks between the parsed existing config and the recomputed config.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.