Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

On 06/09/2016 08:01 AM, Matthieu Moy wrote:

Samuel GROOT  writes:


On 06/08/2016 06:09 PM, Junio C Hamano wrote:

Samuel GROOT  writes:


Actually we had issues when trying to refactor send-email's email
parsing loop [1]. Email addresses in output file `commandeline1` in
tests weren't sorted the same way as the reference file it was
compared to. E.g.:

  !nob...@example.com!
  !aut...@example.com!
  !o...@example.com!
  !t...@example.com!


And the reason why these addresses that are collected from the same
input (i.e. command line, existing e-mail fields, footers, etc.) are
shown in different order in your implementation is...?


It's not shown in different order in our implementation, it's just a
leftover of my refactor attempt [1].


I think the refactoring makes sense, but having this patch as PATCH 1/6
in a series about --in-reply-to confuses reviewers: they would expect
this patch to be useful to the others in the series.

If you have "reply to a message in a file" ready without the
refactoring, and a mostly ready refactoring, then I think it makes sense
to have two patch series, the first being only "reply to a message in a
file". If the refactoring itself is not ready, you may send a separate
series "tests clean up" and explain on the cover-letter that it's, well,
only a test clean up.


I think I will split the patch series into 3 smaller series:
- "quote-email" feature
- tests clean up
- send-email code cleanup (including send-email's output)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Tom Russello]

On 06/09/16 07:51, Matthieu Moy wrote:
> Junio C Hamano  writes:
> 
>> Tom Russello  writes:
>>
>>> +# Check if two files have the same content, non-order sensitive
>>> +test_cmp_noorder () {
>>> +   sort $1 >$1;
>>
>> Here is what I think happens:
>>
>> 0) the shell parses this command line;
>> 1) the shell notices that the output has to go to $1;
>> 2) the shell does open(2) of $1,
>> 3) the shell spawns "sort" with a single argument, with its
>>output connected to the file descriptor obtained in 2).
>>
>> Because "$1" becomes an empty file at 2), "sort" reads nothing and
>> writes nothing.
> 
> Tom: in case you're not convinced, try this:
> 
> $ (echo b; echo a) >f
> $ sort f
> a
> b
> $ sort f >f
> $ cat f
> $
> 
> Also, useless ';' and missing double-quotes around "$1" to avoid bad
> surprises ifever test_cmp_noorder is called on file names with special
> characters.

I was totally convinced by Junio's explanation, it is partially fixed in
v4 and will be entirely fixed in v5.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Matthieu Moy]

Samuel GROOT  writes:

> On 06/08/2016 06:09 PM, Junio C Hamano wrote:
>> Samuel GROOT  writes:
>>
>>> Actually we had issues when trying to refactor send-email's email
>>> parsing loop [1]. Email addresses in output file `commandeline1` in
>>> tests weren't sorted the same way as the reference file it was
>>> compared to. E.g.:
>>>
>>>   !nob...@example.com!
>>>   !aut...@example.com!
>>>   !o...@example.com!
>>>   !t...@example.com!
>>
>> And the reason why these addresses that are collected from the same
>> input (i.e. command line, existing e-mail fields, footers, etc.) are
>> shown in different order in your implementation is...?
>
> It's not shown in different order in our implementation, it's just a
> leftover of my refactor attempt [1].

I think the refactoring makes sense, but having this patch as PATCH 1/6
in a series about --in-reply-to confuses reviewers: they would expect
this patch to be useful to the others in the series.

If you have "reply to a message in a file" ready without the
refactoring, and a mostly ready refactoring, then I think it makes sense
to have two patch series, the first being only "reply to a message in a
file". If the refactoring itself is not ready, you may send a separate
series "tests clean up" and explain on the cover-letter that it's, well,
only a test clean up.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Matthieu Moy]

Junio C Hamano  writes:

> Tom Russello  writes:
>
>> +# Check if two files have the same content, non-order sensitive
>> +test_cmp_noorder () {
>> +sort $1 >$1;
>
> Here is what I think happens:
>
> 0) the shell parses this command line;
> 1) the shell notices that the output has to go to $1;
> 2) the shell does open(2) of $1,
> 3) the shell spawns "sort" with a single argument, with its
>output connected to the file descriptor obtained in 2).
>
> Because "$1" becomes an empty file at 2), "sort" reads nothing and
> writes nothing.

Tom: in case you're not convinced, try this:

$ (echo b; echo a) >f
$ sort f
a
b
$ sort f >f
$ cat f
$

Also, useless ';' and missing double-quotes around "$1" to avoid bad
surprises ifever test_cmp_noorder is called on file names with special
characters.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

On 06/08/2016 06:09 PM, Junio C Hamano wrote:

Samuel GROOT  writes:


