Re: Review of new man page of sensible-editor
RL wrote:
>> This looks like a great improvement on what we've got, and also better
>> than the quarter-finished version I had, but unfortunately it looks
>> like an improvement on the version in buster. The sid version has had
>> some refactoring - though it doesn't fix the bug we'd noticed - so it
>> would make sense to start from there.
>
> Oh good point - i use stable. Updated version below - using 0.0.17
>
> The sid version also has a slightly different logic to bullseye: it
> tries VISUAL (from the environment) but then it reads ~/.selected_editor
> and then tries EDITOR, SELECTED_EDITOR - so if you did 'export
> EDITOR=vi' but ~/.selected_editor said EDITOR=emacs you'd get emacs. I
> assume that is another bug and that the environment should take
> precedence over the file.
I would have thought so. At least if it isn't meant to work like that
I'd expect the man page to say so!
> My previous version also had too much quoting in places so that passing
> arguments inside EDITOR would not have worked. This one should be
> better. (tested with "EDITOR='ls -l' and it seems to work)
>
> And going back to the man-page: sensible-editor doesn't use the variable
> SENSIBLE_EDITOR at all, so that bit (3rd bullet) can go.
I completely missed that. I noticed the bit in environ(7) about the
controversy over what variables named after an executable should be
expected to contain, and I intended to check exactly what
sensible-editor did with SENSIBLE_EDITOR; but since in fact it does
nothing I then forgot about it!
> #!/bin/sh
> # Copyright 2007 Jari Aalto; Released under GNU GPL v2 or any later version
> # Copyright 201
(I wonder what's going on here?)
> # Prevent recursive loops, where environment variables are set to this script
> p="$(command -v "$0")"
Of course this loop-detector won't catch VISUAL='sensible-editor --',
but I suppose that's just another variety of "don't do that then".
> PreventLoops()
> {
> [ -n "$VISUAL" ] && [ "$(command -v "$VISUAL" || true)" = "$p" ] && VISUAL=
The "||true"s here seem pointless too; if "command" fails, you'll just
end up comparing "" to "$p", which is fine.
> [ -n "$EDITOR" ] && [ "$(command -v "$EDITOR" || true)" = "$p" ] && EDITOR=
> [ -n "$SELECTED_EDITOR" ] && [ "$(command -v "$SELECTED_EDITOR" || true)" = "$p" ] && SELECTED_EDITOR=
> }
>
> UnableToRun()
> {
> # 'Command not found' or 'Permission denied'
> [ "$1" -ne 126 ] && [ "$1" -ne 127 ]
> }
>
> Try()
> {
> "$@"
> ret=$?
> if UnableToRun "$ret"; then
> exit "$ret"
> fi
> }
>
> # work around for #991982
> nano ()
> {
> if [ -z "$TERM" ]; then
> return 126
> else
> command nano "$@"
> fi
> }
(That's some enthusiastic indenting...)
> PreventLoops
> for candidate in "$VISUAL" "$EDITOR"; do
> if [ -n "$candidate" ]; then
> # $candidate must be unquoted on next line as it may contain arguments
> Try $candidate "$@"
> fi
> done
>
> # fix #987675
I suspect this *could* use
: "${HOME:=$( printf '%s' ~ )}"
but that's too cunning for its own good.
> if [ -n "$HOME" ]; then
> if [ -r ~/.selected_editor ]; then
> . ~/.selected_editor 2>/dev/null
> elif [ -z "$EDITOR" ] && [ -z "$SELECTED_EDITOR" ] && [ -t 0 ]; then
> select-editor && . ~/.selected_editor 2>/dev/null
> fi
> fi
> PreventLoops
> for candidate in "$EDITOR" "$SELECTED_EDITOR" editor nano nano-tiny vi; do
> if [ -n "$candidate" ]; then
> Try $candidate "$@"
> fi
> done
I suppose you could move more logic into that one function:
Try()
{
# global $candidate
[ -z "$candidate" ] && return
$candidate "$@"
ret=$?
UnableToRun "$ret" && exit "$ret"
}
[...]
for candidate in "$EDITOR" "$SELECTED_EDITOR" editor nano nano-tiny vi; do
Try "$@"
done
Oh, and couldn't it absorb the unable-to-run part, and maybe even the
PreventLoops part too? That just needs a line
[ "$candidate" = "$0" ] && return
>
> echo "Couldn't find an editor!" 1>&2
> echo 'Set the $EDITOR environment variable to your desired editor.' 1>&2
> exit 1
I seem to have missed the part where, if everything works, it exits
successfully - am I just losing track or has it gone astray?
>
> (i dont know if #991982 also applies to nano-tiny as well as nano?)
Oh, nano-tiny installs its binary as /bin/nano-tiny, not as /bin/nano.
But I'm guessing nano-tiny and sensible-utils rarely meet...
(Thinking about the question of whether you've incidentally removed
the last justification for fallbacks beyond "editor", here's a
terrible idea: any system running a Debian kernel also has busybox for
use in the initramfs, so it could call "busybox vi" as its last
desperate fallback! Shudder.)
--
JBR with qualifications in linguistics, experience as a Debian
sysadmin, and probably no clue about this particular package
Reply to: