[PATCH vis] fix[shell scripts]: review of the scripts and make them more uniform
Export this patch
I have tightened security (all scripts have `set -eu`), added
`vc_fatal` for reporting and `vc_usage` for usage, and generally
cleaned up syntax (to make shellcheck happy).
---
vis-clipboard | 21 ++++++++++++++++ -----
vis-complete | 45 ++++++++++++++++++++++++++ -------------------
vis-open | 30 ++++++++++++++++++++ ----------
3 files changed, 62 insertions(+), 34 deletions(-)
I don't really see the point of the prefixed names (vc_*).
Personally I would remove it since I doubt there is any reason to
source these files from other shell scripts but if you insist on
keeping it the prefix should be correct for the file its in (eg.
vo for vis-open).
Yeah, no problem. I have eliminated these prefixes everywhere
except of vis-clipboard, where it has always been everywhere.
Further comments inline.
diff --git a/vis-clipboard b/vis-clipboard
index ad0408d6..948c317b 100755
--- a/vis-clipboard
+++ b/vis-clipboard
@@ -1,6 +1,7 @@
#!/bin/sh
-
# Copyright (C) 2016 Richard Burke, ISC licensed
+ # shellcheck disable=SC2317
+ set -eu
vc_fatal() {
echo "$@" >&2
@@ -8,7 +9,11 @@ vc_fatal() {
}
vc_usage() {
- vc_fatal "$(basename "$0") [--selection sel] [--usable|--copy|--paste]"
+ vc_fatal "Usage: $(basename "$0") [--selection sel] [--usable|--copy|--paste]\n
+ Copy/paste clipboard interface with support on all provided platforms.\n
+
+ Available selections: clipboard, primary\n
+ Example: $(basename "$0") --copy --selection primary"
}
vc_determine_command() {
@@ -148,17 +153,23 @@ vc_cygwin_paste() {
cat /dev/clipboard
}
+ fn="false"
while [ $# -gt 0 ]; do
case "$1" in
--usable) fn=vc_usable;;
--copy) fn=vc_copy;;
--paste) fn=vc_paste;;
- --selection) shift; sel="$1";;
- *) ;;
+ --selection)
+ shift
+ if [ "$1" != "clipboard" ] && [ "$1" != "primary" ]; then
+ vc_fatal "Invalid selection: $1. Valid options are 'clipboard' or 'primary'."
+ fi
+ sel="$1";;
+ *) vc_usage;;
esac
shift
done
- sel=${sel:-"clipboard"} $fn
+ sel=${sel:-"clipboard"} "$fn"
vc_usage
diff --git a/vis-complete b/vis-complete
index db925f69..b63a89f6 100755
--- a/vis-complete
+++ b/vis-complete
@@ -1,16 +1,27 @@
#!/bin/sh
- set -e
+ set -ue
- usage() {
- printf '%s\n' "usage: $(basename "$0") [--file|--word] pattern" \
- " $(basename "$0") -h|--help"
+ vc_fatal() {
+ echo "$@" >&2
+ exit 1
+ }
+
+ vc_usage() {
+ vc_fatal "Usage: $(basename "$0") [--file|--word] [-h] [--] pattern\n
+ Interactively complete file or word
+
+ --file this passes pattern to find to obtain a list of matching
+ file names (this is the default).
+ --word this reads standard input to obtain a list of lines
+ matching pattern.
+ pattern pattern to be completed"
}
basic_regex_quote() { printf "%s" "$1" | sed 's|[\\.*^$[]|\\&|g'; }
glob_quote () { printf "%s" "$1" | sed 's|[\\?*[]]|\\&|g'; }
COMPLETE_WORD=0
- FIND_FILE_LIMIT=1000
+ FIND_FILE_LIMIT="${FIND_FILE_LIMIT:-1000}"
while [ $# -gt 0 ]; do
case "$1" in
@@ -27,12 +38,10 @@ while [ $# -gt 0 ]; do
break
;;
-h|--help)
- usage
- exit 0
+ vc_usage
Just merge this with the case below. There is no reason for '-h'
to even exist when its already covered (same goes for the mention
in the usage blurb).
;;
- -*)
- usage
- exit 1
+ -*|'')
+ vc_usage
;;
*)
break
@@ -40,35 +49,33 @@ while [ $# -gt 0 ]; do
esac
done
- if [ $# -ne 1 ]; then
- usage
- exit 1
- fi
+ [ "$#" -lt 1 ] && vc_usage
PATTERN="$1"
if [ $COMPLETE_WORD = 1 ]; then
- tr -s '\t {}()[],<>%^&.\\' '\n' |
+ tr -s '\t {}()[],<>%^&.'\' '\n' |
Doesn't this change the match set? Before '\' was part of the set
now ' is part of the set. '\' is important for completion in latex
files.
grep "^$(basic_regex_quote "$PATTERN")." |
sort -u
else
# Expand to absolute path because of the -path option below.
+ # shellcheck disable=SC2088
case $PATTERN in
/*)
- XPATTERN=$PATTERN
+ XPATTERN="$PATTERN"
;;
'~'|'~/'*)
- XPATTERN=$HOME$(echo $PATTERN | tail -c +2)
+ XPATTERN="$HOME$(echo $PATTERN | tail -c +2)"
;;
*)
- XPATTERN=$PWD/$PATTERN
+ XPATTERN="$PWD/$PATTERN"
;;
esac
# The first path condition rules out paths that start with "." unless
# they start with "..". That way, hidden paths should stay hidden, but
# non-normalised paths should still show up.
- find $(dirname "$XPATTERN") \
+ find "$(dirname "$XPATTERN")" \
-name '.*' -prune \
-o \( \
! -name '.*' \
diff --git a/vis-open b/vis-open
index 28d1b1a9..e877b4bf 100755
--- a/vis-open
+++ b/vis-open
@@ -1,17 +1,24 @@
#!/bin/sh
- set -e
+ set -ue
# Later, we're going to want to set $IFS to a single newline, so let's prepare one.
- NL='
- '
+ NL="$(printf '\nx')"; NL="${NL%x}"
Whats the point of this? The original code was very idiomatic and more readable.
- if [ -z "$VIS_OPEN_LINES" ]; then
- VIS_OPEN_LINES='0'
- fi
+ vc_fatal() {
+ echo "$@" >&2
+ exit 1
+ }
- VIS_MENU_PROMPT=''
- ALLOW_AUTO_SELECT='1'
+ vc_usage() {
+ vc_fatal "Usage: $(basename "$0") [-f] [-h] [-p prompt] [--] [file-pattern]\n
+ Interactively select a file to open
+ -f present the argument it is given, even only one
+ -p prompt
+ file-pattern list of filenames and directories"
+ }
+
+ # print a list of filenames on stdin and distinguish directories
wrap_dirs() {
while read -r filename
do
@@ -23,6 +30,10 @@ wrap_dirs() {
done
}
+ VIS_OPEN_LINES="${VIS_OPEN_LINES:-0}"
+ VIS_MENU_PROMPT=''
+ ALLOW_AUTO_SELECT='1'
+
while getopts fhp: opt; do
case "$opt" in
f)
@@ -32,8 +43,7 @@ while getopts fhp: opt; do
VIS_MENU_PROMPT="$OPTARG"
;;
h|?)
- printf 'usage: %s [-f] [-h] [-p prompt] [--] [file-pattern]\n' "${0##*/}"
- exit 0
+ vc_usage
;;
esac
done
--
2.47.1
vis/patches: SUCCESS in 1m25s
[fix[shell scripts]: review of the scripts and make them more uniform][0] from [Matěj Cepl][1]
[0]: https://lists.sr.ht/~martanne/devel/patches/56732
[1]: mailto:mcepl@cepl.eu
✓ #1401317 SUCCESS vis/patches/debian.yml https://builds.sr.ht/~martanne/job/1401317
✓ #1401319 SUCCESS vis/patches/openbsd.yml https://builds.sr.ht/~martanne/job/1401319
✓ #1401316 SUCCESS vis/patches/alpine.yml https://builds.sr.ht/~martanne/job/1401316
✓ #1401318 SUCCESS vis/patches/freebsd.yml https://builds.sr.ht/~martanne/job/1401318
Matěj Cepl <mcepl@cepl.eu> wrote:
[PATCH vis v2] fix[shell scripts]: review of the scripts and make them more uniform
Export this patch
I have tightened security (all scripts have `set -eu`), added
`vc_fatal` for reporting and `vc_usage` for usage, and generally
cleaned up syntax (to make shellcheck happy).
---
vis-clipboard | 23 +++++++++++++++++ ------
vis-complete | 44 ++++++++++++++++++++++++++ ------------------
vis-open | 30 ++++++++++++++++++++ ----------
3 files changed, 63 insertions(+), 34 deletions(-)
diff --git a/vis-clipboard b/vis-clipboard
index ad0408d6..c4bc07da 100755
--- a/vis-clipboard
+++ b/vis-clipboard
@@ -1,6 +1,7 @@
#!/bin/sh
-
# Copyright (C) 2016 Richard Burke, ISC licensed
+ # shellcheck disable=SC2317
+ set -eu
vc_fatal() {
echo "$@" >&2
@@ -8,7 +9,11 @@ vc_fatal() {
}
vc_usage() {
- vc_fatal "$(basename "$0") [--selection sel] [--usable|--copy|--paste]"
+ vc_fatal "Usage: $(basename "$0") [--selection sel] [--usable|--copy|--paste]\n
+ Copy/paste clipboard interface with support on all provided platforms.\n
+
+ Available selections: clipboard, primary\n
+ Example: $(basename "$0") --copy --selection primary"
}
vc_determine_command() {
@@ -141,24 +146,30 @@ vc_mac_paste() {
}
vc_cygwin_copy() {
- cat >/dev/clipboard
+ cat >/devclipboard
}
vc_cygwin_paste() {
cat /dev/clipboard
}
+ fn="false"
while [ $# -gt 0 ]; do
case "$1" in
--usable) fn=vc_usable;;
--copy) fn=vc_copy;;
--paste) fn=vc_paste;;
- --selection) shift; sel="$1";;
- *) ;;
+ --selection)
+ shift
+ if [ "$1" != "clipboard" ] && [ "$1" != "primary" ]; then
+ vc_fatal "Invalid selection: $1. Valid options are 'clipboard' or 'primary'."
+ fi
+ sel="$1";;
+ *) vc_usage;;
esac
shift
done
- sel=${sel:-"clipboard"} $fn
+ sel=${sel:-"clipboard"} "$fn"
vc_usage
diff --git a/vis-complete b/vis-complete
index db925f69..bd2b0c6b 100755
--- a/vis-complete
+++ b/vis-complete
@@ -1,16 +1,27 @@
#!/bin/sh
- set -e
+ set -ue
+
+ fatal() {
+ echo "$@" >&2
+ exit 1
+ }
usage() {
- printf '%s\n' "usage: $(basename "$0") [--file|--word] pattern" \
- " $(basename "$0") -h|--help"
+ fatal "Usage: $(basename "$0") [--file|--word] [-h] [--] pattern\n
+ Interactively complete file or word
+
+ --file this passes pattern to find to obtain a list of matching
+ file names (this is the default).
+ --word this reads standard input to obtain a list of lines
+ matching pattern.
+ pattern pattern to be completed"
}
basic_regex_quote() { printf "%s" "$1" | sed 's|[\\.*^$[]|\\&|g'; }
glob_quote () { printf "%s" "$1" | sed 's|[\\?*[]]|\\&|g'; }
COMPLETE_WORD=0
- FIND_FILE_LIMIT=1000
+ FIND_FILE_LIMIT="${FIND_FILE_LIMIT:-1000}"
while [ $# -gt 0 ]; do
case "$1" in
@@ -28,11 +39,9 @@ while [ $# -gt 0 ]; do
;;
-h|--help)
usage
- exit 0
;;
- -*)
+ -*|'')
usage
- exit 1
;;
*)
break
@@ -40,45 +49,44 @@ while [ $# -gt 0 ]; do
esac
done
- if [ $# -ne 1 ]; then
- usage
- exit 1
- fi
+ [ "$#" -lt 1 ] && usage
PATTERN="$1"
if [ $COMPLETE_WORD = 1 ]; then
+ # shellcheck disable=SC1003
tr -s '\t {}()[],<>%^&.\\' '\n' |
grep "^$(basic_regex_quote "$PATTERN")." |
sort -u
else
# Expand to absolute path because of the -path option below.
+ # shellcheck disable=SC2088
case $PATTERN in
/*)
- XPATTERN=$PATTERN
+ XPATTERN="$PATTERN"
;;
'~'|'~/'*)
- XPATTERN=$HOME$(echo $PATTERN | tail -c +2)
+ XPATTERN="$HOME$(echo "$PATTERN" | tail -c +2)"
;;
*)
- XPATTERN=$PWD/$PATTERN
+ XPATTERN="$PWD/$PATTERN"
;;
esac
# The first path condition rules out paths that start with "." unless
# they start with "..". That way, hidden paths should stay hidden, but
# non-normalised paths should still show up.
- find $(dirname "$XPATTERN") \
+ find "$(dirname "$XPATTERN")" \
-name '.*' -prune \
-o \( \
! -name '.*' \
-a -path "$(glob_quote "$XPATTERN")*" \
-print \
\) 2>/dev/null |
- head -n $FIND_FILE_LIMIT |
+ head -n "$FIND_FILE_LIMIT" |
sort |
- sed "s|^$(dirname $XPATTERN)/||"
+ sed "s|^$(dirname "$XPATTERN")/||"
fi |
vis-menu -b |
- sed "s|^$(basename $PATTERN)$(echo $PATTERN | tail -c 2 | grep -F /)||" |
+ sed "s|^$(basename "$PATTERN")$(echo "$PATTERN" | tail -c 2 | grep -F /)||" |
tr -d '\n'
diff --git a/vis-open b/vis-open
index 28d1b1a9..119473c3 100755
--- a/vis-open
+++ b/vis-open
@@ -1,17 +1,24 @@
#!/bin/sh
- set -e
+ set -ue
# Later, we're going to want to set $IFS to a single newline, so let's prepare one.
- NL='
- '
+ NL="$(printf '\nx')"; NL="${NL%x}"
- if [ -z "$VIS_OPEN_LINES" ]; then
- VIS_OPEN_LINES='0'
- fi
+ fatal() {
+ echo "$@" >&2
+ exit 1
+ }
- VIS_MENU_PROMPT=''
- ALLOW_AUTO_SELECT='1'
+ usage() {
+ fatal "Usage: $(basename "$0") [-f] [-h] [-p prompt] [--] [file-pattern]\n
+ Interactively select a file to open
+ -f present the argument it is given, even only one
+ -p prompt
+ file-pattern list of filenames and directories"
+ }
+
+ # print a list of filenames on stdin and distinguish directories
wrap_dirs() {
while read -r filename
do
@@ -23,6 +30,10 @@ wrap_dirs() {
done
}
+ VIS_OPEN_LINES="${VIS_OPEN_LINES:-0}"
+ VIS_MENU_PROMPT=''
+ ALLOW_AUTO_SELECT='1'
+
while getopts fhp: opt; do
case "$opt" in
f)
@@ -32,8 +43,7 @@ while getopts fhp: opt; do
VIS_MENU_PROMPT="$OPTARG"
;;
h|?)
- printf 'usage: %s [-f] [-h] [-p prompt] [--] [file-pattern]\n' "${0##*/}"
- exit 0
+ usage
;;
esac
done
--
2.47.1
Matěj Cepl <mcepl@cepl.eu> wrote:
Applied with tweaks:
- corrected some wording
- removed -u flag; we usually want to check for unset
variables ourselves and this broke vis-clipboard on
my end.
Thanks!
I also pushed a commit on top which consistently describes each
script's options.
--
https://rnpnr.xyz/
GPG Fingerprint: B8F0 CF4C B6E9 415C 1B27 A8C4 C8D2 F782 86DF 2DC5