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

util: add exec command for arbitrary aliases #4759

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

senekor
Copy link
Collaborator

@senekor senekor commented Nov 3, 2024

closes #3001

I also believe that this PR, if accepted, makes #3673 redundant. The discussion there revolves around some kind of custom, lightweight, embedded scripting language. But the original use case is just streamlining workflows, which is fully covered by this PR. Nothing gives the user more flexibility and is less of a maintenance burden to us than letting users bring their own scripting language.

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

@senekor
Copy link
Collaborator Author

senekor commented Nov 3, 2024

thread 'test_util_command::test_util_exec' panicked
stderr="Error: No such file or directory (os error 2)\n"

Nix doesn't have echo installed by default? 😳 lol. What do I do? 🤔

cli/src/commands/util.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

Blocking on missing consensus.

cli/src/commands/util.rs Outdated Show resolved Hide resolved
cli/tests/test_util_command.rs Outdated Show resolved Hide resolved
@senekor senekor force-pushed the remo/util-exec branch 3 times, most recently from 22aac15 to 0145ba3 Compare November 3, 2024 14:42
@senekor
Copy link
Collaborator Author

senekor commented Nov 3, 2024

also closes #3082

cli/src/commands/util.rs Outdated Show resolved Hide resolved
cli/src/commands/util.rs Outdated Show resolved Hide resolved
cli/src/commands/util.rs Outdated Show resolved Hide resolved
cli/src/commands/util.rs Outdated Show resolved Hide resolved
cli/src/commands/util.rs Outdated Show resolved Hide resolved
@senekor senekor linked an issue Nov 4, 2024 that may be closed by this pull request
@senekor
Copy link
Collaborator Author

senekor commented Nov 4, 2024

@PhilipMetzger who should give their approval for consensus to be achieved? Are there remaining open questions?

@PhilipMetzger
Copy link
Collaborator

@PhilipMetzger who should give their approval for consensus to be achieved? Are there remaining open questions?

We have no final decision if we want to support a jj util exec command or pursue @matts1's work in #3673 (#4605) further. While this is definitely a step-up, I'd rather avoid such a capability until we have a agreed upon solution. There's a reason why the discussion in the issue became dormant.

The discussion there revolves around some kind of custom, lightweight, embedded scripting language. But the original use case is just streamlining workflows, which is fully covered by this PR. Nothing gives the user more flexibility and is less of a maintenance burden to us than letting users bring their own scripting language.

We've also had a few talks about having a embeddedable scripting language with jj script, so I think it is the favored approach.

@senekor
Copy link
Collaborator Author

senekor commented Nov 4, 2024

if we want to support jj util exec command

There's not much to support, the essence of this feature is a one-liner.

I'd rather avoid such a capability until we have a agreed upon solution

What's the harm in having both? Even if an embedded scripting language ends up materializing at some point, this trivial feature does not impose any additional maintenance burden.

jj is not at 1.0 yet, this feature could even be removed if an alternative, preferred solution has become mature enough.

I'm personally not in favor of an embedded scripting language. I don't see any benefit compared to this simple approach. On the contrary, it is a huge amount of work and maintenance burden. It will never be as flexible as shelling out. Even if we embed the most-amazing-and-objectively-superior-to-any-other-scripting-language TM (unlikely), we will still be forcing people to learn yet-another-language instead of letting them use the tools they know and are good at.

I don't want to have to argue against it though, that's no fun. I'd rather just we have both, to which there are no downsides.

@thoughtpolice
Copy link
Collaborator

thoughtpolice commented Nov 4, 2024

I don't see any benefit compared to this simple approach.

The most important one are the subtle downstream consequences. Scripts can be pure and deterministic by default, so you can be sure that rerunning commands is either idempotent or fully reversible through jj undo; many things that e.g. reword commits, check commit messages, or any number of things that are pure operations on the commit graph are great for this. Scripts can be used on every platform, because they don't need any configuration changes and can be portable by default; a subtle and non-obvious consequence of your example above is that /path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine. This means users can no longer use a trivial single configuration file but instead has to have multiple configurations for each platform (or we need to introduce like platform-conditional maps in our TOML files, [[platform.x86_64-unknown-linux-gnu]] like cargo... Ugh.)[1]

