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

Prefer PWD environment variable over cwd if available to better support symbolic links #783

Merged
merged 1 commit into from Mar 14, 2023

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Mar 14, 2023

This PR uses the PWD environment variable if it is available instead of calling env::current_dir().

Motivation: I was short on storage space in my main /home/user filesystem, so I moved all my /home/user/workspaces to another disk at /home/newdisk/user/workspaces, then created a symbolic link from /home/user/workspaces to /home/newdisk/user/workspaces.
To my dismay, when I searched my command history with the directory filter mode, nothing showed up.

For reference:

@vercel
Copy link

vercel bot commented Mar 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
atuin ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 14, 2023 at 4:12PM (UTC)

Comment on lines 183 to 184
// It's better for atuin to silently fail here and attempt to
// store whatever is ran, than to throw an error to the terminal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not believe this comment is worth keeping as utils::get_current_dir() returns an empty string if all else fails.

@conradludgate
Copy link
Collaborator

Hmmm. Just so I'm clear PWD would produce /home/user/workspaces but cwd() would follow the symlink to /home/newdisk/user/workspaces?

@pdecat
Copy link
Contributor Author

pdecat commented Mar 14, 2023

Yes, precisely.

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Definitely reasonable behaviour, and I think the user is more likely to want this

Could consider making it configurable in the future

Thank you!

@ellie ellie merged commit efd2230 into atuinsh:main Mar 14, 2023
6 checks passed
@pdecat pdecat deleted the prefer_pwd branch March 14, 2023 23:17
@pdecat
Copy link
Contributor Author

pdecat commented Mar 17, 2023

Just figured it may cause troubles for users already having commands in their history executed from symlinked folders (it was the case for one of mines).

Maybe this behavior should be made be opt-in with a configuration option?

An alternative would be to allow mass updating commands, but I guess its the same trouble as for deleting records, maybe even harder.

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.

None yet

3 participants