Actually we had issues when trying to refactor send-email's email
parsing loop [1]. Email addresses in output file `commandeline1` in
tests weren't sorted the same way as the reference file it was
compared to. E.g.:

  !nob...@example.com!
  !aut...@example.com!
  !o...@example.com!
  !t...@example.com!


And the reason why these addresses that are collected from the same
input (i.e. command line, existing e-mail fields, footers, etc.) are
shown in different order in your implementation is...?


It's not shown in different order in our implementation, it's just a 
leftover of my refactor attempt [1].


Maybe it's a bad idea to increase tests' complexity, but IMHO tests 
should be independent to the addresses' order.


Plus, it would help refactor in the future, the data being processed 
differently: parsing and processing in different subroutines rather than 
doing everything in one gigantic loop.


We can drop it if necessary, but it may be useful to make 
git-send-email.perl easier to maintain.


[1] * http://article.gmane.org/gmane.comp.version-control.git/295753
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

Samuel GROOT  writes:

> Actually we had issues when trying to refactor send-email's email
> parsing loop [1]. Email addresses in output file `commandeline1` in
> tests weren't sorted the same way as the reference file it was
> compared to. E.g.:
>
>   !nob...@example.com!
>   !aut...@example.com!
>   !o...@example.com!
>   !t...@example.com!

And the reason why these addresses that are collected from the same
input (i.e. command line, existing e-mail fields, footers, etc.) are
shown in different order in your implementation is...?


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Samuel GROOT]

On 06/08/2016 03:07 AM, Junio C Hamano wrote:

I am having a hard time guessing what prompted you to sort the
output, i.e. what problem you were trying to solve.  It cannot be
because addresses on a list (e.g. Cc:) could come out in an
indeterministic order, because the address that a test expects to be
the first (c...@example.com in the above example) may not appear as
the first one, but in the textual output it _is_ shown differently
from the remainder (i.e. even if you sort, from "Cc:
c...@example.com," it is clear it was the first one output for Cc: and
diferent from "A ".


Actually we had issues when trying to refactor send-email's email 
parsing loop [1]. Email addresses in output file `commandeline1` in 
tests weren't sorted the same way as the reference file it was compared 
to. E.g.:


  !nob...@example.com!
  !aut...@example.com!
  !o...@example.com!
  !t...@example.com!

I agree replacing test_cmp with test_cmp_noorder is pointless, I will 
fix it and re-roll.


Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] t9001: non order-sensitive file comparison

[Junio C Hamano]

Tom Russello  writes:

> +# Check if two files have the same content, non-order sensitive
> +test_cmp_noorder () {
> + sort $1 >$1;

Here is what I think happens:

0) the shell parses this command line;
1) the shell notices that the output has to go to $1;
2) the shell does open(2) of $1,
3) the shell spawns "sort" with a single argument, with its
   output connected to the file descriptor obtained in 2).

Because "$1" becomes an empty file at 2), "sort" reads nothing and
writes nothing.

> + sort $2 >$2;
> + return $(test_cmp $1 $2)

What is this return doing?  I would understand if it were just

test_cmp $1 $2

Of course, all the places you use test_cmp_noorder are happy when
this function returns 0/success, and because $1 and $2 at this point
are both empty files and test_cmp will not say anything to its
standard output, the return will just yield 0/success to the caller
of the function, so it is likely that with this patch t9001 would
have passed for you, but that is not necessarily a good thing X-<.

> @@ -269,7 +276,7 @@ test_expect_success $PREREQ 'Show all headers' '
>   -e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
>   -e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
>   >actual-show-all-headers &&
> - test_cmp expected-show-all-headers actual-show-all-headers
> + test_cmp_noorder expected-show-all-headers actual-show-all-headers
>  '

It is dubious that it is a good idea to blindly sort two files and
compare, especially because expected-show-all-headers is actually
something like this:

cat >expected-show-all-headers <<\EOF
0001-Second.patch
(mbox) Adding cc: A  from line 'From: A 
'
(mbox) Adding cc: One  from line 'Cc: One 
, t...@example.com'
(mbox) Adding cc: t...@example.com from line 'Cc: One , 
t...@example.com'
Dry-OK. Log says:
Server: relay.example.com
MAIL FROM:
RCPT TO:
RCPT TO:
...
To: t...@example.com
Cc: c...@example.com,
A ,
One ,
t...@example.com
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
Message-Id: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
In-Reply-To: 
References: 

Result: OK
EOF

We do want to see MAIL FROM: as the first thing we give to the
server, followed by RCPT TO:, followed by the headers.

I am having a hard time guessing what prompted you to sort the
output, i.e. what problem you were trying to solve.  It cannot be
because addresses on a list (e.g. Cc:) could come out in an
indeterministic order, because the address that a test expects to be
the first (c...@example.com in the above example) may not appear as
the first one, but in the textual output it _is_ shown differently
from the remainder (i.e. even if you sort, from "Cc:
c...@example.com," it is clear it was the first one output for Cc: and
diferent from "A ".

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/6] t9001: non order-sensitive file comparison

