From 2008991749c1e6941b07668e90ee3168acd86ae8 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 21 Jul 2024 19:27:36 +0900 Subject: [PATCH] cli: do not attempt to merge op heads if --at-op=@ is specified The idea is that --at-op specifies a certain operation, so --at-op=@ can be interpreted as the option to select _the_ known head operation. This helps eliminate special cases from "op log" which doesn't snapshot nor merge concurrent ops. --- CHANGELOG.md | 2 ++ cli/src/cli_util.rs | 38 ++++++++++++++++--------- cli/src/commands/debug/index.rs | 5 +++- cli/src/commands/debug/reindex.rs | 5 +++- cli/src/commands/git/clone.rs | 2 +- cli/src/commands/git/init.rs | 2 +- cli/src/commands/init.rs | 2 +- cli/src/commands/operation/abandon.rs | 5 ++-- cli/src/commands/operation/log.rs | 2 +- cli/src/commands/util.rs | 2 +- cli/tests/cli-reference@.md.snap | 6 ++-- cli/tests/test_concurrent_operations.rs | 7 +++++ cli/tests/test_global_opts.rs | 2 +- 13 files changed, 53 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ea5ed48ec..f42cfd4ef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). preserved. If a checked-out named branch gets deleted locally or remotely, the corresponding commits will be abandoned. +* `jj --at-op=@` no longer merges concurrent operations if explicitly specified. + ### Deprecations * The original configuration syntax for `jj fix` is now deprecated in favor of diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index bd04908008..bdaf04e2bb 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -301,8 +301,7 @@ impl CommandHelper { let workspace = self.load_workspace()?; let op_head = self.resolve_operation(ui, workspace.repo_loader())?; let repo = workspace.repo_loader().load_at(&op_head)?; - let loaded_at_head = self.global_args.at_operation == "@"; - WorkspaceCommandHelper::new(ui, self, workspace, repo, loaded_at_head) + WorkspaceCommandHelper::new(ui, self, workspace, repo, self.is_at_head_operation()) } pub fn get_working_copy_factory(&self) -> Result<&dyn WorkingCopyFactory, CommandError> { @@ -329,13 +328,26 @@ impl CommandHelper { .map_err(|err| map_workspace_load_error(err, self.global_args.repository.as_deref())) } + /// Returns true if the current operation is considered to be the head. + pub fn is_at_head_operation(&self) -> bool { + // TODO: should we accept --at-op= as the head op? or should we + // make --at-op=@ imply --ignore-workign-copy (i.e. not at the head.) + matches!(self.global_args.at_operation.as_deref(), None | Some("@")) + } + + /// Resolves the current operation from the command-line argument. + /// + /// If no `--at-operation` is specified, the head operations will be + /// loaded. If there are multiple heads, they'll be merged. #[instrument(skip_all)] pub fn resolve_operation( &self, ui: &mut Ui, repo_loader: &RepoLoader, ) -> Result { - if self.global_args.at_operation == "@" { + if let Some(op_str) = &self.global_args.at_operation { + Ok(op_walk::resolve_op_for_load(repo_loader, op_str)?) + } else { op_heads_store::resolve_op_heads( repo_loader.op_heads_store().as_ref(), repo_loader.op_store(), @@ -366,10 +378,6 @@ impl CommandHelper { .clone()) }, ) - } else { - let operation = - op_walk::resolve_op_for_load(repo_loader, &self.global_args.at_operation)?; - Ok(operation) } } @@ -2429,10 +2437,14 @@ pub struct GlobalArgs { /// Operation to load the repo at /// /// Operation to load the repo at. By default, Jujutsu loads the repo at the - /// most recent operation. You can use `--at-op=` to see what - /// the repo looked like at an earlier operation. For example `jj - /// --at-op= st` will show you what `jj st` would have - /// shown you when the given operation had just finished. + /// most recent operation, or at the merge of the concurrent operations if + /// any. + /// + /// You can use `--at-op=` to see what the repo looked like at + /// an earlier operation. For example `jj --at-op= st` will + /// show you what `jj st` would have shown you when the given operation had + /// just finished. `--at-op=@` is pretty much the same as the default except + /// that concurrent operations will never be merged. /// /// Use `jj op log` to find the operation ID you want. Any unambiguous /// prefix of the operation ID is enough. @@ -2444,8 +2456,8 @@ pub struct GlobalArgs { /// earlier operation. Doing that is equivalent to having run concurrent /// commands starting at the earlier operation. There's rarely a reason to /// do that, but it is possible. - #[arg(long, visible_alias = "at-op", global = true, default_value = "@")] - pub at_operation: String, + #[arg(long, visible_alias = "at-op", global = true)] + pub at_operation: Option, /// Enable debug logging #[arg(long, global = true)] pub debug: bool, diff --git a/cli/src/commands/debug/index.rs b/cli/src/commands/debug/index.rs index 2c9eb06a68..c626dc4df7 100644 --- a/cli/src/commands/debug/index.rs +++ b/cli/src/commands/debug/index.rs @@ -35,7 +35,10 @@ pub fn cmd_debug_index( // merge concurrent operations and update the index. let workspace = command.load_workspace()?; let repo_loader = workspace.repo_loader(); - let op = op_walk::resolve_op_for_load(repo_loader, &command.global_args().at_operation)?; + let op = op_walk::resolve_op_for_load( + repo_loader, + command.global_args().at_operation.as_deref().unwrap_or("@"), + )?; let index_store = repo_loader.index_store(); let index = index_store .get_index_at_op(&op, repo_loader.store()) diff --git a/cli/src/commands/debug/reindex.rs b/cli/src/commands/debug/reindex.rs index 02ecc6f009..0e37e7b527 100644 --- a/cli/src/commands/debug/reindex.rs +++ b/cli/src/commands/debug/reindex.rs @@ -35,7 +35,10 @@ pub fn cmd_debug_reindex( // be rebuilt while loading the repo. let workspace = command.load_workspace()?; let repo_loader = workspace.repo_loader(); - let op = op_walk::resolve_op_for_load(repo_loader, &command.global_args().at_operation)?; + let op = op_walk::resolve_op_for_load( + repo_loader, + command.global_args().at_operation.as_deref().unwrap_or("@"), + )?; let index_store = repo_loader.index_store(); if let Some(default_index_store) = index_store.as_any().downcast_ref::() { default_index_store.reinit().map_err(internal_error)?; diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index b7cfdf8101..004985f17e 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -85,7 +85,7 @@ pub fn cmd_git_clone( command: &CommandHelper, args: &GitCloneArgs, ) -> Result<(), CommandError> { - if command.global_args().at_operation != "@" { + if command.global_args().at_operation.is_some() { return Err(cli_error("--at-op is not respected")); } let remote_name = "origin"; diff --git a/cli/src/commands/git/init.rs b/cli/src/commands/git/init.rs index 4b463c8dc2..f627625c53 100644 --- a/cli/src/commands/git/init.rs +++ b/cli/src/commands/git/init.rs @@ -76,7 +76,7 @@ pub fn cmd_git_init( if command.global_args().ignore_working_copy { return Err(cli_error("--ignore-working-copy is not respected")); } - if command.global_args().at_operation != "@" { + if command.global_args().at_operation.is_some() { return Err(cli_error("--at-op is not respected")); } let cwd = command.cwd(); diff --git a/cli/src/commands/init.rs b/cli/src/commands/init.rs index 71ff673579..d499f5c2cd 100644 --- a/cli/src/commands/init.rs +++ b/cli/src/commands/init.rs @@ -55,7 +55,7 @@ pub(crate) fn cmd_init( if command.global_args().ignore_working_copy { return Err(cli_error("--ignore-working-copy is not respected")); } - if command.global_args().at_operation != "@" { + if command.global_args().at_operation.is_some() { return Err(cli_error("--at-op is not respected")); } let cwd = command.cwd(); diff --git a/cli/src/commands/operation/abandon.rs b/cli/src/commands/operation/abandon.rs index d337b399fe..3ec22090df 100644 --- a/cli/src/commands/operation/abandon.rs +++ b/cli/src/commands/operation/abandon.rs @@ -53,11 +53,10 @@ pub fn cmd_op_abandon( let op_store = repo_loader.op_store(); // It doesn't make sense to create concurrent operations that will be merged // with the current head. - let head_op_str = &command.global_args().at_operation; - if head_op_str != "@" { + if command.global_args().at_operation.is_some() { return Err(cli_error("--at-op is not respected")); } - let current_head_op = op_walk::resolve_op_for_load(repo_loader, head_op_str)?; + let current_head_op = op_walk::resolve_op_for_load(repo_loader, "@")?; let resolve_op = |op_str| op_walk::resolve_op_at(op_store, ¤t_head_op, op_str); let (abandon_root_op, abandon_head_op) = if let Some((root_op_str, head_op_str)) = args.operation.split_once("..") { diff --git a/cli/src/commands/operation/log.rs b/cli/src/commands/operation/log.rs index d0bc505ff5..d86b98b0ba 100644 --- a/cli/src/commands/operation/log.rs +++ b/cli/src/commands/operation/log.rs @@ -54,7 +54,7 @@ pub fn cmd_op_log( // operation id to be abandoned. let workspace = command.load_workspace()?; let repo_loader = workspace.repo_loader(); - let head_op_str = &command.global_args().at_operation; + let head_op_str = command.global_args().at_operation.as_deref().unwrap_or("@"); let head_ops = if head_op_str == "@" { // If multiple head ops can't be resolved without merging, let the // current op be empty. Beware that resolve_op_for_load() will eliminate diff --git a/cli/src/commands/util.rs b/cli/src/commands/util.rs index 54feeb7f84..aa85f0c067 100644 --- a/cli/src/commands/util.rs +++ b/cli/src/commands/util.rs @@ -173,7 +173,7 @@ fn cmd_util_gc( command: &CommandHelper, args: &UtilGcArgs, ) -> Result<(), CommandError> { - if command.global_args().at_operation != "@" { + if !command.is_at_head_operation() { return Err(user_error( "Cannot garbage collect from a non-head operation", )); diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index b06a2f5e0f..5fcc87bce9 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -166,15 +166,15 @@ To get started, see the tutorial at https://github.com/martinvonz/jj/blob/main/d This option only affects the check. It does not affect the `immutable_heads()` revset or the `immutable` template keyword. * `--at-operation ` — Operation to load the repo at - Operation to load the repo at. By default, Jujutsu loads the repo at the most recent operation. You can use `--at-op=` to see what the repo looked like at an earlier operation. For example `jj --at-op= st` will show you what `jj st` would have shown you when the given operation had just finished. + Operation to load the repo at. By default, Jujutsu loads the repo at the most recent operation, or at the merge of the concurrent operations if any. + + You can use `--at-op=` to see what the repo looked like at an earlier operation. For example `jj --at-op= st` will show you what `jj st` would have shown you when the given operation had just finished. `--at-op=@` is pretty much the same as the default except that concurrent operations will never be merged. Use `jj op log` to find the operation ID you want. Any unambiguous prefix of the operation ID is enough. When loading the repo at an earlier operation, the working copy will be ignored, as if `--ignore-working-copy` had been specified. It is possible to run mutating commands when loading the repo at an earlier operation. Doing that is equivalent to having run concurrent commands starting at the earlier operation. There's rarely a reason to do that, but it is possible. - - Default value: `@` * `--debug` — Enable debug logging * `--color ` — When to colorize output (always, never, debug, auto) * `--quiet` — Silence non-primary command output diff --git a/cli/tests/test_concurrent_operations.rs b/cli/tests/test_concurrent_operations.rs index 6bf8e96107..40fef43489 100644 --- a/cli/tests/test_concurrent_operations.rs +++ b/cli/tests/test_concurrent_operations.rs @@ -30,6 +30,13 @@ fn test_concurrent_operation_divergence() { &["describe", "-m", "message 2", "--at-op", "@-"], ); + // "--at-op=@" disables op heads merging + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "--at-op=@"]); + insta::assert_snapshot!(stderr, @r###" + Error: The "@" expression resolved to more than one operation + Hint: Try specifying one of the operations by ID: e31015019d90, 48f4a48f3f70 + "###); + // "op log" doesn't merge the concurrent operations let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(stdout, @r###" diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 00502df279..088c346cf7 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -607,7 +607,7 @@ fn test_help() { -R, --repository Path to repository to operate on --ignore-working-copy Don't snapshot the working copy, and don't update it --ignore-immutable Allow rewriting immutable commits - --at-operation Operation to load the repo at [default: @] [aliases: at-op] + --at-operation Operation to load the repo at [aliases: at-op] --debug Enable debug logging --color When to colorize output (always, never, debug, auto) --quiet Silence non-primary command output