[PATCH v4] Thunderbird: fix appp.sh format problems

[PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

The current script has got the following problems:

1) It doesn't work if the language used by Thunderbird is not english;
2) The field To: filled by format-patch is not evaluated;
3) The field Cc: is loaded from Cc used in the commit message
instead of using the Cc field filled by format-patch in the email
header.

Added comments for point 1), added parsing of To: for point 2) and
added parsing of Cc: in the email header for point 3), removing the
Cc: parsing from commit message.

Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---

v4: create a tmp file to allow correct perl parsing
v3: parse only To: and Cc: in the email header, fix some comments
v2: changed the commit message to reflect better the script implementation
v1: first draft

 contrib/thunderbird-patch-inline/appp.sh |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh
index 5eb4a51..0daeb29 100755
--- a/contrib/thunderbird-patch-inline/appp.sh
+++ b/contrib/thunderbird-patch-inline/appp.sh
@@ -6,6 +6,9 @@
 
 # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2
 
+# NOTE: You must change some words in this script according to the language
+# used by Mozilla Thunderbird, as <Subject>, <To>, <Don't remove this line>.
+
 CONFFILE=~/.appprc
 
 SEP="-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=-"
@@ -26,17 +29,32 @@ fi
 cd - > /dev/null
 
 SUBJECT=`sed -n -e '/^Subject: /p' "${PATCH}"`
-HEADERS=`sed -e '/^'"${SEP}"'$/,$d' $1`
 BODY=`sed -e "1,/${SEP}/d" $1`
 CMT_MSG=`sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}"`
 DIFF=`sed -e '1,/^---$/d' "${PATCH}"`
+MAILHEADER=`sed '/^$/q' "${PATCH}"`
+PATCHTMP="${PATCH}.tmp"
+
+echo $MAILHEADER > $PATCHTMP
+
+export PATCHTMP
+CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
+close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
+print $addr;'`
+
+TO=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
+close FILE; $addr = $1 if $text =~ /To: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
+print $addr;'`
 
-CCS=`echo -e "$CMT_MSG\n$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp' \
-	-e 's/^Signed-off-by: \(.*\)/\1,/gp'`
+rm -rf $PATCHTMP
 
+# Change Subject: before next line according to Thunderbird language
+# for example:
+# SUBJECT=`echo $SUBJECT | sed -e 's/Subject/Oggetto/g'`
 echo "$SUBJECT" > $1
+# Change To: according to Thunderbird language
+echo "To: $TO" >> $1
 echo "Cc: $CCS" >> $1
-echo "$HEADERS" | sed -e '/^Subject: /d' -e '/^Cc: /d' >> $1
 echo "$SEP" >> $1
 
 echo "$CMT_MSG" >> $1
-- 
1.7.3.4

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> The current script has got the following problems:
>
> 1) It doesn't work if the language used by Thunderbird is not english;
> 2) The field To: filled by format-patch is not evaluated;
> 3) The field Cc: is loaded from Cc used in the commit message
> instead of using the Cc field filled by format-patch in the email
> header.
>
> Added comments for point 1), added parsing of To: for point 2) and
> added parsing of Cc: in the email header for point 3), removing the
> Cc: parsing from commit message.
>
> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
> ---

Thanks. [+cc Lukas].

A few new issues your patch introduced:

 - MAILHEADER is only set once to read from sed, and then used once
   to be echoed to another file.  Just send sed output to the file.

 - The "s/Subject/Oggetto/g" bit in my previous review.

(find the fix-up at the end).

