Thunderbird: fix appp.sh format problems

PATCH Superseded Marco Stornelli 3 participants 21 messages Jun 15, 2016

lore

Share & tools

lore

Marco Stornelli

author

2016-06-15 22:54 UTC #0 lore raw

PATCH v4 1/1 Thunderbird: fix appp.sh format problems

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 <[email protected]>

---

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(-)

contrib/thunderbird-patch-inline/appp.sh +22 -4

index 5eb4a51..0daeb29 100755
@@ -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

Signature · 1 line

1.7.3.4

Junio C Hamano

2016-06-15 22:54 UTC #1 lore raw

Quoted 8 lines · 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.

Quoted 6 lines

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 <[email protected]>
---

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).

Quoted 50 lines

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

contrib/thunderbird-patch-inline/appp.sh +18 -3

index 5eb4a51..0daeb29 100755
@@ -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

contrib/thunderbird-patch-inline/appp.sh fi · line 60

Quoted 10 lines

contrib/thunderbird-patch-inline/appp.sh +4 -1

+# 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(-)

contrib/thunderbird-patch-inline/appp.sh +3 -4

index 0daeb29..18d8bfa 100755
@@ -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

author

2016-06-15 22:54 UTC #2 lore raw

Il 31/08/2012 19:08, Junio C Hamano ha scritto:

Quoted 87 lines

Quoted 8 lines · 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.
Quoted 6 lines
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 <[email protected]>
---
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).
Quoted 59 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(-)
contrib/thunderbird-patch-inline/appp.sh +22 -4
index 5eb4a51..0daeb29 100755
@@ -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
 

Quoted 5 lines

contrib/thunderbird-patch-inline/appp.sh fi · line 60
Quoted 1 line
contrib/thunderbird-patch-inline/appp.sh +0 -0
  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>?

Quoted 2 lines

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

2016-06-15 22:54 UTC #3 lore raw

Quoted 4 lines · Marco Stornelli

Quoted 2 lines
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.

Johannes Sixt

2016-06-15 22:54 UTC #4 lore raw

In reply to Marco Stornelli

Am 31.08.2012 16:09, schrieb Marco Stornelli:

contrib/thunderbird-patch-inline/appp.sh fi · line 43

Quoted 3 lines

contrib/thunderbird-patch-inline/appp.sh +3 -0

+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

author

2016-06-15 22:54 UTC #5 lore raw

Il 31/08/2012 23:35, Johannes Sixt ha scritto:

Quoted 6 lines

Am 31.08.2012 16:09, schrieb Marco Stornelli:
contrib/thunderbird-patch-inline/appp.sh fi · line 43
Quoted 3 lines
contrib/thunderbird-patch-inline/appp.sh +3 -0
+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.

Quoted 4 lines

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.

Quoted 36 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

2016-06-15 22:54 UTC #6 lore raw

Am 01.09.2012 09:43, schrieb Marco Stornelli:

Quoted 10 lines

Il 31/08/2012 23:35, Johannes Sixt ha scritto:
Quoted 8 lines
Am 31.08.2012 16:09, schrieb Marco Stornelli:
Quoted 5 lines
+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).

Quoted 2 lines

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

author

2016-06-15 22:54 UTC #7 lore raw

Il 01/09/2012 15:59, Johannes Sixt ha scritto:

Quoted 18 lines

Am 01.09.2012 09:43, schrieb Marco Stornelli:
Quoted 10 lines
Il 31/08/2012 23:35, Johannes Sixt ha scritto:
Quoted 8 lines
Am 31.08.2012 16:09, schrieb Marco Stornelli:
Quoted 5 lines
+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).
Quoted 2 lines
I don't understand what you mean, I'm using this script and it works perfectly.
Look how you write:
   perl -e '... $ENV{'PATCHTMP'} ...'

Quoted 10 lines

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

2016-06-15 22:54 UTC #8 lore raw

Quoted 8 lines · Marco Stornelli

Il 01/09/2012 15:59, Johannes Sixt ha scritto:
Quoted 6 lines
Look how you write:
   perl -e '... $ENV{'PATCHTMP'} ...'