But also, scripts can be much more secure by default; they can for example be given no access to the network or any other number of features, can invoke UI prompts or wizards to the user, etc. That also means scripts can easily be checked into the source repository without fear that random things will get run without review. In the future, I also hope that a jj-script mechanism could work on the server side, and also inside your IDE. That means that you could write a script that shows a prompt in visual studio code instead, or one that runs a script on the server-side for your code review tool, etc. For example, people like my "Mega Merge" strategy, and you can imagine a script that could prompt you for the set of branches to make a mega-merge out of, or what base branch they should be rebased on, etc. "Randomly invoke thing out of $PATH" will never clear a security review for many cases and is a complete non-starter anyway in things like a server-side component.

As a more concrete example, one of Matt's previous requests was that jj gerrit should be able to reject a commit message if some kind of "tag" isn't there e.g. BUG=1234 is a requirement for all Chromium commits, I think. This isn't very flexible in the current implementation, but with a scripting solution any team could write a customized jj-gerrit.bzl that meets their exact requirements, and then check it into the repository for all users to use. My goal with a jj script is that it should at minimum be able to implement an alternative to #2845, since it has a very limited scope for the most basic uses, and more advanced cases (API access) can be handled incrementally with something like secure HTTP calls.

So, there are many conceptual advantages. I have a private prototype but it's very slow going because I'm busy with a lot right now. But I want to be clear that an unintended consequence of just giving people access to the lowest-common-denominator fast-access dopamine-hit tool — in this case, random shell scripts — is that they often will suck the air out of the room for alternative tools, and they will often miss many subtle things like the ones I listed above (e.g. requiring some monstrous Git bash installation, or just not working on Windows at all.)

I also want to be clear this isn't a rejection of the idea of having an exec utility command. It isn't an endorsement, either. I'm just giving you some clarity on what I think can be gained from the alternatives, is all.

we will still be forcing people to learn yet-another-language instead of letting them use the tools they know and are good at.

Well, to be fair, they already have to adopt Jujutsu, which they probably aren't very good with at first, in my experience (speaking as someone who felt that myself for a little while.) In my case the scripting language would be Starlark based, which I think is probably as good as it gets in the "familiar" territory (it's just Python-lite.)


[1] Actually, the cross-platform thing extends to many other examples, but this is the most obvious. The reason I did not just write a wrapper jj-gerrit command is exactly because I wanted to use my config and this tool on all my machines, and I use Windows too. Therefore, I have to either write the tool in a full blown language anyway, completely negating the fast scripting advantage, or write it twice, etc etc...

@senekor
Copy link
Collaborator Author

senekor commented Nov 4, 2024

Scripts can be pure and deterministic by default, so you can be sure that rerunning commands is either idempotent or fully reversible through jj undo

That sounds legitimately neat. But framed this way, there is really very little overlap between these two features and therefore even less reason to pit them against each other. A scripting language that only performs pure operations on the commit graph may have a bunch of benefits, but it is also extremely limited by definition in the context of use cases that are fundamentally impure. What if I want a command jj lfs-sync or something? That's not pure. But it fits into the workflow of users of a VCS. It's useful. Let's have both.

/path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine

I wanted to use my config and this tool on all my machines, and I use Windows too. Therefore, I have to either write the tool in a full blown language anyway, completely negating the fast scripting advantage, or write it twice, etc etc...

The fact that jj util exec doesn't single-handedly resolve all differences between unix and windows doesn't mean that it isn't useful or doesn't pull its weight in maintenance burden. A pure, jj-specific scripting language will not solve these problems either, so it's simply an irrelevant point.

The main use case for this feature is for individuals to streamline their own workflows. Few people use unix and windows regularly. Which doesn't mean we should dismiss their needs, but there are purpose-built tools to solve these problems. For example, I use chezmoi to manage my dotfiles, including my jj-config and therefore any such aliases. Chezmoi has a templating system, which I use for differences between workstations & servers, private & company machines. You could use it to paper over differences between unix and windows, like path syntax. I assume your jj-gerrit command is not the only piece of workflow automation you would like to share across your machines. In that case, jj-script won't solve your problem and you'll have to reach for another tool like chezmoi anyway (or just live with the inconvenience)