>
> v4: create a tmp file to allow correct perl parsing
> v3: parse only To: and Cc: in the email header, fix some comments
> v2: changed the commit message to reflect better the script implementation
> v1: first draft
>
>  contrib/thunderbird-patch-inline/appp.sh |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh
> index 5eb4a51..0daeb29 100755
> --- a/contrib/thunderbird-patch-inline/appp.sh
> +++ b/contrib/thunderbird-patch-inline/appp.sh
> @@ -6,6 +6,9 @@
>  
>  # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2
>  
> +# NOTE: You must change some words in this script according to the language
> +# used by Mozilla Thunderbird, as <Subject>, <To>, <Don't remove this line>.
> +
>  CONFFILE=~/.appprc
>  
>  SEP="-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=-"
> @@ -26,17 +29,32 @@ fi
>  cd - > /dev/null
>  
>  SUBJECT=`sed -n -e '/^Subject: /p' "${PATCH}"`
> -HEADERS=`sed -e '/^'"${SEP}"'$/,$d' $1`
>  BODY=`sed -e "1,/${SEP}/d" $1`
>  CMT_MSG=`sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}"`
>  DIFF=`sed -e '1,/^---$/d' "${PATCH}"`
> +MAILHEADER=`sed '/^$/q' "${PATCH}"`
> +PATCHTMP="${PATCH}.tmp"
> +
> +echo $MAILHEADER > $PATCHTMP
> +
> +export PATCHTMP
> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
> +print $addr;'`
> +
> +TO=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
> +close FILE; $addr = $1 if $text =~ /To: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
> +print $addr;'`
>  
> -CCS=`echo -e "$CMT_MSG\n$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp' \
> -	-e 's/^Signed-off-by: \(.*\)/\1,/gp'`
> +rm -rf $PATCHTMP
>  
> +# Change Subject: before next line according to Thunderbird language
> +# for example:
> +# SUBJECT=`echo $SUBJECT | sed -e 's/Subject/Oggetto/g'`
>  echo "$SUBJECT" > $1
> +# Change To: according to Thunderbird language
> +echo "To: $TO" >> $1
>  echo "Cc: $CCS" >> $1
> -echo "$HEADERS" | sed -e '/^Subject: /d' -e '/^Cc: /d' >> $1
>  echo "$SEP" >> $1
>  
>  echo "$CMT_MSG" >> $1


I also wonder what would happen if To: and Cc: in the input were
split into continuation lines, but that was already present in the
version before your patch, so the attached fix-up won't touch that
part, but you may want to think about it.

 contrib/thunderbird-patch-inline/appp.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git c/contrib/thunderbird-patch-inline/appp.sh w/contrib/thunderbird-patch-inline/appp.sh
index 0daeb29..18d8bfa 100755
--- c/contrib/thunderbird-patch-inline/appp.sh
+++ w/contrib/thunderbird-patch-inline/appp.sh
@@ -11,6 +11,7 @@
 
 CONFFILE=~/.appprc
 
+# Change "Dont' remove this line" to what Thunderbird writs for your language
 SEP="-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=-"
 if [ -e "$CONFFILE" ] ; then
 	LAST_DIR=`grep -m 1 "^LAST_DIR=" "${CONFFILE}"|sed -e 's/^LAST_DIR=//'`
@@ -32,10 +33,8 @@ SUBJECT=`sed -n -e '/^Subject: /p' "${PATCH}"`
 BODY=`sed -e "1,/${SEP}/d" $1`
 CMT_MSG=`sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}"`
 DIFF=`sed -e '1,/^---$/d' "${PATCH}"`
-MAILHEADER=`sed '/^$/q' "${PATCH}"`
 PATCHTMP="${PATCH}.tmp"
-
-echo $MAILHEADER > $PATCHTMP
+sed '/^$/q' "${PATCH}" >"$PATCHTMP"
 
 export PATCHTMP
 CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
@@ -50,7 +49,7 @@ rm -rf $PATCHTMP
 
 # Change Subject: before next line according to Thunderbird language
 # for example:
-# SUBJECT=`echo $SUBJECT | sed -e 's/Subject/Oggetto/g'`
+# SUBJECT=`echo $SUBJECT | sed -e 's/Subject:/Oggetto:/'`
 echo "$SUBJECT" > $1
 # Change To: according to Thunderbird language
 echo "To: $TO" >> $1

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

