search

franciscolourenco / done

A fish-shell package to automatically receive notifications when long processes finish.
MIT License
766 stars 70 forks source link

Fix failure in tmux detection #128

Closed yuttie closed 9 months ago

yuttie commented 2 years ago

Obtaining process name with basename (ps -o command= -p $tmux_fish_ppid) could fail.

Let's suppose tmux was launched as tmux new-session -c /path/to/dir. The ps command would return tmux new-session -c /path/to/dir, and the result of taking basename of it would be dir, which is not what we expect.

In my opinion, the ps command should return a file path because basename command is used. Prior to the commit 346adbe63714d859aa7fbcfe1fb799de34ddf16c, ps was given -o exe= instead of -o command=. With -o exe=, ps returns a path to the tmux executable, and we don't need to care about what kinds of arguments were passed to tmux.

At the moment, the pull request #124 also tries to solve the same problem. However, I think this solution is better because it doesn't need to consider problematic argument patterns in advance.

Having said that, because I don't know why that commit (346adbe) replaced -o exe= with -o command=, there is a possibility that this commit might cause another problem. It would be better to clarify the reason before merge.

franciscolourenco commented 9 months ago

@SamuelSarle I believe you were the one replacing -o exe= with -o command=. Care to pitch in? Thanks!

franciscolourenco commented 9 months ago

@yuttie this was the reason: https://github.com/franciscolourenco/done/pull/110

SamuelSarle commented 9 months ago

The -o exe= won't be available on some platforms. I'm on macOS nowadays but it wouldn't work here either:

$ ps -o exe=
ps: exe: keyword not found
ps: no valid keywords; valid keywords:
%cpu %mem acflag acflg args blocked comm command cpu cputime etime f flags gid group inblk inblock jobc ktrace ktracep lim login logname lstart majflt minflt msgrcv msgsnd ni nice nivcsw nsignals
nsigs nswap nvcsw nwchan oublk oublock p_ru paddr pagein pcpu pending pgid pid pmem ppid pri prsna pstime putime re rgid rgroup rss ruid ruser sess sig sigmask sl start stat state stime svgid svuid
tdev time tpgid tsess tsiz tt tty ucomm uid upr user usrpri utime vsize vsz wchan wq wqb wql wqr xstat

If you've had a chance to test #124 and confirmed that it works, I'd suggest going that direction as it keeps the -o command=.

franciscolourenco commented 9 months ago

Thank you for the input @SamuelSarle and for the PR @yuttie. Closing this PR in favor of one of the other candidates.