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

smartlog: replace --reverse flag with config opt #1315

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

e-q
Copy link

@e-q e-q commented May 20, 2024

My perspective, based on my own experience, is that folks generally stick to one log orientation/direction. In principle, the reverse flag can be incorporated by default via an alias, but this doesn’t catch the auto-smartlog that is emitted after some operations.

This PR adds a Boolean configuration option that replaces the —reverse flag: branchless.smartlog.reverse

This is a breaking CLI change, so I don’t imagine this would incorporated as-is. However, I didn’t want to go too far down this path without considering your thoughts and opinions on how a configuration option may be incorporated. (Not to mention my very limited Rust experience 😅)

What do you think? Thanks for all your work on such a helpful tool!

@arxanas
Copy link
Owner

arxanas commented May 26, 2024

A configuration option makes sense to me, since I've never once wanted to enable it for git-branchless and never once wanted to disable the equivalent option for jj 😅.

The git-branchless-smartlog output isn't really meant to be machine parseable (you can use git-branchless-query for that), so I'm optimistic that nobody's workflow would be broken by this. I posted in git-branchless Discord and jj Discord as well to see if anyone had any thoughts.

If we're worried about it, we could make the command-line flag deprecated (print a warning when used) but continue to support it.

@e-q e-q force-pushed the reverse-opt branch 2 times, most recently from 8aeb3cc to 6135b32 Compare May 28, 2024 14:35
@e-q
Copy link
Author

e-q commented May 28, 2024

Thanks for considering this! I’ve added a commit that puts the CLI flag back, and prints a deprecation warning. Feel free to modify this as you see fit.

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.

2 participants