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

grive-sync: Remove "$HOME" subdirectory requirement #385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KreAch3R
Copy link

@KreAch3R KreAch3R commented Aug 4, 2023

Check #383

@vitalif
Copy link
Owner

vitalif commented Aug 4, 2023

Hi. Just remove it, don't comment it out
This script was contributed by someone here in PRs so I have no idea why it was limited to $HOME

@KreAch3R
Copy link
Author

KreAch3R commented Aug 4, 2023

Hi. Just remove it, don't comment it out This script was contributed by someone here in PRs so I have no idea why it was limited to $HOME

Hello, ah, ok. Done. If you are interested in clean code, I can try to force push another clean commit but I'm not sure how the PR is going to handle it.

@@ -17,11 +17,6 @@ cd ~
SCRIPT="${0}"
DIRECTORY=$(systemd-escape --unescape -- "$2")

if [[ -z "$DIRECTORY" ]] || [[ ! -d "$DIRECTORY" ]] ; then
echo "Need a directory name in the current users home directory as second argument. Aborting."
Copy link

@jankatins jankatins Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is alright (assuming -z tests for "not defined"), but the msg should be "need a directory as either absolute path or relative to $HOME"

Copy link
Author

@KreAch3R KreAch3R Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right. I took a better look and what exactly was happening and it was all about a "/"....

@KreAch3R KreAch3R force-pushed the 0.5.3-no-home-dir branch from ed54894 to a9775e5 Compare August 6, 2023 19:05
@jankatins
Copy link

Right now I feel that the original test is better suited: if I would configure the script to sync opt/whatever, I would be very suprised if it would use /opt/whatever and not ${HOME}/opt/whatever (for the former, i would want to configure /opt/whatever with a leading slash). For that reason, I would go with just changing the help message:

if [[ -z "${DIRECTORY}" ]] || [[ ! -d "${DIRECTORY}" ]] ; then
	echo "Need a directory name (absolute or relative to the current users home directory) as second argument. Got ${DIRECTORY}. Aborting."
	exit 1
fi

I've actually adjusted that in my local version of the script which I started with systemctl --user start $(systemd-escape [email protected] -- "${HOME}/.gdrive"), so an absolute path.

@jankatins
Copy link

See #389 for my version

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

Successfully merging this pull request may close these issues.

3 participants