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

Use file/git libraries instead of always shelling out #74

Open
nickserv opened this issue Jan 6, 2014 · 12 comments
Open

Use file/git libraries instead of always shelling out #74

nickserv opened this issue Jan 6, 2014 · 12 comments

Comments

@nickserv
Copy link
Collaborator

nickserv commented Jan 6, 2014

Advantages

  • Shelling out takes a while, and you can get major performance and stability boosts with the right libraries.
  • We could save time writing ruby wrappers around command line functionality by using an existing library.
  • The code would be a bit less bloated with generalized file/git/etc. code placed somewhere else.
  • Many Ruby libraries give comfortable object-oriented interfaces to existing command line functionality.

File stuff

Ruby has a lot of awesome command-line-inspired libraries built in, including FileUtils, which could easily replace many calls to commands like mv, cp, rm, and ln.

Git stuff

Grit looks pretty awesome, as it's what GitHub uses internally. See Supercharged Ruby-Git.
Grit is deprecated, and its readme recommends switching to Rugged.

@JCook21
Copy link
Collaborator

JCook21 commented Jan 6, 2014

👍

@trobrock
Copy link
Collaborator

trobrock commented Jan 6, 2014

This should also make things easier to test 💯

@technicalpickles
Copy link
Owner

major performance and stability boosts

This is all relative. homesick in reality doesn't do so much work, so it doesn't really need to be super charged.

It's going to depend on a lot on what functions need to be done. When I first wrote jeweler, grit was mostly read-only access. I used ruby-git for a lot of things, but then still had to shell out.

Ruby has a lot of awesome command-line-inspired libraries built in, including FileUtils, which could easily replace many calls to commands like mv, cp, rm, and ln.

For what it's worth, these are calls still needing to be stubbed out in test. Current code uses system, which is just as stubbable.


Not saying "no", just saying "yes, but..." 😁

@nickserv
Copy link
Collaborator Author

nickserv commented Jan 6, 2014

@technicalpickles You have a good point about still needing to stub, but I don't think it would be much of an issue. And I'm not too familiar with how stubs work especially with RSpec, but I feel like there might be nicer stubs built into things like grit. I could be wrong though.

@technicalpickles
Copy link
Owner

I feel like there might be nicer stubs built into things like grit.

This isn't a good assumption. Depending on how it's written, it might actually be more complex.

The best bet is probably to get a PR going to swap out system calls for grit or something else, review the implementation from there.

@nickserv
Copy link
Collaborator Author

nickserv commented Jan 9, 2014

The best bet is probably to get a PR going to swap out system calls for grit or something else, review the implementation from there.

If you want, I could try this after I'm done with some of the other refactoring I've been working on

@technicalpickles
Copy link
Owner

@thenickperson sure. I would suggest keeping the changes tightly focused around that, to make reviewing the diff easier.

@nickserv
Copy link
Collaborator Author

I'm pretty much done using libraries for the non-git system calls. That part is easy thanks to FileUtils. However, the git part is a bit trickier. I have managed to get some of the system calls switched over to Grit, which seems to be working nicely for replacing some of the system calls.

However, a lot of our tests expect certain output to result from git-related commands, because homesick can use part of git's command line interface for convenience. This means that homesick's output depends on the git shell, but libraries like Grit aren't meant to reimplement git's porcelain command output.

I'm thinking of three options for dealing with this:

  1. Switch to a git library entirely, removing all git-related system calls. This is probably a bad idea, since we would either have to show less output for git-tasks, or reimplement part of git's UI ourselves.
  2. Use a git library for commands that don't show output to the user.
  3. Keep shelling out for all git-related functionality. This may or may not be more efficient than option 2, depending on how long it takes to load libraries and if they would be slower or faster than shelling out. Another thing to consider is that some Ruby git libraries still shell out to git. I'm starting to feel like this would be our best option.

@JCook21
Copy link
Collaborator

JCook21 commented Jan 17, 2014

I think I'm responsible for a lot of the tests that check the text that shell commands return. I don't particularly like this as I think it makes for brittle tests but it was the simplest way of adding tests at the time. If you do replace the system calls I'd be in favour of mocking and stubbing the library calls wherever possible and I'd be happy to help with this.

@nickserv
Copy link
Collaborator Author

The problem is that not only would we have to change the expected test output, but we would probably have to remove it entirely. As far as I know, libraries like Grit don't show you any command line output for git commands. This means that we would either have to remove that from homesick, or reimplement large portions of git's command line output.

Thanks for offering your help, but I'm feeling like it won't be worth the trouble to replace our system calls to git. So far, I have only found two system calls that we use for git that can easily be swapped out to Grit without breaking anything (including command line output). We rely on the UI too heavily for it to be an easy transition. Let me know what you think, though.

@nickserv
Copy link
Collaborator Author

nickserv commented Apr 4, 2014

Now that Grit is deprecated in favor of Rugged, I just created a new use-rugged branch using Rugged to replace some shell calls. This new branch is based off of my older use-grit branch, though it's only as complete as my use-grit branch was previously (very incomplete).

While I'm starting to prefer Rugged's interface (it can discover repositories from subdirectories, which is pretty cool), I'm still having having the same problem with it being a Ruby client for git that does not display output on the command line by default (as supposed to shelling out to git).

If we're going to use a git wrapper library fully or partially, I think we should definitely use Rugged over Grit. However, we still have to deal with the UI problems I mentioned above.

CC @christianbundy @JCook21

@nickserv
Copy link
Collaborator Author

nickserv commented Apr 5, 2014

I have created a table of my current status on use-rugged.

Unfortunately, there are several important git features that homesick currently uses that Rugged does not support (such as committing, pulling, full status display, and submodules). There are places where Rugged could be used to replace shell calls, but not without losing git's useful command line output. Output for some git commands would have to be rewritten by us in order to fully switch to Rugged.

I think that if we wanted to remove our git dependency entirely, it would be best to use Rugged for some git features and require the user to use homesick exec for other functionality like pushing, pulling, committing, and status viewing. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants