Improvements and cleanup #19
Conversation
Signed-off-by: Olliver Schinagl <[email protected]>
The current file is mixing tabs and spaces on various indents. Since tabs are use the most, I suppose that's what was intended to be used. Signed-off-by: Olliver Schinagl <[email protected]>
Ensure consistent editing where supported by adding the optional .editorconfig. This does not enforce any style and only works if the editor supports it (via a plugin for example). Signed-off-by: Olliver Schinagl <[email protected]>
By putting udevadm in a variable we only have one place to have the whole string, could easily replace it with UDEVADM="$(command -v udevadm)" or otherwise. Furthermore add a test for the executability of the binary. Signed-off-by: Olliver Schinagl <[email protected]>
As udevadm, systemd-escape and the various service files are hard requirement, list it as explicitly so in the debian control file. Signed-off-by: Olliver Schinagl <[email protected]>
According to the udevadm manual page --export produces a parseable list.
-x, --export
Print output as key/value pairs. Values are enclosed in single quotes.
So using grep to drop lines that are not key/value pairs is just a
resource waste without doing anything.
Signed-off-by: Olliver Schinagl <[email protected]>
By putting blkid in a variable we only have one place to have the whole string, could easily replace it with BLKID="$(command -v blkid)" or otherwise. Signed-off-by: Olliver Schinagl <[email protected]>
The busybox variant of blkid does not work by default with usbmount. So ensure we depend on the real thing. Things not supported: -p gets ignored and 'USAGE' is not returned. -o value is not supported -s FIELD is ignored It basically just dumps the known info and that's that. Signed-off-by: Olliver Schinagl <[email protected]>
As with blkid, having the standard usbmount directories in variables makes use of these variables less error-prone on usage. Signed-off-by: Olliver Schinagl <[email protected]>
Using quotes and curly braces around shell variables everywhere can be considered good programming practise andtends to be more consistent. Signed-off-by: Olliver Schinagl <[email protected]>
It has a series of undefined behaviors related to quoting in POSIX. It imposes a custom escaping mode with surprising results. It's exceptionally hard to nest. Signed-off-by: Olliver Schinagl <[email protected]>
The blkid tool can output exactly what we need using various parameters. This saves us some sed headaches. The only 'downside' is we can't use our own names for the variables, which affects 'FSTYPE' so that was renamed (rather then do a FSTYPE=$TYPE). Signed-off-by: Olliver Schinagl <[email protected]>
Using egrep is considered deprecated and should be avoided and not used. While we could use grep -q -E, using two simple string comparissions avoid having to call an external tool, making the script lighter and cleaner. Signed-off-by: Olliver Schinagl <[email protected]>
Use grep -E when quering fstab to avoid the deprecated egrep. Signed-off-by: Olliver Schinagl <[email protected]>
Parsing /proc/mounts by hand is not needed as we have the 'mountpoint' utility (part of util-linux and busybox). This eases some regex foo which improves readbility. Signed-off-by: Olliver Schinagl <[email protected]>
While not strictly needed it is considered good practise to ensure proper exit at the end of the script. Signed-off-by: Olliver Schinagl <[email protected]>
By default, read will interpret backslashes before spaces and line feeds, and otherwise strip them. This is rarely expected or desired. Signed-off-by: Olliver Schinagl <[email protected]>
The read utility reads multiple variables with a dummy variable called 'remainder'. Instead, use the more common and well recognized by checkers '_' (underscore) as a dummy variable. Signed-off-by: Olliver Schinagl <[email protected]>
When passing regex to grep, make this explicit with the -E parameter. Signed-off-by: Olliver Schinagl <[email protected]>
With usbmount now being shellcheck compliant, there is only one remaining warning, SC1091 about our sourced file. While we cannot scan whatever the user has installed, we can point shellcheck to the local default config instead. Note, that this will still produce a warning on shellcheck.net as it obviously still cannot access the file. Signed-off-by: Olliver Schinagl <[email protected]>
|
If your hoping to have these commits reviewed and merged within a reasonable time frame, I would suggest that you break them up into multiple pull requests as this list of commits is long enough to deter most people from taking the time to have a look. |
|
With that being said, I have given a quick look at the first few and they seem simple enough and well formatted so I'll probably be able to do a quick review. Unfortunately, I do not have commit access so I can merge myself but at least I can lessen the load of the primary maintainer. |
|
I will try to take a look at this pull request by this weekend... If I miss
it, PLEASE,feel free to ping me...
Thanks a lot for your interest in having usbmount back from the dead.
…--
Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://cynic.cc/blog/ : github.com/rbrito : profiles.google.com/rbrito
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br
Em ter, 30 de out de 2018 17:06, mathieulj <[email protected]
escreveu:
With that being said, I have given a quick look at the first few and they
seem simple enough and well formatted so I'll probably be able to do a
quick review. Unfortunately, I do not have commit access so I can merge
myself but at least I can lessen the load of the primary maintainer.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#19 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAM8sZZfSNo7OmoDETl2RAiKaj9M7bF6ks5uqLE8gaJpZM4X42Te>
.
|
|
ok with that being said, do note, i have not yet tested it!! (while i do not expect much from it, this is mostly cleanup work) I will be testing it in a VM shortly (I cannot do it on my current system easily as it will interfere with a lot of stuff. Since I am going for LABEL support and 'runtime' directory creation and removal, I will be working on that as well. As for the separate pull requests, I thought this was quite separated :) It is all neatly organized in commits, if you read it commit by commit, it becomes very easy and sensible I hope :) But again, i can split it up in multiple PR if that helps the load or burden! Olliver |
|
Hello, |
|
Those commits LGTM. Very nicely organised and explained. A real joy to review. I have yet to test it but I should be able to do so soon. |
|
I have to admit I also still have failed to test it; so sorry for that, but hopefully soon; i just installed a KVM with debian in it and will start testing stuff; as I need it for the second series of patches I have planned :) |
|
Is this gonna be merged in 2021 :D Or are there any better alternatives to usbmount nowdays? :P |
|
I have merged all outstanding pull requests to a fork of this repo due to the death in April 2021 of @rbrito. RIP. I have no particular experience with usbmount other than using it on one of my projects, so I'm probably not a good candidate to look to for ongoing work on the project. However, it's there (presently untested) if anybody needs it. |
This series contains a bunch of fixes and cleanups to improve overall consistency and quality. As it's quite a huge changeset (touching pretty much every single line) it is thus recommended to review it per commit (as intended).
Olliver