mfaerevaag / wd

:rocket: Jump to custom directories in zsh
MIT License
690 stars 44 forks source link

Bug report: can't delete points with noclobber enabled #102

Closed heartburn22 closed 3 years ago

heartburn22 commented 3 years ago

Trying to delete points, I would always receive an error: file exists. "Something bad happened! Sorry."

After some poking around, I found that this line was causing the error:

if sed -n "/^${point_name}:.*$/!p" "$WD_CONFIG" > "$config_tmp" && command cp "$config_tmp" "$WD_CONFIG" && command rm "$config_tmp"

Then I realized that I must have noclobber on. I modified the line like this, and now the script works fine:

if sed -n "/^${point_name}:.*$/!p" "$WD_CONFIG" >| "$config_tmp" && command cp "$config_tmp" "$WD_CONFIG" && command rm "$config_tmp"

Thanks!

alpha-tango-kilo commented 3 years ago

Thanks for the report! I'll look into over the next few days

Link to noclobber

alpha-tango-kilo commented 3 years ago

Hi @heartburn22, I'm looking into this and I'm struggling to replicate the issue. I know it's an easy fix, but I still want to be able to replicate the issue for the sake of adding a test case for this. Could you provide the minimal set of steps from a fresh wd install (can be faked by simply changing the WD_CONFIG variable) to getting this error, provided noclobber is set? Cheers

heartburn22 commented 3 years ago

Hi!

I reverted back to the original version of the script, logged out and back in and did this:


Password:

~
❯ wd list
 * All warp points:
c  ->  ~/.config/zsh
d  ->  ~/.dotfiles

~
❯ wd add h
wd_add:30: file exists: /home/dot/.config/zsh/wd-bookmarks
 * Warp point added

~
❯ wd rm h
wd_remove:14: file exists: /home/dot/.cache/tmp/wd.mpOKA0WBg9
 * Something bad happened! Sorry.

~
❯

I am running zsh with a mostly out-of-the-box prezto. I suspect that the conflict is because the default prezto configuration does "unsetopt CLOBBER" The relevant source is here: https://github.com/sorin-ionescu/prezto/blob/master/modules/directory/init.zsh

I hope that helps. Let me know if I can do anything else.

heartburn22 commented 3 years ago

I just realized that i had my environment set because I was trying to debug it. I commented that stuff out and did it again

#    WD_CONFIG=${ZDOTDIR}/wd-bookmarks; \
#    TMPDIR=${XDG_CACHE_HOME}/tmp; \
#    source ${ZDOTDIR}/wd.sh
    source /usr/share/wd/wd.sh
Password:

~
❯ wd list
 * All warp points:

~
❯ wd add h
wd_add:30: file exists: /home/dot/.warprc
 * Warp point added

~
❯ wd rm h
wd_remove:14: file exists: /tmp/wd.N4vtoJsw5o
 * Something bad happened! Sorry.

That should be a better example

heartburn22 commented 3 years ago

I did this:

#    WD_CONFIG=${ZDOTDIR}/wd-bookmarks; \
#    TMPDIR=${XDG_CACHE_HOME}/tmp; \
#    source ${ZDOTDIR}/wd.sh

    setopt CLOBBER
    source /usr/share/wd/wd.sh

and I got this:

Password:

~
❯ wd list
 * All warp points:

~
❯ wd add h
 * Warp point added

~
❯ wd rm h
 * Warp point removed
alpha-tango-kilo commented 3 years ago

I still haven't been able to reproduce this myself, but I'll merge the fix regardless.

Please note that sourcing wd.sh is not the correct way to use wd, it's instead meant to be added as a function to your .zshrc (see here)

heartburn22 commented 3 years ago

Am I doing it wrong? This is the whole thing from my .zshrc

## enable wd (bookmarks)
wd() {
    WD_CONFIG=${ZDOTDIR}/wd-bookmarks; \
    TMPDIR=${XDG_CACHE_HOME}/tmp; \
    source ${ZDOTDIR}/wd.sh

#    setopt CLOBBER
#    source /usr/share/wd/wd.sh
}

Anyway, thanks! I'm enjoying using wd!

alpha-tango-kilo commented 3 years ago

What you're doing will work (as you've noticed), but isn't really sensible. There's no need to set the environment variables every time you call wd, environment variables are meant to be mainly static anyway. The semi-colons and backslashes also basically cancel each other out in that ; specifies the end of a command (loosely speaking), and \ would be used (typically) at the end of a line to escape the linebreak, allowing you to have a single command span multiple lines, for readability.

I'd re-arrange and modify the lines to something like this:

WD_CONFIG=${ZDOTDIR}/wd-bookmarks
TMPDIR=${XDG_CACHE_HOME}/tmp

wd() {
    . /usr/share/wd/wd.sh
}