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.

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

describe: allow updating the description of multiple commits #3828

Merged
merged 1 commit into from
Aug 4, 2024
Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

* Added revset functions `author_date` and `committer_date`.

* `jj describe` can now update the description of multiple commits.

### Fixed bugs

* `jj status` will show different messages in a conflicted tree, depending
Expand Down
7 changes: 7 additions & 0 deletions cli/src/command_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use jj_lib::workspace::WorkspaceInitError;
use thiserror::Error;

use crate::cli_util::short_operation_hash;
use crate::description_util::ParseBulkEditMessageError;
use crate::diff_util::DiffRenderError;
use crate::formatter::{FormatRecorder, Formatter};
use crate::merge_tools::{ConflictResolveError, DiffEditError, MergeToolConfigError};
Expand Down Expand Up @@ -541,6 +542,12 @@ impl From<GitIgnoreError> for CommandError {
}
}

impl From<ParseBulkEditMessageError> for CommandError {
fn from(err: ParseBulkEditMessageError) -> Self {
user_error(err)
}
}

fn find_source_parse_error_hint(err: &dyn error::Error) -> Option<String> {
let source = err.source()?;
if let Some(source) = source.downcast_ref() {
Expand Down
197 changes: 167 additions & 30 deletions cli/src/commands/describe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,33 +12,45 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::collections::HashMap;
use std::io::{self, Read};

use itertools::Itertools;
use jj_lib::commit::CommitIteratorExt;
use jj_lib::object_id::ObjectId;
use tracing::instrument;

use crate::cli_util::{CommandHelper, RevisionArg};
use crate::command_error::CommandError;
use crate::description_util::{description_template, edit_description, join_message_paragraphs};
use crate::command_error::{user_error, CommandError};
use crate::description_util::{
description_template, edit_description, edit_multiple_descriptions, join_message_paragraphs,
ParsedBulkEditMessage,
};
use crate::ui::Ui;

/// Update the change description or other metadata
///
/// Starts an editor to let you edit the description of a change. The editor
/// Starts an editor to let you edit the description of changes. The editor
/// will be $EDITOR, or `pico` if that's not defined (`Notepad` on Windows).
#[derive(clap::Args, Clone, Debug)]
#[command(visible_aliases = &["desc"])]
pub(crate) struct DescribeArgs {
/// The revision whose description to edit
/// The revision(s) whose description to edit
#[arg(default_value = "@")]
revision: RevisionArg,
revisions: Vec<RevisionArg>,
/// Ignored (but lets you pass `-r` for consistency with other commands)
#[arg(short = 'r', hide = true)]
unused_revision: bool,
#[arg(short = 'r', hide = true, action = clap::ArgAction::Count)]
unused_revision: u8,
/// The change description to use (don't open editor)
///
/// If multiple revisions are specified, the same description will be used
/// for all of them.
#[arg(long = "message", short, value_name = "MESSAGE")]
message_paragraphs: Vec<String>,
/// Read the change description from stdin
///
/// If multiple revisions are specified, the same description will be used
/// for all of them.
#[arg(long)]
stdin: bool,
/// Don't open an editor
Expand All @@ -65,39 +77,164 @@ pub(crate) fn cmd_describe(
args: &DescribeArgs,
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let commit = workspace_command.resolve_single_rev(&args.revision)?;
workspace_command.check_rewritable([commit.id()])?;
let commits: Vec<_> = workspace_command
.parse_union_revsets(&args.revisions)?
.evaluate_to_commits()?
.try_collect()?; // in reverse topological order
if commits.is_empty() {
writeln!(ui.status(), "No revisions to describe.")?;
return Ok(());
}
workspace_command.check_rewritable(commits.iter().ids())?;

let mut tx = workspace_command.start_transaction();
let mut commit_builder = tx
.mut_repo()
.rewrite_commit(command.settings(), &commit)
.detach();
if args.reset_author {
commit_builder.set_author(commit_builder.committer().clone());
}
let tx_description = if commits.len() == 1 {
format!("describe commit {}", commits[0].id().hex())
} else {
format!(
"describe commit {} and {} more",
commits[0].id().hex(),
commits.len() - 1
)
};

let description = if args.stdin {
let shared_description = if args.stdin {
let mut buffer = String::new();
io::stdin().read_to_string(&mut buffer)?;
buffer
Some(buffer)
} else if !args.message_paragraphs.is_empty() {
join_message_paragraphs(&args.message_paragraphs)
} else if args.no_edit {
commit.description().to_owned()
Some(join_message_paragraphs(&args.message_paragraphs))
} else {
if commit_builder.description().is_empty() {
commit_builder.set_description(command.settings().default_description());
None
};

let commit_descriptions: Vec<(_, _)> = if args.no_edit || shared_description.is_some() {
commits
.iter()
.map(|commit| {
let new_description = shared_description
.as_deref()
.unwrap_or_else(|| commit.description());
(commit, new_description.to_owned())
})
.collect()
} else {
let repo = tx.base_repo().clone();
let temp_commits: Vec<(_, _)> = commits
.iter()
// Edit descriptions in topological order
.rev()
.map(|commit| -> Result<_, CommandError> {
let mut commit_builder = tx
.mut_repo()
.rewrite_commit(command.settings(), commit)
.detach();
if commit_builder.description().is_empty() {
commit_builder.set_description(command.settings().default_description());
}
if args.reset_author {
let new_author = commit_builder.committer().clone();
commit_builder.set_author(new_author);
}
let temp_commit = commit_builder.write_hidden()?;
Ok((commit.id(), temp_commit))
})
.try_collect()?;

if let [(_, temp_commit)] = &*temp_commits {
let template = description_template(&tx, "", temp_commit)?;
let description = edit_description(&repo, &template, command.settings())?;

vec![(&commits[0], description)]
} else {
let ParsedBulkEditMessage {
descriptions,
missing,
duplicates,
unexpected,
} = edit_multiple_descriptions(&mut tx, &repo, &temp_commits, command.settings())?;
if !missing.is_empty() {
return Err(user_error(format!(
bnjmnt4n marked this conversation as resolved.
Show resolved Hide resolved
"The description for the following commits were not found in the edited \
message: {}",
missing.join(", ")
)));
}
if !duplicates.is_empty() {
return Err(user_error(format!(
"The following commits were found in the edited message multiple times: {}",
duplicates.join(", ")
)));
}
if !unexpected.is_empty() {
return Err(user_error(format!(
"The following commits were not being edited, but were found in the edited \
message: {}",
unexpected.join(", ")
)));
}

let commit_descriptions = commits
.iter()
.map(|commit| {
let description = descriptions.get(commit.id()).unwrap().to_owned();
(commit, description)
})
.collect();

commit_descriptions
}
let temp_commit = commit_builder.write_hidden()?;
let template = description_template(&tx, "", &temp_commit)?;
edit_description(tx.base_repo(), &template, command.settings())?
};
commit_builder.set_description(description);

if commit_builder.description() != commit.description() || args.reset_author {
commit_builder.write(tx.mut_repo())?;
// Filter out unchanged commits to avoid rebasing descendants in
// `transform_descendants` below unnecessarily.
let commit_descriptions: HashMap<_, _> = commit_descriptions
.into_iter()
.filter_map(|(commit, new_description)| {
if *new_description == *commit.description() && !args.reset_author {
None
} else {
Some((commit.id(), new_description))
}
})
.collect();

let mut num_described = 0;
let mut num_rebased = 0;
// Even though `MutRepo::rewrite_commit` and `MutRepo::rebase_descendants` can
// handle rewriting of a commit even if it is a descendant of another commit
// being rewritten, using `MutRepo::transform_descendants` prevents us from
// rewriting the same commit multiple times, and adding additional entries
// in the predecessor chain.
tx.mut_repo().transform_descendants(
bnjmnt4n marked this conversation as resolved.
Show resolved Hide resolved
command.settings(),
commit_descriptions
.keys()
.map(|&id| id.clone())
.collect_vec(),
|rewriter| {
let old_commit_id = rewriter.old_commit().id().clone();
let mut commit_builder = rewriter.rebase(command.settings())?;
if let Some(description) = commit_descriptions.get(&old_commit_id) {
commit_builder = commit_builder.set_description(description);
if args.reset_author {
let new_author = commit_builder.committer().clone();
commit_builder = commit_builder.set_author(new_author);
}
num_described += 1;
} else {
num_rebased += 1;
}
commit_builder.write()?;
Ok(())
},
)?;
if num_described > 1 {
writeln!(ui.status(), "Updated {} commits", num_described)?;
}
if num_rebased > 0 {
writeln!(ui.status(), "Rebased {} descendant commits", num_rebased)?;
}
tx.finish(ui, format!("describe commit {}", commit.id().hex()))?;
tx.finish(ui, tx_description)?;
Ok(())
}
Loading