themes/base: don't invoke svn if possible

[gaelicWizard]

Description

If we are specifically in the situation #1612 (where Xcode's xcrun is masquerading as svn), then check for a working svn command. If we're not in that situation, then don't waste time on it. This check was added in #1613.

Motivation and Context

This is one of the causes of my shell taking several seconds to start.

How Has This Been Tested?

Tested locally.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[gaelicWizard]

I think that test 'themes base: SVN: ignore SVN repo when using broken SVN command' should only be running on a Mac worker, and alsö it may be a little weird to make the test fail predictably without a broken svn in the standard location. We're testing for a known-broken svn, not for "literally any svn that isn't behaving for unknown reasons". Thoughts?

[NoahGorny]

I think that test 'themes base: SVN: ignore SVN repo when using broken SVN command' should only be running on a Mac worker, and alsö it may be a little weird to make the test fail predictably without a broken svn in the standard location. We're testing for a known-broken svn, not for "literally any svn that isn't behaving for unknown reasons". Thoughts?

I agree that the test can be changed. I do not think it should only run on macs though- we should mock the conditions for the check and test it, imitating that it is indeed a problematic Mac

[gaelicWizard]

As an alternative to my patch here, we could check for xcrun and then invoke xcrun svn to test for broken... I'll see if I can do a version that tries that. It would allow us to mock xcrun for testing without requiring it be at /usr/bin/xcrun

[gaelicWizard]

xcrun --find svn

Ok, so how about: (1) if svn found in path, (2) and if xcrun found in path, (3) check if the two are siblings (covers (a) mocking, or (b) non-standard $DEVELOPER_DIR, or (c) /usr/bin/{xcrun,svn}, (4) and if all are true then invoke svn to see if it fails. Should still be much faster than just running svn every shell startup!

[NoahGorny]

Sounds like a good idea!

[gaelicWizard]

I'm sorry, I haven't had much time to learn BATS so this is just sitting waiting for me to circle back to it.

[NoahGorny]

I'm sorry, I haven't had much time to learn BATS so this is just sitting waiting for me to circle back to it.

There is not much to learn actually, you can look at our docs for reference

[gaelicWizard]

Done and passing.

[NoahGorny]

I left one comment, otherwise looks great!

[gaelicWizard]

And now I think I'm going to change the test local setup function too. If we're testing that the file loads properly, then loading the file should be part of the test not the setup.

See? This is why I didn't touch BATS at first. Once I start, my brain drags me down the rabbit hole! I may need to start a whole branch just for BATS cleanup! 🙃

(I have a love/hate relationship with my brain. I'll spare you my further whining about my brain.)

[gaelicWizard]

Serious question: although it doesn't matter for this particular PR, what are your thoughts on naming the library init function?

Several plugins use init functions (and call them immediately, like I do here) and I very much do like the paradigm of keeping the definition separate from the invocation. I alsö want to introduce a library init hook and plugin init hook (later, in a future PR) such that loading a file doesn't do anything besides declare/define variables and functions and then the we separately run all the init hooks afterwards. This would solve most(?) of the need for explicit ordering of file load (default priorities, &c.), and would make bash-it reload much easier to reason about and test.

I'd like a common prefix for init functions so that "${!_bash-it-hook-enable-@}" expands to "${_bash-it-hook-enable-docker}" "${_bash-it-hook-enable-rvm}" [...] for easier invocation. (But of course, that's for variable names not function names...)

Should I name the one I introduce here _bash-it-hook-lib-init-scm maybe? (Can always rename later if/when implementing the hooks; just trying to think ahead.)

[NoahGorny]

I have another small comment 😄

About the init function- i am pretty sure it will take a long refactor to get bash-it-reload to just call the init of each file and I am not sure it will be worth the effort. I am okay with adding init functions like you did, not sure that changing the reload will happen.

[gaelicWizard]

About the init function- i am pretty sure it will take a long refactor to get bash-it-reload to just call the init of each file and I am not sure it will be worth the effort. I am okay with adding init functions like you did, not sure that changing the reload will happen.

That's a whole other discussion that is wildly out of scope here...I only mentioned it because it's on my mind. Functions have scope! (I've shot myself in the foot with this one, can you tell? 😉)

[NoahGorny]

Well done @gaelicWizard!
Another great work by you (or your brain worms)

[gaelicWizard]

Anything else needed for this PR?

[NoahGorny]

nope- well done!