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

fix(cli): expand sync path argument #16

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonas-w
Copy link
Contributor

@jonas-w jonas-w commented Nov 22, 2024

Relative paths or paths containing '~' were interpreted as relative paths.

e.g. sync = "~/edu-sync" while you are in the directory ~/Downloads/ would synchronize into ~/Downloads/\~/edu-sync
or sync = "." would always sync into the current working directory.

@mkroening mkroening self-assigned this Nov 22, 2024
@mkroening
Copy link
Owner

mkroening commented Nov 22, 2024

I am currently on the phone. Is path::absolute sufficient? The source code of path::absolute does not seem to expand ~. We could use use shellexpand.

@jonas-w jonas-w marked this pull request as ready for review November 22, 2024 16:14
@jonas-w
Copy link
Contributor Author

jonas-w commented Nov 22, 2024

@mkroening I tested it, and it did expand ~ to my home folder, though I'll do some more testing to be sure

@jonas-w
Copy link
Contributor Author

jonas-w commented Nov 22, 2024

@mkroening thanks for pointing this out, I developed this with Termux on Android, and it seems like it does some weird path things there, on my laptop it does in fact not expand ~ to home

@jonas-w
Copy link
Contributor Author

jonas-w commented Nov 22, 2024

Okay don't know what's happening on Termux. shellexpand does look like a good option, but like path::absolute it does not remove .. from the path.

I'd propose the following solution:

  1. Use shellexpand to expand the tilde
  2. Create the directory, and then use std::fs::canonicalize (which has the requirement, that the path already exists) to get the "real path", this would double as a check if the directory can be created and later on used for syncing

Alternative:
Just use shellexpand, and leave the .. in there, which could theoretically make problems later on. For example, you are in the directory /home/user/foo and add an account to edu-sync with the path ../bar, this would be the path /home/user/foo/../bar. This works while /home/user/foo exists, but will fail if it doesn't exist anymore even though /home/user/bar still exists.

@mkroening
Copy link
Owner

Your proposal sounds good. 👍

@jonas-w jonas-w force-pushed the fix/absolute_sync_path branch from 43bfefb to d86f217 Compare November 22, 2024 19:27
@jonas-w
Copy link
Contributor Author

jonas-w commented Nov 22, 2024

I played around a little with paths like /home/user/foo/../bar in the configuration file, and then decided to use the same approach for the path from the configuration file as I did with the path CLI argument because these paths caused unreachable! statements to execute (not reliably, couldn't reproduce this again). And edu-sync-cli kind of worked with these paths, but not really.

This is now a bit duplicate, as it should get canonicalized during the setup and the path gets canonicalized every invocation of sync, but didn't know if there was a simpler way to be sure the path won't break edu-sync somehow.

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.

2 participants