Re: proposed patches to notmuch-emacs-mua

Re: proposed patches to notmuch-emacs-mua

[Tomi Ollila]

On Sat, Jul 22 2017, Joseph Mingrone wrote:

> Hi,
>
> (Apologies if you get this twice.  I am sending a second time, since
> there seems to be a problem with the notmuch@notmuchmail.org list at the
> moment.)
>
> The first patch removes a few bash-specific calls to make
> notmuch-emacs-mua POSIX compliant.  This is useful for systems that do
> not include bash in the base system.  The downside is that, instead of
> using a built-in bash feature, it calls sed.
>
> The second patch deals with how file contents are inserted into the
> message body.  It makes the insertion happen just before the signature
> (if one exists) instead of at the very beginning of the message.  This
> is useful when users have hooks to insert MML at the beginning of the
> message.
>
> The third patch includes a few trivial changes to silence a few
> shellcheck.net warnings.

Good stuff! comments inline (as usual).

>
> Joseph
>
> From 8f7b2b0f95a1d08320604984866069d910bd2eb1 Mon Sep 17 00:00:00 2001
> From: Joseph Mingrone <jrm@ftfl.ca>
> Date: Thu, 20 Jul 2017 20:23:39 -0300
> Subject: [PATCH 1/3] Remove bash-specific calls to make notmuch-emacs-mua
>  POSIX compliant
>
> ---
>  emacs/notmuch-emacs-mua | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch-emacs-mua b/emacs/notmuch-emacs-mua
> index a5214977..1af2f413 100755
> --- a/emacs/notmuch-emacs-mua
> +++ b/emacs/notmuch-emacs-mua
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/bin/sh
>  #
>  # notmuch-emacs-mua - start composing a mail on the command line
>  #
> @@ -26,8 +26,8 @@ set -eu
>  # calling convention: escape -v var "$arg" (like in bash printf).
>  escape ()
>  {
> -    local __escape_arg__=${3//\\/\\\\}
> -    printf -v $2 '%s' "${__escape_arg__//\"/\\\"}"
> +    __escape_arg__="$(printf '%s' "$3" | sed -e 's|\\|\\\\|g' -e 's|\"|\\\"|g')"
> +    eval "$2=\$__escape_arg__"

I'd like to know why bash is to be replaced; (e.g. since it is not in base
system, but so not is e.g. emacs...)

if this couple of order of magnitude heavier solution is used, then it
could be first checked whether to do so; e.g using case $3 in *['"\']*) and
and then do escaping on the need basis (an option to use posix shell
constructs to do such a thing looks probably too complicated... (*))

>  }
>  
>  EMACS=${EMACS:-emacs}
> -- 
> 2.13.3
>
> From e44343e36b5591c3ffbb150c64da4855aa9127f0 Mon Sep 17 00:00:00 2001
> From: Joseph Mingrone <jrm@ftfl.ca>
> Date: Thu, 20 Jul 2017 20:28:06 -0300
> Subject: [PATCH 2/3] Use message-goto-signature to insert file in message body
>
> Users may call, e.g., mml-secure-message-sign to insert MML at the top
> of the message.  By using message-goto-signature to insert file
> contents into the body of the message, the MML remains at the top.

looks reasonable... this probably doesn't https://xkcd.com/1172/ ...

> ---
>  emacs/notmuch-emacs-mua | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-emacs-mua b/emacs/notmuch-emacs-mua
> index 1af2f413..521dd342 100755
> --- a/emacs/notmuch-emacs-mua
> +++ b/emacs/notmuch-emacs-mua
> @@ -95,7 +95,7 @@ while getopts :s:c:b:i:h opt; do
>  	    ELISP="${ELISP} (message-goto-bcc) (insert \"${OPTARG}, \")"
>  	    ;;
>  	--body|i)
> -	    ELISP="${ELISP} (message-goto-body) (insert-file \"${OPTARG}\")"
> +	    ELISP="${ELISP} (message-goto-signature) (insert-file \"${OPTARG}\")"
>  	    ;;
>  	--print)
>  	    PRINT_ONLY=1
> -- 
> 2.13.3
>
> From b552bdaad1686256ca1da388be0c714d4f0974f0 Mon Sep 17 00:00:00 2001
> From: Joseph Mingrone <jrm@ftfl.ca>
> Date: Thu, 20 Jul 2017 20:35:24 -0300
> Subject: [PATCH 3/3] Trivial changes to silence a few shellcheck.net warnings
>
> ---
>  emacs/notmuch-emacs-mua | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/emacs/notmuch-emacs-mua b/emacs/notmuch-emacs-mua
> index 521dd342..c66a0c00 100755
> --- a/emacs/notmuch-emacs-mua
> +++ b/emacs/notmuch-emacs-mua
> @@ -150,14 +150,14 @@ if [ -n "${MAILTO}" ]; then
>  	exit 1
>      fi
>      ELISP="(browse-url-mail \"${MAILTO}\")"
> -elif [ -z "${ELISP}" -a -n "${HELLO}" ]; then
> +elif [ -z "${ELISP}" ] && [ -n "${HELLO}" ]; then
>      ELISP="(notmuch)"
>  else
>      ELISP="(notmuch-mua-new-mail) ${ELISP}"
>  fi
>  
>  # Kill the terminal/frame if we're creating one.
> -if [ -z "$USE_EMACSCLIENT" -o -n "$CREATE_FRAME" -o -n "$NO_WINDOW" ]; then
> +if [ -z "$USE_EMACSCLIENT" ] || [ -n "$CREATE_FRAME" ] || [ -n "$NO_WINDOW" ]; then
>      ELISP="${ELISP} (message-add-action #'save-buffers-kill-terminal 'exit)"
>  fi
>  
> @@ -167,13 +167,13 @@ escape -v pwd "$PWD"
>  ELISP="(prog1 'done (require 'notmuch) (cd \"$pwd\") ${ELISP})"
>  
>  if [ -n "$PRINT_ONLY" ]; then
> -    echo ${ELISP}
> +    echo "${ELISP}"
>      exit 0
>  fi

