fix: stash uncommitted changes instead of aborting during ref checkout, update deps#24
fix: stash uncommitted changes instead of aborting during ref checkout, update deps#24
Conversation
Version AnalysisNext Version: Changelog: 📄 View detailed changelog in job summary |
There was a problem hiding this comment.
Pull request overview
This pull request improves the generateDocs function to automatically stash and restore uncommitted changes when checking out different git refs, instead of aborting with an error. This is particularly useful in CI environments where previous build steps (like flutter pub get) may create uncommitted artifacts.
Changes:
- Added
stashChanges()andpopStash()methods toGitUtilsclass for managing git stash operations - Replaced hard exit on uncommitted changes with automatic stashing before ref checkout
- Added stash restoration logic to
restoreOriginalState()function - Fixed missing
awaitonrestoreOriginalState()call in the finally block
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/doc_generator/git_utils.dart | Added two new methods: stashChanges() to stash uncommitted changes with a specific message, and popStash() to restore the most recent stash entry |
| lib/doc_generator/doc_generator.dart | Modified to stash uncommitted changes before checking out a different ref, restore them afterwards, and fixed missing await in finally block |
Comments suppressed due to low confidence (1)
lib/doc_generator/doc_generator.dart:50
- When
gitRefis 'HEAD', uncommitted changes are not stashed (line 36 condition is false), but the code still performs a checkout operation (line 50) which switches from the current branch to detached HEAD state at the same commit. This checkout operation could fail if there are uncommitted changes that would be overwritten. Consider either: 1) skipping the checkout entirely whengitRef == 'HEAD'and we're already at that commit, or 2) also checking and stashing uncommitted changes whengitRef == 'HEAD'to ensure the checkout succeeds.
if (gitRef != 'HEAD' && GitUtils.hasUncommittedChanges(gitRoot.path)) {
logger.info('Stashing uncommitted changes before checking out ref...');
didStash = await GitUtils.stashChanges(gitRoot.path);
}
final originalRef = await GitUtils.getCurrentRef(gitRoot.path);
final originalBranch = await GitUtils.getCurrentBranch(gitRoot.path);
// If the ref is HEAD, get the current ref (commit hash)
if (gitRef == 'HEAD') {
gitRef = await GitUtils.getCurrentRef(gitRoot.path);
}
logger.info('Checking out ref: $gitRef');
await GitUtils.checkoutRef(gitRef, gitRoot.path);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Pops the most recent stash entry. | ||
| /// Silently succeeds if there is nothing to pop. | ||
| static Future<void> popStash(String? root) async { | ||
| try { | ||
| final result = await Process.run( | ||
| 'git', | ||
| ['stash', 'pop'], | ||
| workingDirectory: root, | ||
| ); | ||
| if (result.exitCode != 0) { | ||
| // Log but don't throw – a pop failure shouldn't crash the tool | ||
| logger.err('Warning: Failed to pop stash: ${result.stderr}'); | ||
| } | ||
| } on ProcessException catch (e) { | ||
| throw GitException('Git command not found: ${e.message}'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The popStash method uses git stash pop without verifying which stash entry it's popping. If another process or user creates a stash between when this tool stashes and when it pops, the wrong stash could be restored. Consider using a more targeted approach: either use git stash apply stash@{n} after finding the correct stash by message, or use git stash pop with the stash reference that matches the message 'mtrust_api_guard_auto_stash'. This would make the stash operations more robust in multi-user or concurrent scenarios.
| static Future<void> popStash(String? root) async { | ||
| try { | ||
| final result = await Process.run( | ||
| 'git', | ||
| ['stash', 'pop'], | ||
| workingDirectory: root, | ||
| ); | ||
| if (result.exitCode != 0) { | ||
| // Log but don't throw – a pop failure shouldn't crash the tool | ||
| logger.err('Warning: Failed to pop stash: ${result.stderr}'); | ||
| } | ||
| } on ProcessException catch (e) { | ||
| throw GitException('Git command not found: ${e.message}'); | ||
| } | ||
| } |
There was a problem hiding this comment.
When git stash pop is executed, it can result in merge conflicts if the stashed changes conflict with the current working tree state. The current implementation logs an error but doesn't handle this scenario gracefully. Consider checking for conflicts after the pop operation and providing clear guidance to users on how to resolve them. You might want to check the exit code more specifically (exit code 1 with conflicts vs. other failures) and provide different messaging or recovery strategies.
| if (gitRef != 'HEAD' && GitUtils.hasUncommittedChanges(gitRoot.path)) { | ||
| logger.err('Repository has uncommitted changes. Please commit or stash them before proceeding.'); | ||
| logger.err('This prevents potential data loss during ref checkout.'); | ||
| exit(1); | ||
| logger.info('Stashing uncommitted changes before checking out ref...'); | ||
| didStash = await GitUtils.stashChanges(gitRoot.path); | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. The check hasUncommittedChanges is synchronous and happens at one point in time, but stashChanges is async and executes later. Between these two operations, the working tree state could theoretically change. While this is unlikely in practice for a single-threaded tool, it would be more robust to either make hasUncommittedChanges async or have stashChanges always check the state internally before attempting to stash. The current pattern where stashChanges returns false when nothing was stashed partially addresses this, but the initial check still creates a temporal gap.
| /// Stashes any uncommitted changes (including untracked files) | ||
| /// Returns true if changes were stashed, false if the working tree was clean. | ||
| static Future<bool> stashChanges(String? root) async { | ||
| try { | ||
| final result = await Process.run( | ||
| 'git', | ||
| ['stash', 'push', '--include-untracked', '-m', 'mtrust_api_guard_auto_stash'], | ||
| workingDirectory: root, | ||
| ); | ||
| if (result.exitCode != 0) { | ||
| throw GitException('Failed to stash changes: ${result.stderr}'); | ||
| } | ||
| // "No local changes to save" means nothing was stashed | ||
| return !result.stdout.toString().contains('No local changes to save'); | ||
| } on ProcessException catch (e) { | ||
| throw GitException('Git command not found: ${e.message}'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new stash functionality in GitUtils and its usage in generateDocs lacks test coverage. Given that the repository has comprehensive automated testing (as seen in test/commands/ directory with git operations being tested), these new methods should be covered by tests. Consider adding tests that verify: 1) stashing when there are uncommitted changes, 2) successful restoration of stashed changes, 3) handling of cases where nothing needs to be stashed, 4) proper error handling when stash operations fail, and 5) the complete flow in generateDocs when uncommitted changes are present.
flxapps
changed the title
|
closed as unfinished in favour of #15. |
Fixed the
generateDocscrash when the working tree has uncommitted changes (e.g. fromflutter pub getin CI). Instead of hard-exiting, the tool now stashes and restores changes automatically.Changes
stashChanges()andpopStash()methods toGitUtilsexit(1)on uncommitted changes with automaticgit stash push --include-untrackedrestoreOriginalState()so changes are always recoveredawaitonrestoreOriginalState()in thefinallyblock