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

add completions for diskutil (osx) #2738

Closed
wants to merge 2 commits into from

Conversation

@elnappo
Copy link
Contributor

@elnappo elnappo commented Feb 18, 2016

I've added completions for diskutil, a tool on osx to manage your disk and filesystems.

diskutil manipulates the structure of local disks. It provides information about, and allows the administration of, the partitioning schemes, layouts, and formats of disks. This includes hard disks, solid state disks, optical discs, CoreStorage volumes, and AppleRAID sets. It generally manipulates whole volumes instead of individual files and directories.

manpage for this tool can be found here: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man8/diskutil.8.html

I've implemented my own __fish_diskutil_mounted_volumes instead of using __fish_print_mounted because diskutil umount is not able to umount e.g. a smb share //user@smb-server.example.com/mydir. This only works via mount points i.e. /Volumes/mydir.

end

function __fish_diskutil_devices
ls /dev/disk*

This comment has been minimized.

@ghost

ghost Feb 18, 2016

Maybe use command ls instead?

This comment has been minimized.

@faho

faho Feb 18, 2016
Member

Or remove the fork entirely by using builtins. Would something like string replace -r '^.*/' '' -- /dev/disk*/* work? Or how is the structure here? I'm assuming there's directories named "disk*"?


function __fish_diskutil_using_command
set cmd (commandline -opc)
if [ (count $cmd) -gt 1 ]

This comment has been minimized.

@ghost

ghost Feb 18, 2016

Just for consistency sake with other code, prefer test ... instead of [ ].

#completion for diskutil

function __fish_diskutil_needs_command
set cmd (commandline -opc)

This comment has been minimized.

@elnappo elnappo force-pushed the elnappo:diskutil-completion branch from b048e87 to d88c6bd Feb 18, 2016
@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 18, 2016

Thank you for your valuable feedback! Some completions are using the same functions __fish_diskutil_using_command and __fish_diskutil_needs_command e.g. vagrant.fish, brew.fish and lein.fish What about adding a "global" function like __fish_print_mounted for this?

All of your comments could also be applied to brew.fish. Should I extend this pull request with this?

end

function __fish_diskutil_mounted_volumes
ls /Volumes/

This comment has been minimized.

@faho

faho Feb 18, 2016
Member

This could be string replace '/Volumes/' '' -- /Volumes/*.

@faho
Copy link
Member

@faho faho commented Feb 18, 2016

I've implemented my own __fish_diskutil_mounted_volumes instead of using __fish_print_mounted because diskutil umount is not able to umount e.g. a smb share //user@smb-server.example.com/mydir. This only works via mount points i.e. /Volumes/mydir.

@elnappo: Could this be added to __fish_print_mounted? You'd probably want to conditionalize it on type -q diskutil. Or does OSX also have (e.g.) umount that doesn't have these limitations?

@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 18, 2016

@faho osx also has umount which is working as expected. This only applies to diskutil.

@faho
Copy link
Member

@faho faho commented Feb 18, 2016

Okay, I've got a few more questions (which you might not have seen):

  • Is it possible to replace the ls calls entirely with string replace? The /Volumes/ thing should be string replace '/Volumes/' '' -- /Volumes/*, the /dev/disk thing depends on the structure.
  • Is /Volumes/ the only point where diskutil can mount something? It seems it isn't (though the common case is mounting there), so you'd be missing some edgecases.
  • It would be nice to make {en,dis}ableJournal only list HFS+ volumes (if it only works on HFS+)
  • The {needs,using}_command stuff can be replaced with __fish_seen_subcommand_from $list_of_commands
@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 18, 2016

It would be nice to make {en,dis}ableJournal only list HFS+ volumes (if it only works on HFS+)

The problem with this is that {en,dis}ableJournal works with mounted an unmounted devices and images. While its easy to found mounted HFS+ volumes (via mount) it isn't for unmounted (how to find all unmounted devices and images using HFS+?).

Is /Volumes/ the only point where diskutil can mount something? It seems it isn't (though the common case is mounting there), so you'd be missing some edgecases.

diskutil can mount filesystems, like mount, into a given directory. For diskutil mount I added a completion for -mountPoint which allows to mount a filesystem to any given location. Do you refer to diskutil umount? For diskutil umount, thats true. I'm missing some edgecases (mount point outside of /Volumes/. Furthermore it feels more natural to unmount Macintosh HD instead of /dev/disk0. It's hard to catch them all so I focused on the most common cases.

The {needs,using}_command stuff can be replaced with __fish_seen_subcommand_from $list_of_commands

I'l take a look at this.

Is it possible to replace the ls calls entirely with string replace? The /Volumes/ thing should be string replace '/Volumes/' '' -- /Volumes/*, the /dev/disk thing depends on the structure.

What are the advantages from this over ls?

Generally I focused on the main use cases. I.e. device parameter could be more then /dev/disk* e.g. /Volumes/Untitled, file:///Volumes/Untitled or a UUID

@elnappo elnappo force-pushed the elnappo:diskutil-completion branch from d88c6bd to 829a6cb Feb 18, 2016
@faho
Copy link
Member

@faho faho commented Feb 18, 2016

The problem with this is that {en,dis}ableJournal works with mounted an unmounted devices and images. While its easy to found mounted HFS+ volumes (via mount) it isn't for unmounted (how to find all unmounted devices and images using HFS+?).

This is a case where you can't know everything (so you might want to keep file completions), but you can at least give known options - via something like reading fstab. Unless of course there's a command that can tell you whether a given file is an HFS+ image.

For diskutil umount, thats true. I'm missing some edgecases (mount point outside of /Volumes/.

In __fish_print_mounted, we read mtab (as an alternative to reading the output of mount) - won't something like that work here? I.e. instead of implementing the logic of checking for mountpoints ourselves, call something that already knows it? Does the output of diskutil list help here?

What are the advantages from this over ls

  • One less fork, i.e. better performance (I recently sped up a bit of fish code by 50% by removing a call to seq)
  • Parsing ls output isn't as bad in fish as it is in bash, but I'm still wary - for one, GNU coreutils' ls just changed to quoting by default (though IIUC not in this case since the output doesn't go to a terminal). Even though that doesn't apply to OSX, ls isn't actually meant to be scraped, and it's rather easy to implement in builtins.
@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 18, 2016

Is it possible to replace the ls calls entirely with string replace? The /Volumes/ thing should be string replace '/Volumes/' '' -- /Volumes/*, the /dev/disk thing depends on the structure.

Sorry I don't get this...

function __fish_diskutil_mounted_volumes
    string replace '/Volumes/' '' -- /Volumes/*
end
diskutil.fish (line 1): string replace '/Volumes/' '' -- /Volumes/*
                        ^
in function '__fish_diskutil_mounted_volumes'
    called on standard input

in command substitution
    called on standard input

In __fish_print_mounted, we read mtab (as an alternative to reading the output of mount) - won't something like that work here? I.e. instead of implementing the logic of checking for mountpoints ourselves, call something that already knows it? Does the output of diskutil list help here?

diskutil list is kind of like fdisk -l so this doesn't help.
As I mentioned before, I've implemented my own __fish_diskutil_mounted_volumes instead of using __fish_print_mounted because diskutil umount is not able to unmount e.g. a smb share //user@smb-server.example.com/mydir. This only works via mount points i.e. /Volumes/mydir.

$ __fish_print_mounted
/dev/disk1
//user@smb-server.example.com/mydir

$ __fish_diskutil_mounted_volumes
Macintosh HD
mydir

Another option would be to use __fish_print_mounted and ignore //. But I think it feels more natural to unmount Macintosh HD instead of /dev/disk0.

This is a case where you can't know everything (so you might want to keep file completions), but you can at least give known options - via something like reading fstab. Unless of course there's a command that can tell you whether a given file is an HFS+ image.

I'm not even sure if someone is ever using this command. To further improve this one could parse the output of mount and extract the hfs mounts or try to parse diskutil list, but I don't think its worth the afford.

@faho
Copy link
Member

@faho faho commented Feb 18, 2016

Is it possible to replace the ls calls entirely with string replace? The /Volumes/ thing should be string replace '/Volumes/' '' -- /Volumes/*, the /dev/disk thing depends on the structure.

Sorry I don't get this...

function __fish_diskutil_mounted_volumes
    string replace '/Volumes/' '' -- /Volumes/*
end
diskutil.fish (line 1): string replace '/Volumes/' '' -- /Volumes/*
                        ^
in function '__fish_diskutil_mounted_volumes'
  called on standard input

in command substitution
  called on standard input

You'll need a fish after 2.2.0 since string isn't in a release
yet. Sorry, I forgot to mention that.

In __fish_print_mounted, we read mtab (as an alternative to reading the output of mount) - won't something like that work here? I.e. instead of implementing the logic of checking for mountpoints ourselves, call something that already knows it? Does the output of diskutil list help here?

diskutil list is kind of like fdisk -l so this doesn't help.
As I mentioned before, I've implemented my own __fish_diskutil_mounted_volumes instead of using __fish_print_mounted because diskutil umount is not able to unmount e.g. a smb share //user@smb-server.example.com/mydir. This only works via mount points i.e. /Volumes/mydir.

$ __fish_print_mounted
/dev/disk1
//user@smb-server.example.com/mydir

$ __fish_diskutil_mounted_volumes
Macintosh HD
mydir

Another option would be to use __fish_print_mounted and ignore //. But I think it feels more natural to unmount Macintosh HD instead of /dev/disk0.

Okay, then keep it that way.

This is a case where you can't know everything (so you might want to keep file completions), but you can at least give known options - via something like reading fstab. Unless of course there's a command that can tell you whether a given file is an HFS+ image.

I'm not even sure if someone is ever using this command. To further improve this one could parse the output of mount and extract the hfs mounts or try to parse diskutil list, but I don't think its worth the afford.

Okay.

@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 19, 2016

Thank you! I found another problem. string replace '/Volumes/' '' -- /Volumes/* is working fine, but:

$ __fish_print_mounted (from HEAD)
/dev/disk1
/
/dev
/Volumes/MobileBackups
//user@smb-server.example.com/mydir
/Volumes/mydir/

$ __fish_diskutil_mounted_volumes
Macintosh HD
MobileBackups
mydir
Disk 2

$ diskutil umount Disk 2
Volume Disk 2 on disk2 unmounted

$ diskutil umount mydir
Unmount failed for mydir

$ diskutil umount /Volumes/mydir/
Unmount successful for /Volumes/mydir/

So we need to preserve the /Volume/ how do I do this with string? string replace '' '' -- /Volumes/* feels wrong.
What do you think is better for diskutil mount completion, /dev/disk4 or disk4 both are possible. I think it should be /dev/disk4 because there is less "magic".

@faho
Copy link
Member

@faho faho commented Feb 19, 2016

So we need to preserve the /Volume/ how do I do this with string? string replace '' '' -- /Volumes/* feels wrong.

Well, if you don't replace anything, use printf instead - i.e. printf '%s\n' /Volumes/*.

@elnappo elnappo force-pushed the elnappo:diskutil-completion branch from 829a6cb to e4e8595 Feb 19, 2016
@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 19, 2016

Okay, thank you! I never used printf this way :)

@faho
Copy link
Member

@faho faho commented Feb 19, 2016

Urgh - 62b76b2 gets in the way here a bit. You'll want to set -l mountpoints /Volumes/*; printf '%s\n' $mountpoints, or you'll get an annoying error message if there are no mountpoints.

Also, I forgot to mention this before, usually we'll squash up the changes when merging, so I'd like it more when you don't force-push. It's not a big deal, but makes it a bit nicer to review.

@elnappo
Copy link
Contributor Author

@elnappo elnappo commented Feb 22, 2016

Is anything missing?

@faho
Copy link
Member

@faho faho commented Feb 23, 2016

Nothing's missing - the edge-cases can be fixed later.

Merged as a805d40, thanks!

@faho faho closed this Feb 23, 2016
@zanchey zanchey added the completions label Feb 25, 2016
@zanchey zanchey added this to the next-2.x milestone Feb 25, 2016
@faho faho added the release notes label Apr 2, 2016
@elnappo elnappo deleted the elnappo:diskutil-completion branch Sep 20, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants