fix(core): Use absolute path for git

[SriramKeerthi]

On installations such as OctoPi, git is shadowed by /root/bin/git. This change forces the script to use /usr/bin/git which isn't affected by other executables with the name "git" in the path.

[coolapso]

Hi, thank you very much for your pull request, but let me tell you a story first.

Octopi is not an OS supported by PiVPN, and they were not very keen on collaborating with us when I approached them about this particular issue. Therefore, I won't be making or accepting any changes based on that particular distribution, and Octopi users should either complain there, use the workaround provided in the docs, or use any of the PiVPN-supported systems.

However, I am open to putting that aside and accepting a solution that works well for both sides.

Even though I don't dislike your solution, and before doing any review, I see a potentially big problem with your approach.

When we simply call the git binary directly, we are sure that we are going to get git regardless of where it might be on $PATH. By hardcoding the git binary directory, we are putting ourselves in a scenario where PiVPN will become broken if, for any reason, any of the main supported operating systems decide to move the place where the git binary is installed by default. Even though it is unlikely to happen, it certainly wouldn't be the first or the last time I see this happening. Therefore, I don't feel this is a safe solution to apply. This means that by calling the git binary directly, we are always safe because whatever those future changes might be, it's very likely that the git binary will end up on the $PATH or that the default $PATH will be updated to contain that new location.

Also Please read this about Pull requests on PiVPN: https://github.com/pivpn/pivpn#pull-requests

@orazioedoardo @TinCanTech Any thoughts?

[SriramKeerthi]

@4s3ti Thank you for the backstory. I also just looked through the issues/discussions from 2017.

Thank you for adjusting the branch the PR is pointing towards.

On the topic of calling git vs /usr/bin/git, safe is subjective. I'd argue that a message that looks like "no such file or directory: /usr/bin/git", followed by the script terminating in the future is a better, and arguably safer outcome than the script executing some other binary and failing without a clear error.

Users trying to use OctoPi or some other "dumbed down" OS are likely to run into PiVPN's script not working more often than their git being installed in some random location.

After searching for quite some time, the only instances that I saw git being installed in a location other than /usr/bin/git was on GitHub for Mac (/usr/local/bin/git) or on Windows, both of which aren't (AFAIK) target OSes for PiVPN.

However, if you believe that this isn't a solution to OctoPi, or other git-related issues, please feel free to close the PR.

[orazioedoardo]

If OctoPi compatibility reduces to just this you might as well make the script detect OptoPI somehow and use the full path in that case.

[coolapso]

well ... that might be to much ... i'd say we can do it like this instead.

set a var called GITBIN=/usr/bin/git
Replace all the calls to git with the var ${GITBIN}

This way if something happens its easy to fix by updating only the value of the variable.

and please update your git commits following the commit convention mentioned here: https://github.com/pivpn/pivpn#pull-requests
this is more to a refactor than to a feature or fix, but let's consider it a fix(core): Use git absolute path or something like that

[SriramKeerthi]

Updated. Apologies for both the delay in getting back on this, and the multiple commits. Had some trouble getting the description to show up correctly with the commit.

[coolapso]

No worries!
LGTM.

Thank you for your contribution!!

Merging, into test branch.

[coolapso]

🎉 This PR is included in version 4.2.1-test.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[coolapso]

🎉 This PR is included in version 4.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