Check for git before all relevant homesick commands

[christianbundy]

To close #25 I've added a small method to run before git-related homesick commands, which was originally suggested by @technicalpickles.

I was originally going to insert this into the git-specific functions (git_clone, git_diff, git status, etc), but it would be more expensive in scenarios that have multiple git commands, as well as scenarios where git isn't called until the end of the function (which could have all sorts of side effects).

[technicalpickles]

It'd be better to raise an exception here, and catch it elsewhere to exit. That way, this would be unit-testable without causing the test runner to exit 😓

[christianbundy]

Thanks, I'll do that! Just for future code, do you prefer multiple commits in a pull-request or everything squashed into a feature commit?

[technicalpickles]

Multiple commits is fine to me. Makes it easier to track changes over time, and also doesn't require a force push nor new PR.

[peterfpeterson]

The "proper" way to do this is to use "command -v #{command}" which returns zero if the "command" exists, and one if it doesn't. You can learn more about it at this stackoverflow question.

[muratayusuke]

I think the problem is that system function does not show standard error. How do you think about handling standard error in all command execution like raising exception and showing standard error message?

[technicalpickles]

@muratayusuke you can always add 2>&1 to get the stderr on stdout for the call. which doesn't really have any useful stdout, so it's hard to check the output.

Actually, a better approach would be checking the exit code, and %x{} so the stdout doesn't display by default, like:

%x{which #{command}}
installed = $?.success?

[JCook21]

I'd love to see this get implemented. Is there a way that we could do the Git check in Homesick's constructor? That way we could raise an exception straight away if git is not installed. This seems to be a good way to go to me since Git is pretty central to Homesick's operation. Any thoughts?

[nickserv]

@JCook21 That sounds like a pretty good idea to me. Even if there are a small number of homesick commands that wouldn't require git, the tool as a whole still won't be very useful without it.

Checking for git should be as simple as calling a method at the start of Homesick#initialize and raising an exception if git isn't found.

[peterfpeterson]

Can you also get the version of git if it is installed? The version on RHEL6 doesn't support all of the git commands you use and I can specialize them based on which git is found/used.

[JCook21]

What do you think the behaviour should be if not all git commands are
supported? I was originally thinking of throwing an exception and exiting
but do you think we should detect the version and print out a warning if
the version is lower than the needed one?
On 2 Apr 2014 09:13, "Pete Peterson" [email protected] wrote:

Can you also get the version of git if it is installed? The version on
RHEL6 doesn't support all of the git commands you use and I can specialize
them based on which git is found/used.

Reply to this email directly or view it on GitHubhttps://github.com//pull/61#issuecomment-39327985
.

[peterfpeterson]

The main one I ran into was --config is not supported on my system during the initial castle cloning. I was just going to track down the equivalent command and switch to it if the git was not new enough. No need to warn anybody. My current workaround is to clone it with git myself (into the right directory) and then homesick does everything else "right".

[JCook21]

@peterfpeterson how about if we store a constant for the minimum recommended Git version? Then if Git is not installed we bale straight away with an error and if Git is below the minimum version we output a warning. Does that sound like a good approach?

[nickserv]

I feel like if multiple version of git would offer different levels of support, it would be better to check for the existence of individual git commands where appropriate instead of doing a global git version check in homesick's constructor. That way, users could still access a majority of homesick's features if they have an older version of git that supports most homesick commands. It might be a little less efficient, but we could help this by caching information about existing git commands during individual homesick runs.

[peterfpeterson]

All commands that need to know the version (my experience is only git_clone) will need to have the same information. In keeping with DRY it would be nice to have it as a single method in the homesick class that those commands can use and others can ignore.

[JCook21]

@nicolasmccurdy the issue with checking for individual commands wherever necessary is that it gets really messy really quickly. I'd rather check for a version of Git in the constructor and give the user a warning that their version of Git may not give full functionality. What does anyone else think about this? Perhaps this is another argument for extracting all Git functionality out into a helper library?

@peterfpeterson I think we're more or less in agreement about keeping things as DRY as possible.

[christianbundy]

What about using something like rugged? This keeps things DRY and removes the native git dependency.

[peterfpeterson]

I was just thinking a method with the body

version = `git version`
version = version[/\d+\.\d+(\.\d+)+/]

which is a modified version of code found near line 715 of this file in ruby-git. Plain system calls is probably good enough.

[JCook21]

I'd like to explore the option of using something like rugged some more. I think it could help clear up responsibilities in the app as a whole by separating concerns. I know we've had this discussion before but I'd be interested to hear arguments for and against.

[nickserv]

@christianbundy @JCook21 Rugged and Grit are useful, though I ran into an issue with them that I discussed earlier on a different issue:

... 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.

[JCook21]

I've also had a go at a simple implementation of a Git check in #127 since this seems to be dead. I'd appreciate any comments.

[JCook21]

I'm going to close this since I think #127 takes care of this.