That's for individuals, what about teams working on a project? Chezmoi is not fit to solve that problem. But neither is a jj-specific scripting language. Every project has tools, scripts, automation and whatnot that go along with the development process. Here again, there are purpose-built tools to solve these problems. Some may specifically seek out platform-independent tools like just and nushell or python instead of make and bash. Some may tell their windows coworkers to use WSL. Some may stuff all their tooling in docker containers. Whatever a project ends up doing, jj util exec will be perfectly compatible with it.

My main point here is that "solving the problem of platform incompatibility" and "streamlining VCS workflows" are two orthogonal problems and jj util exec shouldn't be arbitrarily held to the standard of solving both of them.

[scripts] can for example be given no access to the network or any other number of features

That also means scripts can easily be checked into the source repository without fear that random things will get run without review. In the future, I also hope that a jj-script mechanism could work on the server side, and also inside your IDE.

"Randomly invoke thing out of $PATH" will never clear a security review for many cases and is a complete non-starter anyway in things like a server-side component.

Like purity, these secure-by-default features sound legitimiately useful. This definitely makes me more excited to see how the scripting endeavor develops.

But I don't think it has anything to do with jj util exec. Just don't let alisas be checked into version control like they already can't. That doesn't prevent pure scripts from being checked in VC in the future under a different system than aliases. These things have nothing to do with each other and I don't think they should be pitted against each other.

we will still be forcing people to learn yet-another-language instead of letting them use the tools they know and are good at.

I agree that by your description of a pure, secure scripting language we would have to make an opinionated choice anyway so we can precisely control the API. So I don't mind "forcing" a specific language in that case. I would mind if the scripting language ended up having all the capabilities of a regular shell language and I would be forced to use that specific one for no reason. But that doesn't seem to be the plan, so my original point is mute.


I'll repeat my main point, which isn't that we shouldn't do the scripting language. The scripting language sounds legitimately useful and I'm excited for it now. My main point is that there is no reason for these two features to be mutually exclusive. They are different features, serving different purposes, and they can live side-by-side without any issues.

@yuja
Copy link
Collaborator

yuja commented Nov 5, 2024

I'm not against adding jj util exec or something similar even if we're going to add a full-blown scripting feature at some point. It's simple and wouldn't conflict with any future alias syntax changes.

@thoughtpolice
Copy link
Collaborator

The scripting language sounds legitimately useful and I'm excited for it now

Just to be clear, my only point was to emphasize that a scripting solution can and does have conceptual benefits over just calling out to random scripts in $YOUR_LANGUAGE. I don't disagree that the util exec feature is complementary (though I stand by my point it sucks some air out of the room because it's deceptively simple but has some flaws, but there are worse things in life and it does the job.)

Actually, this approach of requiring a subcommand does fix at least one of my gripes about "just execute things from $PATH", which is that you don't really know what anything is. But this provides a way to do something like jj util exec my-thing and then set an alias in your user config that sets aliases.my-thing = ["util", "exec", "my-thing.exe"], which is 99% of what people want. In other words, jj isn't executing random commands out of thin air, it only does what you ask, and some aliases can make the syntax shorter. Which I think is an OK compromise.

Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

if we want to support jj util exec command

There's not much to support, the essence of this feature is a one-liner.

There's is more to support, beyond the technical implementation such as user expectations, which is the reason why I'm so split on the feature.

I don't see any benefit compared to this simple approach.

The most important one are the subtle downstream consequences. Scripts can be pure and deterministic by default, so you can be sure that rerunning commands is either idempotent or fully reversible through jj undo; many things that e.g. reword commits, check commit messages, or any number of things that are pure operations on the commit graph are great for this. Scripts can be used on every platform, because they don't need any configuration changes and can be portable by default; a subtle and non-obvious consequence of your example above is that /path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine.

