simplify git completion to use system-provided completions only

[cornfeedhobo]

This stops embedding a copied work, and it was broken anyways. I've only tested these paths on two distros, so suggestions are welcome!

Partially addresses #512

[nwinkler]

Thanks for providing this, @cornfeedhobo! I'm all for removing old copy/paste code...

The question I have is how does this compare to the existing system completion that we already have: https://github.com/Bash-it/bash-it/blob/master/completion/available/system.completion.bash

The system completion checks some known locations for the presence of the bash-completion package, e.g. /etc/bash_completion and then loads all completions found there. Most Linux packages install these completion files there by default, including the git OS package.

The system completion also checks for Homebrew installs on macOS and sources the respective file.

Instead of building individual completions that mimic what the system completion is doing, I'd rather have us work on improving the system completion to make sure it works in most cases.

Thoughts?

[cornfeedhobo]

@nwinkler I did not know about system. Seems better. Sounds like there are a number of completions that can/should be removed? Is there a way to tell users about the deprecation and point them to the system completion?

Edit: After testing with MacOS, I see the biggest difference: system assumes git was installed with brew. So folks using xcode's git are missed, and this completion includes those locations. With this in mind, what do you think about removing the paths that overlap with system and leave the rest?

[nwinkler]

@cornfeedhobo Thanks - good points!

• I thought about adding a note regarding the system completion to the Readme file, pointing people to system in favor of the individual completions. We would probably need instructions for installing the system completion OS package (e.g. Homebrew).
• Let's go with your suggestions - could you please remove the overlapping paths that conflict with system completion, leave the ones in for Xcode. Maybe add a note at the top of the file to point to the system completion as well...

[cornfeedhobo]

@nwinkler Hey, I was just writing out my comment for the next commit, and it has me thinking; There might be more edge cases that come up, and a sprawling list of completion plugins that source system distributed files is probably not great. Then I got to looking at adding that to system, and that led me to a bug in projects. I'm going to submit a series of PRs and issues to unpack it all, but I'll try to not drag this out.

[cornfeedhobo]

@nwinkler Please take another look. I think this better captures what we've learned and discussed. I'll clean up my other PRs after this is merged.

