Re: [PATCH v2] git-am: fix maildir support regression for unordered headers in emails
Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a
> may refuse valid patches from verbatim emails.
It is unclear what you meant by "verbatim email". A verbatim e-mail in mbox begins with "From " header that is already covered in the existing code long before support for stgit/hg was added.
> + # Keep maildir workflows support.
> + # Verbatim emails may have header fields in random order.
> + is_verbatim_email='true'
We do not fold lines like this,
> + for line in "$l1" "$l2" "$l3"; do
Instead, we write like this:
for x in a b c
do
cmd ...
> + printf "$line" | grep --quiet --extended-regexp '^([^\ ])+: +.*' ||
We use GNUism spelling --extended-regexp nowhere in scripted Porcelains; just say -E here, and do not omit -e before the pattern.
Likewise for --quiet. Just say -q. When in doubt, be conservative and stick to POSIX for portability.
http://www.opengroup.org/onlinepubs/9699919799/utilities/grep.html#tag_20_55
Your regexp has too many issues.
- It is too loose and too strict at the same time. RFC 2822 section 2.2
and 3.6.8 specify that a field name must be composed of printable
US-ASCII characters except colon and space, so if you really want to be
lenient, it should instead begin with [^: ]+: (and you do not need any
capture).
- I think however starting the regexp with "^[A-Za-z]+(-[A-Za-z]+)*:"
would be more appropriate in practice, though.
- You do not need to end the expression with ".*"; omitting that would
match the same set of lines anyway.
- I thought you were advocating for not requiring SP after the colon?
- What happens if $l2 or $l3 is a subsequent folded line? For example,
in my MUA edit buffer, this message begins with:
To: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
Cc: <git@vger.kernel.org>,
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>,
Johannes Sixt <j.sixt@viscovery.net>
Subject: Re: [PATCH v2] git-am: fix maildir support ... The third line would not match your regexp.
> + is_verbatim_email='false'
> + done
> + # next treatments don't differ from mailbox format
> + [[ $is_verbatim_email == 'true' ]] && patch_format=mbox
We do not use non-portable [[ ]] anywhere in our shell script. Write
if test true = "$is_verbatim_email"
then
patch_format=mbox
fiif you really want to keep this code structure.
I actually do not think you would even need an extra is_verbatim_email variable, though. Assuming that I understand what you are trying to do, this is probably how I would write it:
sed -e '/^$/q' -e '/^[ ]/d' "$1" |
grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||
patch_format=mboxBut I am not convinced that I understand what _problem_ you are trying to solve in the first place.