That is, perl actually sees this script:

Quoted 7 lines

Quoted 4 lines
   ... $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

author

2016-06-15 22:54 UTC #9 lore raw

Il 02/09/2012 22:42, Junio C Hamano ha scritto:

Quoted 32 lines

Quoted 8 lines · Marco Stornelli
Il 01/09/2012 15:59, Johannes Sixt ha scritto:
Quoted 6 lines
Look how you write:
    perl -e '... $ENV{'PATCHTMP'} ...'
That is, perl actually sees this script:
Quoted 7 lines
Quoted 4 lines
    ... $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:

Quoted 8 lines

     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

2016-06-15 22:54 UTC #10 lore raw

Quoted 3 lines · Marco Stornelli

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

author

2016-06-15 22:54 UTC #11 lore raw

2012/9/3 Junio C Hamano <[email protected]>:

Quoted 12 lines

Quoted 3 lines · Marco Stornelli
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.

Quoted 5 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>"

Quoted 7 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

2016-06-15 22:54 UTC #12 lore raw

Quoted 6 lines · Marco Stornelli

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

author

2016-06-15 22:54 UTC #13 lore raw

2012/9/4 Junio C Hamano <[email protected]>:

Quoted 11 lines

Quoted 6 lines · Marco Stornelli
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.

Quoted 8 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

2016-06-15 22:54 UTC #14 lore raw

Quoted 10 lines · Marco Stornelli

2012/9/4 Junio C Hamano <[email protected]>:
Quoted 7 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).

Quoted 5 lines

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

author

2016-06-15 22:54 UTC #15 lore raw

Il 04/09/2012 17:49, Junio C Hamano ha scritto:

Quoted 22 lines

Quoted 10 lines · Marco Stornelli
2012/9/4 Junio C Hamano <[email protected]>:
Quoted 7 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).
Quoted 5 lines
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:

Quoted 9 lines

- 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

2016-06-15 22:54 UTC #16 lore raw

Quoted 2 lines · Marco Stornelli

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

author

2016-06-15 22:54 UTC #17 lore raw

2012/9/4 Junio C Hamano <[email protected]>:

Quoted 8 lines

Quoted 2 lines · Marco Stornelli
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

Marco Stornelli

author

2016-06-15 22:54 UTC #18 lore raw

In reply to Junio C Hamano

2012/9/2 Junio C Hamano <[email protected]>:

Quoted 32 lines

Quoted 8 lines · Marco Stornelli
Il 01/09/2012 15:59, Johannes Sixt ha scritto:
Quoted 6 lines
Look how you write:
   perl -e '... $ENV{'PATCHTMP'} ...'
That is, perl actually sees this script:
Quoted 7 lines
Quoted 4 lines
   ... $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:

Quoted 7 lines

    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

Junio C Hamano

2016-06-15 22:54 UTC #19 lore raw

In reply to Johannes Sixt

Quoted 6 lines · Johannes Sixt

Am 31.08.2012 16:09, schrieb Marco Stornelli:
contrib/thunderbird-patch-inline/appp.sh fi · line 43
Quoted 3 lines
contrib/thunderbird-patch-inline/appp.sh +3 -0
+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.

Quoted 2 lines

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.

Quoted 26 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'

Quoted 1 line

done <"$PATCH" >"$1"

I think that is much more readable.

Junio C Hamano

2016-06-15 22:54 UTC #20 lore raw

In reply to Marco Stornelli

Quoted 6 lines · Marco Stornelli

contrib/thunderbird-patch-inline/appp.sh +6 -0

+PATCHTMP="${PATCH}.tmp"
+
+echo $MAILHEADER > $PATCHTMP
+
+export PATCHTMP
+CCS=`perl -e 'local $/=undef; open FILE, $ENV{'PATCHTMP'}; $text=<FILE>;

contrib/thunderbird-patch-inline/appp.sh fi · line 49

Quoted 10 lines

contrib/thunderbird-patch-inline/appp.sh +7 -2

+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"