Skip to content

Commit

Permalink
fix(rewrite): fix cases of moving all parents of merge commits
Browse files Browse the repository at this point in the history 
The existing implementation of `collect_descendants` was from a time before the segmented changelog DAG, and it didn't handle certain merge commits correctly (as can be determined from the fact that `parent_oids` never has more than one parent). Fortunately, we can remove the old recursive implementation and use the segmented changelog DAG instead.

Many of the merge commit issues were documented by @claytonrcarter in his thorough testing of `git reword`, for which we can now remove the `FIXME` comments from the test code 😊.

There was also a bug where we failed to apply the children of a merge commit by indiscriminately returning `Ok(acc)` after finishing the application of the merge commit, which is also fixed in this commit.
  • Loading branch information
@arxanas
arxanas committed Apr 23, 2023
1 parent 5dd5cca commit 573a8b8
Show file tree
Hide file tree
Showing 3 changed files with 312 additions and 193 deletions.
107 changes: 48 additions & 59 deletions git-branchless-lib/src/core/rewrite/plan.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,48 +320,31 @@ impl<'a> ConstraintGraph<'a> {
Ok(())
}

#[instrument]
fn collect_descendants(
&self,
visible_commits: &CommitSet,
acc: &mut Vec<Constraint>,
current_oid: NonZeroOid,
) -> eyre::Result<()> {
let children_oids = self
.dag
.query_children(CommitSet::from(current_oid))?
.intersection(visible_commits);
let children_oids = self.dag.commit_set_to_vec(&children_oids)?;
for child_oid in children_oids {
if self.commits_to_move().contains(&child_oid) {
continue;
}
acc.push(Constraint::MoveSubtree {
parent_oids: vec![current_oid],
child_oid,
});
self.collect_descendants(visible_commits, acc, child_oid)?;
}
Ok(())
}

/// Add additional edges to the constraint graph for each descendant commit
/// of a referred-to commit. This adds enough information to the constraint
/// graph that it now represents the actual end-state commit graph that we
/// want to create, not just a list of constraints.
fn add_descendant_constraints(&mut self, effects: &Effects) -> eyre::Result<()> {
let (effects, progress) = effects.start_operation(OperationType::ConstrainCommits);
let (effects, _progress) = effects.start_operation(OperationType::ConstrainCommits);
let _effects = effects;

let all_descendants_of_constrained_nodes = {
let visible_commits = self.dag.query_visible_commits_slow()?;

let mut acc = Vec::new();
let parents = self.commits_to_move();
progress.notify_progress(0, parents.len());
for parent_oid in parents {
self.collect_descendants(visible_commits, &mut acc, parent_oid)?;
progress.notify_progress_inc(1);
let commits_to_move: CommitSet = self.commits_to_move().into_iter().collect();
let descendants = self.dag.query_descendants(commits_to_move.clone())?;
let descendants = descendants.difference(&commits_to_move);
let descendants = self.dag.filter_visible_commits(descendants)?;
let descendant_oids = self.dag.commit_set_to_vec(&descendants)?;
for descendant_oid in descendant_oids {
let parents = self.dag.query_parent_names(descendant_oid)?;
let parent_oids = parents
.into_iter()
.map(NonZeroOid::try_from)
.try_collect()?;
acc.push(Constraint::MoveSubtree {
parent_oids,
child_oid: descendant_oid,
});
}
acc
};
Expand Down Expand Up @@ -796,33 +779,39 @@ impl<'a> RebasePlanBuilder<'a> {
}
};

if let Some(commits_to_merge) = commits_to_merge {
// All parents have been committed.
let (first_parent, merge_parents) = match commits_to_merge.as_slice() {
[] => {
unreachable!("Already verified that there's at least one parent commit")
}
[first, rest @ ..] => (first, rest.to_vec()),
};
acc.push(RebaseCommand::Reset {
target: first_parent.clone(),
});
acc.push(
match self.replacement_commits.get(&current_commit.get_oid()) {
None => RebaseCommand::Merge {
commit_oid: current_commit.get_oid(),
commits_to_merge: merge_parents.to_vec(),
},
Some(replacement_commit_oid) => RebaseCommand::Replace {
commit_oid: current_commit.get_oid(),
replacement_commit_oid: *replacement_commit_oid,
parents: commits_to_merge,
},
},
);
}
let commits_to_merge = match commits_to_merge {
None => {
// Some parents need to be committed. Wait for caller to come back to this commit later and then proceed to any child commits.
return Ok(acc);
}
Some(commits_to_merge) => {
// All parents have been committed.
commits_to_merge
}
};

return Ok(acc);
let (first_parent, merge_parents) = match commits_to_merge.as_slice() {
[] => {
unreachable!("Already verified that there's at least one parent commit")
}
[first, rest @ ..] => (first, rest.to_vec()),
};
acc.push(RebaseCommand::Reset {
target: first_parent.clone(),
});
acc.push(
match self.replacement_commits.get(&current_commit.get_oid()) {
None => RebaseCommand::Merge {
commit_oid: current_commit.get_oid(),
commits_to_merge: merge_parents.to_vec(),
},
Some(replacement_commit_oid) => RebaseCommand::Replace {
commit_oid: current_commit.get_oid(),
replacement_commit_oid: *replacement_commit_oid,
parents: commits_to_merge,
},
},
);
} else {
// Normal one-parent commit (or a zero-parent commit?), just
// rebase it and continue.
Expand Down
48 changes: 19 additions & 29 deletions git-branchless-lib/tests/test_rewrite_plan.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,35 +632,25 @@ fn test_plan_moving_subtree_with_merge_commit() -> eyre::Result<()> {
Ok(())
})?;

