cli: make condition for committing working copy slightly smarter

Before this patch, `jj log` would always commit the working copy and
most other commands would commit the working copy only if they were
passed a revset of exactly "@". This patch makes it so they all commit
the working copy unless they are passed just a symbol other than "@"
(typically a commit id). That means that we will not commit the
working copy if the user does `jj diff -r abc123`, but we will if they
do `jj diff -r :abc123`. It's clearly unnecessary in both those cases,
and we should fix, but this is probably good enough for now.
This commit is contained in:
Martin von Zweigbergk 2021-05-28 10:12:53 -07:00
parent 440a423b54
commit 11005ae9b8

View file

@ -259,22 +259,27 @@ impl RepoCommandHelper {
} }
fn parse_revset(&mut self, revision_str: &str) -> Result<RevsetExpression, CommandError> { fn parse_revset(&mut self, revision_str: &str) -> Result<RevsetExpression, CommandError> {
// If we're looking up the working copy commit ("@"), make sure that it is up to let expression = revset::parse(revision_str)?;
// date (the lib crate only looks at the checkout in the view). // If the revset is exactly "@", then we need to commit the working copy. If
// TODO: How do we generally figure out if a revset needs to commit the working // it's another symbol, then we don't. If it's more complex, then we do
// copy? For example, ":@" should ideally not result in a new working copy // (just to be safe). TODO: Maybe make this smarter. How do we generally
// commit, but "::@" should. "foo::" is probably also should, since we would // figure out if a revset needs to commit the working copy? For example,
// ":@" should perhaps not result in a new working copy commit, but
// "::@" should. "foo::" is probably also should, since we would
// otherwise need to evaluate the revset and see if "foo::" includes the // otherwise need to evaluate the revset and see if "foo::" includes the
// parent of the current checkout. Other interesting cases include some kind of // parent of the current checkout. Other interesting cases include some kind of
// reference pointing to the working copy commit. If it's a // reference pointing to the working copy commit. If it's a
// type of reference that would get updated when the commit gets rewritten, then // type of reference that would get updated when the commit gets rewritten, then
// we probably should create a new working copy commit. // we probably should create a new working copy commit.
if revision_str == "@" && !self.working_copy_committed { let mentions_checkout = match &expression {
RevsetExpression::Symbol(name) => name == "@",
_ => true,
};
if mentions_checkout && !self.working_copy_committed {
self.working_copy_committed = true; self.working_copy_committed = true;
self.commit_working_copy(); self.commit_working_copy();
} }
Ok(expression)
Ok(revset::parse(revision_str)?)
} }
fn check_rewriteable(&self, commit: &Commit) -> Result<(), CommandError> { fn check_rewriteable(&self, commit: &Commit) -> Result<(), CommandError> {
@ -1217,18 +1222,13 @@ fn cmd_log(
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let mut repo_command = command.repo_helper(ui)?; let mut repo_command = command.repo_helper(ui)?;
let use_graph = !sub_matches.is_present("no-graph");
if use_graph {
// Commit so the latest working copy is reflected in the view's checkout and
// visible heads
repo_command.commit_working_copy();
}
let revset_expression = let revset_expression =
repo_command.parse_revset(sub_matches.value_of("revisions").unwrap())?; repo_command.parse_revset(sub_matches.value_of("revisions").unwrap())?;
let repo = repo_command.repo(); let repo = repo_command.repo();
let revset = revset_expression.evaluate(repo.as_repo_ref())?; let revset = revset_expression.evaluate(repo.as_repo_ref())?;
let store = repo.store(); let store = repo.store();
let use_graph = !sub_matches.is_present("no-graph");
let template_string = match sub_matches.value_of("template") { let template_string = match sub_matches.value_of("template") {
Some(value) => value.to_string(), Some(value) => value.to_string(),
None => { None => {