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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nicolas Sebrecht <nicolas.s.dev@xxxxxx> 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@xxxxxx>
       Cc: <git@xxxxxxxxxxxxxxx>,
           Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>,
           Johannes Sixt <j.sixt@xxxxxxxxxxxxx>
       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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]