Edit: gimme a sec. gonna fix this broken test :(

[nwinkler]

Left a review comment...

[nwinkler]

For the broken test cases, it looks like you need to include the new log.bash file in a couple of test cases (e.g. search, I think).

[cornfeedhobo]

I can't reproduce the failures locally and I don't see how you tracked this down to inclusion from the travis log

[nwinkler]

I can see some errors related to the new log functionality here: https://travis-ci.com/github/Bash-it/bash-it/jobs/368158047#L283

I've tried to fix them in #1647 - let's see if that helps. I'll wait for the tests in #1647 to complete - if that fixes the error messages, you can try to rebase your branch on that.

[nwinkler]

@cornfeedhobo #1647 has been merged, it fixed some error messages during the test run. Can you rebase your branch on top of this and see if it fixes the issue with this PR's build jobs?

[cornfeedhobo]

No dice. I can reproduce the exit code locally, but I don't see any actual failures. I can try to pry in later.

[nwinkler]

OK, this is a tricky one... I think I found the issue, will open a PR in a moment. Here's what I did:

Noticed that one of the test runs on Travis shows an error here: https://travis-ci.com/github/Bash-it/bash-it/jobs/368318142#L240

/Users/travis/build/Bash-it/bash-it/test/plugins/../../completion/available/git.completion.bash: line 7: _command_exists: command not found
241/Users/travis/build/Bash-it/bash-it/test/plugins/../../completion/available/git.completion.bash: line 7: _command_exists: command not found

This seems to be related to the recent changes in this PR.

To find out what's causing this, I ran the tests locally - to make sure that one test is executed at a time (not in parallel), I set the test execution threads to 1:

TEST_JOBS=1 test/run 

I could see the same error in this case, and the final line read:

220 tests, 0 failures, 2 not run

Those two error messages are causing two tests to not run, now to find out which these are. I noticed that the errors show before the base.plugin.bats test cases. In the test/plugin folder, there's the alias-completion.plugin.bats before base.plugin.bats. I tried to run that:

> TEST_JOBS=1 test/run test/plugins/alias-completion.plugin.bats 
/Users/nils.winkler/.bash_it/test/plugins/../../completion/available/git.completion.bash: line 7: _command_exists: command not found
/Users/nils.winkler/.bash_it/test/plugins/../../completion/available/git.completion.bash: line 7: _command_exists: command not found

2 tests, 0 failures, 2 not run

There it is...

Will open a PR to fix this in a second.

[nwinkler]

Merged #1648, can you please rebase your branch on top of that? That should hopefully do it - I did not see any other test errors so far.

[cornfeedhobo]

@nwinkler I don't know how you got bats to show you those errors, but they are still happening. when I run that same command, the output is seemingly swallowed and i just see the test skips

[nwinkler]

Interesting! If you take a look here, you will see that the macOS tests ran fine, it's the Linux ones that are failing now.

No errors in the log (https://travis-ci.com/github/Bash-it/bash-it/jobs/369208799#L297), but you can see that the test cases 150 and 151 are missing for some reason - it goes straight from 149 to 152. The alias-completion.plugin.bats are supposed to run there. I'm wondering whether the check for Darwin in the git completion file is causing this error for some reason.

[nwinkler]

I'll try to install a Linux VM (Docker or Vagrant) later today if I find the time...

[cornfeedhobo]

@nwinkler Interesting. Yeah I see the skips from 149 to 152, and when I run that single test I of course still see the skips, but no actual error

$ TEST_JOBS=1 test/run test/plugins/alias-completion.plugin.bats

2 tests, 0 failures, 2 not run

But you are right, when I comment out the Darwin check, the tests start succeeding again.

FWIW, I'm running these tests on opensuse 15.1

[cornfeedhobo]

Please don't merge this until I reply. I'm going to test this at work with a MacBook and see if there is drift between the tests and reality, or what. I'm curious because I changed this to an -eq comparison and it started working, but -eq is a numeric comparison and unless I'm mistaken, it should be comparing strings which requires = or ==.

[nwinkler]

Thanks for working through this! The other variations we have for checking for OS are these:

• Checking for the $OSTYPE env variable (if/fi with double brackets):

  bash-it/plugins/available/gif.plugin.bash

  Line 39 in f120554

  if [[ "$OSTYPE" == "darwin"* ]] ; then

• Checking for uname with == (if/fi with single brackets):

  bash-it/completion/available/system.completion.bash

  Line 16 in f120554

  if [ $(uname) = "Darwin" ] && command -v brew &>/dev/null ; then

• Lots of case statements for $OSTYPE, e.g. here:

  bash-it/aliases/available/vim.aliases.bash

  Lines 12 to 13 in f120554

  	case $OSTYPE in
  	darwin*)

Plus some other variations. Not sure why your earlier version was failing. The if/fi seems to be important, different than the || return...

[nwinkler]

BTW: This works consistently for me on both macOS and Linux (tested with Ubuntu Xenial64 in Vagrant):

[[ "$(uname -s)" == 'Darwin' ]] || return 0

The return 0 seems to make the difference.

With -eq, the comparison never works (this is on Ubuntu):

> [[ "$(uname -s)" -eq 'Darwin' ]] || echo "Foo"

> [[ "$(uname -s)" -eq 'Linux' ]] || echo "Foo"

> [[ "$(uname -s)" == 'Darwin' ]] || echo "Foo"
Foo

[cornfeedhobo]

@nwinkler Great find. I did mostly the same sleuthing but didn't even think to verify to exit code. I'm not sure why it would be anything but 0. Either way, maybe I'll make some helper functions related to checking OS and then use this as a test case.

Edit: please squash-merge when you do :)

[cornfeedhobo]

@nwinkler I can't find documentation for this, but it would seem that TIL return passes $? when an explicit exit code is not passed. This means that all the places bash-it does || return are probably throwing a >0 code and the caller handles this gracefully, and the aliases test didn't. Hints that we have a lot of code to clean up :p

#!/bin/bash
source foobaar.sh
echo $?

#!/bin/bash
bash foobaaar.sh || return

#!/bin/bash
exit 127

$ bash foobar.sh 
127

[nwinkler]

Awesome - thanks for working through this!

[cornfeedhobo]

@nwinkler No problem and thank you as well! Always fun uncovering another nuance I wasn't aware of.

P.S. I'm getting a real kick out of my contribution record. So far I've removed much more than I've added, which I'm taking as a good sign :p

[nwinkler]

Agreed - removing old/unused code that is no longer required feels good!