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 a34b7e0
Show file tree
Hide file tree
Showing 5 changed files with 412 additions and 93 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

* The default template alias `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` was replaced by `format_root_commit(root: Commit)`.

* The `--revision` option of `jj rebase` is renamed to `--revisions`. The short
alias `-r` is still supported.

### New features

* The list of conflicted paths is printed whenever the working copy changes.
Expand Down Expand Up @@ -48,6 +51,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* You can check whether Watchman fsmonitor is enabled or installed with the new
`jj debug watchman status` command.

* `jj rebase` now accepts revsets resolving to multiple revisions with the
`--revisions`/`-r` option.

### Fixed bugs

* Revsets now support `\`-escapes in string literal.
Expand Down
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
Loading

0 comments on commit a34b7e0

Please sign in to comment.