The changes in this 3/3 up to this point looks good, but the change below
is somewhat controversial. It is convenient that $EMACS is split to command
and arguments on $IFS variables (space, tab, newline). Some tools use
similar approach; e.g. I just used GIT_SSH_COMMAND='ssh -vvv' git pull to 
debug failing ssh login (it did not help, sudo strace -f -p <sshd-pid> was
to the rescue this time ;)

>  
>  if [ -n "$USE_EMACSCLIENT" ]; then
>      # Evaluate the progn.
> -    exec ${EMACSCLIENT} ${NO_WINDOW} ${CREATE_FRAME} ${AUTO_DAEMON} --eval "${ELISP}"
> +    exec "${EMACSCLIENT}" ${NO_WINDOW} ${CREATE_FRAME} ${AUTO_DAEMON} --eval "${ELISP}"
>  else
> -    exec ${EMACS} ${NO_WINDOW} --eval "${ELISP}"
> +    exec "${EMACS}" ${NO_WINDOW} --eval "${ELISP}"
>  fi
> -- 
> 2.13.3


Tomi


(*) this might work
        r=$3 p=
        while case $r in *[\\]*) true ;; *) false ;; esac; do
              p=$p${r%%\\*}\\\\ r=${r#*\\}
        done 
        test -z "$p" || r=$p$r

and the same for "

Re: proposed patches to notmuch-emacs-mua

[Tomi Ollila]

On Sat, Aug 19 2017, Joseph Mingrone wrote:

> Hello Tomi,
>
> Tomi Ollila <tomi.ollila@iki.fi> writes:
>> I'd like to know why bash is to be replaced; (e.g. since it is not in base
>> system, but so not is e.g. emacs...)
>
>> if this couple of order of magnitude heavier solution is used, then it
>> could be first checked whether to do so; e.g using case $3 in *['"\']*) and
>> and then do escaping on the need basis (an option to use posix shell
>> constructs to do such a thing looks probably too complicated... (*))
>
> Indeed, bash could be pulled in as a dependency to notmuch, but I'm
> shell shocked to hear you say the POSIX sh solution is an order of
> magnitude heavier than the bash solution.  

if you try strace -ff -o forks -e trace=execve sh -c 'echo $(printf %s\\n foo | sed s/o/a/)'

you get 4 files. one is surrounding sh and 3 are forks creating that
pipeline. in one of those 3 forks sed(1) is execve'd. it takes some time
to execute these system calls instead of just doing internal bash string
handling.

But we'd get (possibly, YMMV) clearer solution my just doing

case $3 in 
    *['"\']*)
        __escape_arg__=$(printf '%s' "$3" | sed -e 's|\\|\\\\|g' -e 's|\"|\\\"|g')
        eval $2=\$__escape_arg__
        ;;
    *)
       eval $2=\$3
esac

i.e. execute pipeline only when there is something to be escaped...

> Building on your suggestion,
> maybe this function is a reasonable alternative that doesn't call an
> external command.
>
> escape ()
> {
>   r=$3 p=
>   while case $r in *\\*) true ;; *) false ;; esac; do
>     p=$p${r%%\\*}\\\\ r=${r#*\\}
>   done
>   r="$p$r" p=
>
>   while case $r in *\"*) true ;; *) false ;; esac; do
>     p=$p${r%%\"*}\\\" r=${r#*\"}
>   done
>
>   eval "$2=\$p\$r"
> }
>
> It's no big deal either way.  I just followed a convention when updating
> the FreeBSD notmuch package.  If the script is close to POSIX
> compliance, then patch out the bashisms, otherwise pull in bash.  Why
> not attempt to comply to a standard and not pull in an external
> dependency (for some systems) when the script is already so close?  I
> thought I would upstream those changes in case they were useful.
>
>>> Users may call, e.g., mml-secure-message-sign to insert MML at the top
>>> of the message.  By using message-goto-signature to insert file
>>> contents into the body of the message, the MML remains at the top.
>
>> looks reasonable... this probably doesn't https://xkcd.com/1172/ ...
>
> You are targeting Emacs mail users.  Is it an esoteric workflow to,
> e.g., sign and encrypt mail messages?

It sure is not ... :D


>
>> The changes in this 3/3 up to this point looks good, but the change below
>> is somewhat controversial. It is convenient that $EMACS is split to command
>> and arguments on $IFS variables (space, tab, newline). Some tools use
>> similar approach; e.g. I just used GIT_SSH_COMMAND='ssh -vvv' git pull to 
>> debug failing ssh login (it did not help, sudo strace -f -p <sshd-pid> was
>> to the rescue this time ;)
>
>>>  if [ -n "$USE_EMACSCLIENT" ]; then
>>>      # Evaluate the progn.
>>> -    exec ${EMACSCLIENT} ${NO_WINDOW} ${CREATE_FRAME} ${AUTO_DAEMON} --eval "${ELISP}"
>>> +    exec "${EMACSCLIENT}" ${NO_WINDOW} ${CREATE_FRAME} ${AUTO_DAEMON} --eval "${ELISP}"
>>>  else
>>> -    exec ${EMACS} ${NO_WINDOW} --eval "${ELISP}"
>>> +    exec "${EMACS}" ${NO_WINDOW} --eval "${ELISP}"
>>>  fi
>>> -- 
>>> 2.13.3
>
> Agreed.  I take back this suggested change.
>
> Regards,
>
> Joseph

Re: proposed patches to notmuch-emacs-mua

[Jani Nikula]

On Sat, 19 Aug 2017, Joseph Mingrone <jrm@ftfl.ca> wrote:
> Hello Tomi,
>
> Tomi Ollila <tomi.ollila@iki.fi> writes:
>> I'd like to know why bash is to be replaced; (e.g. since it is not in base
>> system, but so not is e.g. emacs...)
>
>> if this couple of order of magnitude heavier solution is used, then it
>> could be first checked whether to do so; e.g using case $3 in *['"\']*) and
>> and then do escaping on the need basis (an option to use posix shell
>> constructs to do such a thing looks probably too complicated... (*))
>
> Indeed, bash could be pulled in as a dependency to notmuch, but I'm
> shell shocked to hear you say the POSIX sh solution is an order of
> magnitude heavier than the bash solution.  Building on your suggestion,
> maybe this function is a reasonable alternative that doesn't call an
> external command.
>
> escape ()
> {
>   r=$3 p=
>   while case $r in *\\*) true ;; *) false ;; esac; do
>     p=$p${r%%\\*}\\\\ r=${r#*\\}
>   done
>   r="$p$r" p=
>
>   while case $r in *\"*) true ;; *) false ;; esac; do
>     p=$p${r%%\"*}\\\" r=${r#*\"}
>   done
>
>   eval "$2=\$p\$r"
> }
>
> It's no big deal either way.  I just followed a convention when updating
> the FreeBSD notmuch package.  If the script is close to POSIX
> compliance, then patch out the bashisms, otherwise pull in bash.  Why
> not attempt to comply to a standard and not pull in an external
> dependency (for some systems) when the script is already so close?  I
> thought I would upstream those changes in case they were useful.

IMHO don't fix it if it isn't broken. The proposed solutions to "fix"
the bashism are more complicated than the original, and might introduce
bugs. And sticking to sh here blocks future use of useful bashisms.

BR,
Jani.