Austin also really has a great point with this, and we may allow a checked in config file in the future (#3684) which will make this a major pitfall.

/path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine
I wanted to use my config and this tool on all my machines, and I use Windows too. Therefore, I have to either write the tool in a full blown language anyway, completely negating the fast scripting advantage, or write it twice, etc etc...

The fact that jj util exec doesn't single-handedly resolve all differences between unix and windows doesn't mean that it isn't useful or doesn't pull its weight in maintenance burden. [...]

The main use case for this feature is for individuals to streamline their own workflows. [...] For example, I use chezmoi to manage my dotfiles, including my jj-config and therefore any such aliases. Chezmoi has a templating system, which I use for differences between workstations & servers, private & company machines. You could use it to paper over differences between unix and windows, like path syntax. I assume your jj-gerrit command is not the only piece of workflow automation you would like to share across your machines. In that case, jj-script won't solve your problem and you'll have to reach for another tool like chezmoi anyway (or just live with the inconvenience)

That's for individuals, what about teams working on a project? Chezmoi is not fit to solve that problem. But neither is a jj-specific scripting language. Every project has tools, scripts, automation and whatnot that go along with the development process. Here again, there are purpose-built tools to solve these problems. Some may specifically seek out platform-independent tools like just and nushell or python instead of make and bash. Some may tell their windows coworkers to use WSL. Some may stuff all their tooling in docker containers. Whatever a project ends up doing, jj util exec will be perfectly compatible with it.

I disagree with your point here, as giving the capability to jj makes it implicitly supported which is something we cannot control. A general purpose scripting language built on the base Austin mentioned, lets us control the behavior which is desirable, as we can move users off of bad patterns and deprecate and remove bad APIs and assumptions. So it is definitely not a orthogonal feature for a potential future, which I'm once again weary of.

The scripting language sounds legitimately useful and I'm excited for it now

Just to be clear, my only point was to emphasize that a scripting solution can and does have conceptual benefits over just calling out to random scripts in $YOUR_LANGUAGE. I don't disagree that the util exec feature is complementary (though I stand by my point it sucks some air out of the room because it's deceptively simple but has some flaws, but there are worse things in life and it does the job.)

Otherwise I agree with both Austin and Yuya and will accept the feature with some caveats and a warning in the documentation. This was not my favorite decision 😐.

cli/src/commands/util/exec.rs Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
cli/src/commands/util/exec.rs Outdated Show resolved Hide resolved
@senekor
Copy link
Collaborator Author

senekor commented Nov 5, 2024

In case I came across as too pushy, my goal is not to get this PR merged before reaching concensus. Thank you for stating your concerns, I don't want to merge this before they are addressed.

[...] Scripts can be used on every platform, because they don't need any configuration changes and can be portable by default; a subtle and non-obvious consequence of your example above is that /path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine.

Austin also really has a great point with this, and we may allow a checked in config file in the future (#3684) which will make this a major pitfall.

I tried to address this with the following:

Just don't let alisas be checked into version control like they already can't. That doesn't prevent pure scripts from being checked in VC in the future under a different system than aliases.

Whatever we allow to be checked into VC in the future, it won't make sense to check in an entire jj config, right? Are we going to allow checking in user.email? Admittedly, even if that doesn't make sense, it's definitely a smaller problem than arbitrary code execution.

Either way, do you think it would be problematic to differentiate explicitly about what kind of configuration can and cannot be checked into VC?

Whatever a project ends up doing, jj util exec will be perfectly compatible with it.

I disagree with your point here, as giving the capability to jj makes it implicitly supported which is something we cannot control.

What do you think we would be implicitly supporting with this feature? In my mind, what we support would be a convenient syntax to shell out to the environment. What the environment does with that is not our concern. It could only turn into a problem for us if users are somehow confused about this, but I'm having a hard time imagining what would cause this confusing.

I guess this goes into your concern about user expectations. I haven't thought about the expectation of jj commands always being jj undo-able. Aren't there already commands that break this assumption? I can't undo jj git push. What about jj abandon -r '..' --ignore-immutable + jj util gc --expire now? I checked, and it doesn't actually nuke the op log. But judging from the docs, I expect it can and will do that in the future. jj util exec -- rm -rf .jj is just a little more reliable, and I can't imagine a user being surprised by it.

I'm happy to make it very clear in the documenatation that this is just a convenient way to run arbitrary code on your system (including all the danger) which may not even be related to jj.

A general purpose scripting language built on the base Austin mentioned, lets us control the behavior which is desirable, as we can move users off of bad patterns and deprecate and remove bad APIs and assumptions. So it is definitely not a orthogonal feature for a potential feature, which I'm once again weary of.

I still feel like this hypothetical scripting language we talk about changes its promised feature set depending on the context of the conversation. We need to be on the same page about what we're talking about.

a) A pure and secure scripting language. This has several potential benefits over shelling out. I hope this will become reality. However, this is orthogonal to shelling out, because it doesn't cover many use cases that shelling out does. (e.g. jj lfs-sync) I stand by my point that these two features would complement each other, they aren't mutually exclusive.

