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: change ui.default-revset to be based on trunk() #2264

Closed
wants to merge 1 commit into from

Conversation

rslabbert
Copy link
Contributor

@rslabbert rslabbert commented Sep 18, 2023

As with #2088, I'm not wedded to this, but it seems like a valuable change for my workflow at least and matches git-branchless sl.

Still a work in progress, requires updating tests which use raw jj log. Most likely I'll throw in a PR before this one to change them all to use jj log -rall() for robustness if the default needs to be changed in the future.

@@ -355,7 +355,7 @@ struct StatusArgs {}
#[derive(clap::Args, Clone, Debug)]
struct LogArgs {
/// Which revisions to show. Defaults to the `revsets.log` setting, or
/// `@ | ancestors((remote_branches() | tags()).., 2)` if it is not set.
/// `(trunk()..@):: | (trunk()..@)-` if it is not set.
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to see all unmerged branches (i.e. ancestors(trunk().., 2)) by default

Copy link
Contributor

Choose a reason for hiding this comment

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

For either option, I'd change it a bit to add a few more revisions:

ancestors(trunk().., 2) | trunk() | @ in Yuya's case.

ancestors(trunk()..@, 2)| trunk() in your case.

Less actionable thoughts

I have the same preference as Yuya for my personal config, though I'm not sure this is everybody's preference. I think the revset you suggested is also useful. I'm trying out a stack alias for trunk()..@ and a logstack alias for the revset I proposed above.

I also put some thoughts about how a default revset could work in a world with immutable heads in #2247 (comment).

@@ -159,7 +159,7 @@ impl UserSettings {
// For compatibility with old config files (<0.8.0)
self.config
.get_string("ui.default-revset")
.unwrap_or_else(|_| "@ | ancestors((remote_branches() | tags()).., 2)".to_string())
.unwrap_or_else(|_| "(trunk()..@):: | (trunk()..@)-".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to update cli/src/config-schema.json

Copy link
Contributor

Choose a reason for hiding this comment

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

Re commit description and PR name: please change ui.default-revset to revsets.log. The former only seems to exists for backwards compatibility.

@martinvonz
Copy link
Member

The last commit in #2247 probably replaces this PR.

@rslabbert rslabbert closed this Sep 25, 2023
@rslabbert rslabbert deleted the push-vzxvsmropynu branch September 25, 2023 21:34
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.

4 participants