Re: [PATCH v2] git-am: fix maildir support regression for unordered headers in emails

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2009, 07:27 UTC
Message-ID: <7vljmqflti.fsf@alter.siamese.dyndns.org>
In-Reply-To: <2433101adeafddeab78815083446552ff3ea9f49.1247636959.git.nicolas.s.dev@gmx.fr>

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
	fi

if 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=mbox

But I am not convinced that I understand what _problem_ you are trying to solve in the first place.