Il 31/08/2012 19:08, Junio C Hamano ha scritto:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> The current script has got the following problems:
>>
>> 1) It doesn't work if the language used by Thunderbird is not english;
>> 2) The field To: filled by format-patch is not evaluated;
>> 3) The field Cc: is loaded from Cc used in the commit message
>> instead of using the Cc field filled by format-patch in the email
>> header.
>>
>> Added comments for point 1), added parsing of To: for point 2) and
>> added parsing of Cc: in the email header for point 3), removing the
>> Cc: parsing from commit message.
>>
>> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
>> ---
>
> Thanks. [+cc Lukas].
>
> A few new issues your patch introduced:
>
>   - MAILHEADER is only set once to read from sed, and then used once
>     to be echoed to another file.  Just send sed output to the file.
>
>   - The "s/Subject/Oggetto/g" bit in my previous review.
>
> (find the fix-up at the end).
>
>>
>> v4: create a tmp file to allow correct perl parsing
>> v3: parse only To: and Cc: in the email header, fix some comments
>> v2: changed the commit message to reflect better the script implementation
>> v1: first draft
>>
>>   contrib/thunderbird-patch-inline/appp.sh |   26 ++++++++++++++++++++++----
>>   1 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh
>> index 5eb4a51..0daeb29 100755
>> --- a/contrib/thunderbird-patch-inline/appp.sh
>> +++ b/contrib/thunderbird-patch-inline/appp.sh
>> @@ -6,6 +6,9 @@
>>
>>   # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2
>>
>> +# NOTE: You must change some words in this script according to the language
>> +# used by Mozilla Thunderbird, as <Subject>, <To>, <Don't remove this line>.
>> +
>>   CONFFILE=~/.appprc
>>
>>   SEP="-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=-"
>> @@ -26,17 +29,32 @@ fi
>>   cd - > /dev/null
>>
>>   SUBJECT=`sed -n -e '/^Subject: /p' "${PATCH}"`
>> -HEADERS=`sed -e '/^'"${SEP}"'$/,$d' $1`
>>   BODY=`sed -e "1,/${SEP}/d" $1`
>>   CMT_MSG=`sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}"`
>>   DIFF=`sed -e '1,/^---$/d' "${PATCH}"`
>> +MAILHEADER=`sed '/^$/q' "${PATCH}"`
>> +PATCHTMP="${PATCH}.tmp"
>> +
>> +echo $MAILHEADER > $PATCHTMP
>> +
>> +export PATCHTMP
>> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
>> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
>> +print $addr;'`
>> +
>> +TO=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
>> +close FILE; $addr = $1 if $text =~ /To: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
>> +print $addr;'`
>>
>> -CCS=`echo -e "$CMT_MSG\n$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp' \
>> -	-e 's/^Signed-off-by: \(.*\)/\1,/gp'`
>> +rm -rf $PATCHTMP
>>
>> +# Change Subject: before next line according to Thunderbird language
>> +# for example:
>> +# SUBJECT=`echo $SUBJECT | sed -e 's/Subject/Oggetto/g'`
>>   echo "$SUBJECT" > $1
>> +# Change To: according to Thunderbird language
>> +echo "To: $TO" >> $1
>>   echo "Cc: $CCS" >> $1
>> -echo "$HEADERS" | sed -e '/^Subject: /d' -e '/^Cc: /d' >> $1
>>   echo "$SEP" >> $1
>>
>>   echo "$CMT_MSG" >> $1
>
>
> I also wonder what would happen if To: and Cc: in the input were
> split into continuation lines, but that was already present in the

Do you mean To: <mail1>,.....<mailN>\nCc: <mail1>,.....<mailN>?

> version before your patch, so the attached fix-up won't touch that
> part, but you may want to think about it.

Actually I'm trying the script in two ways: with --to and --cc of git 
format patch => multilines, and with a little script (see below) to have 
automatically Cc: from other script, in this case get_maintainer.pl from 
Linux kernel source tree, and it works perfectly. In the last case Cc: 
mail addresses are on the same line.
Maybe we can add even this script, but maybe it's too kernel-specific.


#!/bin/bash

if [[ ! $# -eq 1 ]]
then
         echo "Usage: command <PATH>, where PATH contains kernel and 
patches"
         exit
fi

PATCHPATH=${1%/}
for i in `ls $PATCHPATH/*.patch`
do

         CCN=`grep "Cc:" $i | wc -l`
         if [ $CCN -ge 1 ]
         then
                 echo "Cc: list already present, skip..."
         else

                 CCLIST=`$PATCHPATH/scripts/get_maintainer.pl 
--norolestats --no-git --separator , $i`

                 sed -n -e "/^$/,9999999 ! p" $i > $i.new
                 echo "Cc: $CCLIST" >> $i.new
                 sed -n -e "/^$/,9999999 p" $i >> $i.new
                 mv $i.new $i
         fi
done

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

>> I also wonder what would happen if To: and Cc: in the input were
>> split into continuation lines, but that was already present in the
>
> Do you mean To: <mail1>,.....<mailN>\nCc: <mail1>,.....<mailN>?

No, I meant "To: <mail1>,...\n <mailN>\n".

But see my response to J6t's message.

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> +PATCHTMP="${PATCH}.tmp"
> +
> +echo $MAILHEADER > $PATCHTMP
> +
> +export PATCHTMP
> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
> +print $addr;'`
> +
> +TO=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
> +close FILE; $addr = $1 if $text =~ /To: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
> +print $addr;'`
>  
> -CCS=`echo -e "$CMT_MSG\n$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp' \
> -	-e 's/^Signed-off-by: \(.*\)/\1,/gp'`
> +rm -rf $PATCHTMP

One thing I forgot to say.  Make sure you quote your variables (when
in doubt, quote).  Also make it a habit not to say "rm -r" when you
know you are removing a regular file you created.  I.e.

	rm -f "$PATCHTMP"

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Johannes Sixt]

Am 31.08.2012 16:09, schrieb Marco Stornelli:
> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
> +print $addr;'`

The quoting is broken in this line (sq within sq does not work).

Am I correct that you intend to treat continuation lines with this
non-trivial perl script? All this processing gets tedious and
unreadable. Let me suggest this pattern instead:

# translate to Thunderbird language
LANG_TO="To:"
LANG_SUBJ="Subject:"
LANG_CC="Cc:"

LF=	# terminates the _previous_ line
while read -r line
do
	case $line in
	'To: '*)
		printf "${LF}%s" "$LANG_TO ${line#To: }"
		;;
	'Cc: '*) ...similar...
	'Subject: '*) ...similar...
	' '*)	# continuation line
		printf "%s" "$line"
		;;
	'')
		print "${LF}\n"
		cat
		;;
	esac
	LF='\n'
done <"$PATCH" >"$1"

Instead of printing right away, you can also accumulate the data in
variables and print them right before the 'cat'. I would do that only if
a particular order of To:, Cc:, and Subject: is required in the output.

(I don't know how the "do not delete this line" line fits in the
picture, but I'm sure you can figure it out.)

-- Hannes

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

Il 31/08/2012 23:35, Johannes Sixt ha scritto:
> Am 31.08.2012 16:09, schrieb Marco Stornelli:
>> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
>> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
>> +print $addr;'`
>
> The quoting is broken in this line (sq within sq does not work).

I don't understand what you mean, I'm using this script and it works 
perfectly.

>
> Am I correct that you intend to treat continuation lines with this
> non-trivial perl script? All this processing gets tedious and
> unreadable. Let me suggest this pattern instead:

The perl script is used to have To: and Cc: email addresses in single 
line, without using \n. I tried with sed, but it's a little bit 
difficult (at least for me) because sed usually works on a single line, 
it's tricky with multilines.

>
> # translate to Thunderbird language
> LANG_TO="To:"
> LANG_SUBJ="Subject:"
> LANG_CC="Cc:"
>
> LF=	# terminates the _previous_ line
> while read -r line
> do
> 	case $line in
> 	'To: '*)
> 		printf "${LF}%s" "$LANG_TO ${line#To: }"
> 		;;
> 	'Cc: '*) ...similar...
> 	'Subject: '*) ...similar...
> 	' '*)	# continuation line
> 		printf "%s" "$line"
> 		;;
> 	'')
> 		print "${LF}\n"
> 		cat
> 		;;
> 	esac
> 	LF='\n'
> done <"$PATCH" >"$1"
>
> Instead of printing right away, you can also accumulate the data in
> variables and print them right before the 'cat'. I would do that only if
> a particular order of To:, Cc:, and Subject: is required in the output.
>
> (I don't know how the "do not delete this line" line fits in the
> picture, but I'm sure you can figure it out.)
>
> -- Hannes
>
>

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Johannes Sixt]

Am 01.09.2012 09:43, schrieb Marco Stornelli:
> Il 31/08/2012 23:35, Johannes Sixt ha scritto:
>> Am 31.08.2012 16:09, schrieb Marco Stornelli:
>>> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'};
>>> $text=<FILE>;
>>> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr
>>> =~ s/\n//g;
>>> +print $addr;'`
>>
>> The quoting is broken in this line (sq within sq does not work).
> 
> I don't understand what you mean, I'm using this script and it works
> perfectly.

Look how you write:

  perl -e '... $ENV{'PATCHTMP'} ...'

That is, perl actually sees this script:

  ... $ENV{PATCHTMP} ...

