From 11005ae9b8d73ccd3ed83606550be9828fab3645 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Fri, 28 May 2021 10:12:53 -0700
Subject: [PATCH] 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.
---
 src/commands.rs | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/commands.rs b/src/commands.rs
index dac511ae0..7e7548067 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -259,22 +259,27 @@ impl RepoCommandHelper {
     }
 
     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
-        // date (the lib crate only looks at the checkout in the view).
-        // TODO: How do we generally figure out if a revset needs to commit the working
-        // copy? For example, ":@" should ideally not result in a new working copy
-        // commit, but "::@" should. "foo::" is probably also should, since we would
+        let expression = revset::parse(revision_str)?;
+        // If the revset is exactly "@", then we need to commit the working copy. If
+        // it's another symbol, then we don't. If it's more complex, then we do
+        // (just to be safe). TODO: Maybe make this smarter. How do we generally
+        // 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
         // parent of the current checkout. Other interesting cases include some kind of
         // reference pointing to the working copy commit. If it's a
         // type of reference that would get updated when the commit gets rewritten, then
         // 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.commit_working_copy();
         }
-
-        Ok(revset::parse(revision_str)?)
+        Ok(expression)
     }
 
     fn check_rewriteable(&self, commit: &Commit) -> Result<(), CommandError> {
@@ -1217,18 +1222,13 @@ fn cmd_log(
 ) -> Result<(), CommandError> {
     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 =
         repo_command.parse_revset(sub_matches.value_of("revisions").unwrap())?;
     let repo = repo_command.repo();
     let revset = revset_expression.evaluate(repo.as_repo_ref())?;
     let store = repo.store();
 
+    let use_graph = !sub_matches.is_present("no-graph");
     let template_string = match sub_matches.value_of("template") {
         Some(value) => value.to_string(),
         None => {