b) A general purpose scripting language that can do most if not all the things bash can do. Certainly, such a feature would make jj util exec redundant. But it has none of the benefits of a pure and secure scripting language while imposing a massive maintenance burden compared to shelling out as well as forcing users to learn and use a specific thing unnecessarily. If the language let's me make HTTP requests, I will run a server on localhost that shells out to whatever it finds in the query string. If the language let's me read from the file system, I will mount my own FUSE that calls any command encoded in a file path that my script attempts to read from. (Is that possible? Just came up with it. Have to check. (Edit: it works)) My point being, the secure part of the scripting language is extremely limiting in terms of functionality and use cases.

On a side note, are you also concerned about extension support for the same reasons? As I pointed out in this comment, the proposal is essentially a superset of jj util exec.


We can always remove jj util exec in the future. The worst that will happen is that we'll have to tell users to type jj-lfs-sync instead of jj lfs-sync. I don't think we'll need to do that, but it wouldn't be the end of the world.

@senekor senekor force-pushed the remo/util-exec branch 2 times, most recently from 0efe5e6 to d63a98c Compare November 5, 2024 23:00
@matts1
Copy link
Collaborator

matts1 commented Nov 5, 2024

As the person who filed #3673, I'd like to express my support for jj util exec as an alternative to my proposal. I do think that my proposal had issues with either not being expressive enough, or adding too much complexity into the DSL.

FWIW, I'm a big fan of the starlark choice. I'm not a huge fan of arbitrary binaries, especially given that if it's a compiled language, it'll be platform-specific.

a) A pure and secure scripting language. This has several potential benefits over shelling out. I hope this will become reality. However, this is orthogonal to shelling out, because it doesn't cover many use cases that shelling out does. (e.g. jj lfs-sync) I stand by my point that these two features would complement each other, they aren't mutually exclusive.

b) A general purpose scripting language that can do most if not all the things bash can do. Certainly, such a feature would make jj util exec redundant. But it has none of the benefits of a pure and secure scripting language while imposing a massive maintenance burden compared to shelling out as well as forcing users to learn and use a specific thing unnecessarily. If the language let's me make HTTP requests, I will run a server on localhost that shells out to whatever it finds in the query string. If the language let's me read from the file system, I will mount my own FUSE that calls any command encoded in a file path that my script attempts to read from. (Is that possible? Just came up with it. Have to check.) My point being, the secure part of the scripting language is extremely limiting in terms of functionality and use cases.

Personally, I'd be in favor of a solution where only support pure and secure scripting language, and similar to bazel's repository_ctx.execute, we provide some kind of mechanism to do those general purpose things. This would be roughly similar to rust's unsafe, where we could provide a namespace nonhermetic, and we know that if the user doesn't call nonhermetic.execute, or other functions in that namespace, then it should meet certain guarantees.

Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

Another round of minor nits.

cli/src/commands/util/exec.rs Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@senekor senekor force-pushed the remo/util-exec branch 2 times, most recently from 7bcf0cb to 08daf32 Compare November 8, 2024 18:38
@PhilipMetzger
Copy link
Collaborator

Sorry for the late response here. Now onto my thoughts:

[...] Scripts can be used on every platform, because they don't need any configuration changes and can be portable by default; a subtle and non-obvious consequence of your example above is that /path/to/foo on your Linux machine is called C:\\path\\to\\foo.exe on my machine.

