akirak / helm-linux-disks

Emacs Helm interface for mounting removable disks in Linux
11 stars 0 forks source link

Spaces are not supported in mountpoints #2

Open Ambrevar opened 5 years ago

Ambrevar commented 5 years ago

When a drive name contains a space, most actions will fail due to mountpoint only containing the first part of the name until the first space.

The relevant code is here I believe

(defun helm-linux-disks--lsblk ()
  "Get an alist of candidates for `helm-linux-disks--lsblk-source'."
  (cl-loop for ((level . raw) . kdr) on (helm-linux-disks--lsblk-with-levels)
           for next-level = (when kdr (caar kdr))
           for has-child = (and next-level
                                (< level next-level))
           for (name mountpoint fstype type _size) = (pcase (split-string
                                                             (helm-linux-disks--lsblk-trim raw))
                                                       (`(,name ,type ,size)
                                                        (list name nil nil type nil size))
                                                       (`(,name ,fstype ,type ,size)
                                                        (list name nil fstype type size))
                                                       (fields fields))
           collect (cons raw
                         (make-linux-disk
                          :path name
                          :mountpoint mountpoint
                          :fstype fstype
                          :type (intern type)
                          :has-child-p has-child))))

You could either get the mountpoint as "everything but the last 2 words" or by calling lsblk -n -p -o "mountpoint" or something like that.

akirak commented 5 years ago

I've experimentally created device-names-with-spaces branch that would support device names containing spaces. I can't test it right now, but if you test it on behalf of me (i.e. if it works even for LVM on LUKS), I will merge it into master.

Ambrevar commented 5 years ago

Sorry, it fails even without spaces now :p

Actually I thought of something much easier: simply change the format to output the mount point in last position. This way all other columns have a predictable format, and the mount point is "all that remains."

Should be trivial then. And you don't even need the "s" library with that :)

akirak commented 5 years ago

I've created a new branch named mp-last which basically follows your advice. Does it work?

Ambrevar commented 5 years ago

I had to apply the following patch to fix it:

diff --git a/helm-linux-disks.el b/helm-linux-disks.el
index 31df384..070dbf4 100644
--- a/helm-linux-disks.el
+++ b/helm-linux-disks.el
@@ -99,10 +99,14 @@
                                                        (`(,name ,fstype ,type ,size)
                                                         (list name fstype type size nil))
                                                        (fields
-                                                        (append (seq-take fields 4)
-                                                                (string-join
-                                                                 (seq-drop fields 4)
-                                                                 " "))))
+                                                        (append
+                                                         (list (nth 0 fields)
+                                                               nil
+                                                               (nth 1 fields)
+                                                               (nth 2 fields))
+                                                         (list (string-join
+                                                                (seq-drop fields 3)
+                                                                " ")))))
            collect (cons raw
                          (make-linux-disk
                           :path name

At this point, it seems a bit too complicated, I wonder if there is not anything more straightforward.

akirak commented 5 years ago

Yeah, it needs to retrieve name, mountpoint, and type from lsblk, but it is rather hacky.

Alternatively, it is possible to make lsblk produce json output, parse json, and format the data in Helm, but then it can't use the indentation level to check if each item has children. I didn't intend to make this package popular at first, so the code is not very clean.

Ambrevar commented 5 years ago

Alternatively, it is possible to make lsblk produce json output, parse json, and format the data in Helm, but then it can't use the indentation level to check if each item has children. I didn't intend to make this package popular at first, so the code is not very clean.

Didn't know that, that's perfect!

So what's the matter with JSON? I tried it, and it gives me the children, you don't need to rely on indentation which is rather clumsy :) Did I misunderstand something?

akirak commented 5 years ago

The JSON solution probably won't have a problem, but the plain format produces a hierarchical overview of devices, which makes me feel safe when I work on LUKS-crypted devices. With this, I can easily ensure that unlocked devices are properly closed and then ejected. Even if not everyone need it, I like this feature.

Maybe a more reliable solution is to run lsblk twice: Get the JSON output to obtain accurate device information and then the plain text for a hierarchical view. The latter can be optional, and if it is omitted, a flat view is generated from the data parsed from JSON.

Ambrevar commented 5 years ago

I don't understand why JSON would result in a "flat view": the parent nodes in the JSON also have children which translate the hierarchy just like in plain text, so from there you can build a hierarchical view as well, no need to parse the tree output of lsblk. Am I missing something?

akirak commented 5 years ago

I didn't know that. I really can't test this package under the expected condition right now, so I will try out the idea later.