rewrite: incomplete() revset for updating author timestamp

It's common to create empty working-copy commits while using jj, and
currently the author timestamp for a commit is only set when it is first
created. If you create an empty commit, then don't work on a repo for a
few days, and then start working on a new feature without abandoning the
working-copy commit, the author timestamp will remain as the time the
commit was created rather than being updated to the time that work began
or finished.

This commit adds a new revset `incomplete()` which specifies which
commits are "incomplete" and should have their author timestamp updated
whenever changes are made to them. This can be configured to fit many
different workflows. By default, empty commits with no description are
considered incomplete, meaning that the author timestamp will become
finalized whenever a commit is given a description or becomes non-empty.

The author timestamp is only updated if the committer is the author, and
it is never updated when rebasing.

Resolves jj-vcs#2000.

CHANGELOG.md

@@ -52,6 +52,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj file` now groups commands for working with files.
 
+* `incomplete()` revset can be used to configure when author timestamps should
+  be updated when rewriting commits.
+
 ### Fixed bugs
 
 ## [0.18.0] - 2024-06-05

cli/examples/custom-command/main.rs

@@ -43,7 +43,7 @@ fn run_custom_command(
             let mut tx = workspace_command.start_transaction();
             let new_commit = tx
                 .mut_repo()
-                .rewrite_commit(command_helper.settings(), &commit)
+                .rewrite_commit(command_helper.settings(), &commit, false)
                 .set_description("Frobnicated!")
                 .write()?;
             tx.finish(ui, "Frobnicate")?;

cli/src/cli_util.rs

@@ -25,7 +25,7 @@ use std::rc::Rc;
 use std::str::FromStr;
 use std::sync::Arc;
 use std::time::SystemTime;
-use std::{fs, str};
+use std::{fs, iter, str};
 
 use clap::builder::{
     MapValueParser, NonEmptyStringValueParser, TypedValueParser, ValueParserFactory,
@@ -1156,8 +1156,37 @@ impl WorkspaceCommandHelper {
         self.check_repo_rewritable(self.repo().as_ref(), commits)
     }
 
+    pub fn find_incomplete<'a>(
+        &self,
+        commits: impl IntoIterator<Item = &'a CommitId>,
+    ) -> Result<Vec<CommitId>, CommandError> {
+        let incomplete = revset_util::parse_incomplete_expression(&self.revset_parse_context())
+            .map_err(|e| config_error_with_message("Invalid `revset-aliases.incomplete()`", e))?;
+        let mut expression = RevsetExpressionEvaluator::new(
+            self.repo().as_ref(),
+            self.revset_extensions.clone(),
+            self.id_prefix_context()?,
+            incomplete,
+        );
+        expression.intersect_with(&RevsetExpression::commits(
+            commits.into_iter().cloned().collect_vec(),
+        ));
+        Ok(expression
+            .evaluate_to_commit_ids()
+            .map_err(|e| config_error_with_message("Invalid `revset-aliases.incomplete()`", e))?
+            .collect_vec())
+    }
+
+    pub fn check_commit_incomplete(&self, commit_id: &CommitId) -> Result<bool, CommandError> {
+        Ok(!self.find_incomplete(iter::once(commit_id))?.is_empty())
+    }
+
     #[instrument(skip_all)]
     fn snapshot_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> {
+        let incomplete = revset_util::parse_incomplete_expression(&self.revset_parse_context())
+            .map_err(|e| config_error_with_message("Invalid `revset-aliases.incomplete()`", e))?;
+        let id_prefix_context = self.id_prefix_context()?.clone();
+
         let workspace_id = self.workspace_id().to_owned();
         let get_wc_commit = |repo: &ReadonlyRepo| -> Result<Option<_>, _> {
             repo.view()
@@ -1228,12 +1257,28 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin
         })?;
         drop(progress);
         if new_tree_id != *wc_commit.tree_id() {
+            let update_author_timestamp = {
+                let mut expression = RevsetExpressionEvaluator::new(
+                    self.user_repo.repo.as_ref(),
+                    self.revset_extensions.clone(),
+                    &id_prefix_context,
+                    incomplete,
+                );
+                expression.intersect_with(&RevsetExpression::commit(wc_commit.id().clone()));
+                expression
+                    .evaluate_to_commit_ids()
+                    .map_err(|e| {
+                        config_error_with_message("Invalid `revset-aliases.incomplete()`", e)
+                    })?
+                    .next()
+                    .is_some()
+            };
             let mut tx =
                 start_repo_transaction(&self.user_repo.repo, &self.settings, &self.string_args);
             tx.set_is_snapshot(true);
             let mut_repo = tx.mut_repo();
             let commit = mut_repo
-                .rewrite_commit(&self.settings, &wc_commit)
+                .rewrite_commit(&self.settings, &wc_commit, update_author_timestamp)
                 .set_tree_id(new_tree_id)
                 .write()?;
             mut_repo.set_wc_commit(workspace_id, commit.id().clone())?;

cli/src/commands/commit.rs

@@ -59,6 +59,7 @@ pub(crate) fn cmd_commit(
     let advanceable_branches = workspace_command.get_advanceable_branches(commit.parent_ids())?;
     let diff_selector =
         workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
+    let update_author_timestamp = workspace_command.check_commit_incomplete(commit.id())?;
     let mut tx = workspace_command.start_transaction();
     let base_tree = commit.parent_tree(tx.repo())?;
     let instructions = format!(
@@ -104,7 +105,7 @@ new working-copy commit.
 
     let new_commit = tx
         .mut_repo()
-        .rewrite_commit(command.settings(), &commit)
+        .rewrite_commit(command.settings(), &commit, update_author_timestamp)
         .set_tree_id(tree_id)
         .set_description(description)
         .write()?;

cli/src/commands/describe.rs

@@ -85,10 +85,11 @@ pub(crate) fn cmd_describe(
     if description == *commit.description() && !args.reset_author {
         writeln!(ui.status(), "Nothing changed.")?;
     } else {
+        let update_author_timestamp = workspace_command.check_commit_incomplete(commit.id())?;
         let mut tx = workspace_command.start_transaction();
         let mut commit_builder = tx
             .mut_repo()
-            .rewrite_commit(command.settings(), &commit)
+            .rewrite_commit(command.settings(), &commit, update_author_timestamp)
             .set_description(description);
         if args.reset_author {
             let new_author = commit_builder.committer().clone();

cli/src/commands/diffedit.rs

@@ -88,6 +88,7 @@ pub(crate) fn cmd_diffedit(
     workspace_command.check_rewritable([target_commit.id()])?;
 
     let diff_editor = workspace_command.diff_editor(ui, args.tool.as_deref())?;
+    let update_author_timestamp = workspace_command.check_commit_incomplete(target_commit.id())?;
     let mut tx = workspace_command.start_transaction();
     let instructions = format!(
         "\
@@ -107,7 +108,7 @@ don't make any changes, then the operation will be aborted.",
     } else {
         let mut_repo = tx.mut_repo();
         let new_commit = mut_repo
-            .rewrite_commit(command.settings(), &target_commit)
+            .rewrite_commit(command.settings(), &target_commit, update_author_timestamp)
             .set_tree_id(tree_id)
             .write()?;
         // rebase_descendants early; otherwise `new_commit` would always have

cli/src/commands/duplicate.rs

@@ -70,7 +70,7 @@ pub(crate) fn cmd_duplicate(
             .map(|id| duplicated_old_to_new.get(id).map_or(id, |c| c.id()).clone())
             .collect();
         let new_commit = mut_repo
-            .rewrite_commit(command.settings(), &original_commit)
+            .rewrite_commit(command.settings(), &original_commit, false)
             .generate_new_change_id()
             .set_parents(new_parents)
             .write()?;

cli/src/commands/file/chmod.rs

@@ -85,6 +85,7 @@ pub(crate) fn cmd_chmod(
     let matcher = fileset_expression.to_matcher();
     print_unmatched_explicit_paths(ui, &workspace_command, &fileset_expression, [&tree])?;
 
+    let update_author_timestamp = workspace_command.check_commit_incomplete(commit.id())?;
     let mut tx = workspace_command.start_transaction();
     let store = tree.store();
     let mut tree_builder = MergedTreeBuilder::new(commit.tree_id().clone());
@@ -123,7 +124,7 @@ pub(crate) fn cmd_chmod(
 
     let new_tree_id = tree_builder.write_tree(store)?;
     tx.mut_repo()
-        .rewrite_commit(command.settings(), &commit)
+        .rewrite_commit(command.settings(), &commit, update_author_timestamp)
         .set_tree_id(new_tree_id)
         .write()?;
     tx.finish(

cli/src/commands/resolve.rs

@@ -96,11 +96,12 @@ pub(crate) fn cmd_resolve(
         "Resolving conflicts in: {}",
         workspace_command.format_file_path(repo_path)
     )?;
+    let update_author_timestamp = workspace_command.check_commit_incomplete(commit.id())?;
     let mut tx = workspace_command.start_transaction();
     let new_tree_id = merge_editor.edit_file(&tree, repo_path)?;
     let new_commit = tx
         .mut_repo()
-        .rewrite_commit(command.settings(), &commit)
+        .rewrite_commit(command.settings(), &commit, update_author_timestamp)
         .set_tree_id(new_tree_id)
         .write()?;
     tx.finish(

cli/src/commands/restore.rs

@@ -106,10 +106,11 @@ pub(crate) fn cmd_restore(
     if &new_tree_id == to_commit.tree_id() {
         writeln!(ui.status(), "Nothing changed.")?;
     } else {
+        let update_author_timestamp = workspace_command.check_commit_incomplete(to_commit.id())?;
         let mut tx = workspace_command.start_transaction();
         let mut_repo = tx.mut_repo();
         let new_commit = mut_repo
-            .rewrite_commit(command.settings(), &to_commit)
+            .rewrite_commit(command.settings(), &to_commit, update_author_timestamp)
             .set_tree_id(new_tree_id)
             .write()?;
         // rebase_descendants early; otherwise `new_commit` would always have

cli/src/commands/split.rs

@@ -85,6 +85,7 @@ pub(crate) fn cmd_split(
         args.tool.as_deref(),
         args.interactive || args.paths.is_empty(),
     )?;
+    let update_author_timestamp = workspace_command.check_commit_incomplete(commit.id())?;
     let mut tx = workspace_command.start_transaction();
     let end_tree = commit.tree()?;
     let base_tree = commit.parent_tree(tx.repo())?;
@@ -132,7 +133,7 @@ the operation will be aborted.
     let first_description = edit_description(tx.base_repo(), &first_template, command.settings())?;
     let first_commit = tx
         .mut_repo()
-        .rewrite_commit(command.settings(), &commit)
+        .rewrite_commit(command.settings(), &commit, update_author_timestamp)
         .set_tree_id(selected_tree_id)
         .set_description(first_description)
         .write()?;
@@ -169,7 +170,7 @@ the operation will be aborted.
     };
     let second_commit = tx
         .mut_repo()
-        .rewrite_commit(command.settings(), &commit)
+        .rewrite_commit(command.settings(), &commit, true)
         .set_parents(second_commit_parents)
         .set_tree_id(second_tree.id())
         // Generate a new change id so that the commit being split doesn't

cli/src/commands/squash.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::iter;
+
 use itertools::Itertools as _;
 use jj_lib::commit::{Commit, CommitIteratorExt};
 use jj_lib::matchers::Matcher;
@@ -249,6 +251,14 @@ from the source will be moved into the destination.
         return Ok(());
     }
 
+    let incomplete_commits = tx.base_workspace_helper().find_incomplete(
+        source_commits
+            .iter()
+            .filter(|source| !source.abandon)
+            .map(|source| source.commit.id())
+            .chain(iter::once(destination.id())),
+    )?;
+
     for source in &source_commits {
         if source.abandon {
             tx.mut_repo()
@@ -258,7 +268,11 @@ from the source will be moved into the destination.
             // Apply the reverse of the selected changes onto the source
             let new_source_tree = source_tree.merge(&source.selected_tree, &source.parent_tree)?;
             tx.mut_repo()
-                .rewrite_commit(settings, source.commit)
+                .rewrite_commit(
+                    settings,
+                    source.commit,
+                    incomplete_commits.contains(source.commit.id()),
+                )
                 .set_tree_id(new_source_tree.id().clone())
                 .write()?;
         }
@@ -300,7 +314,11 @@ from the source will be moved into the destination.
             .map(|source| source.commit.id().clone()),
     );
     tx.mut_repo()
-        .rewrite_commit(settings, &rewritten_destination)
+        .rewrite_commit(
+            settings,
+            &rewritten_destination,
+            incomplete_commits.contains(destination.id()),
+        )
         .set_tree_id(destination_tree.id().clone())
         .set_predecessors(predecessors)
         .set_description(description)

cli/src/commands/unsquash.rs

@@ -105,22 +105,36 @@ aborted.
         tx.mut_repo().record_abandoned_commit(parent.id().clone());
         let description =
             combine_messages(tx.base_repo(), &[&parent], &commit, command.settings())?;
+        let update_author_timestamp = tx
+            .base_workspace_helper()
+            .check_commit_incomplete(commit.id())?;
         // Commit the new child on top of the parent's parents.
         tx.mut_repo()
-            .rewrite_commit(command.settings(), &commit)
+            .rewrite_commit(command.settings(), &commit, update_author_timestamp)
             .set_parents(parent.parent_ids().to_vec())
             .set_description(description)
             .write()?;
     } else {
+        let incomplete_commits = tx
+            .base_workspace_helper()
+            .find_incomplete([parent.id(), commit.id()])?;
         let new_parent = tx
             .mut_repo()
-            .rewrite_commit(command.settings(), &parent)
+            .rewrite_commit(
+                command.settings(),
+                &parent,
+                incomplete_commits.contains(parent.id()),
+            )
             .set_tree_id(new_parent_tree_id)
             .set_predecessors(vec![parent.id().clone(), commit.id().clone()])
             .write()?;
         // Commit the new child on top of the new parent.
         tx.mut_repo()
-            .rewrite_commit(command.settings(), &commit)
+            .rewrite_commit(
+                command.settings(),
+                &commit,
+                incomplete_commits.contains(commit.id()),
+            )
             .set_parents(vec![new_parent.id().clone()])
             .write()?;
     }

cli/src/commands/untrack.rs

@@ -60,7 +60,7 @@ pub(crate) fn cmd_untrack(
     let new_tree_id = tree_builder.write_tree(&store)?;
     let new_commit = tx
         .mut_repo()
-        .rewrite_commit(command.settings(), &wc_commit)
+        .rewrite_commit(command.settings(), &wc_commit, false)
         .set_tree_id(new_tree_id)
         .write()?;
     // Reset the working copy to the new commit

cli/src/config/revsets.toml

@@ -21,3 +21,4 @@ latest(
 'immutable_heads()' = 'trunk() | tags()'
 'immutable()' = '::(immutable_heads() | root())'
 'mutable()' = '~immutable()'
+'incomplete()' = 'description(exact:"") & empty()'

cli/src/revset_util.rs

@@ -37,6 +37,7 @@ use crate::templater::TemplateRenderer;
 use crate::ui::Ui;
 
 const BUILTIN_IMMUTABLE_HEADS: &str = "immutable_heads";
+const BUILTIN_INCOMPLETE: &str = "incomplete";
 
 #[derive(Debug, Error)]
 pub enum UserRevsetEvaluationError {
@@ -174,6 +175,17 @@ pub fn parse_immutable_expression(
     Ok(heads.union(&RevsetExpression::root()).ancestors())
 }
 
+pub fn parse_incomplete_expression(
+    context: &RevsetParseContext,
+) -> Result<Rc<RevsetExpression>, RevsetParseError> {
+    let (_, _, incomplete_str) = context
+        .aliases_map()
+        .get_function(BUILTIN_INCOMPLETE, 0)
+        .unwrap();
+
+    revset::parse(incomplete_str, context)
+}
+
 pub(super) fn evaluate_revset_to_single_commit<'a>(
     revision_str: &str,
     expression: &RevsetExpressionEvaluator<'_>,