// FIXME: this is wrong, the merge commit should be moved as well.
let stdout = git.smartlog()?;
insta::assert_snapshot!(stdout, @r###"
O f777ecc (master) create initial.txt
|
o 62fc20d create test1.txt
|
o 96d1c37 create test2.txt
|\
| o b8f27a8 create test3.txt
| |\
| | @ 2b47b50 create test5.txt
| |
| o 22cf458 create test4.txt
|
x 70deb1e (rewritten as b8f27a86) create test3.txt
|\
| x 355e173 (rewritten as 22cf4586) create test4.txt
| & (merge) 8fb706a Merge commit '355e173bf9c5d2efac2e451da0cdad3fb82b869a' into HEAD
|
x 9ea1b36 (rewritten as 2b47b505) create test5.txt
|
| & (merge) 355e173 (rewritten as 22cf4586) create test4.txt
|/
o 8fb706a Merge commit '355e173bf9c5d2efac2e451da0cdad3fb82b869a' into HEAD
hint: there is 1 abandoned commit in your commit graph
hint: to fix this, run: git restack
hint: disable this hint by running: git config --global branchless.hint.smartlogFixAbandoned false
"###);
O f777ecc (master) create initial.txt
|
o 62fc20d create test1.txt
|
o 96d1c37 create test2.txt
|
o 70deb1e create test3.txt
|\
| o 355e173 create test4.txt
| & (merge) 8fb706a Merge commit '355e173bf9c5d2efac2e451da0cdad3fb82b869a' into HEAD
|
@ 9ea1b36 create test5.txt
|
| & (merge) 355e173 create test4.txt
|/
o 8fb706a Merge commit '355e173bf9c5d2efac2e451da0cdad3fb82b869a' into HEAD
"###);

Ok(())
}
Expand Down Expand Up @@ -715,9 +705,9 @@ fn create_and_execute_plan(
now,
event_tx_id: event_log_db.make_transaction_id(now, "test plan")?,
preserve_timestamps: false,
force_in_memory: true,
force_in_memory: false,
force_on_disk: false,
resolve_merge_conflicts: false,
resolve_merge_conflicts: true,
check_out_commit_options: CheckOutCommitOptions {
additional_args: Default::default(),
reset: false,
Expand Down

0 comments on commit 573a8b8

Please sign in to comment.