Austin also really has a great point with this, and we may allow a checked in config file in the future (#3684) which will make this a major pitfall.

I tried to address this with the following:

Just don't let alisas be checked into version control like they already can't. That doesn't prevent pure scripts from being checked in VC in the future under a different system than aliases.

Whatever we allow to be checked into VC in the future, it won't make sense to check in an entire jj config, right?

There probably won't be a full config checked in, but a common set of aliases, which then leads to a chicken and egg problem if you have any alias depending on jj util exec without a checked in script.

Are we going to allow checking in user.email?

Depends on how you look at it. If we only want a common configuration then it could be disallowed but I assume some smart users will use the feature to configure a bot and its config.

Admittedly, even if that doesn't make sense, it's definitely a smaller problem than arbitrary code execution.

Yes, which jj util exec gracefully avoids.

Either way, do you think it would be problematic to differentiate explicitly about what kind of configuration can and cannot be checked into VC?

Any suggestion we make in the docs will be understood as a Guideline not something we actively prevent, so as soon as you have a user which ignores it, you're bound by Hyrum's Law. Preventing it in code will also be hard as long as there is no IFTT (If-This-Then-That) linter for it.

Whatever a project ends up doing, jj util exec will be perfectly compatible with it.

I disagree with your point here, as giving the capability to jj makes it implicitly supported which is something we cannot control.

What do you think we would be implicitly supporting with this feature?

Basically anything, as long as it runs. As it is "supported" now, a user will come and scream at us, when we remove their favorite plaything.

In my mind, what we support would be a convenient syntax to shell out to the environment. What the environment does with that is not our concern. It could only turn into a problem for us if users are somehow confused about this, but I'm having a hard time imagining what would cause this confusing.

This is not entirely wrong but does not represent the user expectation that jj util exec for a custom alias must continue to work until the end of eternity, which hits the issue of #3549 and its removal/supersedence with #4496 even though we advertised the feature as purely experimental (relevant Discord Thread).

[..] I haven't thought about the expectation of jj commands always being jj undo-able. Aren't there already commands that break this assumption? I can't undo jj git push. What about jj abandon -r '..' --ignore-immutable + jj util gc --expire now?

Currently the Git backend/store and related functionality break that currently. In the future with a native store and server for it, we should strive for making every comand/operation cleanly revertible.

I checked, and it doesn't actually nuke the op log.

The op log is a separate thing, see #3700 (comment) for a nice explanation of it. And as you noted at some point util gc may garbage collect it too.

A general purpose scripting language built on the base Austin mentioned, lets us control the behavior which is desirable, as we can move users off of bad patterns and deprecate and remove bad APIs and assumptions. So it is definitely not a orthogonal feature for a potential feature, which I'm once again weary of.

I still feel like this hypothetical scripting language we talk about changes its promised feature set depending on the context of the conversation. We need to be on the same page about what we're talking about.

a) A pure and secure scripting language. This has several potential benefits over shelling out. I hope this will become reality. However, this is orthogonal to shelling out, because it doesn't cover many use cases that shelling out does. (e.g. jj lfs-sync) I stand by my point that these two features would complement each other, they aren't mutually exclusive.

b) A general purpose scripting language that can do most if not all the things bash can do. Certainly, such a feature would make jj util exec redundant. But it has none of the benefits of a pure and secure scripting language while imposing a massive maintenance burden compared to shelling out as well as forcing users to learn and use a specific thing unnecessarily. If the language let's me make HTTP requests, I will run a server on localhost that shells out to whatever it finds in the query string. If the language let's me read from the file system, I will mount my own FUSE that calls any command encoded in a file path that my script attempts to read from. (Is that possible? Just came up with it. Have to check. (Edit: it works)) My point being, the secure part of the scripting language is extremely limiting in terms of functionality and use cases.

Sorry for badly expressing myself here, I just meant that more "generally purpose" functionality can be built on top of the Starlark language for Revsets which we then could use to run something like a CI on the server. Otherwise I agree with Matt's point and your definitions.

On a side note, are you also concerned about extension support for the same reasons?

Yes, although they only operate in a universe where we call the shots (as we define the extension points).

@senekor
Copy link
Collaborator Author

senekor commented Nov 8, 2024

Whatever we allow to be checked into VC in the future, it won't make sense to check in an entire jj config, right?

There probably won't be a full config checked in, but a common set of aliases, which then leads to a chicken and egg problem if you have any alias depending on jj util exec without a checked in script.

I was thinking along the lines of the [aliases] section in the config being completely forbidden from being checked in, whereas a new section [scripts] could be checked in, because it is safe. I'm assuming the scripting language would be at least as powerful as current aliases, so there would be no need to check in the [aliases] section.

(We'd have to make a decision abouot precedence if there is an alias and a script with the same name, but that doesn't seem like too big of an issue.)

Either way, do you think it would be problematic to differentiate explicitly about what kind of configuration can and cannot be checked into VC?

Any suggestion we make in the docs will be understood as a Guideline not something we actively prevent, so as soon as you have a user which ignores it, you're bound by Hyrum's Law. Preventing it in code will also be hard as long as there is no IFTT (If-This-Then-That) linter for it.

