From 28f4b381d48fce18c0184656a181a211b8bd8102 Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Fri, 14 Jun 2024 21:10:59 -0500 Subject: [PATCH] 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. --- CHANGELOG.md | 3 ++ cli/examples/custom-command/main.rs | 2 +- cli/src/cli_util.rs | 49 +++++++++++++++++++++++++++-- cli/src/commands/commit.rs | 3 +- cli/src/commands/describe.rs | 3 +- cli/src/commands/diffedit.rs | 3 +- cli/src/commands/duplicate.rs | 2 +- cli/src/commands/file/chmod.rs | 3 +- cli/src/commands/resolve.rs | 3 +- cli/src/commands/restore.rs | 3 +- cli/src/commands/split.rs | 5 +-- cli/src/commands/squash.rs | 22 +++++++++++-- cli/src/commands/unsquash.rs | 20 ++++++++++-- cli/src/commands/untrack.rs | 2 +- cli/src/config/revsets.toml | 1 + cli/src/revset_util.rs | 12 +++++++ docs/revsets.md | 7 +++++ lib/src/commit_builder.rs | 7 +++++ lib/src/id_prefix.rs | 4 ++- lib/src/repo.rs | 3 +- lib/src/rewrite.rs | 4 +-- 21 files changed, 139 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbd818cd13..c390d5bae7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cli/examples/custom-command/main.rs b/cli/examples/custom-command/main.rs index 69980175e1..82ae52a68b 100644 --- a/cli/examples/custom-command/main.rs +++ b/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")?; diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 72af109d39..3a349a5768 100644 --- a/cli/src/cli_util.rs +++ b/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, + ) -> 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 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 { + 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, _> { 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())?; diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 4ad91c6d7c..fe8d681faa 100644 --- a/cli/src/commands/commit.rs +++ b/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()?; diff --git a/cli/src/commands/describe.rs b/cli/src/commands/describe.rs index a944eee50b..2f4c1c8bf4 100644 --- a/cli/src/commands/describe.rs +++ b/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(); diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 7aba4a0c12..3632faf5e4 100644 --- a/cli/src/commands/diffedit.rs +++ b/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 diff --git a/cli/src/commands/duplicate.rs b/cli/src/commands/duplicate.rs index 0ebe30336c..9b6a3c4569 100644 --- a/cli/src/commands/duplicate.rs +++ b/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()?; diff --git a/cli/src/commands/file/chmod.rs b/cli/src/commands/file/chmod.rs index 77c97d9bc8..769a5c7fac 100644 --- a/cli/src/commands/file/chmod.rs +++ b/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( diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 22b5c90a22..59047b3366 100644 --- a/cli/src/commands/resolve.rs +++ b/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( diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index 2c9e7ba106..e58d3af4ac 100644 --- a/cli/src/commands/restore.rs +++ b/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 diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index f0d2b27932..df03181c43 100644 --- a/cli/src/commands/split.rs +++ b/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 diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 58290fb8ff..96a3c33574 100644 --- a/cli/src/commands/squash.rs +++ b/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) diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index 3cb3c9aad8..0b2de03f1c 100644 --- a/cli/src/commands/unsquash.rs +++ b/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()?; } diff --git a/cli/src/commands/untrack.rs b/cli/src/commands/untrack.rs index 449724468f..da6ff03e68 100644 --- a/cli/src/commands/untrack.rs +++ b/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 diff --git a/cli/src/config/revsets.toml b/cli/src/config/revsets.toml index d84a745b3e..e1f2290f1a 100644 --- a/cli/src/config/revsets.toml +++ b/cli/src/config/revsets.toml @@ -21,3 +21,4 @@ latest( 'immutable_heads()' = 'trunk() | tags()' 'immutable()' = '::(immutable_heads() | root())' 'mutable()' = '~immutable()' +'incomplete()' = 'description(exact:"") & empty()' diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 986c0719d1..436c1bea30 100644 --- a/cli/src/revset_util.rs +++ b/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, 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<'_>, diff --git a/docs/revsets.md b/docs/revsets.md index be7b165e70..25e7ae5eae 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -383,6 +383,13 @@ for a comprehensive list. *not* change whether a commit is immutable. To do that, edit `immutable_heads()`. +* `incomplete()`: The set of commits which should have their author timestamp + updated to match the committer timestamp when they are rewritten. Resolves + to `description(exact:"") & empty()` by default (commits which are empty and + have no description). Rebasing commits never updates the author timestamp, + and commits by a different author are never updated. A more Git-like behavior + might be to finalize a commit's author timestamp when a description is set, + which could be achieved using `description(exact:"")`. ## The `all:` modifier diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index cef5b9789b..d249aaac17 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -69,6 +69,7 @@ impl CommitBuilder<'_> { mut_repo: &'repo mut MutableRepo, settings: &UserSettings, predecessor: &Commit, + update_author_timestamp: bool, ) -> CommitBuilder<'repo> { let mut commit = predecessor.store_commit().clone(); commit.predecessors = vec![predecessor.id().clone()]; @@ -85,6 +86,12 @@ impl CommitBuilder<'_> { { commit.author.email = commit.committer.email.clone(); } + if update_author_timestamp + && commit.author.name == commit.committer.name + && commit.author.email == commit.committer.email + { + commit.author.timestamp = commit.committer.timestamp.clone(); + } CommitBuilder { mut_repo, commit, diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index f3a47e706d..8f6acea413 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -32,11 +32,13 @@ use crate::revset::{ struct PrefixDisambiguationError; +#[derive(Clone)] struct DisambiguationData { expression: Rc, indexes: OnceCell, } +#[derive(Clone)] struct Indexes { commit_change_ids: Vec<(CommitId, ChangeId)>, commit_index: IdIndex, @@ -99,7 +101,7 @@ impl IdIndexSourceEntry for &'_ (CommitId, ChangeId) { } } -#[derive(Default)] +#[derive(Clone, Default)] pub struct IdPrefixContext { disambiguation: Option, extensions: Arc, diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 1382dc9224..7f69d74111 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -825,8 +825,9 @@ impl MutableRepo { &mut self, settings: &UserSettings, predecessor: &Commit, + update_author_timestamp: bool, ) -> CommitBuilder { - CommitBuilder::for_rewrite_from(self, settings, predecessor) + CommitBuilder::for_rewrite_from(self, settings, predecessor, update_author_timestamp) // CommitBuilder::write will record the rewrite in // `self.rewritten_commits` } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 5c48c4ec6e..6fe8e0f69a 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -247,7 +247,7 @@ impl<'repo> CommitRewriter<'repo> { let builder = self .mut_repo - .rewrite_commit(settings, &self.old_commit) + .rewrite_commit(settings, &self.old_commit, false) .set_parents(self.new_parents) .set_tree_id(new_tree_id); Ok(Some(builder)) @@ -265,7 +265,7 @@ impl<'repo> CommitRewriter<'repo> { pub fn reparent(self, settings: &UserSettings) -> BackendResult> { Ok(self .mut_repo - .rewrite_commit(settings, &self.old_commit) + .rewrite_commit(settings, &self.old_commit, false) .set_parents(self.new_parents)) } }