diff --git a/CHANGELOG.md b/CHANGELOG.md index cf0a62a93e..935f07b6ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 pattern syntax](docs/revsets.md#string-patterns). The `--glob` option is deprecated in favor of `glob:` pattern. +* The `branches`/`tags`/`git_refs`/`git_head` template keywords now return a + list of `RefName`s. They were previously pre-formatted strings. + ### Fixed bugs * Updating the working copy to a commit where a file that's currently ignored diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 404ef3c034..1d4009d23c 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -24,7 +24,7 @@ use jj_lib::hex_util::to_reverse_hex; use jj_lib::id_prefix::IdPrefixContext; use jj_lib::op_store::{RefTarget, WorkspaceId}; use jj_lib::repo::Repo; -use jj_lib::rewrite; +use jj_lib::{git, rewrite}; use once_cell::unsync::OnceCell; use crate::formatter::Formatter; @@ -35,7 +35,7 @@ use crate::template_parser::{ self, FunctionCallNode, TemplateAliasesMap, TemplateParseError, TemplateParseResult, }; use crate::templater::{ - IntoTemplate, PlainTextFormattedProperty, Template, TemplateFunction, TemplateProperty, + self, IntoTemplate, PlainTextFormattedProperty, Template, TemplateFunction, TemplateProperty, TemplatePropertyFn, }; use crate::text_util; @@ -79,6 +79,18 @@ impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo, '_> { |item| self.wrap_commit(item), ) } + CommitTemplatePropertyKind::RefName(property) => { + build_ref_name_method(self, build_ctx, property, function) + } + CommitTemplatePropertyKind::RefNameList(property) => { + template_builder::build_formattable_list_method( + self, + build_ctx, + property, + function, + |item| self.wrap_ref_name(item), + ) + } CommitTemplatePropertyKind::CommitOrChangeId(property) => { build_commit_or_change_id_method(self, build_ctx, property, function) } @@ -106,6 +118,20 @@ impl<'repo> CommitTemplateLanguage<'repo, '_> { CommitTemplatePropertyKind::CommitList(Box::new(property)) } + fn wrap_ref_name( + &self, + property: impl TemplateProperty + 'repo, + ) -> CommitTemplatePropertyKind<'repo> { + CommitTemplatePropertyKind::RefName(Box::new(property)) + } + + fn wrap_ref_name_list( + &self, + property: impl TemplateProperty> + 'repo, + ) -> CommitTemplatePropertyKind<'repo> { + CommitTemplatePropertyKind::RefNameList(Box::new(property)) + } + fn wrap_commit_or_change_id( &self, property: impl TemplateProperty + 'repo, @@ -125,6 +151,8 @@ enum CommitTemplatePropertyKind<'repo> { Core(CoreTemplatePropertyKind<'repo, Commit>), Commit(Box + 'repo>), CommitList(Box> + 'repo>), + RefName(Box + 'repo>), + RefNameList(Box> + 'repo>), CommitOrChangeId(Box + 'repo>), ShortestIdPrefix(Box + 'repo>), } @@ -133,8 +161,16 @@ impl<'repo> IntoTemplateProperty<'repo, Commit> for CommitTemplatePropertyKind<' fn try_into_boolean(self) -> Option + 'repo>> { match self { CommitTemplatePropertyKind::Core(property) => property.try_into_boolean(), - // TODO: should we allow implicit cast of List type? - _ => None, + CommitTemplatePropertyKind::Commit(_) => None, + CommitTemplatePropertyKind::CommitList(property) => { + Some(Box::new(TemplateFunction::new(property, |l| !l.is_empty()))) + } + CommitTemplatePropertyKind::RefName(_) => None, + CommitTemplatePropertyKind::RefNameList(property) => { + Some(Box::new(TemplateFunction::new(property, |l| !l.is_empty()))) + } + CommitTemplatePropertyKind::CommitOrChangeId(_) => None, + CommitTemplatePropertyKind::ShortestIdPrefix(_) => None, } } @@ -162,6 +198,8 @@ impl<'repo> IntoTemplateProperty<'repo, Commit> for CommitTemplatePropertyKind<' CommitTemplatePropertyKind::Core(property) => property.try_into_template(), CommitTemplatePropertyKind::Commit(_) => None, CommitTemplatePropertyKind::CommitList(_) => None, + CommitTemplatePropertyKind::RefName(property) => Some(property.into_template()), + CommitTemplatePropertyKind::RefNameList(property) => Some(property.into_template()), CommitTemplatePropertyKind::CommitOrChangeId(property) => { Some(property.into_template()) } @@ -273,23 +311,23 @@ fn build_commit_keyword_opt<'repo>( } "branches" => { let index = cache.branches_index(repo).clone(); - language.wrap_string(wrap_fn(property, move |commit| { - index.get(commit.id()).join(" ") // TODO: return Vec? + language.wrap_ref_name_list(wrap_fn(property, move |commit| { + index.get(commit.id()).to_vec() })) } "tags" => { let index = cache.tags_index(repo).clone(); - language.wrap_string(wrap_fn(property, move |commit| { - index.get(commit.id()).join(" ") // TODO: return Vec? + language.wrap_ref_name_list(wrap_fn(property, move |commit| { + index.get(commit.id()).to_vec() })) } "git_refs" => { let index = cache.git_refs_index(repo).clone(); - language.wrap_string(wrap_fn(property, move |commit| { - index.get(commit.id()).join(" ") // TODO: return Vec? + language.wrap_ref_name_list(wrap_fn(property, move |commit| { + index.get(commit.id()).to_vec() })) } - "git_head" => language.wrap_string(wrap_repo_fn(repo, property, extract_git_head)), + "git_head" => language.wrap_ref_name_list(wrap_repo_fn(repo, property, extract_git_head)), "divergent" => language.wrap_boolean(wrap_fn(property, |commit| { // The given commit could be hidden in e.g. obslog. let maybe_entries = repo.resolve_change_id(commit.change_id()); @@ -332,22 +370,89 @@ fn extract_working_copies(repo: &dyn Repo, commit: &Commit) -> String { names.join(" ") } +/// Branch or tag name with metadata. +#[derive(Clone, Debug, Eq, PartialEq)] +struct RefName { + /// Local name. + name: String, + /// Remote name if this is a remote or Git-tracking ref. + remote: Option, + /// Ref target has conflicts. + conflict: bool, + /// Local ref is synchronized with all tracking remotes. + synced: bool, +} + +impl RefName { + fn is_local(&self) -> bool { + self.remote.is_none() + } +} + +impl Template<()> for RefName { + fn format(&self, _: &(), formatter: &mut dyn Formatter) -> io::Result<()> { + write!(formatter.labeled("name"), "{}", self.name)?; + if let Some(remote) = &self.remote { + write!(formatter, "@")?; + write!(formatter.labeled("remote"), "{remote}")?; + } + // Don't show both conflict and unsynced sigils as conflicted ref wouldn't + // be pushed. + if self.conflict { + write!(formatter, "??")?; + } else if self.is_local() && !self.synced { + write!(formatter, "*")?; + } + Ok(()) + } +} + +impl Template<()> for Vec { + fn format(&self, _: &(), formatter: &mut dyn Formatter) -> io::Result<()> { + templater::format_joined(&(), formatter, self, " ") + } +} + +fn build_ref_name_method<'repo>( + language: &CommitTemplateLanguage<'repo, '_>, + _build_ctx: &BuildContext>, + self_property: impl TemplateProperty + 'repo, + function: &FunctionCallNode, +) -> TemplateParseResult> { + let property = match function.name { + "name" => { + template_parser::expect_no_arguments(function)?; + language.wrap_string(TemplateFunction::new(self_property, |ref_name| { + ref_name.name + })) + } + "remote" => { + template_parser::expect_no_arguments(function)?; + language.wrap_string(TemplateFunction::new(self_property, |ref_name| { + ref_name.remote.unwrap_or_default() + })) + } + // TODO: expose conflict, synced, remote.is_some() + _ => return Err(TemplateParseError::no_such_method("RefName", function)), + }; + Ok(property) +} + /// Cache for reverse lookup refs. #[derive(Clone, Debug, Default)] struct RefNamesIndex { - // TODO: store Branch/NameRef type in place of String? - index: HashMap>, + index: HashMap>, } impl RefNamesIndex { - fn insert<'a>(&mut self, ids: impl IntoIterator, name: String) { + fn insert<'a>(&mut self, ids: impl IntoIterator, name: RefName) { for id in ids { let ref_names = self.index.entry(id.clone()).or_default(); ref_names.push(name.clone()); } } - fn get(&self, id: &CommitId) -> &[String] { + fn get(&self, id: &CommitId) -> &[RefName] { if let Some(names) = self.index.get(id) { names } else { @@ -364,28 +469,25 @@ fn build_branches_index(repo: &dyn Repo) -> RefNamesIndex { let unsynced_remote_refs = remote_refs.iter().copied().filter(|&(_, remote_ref)| { !remote_ref.is_tracking() || remote_ref.target != *local_target }); - let has_unsynced_tracking_refs = || { - remote_refs.iter().any(|&(_, remote_ref)| { - remote_ref.is_tracking() && remote_ref.target != *local_target - }) - }; if local_target.is_present() { - let decorated_name = if local_target.has_conflict() { - format!("{branch_name}??") - } else if has_unsynced_tracking_refs() { - format!("{branch_name}*") - } else { - branch_name.to_owned() + let ref_name = RefName { + name: branch_name.to_owned(), + remote: None, + conflict: local_target.has_conflict(), + synced: remote_refs.iter().all(|&(_, remote_ref)| { + !remote_ref.is_tracking() || remote_ref.target == *local_target + }), }; - index.insert(local_target.added_ids(), decorated_name); + index.insert(local_target.added_ids(), ref_name); } for (remote_name, remote_ref) in unsynced_remote_refs { - let decorated_name = if remote_ref.target.has_conflict() { - format!("{branch_name}@{remote_name}?") - } else { - format!("{branch_name}@{remote_name}") + let ref_name = RefName { + name: branch_name.to_owned(), + remote: Some(remote_name.to_owned()), + conflict: remote_ref.target.has_conflict(), + synced: false, }; - index.insert(remote_ref.target.added_ids(), decorated_name); + index.insert(remote_ref.target.added_ids(), ref_name); } } index @@ -396,27 +498,30 @@ fn build_ref_names_index<'a>( ) -> RefNamesIndex { let mut index = RefNamesIndex::default(); for (name, target) in ref_pairs { - let decorated_name = if target.has_conflict() { - format!("{name}?") - } else { - name.clone() + let ref_name = RefName { + name: name.to_owned(), + remote: None, + conflict: target.has_conflict(), + synced: true, // has no tracking remotes }; - index.insert(target.added_ids(), decorated_name); + index.insert(target.added_ids(), ref_name); } index } -// TODO: return NameRef? -fn extract_git_head(repo: &dyn Repo, commit: &Commit) -> String { +// TODO: maybe add option or nullable type? +fn extract_git_head(repo: &dyn Repo, commit: &Commit) -> Vec { let target = repo.view().git_head(); if target.added_ids().contains(commit.id()) { - if target.has_conflict() { - "HEAD@git?".to_string() - } else { - "HEAD@git".to_string() - } + let ref_name = RefName { + name: "HEAD".to_owned(), + remote: Some(git::REMOTE_NAME_FOR_LOCAL_GIT_REPO.to_owned()), + conflict: target.has_conflict(), + synced: false, // has no local counterpart + }; + vec![ref_name] } else { - "".to_string() + vec![] } } diff --git a/cli/src/operation_templater.rs b/cli/src/operation_templater.rs index 5533829186..d538ce76d7 100644 --- a/cli/src/operation_templater.rs +++ b/cli/src/operation_templater.rs @@ -81,7 +81,7 @@ impl IntoTemplateProperty<'static, Operation> for OperationTemplatePropertyKind fn try_into_boolean(self) -> Option>> { match self { OperationTemplatePropertyKind::Core(property) => property.try_into_boolean(), - _ => None, + OperationTemplatePropertyKind::OperationId(_) => None, } } diff --git a/cli/src/template_builder.rs b/cli/src/template_builder.rs index 663c72a866..20539c8c3c 100644 --- a/cli/src/template_builder.rs +++ b/cli/src/template_builder.rs @@ -162,9 +162,19 @@ impl<'a, I: 'a> IntoTemplateProperty<'a, I> for CoreTemplatePropertyKind<'a, I> CoreTemplatePropertyKind::String(property) => { Some(Box::new(TemplateFunction::new(property, |s| !s.is_empty()))) } - // TODO: should we allow implicit cast of List type? + CoreTemplatePropertyKind::StringList(property) => { + Some(Box::new(TemplateFunction::new(property, |l| !l.is_empty()))) + } CoreTemplatePropertyKind::Boolean(property) => Some(property), - _ => None, + CoreTemplatePropertyKind::Integer(_) => None, + CoreTemplatePropertyKind::Signature(_) => None, + CoreTemplatePropertyKind::Timestamp(_) => None, + CoreTemplatePropertyKind::TimestampRange(_) => None, + // Template types could also be evaluated to boolean, but it's less likely + // to apply label() or .map() and use the result as conditional. It's also + // unclear whether ListTemplate should behave as a "list" or a "template". + CoreTemplatePropertyKind::Template(_) => None, + CoreTemplatePropertyKind::ListTemplate(_) => None, } } @@ -1145,6 +1155,50 @@ mod tests { "###); } + #[test] + fn test_boolean_cast() { + let mut env = TestTemplateEnv::default(); + + insta::assert_snapshot!(env.render_ok(r#"if("", true, false)"#), @"false"); + insta::assert_snapshot!(env.render_ok(r#"if("a", true, false)"#), @"true"); + + env.add_keyword("sl0", |language| { + language.wrap_string_list(Literal::>(vec![])) + }); + env.add_keyword("sl1", |language| { + language.wrap_string_list(Literal(vec!["".to_owned()])) + }); + insta::assert_snapshot!(env.render_ok(r#"if(sl0, true, false)"#), @"false"); + insta::assert_snapshot!(env.render_ok(r#"if(sl1, true, false)"#), @"true"); + + // No implicit cast of integer + insta::assert_snapshot!(env.parse_err(r#"if(0, true, false)"#), @r###" + --> 1:4 + | + 1 | if(0, true, false) + | ^ + | + = Expected expression of type "Boolean" + "###); + + insta::assert_snapshot!(env.parse_err(r#"if(label("", ""), true, false)"#), @r###" + --> 1:4 + | + 1 | if(label("", ""), true, false) + | ^-----------^ + | + = Expected expression of type "Boolean" + "###); + insta::assert_snapshot!(env.parse_err(r#"if(sl0.map(|x| x), true, false)"#), @r###" + --> 1:4 + | + 1 | if(sl0.map(|x| x), true, false) + | ^------------^ + | + = Expected expression of type "Boolean" + "###); + } + #[test] fn test_list_method() { let mut env = TestTemplateEnv::default(); diff --git a/cli/tests/test_commit_template.rs b/cli/tests/test_commit_template.rs index 1b74165168..d97683c99f 100644 --- a/cli/tests/test_commit_template.rs +++ b/cli/tests/test_commit_template.rs @@ -408,7 +408,7 @@ fn test_log_branches() { test_env.jj_cmd_ok(&origin_path, &["git", "export"]); test_env.jj_cmd_ok(&workspace_root, &["git", "fetch"]); - let template = r#"commit_id.short() ++ " " ++ branches"#; + let template = r#"commit_id.short() ++ " " ++ if(branches, branches, "(no branches)")"#; let output = test_env.jj_cmd_success(&workspace_root, &["log", "-T", template]); insta::assert_snapshot!(output, @r###" ◉ fed794e2ba44 branch3?? branch3@origin @@ -419,7 +419,21 @@ fn test_log_branches() { │ @ a5b4d15489cc branch2* new-branch │ ◉ 8476341eb395 branch2@origin ├─╯ - ◉ 000000000000 + ◉ 000000000000 (no branches) + "###); + + let template = r#"branches.map(|b| separate("/", b.remote(), b.name())).join(", ")"#; + let output = test_env.jj_cmd_success(&workspace_root, &["log", "-T", template]); + insta::assert_snapshot!(output, @r###" + ◉ branch3, origin/branch3 + │ ◉ branch3 + ├─╯ + │ ◉ branch1 + ├─╯ + │ @ branch2, new-branch + │ ◉ origin/branch2 + ├─╯ + ◉ "###); } diff --git a/docs/templates.md b/docs/templates.md index 7cc0c9cb86..36bb88a1b8 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -25,10 +25,10 @@ The following keywords can be used in `jj log`/`jj obslog` templates. working-copy commit as `@`. * `current_working_copy: Boolean`: True for the working-copy commit of the current workspace. -* `branches: String` -* `tags: String` -* `git_refs: String` -* `git_head: String` +* `branches: List` +* `tags: List` +* `git_refs: List` +* `git_head: List` * `divergent: Boolean`: True if the commit's change id corresponds to multiple visible commits. * `hidden: Boolean`: True if the commit is not visible (a.k.a. abandoned). @@ -95,7 +95,8 @@ No methods are defined. ### List type -The following methods are defined. +A list can be implicitly converted to `Boolean`. The following methods are +defined. * `.join(separator: Template) -> Template`: Concatenate elements with the given `separator`. @@ -114,6 +115,13 @@ The following methods are defined. * `.short([len: Integer]) -> String` +### RefName type + +The following methods are defined. + +* `.name() -> String`: Local branch or tag name. +* `.remote() -> String`: Remote name or empty if this is a local ref. + ### ShortestIdPrefix type The following methods are defined.