I agree that documentation is not good enough, I was thinking about preventing it in code. What is difficult about this approach? If the config is checked into VCS and it contains a [aliases] section, ignore it and emit a warning or even hard error.

What do you think we would be implicitly supporting with this feature?

Basically anything, as long as it runs. As it is "supported" now, a user will come and scream at us, when we remove their favorite plaything.

the user expectation that jj util exec for a custom alias must continue to work until the end of eternity

I don't think we have to worry about that. As mentioned, transitioning from jj my-script to jj-my-script is a pretty mild breaking change.

On a side note, are you also concerned about extension support for the same reasons?

Yes, although they only operate in a universe where we call the shots (as we define the extension points).

What do you mean by "we call the shots"? If the method of invoking these extensions is shelling out to them, they are going to have all the capabilities of a bash script plus any extension points we provide.

@PhilipMetzger
Copy link
Collaborator

Whatever we allow to be checked into VC in the future, it won't make sense to check in an entire jj config, right?

There probably won't be a full config checked in, but a common set of aliases, which then leads to a chicken and egg problem if you have any alias depending on jj util exec without a checked in script.

I was thinking along the lines of the [aliases] section in the config being completely forbidden from being checked in, whereas a new section [scripts] could be checked in, because it is safe. I'm assuming the scripting language would be at least as powerful as current aliases, so there would be no need to check in the [aliases] section.

That definitely could work but at this point we're already far into a hypothetical bikeshed so I'd rather drop it and move on.

Either way, do you think it would be problematic to differentiate explicitly about what kind of configuration can and cannot be checked into VC?

Any suggestion we make in the docs will be understood as a Guideline not something we actively prevent, so as soon as you have a user which ignores it, you're bound by Hyrum's Law. Preventing it in code will also be hard as long as there is no IFTT (If-This-Then-That) linter for it.

I agree that documentation is not good enough, I was thinking about preventing it in code. What is difficult about this approach?

You need to catch it at Codereview time, which isn't easy if you don't have a good infra for it. This also needs to encompass renames and more, with the thought in mind that we'll always add more config flags to jj keeping in mind that we're at the start of the project. So it's a social rather than technical issue.

What do you think we would be implicitly supporting with this feature?

Basically anything, as long as it runs. As it is "supported" now, a user will come and scream at us, when we remove their favorite plaything.

the user expectation that jj util exec for a custom alias must continue to work until the end of eternity

I don't think we have to worry about that. As mentioned, transitioning from jj my-script to jj-my-script is a pretty mild breaking change.

I still respectfully disagree and even gave you a example.

On a side note, are you also concerned about extension support for the same reasons?

Yes, although they only operate in a universe where we call the shots (as we define the extension points).

What do you mean by "we call the shots"? If the method of invoking these extensions is shelling out to them, they are going to have all the capabilities of a bash script plus any extension points we provide.

See #3869 (visit the Design Doc) and #3577 for the long discussions around the actual suggested feature here, where I already spent many hours discussing a agreeable design . I do not have the capacity to give you a TL;DR right now, as it's just to much content.

At this point I'm ready to drop my concerns, as we discussed a potential future and migration plan for the feature, its major drawbacks and warned users about it, which is a major step-up from other existing systems. And since it solves a bunch of user problems we should accept it on the ground of "perfect is the enemy of good" while a better design is in the works.

And on a side-note, as I forgot that part in my last reply:

In case I came across as too pushy, my goal is not to get this PR merged before reaching concensus. Thank you for stating your concerns, I don't want to merge this before they are addressed.

While it was a bit pushy, the discussion never got heated which I really value after having a bunch of heated discussions in other issues or with other users.

And feel free to solve conversations after you addressed them.

cli/src/commands/util/exec.rs Outdated Show resolved Hide resolved
cli/src/commands/util/exec.rs Outdated Show resolved Hide resolved
cli/tests/test_util_command.rs Outdated Show resolved Hide resolved
@senekor senekor force-pushed the remo/util-exec branch 2 times, most recently from edf94d9 to 551a34b Compare November 9, 2024 10:12
@senekor senekor merged commit db2b589 into martinvonz:main Nov 9, 2024
18 checks passed
@senekor senekor deleted the remo/util-exec branch November 9, 2024 10:50
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: Support shell command aliases FR: run jj-* commands in PATH
6 participants