Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 15 additions & 10 deletions src/jj/bookmarks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,16 @@ pub(crate) async fn resolve_stack_resolution(
Ok(resolution)
}

/// Resolve the stack when nothing is owned but frozen bookmarks are still in
/// scope. Returns `Ok(Some(..))` when `@` sits on the frozen dependency chain
/// (still building on top of not-yet-merged deps), or `Ok(None)` when `@` is
/// detached from every frozen chain — e.g. the frozen deps merged and we are
/// parked on a plain trunk-ish commit. Callers treat `None` as "nothing to
/// sync here; advance to trunk" rather than an error.
pub(crate) async fn resolve_purely_frozen_stack(
runner: &impl CommandRunner,
frozen_bookmarks: Vec<FrozenBookmark>,
) -> Result<StackResolution> {
) -> Result<Option<StackResolution>> {
if frozen_bookmarks.is_empty() {
bail!(
CliError::new("empty owned stack and no frozen bookmarks in scope")
Expand All @@ -179,10 +185,10 @@ pub(crate) async fn resolve_purely_frozen_stack(
if let Some(frozen_dependencies) =
frozen_dependency_chain_ending_at(runner, &frozen_changes, &at_commit).await?
{
return Ok(StackResolution {
return Ok(Some(StackResolution {
owned: Vec::new(),
frozen_dependencies,
});
}));
}

let current = resolve_stack(runner, "@").await.context("resolve current revision for frozen sync")?;
Expand All @@ -192,20 +198,19 @@ pub(crate) async fn resolve_purely_frozen_stack(
if let Some(frozen_dependencies) =
frozen_dependency_chain_ending_at(runner, &frozen_changes, parent).await?
{
return Ok(StackResolution {
return Ok(Some(StackResolution {
owned: Vec::new(),
frozen_dependencies,
});
}));
}
}
}
};

bail!(CliError::new(format!(
"empty owned stack and current revision {} is not a `forklift/frozen/pr-*` bookmark target or empty child of one",
short_commit_id(&at_commit)
))
.resolution("run `forklift get <pr>` first, or move to a mutable stack"));
// `@` is off every frozen chain with nothing owned to sync. The frozen
// bookmarks are stale leftovers (their PRs merged, or we moved away); don't
// block getting home — signal the caller to advance to trunk.
Ok(None)
}

pub(crate) async fn resolve_frozen_changes(
Expand Down
70 changes: 55 additions & 15 deletions src/workflows/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,30 +175,43 @@ pub(crate) async fn sync_stack(
// fetched remote tip and finish, reporting any branches we cleaned up rather
// than failing on the empty stack.
if stack.is_empty() && frozen_bookmarks.is_empty() {
diagnostics.phase("move-trunk");
move_trunk_to_remote(runner, config, diagnostics)
.await
.map_err(|error| phase_error("move-trunk", &config.trunk, error))?;
carry_empty_working_copy_to_trunk(runner, config, diagnostics)
.await
.map_err(|error| phase_error("carry-working-copy", "@", error))?;
return Ok(SyncSummary {
rebased_roots: 0,
submit_ran: false,
return finish_sync_at_trunk(
runner,
config,
cleaned_branches,
pruned_duplicates,
conflicts: 0,
});
diagnostics,
)
.await;
}
let stack_resolution = if stack.is_empty() {
resolve_purely_frozen_stack(runner, frozen_bookmarks).await
match resolve_purely_frozen_stack(runner, frozen_bookmarks)
.await
.map_err(|error| phase_error("resolve-stack", revset, error))?
{
Some(resolution) => resolution,
// `@` is detached from every frozen chain with nothing owned to sync
// (e.g. the frozen deps merged and we're parked on a plain commit).
// Advance to trunk and finish instead of erroring — same as an empty
// stack with no frozen bookmarks.
None => {
return finish_sync_at_trunk(
runner,
config,
cleaned_branches,
pruned_duplicates,
diagnostics,
)
.await;
}
}
} else {
match validate_stack_shape(runner, &stack, revset).await {
Ok(()) => resolve_stack_resolution(runner, stack, frozen_bookmarks).await,
Err(error) => Err(error),
}
}
.map_err(|error| phase_error("resolve-stack", revset, error))?;
.map_err(|error| phase_error("resolve-stack", revset, error))?
};
if diagnostics.verbose {
print_stack(&stack_resolution.owned);
}
Expand Down Expand Up @@ -322,6 +335,33 @@ pub(crate) async fn sync_stack(
})
}

/// Finish a sync that has nothing left to do: move trunk to the fetched remote
/// tip and carry the empty working copy onto it. Used when the owned stack is
/// empty and either no frozen bookmarks remain or `@` is detached from every
/// frozen chain — the "get me back to trunk" recovery path.
async fn finish_sync_at_trunk(
runner: &impl CommandRunner,
config: &AppConfig,
cleaned_branches: usize,
pruned_duplicates: usize,
diagnostics: Diagnostics,
) -> Result<SyncSummary> {
diagnostics.phase("move-trunk");
move_trunk_to_remote(runner, config, diagnostics)
.await
.map_err(|error| phase_error("move-trunk", &config.trunk, error))?;
carry_empty_working_copy_to_trunk(runner, config, diagnostics)
.await
.map_err(|error| phase_error("carry-working-copy", "@", error))?;
Ok(SyncSummary {
rebased_roots: 0,
submit_ran: false,
cleaned_branches,
pruned_duplicates,
conflicts: 0,
})
}

pub(crate) async fn prune_landed_duplicate_changes(
runner: &impl CommandRunner,
config: &AppConfig,
Expand Down
34 changes: 34 additions & 0 deletions tests/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,40 @@ fn sync_from_empty_child_above_frozen_pr_succeeds() -> anyhow::Result<()> {
Ok(())
}

#[test]
fn sync_from_detached_commit_with_leftover_frozen_advances_to_trunk() -> anyhow::Result<()> {
// After `get` freezes a stack and its PRs later merge, the working copy can
// end up on a plain commit that is no longer on any frozen chain. Sync must
// recover by advancing to trunk instead of erroring that `@` is not a frozen
// bookmark target.
let repo = TestRepo::new("sync-detached-frozen")?;
repo.init_main()?;
let imported = repo.create_change("imported", "imported title", "imported body")?;
let branch = branch_for("imported-title", &imported.change_id);
repo.set_bookmark(&branch, &imported.commit_id)?;
repo.push_bookmark(&branch)?;
repo.seed_pr(11, &branch, "main", "imported title", "imported body")?;

let get_output = repo.run(&["get", "11"])?;
assert_success("get 11", &get_output);

// Move off the frozen chain onto a fresh empty child of trunk, leaving the
// `forklift/frozen/pr-11` bookmark behind.
repo.jj(&["new", "main"])?;

let output = repo.run(&["sync"])?;
assert_success("sync", &output);
let stderr = stderr_of(&output);
// Recovered by advancing to trunk (Moving trunk) rather than erroring on the
// detached `@`, with nothing owned to rebase.
assert!(stderr.contains("Moving trunk"), "stderr:\n{stderr}");
assert!(
stderr.contains("0 roots rebased"),
"stderr:\n{stderr}"
);
Ok(())
}

#[test]
fn sync_frozen_suffix_based_on_unfrozen_parent_succeeds() -> anyhow::Result<()> {
let repo = TestRepo::new("sync-frozen-suffix")?;
Expand Down