marcusbuffett / pipe-rename

Rename your files using your favorite text editor
https://crates.io/crates/pipe-rename
MIT License
397 stars 12 forks source link

Expand ~ to $HOME before performing renames #48

Closed mtimkovich closed 2 years ago

mtimkovich commented 2 years ago

https://github.com/marcusbuffett/pipe-rename/issues/35

marcusbuffett commented 2 years ago

I get this when testing out the branch, we should probably do this replacement before checking for the files existing?

image
mtimkovich commented 2 years ago

My goal was actually to cover the output case i.e. what the user types in their editor. Mainly because if you were to enter your command as renamer ~/Documents/notes/blog.md the shell actually handles expanding ~ to $HOME before passing in the path to renamer.

I wouldn't mind expanding the input case as well if you think it'd be a useful thing to have tho.

bew commented 2 years ago

I wouldn't mind expanding the input case as well if you think it'd be a useful thing to have tho.

but wouldn't that prevent renaming of a file that actually starts with a ~ ? (which is probably a mistake but is possible) Maybe only try to substitute it if the file doesn't exist ?

mtimkovich commented 2 years ago

That's (fortunately) not a concern because under the hood I'm checking for ~/ and / is not a valid character in a filename.

mtimkovich commented 2 years ago

@marcusbuffett after doing a little more research, I think the failure you're seeing is working as intended. If you were to enter a file path in quotes to a different cli tool, it would behave the same way.

$ ls -l "~/.bashrc"
ls: cannot access '~/.bashrc': No such file or directory

$ ls -l ~/.bashrc
lrwxrwxrwx 1 max 26 May 21 08:35 /home/max/.bashrc -> /home/max/dotfiles/.bashrc
marcusbuffett commented 2 years ago

Ah gotcha yeah you're right, so good to merge?

marcusbuffett commented 2 years ago

@mtimkovich I've added you as a collaborator, so feel free to merge whenever you want. Will make another release when this is in. I'll add an automated release thing at one point, for now if you merge anything and notice it hasn't been deployed just ping me. Really appreciate all your contributions.

mtimkovich commented 2 years ago

And I really appreciate you for making this ❤️

assarbad commented 2 years ago

@mtimkovich @marcusbuffett Just ran into another problem with this aside from #57 ...

NB: I am currently in the process of improving test coverage as much as I can manage.

Currently this feature isn't easily testable. Essentially for this to be tested one would have to use a contrived value for $HOME that either depends on the (unknown to the outside) value of the tempdir (in fn run_with_env) or is relative (which seems more feasible but equally awkward).

This special case introduces another weird behavior, too (even on unixoid systems). Let's say you have a file $HOME/.profile and call renamer $HOME/.profile (or renamer ~/.profile) and then "rename" that in the opening editor to ~/.profile in order to trigger the "output case" mentioned above. Suddenly the whole logic which would normally skip identical items (no actual renaming) fails (probably because the comparison comes before the replacement logic):

image

The following replacements overwrite existing files:
/home/oliver/.profile -> /home/oliver/.profile

Execute these renames?:
> Edit
  Yes
  No
  Reset

I reckon this isn't quite working as expected.


I also noticed that this code will in all likelihood¹ not work on Windows (i.e. when encountering the ~/). Reason: neither PowerShell nor cmd.exe set HOME and it isn't a standard environment variable on Windows either. The closest corresponding value could arguably be retrieved from $env:USERPROFILE and %USERPROFILE% respectively (depending on the used shell; set to C:\Users\Oliver in the clip below). And even then a Windows user is more likely to enter ~\ than ~/, which isn't even covered in the current logic. On the other hand PowerShell seems to be able and "willing" to expand the tilde:

powershell-tilde-expansion

I'm wondering what's the best way out here. Not much would be lost by removing this behavior except on unixoid systems. But then PowerShell tries to accommodate even users expecting unix-y behavior, it seems ... oh my.

¹Now checking this requires to use an editor different from built-on Notepad (notepad2 from Git for Windows will work, if in the PATH), because the leading . in the temporary file name trips up Notepad² on Windows. When I use that the outcome is:

> renamer C:\Users\Oliver\.bash_profile

The following replacements were found:

C:\Users\Oliver\.bash_profile -> ~/.bash_profile

Execute these renames?:
> Yes
  No
  Edit
  Reset

... which for obvious reasons fails with:

Error: The system cannot find the path specified. (os error 3)

²Windows places a different meaning on file extensions than your average Unix.