threads / patch / 31396

v4Thunderbird: fix appp.sh format problems

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

## tl;dr

21 messages between Aug 31, 2012 and Sep 5, 2012. Diffs are folded; open one to read it.

replies: 20people: 3as markdown or json

Marco Stornelli· Aug 31, 2012, 14:09 UTC · lore
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(-)
Show changes to contrib/thunderbird-patch-inline/appp.sh +22 −4
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
Junio C Hamano· Aug 31, 2012, 17:08 UTC · re: Marco Stornelli · lore

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

Marco Stornelli <marco.stornelli@gmail.com> writes:
Show 14 quoted lines
> 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).
Show 60 quoted lines
>
> 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(-)
Show changes to diff +3 −4
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
Marco Stornelli· Sep 1, 2012, 07:52 UTC · re: Junio C Hamano · lore

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

Il 31/08/2012 19:08, Junio C Hamano ha scritto:
Show 92 quoted lines
> 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
Junio C Hamano· Sep 2, 2012, 18:44 UTC · re: Marco Stornelli · lore

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

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.
Junio C Hamano· Aug 31, 2012, 20:01 UTC · re: Marco Stornelli · lore

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

Marco Stornelli <marco.stornelli@gmail.com> writes:
Show 16 quoted lines
> +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"
Johannes Sixt· Aug 31, 2012, 21:35 UTC · re: Marco Stornelli · lore

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

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
Marco Stornelli· Sep 1, 2012, 07:43 UTC · re: Johannes Sixt · lore

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

Il 31/08/2012 23:35, Johannes Sixt ha scritto:
Show 6 quoted lines
> 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.

Show 36 quoted lines
>
> # 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
>
>
Johannes Sixt· Sep 1, 2012, 13:59 UTC · re: Marco Stornelli · lore

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

Am 01.09.2012 09:43, schrieb Marco Stornelli:
Show 12 quoted lines
> 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
Marco Stornelli· Sep 1, 2012, 19:18 UTC · re: Johannes Sixt · lore

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

Il 01/09/2012 15:59, Johannes Sixt ha scritto:
Show 28 quoted lines
> 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
Junio C Hamano· Sep 2, 2012, 20:42 UTC · re: Marco Stornelli · lore

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

Marco Stornelli <marco.stornelli@gmail.com> writes:
Show 15 quoted lines
> 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.

Marco Stornelli· Sep 3, 2012, 10:51 UTC · re: Junio C Hamano · lore

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

2012/9/2 Junio C Hamano <gitster@pobox.com>:
Show 39 quoted lines
> 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
Marco Stornelli· Sep 3, 2012, 15:48 UTC · re: Junio C Hamano · lore

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

Il 02/09/2012 22:42, Junio C Hamano ha scritto:
Show 40 quoted lines
> 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
Junio C Hamano· Sep 3, 2012, 20:16 UTC · re: Marco Stornelli · lore

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

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".
Marco Stornelli· Sep 4, 2012, 06:37 UTC · re: Junio C Hamano · lore

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

2012/9/3 Junio C Hamano <gitster@pobox.com>:
Show 17 quoted lines
> 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>"
Show 7 quoted lines
>
> 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
Junio C Hamano· Sep 4, 2012, 09:01 UTC · re: Marco Stornelli · lore

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

Marco Stornelli <marco.stornelli@gmail.com> writes:
Show 6 quoted lines
> 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.
Marco Stornelli· Sep 4, 2012, 11:22 UTC · re: Junio C Hamano · lore

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

2012/9/4 Junio C Hamano <gitster@pobox.com>:
Show 11 quoted lines
> 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.
Show 8 quoted lines
>
> 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
Junio C Hamano· Sep 4, 2012, 15:49 UTC · re: Marco Stornelli · lore

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

Marco Stornelli <marco.stornelli@gmail.com> writes:
Show 15 quoted lines
> 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?

Marco Stornelli· Sep 4, 2012, 18:59 UTC · re: Junio C Hamano · lore

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

Il 04/09/2012 17:49, Junio C Hamano ha scritto:
Show 31 quoted lines
> 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
Junio C Hamano· Sep 4, 2012, 19:22 UTC · re: Marco Stornelli · lore

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

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.

Marco Stornelli· Sep 5, 2012, 06:30 UTC · re: Junio C Hamano · lore

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

2012/9/4 Junio C Hamano <gitster@pobox.com>:
Show 8 quoted lines
> 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
Junio C Hamano· Sep 2, 2012, 18:42 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 6 quoted lines
> 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.
Show 27 quoted lines
> 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.

← back to recent threads