Skip to content

Commit

Permalink
rebase: allow -r to accept multiple revisions
Browse files Browse the repository at this point in the history
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Apr 24, 2024
1 parent 988ca2b commit 940ac95
Show file tree
Hide file tree
Showing 4 changed files with 406 additions and 93 deletions.
275 changes: 203 additions & 72 deletions cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,12 @@ use std::sync::Arc;
use clap::ArgGroup;
use indexmap::IndexSet;
use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::commit::{Commit, CommitIteratorExt};
use jj_lib::object_id::ObjectId;
use jj_lib::repo::{ReadonlyRepo, Repo};
use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
use jj_lib::rewrite::{
rebase_commit, rebase_commit_with_options, CommitRewriter, EmptyBehaviour, RebaseOptions,
};
use jj_lib::rewrite::{rebase_commit_with_options, CommitRewriter, EmptyBehaviour, RebaseOptions};
use jj_lib::settings::UserSettings;
use tracing::instrument;

Expand Down Expand Up @@ -91,7 +90,7 @@ use crate::ui::Ui;
/// J J
/// ```
///
/// With `-r`, the command rebases only the specified revision onto the
/// With `-r`, the command rebases only the specified revisions onto the
/// destination. Any "hole" left behind will be filled by rebasing descendants
/// onto the specified revision's parent(s). For example, `jj rebase -r K -d M`
/// would transform your history like this:
Expand Down Expand Up @@ -125,7 +124,7 @@ use crate::ui::Ui;
/// commit. This is true in general; it is not specific to this command.
#[derive(clap::Args, Clone, Debug)]
#[command(verbatim_doc_comment)]
#[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revision"])))]
#[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revisions"])))]
pub(crate) struct RebaseArgs {
/// Rebase the whole branch relative to destination's ancestors (can be
/// repeated)
Expand All @@ -147,15 +146,15 @@ pub(crate) struct RebaseArgs {
/// If none of `-b`, `-s`, or `-r` is provided, then the default is `-b @`.
#[arg(long, short)]
source: Vec<RevisionArg>,
/// Rebase only this revision, rebasing descendants onto this revision's
/// Rebase the given revisions, rebasing descendants onto this revision's
/// parent(s)
///
/// Unlike `-s` or `-b`, you may `jj rebase -r` a revision `A` onto a
/// descendant of `A`.
///
/// If none of `-b`, `-s`, or `-r` is provided, then the default is `-b @`.
#[arg(long, short)]
revision: Option<RevisionArg>,
revisions: Vec<RevisionArg>,
/// The revision(s) to rebase onto (can be repeated to create a merge
/// commit)
#[arg(long, short, required = true)]
Expand All @@ -165,7 +164,7 @@ pub(crate) struct RebaseArgs {
/// abandoned. It will not be abandoned if it was already empty before the
/// rebase. Will never skip merge commits with multiple non-empty
/// parents.
#[arg(long, conflicts_with = "revision")]
#[arg(long, conflicts_with = "revisions")]
skip_empty: bool,

/// Deprecated. Please prefix the revset with `all:` instead.
Expand Down Expand Up @@ -198,7 +197,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
.resolve_some_revsets_default_single(&args.destination)?
.into_iter()
.collect_vec();
if let Some(rev_arg) = &args.revision {
if !args.revisions.is_empty() {
assert_eq!(
// In principle, `-r --skip-empty` could mean to abandon the `-r`
// commit if it becomes empty. This seems internally consistent with
Expand All @@ -214,12 +213,16 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
EmptyBehaviour::Keep,
"clap should forbid `-r --skip-empty`"
);
rebase_revision(
let revision_commits: IndexSet<Commit> = workspace_command
.parse_union_revsets(&args.revisions)?
.evaluate_to_commits()?
.try_collect()?; // in reverse topological order
rebase_revisions(
ui,
command.settings(),
&mut workspace_command,
&new_parents,
rev_arg,
&revision_commits,
)?;
} else if !args.source.is_empty() {
let source_commits = workspace_command.resolve_some_revsets_default_single(&args.source)?;
Expand Down Expand Up @@ -348,92 +351,99 @@ fn rebase_descendants_transaction(
Ok(())
}

fn rebase_revision(
fn rebase_revisions(
ui: &mut Ui,
settings: &UserSettings,
workspace_command: &mut WorkspaceCommandHelper,
new_parents: &[Commit],
rev_arg: &RevisionArg,
target_commits: &IndexSet<Commit>,
) -> Result<(), CommandError> {
let to_rebase_commit = workspace_command.resolve_single_rev(rev_arg)?;
workspace_command.check_rewritable([to_rebase_commit.id()])?;
if new_parents.contains(&to_rebase_commit) {
return Err(user_error(format!(
"Cannot rebase {} onto itself",
short_commit_hash(to_rebase_commit.id()),
)));
workspace_command.check_rewritable(target_commits.iter().ids())?;
for commit in target_commits.iter() {
if new_parents.contains(commit) {
return Err(user_error(format!(
"Cannot rebase {} onto itself",
short_commit_hash(commit.id()),
)));
}
}

let mut tx = workspace_command.start_transaction();
let mut rebased_commit_ids = HashMap::new();
let tx_description = if target_commits.len() == 1 {
format!("rebase commit {}", target_commits[0].id().hex())
} else {
format!(
"rebase commit {} and {} more",
target_commits[0].id().hex(),
target_commits.len() - 1
)
};

// First, rebase the descendants of `target_commits`.
let (initial_rebased_commit_ids, target_roots) =
extract_commits(&mut tx, settings, target_commits)?;

// We now update `new_parents` to account for the rebase of all of
// `target_commits`'s descendants. Even if some of the original `new_parents`
// were descendants of `target_commits`, this will no longer be the case after
// the update.
let new_parents = new_parents
.iter()
.map(|new_parent| {
get_possibly_rewritten_commit_id(&initial_rebased_commit_ids, new_parent.id())
})
.collect_vec();
let mut new_rebased_commit_ids = HashMap::new();
let mut skipped_commits = Vec::new();

// First, rebase the descendants of `to_rebase_commit`.
// TODO(ilyagr): Consider making it possible for these descendants to become
// emptied, like --skip_empty. This would require writing careful tests.
// At this point, all commits in the target set will only have other commits in
// the set as their ancestors. We can now safely rebase `target_commits` onto
// the `new_parents`, by updating the roots' parents and rebasing its
// descendants.
tx.mut_repo().transform_descendants(
settings,
vec![to_rebase_commit.id().clone()],
target_roots.iter().cloned().collect_vec(),
|mut rewriter| {
let old_commit = rewriter.old_commit();
let old_commit_id = old_commit.id().clone();

// Replace references to `to_rebase_commit` with its parents.
rewriter.replace_parent(to_rebase_commit.id(), to_rebase_commit.parent_ids());
if target_roots.contains(&old_commit_id) {
rewriter.set_new_parents(new_parents.clone());
}
if rewriter.parents_changed() {
let builder = rewriter.rebase(settings)?;
let commit = builder.write()?;
rebased_commit_ids.insert(old_commit_id, commit.id().clone());
new_rebased_commit_ids.insert(old_commit_id, commit.id().clone());
} else {
skipped_commits.push(rewriter.old_commit().clone());
}
Ok(())
},
)?;

let num_rebased_descendants = rebased_commit_ids.len();

// We now update `new_parents` to account for the rebase of all of
// `to_rebase_commit`'s descendants. Even if some of the original `new_parents`
// were descendants of `to_rebase_commit`, this will no longer be the case after
// the update.
let new_parents = new_parents
.iter()
.map(|new_parent| {
rebased_commit_ids
.get(new_parent.id())
.unwrap_or(new_parent.id())
.clone()
})
.collect();

let tx_description = format!("rebase commit {}", to_rebase_commit.id().hex());
// Finally, it's safe to rebase `to_rebase_commit`. We can skip rebasing if it
// is already a child of `new_parents`. Otherwise, at this point, it should
// no longer have any children; they have all been rebased and the originals
// have been abandoned.
let skipped_commit_rebase = if to_rebase_commit.parent_ids() == new_parents {
if let Some(mut formatter) = ui.status_formatter() {
write!(formatter, "Skipping rebase of commit ")?;
tx.write_commit_summary(formatter.as_mut(), &to_rebase_commit)?;
writeln!(formatter)?;
if let Some(mut fmt) = ui.status_formatter() {
if !skipped_commits.is_empty() {
if skipped_commits.len() == 1 {
write!(fmt, "Skipping rebase of commit ")?;
tx.write_commit_summary(fmt.as_mut(), &skipped_commits[0])?;
writeln!(fmt)?;
} else {
writeln!(fmt, "Skipping rebase of commits:")?;
for commit in skipped_commits {
write!(fmt, " ")?;
tx.write_commit_summary(fmt.as_mut(), &commit)?;
writeln!(fmt)?;
}
}
}
true
} else {
rebase_commit(settings, tx.mut_repo(), to_rebase_commit, new_parents)?;
debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
false
};

if num_rebased_descendants > 0 {
if skipped_commit_rebase {
writeln!(
ui.status(),
"Rebased {num_rebased_descendants} descendant commits onto parent of commit"
)?;
} else {
writeln!(
ui.status(),
"Also rebased {num_rebased_descendants} descendant commits onto parent of rebased \
commit"
)?;
let num_rebased = new_rebased_commit_ids.len()
+ initial_rebased_commit_ids
.iter()
.filter(|(_, new_commit_id)| !new_rebased_commit_ids.contains_key(new_commit_id))
.count();
if num_rebased > 0 {
writeln!(fmt, "Rebased {num_rebased} commits")?;
}
}
if tx.mut_repo().has_changes() {
Expand All @@ -443,6 +453,127 @@ fn rebase_revision(
}
}

/// Extracts `target_commits` from the graph by rebasing its descendants onto
/// its parents. This assumes that `target_commits` can be rewritten.
/// Returns a map from the original commit IDs to the rewritten commit IDs, and
/// the roots of the `target_commits` set, using the rewritten commit ID if
/// applicable.
fn extract_commits(
tx: &mut WorkspaceCommandTransaction,
settings: &UserSettings,
target_commits: &IndexSet<Commit>,
) -> Result<(HashMap<CommitId, CommitId>, IndexSet<CommitId>), CommandError> {
// Commits in the target set should only have other commits in the set as
// parents, except the roots of the set, which persist their original
// parents.
let mut new_target_parents: HashMap<CommitId, Vec<CommitId>> = HashMap::new();
for commit in target_commits.iter().rev() {
// The roots of the set will not have any parents found in `new_target_parents`,
// and will be stored in `new_target_parents` as an empty vector.
let mut new_parents = vec![];
for old_parent in commit.parent_ids() {
if new_target_parents.contains_key(old_parent) {
new_parents.push(old_parent.clone());
}
}
new_target_parents.insert(commit.id().clone(), new_parents);
}

// If a commit outside the target set has a commit in the target set as a
// parent, then - after the transformation - it should have that commit's
// ancestors which are not in the target set as parents.
let mut new_child_parents: HashMap<CommitId, IndexSet<CommitId>> = HashMap::new();
for commit in target_commits.iter().rev() {
let mut new_parents = IndexSet::new();
for old_parent in commit.parent_ids() {
if let Some(parents) = new_child_parents.get(old_parent) {
new_parents.extend(parents.iter().cloned());
} else {
new_parents.insert(old_parent.clone());
}
}
new_child_parents.insert(commit.id().clone(), new_parents);
}

let mut rebased_commit_ids = HashMap::new();

// TODO(ilyagr): Consider making it possible for these descendants
// to become emptied, like --skip-empty. This would require writing careful
// tests.
tx.mut_repo().transform_descendants(
settings,
target_commits.iter().ids().cloned().collect_vec(),
|mut rewriter| {
let old_commit = rewriter.old_commit();
let old_commit_id = old_commit.id().clone();

// Commits in the target set should persist only rebased parents from the target
// sets.
if let Some(new_parents) = new_target_parents.get(&old_commit_id) {
// If the commit does not have any parents in the target set, its parents
// will be persisted since it is one of the roots of the set.
if !new_parents.is_empty() {
rewriter.set_new_rewritten_parents(new_parents.clone());
}
}
// Commits outside the target set should have references to commits inside the set
// replaced.
else if rewriter
.old_commit()
.parent_ids()
.iter()
.any(|id| new_child_parents.contains_key(id))
{
let mut new_parents = vec![];
for parent in rewriter.old_commit().parent_ids() {
if let Some(parents) = new_child_parents.get(parent) {
new_parents.extend(parents.iter().cloned());
} else {
new_parents.push(parent.clone());
}
}
rewriter.set_new_rewritten_parents(new_parents);
}
if rewriter.parents_changed() {
let builder = rewriter.rebase(settings)?;
let commit = builder.write()?;
rebased_commit_ids.insert(old_commit_id, commit.id().clone());
}
Ok(())
},
)?;

// Compute the roots of `target_commits`, and update them to account for the
// rebase of all of `target_commits`'s descendants.
let target_roots: IndexSet<_> = new_target_parents
.iter()
.filter_map(|(commit_id, parents)| {
if parents.is_empty() {
Some(get_possibly_rewritten_commit_id(
&rebased_commit_ids,
commit_id,
))
} else {
None
}
})
.collect();

Ok((rebased_commit_ids, target_roots))
}

/// Returns either the `commit_id` if the commit was not rewritten, or the
/// ID of the rewritten commit corresponding to `commit_id`.
fn get_possibly_rewritten_commit_id(
rewritten_commit_ids: &HashMap<CommitId, CommitId>,
commit_id: &CommitId,
) -> CommitId {
rewritten_commit_ids
.get(commit_id)
.unwrap_or(commit_id)
.clone()
}

fn check_rebase_destinations(
repo: &Arc<ReadonlyRepo>,
new_parents: &[Commit],
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,7 @@ O N'
J J
```
With `-r`, the command rebases only the specified revision onto the
With `-r`, the command rebases only the specified revisions onto the
destination. Any "hole" left behind will be filled by rebasing descendants
onto the specified revision's parent(s). For example, `jj rebase -r K -d M`
would transform your history like this:
Expand Down Expand Up @@ -1534,7 +1534,7 @@ commit. This is true in general; it is not specific to this command.
* `-b`, `--branch <BRANCH>` — Rebase the whole branch relative to destination's ancestors (can be repeated)
* `-s`, `--source <SOURCE>` — Rebase specified revision(s) together with their trees of descendants (can be repeated)
* `-r`, `--revision <REVISION>` — Rebase only this revision, rebasing descendants onto this revision's parent(s)
* `-r`, `--revisions <REVISIONS>` — Rebase the given revisions, rebasing descendants onto this revision's parent(s)
* `-d`, `--destination <DESTINATION>` — The revision(s) to rebase onto (can be repeated to create a merge commit)
* `--skip-empty` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents
Expand Down
Loading

0 comments on commit 940ac95

Please sign in to comment.