As a result of latest discussions with Diana Coman and Mircea Popescu, I have made some changes to the the v.sh shell wrapper. The main requests for changes were:
- Stop using "-" as a synonym for "all vpatches". Possibly select all vpatches for pressing if the "leafs" argument is not provided.
- Add a command for visual inspection of the vtree.
- Have a verbose mode, make sure that the tool output is machine-readable and pipeable.
- There was also a discussion about code style: whether the patches, seals, and wot directories should appear verbatim in the source or indirectly via corresponding variables.
As the discussion was going on, I took time to pass v.sh through a tool called shellcheck1, which intends to detect insecure or incorrect shell usage, and did give some useful, albeit minor, suggestions. Those are, with the examples from vpatch:
- Missing quoting of variables. It's hard to foresee how this can cause probles, but than again if the robustness of the tool can be improved without extra complexity, I'm all for it.
@@ -52,22 +52,22 @@ trap cleanup TERM INT exit_msg() { - echo $1 + echo "$1" cleanup } verify() { vpatchpath="$1" - vpatch="$(basename $vpatchpath)" + vpatch=$(basename "$vpatchpath") signers="" checkdir seals for i in seals/$vpatch.*.sig; do - signature="$(basename $i | sed 's/.sig$//g')" + signature=$(basename "$i" | sed 's/.sig$//g') signer="${signature#$vpatch.}" if ! gpg --homedir "$vgpgdir" --logger-fd 1 --batch \ - Bad order of redirections. This is a useful one, as I missed it during development.
if ! gpg --homedir "$vgpgdir" --logger-fd 1 --batch \ --keyid-format=long \ - --verify "$i" "$vpatchpath" 2>&1 >/dev/null; then + --verify "$i" "$vpatchpath" >/dev/null 2>&1; then exit_msg "Bad signature $i for $vpatch" fi signers="$signers $signer"
- An unused variable.
@@ -116,8 +116,7 @@ vpatchflow() { [ -z "$*" ] && exit_msg "No leaves selected." filterall | internall >"$filtered" - selected="" - if [ "$*" == "-" ]; then + if [ "$*" = "-" ]; then echo "S" >"$selfile" else echo $* | tr ' ' '\n' | vpatchnos | sed -e 's/^/s /g' - >"$selfile" - Use of $* instead of "$@". In this suggestion was not helpful, because after considering code end-to-end, it became clear, that vpatches with spaces in the filenames would require more extensive changes to be supported. Maybe in the next iteration, if it is worth the extra complexity. Piping stuff with space in a name is a mess.
if [ "$1" = "flow" ]; then shift vpatchflow $* echo "Press order:" awk '$1 == "P" {print $2}' < "$topo" | vpatchnames vpatchflowerr cleanup fi
What this tool did not catch, was that "==" is nonstandard syntax, even though commonly supported. However, I did find a system under my control where this problem surfaced. So fix was applied in this vpatch as well.
@@ -224,7 +223,7 @@ cleanup fi; -if [ "$1" == "flow" ]; then +if [ "$1" = "flow" ]; then shift vpatchflow $*
In general, I'd say this this tool is useful for learning, but as with any other tool, its suggestions cannot be accepted without thinking. What is sane about it is the set of explanations of corrections (example) - perhaps those are worth much more then the tool itself.
curl 'http://bvt-trace.net/vpatches/vtools_vsh_shellcheck.vpatch' > vtools_vsh_shellcheck.vpatch curl 'http://bvt-trace.net/vpatches/vtools_vsh_shellcheck.vpatch.bvt.sig' > vtools_vsh_shellcheck.vpatch.bvt.sig
The next set of changes come from discussion with Diana Coman and Mircea Popescu:
- "$allkeyword" is introduced, to replace the usage of "-" to signify 'all vpatches', but now set to "all".
- Not specifying any vpatch now selects all vpatches (equivalent to "$allkeyword").
- A command for visual inspection of vpatch tree ("vtree") is added. It prints the flow, indented by the number of antecedents.
- A as a result of discussion between Diana and Mircea, I decided not to change wot/patches/seals directories with "$wotdir", etc. If this ever needs to be changed, I'll introduce the variables for them. If not, they will stay like that.
I would like to take more time to think about the verbose flag, so in meantime I have unsuppressed output of vpatch. So far my idea of how this will be implemented:
- The verbose mode is enabled by setting the VERBOSE environment variable.
- set -x will be executed at the very beginning of a script, enabling the echoing of each command executed in v.sh.
- All output currently redirected to /dev/null will go to stdout.
- Perhaps, I will also avoid deleting the temporary directory, for better debugging.
What do you think?
For easier piping, I did some changes in this very vpatch:
- The tool exits with non-zero exit codes on errors.
- There are no headers in case of correct output.
- The error reporting is redirected to stderr.
GNU tools typically check whether the stdout is a tty (literally isatty(3)) - first, the equivalent is not available in Shell, to the best of my knowledge. Second, IMO it's better to massage the output into being usable for both human and machine, than to rely on DWIM.
--- a/vtools/src/vflow.adb 400069490b8212c36d75f27a9935f5017ad6bdb49c9f520293082a4dcc4a1e902fe3733cf34c4356c8a0c315905b8534236ad9b1557a32ec501742693f644514
+++ b/vtools/src/vflow.adb e6faf838d6886b05028f7483b7ff738584217d570a2a894fbe91380614a576f25952120f7fa5c1dff3bd6b16e53364cb5859ed2407412ab121c1c76d0826044c
@@ -366,6 +366,8 @@
for I in Vpatch_Order'Range loop
if Selected_Vpatches(Vpatch_Order(I)) then
Put_Line("P " & Integer'Image(Vpatch_Order(I)));
+ else
+ Put_Line("p " & Integer'Image(Vpatch_Order(I)));
end if;
end loop;
end Print_Flow_Or_Conflicts;
First, changes to vflow press order output. Previously, the tool would print only the patches in the flow of selected leaves. Now, the full flow is printed, and the patches in the flow are printed with capital "P", not in the flow - with lowercase. This change is necessary for the ascii flow visualization.
@@ -2,6 +2,8 @@ set -e +allkeyword="all" + vgpgdir="" vpatches="" filtered=""
The keyword for "all vpatches" is introduced.
@@ -18,13 +20,14 @@
v descendants patch -- list patch descendants
v origin hash -- find patch that introduced the hash
v press directory leaves -- press all patches up to head
+v vtree leaves -- print the vtree structure for a press
EOF
exit 0
}
checkdir() {
if [ ! -d "$1" ]; then
- echo "$1 directory not found"
+ >&2 echo "$1 directory not found"
kill -TERM 0
fi
}
A help entry for "vtree" is added to usage, and error messages are redirected to stderr using >&2.
@@ -47,13 +50,20 @@
if [ ! -z "$vgpgdir" ]; then
rm -rf "$vgpgdir"
fi
- exit 0
+ exit "$1"
+}
+exit_success() {
+ cleanup 0
}
-trap cleanup TERM INT
+
+fail() {
+ cleanup 1
+}
+trap fail TERM INT
exit_msg() {
- echo "$1"
- cleanup
+ >&2 echo "$1"
+ cleanup 1
}
verify() {
cleanup now takes an argument - exit code. Two functions are introduced to: exit_success to invoke cleanup with argument 0, and fail to exit with code 1. On signals, exit with code 1. Also, use stderr instead of stdout in exit_msg wrapper.
@@ -105,7 +115,8 @@
vpatchnos() {
awk -v VF="$vpatches" '
-FILENAME==VF {a[$1]=$2} FILENAME=="-" {print a[$1]}' "$vpatches" -
+FILENAME==VF {a[$1]=$2}
+FILENAME=="-" {if (!($1 in a)) {print "not found:",$1} else {print a[$1]}}' "$vpatches" -
}
vpatchnames() {
Additional error reporting in vpatchnos: vpatchnos is directly handling user input, and as such it now reports the cases when vpatch specified by user does not exit. The in-band signaling is horrible, of course, but awk lacks an option to print to stderr, which I would have used otherwise.
@@ -114,12 +125,15 @@
}
vpatchflow() {
- [ -z "$*" ] && exit_msg "No leaves selected."
filterall | internall >"$filtered"
- if [ "$*" = "-" ]; then
+ if [ "$*" = "$allkeyword" ] || [ -z "$*" ]; then
echo "S" >"$selfile"
else
- echo $* | tr ' ' '\n' | vpatchnos | sed -e 's/^/s /g' - >"$selfile"
+ echo $* | tr ' ' '\n' | vpatchnos >"$selfile"
+ if >&2 grep "not found:" "$selfile"; then
+ fail
+ fi
+ sed -i -e 's/^/s /g' "$selfile"
fi
cat "$filtered" "$selfile" | \
vflow $(wc -l <"$filtered") $(wc -l <"$vpatches") >"$topo"
Now, empty arguments to vpatchflow are considered to be synonym for all vpatches; errors reported by vpatchnos are conveyed to user via stderr.
@@ -133,17 +147,17 @@
END {if (err != 0) exit err; if (ok != 1) exit 3; exit 0;}' <"$topo"
exitcode="$?"
if [ "$exitcode" -eq "1" ]; then
- printf "\nConflicting leaves selected:\n"
- awk '$1 == "C" {print $2}' < "$topo" | vpatchnames | sort | uniq
- cleanup
+ >&2 printf "\nConflicting leaves selected:\n"
+ >&2 awk '$1 == "C" {print $2}' < "$topo" | vpatchnames | sort | uniq
+ fail
elif [ "$exitcode" -eq "2" ]; then
- printf "\nConflicting leaves selected:\n"
- awk '{print $2}' < "$topo" | vpatchnames
- cleanup
+ >&2 printf "\nConflicting leaves selected:\n"
+ >&2 awk '{print $2}' < "$topo" | vpatchnames
+ fail
elif [ "$exitcode" -eq "3" ]; then
- echo "Unknown error. Investigate in $vgpgdir:"
- cat "$topo"
- exit 0 # no cleanup
+ >&2 echo "Unknown error. Investigate in $vgpgdir:"
+ >&2 cat "$topo"
+ exit 1 # no cleanup
fi
}
Use stderr in for error reporting in vpatchflowerr as well.
@@ -153,14 +167,14 @@ if [ "$1" = "wot" ]; then gpg --homedir "$vgpgdir" --logger-fd 1 --batch --keyid-format=long -k - cleanup + exit_success fi verifyall if [ "$1" = "patches" ]; then cat "$vgpgdir"/signers.txt - cleanup + exit_success fi if [ "$1" = "graph" ]; then @@ -170,7 +184,7 @@ svgfile="$1" shift - vpatchflow - + vpatchflow cat >"$dotfile" <> "$dotfile" dot -Tsvg "$dotfile" -o "$svgfile" # dot -Tcmapx $dotfile -o $svgfile.html-map - cleanup + exit_success fi if [ "$1" = "antecedents" ]; then
Use new wrappers for cleanup 0 all over the place.
if [ "$1" = "antecedents" ]; then
@@ -190,13 +204,14 @@
vpatch="$1"
shift
- vpatchflow -
+ vpatchflow
vno=$(awk -v V="$vpatch" '$1==V {print $2}' < "$vpatches")
+ [ -z "$vno" ] && exit_msg "not found: $vpatch"
awk -v N="$vno" '
($1 == "I" || $1 == "d" ) && $2 == N {a[$3]=1}
($1 == "P") {if ($2 in a) {print $2}}
' "$topo" | vpatchnames
- cleanup
+ exit_success
fi
if [ "$1" = "descendants" ]; then
@@ -204,13 +219,14 @@
vpatch="$1"
shift
- vpatchflow -
+ vpatchflow
vno=$(awk -v V="$vpatch" '$1==V {print $2}' < "$vpatches")
+ [ -z "$vno" ] && exit_msg "not found: $vpatch"
awk -v N="$vno" '
($1 == "I" || $1 == "d" ) && $3 == N {a[$2]=1}
($1 == "P") {if ($2 in a) {print $2}}
' "$topo" | vpatchnames
- cleanup
+ exit_success
fi
if [ "$1" = "origin" ]; then
antecedents and descendants commands are changed to invoke vpatchflow without arguments, get extra error reporting, and use the cleanup 0 wrappers.
if [ "$1" = "flow" ]; then
shift
vpatchflow $*
+ vpatchflowerr
- echo "Press order:"
awk '$1 == "P" {print $2}' < "$topo" | vpatchnames
- vpatchflowerr
- cleanup
+ exit_success
fi
flow has lost its header, reports errors in the correct place, and uses the new cleanup wrapper.
if [ "$1" = "press" ]; then
shift
dir="$1"
+ [ -z "$dir" ] && exit_msg "Provide press directory"
shift
[ -d "$dir" ] && exit_msg "Directory $dir already exists"
@@ -249,12 +265,28 @@
awk '$1 == "P" {print $2}' < "$topo" | vpatchnames | while read i; do
i=$(readlink -f "$i")
cd "$dir"
- vpatch <"$i" >/dev/null
+ vpatch <"$i"
cd - >/dev/null
done
- cleanup
+ exit_success
+fi
press has extra error checking: user must provide the directory name, vpatch output is unsuppressed, also, use the new wrapper.
+
+if [ "$1" = "vtree" ]; then
+ shift
+
+ vpatchflow "$@"
+ awk -v VF="$vpatches" '
+function repeat(s,n) {i=0; while (i++ < n) {printf("%s",s);}}
+FILENAME==VF {n[$2]=$1}
+FILENAME=="-" && ($1 == "I" || $1 == "d" ) {a[$2]++;b[$3]=1;}
+FILENAME=="-" && ($1 == "P" || $1 == "p") {
+ if (!($2 in a)) {a[$2]=0};
+ repeat(" ", a[$2]);
+ printf("%s%s%s\n",n[$2], ($2 in b) ? "" : " (*)", ($1 == "P") ? " +" : "" );
+}' "$vpatches" - <"$topo"
+ exit_success
fi
The vtree command prints the flow, one line per vpatch, indented by the number of spaces equal to the number of antecedents of the vpatch. Additionally, it prints "(*)" mark for the leafs, and "+" mark for vpatches selected for press.
The work is done in an awk script:
- repeat prints the designated number of spaces.
- the next line reads the vpatches from file into n array.
- then, the script reads from stdin:
- if it reads "I descendant antecedent" or "I descendant antecedent" command, it increments the number of antecedents of the descendant in the a array, and adds the antecedent into the set of non-leafs in the b array.
- if it reads "P vpatch" or "p vpatch" commands, it sets the count to zero (for genesis), prints the indentation, and outputs the vpatch name, leaf mark, and selection-for-press mark to stdout.
-echo "Unknown command: $1" -cleanup +>&2 echo "Unknown command: $1" +fail
Unknown command is also reported better (stderr & exit code).
The vpatches are here:
curl 'http://bvt-trace.net/vpatches/vtools_vsh_ascii_visualization.vpatch' > vtools_vsh_ascii_visualization.vpatch curl 'http://bvt-trace.net/vpatches/vtools_vsh_ascii_visualization.vpatch.bvt.sig' > vtools_vsh_ascii_visualization.vpatch.bvt.sig
Enjoy!
- Implemented in Haskell, so probably belongs to a machine dedicated for software insanities. [↩]