Re: [Quilt-dev] [PATCH] Add mail option --all-cc-cover to find all CCs and add to cover letter

[Jean Delvare]

Hi John,

Sorry for the late reply. I don't use quilt mail so I hoped somebody
else would review the patch, but that didn't happen so here I go.

I like the idea of this new option, as it avoids receiving isolated
patches without having the context. I'm pretty sure somebody else asked
for it before, although I can't find the reference at the moment.

On Mon, 17 May 2021 16:58:15 -0700, John 'Warthog9' Hawley (VMware) wrote:
> This keeps a track of existing CCs in the patches and adds them all to
> the intriduction e-mail CC if they were present.  This also introduces

Spelling: introduction

> X-Cover-Cc header to keep track of those (though it's never actually
> sent as it's substituted in for the introduction, then removed for
> subsequent patches)

Functionally, I'm wondering why this is limited to Cc:. Kernel patches
will typically include a bunch of lines referring to people, like
Signed-off-by:, Reviewed-by: and Acked-by:. Isn't it odd that people
listed as Cc: would receive the cover letter while people directly
involved in the patches wouldn't? What would be the use case?

Looking at the current code, I see that we have a
quilt_mail_patch_filter() function which deals with these lines in
patches already. So I believe that the right approach to the problem
would be to reuse this function on a temporary file (either a dedicated
one, or maybe the cover letter itself) where all these lines have been
collected from the individual patches. That way we are guaranteed to
have consistent Cc lists between patches and the cover letter.

I'm still reviewing your patch to clarify technical details, but
overall I suspect it's better to write a new one based on the idea
above.

> Issue was raised here:
> https://lore.kernel.org/ksummit/20210422092916.556e5e50@gandalf.local.home/
> 
> and this brings (rough) feature parity on this front with git send-email
> 
> Signed-off-by: John 'Warthog9' Hawley (VMware) <warthog9@eaglescrag.net>
> ---
>  quilt/mail.in | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/quilt/mail.in b/quilt/mail.in
> index b2cbe5a..b174928 100644
> --- a/quilt/mail.in
> +++ b/quilt/mail.in
> @@ -59,6 +59,10 @@ first, and a last patch name of \`-' denotes the last 
> patch in the series.
>  --to, --cc, --bcc
>       Append a recipient to the To, Cc, or Bcc header.
>  
> +--all-cc-cover
> +     Find all Cc's in the patches and also include them in the Cc on
> +     the cover letter only.

"only" is ambiguous, I think the sentence is clearer without it.

> +
>  --charset
>       Specify a particular message encoding on systems which don't use
>       UTF-8 or ISO-8859-15. This character encoding must match the one
> @@ -146,6 +150,17 @@ process_mail()

Function process_mail() is being called once for the cover letter and
then once for each patch. As I understand it, only the cover letter (or
introduction) has the X-Cover-Cc field. So wouldn't it be more
efficient (and overall cleaner) to handle it in a dedicated function
that's only called for the cover letter and not the individual patches?

>  
>       cat > $tmpfile
>  
> +     grep -q "^X-Cover-Cc" $tmpfile

To be on the safe side, the pattern should include the trailing ":".

> +     xcc_status=$?
> +     if [ "${xcc_status}" = "0" ]

The shell is bash and bash doesn't need as much quoting as sh. Hence

        if [ $xcc_status = 0 ]

is guaranteed to work, easier to read, and faster to parse.

> +     then
> +             sed -i -e '/^Cc:/d' -e 's/^X-Cover-Cc:/Cc:/' $tmpfile
> +     fi
> +
> +     local tmp="$( cat $tmpfile | $QUILT_DIR/scripts/edmail --charset 
> $opt_charset \
> +               --remove-empty-headers Cc )"

By convention, we declare local variables at the top of each function
or block where the variable is used. I'm not a big fan of storing whole
files in variables though, I'm pretty sure we had problems with that in
the past, although I can't remember the details (probably had to do
with line endings or white space not being preserved). Plus you have no
control over your memory consumption, the file could be huge.

I'm also questioning the rationale behind that call to edmail. You just
set the Cc: field yourself. If it ends up being empty, why did you do
that in the first place?

> +     echo "${tmp}" > $tmpfile
> +
>       set -- $($QUILT_DIR/scripts/edmail --charset $opt_charset \
>                                 --extract-recipients To \
>                                 --extract-recipients Cc \
> @@ -165,6 +180,12 @@ process_mail()
>               sed -e 's/^From />From /' $tmpfile
>               echo
>       fi
> +
> +     if [ "${xcc_status}" = "0" ]
> +     then
> +             sed -i '/^X-Cover-Cc:/d' $introduction
> +     fi
> +
>       rm -f $tmpfile
>  }
>  
> @@ -181,6 +202,7 @@ join_lines()
>  
>  options=`getopt -o m:M:h \
>               --long from:,to:,cc:,bcc:,subject: \
> +             --long all-cc-cover, \

