Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(rewrite): fix cases of moving all parents of merge commits #915

Merged
merged 1 commit into from
Apr 25, 2023
Merged

fix(rewrite): fix cases of moving all parents of merge commits #915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
107 changes: 48 additions & 59 deletions git-branchless-lib/src/core/rewrite/plan.rs
View file
Open in desktop
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
Open in desktop
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