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

cli: add revsets.graph-prioritize setting #5627

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakobhellermann
Copy link
Contributor

@jakobhellermann jakobhellermann commented Feb 9, 2025

fixes #5506

I've been using this for a while now, using

[revsets]
log-graph-prioritize = "coalesce(dev, present(main))"

where dev is my megamerge. I've found it to increase the readability of the graph by quite a lot.

Before:
image

After:
image

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

^ not yet
also I'm gonna write a better commit message

@@ -147,6 +147,11 @@ pub(crate) fn cmd_log(
}
expression
};
let prio_revset = settings
.get_string("revsets.log-graph-prioritize")
.optional()?
Copy link
Contributor

@yuja yuja Feb 10, 2025

Choose a reason for hiding this comment

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

I think the default present(@) can be defined in config/revsets.toml.

Or do you want to set prioritized branch in addition to @? In that case, which revset should be displayed first? We might have to add a list of prioritized revsets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I originally tried adding it addition, but I don't think it made a big difference from my testing, and a single revset was always enough for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it should just expect that the revset only contains a single revision. I'm not sure renderdag does much with multiple prioritizations, so usually coalesce(1, 2, 3) is what you want instead of 1 | 2 | 3.
Especially since the order of the revset is not defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should just expect that the revset only contains a single revision.

Yes, the set size should usually be 0 or 1, but I wouldn't add extra check for that.

We might want to add some sorting or ordered union revset functions that can be used only in certain context, but I'm not sure.

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.

FR: Make prioritization in jj log configurable
2 participants