(no quotes around PATCHTMP). That my be perfectly valid perl, but is not
what you intended.

-- Hannes

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

Il 01/09/2012 15:59, Johannes Sixt ha scritto:
> Am 01.09.2012 09:43, schrieb Marco Stornelli:
>> Il 31/08/2012 23:35, Johannes Sixt ha scritto:
>>> Am 31.08.2012 16:09, schrieb Marco Stornelli:
>>>> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'};
>>>> $text=<FILE>;
>>>> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr
>>>> =~ s/\n//g;
>>>> +print $addr;'`
>>>
>>> The quoting is broken in this line (sq within sq does not work).
>>
>> I don't understand what you mean, I'm using this script and it works
>> perfectly.
>
> Look how you write:
>
>    perl -e '... $ENV{'PATCHTMP'} ...'
>
> That is, perl actually sees this script:
>
>    ... $ENV{PATCHTMP} ...
>
> (no quotes around PATCHTMP). That my be perfectly valid perl, but is not
> what you intended.
>
> -- Hannes
>
>

I don't understand what you mean when you say "is not what you intended" 
because perl and this script does *exactly* what I want and (with the 
test I've done) it works. Have you got a test case where it doesn't work 
at all? However, since I'm not a perl guru, if someone says that double 
quotes are needed, ok the result is however the same.

Git guys, let me know if a version 6 of the patch is needed, no problem 
to submit another version.

Regards,

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> Il 01/09/2012 15:59, Johannes Sixt ha scritto:
>
>> Look how you write:
>>
>>    perl -e '... $ENV{'PATCHTMP'} ...'
>>
>> That is, perl actually sees this script:
>>
>>    ... $ENV{PATCHTMP} ...
>>
>> (no quotes around PATCHTMP). That my be perfectly valid perl, but is not
>> what you intended.
>
> I don't understand what you mean when you say "is not what you
> intended"...

When you wrote this:

    CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'};...

which one of the following did you mean to feed Perl?

 (1) open FILE, $ENV{'PATCHTMP'};
 (2) open FILE, $ENV{PATCHTMP};

The patch text makes it look as if you wanted to do (1), but what is
fed to perl is (2), as Johannes points out.

Saying:

    CCS=`perl -e 'local $/=undef; open FILE, $ENV{PATCHTMP};...

would have been more natural if you really meant to do (2), don't
you think?  So what you wrote is at least misleading.

But I think I agree with Johannes's rewrite of the loop, so this may
be a moot point.

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

2012/9/2 Junio C Hamano <gitster@pobox.com>:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> Il 01/09/2012 15:59, Johannes Sixt ha scritto:
>>
>>> Look how you write:
>>>
>>>    perl -e '... $ENV{'PATCHTMP'} ...'
>>>
>>> That is, perl actually sees this script:
>>>
>>>    ... $ENV{PATCHTMP} ...
>>>
>>> (no quotes around PATCHTMP). That my be perfectly valid perl, but is not
>>> what you intended.
>>
>> I don't understand what you mean when you say "is not what you
>> intended"...
>
> When you wrote this:
>
>     CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'};...
>
> which one of the following did you mean to feed Perl?
>
>  (1) open FILE, $ENV{'PATCHTMP'};
>  (2) open FILE, $ENV{PATCHTMP};
>
> The patch text makes it look as if you wanted to do (1), but what is
> fed to perl is (2), as Johannes points out.
>
> Saying:
>
>     CCS=`perl -e 'local $/=undef; open FILE, $ENV{PATCHTMP};...
>
> would have been more natural if you really meant to do (2), don't
> you think?  So what you wrote is at least misleading.
>
> But I think I agree with Johannes's rewrite of the loop, so this may
> be a moot point.

Ok, I'll try to rewrite it as Johannes has suggested.

Regards,

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

Il 02/09/2012 22:42, Junio C Hamano ha scritto:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> Il 01/09/2012 15:59, Johannes Sixt ha scritto:
>>
>>> Look how you write:
>>>
>>>     perl -e '... $ENV{'PATCHTMP'} ...'
>>>
>>> That is, perl actually sees this script:
>>>
>>>     ... $ENV{PATCHTMP} ...
>>>
>>> (no quotes around PATCHTMP). That my be perfectly valid perl, but is not
>>> what you intended.
>>
>> I don't understand what you mean when you say "is not what you
>> intended"...
>
> When you wrote this:
>
>      CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'};...
>
> which one of the following did you mean to feed Perl?
>
>   (1) open FILE, $ENV{'PATCHTMP'};
>   (2) open FILE, $ENV{PATCHTMP};
>
> The patch text makes it look as if you wanted to do (1), but what is
> fed to perl is (2), as Johannes points out.
>
> Saying:
>
>      CCS=`perl -e 'local $/=undef; open FILE, $ENV{PATCHTMP};...
>
> would have been more natural if you really meant to do (2), don't
> you think?  So what you wrote is at least misleading.
>
> But I think I agree with Johannes's rewrite of the loop, so this may
> be a moot point.
>

I tried the Johannes's script, but it seems it doesn't work well with 
the pattern of format-patch (To: <mail1>,\n   <mail2>,\n    <mailN>). 
The multilines are not well managed. I can change my script to double 
quotas (I think perl and regexp are much more powerful rather than the 
while loop though it can be less readable), if it's not ok, I let other 
git shell expert to adjust the script. I hope this thread it will be 
useful for others.

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> I tried the Johannes's script, but it seems it doesn't work well with
> the pattern of format-patch (To: <mail1>,\n   <mail2>,\n
> <mailN>). The multilines are not well managed.

I am guessing that the reason why Jonahhes's "copy our headers out
with continuation lines intact" approach does not work is because
Thunderbird does not want to see its own header part (i.e. that
which comes before that $SEP) contain RFC-2822 style continuation
lines.

Can you grab a typical input (i.e. the contents of what is passed as
$1 to the appp.sh script) and show us how it looks like here so that
we can take a look?  It would be fine to paste the contents, but we
might want to protect it from MUA by doing an attachment or a
pastebin URL.

It appears that the original script tries very hard to keep the
Subject: line at the beginning, but I am not sure if that is because
Thunderbird wants to read its "$1" that way, or it is just that
original appp.sh script happened to be written that way without real
reason.  If I were updating this script, what I would do would be
more like:

    0. Open a temporary output file $OUT, input filehandle $IN from
       ${PATCH}, and another input filehandle $TEMPLATE from "$1"

    1. Read from $IN starting from the second line (i.e. skip the
       first line "From [0-9a-f] Mon Sep 17 00:00:00 2001" magic
       marker) upto the empty line, coalescing the RFC-2822 style
       continuation lines into one long line each.

    2. Output the above into $OUT, while applying your "header field
       name translation" (e.g. "Oggetto") and remembering what
       header you wrote out.

    3. Read from $TEMPLATE up to a line that matches the following
       pattern:

	/^-=-=-=-=-=-=-=-=-=#.*#=-=-=-=-=-=-=-=-=-$/

       reject headers that step 2. already emitted, and emit other
       headers to $OUT.  Also emit that line that matched the above
       SEP pattern to $OUT (by doing so, you do not have to care how
       "Don't remove this line" is spelled in the user's language).

    4. Resume reading from $IN and copy to $OUT, including the patch
       separator "---", and stop copying.

    5. Read the remainder of $TEMPLATE and copy to $OUT.

    6. Read the remainder of $IN and copy to $OUT.

    7. Rename the temporary file that was connected to $OUT to "$1".


       

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

2012/9/3 Junio C Hamano <gitster@pobox.com>:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> I tried the Johannes's script, but it seems it doesn't work well with
>> the pattern of format-patch (To: <mail1>,\n   <mail2>,\n
>> <mailN>). The multilines are not well managed.
>
> I am guessing that the reason why Jonahhes's "copy our headers out
> with continuation lines intact" approach does not work is because
> Thunderbird does not want to see its own header part (i.e. that
> which comes before that $SEP) contain RFC-2822 style continuation
> lines.
>
> Can you grab a typical input (i.e. the contents of what is passed as
> $1 to the appp.sh script) and show us how it looks like here so that
> we can take a look?  It would be fine to paste the contents, but we
> might want to protect it from MUA by doing an attachment or a
> pastebin URL.

I don't have thunderbird now but actually it's really simple:

Subject:
To:
Cc:
$SEP

Each data must be in a signle line, for example "Cc: <mail1>,.....,<mailN>"

>
> It appears that the original script tries very hard to keep the
> Subject: line at the beginning, but I am not sure if that is because
> Thunderbird wants to read its "$1" that way, or it is just that
> original appp.sh script happened to be written that way without real
> reason.  If I were updating this script, what I would do would be
> more like:

Ok, good coding then.

Regards,

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> I don't have thunderbird now but actually it's really simple:
>
> Subject:
> To:
> Cc:
> $SEP

The above is not a very useful "example" to advance this discussion,
I have to say.  For one, where is your Oggetto?  Are these fields
the only ones you will ever see?  Are they always empty?

I would expect, at least when you are responding to an existing
message, some of them are filled already (and if so, I think appp.sh
wants to know exactly how, for example, has RFC2047 quoting already
applied, or are we supposed to write in UTF-8 and let Thunderbird
massage the contents when we give the file back to it?), and also
there would appear In-Reply-To: field already filled (possibly there
may be References: as well).

And if you are replying to an existing message and Thunderbird fills
To: or Cc: fields based on the ongoing discussion, I wonder if it is
a mistake to override that based on anything from what we read from
format-patch output in the first place.

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

2012/9/4 Junio C Hamano <gitster@pobox.com>:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> I don't have thunderbird now but actually it's really simple:
>>
>> Subject:
>> To:
>> Cc:
>> $SEP
>
> The above is not a very useful "example" to advance this discussion,
> I have to say.  For one, where is your Oggetto?

1) Where is your Oggetto?
I used english template simply because the question was about position
of each statement, but with translation the positions are the same.

2) Are these fields the only ones you will ever see?
No, the template is dynamic according to the settings of plug-in
"external editor". You can have
Subject, To, Cc, Ccn, In reply to, discussion group (please see
external editor for details).

3)  Are they always empty?
Yes. If they are empty, nothing happens actually. I mean, if you have
the field Cc: empty, in the message composition
Thunderbird will use an empty Cc: line, but no problem in sending.

>
> I would expect, at least when you are responding to an existing
> message, some of them are filled already (and if so, I think appp.sh
> wants to know exactly how, for example, has RFC2047 quoting already
> applied, or are we supposed to write in UTF-8 and let Thunderbird
> massage the contents when we give the file back to it?), and also
> there would appear In-Reply-To: field already filled (possibly there
> may be References: as well).

Message reply is out of scope of my patch. The goal here is send a
patch, so the execution flow is to open a new message,
clik on external editor (configured properly), select patch file and
send. It was the scope of the old script and it is the scope of my
patch. Usually you don't send a patch as reply, however even if you
want to send patch as proof-of-concept as reply, there is no
problem, you can copy&paste the patch body. The script, instead, is
very useful to load all patch info, pre-formatting it and filling with
all To: and Cc. So IMHO the in-reply-to case is simply overkilling for
this script. In addition, I think all this discussion is overkilling
for a simple script like that, if a thing can be improved ok I agree
to improve it, but what are we talking about? About a little script
that is only an external utility for this project, not a core
functionality.

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> 2012/9/4 Junio C Hamano <gitster@pobox.com>:
>
>> I would expect, at least when you are responding to an existing
>> message, some of them are filled already (and if so, I think appp.sh
>> wants to know exactly how, for example, has RFC2047 quoting already
>> applied, or are we supposed to write in UTF-8 and let Thunderbird
>> massage the contents when we give the file back to it?), and also
>> there would appear In-Reply-To: field already filled (possibly there
>> may be References: as well).
>
> Message reply is out of scope of my patch. The goal here is send a
> patch, so the execution flow is to open a new message,
> clik on external editor (configured properly), select patch file and
> send. It was the scope of the old script and it is the scope of my
> patch.

I certainly can understand that you updated the script for that use
case and that use case only, but given that the original tries very
hard to preserve:

 - what was in $HEADERS (by only replacing Subject);
 - the recipients CC'ed in $HEADERS (by grabbing them into $CCS); and
 - the body of the message in $BODY (i.e. what came after $SEP),

I find it hard to believe that it was meant to work on a freshly
created empty message and nothing else.  If people were depending on
the recipients listed on Cc that are taken from $1 to be preserved,
your patch will introduce a regression for them, no?

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

Il 04/09/2012 17:49, Junio C Hamano ha scritto:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> 2012/9/4 Junio C Hamano <gitster@pobox.com>:
>>
>>> I would expect, at least when you are responding to an existing
>>> message, some of them are filled already (and if so, I think appp.sh
>>> wants to know exactly how, for example, has RFC2047 quoting already
>>> applied, or are we supposed to write in UTF-8 and let Thunderbird
>>> massage the contents when we give the file back to it?), and also
>>> there would appear In-Reply-To: field already filled (possibly there
>>> may be References: as well).
>>
>> Message reply is out of scope of my patch. The goal here is send a
>> patch, so the execution flow is to open a new message,
>> clik on external editor (configured properly), select patch file and
>> send. It was the scope of the old script and it is the scope of my
>> patch.
>
> I certainly can understand that you updated the script for that use
> case and that use case only, but given that the original tries very
> hard to preserve:
>
>   - what was in $HEADERS (by only replacing Subject);
>   - the recipients CC'ed in $HEADERS (by grabbing them into $CCS); and
>   - the body of the message in $BODY (i.e. what came after $SEP),
>
> I find it hard to believe that it was meant to work on a freshly
> created empty message and nothing else.  If people were depending on
> the recipients listed on Cc that are taken from $1 to be preserved,
> your patch will introduce a regression for them, no?
>

I think all the process is different. Current script rely on the user to 
fill Cc: and To: in message composition window, it does a union of what 
found in commit message as signed-off-by (adding own address in cc?!) 
and Cc (usually not filled in the commit message and we should even 
count acked-by, tested-by and so on). My vision of things: the user can 
create a patch *already* with all information using --to and --cc. If 
you want to add optional addresses, you can of course add them in the 
composition window. In addition, with this approach is really easy to 
use external script as get_maintainer.pl of linux kernel, load the patch 
and send, really easy. So I don't think it's a regression, it's only a 
change in the work flow.

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Marco Stornelli <marco.stornelli@gmail.com> writes:

> kernel, load the patch and send, really easy. So I don't think it's a
> regression, it's only a change in the work flow.

Any change that forces the user to change an established work flow
supporteed by the existing tool we gave them _is_ a regression, even
if the person who forces that change believes the new way is better.

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Marco Stornelli]

2012/9/4 Junio C Hamano <gitster@pobox.com>:
> Marco Stornelli <marco.stornelli@gmail.com> writes:
>
>> kernel, load the patch and send, really easy. So I don't think it's a
>> regression, it's only a change in the work flow.
>
> Any change that forces the user to change an established work flow
> supporteed by the existing tool we gave them _is_ a regression, even
> if the person who forces that change believes the new way is better.

A change of this kind in external script like this, it isn't an
API/ABI change for me, your sentence is really opinable. With this
policy every application can't be changed to improve them (note here
we are talking about to change a work flow not to remove a feature).
If for you it is a problem, we can close this never ending thread
here. I'm happy with my script.

Marco

Re: [PATCH v4] Thunderbird: fix appp.sh format problems

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 31.08.2012 16:09, schrieb Marco Stornelli:
>> +CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;
>> +close FILE; $addr = $1 if $text =~ /Cc: (.*?(,\n .*?)*)\n/s; $addr =~ s/\n//g;
>> +print $addr;'`
>
> The quoting is broken in this line (sq within sq does not work).

As you said later in the thread, perl lets you be loose and say
$ENV{PATCHTMP} without quoting the string "PATCHTMP", so it is not
quite _broken_ per-se, but the above gives a false impression to
readers that the author meant to feed perl

    open FILE, $ENV{'PATCHTMP'};

which is not happening, so at least it is misleading.

> Am I correct that you intend to treat continuation lines with this
> non-trivial perl script?

As the above regexp seems to try to match

	Cc: marco, git,
          j6t, other recipient

I think that indeed is the intent.

> All this processing gets tedious and
> unreadable. Let me suggest this pattern instead:
>
> # translate to Thunderbird language
> LANG_TO="To:"
> LANG_SUBJ="Subject:"
> LANG_CC="Cc:"
>
> LF=	# terminates the _previous_ line
> while read -r line
> do
> 	case $line in
> 	'To: '*)
> 		printf "${LF}%s" "$LANG_TO ${line#To: }"
> 		;;
> 	'Cc: '*) ...similar...
> 	'Subject: '*) ...similar...
> 	' '*)	# continuation line
> 		printf "%s" "$line"
> 		;;
> 	'')
> 		print "${LF}\n"
> 		cat
> 		;;
> 	esac
> 	LF='\n'
> done <"$PATCH" >"$1"

I think that is much more readable.