Re: [PATCH v2 04/11] contrib: remove "thunderbird-patch-inline"

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

Due to 'text/plan; format=flowed', the attached patch was damaged so
the version I may be commenting on may be slightly different from
what you wanted to show, but there is one thing I noticed.

> diff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh
> index fdcc9483520..72c37aace4e 100755
> --- a/contrib/thunderbird-patch-inline/appp.sh
> +++ b/contrib/thunderbird-patch-inline/appp.sh
> @@ -1,10 +1,12 @@
>  #!/bin/sh
>  # Copyright 2008 Lukas Sandström <luksan@gmail.com>
>  #
> -# AppendPatch - A script to be used together with ExternalEditor
> +# AppendPatch - A script to be used together with ExternalEditorRevived
>  # for Mozilla Thunderbird to properly include patches inline in e-mails.
> -# ExternalEditor can be downloaded at
>    http://globs.org/articles.php?lng=en&pg=2
> +# ExternalEditorRevived extension can be installed using the Add-ons
> +# manager in thunderbird, the source is available at
> +# https://github.com/Frederick888/external-editor-revived
>    CONFFILE=~/.appprc
>  @@ -25,8 +27,12 @@ fi
>    cd - > /dev/null
> +# The headers are separated from the message body by a blanks
> +# line. However the message uses CR LF line ending so on platforms
> +# where the native line ending is LF we see a line with a single CR.
> +SEP="$(printf '^\r\\{0,1\\}$')"

Here we assign to the variable SEP

>  SUBJECT=$(sed -n -e '/^Subject: /p' "${PATCH}")
> -HEADERS=$(sed -e '/^'"${SEP}"'$/,$d' $1)
> +HEADERS=$(sed -e "/${SEP}/"',$d' $1)

The old reference must be expecting that the variable SEP should be
already set up.  I understand that a different separator is used in
the new version, so it is perfectly fine that HEADERS need to be
changed, but shouldn't the previous hunk that assign to SEP be
removing an old assignment to SEP that gave the separator wanted by
the older version?

It turns out that after the post-context of the first hunk there is
assignment that the old separator value is assigned to SEP.  I think
that should go.

>  BODY=$(sed -e "1,/${SEP}/d" $1)
>  CMT_MSG=$(sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}")
>  DIFF=$(sed -e '1,/^---$/d' "${PATCH}")
> @@ -37,7 +43,7 @@ CCS=$(printf '%s\n%s\n' "$CMT_MSG" "$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp
>  echo "$SUBJECT" > $1
>  echo "Cc: $CCS" >> $1
>  echo "$HEADERS" | sed -e '/^Subject: /d' -e '/^Cc: /d' >> $1
> -echo "$SEP" >> $1
> +echo >> $1
>    echo "$CMT_MSG" >> $1
>  echo "---" >> $1