Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop scary red color from immutable divergent commits #5800

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

martinvonz
Copy link
Member

@martinvonz martinvonz commented Feb 24, 2025

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

The top-level `immutable` and `conflict` labels we add to commits in
the node templates are also useful in the text part. For example, we
may want to color change ids for immutable commits differently (we do
at Google). I also added a `mutable` label, which I will use in the
next patch.
Divergent changes in immutable commits are fine. We don't want to
alarm the user by coloring them red.

#4451
"divergent" = "red"
"divergent change_id" = "red"
"mutable divergent" = "red"
"mutable divergent change_id" = "red"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be good to also change mutable divergent change IDs to be yellow or gray or some other less-scary color than red to make it clear that it's not something that absolutely needs to be resolved. Then maybe immutable and mutable commits could use the same styling?

Also, if immutable divergent change IDs look like normal change IDs and the ?? isn't a different color to make it stand out, then it might make it difficult to see in jj log whether to use the change ID or commit ID to refer to the commit. Maybe immutable divergent change IDs could be styled similar to a hidden commit instead to make it clear to users that they should avoid using the change ID?

<<node immutable::◆>> <<log change_id shortest prefix::z>><<log change_id shortest rest::zzzzzzz>><<log:: >><<log root::root()>><<log:: >><<log commit_id shortest prefix::0>><<log commit_id shortest rest::0000000>><<log::>>
[EOF]
");
insta::assert_snapshot!(render(r#"builtin_log_oneline"#), @"\u{1b}[1m\u{1b}[38;5;2m<<node working_copy mutable::@>>\u{1b}[0m \u{1b}[1m\u{1b}[38;5;13m<<log working_copy mutable change_id shortest prefix::r>>\u{1b}[38;5;8m<<log working_copy mutable change_id shortest rest::lvkpnrz>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;9m<<log working_copy mutable email placeholder::(no email set)>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;14m<<log working_copy mutable committer timestamp local format::2001-02-03 08:05:08>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;13m<<log working_copy mutable bookmarks name::my-bookmark>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;12m<<log working_copy mutable commit_id shortest prefix::d>>\u{1b}[38;5;8m<<log working_copy mutable commit_id shortest rest::c315397>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;10m<<log working_copy mutable empty::(empty)>>\u{1b}[39m<<log working_copy mutable:: >>\u{1b}[38;5;10m<<log working_copy mutable empty description placeholder::(no description set)>>\u{1b}[39m<<log working_copy mutable::>>\u{1b}[0m\n<<node mutable::○>> \u{1b}[1m\u{1b}[38;5;5m<<log mutable change_id shortest prefix::q>>\u{1b}[0m\u{1b}[38;5;8m<<log mutable change_id shortest rest::pvuntsm>>\u{1b}[39m<<log mutable:: >>\u{1b}[38;5;3m<<log mutable author email local::test.user>>\u{1b}[39m<<log mutable:: >>\u{1b}[38;5;6m<<log mutable committer timestamp local format::2001-02-03 08:05:07>>\u{1b}[39m<<log mutable:: >>\u{1b}[1m\u{1b}[38;5;4m<<log mutable commit_id shortest prefix::2>>\u{1b}[0m\u{1b}[38;5;8m<<log mutable commit_id shortest rest::30dd059>>\u{1b}[39m<<log mutable:: >>\u{1b}[38;5;2m<<log mutable empty::(empty)>>\u{1b}[39m<<log mutable:: >>\u{1b}[38;5;2m<<log mutable empty description placeholder::(no description set)>>\u{1b}[39m<<log mutable::>>\n\u{1b}[1m\u{1b}[38;5;14m<<node immutable::◆>>\u{1b}[0m \u{1b}[1m\u{1b}[38;5;5m<<log immutable change_id shortest prefix::z>>\u{1b}[0m\u{1b}[38;5;8m<<log immutable change_id shortest rest::zzzzzzz>>\u{1b}[39m<<log immutable:: >>\u{1b}[38;5;2m<<log immutable root::root()>>\u{1b}[39m<<log immutable:: >>\u{1b}[1m\u{1b}[38;5;4m<<log immutable commit_id shortest prefix::0>>\u{1b}[0m\u{1b}[38;5;8m<<log immutable commit_id shortest rest::0000000>>\u{1b}[39m<<log immutable::>>\n[EOF]");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these formatting change will be reverted in the next cargo-insta release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants