{"thread":{"id":"42466","subject":"[WIP-PATCH 1/2] send-email: create email parser subroutine","startedAt":"2016-05-27T14:01:02Z","lastAt":"2016-06-02T19:58:55Z","messageCount":18,"participants":["Samuel GROOT","Eric Wong","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"287674","messageId":"20160527140104.11192-1-samuel.groot@grenoble-inp.org","threadId":"42466","inReplyTo":null,"subject":"[WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-27T14:01:02Z","receivedAt":"2016-05-27T14:01:02Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"While working on the new option `quote-email`, we needed to parse an\nemail file. Since the work is already done, but the parsing and data\nprocessing are in the same loop, we wanted to refactor the parser, to\nmake the code more maintainable.\n\nThis is still WIP, and one of the main issue (and we need your\nadvice on that), is that around 30 tests will fail, and most of them\nare false-negatives: to pass, they rely on the format of what is\ndisplayed by `git send-email`, not only data.\n\n\nFor example, several tests will fail because they do a strict compare\nbetween `git send-email`'s output and:\n\n (mbox) Adding cc: A from line 'Cc: A, One'\n (mbox) Adding cc: One from line 'Cc: A, One'\n\nThough `git send-email` now outputs something like:\n\n (mbox) Adding cc: A from line 'Cc: A'\n (mbox) Adding cc: One from line 'Cc: One'\n\nThe new behavior is explained because parsing and processing are not\ndone in the same function, and processing cannot know the exact line\nthe data came from.\n\nWe can think of several solutions:\n\n 1. Modify the parser, to give the script the knowledge of the exact\n line the data came from.\n\n 2. Hack the tests: modify the script using the parser to artificially\n recreate the supposedly parsed line.\n (e.g. with a list.join(', ')-like function)\n\n 3. Modify the tests to replace exact cmp by greps.\n\n\nIMHO, we should consider option 3, since the tests should rely on data\nrather than exact outputs. It also makes the tests more maintainable,\nin the sense that they will be more resilient to future output\nmodifications.\n\nWhat do you think?\n\n\n [WIP-PATCH 1/2] send-email: create email parser subroutine\n [WIP-PATCH 2/2] send-email: use refactored subroutine to parse patches\n\ngit-send-email.perl | 284 ++++++++++++++++++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 164 insertions(+), 120 deletions(-)\n"},{"id":"287673","messageId":"20160527140104.11192-2-samuel.groot@grenoble-inp.org","threadId":"42466","inReplyTo":"20160527140104.11192-1-samuel.groot@grenoble-inp.org","subject":"[WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-27T14:01:03Z","receivedAt":"2016-05-27T14:01:03Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"Parsing and processing in send-email is done in the same loop.\n\nTo make the code more maintainable, we create two subroutines:\n- `parse_email` to separate header and body\n- `parse_header` to retrieve data from header\n\nSigned-off-by: Samuel GROOT \nSigned-off-by: Tom RUSSELLO \nSigned-off-by: Matthieu MOY \n---\n git-send-email.perl | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 105 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 6958785..f33a083 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1750,3 +1750,108 @@ sub body_or_subject_has_nonascii {\n \t}\n \treturn 0;\n }\n+\n+sub parse_email {\n+\tmy @header = ();\n+\tmy @body = ();\n+\tmy $fh = shift;\n+\n+\t# First unfold multiline header fields\n+\twhile (<$fh>) {\n+\t\tlast if /^\\s*$/;\n+\t\tif (/^\\s+\\S/ and @header) {\n+\t\t\tchomp($header[$#header]);\n+\t\t\ts/^\\s+/ /;\n+\t\t\t$header[$#header] .= $_;\n+\t\t} else {\n+\t\t\tpush(@header, $_);\n+\t\t}\n+\t}\n+\n+\t# Now unfold the message body\n+\twhile (<$fh>) {\n+\t\tpush @body, $_;\n+\t}\n+\n+\treturn (@header, @body);\n+}\n+\n+sub parse_header {\n+\t# Return variables\n+\tmy $from = undef, $subject = undef;\n+\tmy $date = undef, $message_id = undef;\n+\tmy @to = (), @cc = (), @xh = ();\n+\tmy %flags = ();\n+\n+\n+\t# Internal variables\n+\tmy $input_format = undef;\n+\n+\tforeach(@_) {\n+\t\tif (/^From /) {\n+\t\t\t$input_format = 'mbox';\n+\t\t\tnext;\n+\t\t}\n+\t\tchomp;\n+\t\tif (!defined $input_format && /^[-A-Za-z]+:\\s/) {\n+\t\t\t$input_format = 'mbox';\n+\t\t}\n+\n+\t\tif (defined $input_format && $input_format eq 'mbox') {\n+\t\t\tif (/^Subject:\\s+(.*)$/i) {\n+\t\t\t\t$subject = $1;\n+\t\t\t} elsif (/^From:\\s+(.*)$/i) {\n+\t\t\t\t$from = $1;\n+\t\t\t} elsif (/^To:\\s+(.*)$/i) {\n+\t\t\t\tforeach my $addr (parse_address_line($1)) {\n+\t\t\t\t\tpush @to, $addr;\n+\t\t\t\t}\n+\t\t\t} elsif (/^Cc:\\s+(.*)$/i) {\n+\t\t\t\tforeach my $addr (parse_address_line($1)) {\n+\t\t\t\t\tpush @cc, $addr;\n+\t\t\t\t}\n+\t\t\t} elsif (/^Content-type:/i) {\n+\t\t\t\t$flags{\"has_content_type\"} = 1;\n+\t\t\t\tif (/charset=\"?([^ \"]+)/) {\n+\t\t\t\t\t$flags{\"body_encoding\"} = 1;\n+\t\t\t\t}\n+\t\t\t\tpush @xh, $_;\n+\t\t\t} elsif (/^MIME-Version/i) {\n+\t\t\t\t$flags{\"has_mime_version\"} = 1;\n+\t\t\t\tpush @xh, $_;\n+\t\t\t} elsif (/^Message-Id: (.*)/i) {\n+\t\t\t\t$message_id = $1;\n+\t\t\t} elsif (/^Content-Transfer-Encoding: (.*)/i) {\n+\t\t\t\t$flags{\"xfer_encoding\"} = $1 if not defined $flags{\"xfer_encoding\"};\n+\t\t\t} elsif (/^Date:\\s(.*)$/i) {\n+\t\t\t\t$date = $1;\n+\t\t\t} elsif (/^[-A-Za-z]+:\\s+\\S/) {\n+\t\t\t\tpush @xh, $_;\n+\t\t\t}\n+\n+\t\t} else {\n+\t\t\t# In the traditional\n+\t\t\t# \"send lots of email\" format,\n+\t\t\t# line 1 = cc\n+\t\t\t# line 2 = subject\n+\t\t\t# So let's support that, too.\n+\t\t\t$input_format = 'lots';\n+\t\t\tif (@cc == 0) {\n+\t\t\t\tpush @cc, $_;\n+\t\t\t} elsif (!defined $subject) {\n+\t\t\t\t$subject = $_;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn (\n+\t\t\"from\" => $from,\n+\t\t\"subject\" => $subject,\n+\t\t\"date\" => $date,\n+\t\t\"message_id\" => $message_id,\n+\t\t\"to\" => [@to],\n+\t\t\"cc\" => [@cc],\n+\t\t\"xh\" => [@xh],\n+\t\t\"flags\" => {%flags}\n+\t);\n+}\n-- \n2.8.2.537.gb153d2a\n"},{"id":"287675","messageId":"20160527140104.11192-3-samuel.groot@grenoble-inp.org","threadId":"42466","inReplyTo":"20160527140104.11192-1-samuel.groot@grenoble-inp.org","subject":"[WIP-PATCH 2/2] send-email: use refactored subroutine to parse patches","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-27T14:01:04Z","receivedAt":"2016-05-27T14:01:04Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"Use the two subroutines `parse_email` and `parse_header` introduced in\nprevious commit to parse patches.\n\nSigned-off-by: Samuel GROOT \nSigned-off-by: Tom RUSSELLO \nSigned-off-by: Matthieu MOY \n---\n git-send-email.perl | 179 +++++++++++++++++-----------------------------------\n 1 file changed, 59 insertions(+), 120 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex f33a083..7bb4a2d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -161,7 +161,7 @@ my $re_encoded_word = qr/=\\?($re_token)\\?($re_token)\\?($re_encoded_text)\\?=/;\n # Variables we fill in automatically, or via prompting:\n my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,\n \t$initial_reply_to,$initial_subject,@files,\n-\t$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);\n+\t$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);\n \n my $envelope_sender;\n \n@@ -1431,117 +1431,57 @@ $subject = $initial_subject;\n $message_num = 0;\n \n foreach my $t (@files) {\n-\topen my $fh, \"<\", $t or die \"can't open file $t\";\n-\n-\tmy $author = undef;\n-\tmy $sauthor = undef;\n-\tmy $author_encoding;\n-\tmy $has_content_type;\n-\tmy $body_encoding;\n-\tmy $xfer_encoding;\n-\tmy $has_mime_version;\n-\t@to = ();\n-\t@cc = ();\n-\t@xh = ();\n-\tmy $input_format = undef;\n-\tmy @header = ();\n \t$message = \"\";\n \t$message_num++;\n-\t# First unfold multiline header fields\n-\twhile(<$fh>) {\n-\t\tlast if /^\\s*$/;\n-\t\tif (/^\\s+\\S/ and @header) {\n-\t\t\tchomp($header[$#header]);\n-\t\t\ts/^\\s+/ /;\n-\t\t\t$header[$#header] .= $_;\n-\t } else {\n-\t\t\tpush(@header, $_);\n-\t\t}\n-\t}\n-\t# Now parse the header\n-\tforeach(@header) {\n-\t\tif (/^From /) {\n-\t\t\t$input_format = 'mbox';\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tif (!defined $input_format && /^[-A-Za-z]+:\\s/) {\n-\t\t\t$input_format = 'mbox';\n-\t\t}\n \n-\t\tif (defined $input_format && $input_format eq 'mbox') {\n-\t\t\tif (/^Subject:\\s+(.*)$/i) {\n-\t\t\t\t$subject = $1;\n-\t\t\t}\n-\t\t\telsif (/^From:\\s+(.*)$/i) {\n-\t\t\t\t($author, $author_encoding) = unquote_rfc2047($1);\n-\t\t\t\t$sauthor = sanitize_address($author);\n-\t\t\t\tnext if $suppress_cc{'author'};\n-\t\t\t\tnext if $suppress_cc{'self'} and $sauthor eq $sender;\n-\t\t\t\tprintf(\"(mbox) Adding cc: %s from line '%s'\\n\",\n-\t\t\t\t\t$1, $_) unless $quiet;\n-\t\t\t\tpush @cc, $1;\n-\t\t\t}\n-\t\t\telsif (/^To:\\s+(.*)$/i) {\n-\t\t\t\tforeach my $addr (parse_address_line($1)) {\n-\t\t\t\t\tprintf(\"(mbox) Adding to: %s from line '%s'\\n\",\n-\t\t\t\t\t\t$addr, $_) unless $quiet;\n-\t\t\t\t\tpush @to, $addr;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\telsif (/^Cc:\\s+(.*)$/i) {\n-\t\t\t\tforeach my $addr (parse_address_line($1)) {\n-\t\t\t\t\tmy $qaddr = unquote_rfc2047($addr);\n-\t\t\t\t\tmy $saddr = sanitize_address($qaddr);\n-\t\t\t\t\tif ($saddr eq $sender) {\n-\t\t\t\t\t\tnext if ($suppress_cc{'self'});\n-\t\t\t\t\t} else {\n-\t\t\t\t\t\tnext if ($suppress_cc{'cc'});\n-\t\t\t\t\t}\n-\t\t\t\t\tprintf(\"(mbox) Adding cc: %s from line '%s'\\n\",\n-\t\t\t\t\t\t$addr, $_) unless $quiet;\n-\t\t\t\t\tpush @cc, $addr;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\telsif (/^Content-type:/i) {\n-\t\t\t\t$has_content_type = 1;\n-\t\t\t\tif (/charset=\"?([^ \"]+)/) {\n-\t\t\t\t\t$body_encoding = $1;\n-\t\t\t\t}\n-\t\t\t\tpush @xh, $_;\n-\t\t\t}\n-\t\t\telsif (/^MIME-Version/i) {\n-\t\t\t\t$has_mime_version = 1;\n-\t\t\t\tpush @xh, $_;\n-\t\t\t}\n-\t\t\telsif (/^Message-Id: (.*)/i) {\n-\t\t\t\t$message_id = $1;\n-\t\t\t}\n-\t\t\telsif (/^Content-Transfer-Encoding: (.*)/i) {\n-\t\t\t\t$xfer_encoding = $1 if not defined $xfer_encoding;\n-\t\t\t}\n-\t\t\telsif (!/^Date:\\s/i && /^[-A-Za-z]+:\\s+\\S/) {\n-\t\t\t\tpush @xh, $_;\n-\t\t\t}\n+\t# Split email into header and body\n+\topen my $fh, \"<\", $t or die \"can't open file $t\";\n+\tmy (@header, @body) = parse_email($fh);\n+\tclose $fh;\n \n+\t# Parse header\n+\tmy %parsed_header = parse_header(@header);\n+\tmy $from = $parsed_header{\"from\"};\n+\t$subject = $parsed_header{\"subject\"};\n+\t$message_id = $parsed_header{\"message_id\"};\n+\t@to = @{$parsed_header{\"to\"}};\n+\t@cc = @{$parsed_header{\"cc\"}};\n+\t@xh = @{$parsed_header{\"xh\"}};\n+\tmy %flags = %{$parsed_header{\"flags\"}};\n+\n+\t# Process parsed headers\n+\tmy ($author, $author_encoding) = unquote_rfc2047($from);\n+\tmy $sauthor = sanitize_address($author);\n+\tunless ($suppress_cc{'author'} or\n+\t\t($suppress_cc{'self'} and $sauthor eq $sender)) {\n+\t\tprintf(\"(mbox) Adding cc: %s from line 'From: %s'\\n\",\n+\t\t\t$from, $from) unless $quiet;\n+\t\tpush @cc, $from;\n+\t}\n+\n+\tforeach (@to) {\n+\t\tprintf(\"(mbox) Adding to: %s from line 'To: %s'\\n\",\n+\t\t\t$_, $_) unless $quiet;\n+\t}\n+\n+\tmy @tmpcc = ();\n+\tforeach (@cc) {\n+\t\tmy $qaddr = unquote_rfc2047($_);\n+\t\tmy $saddr = sanitize_address($qaddr);\n+\t\tif ($saddr eq $sender) {\n+\t\t\tnext if ($suppress_cc{'self'});\n \t\t} else {\n-\t\t\t# In the traditional\n-\t\t\t# \"send lots of email\" format,\n-\t\t\t# line 1 = cc\n-\t\t\t# line 2 = subject\n-\t\t\t# So let's support that, too.\n-\t\t\t$input_format = 'lots';\n-\t\t\tif (@cc == 0 && !$suppress_cc{'cc'}) {\n-\t\t\t\tprintf(\"(non-mbox) Adding cc: %s from line '%s'\\n\",\n-\t\t\t\t\t$_, $_) unless $quiet;\n-\t\t\t\tpush @cc, $_;\n-\t\t\t} elsif (!defined $subject) {\n-\t\t\t\t$subject = $_;\n-\t\t\t}\n+\t\t\tnext if ($suppress_cc{'cc'});\n \t\t}\n+\t\tprintf(\"(mbox) Adding cc: %s from line 'Cc: %s'\\n\",\n+\t\t\t$_, $_) unless $quiet;\n+\t\tpush @tmpcc, $_;\n \t}\n+\t@cc = @tmpcc;\n+\n+\n \t# Now parse the message body\n-\twhile(<$fh>) {\n+\tforeach (@body) {\n \t\t$message .= $_;\n \t\tif (/^(Signed-off-by|Cc): (.*)$/i) {\n \t\t\tchomp;\n@@ -1559,18 +1499,17 @@ foreach my $t (@files) {\n \t\t\t\t$c, $_) unless $quiet;\n \t\t}\n \t}\n-\tclose $fh;\n \n \tpush @to, recipients_cmd(\"to-cmd\", \"to\", $to_cmd, $t)\n \t\tif defined $to_cmd;\n \tpush @cc, recipients_cmd(\"cc-cmd\", \"cc\", $cc_cmd, $t)\n \t\tif defined $cc_cmd && !$suppress_cc{'cccmd'};\n \n-\tif ($broken_encoding{$t} && !$has_content_type) {\n-\t\t$xfer_encoding = '8bit' if not defined $xfer_encoding;\n-\t\t$has_content_type = 1;\n+\tif ($broken_encoding{$t} && !$flags{\"has_content_type\"}) {\n+\t\t$flags{\"xfer_encoding\"} = '8bit' if not defined $flags{\"xfer_encoding\"};\n+\t\t$flags{\"has_content_type\"} = 1;\n \t\tpush @xh, \"Content-Type: text/plain; charset=$auto_8bit_encoding\";\n-\t\t$body_encoding = $auto_8bit_encoding;\n+\t\t$flags{\"body_encoding\"} = $auto_8bit_encoding;\n \t}\n \n \tif ($broken_encoding{$t} && !is_rfc2047_quoted($subject)) {\n@@ -1580,8 +1519,8 @@ foreach my $t (@files) {\n \tif (defined $sauthor and $sauthor ne $sender) {\n \t\t$message = \"From: $author\\n\\n$message\";\n \t\tif (defined $author_encoding) {\n-\t\t\tif ($has_content_type) {\n-\t\t\t\tif ($body_encoding eq $author_encoding) {\n+\t\t\tif ($flags{\"has_content_type\"}) {\n+\t\t\t\tif ($flags{\"body_encoding\"} eq $author_encoding) {\n \t\t\t\t\t# ok, we already have the right encoding\n \t\t\t\t}\n \t\t\t\telse {\n@@ -1589,24 +1528,24 @@ foreach my $t (@files) {\n \t\t\t\t}\n \t\t\t}\n \t\t\telse {\n-\t\t\t\t$xfer_encoding = '8bit' if not defined $xfer_encoding;\n-\t\t\t\t$has_content_type = 1;\n+\t\t\t\t$flags{\"xfer_encoding\"} = '8bit' if not defined $flags{\"xfer_encoding\"};\n+\t\t\t\t$flags{\"has_content_type\"} = 1;\n \t\t\t\tpush @xh,\n \t\t\t\t \"Content-Type: text/plain; charset=$author_encoding\";\n \t\t\t}\n \t\t}\n \t}\n \tif (defined $target_xfer_encoding) {\n-\t\t$xfer_encoding = '8bit' if not defined $xfer_encoding;\n+\t\t$flags{\"xfer_encoding\"} = '8bit' if not defined $flags{\"xfer_encoding\"};\n \t\t$message = apply_transfer_encoding(\n-\t\t\t$message, $xfer_encoding, $target_xfer_encoding);\n-\t\t$xfer_encoding = $target_xfer_encoding;\n+\t\t\t$message, $flags{\"xfer_encoding\"}, $target_xfer_encoding);\n+\t\t$flags{\"xfer_encoding\"} = $target_xfer_encoding;\n \t}\n-\tif (defined $xfer_encoding) {\n-\t\tpush @xh, \"Content-Transfer-Encoding: $xfer_encoding\";\n+\tif (defined $flags{\"xfer_encoding\"}) {\n+\t\tpush @xh, \"Content-Transfer-Encoding: \".$flags{\"xfer_encoding\"};\n \t}\n-\tif (defined $xfer_encoding or $has_content_type) {\n-\t\tunshift @xh, 'MIME-Version: 1.0' unless $has_mime_version;\n+\tif (defined $flags{\"xfer_encoding\"} or $flags{\"has_content_type\"}) {\n+\t\tunshift @xh, 'MIME-Version: 1.0' unless $flags{\"has_mime_version\"};\n \t}\n \n \t$needs_confirm = (\n-- \n2.8.2.537.gb153d2a\n"},{"id":"287713","messageId":"20160527201436.GA16547@dcvr.yhbt.net","threadId":"42466","inReplyTo":"20160527140104.11192-1-samuel.groot@grenoble-inp.org","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-27T20:14:36Z","receivedAt":"2016-05-27T20:14:36Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Samuel GROOT wrote:\n> While working on the new option `quote-email`, we needed to parse an\n> email file. Since the work is already done, but the parsing and data\n> processing are in the same loop, we wanted to refactor the parser, to\n> make the code more maintainable.\n\nThank you for doing this work :)\n\n> This is still WIP, and one of the main issue (and we need your\n> advice on that), is that around 30 tests will fail, and most of them\n> are false-negatives: to pass, they rely on the format of what is\n> displayed by `git send-email`, not only data.\n> \n> \n> For example, several tests will fail because they do a strict compare\n> between `git send-email`'s output and:\n> \n> (mbox) Adding cc: A from line 'Cc: A, One'\n> (mbox) Adding cc: One from line 'Cc: A, One'\n> \n> Though `git send-email` now outputs something like:\n> \n> (mbox) Adding cc: A from line 'Cc: A'\n> (mbox) Adding cc: One from line 'Cc: One'\nI actually like neither, and would prefer something shorter:\n\n (mbox) Adding cc: A from Cc: header\n (mbox) Adding cc: One from Cc: header\n (mbox) Adding cc: SoB from Signed-off-by: trailer\n\nThat way, there's no redundant addresses in each line and less\nlikely to wrap.\n\nBut I actually never noticed these lines in the past since they\nscrolled off my terminal :x\n\nSince the headers are already shown after those lines, it's\nredundant to have the entire line. And we could add\ntrailers after the headers (with a blank line to delimit):\n\n # existing header output:\n From: F \n Cc: A , One \n Subject: foo\n\n # new trailer output\n Signed-off-by: SoB \n Acked-by: ack \n\n> We can think of several solutions:\n> \n> 1. Modify the parser, to give the script the knowledge of the exact\n> line the data came from.\n> \n> 2. Hack the tests: modify the script using the parser to artificially\n> recreate the supposedly parsed line.\n> (e.g. with a list.join(', ')-like function)\n> \n> 3. Modify the tests to replace exact cmp by greps.\n> \n> \n> IMHO, we should consider option 3, since the tests should rely on data\n> rather than exact outputs. It also makes the tests more maintainable,\n> in the sense that they will be more resilient to future output\n> modifications.\n\nAgreed on 3.\n\nI am not sure if anybody outside of tests parses the stdout of\nsend-email. It's certainly a porcelain and I don't think\nstdout needs to be stable, and maybe the output in\nquestion should go to stderr since it could be considered\ndebug output.\n\nBut I could be wrong...\n"},{"id":"287744","messageId":"vpq8tyujkb0.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"20160527201436.GA16547@dcvr.yhbt.net","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-28T15:04:19Z","receivedAt":"2016-05-28T15:04:19Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Wong writes:\n\n> Samuel GROOT wrote:\n>\n>> (mbox) Adding cc: A from line 'Cc: A, One'\n>> (mbox) Adding cc: One from line 'Cc: A, One'\n>> \n>> Though `git send-email` now outputs something like:\n>> \n>> (mbox) Adding cc: A from line 'Cc: A'\n>> (mbox) Adding cc: One from line 'Cc: One'\n> I actually like neither, and would prefer something shorter:\n>\n> (mbox) Adding cc: A from Cc: header\n> (mbox) Adding cc: One from Cc: header\n> (mbox) Adding cc: SoB from Signed-off-by: trailer\n>\n> That way, there's no redundant addresses in each line and less\n> likely to wrap.\n\nI agree with this. Actually, I'd even say that the current output of\n\"git send-email\" looks sloppy, and internal refactoring may be a good\nopportunity to get it cleaner.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287748","messageId":"vpqeg8mi4wm.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"20160527140104.11192-2-samuel.groot@grenoble-inp.org","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-28T15:22:17Z","receivedAt":"2016-05-28T15:22:17Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Samuel GROOT writes:\n\n> Parsing and processing in send-email is done in the same loop.\n>\n> To make the code more maintainable, we create two subroutines:\n> - `parse_email` to separate header and body\n> - `parse_header` to retrieve data from header\n\nThese routines are not specific to git send-email, nor to Git.\n\nDoes it make sense to use an external library, like\nhttp://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,\neither by depending on it, or by copying it in Git's source tree ?\n\nIf not, I think it would be better to introduce an email parsing library\nin a dedicated Perl module in perl/ in our source tree, to keep\ngit-send-email.perl more focused on the \"send-email\" logic.\n\n> +sub parse_email {\n> +\tmy @header = ();\n> +\tmy @body = ();\n> +\tmy $fh = shift;\n> +\n> +\t# First unfold multiline header fields\n> +\twhile (<$fh>) {\n> +\t\tlast if /^\\s*$/;\n> +\t\tif (/^\\s+\\S/ and @header) {\n> +\t\t\tchomp($header[$#header]);\n> +\t\t\ts/^\\s+/ /;\n> +\t\t\t$header[$#header] .= $_;\n> +\t\t} else {\n> +\t\t\tpush(@header, $_);\n> +\t\t}\n> +\t}\n> +\n> +\t# Now unfold the message body\n\nWhy \"unfold\"? Don't you mean \"split message body into a list of lines\"?\n\n> +\twhile (<$fh>) {\n> +\t\tpush @body, $_;\n> +\t}\n> +\n> +\treturn (@header, @body);\n> +}\n\nPlease document your functions. See e.g. perl/Git.pm for an example of\nwhat perldoc allows you to do.\n\nThis also lacks tests. One advantage of having a clean API is that it\nalso makes it simpler to do unit-testing. Grep \"Test::More\" in t/ to see\nsome existing unit-tests in Perl.\n\n> +\tforeach(@_) {\n\nStyle: space before (.\n\n> +\t\tif (defined $input_format && $input_format eq 'mbox') {\n> +\t\t\tif (/^Subject:\\s+(.*)$/i) {\n> +\t\t\t\t$subject = $1;\n> +\t\t\t} elsif (/^From:\\s+(.*)$/i) {\n> +\t\t\t\t$from = $1;\n\nNot sure we need thes if/elsif/ for generic headers. Email::Simple's API\nseems much simpler and general: $email->header(\"From\");\n\n> +\t\t\t\tforeach my $addr (parse_address_line($1)) {\n> +\t\t\t\t\tpush @to, $addr;\n> +\t\t\t\t}\n\n3 lines for an array concatenation in a high-level language. It looks\nlike 2 more than needed ;-).\n\n> +\t\t\t}\n> +\n> +\t\t} else {\n\nUseless blank line.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287752","messageId":"20160528233329.GA1132@dcvr.yhbt.net","threadId":"42466","inReplyTo":"vpqeg8mi4wm.fsf@anie.imag.fr","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-28T23:33:29Z","receivedAt":"2016-05-28T23:33:29Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Matthieu Moy wrote:\n> Samuel GROOT writes:\n> \n> > Parsing and processing in send-email is done in the same loop.\n> >\n> > To make the code more maintainable, we create two subroutines:\n> > - `parse_email` to separate header and body\n> > - `parse_header` to retrieve data from header\n> \n> These routines are not specific to git send-email, nor to Git.\n> \n> Does it make sense to use an external library, like\n> http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,\n> either by depending on it, or by copying it in Git's source tree ?\n\nThat might be overkill and increase installation/maintenance\nburden. Bundling it would probably be problematic to distros,\ntoo.\n\n> If not, I think it would be better to introduce an email parsing library\n> in a dedicated Perl module in perl/ in our source tree, to keep\n> git-send-email.perl more focused on the \"send-email\" logic.\n\nSounds good, Git.pm already has parse_mailboxes\n\n> > +sub parse_email {\n> > +\tmy @header = ();\n> > +\tmy @body = ();\n> > +\tmy $fh = shift;\n> > +\n> > +\t# First unfold multiline header fields\n> > +\twhile (<$fh>) {\n> > +\t\tlast if /^\\s*$/;\n> > +\t\tif (/^\\s+\\S/ and @header) {\n> > +\t\t\tchomp($header[$#header]);\n> > +\t\t\ts/^\\s+/ /;\n> > +\t\t\t$header[$#header] .= $_;\n> > +\t\t} else {\n> > +\t\t\tpush(@header, $_);\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t# Now unfold the message body\n> \n> Why \"unfold\"? Don't you mean \"split message body into a list of lines\"?\n> \n> > +\twhile (<$fh>) {\n> > +\t\tpush @body, $_;\n> > +\t}\n\nI'd rather avoid the loops entirely and do this:\n\n\tlocal $/ = \"\\n\"; # in case caller clobbers $/\n\t@body = (<$fh>);\n\n> > +\treturn (@header, @body);\n> > +}\n\n\n> > +\t\tif (defined $input_format && $input_format eq 'mbox') {\n> > +\t\t\tif (/^Subject:\\s+(.*)$/i) {\n> > +\t\t\t\t$subject = $1;\n> > +\t\t\t} elsif (/^From:\\s+(.*)$/i) {\n> > +\t\t\t\t$from = $1;\n> \n> Not sure we need thes if/elsif/ for generic headers. Email::Simple's API\n> seems much simpler and general: $email->header(\"From\");\n\nRight. Reading this, it would've been easier to parse headers into a\nhash (normalized keys to lowercase) up front inside parse_email.\n"},{"id":"287758","messageId":"8904f487-985c-bd2d-a8d1-4a712c6ef558@grenoble-inp.org","threadId":"42466","inReplyTo":"20160528233329.GA1132@dcvr.yhbt.net","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-29T17:15:10Z","receivedAt":"2016-05-29T17:15:10Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/29/2016 01:33 AM, Eric Wong wrote:\n> Matthieu Moy wrote:\n>> Samuel GROOT writes:\n>>\n>>> Parsing and processing in send-email is done in the same loop.\n>>>\n>>> To make the code more maintainable, we create two subroutines:\n>>> - `parse_email` to separate header and body\n>>> - `parse_header` to retrieve data from header\n>>\n>> These routines are not specific to git send-email, nor to Git.\n>>\n>> Does it make sense to use an external library, like\n>> http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,\n>> either by depending on it, or by copying it in Git's source tree ?\n>\n> That might be overkill and increase installation/maintenance\n> burden. Bundling it would probably be problematic to distros,\n> too.\n\nI have no opinion on that topic, but it could be interesting to have \nother opinions. For the first patch I thought it would be easier and \nquicker to use code already written, and maybe use another method in the \nnext iteration.\n\nEmail::Simple is licensed under Perl's Artistic License or GPL (v1 or \nany later version), so it's fine to bundle it.\n\n>> If not, I think it would be better to introduce an email parsing library\n>> in a dedicated Perl module in perl/ in our source tree, to keep\n>> git-send-email.perl more focused on the \"send-email\" logic.\n>\n> Sounds good, Git.pm already has parse_mailboxes\n\nI agree, I will look into that.\n\n>>> +sub parse_email {\n>>> +\tmy @header = ();\n>>> +\tmy @body = ();\n>>> +\tmy $fh = shift;\n>>> +\n>>> +\t# First unfold multiline header fields\n>>> +\twhile (<$fh>) {\n>>> +\t\tlast if /^\\s*$/;\n>>> +\t\tif (/^\\s+\\S/ and @header) {\n>>> +\t\t\tchomp($header[$#header]);\n>>> +\t\t\ts/^\\s+/ /;\n>>> +\t\t\t$header[$#header] .= $_;\n>>> +\t\t} else {\n>>> +\t\t\tpush(@header, $_);\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\t# Now unfold the message body\n>>\n>> Why \"unfold\"? Don't you mean \"split message body into a list of lines\"?\n>>\n>>> +\twhile (<$fh>) {\n>>> +\t\tpush @body, $_;\n>>> +\t}\n>\n> I'd rather avoid the loops entirely and do this:\n>\n> \tlocal $/ = \"\\n\"; # in case caller clobbers $/\n> \t@body = (<$fh>);\n\nI didn't know this method before, thanks for suggesting it!\n\n>>> +\treturn (@header, @body);\n>>> +}\n>\n>\n>>> +\t\tif (defined $input_format && $input_format eq 'mbox') {\n>>> +\t\t\tif (/^Subject:\\s+(.*)$/i) {\n>>> +\t\t\t\t$subject = $1;\n>>> +\t\t\t} elsif (/^From:\\s+(.*)$/i) {\n>>> +\t\t\t\t$from = $1;\n>>\n>> Not sure we need thes if/elsif/ for generic headers. Email::Simple's API\n>> seems much simpler and general: $email->header(\"From\");\n>\n> Right. Reading this, it would've been easier to parse headers into a\n> hash (normalized keys to lowercase) up front inside parse_email.\n\nSo should we merge parse_email and parse_header in one unique subroutine?\n"},{"id":"287759","messageId":"7423b133-cec4-e633-f1ef-70ccc6a6cc02@grenoble-inp.org","threadId":"42466","inReplyTo":"vpq8tyujkb0.fsf@anie.imag.fr","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-29T17:21:49Z","receivedAt":"2016-05-29T17:21:49Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/28/2016 05:04 PM, Matthieu Moy wrote:\n> Eric Wong writes:\n>\n>> Samuel GROOT wrote:\n>>\n>>> (mbox) Adding cc: A from line 'Cc: A, One'\n>>> (mbox) Adding cc: One from line 'Cc: A, One'\n>>>\n>>> Though `git send-email` now outputs something like:\n>>>\n>>> (mbox) Adding cc: A from line 'Cc: A'\n>>> (mbox) Adding cc: One from line 'Cc: One'\n>> I actually like neither, and would prefer something shorter:\n>>\n>> (mbox) Adding cc: A from Cc: header\n>> (mbox) Adding cc: One from Cc: header\n>> (mbox) Adding cc: SoB from Signed-off-by: trailer\n>>\n>> That way, there's no redundant addresses in each line and less\n>> likely to wrap.\n>\n> I agree with this. Actually, I'd even say that the current output of\n> \"git send-email\" looks sloppy, and internal refactoring may be a good\n> opportunity to get it cleaner.\n\nI agree.\n\nShould we take what Eric suggested (see below) as standard output?\n\n> Since the headers are already shown after those lines, it's\n> redundant to have the entire line. And we could add\n> trailers after the headers (with a blank line to delimit):\n>\n> # existing header output:\n> From: F \n> Cc: A , One \n> Subject: foo\n>\n> # new trailer output\n> Signed-off-by: SoB \n> Acked-by: ack \n\nAnd keep \"(mbox) Adding ...\" lines as error output, or maybe displayable \nby a new option `--verbose`?\n"},{"id":"287761","messageId":"vpqpos4hht5.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"8904f487-985c-bd2d-a8d1-4a712c6ef558@grenoble-inp.org","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-29T17:53:26Z","receivedAt":"2016-05-29T17:53:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Samuel GROOT writes:\n\n> So should we merge parse_email and parse_header in one unique\n> subroutine?\n\nAt least on the user (i.e. caller of the API) side, one function is\nprobably enough.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287762","messageId":"vpq8tyshh8f.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"7423b133-cec4-e633-f1ef-70ccc6a6cc02@grenoble-inp.org","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-29T18:05:52Z","receivedAt":"2016-05-29T18:05:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Samuel GROOT writes:\n\n> Should we take what Eric suggested (see below) as standard output?\n>\n>> Since the headers are already shown after those lines, it's\n>> redundant to have the entire line. And we could add\n>> trailers after the headers (with a blank line to delimit):\n>>\n>> # existing header output:\n>> From: F \n>> Cc: A , One \n>> Subject: foo\n>>\n>> # new trailer output\n>> Signed-off-by: SoB \n>> Acked-by: ack \n\nI don't think adding the trailers in the output is needed. If the\nmessage says\n\n Adding foo@example from Signed-off-by trailer\n\nI can guess that it's from \"Signed-off-by: foo@example\" without having\nit explicitly.\n\nPerhaps others think differently, but for me, the shortest output would\nbe the better (if only to solve the \"I never see these lines, they\nscrolled out of my terminal\" issue).\n\nIdeally, I'd even like to shorten the current output a bit more (the\nX-Mailer: header is useless IMHO, and the Date: and Message-id: headers\nare probably not useful enough to be shown by default).\n\n(Just thinking aloud, obviously none of this should be a prerequisite to\naccept your refactoring patch)\n\n> And keep \"(mbox) Adding ...\" lines as error output, or maybe\n> displayable by a new option `--verbose`?\n\nI think the \"Adding ...\" lines make sense by default at least for\nbeginners (just a few days ago, I received a bunch of test emails by\nyour team follow by a \"Oops, I just noticed you got Cc-ed in my tests\"\nmessage ;-), that would probably have been worse without the message).\nThere could be an advice.* option to deactivate them, though.\n\nThe (mbox) prefix doesn't seem useful to me OTOH, I think it's a\nleftover debugging message.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287815","messageId":"982672c0-0642-06a2-46dd-8c538568f4bc@grenoble-inp.org","threadId":"42466","inReplyTo":"vpqpos4hht5.fsf@anie.imag.fr","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-30T13:28:08Z","receivedAt":"2016-05-30T13:28:08Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/29/2016 07:53 PM, Matthieu Moy wrote:\n> Samuel GROOT writes:\n>\n>> So should we merge parse_email and parse_header in one unique\n>> subroutine?\n>\n> At least on the user (i.e. caller of the API) side, one function is\n> probably enough.\n>\n\nI will do that, as soon as we decide what's best between including \nEmail::Simple library or making our own API.\n"},{"id":"287817","messageId":"0abbc11b-5741-ee2e-292c-245c2b974f8e@grenoble-inp.org","threadId":"42466","inReplyTo":"vpq8tyshh8f.fsf@anie.imag.fr","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-30T14:01:29Z","receivedAt":"2016-05-30T14:01:29Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/29/2016 08:05 PM, Matthieu Moy wrote:\n> Samuel GROOT writes:\n>\n>> Should we take what Eric suggested (see below) as standard output?\n>>\n>>> Since the headers are already shown after those lines, it's\n>>> redundant to have the entire line. And we could add\n>>> trailers after the headers (with a blank line to delimit):\n>>>\n>>> # existing header output:\n>>> From: F \n>>> Cc: A , One \n>>> Subject: foo\n>>>\n>>> # new trailer output\n>>> Signed-off-by: SoB \n>>> Acked-by: ack \n>\n> I don't think adding the trailers in the output is needed. If the\n> message says\n>\n> Adding foo@example from Signed-off-by trailer\n>\n> I can guess that it's from \"Signed-off-by: foo@example\" without having\n> it explicitly.\n>\n> Perhaps others think differently, but for me, the shortest output would\n> be the better (if only to solve the \"I never see these lines, they\n> scrolled out of my terminal\" issue).\n\nI agree, the shorter the better.\n\n> Ideally, I'd even like to shorten the current output a bit more (the\n> X-Mailer: header is useless IMHO, and the Date: and Message-id: headers\n> are probably not useful enough to be shown by default).\n\nAgreed.\n\n> (Just thinking aloud, obviously none of this should be a prerequisite to\n> accept your refactoring patch)\n>\n>> And keep \"(mbox) Adding ...\" lines as error output, or maybe\n>> displayable by a new option `--verbose`?\n>\n> I think the \"Adding ...\" lines make sense by default at least for\n> beginners (just a few days ago, I received a bunch of test emails by\n> your team follow by a \"Oops, I just noticed you got Cc-ed in my tests\"\n> message ;-), that would probably have been worse without the message).\n> There could be an advice.* option to deactivate them, though.\n\nAn advice.* option seems a good solution to me.\n\n> The (mbox) prefix doesn't seem useful to me OTOH, I think it's a\n> leftover debugging message.\n\n(mbox) prefix was introduced by Ryan Anderson in 2005 (can't find the \nexact commit though), in opposition with the (non-mbox) format (\"lots of \nemail\") that was used before.\n\nIs the \"lots of email\" format still used?\n\nWhen adding Cc: from a Signed-off-by: field, (body) prefix is used.\n"},{"id":"287821","messageId":"vpqfuszaapu.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"0abbc11b-5741-ee2e-292c-245c2b974f8e@grenoble-inp.org","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-30T14:20:45Z","receivedAt":"2016-05-30T14:20:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Samuel GROOT writes:\n\n> (mbox) prefix was introduced by Ryan Anderson in 2005 (can't find the\n> exact commit though), in opposition with the (non-mbox) format (\"lots\n> of email\") that was used before.\n\nThat is actually from the original commit introducing send-email:\n83b2443 ([PATCH] Add git-send-email-script - tool to send emails from\ngit-format-patch-script, 2005-07-31), i.e. ~3 month after Git was born.\n\nAt that time, user-friendlyness was not really a priority ;-).\n\n> Is the \"lots of email\" format still used?\n\nAFAICT, it was initially supported for backward compatibility, and then\nno one removed it, but I wouldn't be surprised if no one actually used\nit.\n\nI vaguely remember a message from Ryan Anderson being surprised to see\nthe old format still supported, but I can't find it in the archives.\n\nIn any case:\n\n- git log --grep 'lots of email' => shows only 83b2443\n- git log -S'lots of email' => likewise\n- git grep 'lots of email' => just one answer in a comment\n\nI'm not sure the feature is even tested.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287842","messageId":"f80f8c1b-6a69-1356-d300-82b7c5af5821@grenoble-inp.org","threadId":"42466","inReplyTo":"vpqfuszaapu.fsf@anie.imag.fr","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-05-30T18:28:01Z","receivedAt":"2016-05-30T18:28:01Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/30/2016 04:20 PM, Matthieu Moy wrote:\n>> Is the \"lots of email\" format still used?\n>\n> AFAICT, it was initially supported for backward compatibility, and then\n> no one removed it, but I wouldn't be surprised if no one actually used\n> it.\n>\n> I vaguely remember a message from Ryan Anderson being surprised to see\n> the old format still supported, but I can't find it in the archives.\n>\n> In any case:\n>\n> - git log --grep 'lots of email' => shows only 83b2443\n> - git log -S'lots of email' => likewise\n> - git grep 'lots of email' => just one answer in a comment\n>\n> I'm not sure the feature is even tested.\n\n`grep \"non-mbox\" t/t9001-send-email.sh` didn't return anything, \napparently it's not tested.\n\nCan we consider this feature obsolete and remove it?\n"},{"id":"287850","messageId":"vpqinxv4a58.fsf@anie.imag.fr","threadId":"42466","inReplyTo":"f80f8c1b-6a69-1356-d300-82b7c5af5821@grenoble-inp.org","subject":"Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-30T19:29:39Z","receivedAt":"2016-05-30T19:29:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Samuel GROOT writes:\n\n> Can we consider this feature obsolete and remove it?\n\nWe're usually quite conservative with backward compatibility. If we\nremove the feature, we may want to announce it in the next feature\nrelease and actually remove it in the one after (unless we get valid\nobjection in the meantime).\n\nI'm all for dropping a feature that no one uses if it turns out to be\nthe case.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288137","messageId":"9535d962-5479-5a13-472e-cd558ef163e0@grenoble-inp.org","threadId":"42466","inReplyTo":"20160528233329.GA1132@dcvr.yhbt.net","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Samuel GROOT","fromEmail":"samuel.groot@grenoble-inp.org","sentAt":"2016-06-02T16:57:34Z","receivedAt":"2016-06-02T16:57:34Z","isPatch":true,"sender":{"key":"samuel.groot@grenoble-inp.org","avatar":null},"body":"On 05/29/2016 01:33 AM, Eric Wong wrote:\n> Matthieu Moy wrote:\n>> Samuel GROOT writes:\n>>\n>>> Parsing and processing in send-email is done in the same loop.\n>>>\n>>> To make the code more maintainable, we create two subroutines:\n>>> - `parse_email` to separate header and body\n>>> - `parse_header` to retrieve data from header\n>>\n>> These routines are not specific to git send-email, nor to Git.\n>>\n>> Does it make sense to use an external library, like\n>> http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,\n>> either by depending on it, or by copying it in Git's source tree ?\n>\n> That might be overkill and increase installation/maintenance\n> burden. Bundling it would probably be problematic to distros,\n> too.\n\nWe have 5 solutions here:\n\n 1. Make a new dependence to Email::Simple.\n\n 2. Bundle Email::Simple in Git's source tree.\n\n 3. Use Email::Simple if installed, else use our library.\n\n 4. Making our own email parser library.\n\n 5. Duplicate parser loop as we did for our patch to implement\n `--quote-email` as proposed in $gmane/295772 .\n\nObviously, option (5) is the easiest one for us, but it leaves \nrefactoring for later, and option (1) is also easier but adds a new \ndependence which is not that good.\n\nSince our project ends next week, we might not have enough time to \nfinish developing a custom parser API so (4) is not a viable option for \nnow but could be done in the future.\n\nWe could consider bundling Email::Simple as the best option, as it's \ndeveloped since 2003 and might be safer to use than anything we could \nwrite in several weeks.\n"},{"id":"288153","messageId":"20160602195855.GA17627@dcvr.yhbt.net","threadId":"42466","inReplyTo":"9535d962-5479-5a13-472e-cd558ef163e0@grenoble-inp.org","subject":"Re: [WIP-PATCH 1/2] send-email: create email parser subroutine","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-06-02T19:58:55Z","receivedAt":"2016-06-02T19:58:55Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Samuel GROOT wrote:\n> On 05/29/2016 01:33 AM, Eric Wong wrote:\n> >Matthieu Moy wrote:\n> >>Samuel GROOT writes:\n> >>\n> >>>Parsing and processing in send-email is done in the same loop.\n> >>>\n> >>>To make the code more maintainable, we create two subroutines:\n> >>>- `parse_email` to separate header and body\n> >>>- `parse_header` to retrieve data from header\n> >>\n> >>These routines are not specific to git send-email, nor to Git.\n> >>\n> >>Does it make sense to use an external library, like\n> >>http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,\n> >>either by depending on it, or by copying it in Git's source tree ?\n> >\n> >That might be overkill and increase installation/maintenance\n> >burden. Bundling it would probably be problematic to distros,\n> >too.\n> \n> We have 5 solutions here:\n> \n> 1. Make a new dependence to Email::Simple.\n> \n> 2. Bundle Email::Simple in Git's source tree.\n> \n> 3. Use Email::Simple if installed, else use our library.\n> \n> 4. Making our own email parser library.\n> \n> 5. Duplicate parser loop as we did for our patch to implement\n> `--quote-email` as proposed in $gmane/295772 .\n> \n> Obviously, option (5) is the easiest one for us, but it leaves refactoring\n> for later, and option (1) is also easier but adds a new dependence which is\n> not that good.\n\nI would go with (5) for now and leave (4) for later (which\nmight just be moving the function to a new file).\n\n> Since our project ends next week, we might not have enough time to finish\n> developing a custom parser API so (4) is not a viable option for now but\n> could be done in the future.\n> \n> We could consider bundling Email::Simple as the best option, as it's\n> developed since 2003 and might be safer to use than anything we could write\n> in several weeks.\n\nIn an ideal world, (1) would be nice. But (IMHO) git-send-email\nshould remain installable on non-ideal systems which do not\nprovide Email::Simple as a package.\n\n(2) would probably be non-ideal for distro maintainers\n(+Cc: Jonathan for opinions), and (3) is the most complex\nand difficult-to-support.\n"}]}