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: ability to disable default configs #5538

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

Conversation

dpc
Copy link

@dpc dpc commented Jan 31, 2025

My motivation is mostly to address #5098 , but in a lot of software there's a flag to disable sourcing default configs, which is useful in a lot contexts.

A more granular (or just atlernative/better) approaches would require more design and debate, while this seems simple and immediately useful (at least to some).

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

Copy link

google-cla bot commented Jan 31, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@martinvonz
Copy link
Member

Thanks, but JJ_CONFIG= jj should already have this effect

@dpc dpc force-pushed the dpc/25-01-31-no-default-config-2 branch from bbdb7f4 to 33c1c16 Compare January 31, 2025 20:49
@dpc
Copy link
Author

dpc commented Jan 31, 2025

@martinvonz The JJ_CONFIG controls the user's config, no? While I'm trying to disable the default configs shipped with jj.

@martinvonz
Copy link
Member

Oh, I'm sorry. Interesting. We have a lot of code that depends on the default configs existing, so I think it's going to be more complicated to support than this PR's current state.

@dpc
Copy link
Author

dpc commented Jan 31, 2025

@martinvonz I just noticed. Oops.

As a first user who'd want this, I don't mind just adding all the required keys to my config, and thus being sure I am in full control.

Also (or/and), I could add unwrap_or_default() on most obvious ones.

Also (or/and) I could add JJ_CONFIG_NO_DEFAULT_COLORS.

And (or/and), some config toml could be considered "essential", and loaded even with JJ_NO_DEFAULT_CONFIG.

@dpc
Copy link
Author

dpc commented Jan 31, 2025

image

This seems to have no issues.

@dpc dpc force-pushed the dpc/25-01-31-no-default-config-2 branch from f595d73 to b5a4577 Compare January 31, 2025 21:35
@ilyagr
Copy link
Contributor

ilyagr commented Feb 1, 2025

For the colors issue, I wonder if it'd be better to introduce a notion of themes. So, the colors we have now would become the default theme, and you could reset them all by using another theme.

Settings like templates.log_node could also be set by a theme, though I don't think there's any reasonable "undefined" value that can have by default1.

Recently, scoped config was introduced. I'm not 100% sure about that direction, but perhaps it's sufficient to create a "default theme" scope that contains all the color config and is activated by default. (This would hopefully be relatively easy, but it's not trivial, it still needs some design work on how these theme scopes work)

In principle, we could do this to things other than the color config, any kind of thing that people would want to "turn off" en masse, and for which leaving it "undefined" makes sense.

Footnotes

  1. There are many other such settings; in my mind, any jj command is allowed to crash if the default config is not loaded. The first example that comes to mind is not merged yet, but https://github.com/jj-vcs/jj/pull/5415 would crash if you deleted the default config and tried using :builtin pager.

@jakobhellermann
Copy link
Contributor

Recently, scoped config was introduced. I'm not 100% sure about that direction, but perhaps it's sufficient to create a "default theme" scope that contains all the color config and is activated by default. (This would hopefully be relatively easy, but it's not trivial, it still needs some design work on how these theme scopes work)

Instead of having a "theme scope", we could introduce a generic "config scope", that is enabled when a config variable is set to a certain value:

[[--scope]]
--when.config = ["ui.enable-colors"] # equivalent to ["ui.enable-colors", "true"]
[--scope.colors]
"error" = { fg = "default", bold = true }
"error_source" = { fg = "default" }
...

or

[[--scope]]
--when.config = ["ui.theme", ":builtin"]
[--scope.colors]
"error" = { fg = "default", bold = true }
"error_source" = { fg = "default" }
...

@yuja
Copy link
Contributor

yuja commented Feb 1, 2025

I wouldn't add any "conditional" thingy that also depends on config values (and/or any other dynamic variables.) That would make resolution complicated.

For color overrides, it's probably better to add some indirection dedicated for [colors] table. #3594

FWIW, the default colors table can be reset by abusing invalid value, but please don't expect this will work forever.

[[--scope]]
colors = 0  # reset colors table by assigning non-table value
[[--scope]]
colors.commit_id = "red"

@ilyagr
Copy link
Contributor

ilyagr commented Feb 3, 2025

I agree that this request seems like it would be covered by a "theme"-like config that deals with the "colors" table only. It makes sense to me to consider this a dupe of #3594 .


Aside: At the same time, the contrarian in me started wondering about how general such a mechanism could be. I think we could, at an extreme, have a special table [config-toggles] that none of the --scope configs are allowed to modify. Then, I think it might be OK (though not necessarily a great idea) to allow scopes that depend on values inside the [config-toggles] table and are allowed to modify (almost?) anything other than the config-toggles table.

In other words, I'm guessing Yuya's main concern was that it would be confusing if scopes would depend on parameters that they could also modify, and I'm curious whether that's correct. There are definitely other concerns, such as whether this amount of flexibility is worth the potentially confusing configs it will make possible.


If there are other examples of configs that would be nice to disable en masse, that'd also be interesting to discuss, but I think it's best to be more specific than "let's disable everything".

For example, if you use jj compiled with this patch for a while, it could be interesting to get an experience report of which configs you found most useful to disable all at once (beyond colors, where the use-case already pretty clear).

@dpc
Copy link
Author

dpc commented Feb 3, 2025

So far I've disabled the colors and moved on. The problem with colors is that it's impossible to reset them after the fact due to way the selector priority works. Everything else seems easily overrideable, so not as important.

To me this is a stop-gap measure. Not sure how long will it take to debate on the more proper system. Might be worth to land a env var to disable just the colors to unblock people like me and focus on whatever more pressing things.

@ilyagr
Copy link
Contributor

ilyagr commented Feb 4, 2025

My preference would be not to merge the stop-gap measure as is. It feels a bit hacky, and it feels like this problem would only be urgent for a few people. Are you unable to use a custom version of jj compiled with this patch? If you're willing to go this far to get jj customized just the way you want it, what's one more step?

I do think that this is a real issue that would be nice to solve, just not so urgent to be worth exposing everybody to a temporary stop-gap that will be very likely to lead to a breaking change later.

See also #5571 (though if we go with something like themes, it might not actually be that relevant).

@ilyagr
Copy link
Contributor

ilyagr commented Feb 4, 2025

Oh, and as a complete aside: the reason I first found jj was that I added a few hacky patches to a different project, that I wanted to use in my personal binary before they were polished and merged. Git wasn't very flexible at tracking the merge of all of my hacky patches and at continuously rebasing them on top of the master branch as it got updated. So, I found git-branchless and then jj as I was looking for better ways to do the job.

jj remains my favorite tools for tracking a set of hacky patches to jj :) (or to any open-source project).

@yuja
Copy link
Contributor

yuja commented Feb 4, 2025

As a workaround, colors table can be emptied by setting non-table value. It's ugly, but works for now. Custom binary isn't needed.
#5538 (comment)

@emilazy
Copy link
Contributor

emilazy commented Feb 4, 2025

It might be nice for Jujutsu to respect $NO_COLOR.

@ilyagr
Copy link
Contributor

ilyagr commented Feb 4, 2025

It might be nice for Jujutsu to respect $NO_COLOR.

It does, though it might not be obvious due to 3d41db1 . JJ_CONFIG=/dev/null NO_COLOR=1 jj log works for me.

(I am not sure if I agree with that design, it certainly makes NO_COLOR not very useful for me, but apparently that's how it's supposed to work according to the spec. I've never relied on NO_COLOR, so I'm OK with it not being very useful to me even if I don't understand why the spec is this way. )

But I think the OP wants color, they just want color for exactly the things they specify and nothing else.

My motivation is mostly to address jj-vcs#5098, even as a stopgap meassure.
@dpc dpc force-pushed the dpc/25-01-31-no-default-config-2 branch from b5a4577 to b7bec6b Compare February 4, 2025 04:59
@dpc
Copy link
Author

dpc commented Feb 4, 2025

But I think the OP wants color, they just want color for exactly the things they specify and nothing else.

Exactly. I'm a very hard to please, opinionated and particular about everything. Just the worst kind of user really.

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.

6 participants