Skip to content

feat: add --abort flag to dgr sync#21

Open
don-petry wants to merge 3 commits intooneirosoft:mainfrom
don-petry:feat/sync-abort
Open

feat: add --abort flag to dgr sync#21
don-petry wants to merge 3 commits intooneirosoft:mainfrom
don-petry:feat/sync-abort

Conversation

@don-petry
Copy link
Copy Markdown
Contributor

@don-petry don-petry commented Mar 31, 2026

Why?

When dgr sync hits a merge conflict, the user resolves it and runs --continue. But sometimes the user decides they don't want to proceed — the conflict is too complex, or they want to take a different approach. Without --abort, the only option is manually running git rebase --abort and deleting operation.json, which is error-prone and undocumented.

Summary

  • Adds --abort flag to dgr sync that safely cancels a paused restack operation by running git rebase --abort (if a rebase is in progress) and clearing operation.json
  • Validates that a pending operation exists before aborting, returning a clear error if there's nothing to abort
  • Makes --continue and --abort mutually exclusive via clap's conflicts_with
  • Updates the pending operation error message to mention --abort as an option alongside --continue
  • Includes tests for both the abort-success and abort-with-no-pending-operation paths

Addresses item 5 from #10.

Test plan

  • dgr sync --abort with no paused operation errors with "no paused dgr operation to abort"
  • dgr sync --abort after a conflict aborts the rebase and clears operation.json
  • dgr sync --continue --abort is rejected by clap as conflicting flags
  • dgr sync --abort prints "Aborted paused restack operation." on success

🤖 Generated with Claude Code

When a restack pauses on a conflict, users had to manually run
git rebase --abort and clean up operation.json. This adds dgr sync --abort
which safely aborts any in-progress git rebase and clears the pending
operation state. The flag is mutually exclusive with --continue via clap's
conflicts_with. The pending operation error message now mentions --abort
as an option alongside --continue.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 31, 2026 02:21
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an --abort escape hatch to dgr sync so users can cancel a paused restack operation by aborting any in-progress git rebase and clearing operation.json.

Changes:

  • Introduces --abort flag for dgr sync, with mutual exclusion vs --continue.
  • Implements core abort flow (abort_sync) and a git helper (git rebase --abort).
  • Updates the “pending operation” error message to suggest --abort as an alternative to --continue.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/core/workflow.rs Updates pending-operation error message to mention dgr sync --abort.
src/core/sync.rs Adds abort_operation option and abort_sync() implementation.
src/core/git.rs Adds abort_rebase() helper that runs git rebase --abort.
src/cli/sync/mod.rs Adds --abort CLI flag, enforces conflicts with --continue, prints success message, updates CLI→core option conversion and tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 18 to 22
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct SyncOptions {
pub continue_operation: bool,
pub abort_operation: bool,
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

Adding abort_operation to SyncOptions requires updating all SyncOptions { ... } struct literals. There is at least one existing literal later in this file (in the tests) that now omits abort_operation, which will not compile. Consider either adding the field at that call site(s) or switching those literals to ..Default::default() to make future option additions non-breaking.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed — verified no other SyncOptions literals exist in the codebase besides the test, which already has the field. Added the missing abort_operation: false to the test struct literal. The other usages all go through SyncOptions::default() which handles it via the derived Default impl.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b5801c8. All SyncOptions struct literals in production and test code now include the abort_operation field. The struct derives Default, so the three call sites using SyncOptions::default() correctly get abort_operation: false. The one explicit struct literal (in the resume test) already had the field from the prior commit. The new abort tests also use explicit struct literals with both fields.

src/core/sync.rs Outdated

fn abort_sync() -> io::Result<SyncOutcome> {
let session = open_initialized("dagger is not initialized; run 'dgr init' first")?;
let pending_operation = load_operation(&session.paths)?
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

pending_operation is loaded but never used in abort_sync, which will trigger an unused-variable warning. If warnings are treated as errors in CI, this will break the build. Rename it to _pending_operation or use it (e.g., include the origin in the abort output / error handling).

Suggested change
let pending_operation = load_operation(&session.paths)?
let _pending_operation = load_operation(&session.paths)?

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the latest push — renamed to _pending_operation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b5801c8. Replaced the unused _pending_operation variable binding with a direct is_none() check: if load_operation(&session.paths)?.is_none() { return Err(...); }. This validates that a pending operation exists before proceeding (returning a clear error if there is nothing to abort) without binding an unused variable.

Comment on lines +315 to +323
fn abort_sync() -> io::Result<SyncOutcome> {
let session = open_initialized("dagger is not initialized; run 'dgr init' first")?;
let pending_operation = load_operation(&session.paths)?
.ok_or_else(|| io::Error::other("no paused dgr operation to abort"))?;

if git::is_rebase_in_progress(&session.repo) {
let abort_output = git::abort_rebase()?;
if !abort_output.status.success() {
return Ok(SyncOutcome {
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The new --abort/abort_sync path doesn’t appear to have coverage in the existing src/core/sync.rs test module (which already exercises --continue resume behavior). Add tests for: (1) erroring when there’s no paused operation, (2) aborting an in-progress rebase, and (3) clearing operation.json on success.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Integration test coverage for abort requires a full conflicting restack setup. Added as a follow-up TODO. The core abort logic is straightforward (check pending op, abort rebase, clear operation file) and is covered by the existing operation persistence tests for the underlying primitives.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b5801c8. Added two tests for the abort path:

  1. abort_cancels_paused_sync_and_clears_pending_operation -- sets up a real conflict (divergent shared.txt on parent and child branches), verifies sync pauses with a pending operation, then calls abort and asserts: the outcome succeeds, paused is false, and the pending operation file is cleared.

  2. abort_returns_error_when_no_pending_operation -- calls abort on an initialized repo with no paused operation and verifies it returns the expected error message.

DJ and others added 2 commits March 30, 2026 19:30
- Prefix unused `pending_operation` variable with underscore
- Add missing `abort_operation` field to SyncOptions struct literal in test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused variable binding in abort_sync; use is_none() check
  directly for clearer control flow
- Confirm all SyncOptions struct literals include abort_operation field
  (all existing uses already covered via Default or explicit fields)
- Add test coverage for abort_sync: verify abort clears pending
  operation after a paused sync, and returns an error when no
  pending operation exists

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants