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

Avoid creating a new shell instance when using wt #15

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

Conversation

mateusauler
Copy link

@mateusauler mateusauler commented Apr 24, 2024

This PR creates shell functions that invoke wt and cd into the given directory. This partially solves #11.

What changed:

  • When running wt and giving it a valid worktree name, it echo's out changedir:<path>.
  • Adds the init sub-command for generating the shell code of the function.
  • The installation now requires the addition of a line to the shell config that invokes wt and runs the generated code. This is reflected in the readme.

I believe some further changes would be nice, but might be outside the scope of this PR:

  • Add a -- prefix for the sub-commands, to avoid collisions with worktree names.
  • The new init operation could also generate the completion scripts to reduce installation complexity. (This could be implemented in this PR)
  • Shell completions could include the sub-commands.

Some notes:

  • This introduces breaking changes, because the installation changed.
  • Instead of the shell function invoking wt, the whole script could be inside the shell function, thus not needing the ugly echoing.
  • The error messages are not the best...
  • Maybe the whole thing should be refactored :)

@mateusauler mateusauler changed the title Avoid creating a new shell when using wt Avoid creating a new shell instance when using wt Apr 24, 2024
@mateusauler mateusauler marked this pull request as ready for review May 1, 2024 22:43
Also use it inside the generated init functions.
@jiriks74
Copy link

jiriks74 commented Nov 12, 2024

Hello.
I'm looking into reworking this a bit and I have a good enough solution with minimal setup needed from the end user.

Doing . wt instead of wt runs the script in the current shell rather than creating a new subshell and running it there. This solves the issue of nested shells and not changing the directory it you avoid creating a new one in the script.

All of this would only need alias wt=". wt" from the end user to make it work.

I'll be working on this today, and comparing it with your work, but I'd like to keep this as simple as possible. I'd like to package this so I'd like the setup needed to be minimal.

I'll be incorporating some changes you mage (like #16).

@jiriks74
Copy link

Well I'll be packaging your fork as simple modifications probably won't do this well enough for publishing. Thanks for your work!

@mateusauler
Copy link
Author

Awesome stuff! Also, thank you! As you saw, I have developed my fork a bit. Feel free to open a PR there!

@jiriks74
Copy link

My changes aren't something you'd want in your package.

I ripped out the update functionality as wt is read only in nix and you don't have the necessary permissions to overwrite it in others. I reduced the functionality to "update available".
I also ripped out the auto update stuff as since you're getting the update through a package manager, you don't want to see this for 3 days until the update is available.

The last modification I did, which I may open a PR for, is that the list option was removed and there's names instead. I'd go back to list as that's what git worktree list so I find it more intuitive both for me and new users.

@mateusauler
Copy link
Author

I have also packaged it for nix :D (sort of) in my config. It also removes the update functionality with a patch. I think it might be best to not handle updates and have it distributed via a package manager.
I also created a names subcommand and removed list, since it felt redundant. Here ced661e, and here 7423c83.
I think the . wt might be a better solution to not spawning a subshell. Does it work in shells other than bash?

@jiriks74
Copy link

jiriks74 commented Nov 13, 2024

I also created a names subcommand and removed list, since it felt redundant.

I saw that but IMHO the better to change the list, rather than create names with basically the same function and then delete the list. The full paths don't really have a use case and if you need them just do git worktree list. As this is a wrapper I'd use this to simplify the out put of git worktree list (as names does) but keep the name. And list is, again IMHO, more intuitive as you want to list your worktrees.

If you're interested here's my PR to nixpkgs.

@mateusauler
Copy link
Author

Fair enough, maybe we should change names back to list.

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