Trailing comma is not needed.

>               --long send,mbox:,charset:,sender: \
>               --long prefix:,reply-to:,signature: -- "$@"`
>  
> @@ -231,6 +253,9 @@ do
>       --bcc)
>               opt_bcc[${#opt_bcc[@]}]=$2
>               shift 2 ;;
> +     --all-cc-cover)
> +             opt_all_cc_cover=1
> +             shift ;;
>       --subject)
>               opt_subject=$2
>               shift 2 ;;
> @@ -467,8 +492,24 @@ $"Unable to extract a subject header from %s\n" 
> "$(print_patch "$patch")" >&2
>               exit 1
>       fi
>       subjects[${#subjects[@]}]="$patch"$'\t'"$subject"
> +     # find al the CC lines for introduction

Spelling: all

> +     xccs="$( echo "${xccs}
> +$( cat_file "${patch_file}" | grep "^Cc: " | sed -e 's/^Cc:[ \t]*//' )" | 
> grep -o '[[:alnum:]+\.\_\-]*@[[:alnum:]+\.\_\-]*' | uniq | grep -v "^$" )"

Looks inefficient to me. You are running the whole pipeline for each
patch, and it isn't cheap. You only need to collect the Cc's from each
patch. The merging of all Cc's can be done just once at the very end.

I don't think your code actually works. "uniq" doesn't filter out all
duplicates, only consecutive ones. You would need to use "sort -u"
instead, or an awk script maybe.

Not sure what's the purpose of the last "grep" BTW, how could there be
an empty line? Either way, that should be prevented upfront, rather
than worked around afterward.

Lastly, the above only extracts the email addresses, not the names.
While this is better than nothing, it's not ideal. For example, I have
a spam filter that tags all incoming email that doesn't have my name
before my email as spam (under the assumption that anyone who has a
legitimate reason to write to me would know my name).

That extraction also isn't very robust, a maliciously formatted Cc
could set it astray. For example "teddybe@r" <ted@example.com> would
include "teddybe@r" in the list.

>  done
>  
> +cover_cc=("${opt_cc[@]}")

This should go in an else block below, to make it clear that this is
only meaningful when --all-cc-cover isn't used.

> +if [ "${opt_all_cc_cover}" = "1" ]
> +then
> +     xccs="$( echo "$(IFS="
> +";echo "${cover_cc[*]}")
> +${xccs}" | uniq | grep -v "^$" )"

OK, so you are filtering duplicates here, then I can't see why you also
did that in the patches loop above.

> +     cover_cc=()
> +     for cc in ${xccs}
> +     do
> +             cover_cc+=("${cc}")
> +     done
> +fi
> +
>  dup_subjects=( $(
>       printf "%s\n" "${subjects[@]}" \
>       | sort -k2 \
> @@ -515,6 +556,7 @@ introduction="$(gen_tempfile)"
>       From: ${opt_from:-$opt_sender}
>       To: $(IFS=,; echo "${opt_to[*]}")
>       Cc: $(IFS=,; echo "${opt_cc[*]}")
> +     X-Cover-Cc: $(IFS=,; echo "${cover_cc[*]}")
>       Bcc: $(IFS=,; echo "${opt_bcc[*]}")
>       EOF
>  
> @@ -580,7 +622,7 @@ sed -e $'s/^\\(Subject:[ \t]\\)/\\1'"$p"'/' \
>      -e '/^Subject-Prefix:/d' \
>  $introduction \
>  | $QUILT_DIR/scripts/edmail --charset $opt_charset \
> -               --remove-empty-headers To Cc Bcc \
> +               --remove-empty-headers To Bcc \

Why?

>  | process_mail
>  
>  if [ -n "$opt_mbox" ]

Thanks,
-- 
Jean Delvare
SUSE L3 Support