[Tom Russello]

Tests might fail if lines compared in text files don't have the same order.

Signed-off-by: Samuel GROOT 
Signed-off-by: Tom RUSSELLO 
Signed-off-by: Matthieu MOY 
---
 t/t9001-send-email.sh | 61 ---
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index b3355d2..4558e0f 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -54,6 +54,13 @@ test_no_confirm () {
>no_confirm_okay
 }
 
+# Check if two files have the same content, non-order sensitive
+test_cmp_noorder () {
+   sort $1 >$1;
+   sort $2 >$2;
+   return $(test_cmp $1 $2)
+}
+
 # Exit immediately to prevent hang if a no-confirm test fails
 check_no_confirm () {
if ! test -f no_confirm_okay
@@ -97,7 +104,7 @@ test_expect_success $PREREQ 'setup expect' '
 '
 
 test_expect_success $PREREQ 'Verify commandline' '
-   test_cmp expected commandline1
+   test_cmp_noorder expected commandline1
 '
 
 test_expect_success $PREREQ 'Send patches with --envelope-sender' '
@@ -117,7 +124,7 @@ test_expect_success $PREREQ 'setup expect' '
 '
 
 test_expect_success $PREREQ 'Verify commandline' '
-   test_cmp expected commandline1
+   test_cmp_noorder expected commandline1
 '
 
 test_expect_success $PREREQ 'Send patches with --envelope-sender=auto' '
@@ -137,7 +144,7 @@ test_expect_success $PREREQ 'setup expect' '
 '
 
 test_expect_success $PREREQ 'Verify commandline' '
-   test_cmp expected commandline1
+   test_cmp_noorder expected commandline1
 '
 
 test_expect_success $PREREQ 'setup expect' "
@@ -196,7 +203,7 @@ test_suppress_self () {
>"expected-no-cc-$3" &&
 
(grep '^Cc:' msghdr1-$3 >"actual-no-cc-$3";
-test_cmp expected-no-cc-$3 actual-no-cc-$3)
+test_cmp_noorder expected-no-cc-$3 actual-no-cc-$3)
 }
 
 test_suppress_self_unquoted () {
@@ -269,7 +276,7 @@ test_expect_success $PREREQ 'Show all headers' '
-e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/" \
>actual-show-all-headers &&
-   test_cmp expected-show-all-headers actual-show-all-headers
+   test_cmp_noorder expected-show-all-headers actual-show-all-headers
 '
 
 test_expect_success $PREREQ 'Prompting works' '
@@ -436,13 +443,13 @@ test_expect_success $PREREQ 'In-Reply-To without 
--chain-reply-to' '
2>errors &&
# The first message is a reply to --in-reply-to
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
-   test_cmp expect actual &&
+   test_cmp_noorder expect actual &&
# Second and subsequent messages are replies to the first one
sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
-   test_cmp expect actual &&
+   test_cmp_noorder expect actual &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
-   test_cmp expect actual
+   test_cmp_noorder expect actual
 '
 
 test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '
@@ -457,13 +464,13 @@ test_expect_success $PREREQ 'In-Reply-To with 
--chain-reply-to' '
$patches $patches $patches \
2>errors &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
-   test_cmp expect actual &&
+   test_cmp_noorder expect actual &&
sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
-   test_cmp expect actual &&
+   test_cmp_noorder expect actual &&
sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt2 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
-   test_cmp expect actual
+   test_cmp_noorder expect actual
 '
 
 test_expect_success $PREREQ 'setup fake editor' '
@@ -537,7 +544,7 @@ test_suppression () {
--smtp-server relay.example.com \
$patches | replace_variable_fields \
>actual-suppress-$1${2+"-$2"} &&
-   test_cmp expected-suppress-$1${2+"-$2"} actual-suppress-$1${2+"-$2"}
+   test_cmp_noorder expected-suppress-$1${2+"-$2"} 
actual-suppress-$1${2+"-$2"}
 }
 
 test_expect_success $PREREQ 'sendemail.cc set' '
@@ -1213,7 +1220,7 @@ test_expect_success $PREREQ 'ASCII subject is not RFC2047 
quoted' '
--8bit-encoding=UTF-8 \
email-using-8bit >stdout &&
grep "Subject" msgtxt1 >actual &&
-   test_cmp expected actual
+   test_cmp_noorder expected actual
 '
 
 test_expect_success $PREREQ 'setup expect' '
@@ -1234,7 +1241,7 @@ test_expect_success $PREREQ 'asks about and fixes 8bit 
encodings' '
grep email-using-8bit stdout &&
grep "Which 8bit encoding" stdout &&