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

FR: run jj-* commands in PATH #3001

Closed
jyn514 opened this issue Feb 9, 2024 · 20 comments · Fixed by #4759
Closed

FR: run jj-* commands in PATH #3001

jyn514 opened this issue Feb 9, 2024 · 20 comments · Fixed by #4759
Labels
enhancement New feature or request

Comments

@jyn514
Copy link
Contributor

jyn514 commented Feb 9, 2024

Is your feature request related to a problem? Please describe.
i want to add an alias that involves multiple jj commands (specifically jj branch set main -r @- && jj git push). currently, it's not possible to run multiple commands in aliases, because there's no equivalent of git's ! feature to run things in the shell.

Describe the solution you'd like
that's ok, i'm not asking for it to be changed, but it would be nice if instead jj push-head ran jj-push-head in PATH, so i don't have to remember which commands are aliases and which are shell helpers.

Describe alternatives you've considered
add support for shell execution in aliases

Additional context
jj 0.13.0-81b0e3bf3bf6f230740e93532b9a9767e8a4ef6e

@jyn514 jyn514 self-assigned this Feb 9, 2024
@jyn514
Copy link
Contributor Author

jyn514 commented Feb 9, 2024

err actually i'm going to wait to work on this until i hear back that it's a desired feature

@jyn514 jyn514 removed their assignment Feb 9, 2024
@ilyagr
Copy link
Contributor

ilyagr commented Feb 10, 2024

My solution to this is that I put various scripts called jj.tool, jj.watch, jj.logstatus in ~/.local/bin.

I think this is still a desired feature? I am not actually sure. I don't see an obvious need for it for myself, but it seems expected, and I don't see any harm in it.

@martinvonz
Copy link
Member

We should probably ask the Git folks what they think. They would know if it's caused the Git project any problems. They can probably also help point out any non-obvious benefits.

@yuja
Copy link
Contributor

yuja commented Feb 10, 2024

I slightly prefer shell aliases than jj-<subcommand> lookup in PATH, but I don't know why modern tools like cargo do that.

fwiw, shell aliases could be faked if we had jj util exec/sh-c [ARGS] subcommand.

@ilyagr
Copy link
Contributor

ilyagr commented Feb 10, 2024

fwiw, shell aliases could be faked if we had jj util exec/sh-c [ARGS] subcommand.

What do you mean by "shell aliases could be faked"? By jj util exec, do you mean executing a command in the repo root as opposed to anywhere?

@yuja
Copy link
Contributor

yuja commented Feb 10, 2024

What do you mean by "shell aliases could be faked"? By jj util exec, do you mean executing a command in the repo root as opposed to anywhere?

Yes, just exec or shell out with some environment variables set, and the alias will be something like

[aliases]
shell-alias = ["util", "exec", "sh", "-c", "$JJ ..."]

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 10, 2024

I slightly prefer shell aliases than jj-<subcommand> lookup in PATH, but I don't know why modern tools like cargo do that.

shell aliases can't use spaces, so like @ilyagr found the commands have to be named with some other separator like periods or underscores. that's what i meant by "i don't have to remember which commands are aliases and which are shell helpers".

Yes, just exec or shell out with some environment variables set

what benefit does that have over running things in path? it seems more annoying to setup because of the boilerplate.

We should probably ask the Git folks what they think. They would know if it's caused the Git project any problems. They can probably also help point out any non-obvious benefits.

👍

@yuja
Copy link
Contributor

yuja commented Feb 10, 2024

what benefit does that have over running things in path? it seems more annoying to setup because of the boilerplate.

  • alias commands can be inlined in config file
  • jj[TAB] won't list those uninteresting commands

The boilerplate is yeah annoying. It's just one way to fake shell aliases. We might want better syntax like ! in git.

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 10, 2024

jj[TAB] won't list those uninteresting commands

well, they are interesting to me, or i wouldn't write them. if i didn't want jj <TAB> to complete them i would make them shell functions instead of aliases.

oh, i suppose that's an argument in favor of making it an alias so jj util completion doesn't have to search PATH 🤔

@yuja
Copy link
Contributor

yuja commented Feb 10, 2024

I mean jj<TAB> (not jj <TAB>). It's a bit annoying that git<TAB> doesn't insert space because there are many git- extension commands which are supposed to be executed as git <subcommand>.

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Feb 10, 2024
@epage
Copy link

epage commented Feb 11, 2024

If going down the route of ! support, it might be worth looking into an embeddable shell so people can get some basic cross-platform support. A more basic option is deno_task_shell and duckscript for a more advanced option. I've also been tempted to write an embeddable version of nushell.

I think both plugin commands and ! are useful. ! is great for very lightweight needs with plugins for more complex or more easily shared needs.

No idea why but for cargo, the plugins are invoked as cargo-<plugin> <plugin>. I guess this can help if you want to have a multiple-role binary like busybox. It does cause a lot of confusion and makes it more difficult to run outside of being a plugin.

I can see the auto-completion pain point. I wonder if the way to handle that is by having to register the plugins by defining a ! alias. We could define a non-PATH location for people to put their plugins that we expose as an environment variable to the ! shell.

@PhilipMetzger
Copy link
Contributor

I dislike randomly running stuff in PATH and find Yuya's proposed solution better. Mostly because running stuff prefixed with <cli-tool>- is a neat hack and a very non-portable way to customize the binary which jj already allows.

If we can come up with a good design for plugins, maybe it'd support it, but this isn't something we're discussing right now.

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 14, 2024

another use case for running multiple jj commands at once: until #170 gets implemented i have a jj-absorb() { ... } shell alias in .profile, it would be nice to spell that as jj absorb (whether that's with a jj util exec alias or just running things in PATH).

@nasamuffin
Copy link
Contributor

We should probably ask the Git folks what they think. They would know if it's caused the Git project any problems. They can probably also help point out any non-obvious benefits.

Howdy, Git folk reporting :)

I don't think it's caused any problems. The biggest potential one I can think of is namespace collision - what happens to users who have their own custom git-amend on $PATH when we decide to implement a first party git amend? But in that case we either add it to the C monolith, which means the user's git-amend never has an opportunity to be found, or we add it to the git exec dir alongside scripts like git-submodule and git-send-email, which also gets searched before $PATH does.

Now, I do think there's room for improvement if you do want to let people provide a jj-foo in $PATH - for example, you could provide them some pertinent info about the repo via envvars if you wanted, or you could check for collisions and log some warning (rather than silently ignoring their thing and running your thing). But overall it's been a pretty nice accidental-feature for us, IMO.

@jabolopes
Copy link

The jj absorb feature should probably be a first class feature in the jj library so that it can be used in other environments beyond the CLI. If this is implemented as a separate program or script then it can't be used in Web based IDEs, since those typically don't have a local file system and can't shell out.

Also arbitrary program execution won't pass a security review for a server deployment, so this feature would have to be disabled when the jj lib is deployed on a server.

So the initial use case for jj absorb is probably not the most compelling justification for this feature request, in my opinion.

In general, aliases and functions and PATH discovery really are the responsibility of the shell and not the responsibility of a version control system, in my view, so this feature would be out of place.

Also this being an accidental feature in Git ends up not offering a compelling reason in terms of preexisting art.

In summary, I think there's little compelling reasons to copy this feature from Git into jj.

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 28, 2024

In general, aliases and functions and PATH discovery really are the responsibility of the shell and not the responsibility of a version control system, in my view, so this feature would be out of place.

can you give a concrete example of how to make jj <TAB> work with aliases with only support from the shell?

@jabolopes
Copy link

I think that will depend on the shell. For example, for Bash, I'd look into the Bash completion files / bash_completion.d directory.

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 28, 2024

jj natively supports bash completions with jj util completion. are you suggesting completely scrapping that and writing my own completions from scratch?

@jyn514
Copy link
Contributor Author

jyn514 commented Feb 28, 2024

i'm having a really hard time understanding why you think "single command" aliases should be natively supported but "multi-command" aliases shouldn't.

@senekor
Copy link
Contributor

senekor commented Nov 3, 2024

By clicking through all the interlinked issues and discussions, there are a ton of ideas with various degrees of overlap. Many of them are very powerful and hard to implement, like extensions that communicate with a grpc server.

But Yuya's idea of jj util exec seems like a very simple, immediate win without any downsides.

For the originally requested feature, it adds a trivial line of boilerplate to the config. But anyone who is configuring their CLI tools to this degree probably has a dotfiles repo anyway, so not an issue. It also has distinct benefits:

  • can be inlined in the config, but doesn't have to
  • can name the script whatever you want
  • jj<TAB> will add the space, unlike git<TAB>
  • adding dynamic shell completions for these aliases won't require searching everything on the $PATH (I'm currently laying the groundwork for that here)

it might be worth looking into an embeddable shell so people can get some basic cross-platform support

Sounds cool, but I don't think this should stop us from getting the immediate win of Yuya's idea. If a team can already agree on using jj, they can also agree on installing a cross-platform shell. We (sadly) don't live in a world where everyone is already using jj and expects to be able to clone an open-source repo and use the project-specific jj aliases without having to install any additional tools.

The primary use case here is for individuals to streamline their own workflows. This is perfectly solved with Yuya's idea. Stuff your entire workflow in a simple bash script and add a single line to your jj config.

In general, aliases and functions and PATH discovery really are the responsibility of the shell and not the responsibility of a version control system, in my view, so this feature would be out of place.

This is dismissing the previous discussion. The shell cannot redirect jj my-script to jj-my-script, it must call jj. That's why jj already has support for aliases - they're useful.

Also arbitrary program execution won't pass a security review for a server deployment, so this feature would have to be disabled when the jj lib is deployed on a server.

I'm not a security expert, but if a malicious actor is messing with your config files and putting random binaries in your $PATH, you have already been compromised. Going through a jj alias doesn't give the attacker any additional priviledges that they don't already have at that point(?).


This is very easy to implement, PR incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants